Conversation
WalkthroughTwo PHP service files are modified to add explicit email sender addresses to mail parameters in receipt-handling workflows. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Code Review SummaryWhat Looks Good
SuggestionsNone - the changes are straightforward and appropriate for the stated purpose. Out of ScopeNone identified. Overall: ✅ Approved. The PR correctly updates the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
914-915: Duplicated hardcoded email address across services.The same
fromaddress'Goonj <accounts@goonj.org>'is now hardcoded in bothalterReceiptMail(line 915) andhandleOfflineReceipt(line 960), and also inCollectionCampService. Meanwhile,InductionServiceusesHelperService::getDefaultFromEmail()for its sender address, creating inconsistent sender configurations across the codebase.Consider centralizing the accounts team email in
HelperServiceto maintain a single source of truth:♻️ Suggested approach
Add a method to
HelperService:public static function getAccountsFromEmail(): string { return 'Goonj <accounts@goonj.org>'; }Then use it consistently:
- $params['from'] = 'Goonj <accounts@goonj.org>'; + $params['from'] = HelperService::getAccountsFromEmail();Based on the relevant code snippet from
InductionService.php, this service usesHelperService::getDefaultFromEmail()for its sender, creating a mixed sender experience for users receiving different types of emails.Also applies to: 959-960
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php` around lines 914 - 915, Duplicate hardcoded sender addresses are used in GoonjInitiatedEventsService::alterReceiptMail and ::handleOfflineReceipt (and in CollectionCampService) while InductionService uses HelperService::getDefaultFromEmail(); add a single-source helper like HelperService::getAccountsFromEmail() returning 'Goonj <accounts@goonj.org>' and replace the hardcoded 'Goonj <accounts@goonj.org>' occurrences in GoonjInitiatedEventsService (both alterReceiptMail and handleOfflineReceipt) and CollectionCampService to call HelperService::getAccountsFromEmail() so all services use the same centralized sender address.wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
2197-2198: Hardcoded email string duplicates existing constant.The same
fromaddress is hardcoded here and inalterReceiptMail(line 2112), while a constantACCOUNTS_TEAM_EMAILalready exists at line 45. Note the slight format difference: the constant uses"Goonj"(quoted) vsGoonj(unquoted) here.Consider using the constant for consistency, or if the unquoted format is intentional, update the constant and use it throughout:
♻️ Suggested refactor
- $params['from'] = 'Goonj <accounts@goonj.org>'; + $params['from'] = self::ACCOUNTS_TEAM_EMAIL;If the unquoted format is preferred, first update the constant at line 45:
- const ACCOUNTS_TEAM_EMAIL = '"Goonj" <accounts@goonj.org>'; + const ACCOUNTS_TEAM_EMAIL = 'Goonj <accounts@goonj.org>';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php` around lines 2197 - 2198, Replace the hardcoded from string used when setting $params['from'] with the existing ACCOUNTS_TEAM_EMAIL constant to avoid duplication and ensure consistency (replace the literal 'Goonj <accounts@goonj.org>' in the code that sets $params['from']); if the unquoted format ("Goonj <...>") is intentionally different, update the ACCOUNTS_TEAM_EMAIL constant definition to the desired format and then use that constant both where $params['from'] is assigned and in alterReceiptMail so both places reference the same canonical value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`:
- Around line 2197-2198: Replace the hardcoded from string used when setting
$params['from'] with the existing ACCOUNTS_TEAM_EMAIL constant to avoid
duplication and ensure consistency (replace the literal 'Goonj
<accounts@goonj.org>' in the code that sets $params['from']); if the unquoted
format ("Goonj <...>") is intentionally different, update the
ACCOUNTS_TEAM_EMAIL constant definition to the desired format and then use that
constant both where $params['from'] is assigned and in alterReceiptMail so both
places reference the same canonical value.
In `@wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php`:
- Around line 914-915: Duplicate hardcoded sender addresses are used in
GoonjInitiatedEventsService::alterReceiptMail and ::handleOfflineReceipt (and in
CollectionCampService) while InductionService uses
HelperService::getDefaultFromEmail(); add a single-source helper like
HelperService::getAccountsFromEmail() returning 'Goonj <accounts@goonj.org>' and
replace the hardcoded 'Goonj <accounts@goonj.org>' occurrences in
GoonjInitiatedEventsService (both alterReceiptMail and handleOfflineReceipt) and
CollectionCampService to call HelperService::getAccountsFromEmail() so all
services use the same centralized sender address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a8d370a-ced6-414a-83c4-4a7a12eab832
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.phpwp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php
Summary by CodeRabbit