Add Cron to send Reschedule email volunteer induction status is not Visited#459
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (6)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php (4)
3-5: Remove unused importsThe
ActivityandMessageTemplateclasses are imported but never used in this file. Consider removing these unused imports to maintain clean and clear code.-use Civi\Api4\Activity; -use Civi\Api4\MessageTemplate; use Civi\InductionService;
7-18: Fix documentation inconsistencyThe API specification documentation refers to "InductionSlotBookingRescheduleMail" but the actual function is named "induction_reschedule_cron". Please update the documentation to match the actual implementation.
/** - * Goonjcustom.InductionSlotBookingRescheduleMail API specification (optional) + * Goonjcustom.InductionRescheduleCron API specification (optional) * This is used for documentation and validation. *
20-26: Fix documentation inconsistencyThe function documentation refers to "induction_slot_booking_follow_up_cron" but the actual function is "induction_reschedule_cron". Please update the documentation to match the implementation.
37-47: Consider architectural improvementsA few suggestions to improve the implementation:
The function mixes cron job scheduling with email sending logic. Consider separating these concerns:
- Create a dedicated CronJob class to handle scheduling
- Move email sending logic to a separate service
Error handling could be more specific:
- Catch specific exceptions rather than generic Exception
- Add different error responses for different failure scenarios
- Include more context in error messages
Consider adding parameter validation even though no parameters are required currently:
- Validate $params array
- Add type hints
Here's a suggested refactor:
-function civicrm_api3_goonjcustom_induction_reschedule_cron($params) { +function civicrm_api3_goonjcustom_induction_reschedule_cron(array $params): array { $returnValues = []; try { - InductionService::sendInductionRescheduleEmail(); + // Validate parameters + if (!empty($params)) { + throw new InvalidArgumentException('This API endpoint does not accept any parameters'); + } + + // Execute cron job + $cronJob = new InductionRescheduleCronJob(); + $cronJob->execute(); + } catch (Exception $e) { - \Civi::log()->error('Error in induction reschedule cron: ' . $e->getMessage()); + $context = [ + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]; + \Civi::log()->error('Error in induction reschedule cron', $context); return civicrm_api3_create_error($e->getMessage()); }wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (2)
601-707: Add missing PHPDoc for new methodsThe newly added methods
sendInductionRescheduleEmailandhandleRescheduleEmailActivitylack PHPDoc comments. Including method documentation enhances code readability and provides valuable context for other developers and tools.Apply this diff to add the missing documentation:
+/** + * Sends reschedule emails to volunteers whose induction status is 'Not Visited'. + * Updates the status to 'To be scheduled' or 'No_show' based on email activity. + */ public static function sendInductionRescheduleEmail() { +/** + * Handles reschedule email activity for a volunteer. + * Updates the induction activity status to 'No_show' if a reschedule email was sent. + * + * @param int $contactId + * The contact ID of the volunteer. + * @param int $activityId + * The ID of the induction activity. + * @return bool + * TRUE if the status was updated, FALSE otherwise. + */ public static function handleRescheduleEmailActivity($contactId, $activityId) {
678-707: Optimize email activity retrieval for performanceIn the
handleRescheduleEmailActivitymethod, retrieving all email activities without limiting the results can lead to performance issues, especially with a large dataset. Introduce a limit to the query to optimize performance.Apply this diff to limit the query result:
$emailActivities = Activity::get(FALSE) ->addWhere('activity_type_id:name', '=', 'Email') ->addWhere('subject', '=', $template['msg_subject']) ->addWhere('source_contact_id', '=', $contactId) + ->setLimit(1) ->execute();This ensures that only the first relevant email activity is retrieved, improving the method's efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(5 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php(1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php (1)
37-47: Verify implementation completeness
The current implementation seems to be missing some functionality described in the PR objectives:
-
Logic to mark volunteers as "No Show" if they:
- Receive a reschedule email but don't schedule an induction slot
- Fail to attend their induction after rescheduling
-
Checks for volunteer status being "Not Visited"
Please verify if these features are implemented in the InductionService::sendInductionRescheduleEmail() method.
| return FALSE; | ||
| } | ||
|
|
||
| \Civi::log()->info('check4'); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| \Civi::log()->info('unscheduledInductionActivities', ['unscheduledInductionActivities'=>$unscheduledInductionActivities]); | ||
|
|
||
| // Process each activity in the batch | ||
| foreach ($unscheduledInductionActivities as $activity) { | ||
| // Check if a reschedule email has already been sent and handled | ||
| if (self::handleRescheduleEmailActivity($activity['source_contact_id'], $activity['id'])) { | ||
| \Civi::log()->info('Skipping follow-up email due to existing reschedule email activity'); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicate code by refactoring common logic
The methods sendFollowUpEmails (lines 495-504) and sendInductionRescheduleEmail (lines 606-673) share similar logic for fetching activities, checking email sent status, and sending emails. This duplication violates the DRY (Don't Repeat Yourself) principle and makes the code harder to maintain. Refactor the common functionality into a shared private method to enhance code readability and maintainability.
Here is how you can refactor the code:
- Create a private method to handle email sending logic:
private static function processInductionEmails($activityTypeName, $statusName, $templateTitle, $statusUpdate) {
// Common logic for fetching activities, checking emails, and sending emails
}- Modify
sendFollowUpEmailsmethod:
public static function sendFollowUpEmails() {
- // Existing implementation
+ self::processInductionEmails('Induction', 'To be scheduled', 'Induction_slot_booking_follow_up_email%', 'No_show');
}- Modify
sendInductionRescheduleEmailmethod:
public static function sendInductionRescheduleEmail() {
- // Existing implementation
+ self::processInductionEmails('Induction', 'Not Visited', 'Induction_reschedule_slot_booking%', 'To be scheduled');
}This approach consolidates the shared logic, reduces code duplication, and adheres to best practices.
Also applies to: 606-673
| } | ||
|
|
||
| // Return false if no reschedule email activity exists or update failed | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Correct inconsistent return value in handleRescheduleEmailActivity
The method handleRescheduleEmailActivity does not return a consistent boolean value. If the update fails, it currently returns false, but if the update succeeds, it returns true. However, if no email activity exists, it also returns false. This can be confusing for callers of the method.
Adjust the return statements to consistently reflect the outcome:
if (!empty($emailActivity)) {
// Update status
if ($updateResult) {
return true;
} else {
return false;
}
} else {
- return false;
+ // No email activity found
+ return null;
}Alternatively, document the possible return values and their meanings in the method's PHPDoc to clarify the behavior.
Committable suggestion skipped: line range outside the PR's diff.
| $notVisitedInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'Not Visited') | ||
| ->addWhere('modified_date', '<', date('Y-m-d H:i:s', $rescheduleEmailTimestamp)) | ||
| ->setLimit($batchSize) | ||
| ->setOffset($offset) | ||
| ->execute(); | ||
|
|
||
| \Civi::log()->info('notVisitedInductionActivities', ['notVisitedInductionActivities'=>$notVisitedInductionActivities]); | ||
|
|
||
| // // Process each activity in the batch | ||
| foreach ($notVisitedInductionActivities as $activity) { | ||
| // Check if a follow-up email has already been sent to avoid duplication. | ||
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', '=', $template['msg_subject']) | ||
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | ||
| ->execute(); | ||
|
|
||
| \Civi::log()->info('emailActivities', ['emailActivities'=>$emailActivities]); | ||
|
|
||
| $emailActivity = $emailActivities->first(); | ||
|
|
||
| if (!$emailActivity) { | ||
| \Civi::log()->info('activity', ['activity'=>$activity]); | ||
|
|
||
| $updateResult = Activity::update(FALSE) | ||
| ->addValue('status_id:name', 'To be scheduled') | ||
| ->addWhere('id', '=', $activity['id']) | ||
| ->execute(); | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
|
|
||
| if (!empty($emailActivity)){ | ||
| \Civi::log()->info('check email sent for rescheduling'); | ||
| $updateResult = Activity::update(FALSE) | ||
| ->addValue('status_id:name', 'No_show') | ||
| ->addWhere('id', '=', $activity['id']) | ||
| ->execute(); | ||
| } | ||
| } | ||
|
|
||
| // Move to the next batch by increasing the offset | ||
| $offset += $batchSize; | ||
|
|
||
| } while (count($notVisitedInductionActivities) === $batchSize); | ||
| } |
There was a problem hiding this comment.
Ensure consistent status updates and avoid logical errors
In the sendInductionRescheduleEmail method, the status updates for induction activities might lead to unexpected behavior. Specifically, the condition on lines 660-666 updates the status to 'No_show' even if an email was already sent. This could result in volunteers being incorrectly marked as 'No_show'.
Consider revising the logic to accurately reflect the volunteer's status:
if (!$emailActivity) {
// Send reschedule email and update status to 'To be scheduled'
// Existing code
} else {
- // Update status to 'No_show'
- $updateResult = Activity::update(FALSE)
- ->addValue('status_id:name', 'No_show')
- ->addWhere('id', '=', $activity['id'])
- ->execute();
+ \Civi::log()->info('Reschedule email already sent; no action needed.');
}This change prevents unintentional status updates and ensures that volunteers are not incorrectly marked as 'No_show'.
📝 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.
| $notVisitedInductionActivities = Activity::get(FALSE) | |
| ->addSelect('id', 'source_contact_id', 'created_date') | |
| ->addWhere('activity_type_id:name', '=', 'Induction') | |
| ->addWhere('status_id:name', '=', 'Not Visited') | |
| ->addWhere('modified_date', '<', date('Y-m-d H:i:s', $rescheduleEmailTimestamp)) | |
| ->setLimit($batchSize) | |
| ->setOffset($offset) | |
| ->execute(); | |
| \Civi::log()->info('notVisitedInductionActivities', ['notVisitedInductionActivities'=>$notVisitedInductionActivities]); | |
| // // Process each activity in the batch | |
| foreach ($notVisitedInductionActivities as $activity) { | |
| // Check if a follow-up email has already been sent to avoid duplication. | |
| $emailActivities = Activity::get(FALSE) | |
| ->addWhere('activity_type_id:name', '=', 'Email') | |
| ->addWhere('subject', '=', $template['msg_subject']) | |
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | |
| ->execute(); | |
| \Civi::log()->info('emailActivities', ['emailActivities'=>$emailActivities]); | |
| $emailActivity = $emailActivities->first(); | |
| if (!$emailActivity) { | |
| \Civi::log()->info('activity', ['activity'=>$activity]); | |
| $updateResult = Activity::update(FALSE) | |
| ->addValue('status_id:name', 'To be scheduled') | |
| ->addWhere('id', '=', $activity['id']) | |
| ->execute(); | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| civicrm_api3('Email', 'send', $emailParams); | |
| } | |
| if (!empty($emailActivity)){ | |
| \Civi::log()->info('check email sent for rescheduling'); | |
| $updateResult = Activity::update(FALSE) | |
| ->addValue('status_id:name', 'No_show') | |
| ->addWhere('id', '=', $activity['id']) | |
| ->execute(); | |
| } | |
| } | |
| // Move to the next batch by increasing the offset | |
| $offset += $batchSize; | |
| } while (count($notVisitedInductionActivities) === $batchSize); | |
| } | |
| $notVisitedInductionActivities = Activity::get(FALSE) | |
| ->addSelect('id', 'source_contact_id', 'created_date') | |
| ->addWhere('activity_type_id:name', '=', 'Induction') | |
| ->addWhere('status_id:name', '=', 'Not Visited') | |
| ->addWhere('modified_date', '<', date('Y-m-d H:i:s', $rescheduleEmailTimestamp)) | |
| ->setLimit($batchSize) | |
| ->setOffset($offset) | |
| ->execute(); | |
| \Civi::log()->info('notVisitedInductionActivities', ['notVisitedInductionActivities'=>$notVisitedInductionActivities]); | |
| // // Process each activity in the batch | |
| foreach ($notVisitedInductionActivities as $activity) { | |
| // Check if a follow-up email has already been sent to avoid duplication. | |
| $emailActivities = Activity::get(FALSE) | |
| ->addWhere('activity_type_id:name', '=', 'Email') | |
| ->addWhere('subject', '=', $template['msg_subject']) | |
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | |
| ->execute(); | |
| \Civi::log()->info('emailActivities', ['emailActivities'=>$emailActivities]); | |
| $emailActivity = $emailActivities->first(); | |
| if (!$emailActivity) { | |
| \Civi::log()->info('activity', ['activity'=>$activity]); | |
| $updateResult = Activity::update(FALSE) | |
| ->addValue('status_id:name', 'To be scheduled') | |
| ->addWhere('id', '=', $activity['id']) | |
| ->execute(); | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| civicrm_api3('Email', 'send', $emailParams); | |
| } | |
| if (!empty($emailActivity)){ | |
| \Civi::log()->info('Reschedule email already sent; no action needed.'); | |
| } | |
| } | |
| // Move to the next batch by increasing the offset | |
| $offset += $batchSize; | |
| } while (count($notVisitedInductionActivities) === $batchSize); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3)
496-503: Consider extracting email activity check into a reusable method.The email activity check logic is duplicated across multiple methods. Consider extracting it into a private method for better maintainability.
+ private static function hasEmailActivityBeenSent($contactId, $templateSubject) { + $emailActivities = Activity::get(FALSE) + ->addWhere('activity_type_id:name', '=', 'Email') + ->addWhere('subject', '=', $templateSubject) + ->addWhere('source_contact_id', '=', $contactId) + ->execute(); + return !empty($emailActivities->first()); + }
671-703: Improve return value consistency in handleRescheduleEmailActivity.The method's return values could be more explicit about the different scenarios.
public static function handleRescheduleEmailActivity($contactId, $activityId) { + try { $template = MessageTemplate::get(FALSE) ->addSelect('id', 'msg_subject') ->addWhere('msg_title', 'LIKE', 'Induction_reschedule_slot_booking%') ->execute()->single(); + + if (!$template) { + \Civi::log()->warning('Reschedule email template not found'); + return false; + } $emailActivities = Activity::get(FALSE) ->addWhere('activity_type_id:name', '=', 'Email') ->addWhere('subject', '=', $template['msg_subject']) ->addWhere('source_contact_id', '=', $contactId) ->execute(); $emailActivity = $emailActivities->first(); if (!empty($emailActivity)) { $updateResult = Activity::update(FALSE) ->addValue('status_id:name', 'No_show') ->addWhere('id', '=', $activityId) ->execute(); - if ($updateResult) { - return true; - } + return !$updateResult->isError(); } - return false; + return null; // Indicates no email activity found + } catch (\Exception $e) { + \Civi::log()->error('Error in handleRescheduleEmailActivity: ' . $e->getMessage()); + return false; + } }
600-703: Consider implementing rate limiting for email sending.The email sending functionality in both new methods could benefit from rate limiting to prevent overwhelming the email server.
Consider implementing a rate limiter using a configuration parameter to control the maximum number of emails sent per minute. This would help prevent potential email server throttling or blacklisting.
Example configuration:
const EMAIL_RATE_LIMIT_PER_MINUTE = 60;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
Line range hint 134-147: LGTM!
The modifications to createInductionForVolunteer maintain proper error handling and logging.
|
|
||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public static function sendInductionRescheduleEmail() { | ||
| $rescheduleEmailDelayDays = 1; | ||
| $rescheduleEmailTimestamp = strtotime("-{$rescheduleEmailDelayDays} days"); | ||
|
|
||
| // Retrieve the email template for reschedule-email. | ||
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_reschedule_slot_booking%') | ||
| ->execute()->single(); | ||
|
|
||
| $batchSize = 25; | ||
| $offset = 0; | ||
|
|
||
| do { | ||
| // Retrieve a batch of not visited induction activities older than 1 days | ||
| $notVisitedInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'Not Visited') | ||
| ->addWhere('modified_date', '<', date('Y-m-d H:i:s', $rescheduleEmailTimestamp)) | ||
| ->setLimit($batchSize) | ||
| ->setOffset($offset) | ||
| ->execute(); | ||
|
|
||
| foreach ($notVisitedInductionActivities as $activity) { | ||
| // Check if a follow-up email has already been sent | ||
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', '=', $template['msg_subject']) | ||
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | ||
| ->execute(); | ||
|
|
||
| $emailActivity = $emailActivities->first(); | ||
|
|
||
| if ($emailActivity) { | ||
| // If an email already exists, mark activity as 'No Show' | ||
| $updateResult = Activity::update(FALSE) | ||
| ->addValue('status_id:name', 'No_show') | ||
| ->addWhere('id', '=', $activity['id']) | ||
| ->execute(); | ||
| continue; | ||
| } | ||
|
|
||
| // If no email exists, mark activity for to be scheduled and send the email | ||
| $updateResult = Activity::update(FALSE) | ||
| ->addValue('status_id:name', 'To be scheduled') | ||
| ->addWhere('id', '=', $activity['id']) | ||
| ->execute(); | ||
|
|
||
| if ($updateResult->isError()) { | ||
| continue; | ||
| } | ||
|
|
||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
|
|
||
| $emailResult = civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
|
|
||
| $offset += $batchSize; | ||
|
|
||
| } while (count($notVisitedInductionActivities) === $batchSize); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in sendInductionRescheduleEmail method.
The method lacks proper error handling for the email sending operation and template retrieval.
public static function sendInductionRescheduleEmail() {
+ try {
$template = MessageTemplate::get(FALSE)
->addSelect('id', 'msg_subject')
->addWhere('msg_title', 'LIKE', 'Induction_reschedule_slot_booking%')
->execute()->single();
+
+ if (!$template) {
+ throw new \Exception('Reschedule email template not found');
+ }
// ... rest of the method ...
$emailResult = civicrm_api3('Email', 'send', $emailParams);
+ if ($emailResult['is_error']) {
+ \Civi::log()->error('Failed to send reschedule email', [
+ 'contact_id' => $activity['source_contact_id'],
+ 'error' => $emailResult['error_message']
+ ]);
+ }
+ } catch (\Exception $e) {
+ \Civi::log()->error('Error in sendInductionRescheduleEmail: ' . $e->getMessage());
+ throw $e;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| * Handles induction creation for a volunteer. | ||
| */ | ||
| public static function createInductionForVolunteer(string $op, string $objectName, int $objectId, &$objectRef) { | ||
|
|
| ->setOffset($offset) | ||
| ->execute(); | ||
|
|
||
|
|
2 participants
Summary by CodeRabbit
New Features
Improvements