Open
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Team5000SubscriptionReminderService
participant DB as Database
participant Email as Email System
Service->>Service: processReminders()
par Team 5000 Flow
Service->>DB: Query Team 5000 active recurs<br/>(specific contribution page)
DB-->>Service: Team 5000 recur records
Service->>DB: Check each recur for renewal
DB-->>Service: Has newer recur started?
Service->>Service: processTeam5000Reminders()
Service->>Service: processSubscription(T5k page, isTeam5000=true)
Service->>Email: sendReminderAndLog<br/>(getReminderEmailHtml)
Email-->>Service: Email sent
and Generic Flow
Service->>DB: Query generic active recurs<br/>(In Progress, installments > 0)
DB-->>Service: Generic recur records<br/>(exclude Team 5000 IDs)
Service->>Service: processGenericRecurringReminders()
Service->>Service: processSubscription(NULL page, isTeam5000=false)
Service->>DB: Check payment received<br/>after reminder activity
DB-->>Service: Latest contribution time
Service->>Email: sendReminderAndLog<br/>(getGenericReminderEmailHtml)
Email-->>Service: Email sent
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
While the refactoring appropriately separates concerns, watch for:
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php (4)
39-58:⚠️ Potential issue | 🔴 CriticalCritical:
is_test = TRUEfilters out all production contributions.Lines 43 and 57 filter for
is_test = TRUE, meaning this code will only process test contributions and completely skip real donor data. This is almost certainly a debugging artifact that should not be in production.🐛 Proposed fix
$contributions = Contribution::get(FALSE) ->addSelect('contribution_recur_id') ->addWhere('contribution_page_id:name', '=', self::CONTRIBUTION_PAGE_NAME) ->addWhere('contribution_recur_id', 'IS NOT NULL') - ->addWhere('is_test', '=', TRUE) + ->addWhere('is_test', '=', FALSE) ->execute();$recurringContributions = ContributionRecur::get(FALSE) ->addSelect('id', 'contact_id', 'start_date', 'installments', 'frequency_interval', 'frequency_unit', 'amount', 'contribution_status_id') ->addWhere('id', 'IN', $recurIds) ->addWhere('contribution_status_id:name', '=', 'In Progress') - ->addWhere('is_test', '=', TRUE) + ->addWhere('is_test', '=', FALSE) ->execute();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 39 - 58, The queries for recurring contributions are incorrectly filtering only test data: remove the is_test = TRUE filter (or change it to is_test = FALSE if you explicitly want non-test records) from both the Contribution::get() chain (where addWhere('is_test', '=', TRUE) appears) and the ContributionRecur::get() chain (where addWhere('is_test', '=', TRUE) appears) so that Contribution::get, the $recurIds extraction, and ContributionRecur::get(...) will operate on real production contributions; keep the other addWhere conditions (contribution_page_id, contribution_recur_id, contribution_status_id) unchanged.
374-379:⚠️ Potential issue | 🟠 MajorEmail subject hardcoded to "Team 5000 Membership" for all flows.
Generic recurring donors will receive emails with subjects like "Your Team 5000 Membership Expires Tomorrow" — confusing for donors who aren't Team 5000 members.
🛠️ Suggested fix
- private static function getEmailSubject(int $daysBefore): string { + private static function getEmailSubject(int $daysBefore, bool $isTeam5000 = TRUE): string { + $membershipType = $isTeam5000 ? 'Team 5000 Membership' : 'Recurring Donation'; if ($daysBefore === 1) { - return 'Last Reminder: Your Team 5000 Membership Expires Tomorrow'; + return "Last Reminder: Your {$membershipType} Expires Tomorrow"; } - return "Reminder: Your Team 5000 Membership Expires in {$daysBefore} Days"; + return "Reminder: Your {$membershipType} Expires in {$daysBefore} Days"; }Then pass
$isTeam5000fromsendReminderAndLog().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 374 - 379, The email subject is hardcoded to "Team 5000 Membership" in getEmailSubject(int $daysBefore) which mislabels non‑Team5000 donors; change getEmailSubject to accept a boolean flag (e.g., getEmailSubject(int $daysBefore, bool $isTeam5000)) and return a Team5000‑specific subject only when $isTeam5000 is true, otherwise return a generic subject (e.g., "Your Membership" or flow‑appropriate text); update all call sites (notably sendReminderAndLog() and any other callers) to pass the $isTeam5000 value through so the correct subject is chosen per recipient.
127-173:⚠️ Potential issue | 🟡 MinorLog messages hardcoded to "Team 5000" for both flows.
The method now serves both Team 5000 and generic reminders, but log messages at lines 129, 152, 160-161, and 168 all say "Team 5000:". This will confuse log analysis when debugging generic reminder issues.
💡 Suggested approach
- private static function processSubscription(array $recur, \DateTimeImmutable $now, string $from, ?string $contributionPageName, bool $isTeam5000): void { + private static function processSubscription(array $recur, \DateTimeImmutable $now, string $from, ?string $contributionPageName, bool $isTeam5000): void { + $logPrefix = $isTeam5000 ? 'Team 5000' : 'Recurring reminders'; if (empty($recur['start_date']) || empty($recur['installments'])) { - \Civi::log()->info('Team 5000: Skipping recur {id} — missing start_date or installments', [ + \Civi::log()->info($logPrefix . ': Skipping recur {id} — missing start_date or installments', [Apply similar changes to other log statements in this method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 127 - 173, The log messages inside processSubscription are hardcoded to "Team 5000" even when $isTeam5000 is false; update all log lines in processSubscription (the info calls at/around the checks using hasReminderBeenSent, hasPaymentReceivedAfterReminder, hasRenewed and the initial missing-field log) to use a dynamic prefix or context string—e.g., compute $logPrefix = $isTeam5000 ? 'Team 5000' : 'Reminder' (or include $contributionPageName when present) and interpolate that prefix into each Civi::log()->info message so logs correctly reflect which flow produced them.
360-369:⚠️ Potential issue | 🟠 MajorActivity subject hardcoded to "Team 5000" for all reminder types.
Line 366 creates activities with subject "Team 5000 Subscription Expiry Reminder" regardless of whether this is a Team 5000 or generic reminder. This:
- Creates misleading audit trails for generic recurs
- Could cause
hasPaymentReceivedAfterReminder()to behave unexpectedly if activity type filtering ever changes🛠️ Suggested fix
- private static function createReminderActivity(int $contactId, int $recurId, int $daysBefore): void { + private static function createReminderActivity(int $contactId, int $recurId, int $daysBefore, bool $isTeam5000 = TRUE): void { + $subjectPrefix = $isTeam5000 ? 'Team 5000 Subscription' : 'Recurring Donation'; Activity::create(FALSE) ->addValue('activity_type_id:name', self::ACTIVITY_TYPE_NAME) ->addValue('status_id:name', 'Completed') ->addValue('source_contact_id', $contactId) ->addValue('target_contact_id', $contactId) - ->addValue('subject', 'Team 5000 Subscription Expiry Reminder - ' . $daysBefore . ' days (recur_id:' . $recurId . ')') + ->addValue('subject', $subjectPrefix . ' Expiry Reminder - ' . $daysBefore . ' days (recur_id:' . $recurId . ')') ->addValue('details', 'Automated reminder sent ' . $daysBefore . ' day(s) before subscription expiry.') ->execute(); }Then pass
$isTeam5000fromsendReminderAndLog().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 360 - 369, The activity subject is hardcoded to "Team 5000" in createReminderActivity; change createReminderActivity to accept a boolean flag (e.g., $isTeam5000) and build the subject conditionally (use "Team 5000 Subscription Expiry Reminder" when $isTeam5000 is true, otherwise a generic "Subscription Expiry Reminder"), preserving the daysBefore and recurId suffix; update sendReminderAndLog to pass the $isTeam5000 value into createReminderActivity and update any other callers of createReminderActivity accordingly so the subject correctly reflects the reminder type.
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php (2)
81-101: Code duplication and inconsistentis_testfiltering.Two concerns here:
Duplication: Lines 83-89 fetch Team 5000 contributions identically to lines 39-46. Consider extracting to a helper method.
Missing
is_testfilter: The generic flow at line 91-101 doesn't filter onis_test, while Team 5000 does. This inconsistency could cause generic reminders to process test data in production.♻️ Proposed fix
+ /** + * Returns Team 5000 recur IDs from contributions. + */ + private static function getTeam5000RecurIds(): array { + $contributions = Contribution::get(FALSE) + ->addSelect('contribution_recur_id') + ->addWhere('contribution_page_id:name', '=', self::CONTRIBUTION_PAGE_NAME) + ->addWhere('contribution_recur_id', 'IS NOT NULL') + ->addWhere('is_test', '=', FALSE) + ->execute(); + + return array_unique(array_column((array) $contributions, 'contribution_recur_id')); + }Then add the
is_testfilter to the generic query:$query = ContributionRecur::get(FALSE) ->addSelect('id', 'contact_id', 'start_date', 'installments', 'frequency_interval', 'frequency_unit', 'amount', 'contribution_status_id') ->addWhere('contribution_status_id:name', '=', 'In Progress') + ->addWhere('is_test', '=', FALSE) ->addWhere('installments', 'IS NOT NULL') ->addWhere('installments', '>', 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 81 - 101, The code in processGenericRecurringReminders duplicates the Team 5000 contribution fetch (Contribution::get(...) block) and omits the is_test filter present in the Team 5000 flow; extract the Team 5000 fetch into a helper (e.g., getTeam5000RecurIds or similar) and reuse it in both places to remove duplication, and add the same is_test filter to the generic ContributionRecur query (the $query built from ContributionRecur::get(...) in processGenericRecurringReminders) so test records are excluded consistently.
302-354: Template selection is correct; log messages need consistency.The conditional template selection (lines 331-333) correctly chooses between Team 5000 and generic email templates based on
$isTeam5000. However, the hardcoded "Team 5000:" log prefix at lines 313, 339, and 348 will mislead when processing generic reminders — same pattern noted earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php` around lines 302 - 354, The log prefix is hardcoded to "Team 5000:" in sendReminderAndLog which mislabels generic reminders; introduce a local $logPrefix = $isTeam5000 ? 'Team 5000' : 'Generic Reminder' at the top of sendReminderAndLog and use "{$logPrefix}: ..." in the three logging calls (the info skip when no email, the error on send failure, and the info on send success) so logs reflect whether getReminderEmailHtml or getGenericReminderEmailHtml was used; keep the existing context keys and calls to \Civi::log(), \CRM_Utils_Mail::send, and self::createReminderActivity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php`:
- Around line 39-58: The queries for recurring contributions are incorrectly
filtering only test data: remove the is_test = TRUE filter (or change it to
is_test = FALSE if you explicitly want non-test records) from both the
Contribution::get() chain (where addWhere('is_test', '=', TRUE) appears) and the
ContributionRecur::get() chain (where addWhere('is_test', '=', TRUE) appears) so
that Contribution::get, the $recurIds extraction, and
ContributionRecur::get(...) will operate on real production contributions; keep
the other addWhere conditions (contribution_page_id, contribution_recur_id,
contribution_status_id) unchanged.
- Around line 374-379: The email subject is hardcoded to "Team 5000 Membership"
in getEmailSubject(int $daysBefore) which mislabels non‑Team5000 donors; change
getEmailSubject to accept a boolean flag (e.g., getEmailSubject(int $daysBefore,
bool $isTeam5000)) and return a Team5000‑specific subject only when $isTeam5000
is true, otherwise return a generic subject (e.g., "Your Membership" or
flow‑appropriate text); update all call sites (notably sendReminderAndLog() and
any other callers) to pass the $isTeam5000 value through so the correct subject
is chosen per recipient.
- Around line 127-173: The log messages inside processSubscription are hardcoded
to "Team 5000" even when $isTeam5000 is false; update all log lines in
processSubscription (the info calls at/around the checks using
hasReminderBeenSent, hasPaymentReceivedAfterReminder, hasRenewed and the initial
missing-field log) to use a dynamic prefix or context string—e.g., compute
$logPrefix = $isTeam5000 ? 'Team 5000' : 'Reminder' (or include
$contributionPageName when present) and interpolate that prefix into each
Civi::log()->info message so logs correctly reflect which flow produced them.
- Around line 360-369: The activity subject is hardcoded to "Team 5000" in
createReminderActivity; change createReminderActivity to accept a boolean flag
(e.g., $isTeam5000) and build the subject conditionally (use "Team 5000
Subscription Expiry Reminder" when $isTeam5000 is true, otherwise a generic
"Subscription Expiry Reminder"), preserving the daysBefore and recurId suffix;
update sendReminderAndLog to pass the $isTeam5000 value into
createReminderActivity and update any other callers of createReminderActivity
accordingly so the subject correctly reflects the reminder type.
---
Nitpick comments:
In
`@wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php`:
- Around line 81-101: The code in processGenericRecurringReminders duplicates
the Team 5000 contribution fetch (Contribution::get(...) block) and omits the
is_test filter present in the Team 5000 flow; extract the Team 5000 fetch into a
helper (e.g., getTeam5000RecurIds or similar) and reuse it in both places to
remove duplication, and add the same is_test filter to the generic
ContributionRecur query (the $query built from ContributionRecur::get(...) in
processGenericRecurringReminders) so test records are excluded consistently.
- Around line 302-354: The log prefix is hardcoded to "Team 5000:" in
sendReminderAndLog which mislabels generic reminders; introduce a local
$logPrefix = $isTeam5000 ? 'Team 5000' : 'Generic Reminder' at the top of
sendReminderAndLog and use "{$logPrefix}: ..." in the three logging calls (the
info skip when no email, the error on send failure, and the info on send
success) so logs reflect whether getReminderEmailHtml or
getGenericReminderEmailHtml was used; keep the existing context keys and calls
to \Civi::log(), \CRM_Utils_Mail::send, and self::createReminderActivity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5bf52b63-f62f-4c63-999e-e365bd374c0b
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/Team5000SubscriptionReminderService.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reminder to recurring contributor
Summary by CodeRabbit
New Features
Improvements