Fix(*) Followup cron contact not found #486
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (4)
Line range hint
1-900: Consider refactoring email sending logic to reduce duplicationThe class contains multiple methods with similar email sending patterns (
sendInductionEmail,sendFollowUpEmails,sendInductionRescheduleEmail,sendRemainderEmails). This leads to code duplication and potential maintenance issues.Consider extracting the common email sending logic into a reusable private method:
+ private static function sendTemplatedEmail($contactId, $templatePattern, $customFieldToUpdate = null) { + $template = MessageTemplate::get(FALSE) + ->addSelect('id', 'msg_subject') + ->addWhere('msg_title', 'LIKE', $templatePattern) + ->execute()->single(); + + if (!$template) { + throw new \Exception("Email template not found: $templatePattern"); + } + + $emailParams = [ + 'contact_id' => $contactId, + 'template_id' => $template['id'], + ]; + + $result = civicrm_api3('Email', 'send', $emailParams); + + if ($customFieldToUpdate) { + Contact::update(FALSE) + ->addValue($customFieldToUpdate, 1) + ->addWhere('id', '=', $contactId) + ->execute(); + } + + return $result; + }This would simplify the email sending methods and make the code more maintainable.
Line range hint
513-538: Consider using early returns to reduce nestingThe nested if conditions in the email sending logic make the code harder to follow. Consider using early returns to flatten the structure.
Example refactor:
- if (empty($isMailSent)) { - $emailParams = [ - 'contact_id' => $activity['source_contact_id'], - 'template_id' => $template['id'], - ]; - // ... more nested code - } + if (!empty($isMailSent)) { + return; + } + + $emailParams = [ + 'contact_id' => $activity['source_contact_id'], + 'template_id' => $template['id'], + ]; + // ... rest of the code at the same level
Line range hint
1-900: Consider implementing retry mechanism for email sendingThe email sending operations could fail due to various reasons (network issues, server problems, etc.). Consider implementing a retry mechanism with exponential backoff.
Error handling inconsistency found in email sending operations
The codebase shows inconsistent error handling for email operations:
processQueuedInductionEmail()has proper try-catch with error loggingsendInductionEmailToVolunteer()andcreateInductionForTransitionedVolunteer()lack error handlingsendFollowUpEmails()performs email operations without error handlingsendInductionRescheduleEmail()lacks error handling for the email sending operationKey locations needing attention:
- Line ~510:
sendInductionEmailToVolunteer()needs try-catch block- Line ~590:
sendInductionEmailForTransitionedVolunteer()needs try-catch block- Line ~750:
sendFollowUpEmails()needs try-catch around email operations- Line ~810:
sendInductionRescheduleEmail()needs try-catch around email operations🔗 Analysis chain
Line range hint
1-900: Verify error handling consistency across email operationsSome email sending methods have proper error handling while others don't. Let's verify the consistency of error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for email sending operations without try-catch blocks rg "civicrm_api3\('Email', 'send'" -A 5 -B 5Length of output: 22733
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(2 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (2)
513-513: LGTM: Logging enhancement added
The addition of logging for unscheduled induction activities improves observability.
704-704: LGTM: Fixed contact ID usage
The code now correctly uses the provided contact ID parameter.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes