Skip to content

civiglific Settings usage - #1526

Open
tarunnjoshi wants to merge 9 commits into
devfrom
civiglific-settings-usage
Open

civiglific Settings usage#1526
tarunnjoshi wants to merge 9 commits into
devfrom
civiglific-settings-usage

Conversation

@tarunnjoshi

@tarunnjoshi tarunnjoshi commented Dec 27, 2025

Copy link
Copy Markdown
Member

civiglific Settings usage

Summary by CodeRabbit

  • New Features

    • Added a Settings UI group for Civiglific with configurable API base URL, credentials, template IDs and PDF storage paths.
  • Behavioral Changes

    • Runtime token generation and API calls now read values from the CiviCRM settings with improved error logging.
    • Updated integration mixins for settings and admin interfaces.
  • Documentation

    • README rewritten for UI-based setup and clearer configuration steps for Glific integration and receipts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Dec 27, 2025

Copy link
Copy Markdown

Walkthrough

Moves Civiglific configuration from hard-coded constants to CiviCRM settings: adds settings definitions and mixins, updates GlificHelper, GlificClient, GroupMapping, and MonetaryReceiptService to read via \Civi::settings(), and updates README to document UI-based setup. (50 words)

Changes

Cohort / File(s) Summary
Settings files
wp-content/civi-extensions/civiglific/settings/civiglific.php, wp-content/civi-extensions/civiglific/settings/civiglific.setting.php
New settings group and individual setting definitions (API base URL, phone, password, template IDs, PDF paths, flags, defaults) for the CiviCRM admin UI.
Manifest / mixins
wp-content/civi-extensions/civiglific/info.xml
Removed setting-php@1.0.0; added setting-php@1 and setting-admin@1 mixins to register settings and admin UI integration.
Glific helper & client
wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificHelper.php, wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificClient.php
Replaced constants with \Civi::settings()->get('...') for API base URL, phone, and password. getToken() now builds request from settings and unifies exception handling. Attention: settings access mixed into HTTP logic; minimal validation and no explicit fallback for missing values.
Admin page calling API
wp-content/civi-extensions/civiglific/CRM/Civiglific/Page/GroupMapping.php
Switched API base URL source to \Civi::settings()->get('civiglific_api_base_url') for group fetches; request/response handling unchanged. Check for null/invalid settings before network calls.
Receipt service
wp-content/civi-extensions/civiglific/Civi/MonetaryReceiptService.php
Template IDs and PDF paths now read from settings (civiglific_* keys) instead of constants; path assembly uses settings values. Verify sanitization, existence, and permissions for configured paths.
Docs
wp-content/civi-extensions/civiglific/README.md
Updated installation and setup instructions to describe UI-based configuration and where to enter Glific credentials, template IDs, and PDF/path values.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Admin as "Admin (UI)"
  participant Settings as "Civi::settings()"
  participant Helper as "GlificHelper"
  participant Caller as "Local Caller" 
  participant Glific as "Glific API"
  Note over Admin,Settings: Admin stores credentials and templates via UI
  Admin->>Settings: set civiglific_api_base_url, civiglific_phone, civiglific_password, templates, paths
  Caller->>Helper: request token / perform API action
  Helper->>Settings: get civiglific_api_base_url, civiglific_phone, civiglific_password
  Helper->>Glific: POST /api/v1/session {phone,password}
  Glific-->>Helper: {access_token} or {error}
  alt access_token returned
    Helper-->>Caller: access_token
  else error or missing token
    Helper-->>Caller: FALSE (logs error)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

status : ready for review

Suggested reviewers

  • pokhiii

Poem

📦 Constants fade, settings sing,
Tokens fetched from UI's spring,
Mixins added, docs align,
Paths and templates now in line,
Logs keep watch while calls take wing ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'civiglific Settings usage' directly reflects the main change: migrating the extension from hard-coded constants to CiviCRM settings-based configuration across multiple files.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch civiglific-settings-usage

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9322094 and ae94863.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/civiglific/info.xml
🚧 Files skipped from review as they are similar to previous changes (1)
  • wp-content/civi-extensions/civiglific/info.xml

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
wp-content/civi-extensions/civiglific/settings/civiglific.setting.php (3)

32-42: Add URL format validation for API base URL.

The API base URL setting lacks validation to ensure it's a valid URL format. Consider adding validation to prevent configuration errors that could cause runtime failures in GlificHelper.

Additionally, since the default URL is organization-specific (api.goonj.glific.com), consider adding a description note indicating this should be updated for different Glific instances.


44-60: Add descriptions for template ID settings.

Both template ID settings lack descriptions explaining their purpose and when each should be used. This makes it difficult for administrators to configure these settings correctly.

🔎 Suggested improvements
   'civiglific_template_id_default' => [
     'name' => 'civiglific_template_id_default',
     'type' => 'Integer',
     'html_type' => 'text',
     'title' => ts('Default Template ID'),
+    'description' => ts('Template ID used for standard Glific message communications'),
     'is_domain' => 1,
     'settings_pages' => ['civiglific' => ['weight' => 40]],
   ],

   'civiglific_template_id_team5000' => [
     'name' => 'civiglific_template_id_team5000',
     'type' => 'Integer',
     'html_type' => 'text',
     'title' => ts('Team 5000 Template ID'),
+    'description' => ts('Template ID used specifically for Team 5000 related communications'),
     'is_domain' => 1,
     'settings_pages' => ['civiglific' => ['weight' => 50]],
   ],

62-84: Environment-specific PDF path defaults may not be portable.

The default PDF paths assume a specific WordPress directory structure that may not exist in all installations. If these directories don't exist or lack write permissions, PDF generation will fail silently.

Consider adding validation to check path existence and permissions, or provide clearer documentation about required directory setup.

wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificHelper.php (2)

57-63: Consider more specific exception handling.

The broad \Exception catch might mask specific failures like network timeouts, invalid JSON responses, or HTTP client configuration errors. While the current logging is adequate, consider catching specific exceptions (e.g., GuzzleException, \JsonException) for more precise error handling and potential retry logic.


36-42: Add timeout and consider retry logic for HTTP requests.

The HTTP client lacks timeout configuration, potentially causing requests to hang indefinitely and blocking PHP execution. For production reliability, add appropriate timeouts and consider retry logic for transient failures.

🔎 Suggested improvements
       try {
         $client = new Client();
         $response = $client->post($url, [
           'headers' => ['Content-Type' => 'application/json'],
           'json' => $data,
           'http_errors' => FALSE,
+          'timeout' => 10,
+          'connect_timeout' => 5,
         ]);
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf6cc54 and 4861d73.

📒 Files selected for processing (4)
  • wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificHelper.php
  • wp-content/civi-extensions/civiglific/info.xml
  • wp-content/civi-extensions/civiglific/settings/civiglific.php
  • wp-content/civi-extensions/civiglific/settings/civiglific.setting.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tarunnjoshi
Repo: ColoredCow/goonj PR: 1230
File: wp-content/civi-extensions/civirazorpay/cli/update-recurring-contact-info.php:115-117
Timestamp: 2025-05-27T07:31:44.131Z
Learning: For one-time scripts in the Goonj project, tarunnjoshi prefers to keep hardcoded values rather than creating shared constants or configuration files, prioritizing simplicity over strict adherence to DRY principle to avoid over-engineering temporary code.
🧬 Code graph analysis (1)
wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificHelper.php (1)
wp-content/civi-extensions/civicalendar-3.5.5/CRM/Calendar/Settings.php (1)
  • get (95-113)
🔇 Additional comments (1)
wp-content/civi-extensions/civiglific/settings/civiglific.php (1)

1-13: LGTM!

The settings group definition follows CiviCRM conventions and is properly structured.

Comment on lines +23 to +25
$apiBaseUrl = \Civi::settings()->get('civiglific_api_base_url');
$phone = \Civi::settings()->get('civiglific_phone');
$password = \Civi::settings()->get('civiglific_password');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate settings exist and are non-empty before use.

The code retrieves settings without checking if they're configured. If any setting is NULL or empty (e.g., on first-time installation), subsequent operations will fail with unclear error messages. This degrades the user experience and makes debugging difficult.

🔎 Proposed validation logic
   public static function getToken() {
     $apiBaseUrl = \Civi::settings()->get('civiglific_api_base_url');
     $phone = \Civi::settings()->get('civiglific_phone');
     $password = \Civi::settings()->get('civiglific_password');
+
+    // Validate required settings are configured
+    if (empty($apiBaseUrl) || empty($phone) || empty($password)) {
+      \Civi::log()->error('Glific configuration error:', [
+        'error' => 'Required Glific settings are not configured. Please configure API credentials in CiviCRM settings.',
+        'missing' => [
+          'apiBaseUrl' => empty($apiBaseUrl),
+          'phone' => empty($phone),
+          'password' => empty($password),
+        ],
+      ]);
+      return FALSE;
+    }

     $url = rtrim($apiBaseUrl, '/') . '/api/v1/session';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$apiBaseUrl = \Civi::settings()->get('civiglific_api_base_url');
$phone = \Civi::settings()->get('civiglific_phone');
$password = \Civi::settings()->get('civiglific_password');
$apiBaseUrl = \Civi::settings()->get('civiglific_api_base_url');
$phone = \Civi::settings()->get('civiglific_phone');
$password = \Civi::settings()->get('civiglific_password');
// Validate required settings are configured
if (empty($apiBaseUrl) || empty($phone) || empty($password)) {
\Civi::log()->error('Glific configuration error:', [
'error' => 'Required Glific settings are not configured. Please configure API credentials in CiviCRM settings.',
'missing' => [
'apiBaseUrl' => empty($apiBaseUrl),
'phone' => empty($phone),
'password' => empty($password),
],
]);
return FALSE;
}
🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificHelper.php around
lines 23–25, the three settings ($apiBaseUrl, $phone, $password) are fetched
without validation; update the code to trim and verify each value is set and
non-empty before use, and if any are missing/empty, return early or throw a
clear exception (or log an explicit error) that names the missing setting(s) so
callers get a helpful message instead of failing later; ensure validation is
centralized (e.g., a small helper or guard block) and avoid proceeding with API
calls when validation fails.

<mixin>smarty@1.0.3</mixin>
<mixin>scan-classes@1.0.0</mixin>
<mixin>entity-types-php@2.0.0</mixin>
<mixin>setting-php@1</mixin>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Remove duplicate setting-php mixin declaration.

Line 42 already declares setting-php@1.0.0, making this declaration redundant. Having both version-specific and unversioned mixins may cause conflicts or unpredictable behavior in CiviCRM's mixin resolution.

🔎 Proposed fix
-    <mixin>setting-php@1</mixin>
     <mixin>setting-admin@1</mixin>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<mixin>setting-php@1</mixin>
<mixin>setting-admin@1</mixin>
🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/info.xml around line 46, remove the
redundant <mixin>setting-php@1</mixin> entry because line 42 already declares
setting-php@1.0.0; keep the versioned mixin (setting-php@1.0.0) and delete the
unversioned duplicate to avoid mixin resolution conflicts.

Comment on lines +21 to +30
'civiglific_password' => [
'name' => 'civiglific_password',
'type' => 'String',
'html_type' => 'password',
'title' => ts('Glific Password'),
'description' => ts('Glific API password'),
'is_domain' => 1,
'is_contact' => 0,
'settings_pages' => ['civiglific' => ['weight' => 20]],
],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

CiviCRM settings encryption API credentials storage best practices

💡 Result:

Short best-practices for storing API credentials and other secrets used by CiviCRM

  • Use CiviCRM’s built-in encrypted-credentials feature (CIVICRM_CRED_KEYS) rather than storing plaintext in DB or settings. Put keys in civicrm.settings.php as documented. [1]

  • Follow CiviCRM’s cipher/key format and sizes (use AES-256 variants; prefer hkdf-sha256-encoded strong random keys). Configure multiple keys if you need rotation/roll‑forward. [1][2]

  • Rotate keys properly: add the new key first in CIVICRM_CRED_KEYS, run System.rotateKey (cv api4 System.rotateKey tag=CRED), then remove old keys after a grace period. Test rotation on staging first. [1]

  • Keep the encryption keys out of source control and limit filesystem access to civicrm.settings.php (tight file permissions, separate deploy/build artifacts from runtime secrets). [1][2]

  • Prefer a dedicated secret store (HashiCorp Vault, AWS Secrets Manager, Azure Key Vault, or an HSM) for managing high-value keys or for automated rotation; retrieve into the host environment at startup and inject into civicrm.settings.php or environment variables securely. Audit and RBAC-enable access. [3][4]

  • Minimize exposure: give each API credential the least privileges required, use per-integrator keys where possible (API‑key extension / contact-level keys), and avoid sharing long‑lived credentials between systems. [2][5]

  • Log and monitor access to secrets, keep secure backups of keys (offline/encrypted), and have an incident/revocation plan so you can quickly revoke and rekey if compromised. [3][4]

References

  1. CiviCRM System Administrator Guide — Secret keys (CIVICRM_CRED_KEYS, CIVICRM_SIGN_KEYS, CIVICRM_SITE_KEY).
  2. CiviCRM Developer Docs — Cryptography / CryptoRegistry & CryptoToken.
  3. AWS Secrets Manager / Well‑Architected guidance on secret storage and rotation.
  4. Microsoft Azure key vault best practices (secret protection, HSM, rotation).

🏁 Script executed:

# First, let's understand the file structure and see how this password setting is used
find . -type f -name "*.php" | grep -E "(civiglific|settings)" | head -20

Repository: ColoredCow/goonj

Length of output: 1448


🏁 Script executed:

# Look for how the civiglific_password setting is retrieved or used in the codebase
rg "civiglific_password" -A 3 -B 3

Repository: ColoredCow/goonj

Length of output: 1632


🏁 Script executed:

# Check if there are any encryption-related functions or utilities being used in this extension
rg "encrypt|decrypt|hash" wp-content/civi-extensions/civiglific/ || echo "No encryption utilities found in grep"

Repository: ColoredCow/goonj

Length of output: 97


🏁 Script executed:

# Read the full civiglific settings file to understand the complete context
cat -n wp-content/civi-extensions/civiglific/settings/civiglific.setting.php

Repository: ColoredCow/goonj

Length of output: 3188


🏁 Script executed:

# Check other extensions to see how they define password settings
rg "html_type.*password|type.*password" wp-content/civi-extensions/ -A 2 -B 2

Repository: ColoredCow/goonj

Length of output: 50374


🏁 Script executed:

# Look for any CiviCRM-specific encryption or credential handling in the extension
rg "CryptoRegistry|CryptoToken|credential|secret|encrypt" wp-content/civi-extensions/civiglific/ -i

Repository: ColoredCow/goonj

Length of output: 42


Store API password securely using CiviCRM's encryption.

The Glific API password is stored as plain text in the database. CiviCRM settings are stored unencrypted by default, exposing credentials to anyone with database access.

Use CiviCRM's built-in CIVICRM_CRED_KEYS encryption feature (AES-256) instead. Configure encryption keys in civicrm.settings.php and this setting will be automatically encrypted at rest.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
wp-content/civi-extensions/civiglific/CRM/Civiglific/Page/GroupMapping.php (1)

52-74: Validate settings before making API calls.

If civiglific_api_base_url is not configured, the API URL becomes malformed (/api/), causing the request to fail. While the try-catch logs the error, users won't understand the root cause is missing configuration.

🔎 Proposed fix with validation
 protected function fetchGlificGroups($token) {
-  $url = rtrim(\Civi::settings()->get('civiglific_api_base_url'), '/') . '/api/';
+  $baseUrl = \Civi::settings()->get('civiglific_api_base_url');
+  if (empty($baseUrl)) {
+    CRM_Core_Error::debug_log_message('Glific API base URL not configured in settings');
+    return [];
+  }
+  $url = rtrim($baseUrl, '/') . '/api/';
   $client = new Client();
wp-content/civi-extensions/civiglific/Civi/MonetaryReceiptService.php (1)

154-167: Validate PDF path settings.

If civiglific_persist_pdf_path or civiglific_saved_pdf_path are not configured, paths become malformed, causing file operations to fail or create files in incorrect locations. Validate these settings before file operations.

🔎 Proposed fix with validation
 // Save PDF to persistent location.
 $uploadBase = defined('WP_CONTENT_DIR') ? rtrim(WP_CONTENT_DIR, '/') : rtrim($_SERVER['DOCUMENT_ROOT'] ?? __DIR__, '/');
-$saveDir = $uploadBase . \Civi::settings()->get('civiglific_persist_pdf_path');
+$persistPath = \Civi::settings()->get('civiglific_persist_pdf_path');
+if (empty($persistPath)) {
+  throw new \Exception('PDF persist path not configured in settings');
+}
+$saveDir = $uploadBase . $persistPath;

 if (!file_exists($saveDir)) {
   mkdir($saveDir, 0755, TRUE);
 }

 $savePath = $saveDir . $fileNameForPdf;
 if (@file_put_contents($savePath, $pdfBinary) === FALSE) {
   throw new \Exception("Failed to save PDF to {$savePath}");
 }

 $baseUrl = rtrim(\CRM_Core_Config::singleton()->userFrameworkBaseURL, '/');
-$pdfUrl = $baseUrl . \Civi::settings()->get('civiglific_saved_pdf_path') . $fileNameForPdf;
+$savedPath = \Civi::settings()->get('civiglific_saved_pdf_path');
+if (empty($savedPath)) {
+  throw new \Exception('PDF saved path not configured in settings');
+}
+$pdfUrl = $baseUrl . $savedPath . $fileNameForPdf;
🧹 Nitpick comments (1)
wp-content/civi-extensions/civiglific/README.md (1)

29-30: Add language specifiers to fenced code blocks.

Markdown best practice: specify language for syntax highlighting in fenced code blocks. The blocks at lines 29-30 and 56-57 should use a language specifier (e.g., text or plaintext).

Based on static analysis hints.

🔎 Proposed fix
-   ```
+   ```text
    Administer → System Settings → Civiglific Settings
    ```

Apply the same fix at line 56-57.

Also applies to: 56-57

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4861d73 and 8e13245.

📒 Files selected for processing (4)
  • wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificClient.php
  • wp-content/civi-extensions/civiglific/CRM/Civiglific/Page/GroupMapping.php
  • wp-content/civi-extensions/civiglific/Civi/MonetaryReceiptService.php
  • wp-content/civi-extensions/civiglific/README.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tarunnjoshi
Repo: ColoredCow/goonj PR: 1230
File: wp-content/civi-extensions/civirazorpay/cli/update-recurring-contact-info.php:115-117
Timestamp: 2025-05-27T07:31:44.131Z
Learning: For one-time scripts in the Goonj project, tarunnjoshi prefers to keep hardcoded values rather than creating shared constants or configuration files, prioritizing simplicity over strict adherence to DRY principle to avoid over-engineering temporary code.
🧬 Code graph analysis (2)
wp-content/civi-extensions/civiglific/Civi/MonetaryReceiptService.php (1)
wp-content/civi-extensions/civicalendar-3.5.5/CRM/Calendar/Settings.php (1)
  • get (95-113)
wp-content/civi-extensions/civiglific/CRM/Civiglific/GlificClient.php (1)
wp-content/civi-extensions/civicalendar-3.5.5/CRM/Calendar/Settings.php (1)
  • get (95-113)
🪛 markdownlint-cli2 (0.18.1)
wp-content/civi-extensions/civiglific/README.md

29-29: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


56-56: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Comment on lines 90 to 95
if ($contributionPageName === 'Team_5000') {
$templateId = CIVICRM_GLIFIC_TEMPLATE_ID_TEAM5000;
$templateId = \Civi::settings()->get('civiglific_template_id_team5000');
}
else {
$templateId = CIVICRM_GLIFIC_TEMPLATE_ID_DEFAULT;
$templateId = \Civi::settings()->get('civiglific_template_id_default');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Validate template ID settings.

If the template ID settings are not configured, get() returns null, causing the Glific sendMessage API call (line 240) to fail. The method should validate that template IDs exist before attempting to send messages.

🔎 Proposed fix with validation
 $contributionPageName = $contributionData['contribution_page_id:name'] ?? '';
 if ($contributionPageName === 'Team_5000') {
   $templateId = \Civi::settings()->get('civiglific_template_id_team5000');
 }
 else {
   $templateId = \Civi::settings()->get('civiglific_template_id_default');
 }
+
+if (empty($templateId)) {
+  \Civi::log()->error("[MonetaryReceiptService] Template ID not configured for page: {$contributionPageName}");
+  return NULL;
+}

// Save PDF to persistent location.
$uploadBase = defined('WP_CONTENT_DIR') ? rtrim(WP_CONTENT_DIR, '/') : rtrim($_SERVER['DOCUMENT_ROOT'] ?? __DIR__, '/');
$saveDir = $uploadBase . CIVICRM_PERSIST_PDF_PATH;
$saveDir = $uploadBase . \Civi::settings()->get('civiglific_persist_pdf_path');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Ensure path settings include leading separators.

Path concatenation assumes that civiglific_persist_pdf_path starts with /. If the setting value lacks a leading separator, the resulting path will be malformed (e.g., /var/www/wp-contentuploads/pdfs/ instead of /var/www/wp-content/uploads/pdfs/). Consider normalizing the path or documenting the expected format in the settings UI.

🔎 Proposed fix with path normalization
 $uploadBase = defined('WP_CONTENT_DIR') ? rtrim(WP_CONTENT_DIR, '/') : rtrim($_SERVER['DOCUMENT_ROOT'] ?? __DIR__, '/');
 $persistPath = \Civi::settings()->get('civiglific_persist_pdf_path');
 if (empty($persistPath)) {
   throw new \Exception('PDF persist path not configured in settings');
 }
-$saveDir = $uploadBase . $persistPath;
+// Ensure path starts with separator
+$persistPath = '/' . ltrim($persistPath, '/');
+$saveDir = $uploadBase . $persistPath;

Apply similar logic for civiglific_saved_pdf_path at line 167.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/Civi/MonetaryReceiptService.php around
line 155, the code concatenates $uploadBase and the civiglific_persist_pdf_path
setting without ensuring a leading directory separator; normalize the setting by
trimming whitespace and leading/trailing slashes and then prepend the directory
separator (or use DIRECTORY_SEPARATOR) when concatenating so the resulting path
is always valid; apply the same normalization procedure to
civiglific_saved_pdf_path at line 167.

Comment on lines 19 to 23
public function __construct() {
$this->client = new Client();
$this->token = GlificHelper::getToken();
$this->url = rtrim(CIVICRM_GLIFIC_API_BASE_URL, '/') . '/api/';
$this->url = rtrim(\Civi::settings()->get('civiglific_api_base_url'), '/') . '/api/';
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Add validation for missing settings.

If civiglific_api_base_url is not configured in CiviCRM settings, get() returns null, causing rtrim() to produce an empty string. This results in a malformed URL (/api/) that will break all API calls.

🔎 Proposed fix with validation
 public function __construct() {
   $this->client = new Client();
   $this->token = GlificHelper::getToken();
-  $this->url = rtrim(\Civi::settings()->get('civiglific_api_base_url'), '/') . '/api/';
+  $baseUrl = \Civi::settings()->get('civiglific_api_base_url');
+  if (empty($baseUrl)) {
+    throw new \Exception('Civiglific API base URL is not configured. Please set civiglific_api_base_url in CiviCRM settings.');
+  }
+  $this->url = rtrim($baseUrl, '/') . '/api/';
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function __construct() {
$this->client = new Client();
$this->token = GlificHelper::getToken();
$this->url = rtrim(CIVICRM_GLIFIC_API_BASE_URL, '/') . '/api/';
$this->url = rtrim(\Civi::settings()->get('civiglific_api_base_url'), '/') . '/api/';
}
public function __construct() {
$this->client = new Client();
$this->token = GlificHelper::getToken();
$baseUrl = \Civi::settings()->get('civiglific_api_base_url');
if (empty($baseUrl)) {
throw new \Exception('Civiglific API base URL is not configured. Please set civiglific_api_base_url in CiviCRM settings.');
}
$this->url = rtrim($baseUrl, '/') . '/api/';
}

@@ -70,11 +84,11 @@ Once the extension is installed and configured:

3. **Template Management**
- Update your Glific template IDs in `settings.php` if you need to use different message templates for various campaigns or donor tiers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update outdated reference to settings.php.

Line 86 still references settings.php for template management, which contradicts the UI-based configuration approach described elsewhere in the documentation. This should reference the Civiglific Settings UI instead.

🔎 Proposed fix
-   - Update your Glific template IDs in `settings.php` if you need to use different message templates for various campaigns or donor tiers.
+   - Update your Glific template IDs in **Civiglific Settings** (Administer → System Settings → Civiglific Settings) if you need to use different message templates for various campaigns or donor tiers.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Update your Glific template IDs in `settings.php` if you need to use different message templates for various campaigns or donor tiers.
- Update your Glific template IDs in **Civiglific Settings** (Administer → System Settings → Civiglific Settings) if you need to use different message templates for various campaigns or donor tiers.
🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/README.md around line 86, the
documentation incorrectly instructs users to "Update your Glific template IDs in
`settings.php`"; update this sentence to reference the Civiglific Settings UI
instead (e.g., "Update your Glific template IDs in the Civiglific Settings UI")
and ensure surrounding text aligns with UI-based configuration guidance so there
is no contradiction elsewhere in the README.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
wp-content/civi-extensions/civiglific/README.md (1)

86-89: Verify that references to settings.php have been fully removed.

A previous review flagged outdated references to settings.php at line 86. The current text correctly references "Civiglific Settings" UI. Please confirm no other lingering references to file-based configuration remain elsewhere in the README.

#!/bin/bash
# Verify that settings.php is not referenced elsewhere in README
grep -n "settings\.php" wp-content/civi-extensions/civiglific/README.md || echo "No references to settings.php found"
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e13245 and 9322094.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/civiglific/README.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tarunnjoshi
Repo: ColoredCow/goonj PR: 1230
File: wp-content/civi-extensions/civirazorpay/cli/update-recurring-contact-info.php:115-117
Timestamp: 2025-05-27T07:31:44.131Z
Learning: For one-time scripts in the Goonj project, tarunnjoshi prefers to keep hardcoded values rather than creating shared constants or configuration files, prioritizing simplicity over strict adherence to DRY principle to avoid over-engineering temporary code.
🪛 markdownlint-cli2 (0.18.1)
wp-content/civi-extensions/civiglific/README.md

29-29: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


56-56: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Comment on lines 29 to +30
```
Administer → System Settings → Civiglific Settings

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Specify a language for the fenced code block.

The code block should include a language identifier for proper syntax highlighting.

-   ```
+   ```bash
    Administer → System Settings → Civiglific Settings
-   ```
+   ```
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

29-29: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/README.md around lines 29 to 30, the
fenced code block lacks a language identifier; update the opening fence to
include the language (bash) so the block starts with ```bash and keep the
existing content and closing fence unchanged to enable proper syntax
highlighting.

Comment on lines 56 to +57
```
Administer → System Settings → Civiglific Settings

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Specify a language for the fenced code block.

The code block should include a language identifier for proper syntax highlighting.

-   ```
+   ```bash
    Administer → System Settings → Civiglific Settings
-   ```
+   ```
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

56-56: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
In wp-content/civi-extensions/civiglific/README.md around lines 56 to 57 the
fenced code block lacks a language identifier; update the opening fence to
include the appropriate language (e.g., ```bash) so the snippet is
syntax-highlighted correctly, leaving the block contents and closing fence
unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant