Added cron to update induction status and send followup email#415
Added cron to update induction status and send followup email#415nishant22029 merged 11 commits intodevelopfrom
Conversation
WalkthroughThis pull request introduces two new API files for managing induction slot bookings and updating induction statuses. 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: 2
🧹 Outside diff range and nitpick comments (6)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php (3)
56-91: Ensure consistent batch processing without skipping recordsThe current batch processing logic increments the offset by the batch size (
$offset += $batchSize), which may lead to records being skipped or processed multiple times if new records are added or existing ones are deleted during execution. This can cause inconsistent results in dynamic datasets.To improve reliability, consider using a different batching strategy, such as processing based on the last processed ID or using a cursor. This helps ensure that all records are processed accurately, even if the underlying data changes during execution.
38-38: Enhance log messages for better traceabilityThe current log message
'Check for unscheduled induction activities'is generic. Enhancing log messages with more context, such as timestamps or identifiers, can aid in debugging and monitoring.Consider updating the log message to include additional details.
Apply this diff to improve the log message:
\Civi::log()->info('Check for unscheduled induction activities'); +\Civi::log()->info('Starting induction slot booking follow-up cron at ' . date('Y-m-d H:i:s'));
93-97: Catch specific exceptions instead of genericExceptionCatching the generic
Exceptionclass can obscure the actual exceptions that need handling and may make debugging more difficult. It's a best practice to catch specific exceptions that the code is expected to handle.Consider catching more specific exceptions, such as
CRM_Core_Exception.Apply this diff to catch a more specific exception:
-} catch (Exception $e) { +} catch (CRM_Core_Exception $e) { // Log any errors encountered during the process. \Civi::log()->error('Error in follow-up cron: ' . $e->getMessage()); return civicrm_api3_create_error($e->getMessage()); }wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php (3)
15-17: Consider adding a brief description of the purpose of this function.While the function name is self-explanatory, adding a brief description of the purpose of this function can enhance the code's readability and maintainability.
/** * Goonjcustom.Induction Status Update to No Show API specification (optional) * This is used for documentation and validation. * + * This function defines the API specification for the Goonjcustom.update_induction_status_no_show_cron API. + * It specifies that there are no parameters for this cron job. + * * @param array $spec * Description of fields supported by this API call. * * @return void */ function _civicrm_api3_goonjcustom_update_induction_status_no_show_cron_spec(&$spec) { // There are no parameters for the Goonjcustom cron. }
38-39: Consider extracting the follow-up days as a constant or configuration variable.Extracting the follow-up days as a constant or configuration variable can make it easier to adjust the value in the future and improve code readability.
+define('INDUCTION_STATUS_UPDATE_FOLLOW_UP_DAYS', 30); -$followUpDays = 30; +$followUpDays = INDUCTION_STATUS_UPDATE_FOLLOW_UP_DAYS; $followUpTimestamp = strtotime("-$followUpDays days");
41-42: Consider extracting the batch size as a constant or configuration variable.Extracting the batch size as a constant or configuration variable can make it easier to adjust the value in the future and improve code readability.
+define('INDUCTION_STATUS_UPDATE_BATCH_SIZE', 25); -$batchSize = 25; +$batchSize = INDUCTION_STATUS_UPDATE_BATCH_SIZE; $offset = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php(1 hunks)
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php (4)
1-119: Excellent work on implementing the induction status update cron job!
The code is well-structured, follows best practices, and includes appropriate error handling. The use of API4 for data retrieval and updates is a great choice. The logging statements provide valuable insights into the execution flow and any issues encountered.
A few minor suggestions for further improvement:
- Consider extracting the batch size and follow-up days as constants or configuration variables to make them more easily adjustable.
- Add a brief description of the purpose and functionality of this cron job as a docblock comment at the beginning of the file.
Overall, this is a solid implementation that effectively addresses the requirements. Great job!
94-98: LGTM!
The code for updating the induction activity status to "No_show" is implemented correctly using the Activity API.
110-114: LGTM!
The error handling using a try-catch block and logging of errors is implemented correctly. Returning an error response using civicrm_api3_create_error is the appropriate approach.
116-118: LGTM!
The success logging and return of a success response using civicrm_api3_create_success is implemented correctly.
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | ||
| ->setLimit(1) | ||
| ->execute() | ||
| ->single(); |
There was a problem hiding this comment.
Handle the case where the email template is not found
The code assumes that the email template is always found. If the template is missing or misnamed, single() may return NULL, leading to a fatal error when accessing $template['msg_subject'].
Add a check to ensure the template exists before proceeding.
Apply this diff to handle the missing template scenario:
$template = MessageTemplate::get(FALSE)
->addSelect('id', 'msg_subject')
->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%')
->setLimit(1)
->execute()
->single();
+if (!$template) {
+ throw new CRM_Core_Exception('Email template "Induction_slot_booking_follow_up_email" not found.');
+}
📝 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.
| $template = MessageTemplate::get(FALSE) | |
| ->addSelect('id', 'msg_subject') | |
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | |
| ->setLimit(1) | |
| ->execute() | |
| ->single(); | |
| $template = MessageTemplate::get(FALSE) | |
| ->addSelect('id', 'msg_subject') | |
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | |
| ->setLimit(1) | |
| ->execute() | |
| ->single(); | |
| if (!$template) { | |
| throw new CRM_Core_Exception('Email template "Induction_slot_booking_follow_up_email" not found.'); | |
| } |
| foreach ($unscheduledInductionActivities as $activity) { | ||
| // Check if an followup 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']) | ||
| ->setLimit(1) | ||
| ->execute()->first(); | ||
|
|
||
| if (!$emailActivities) { | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| $emailResult = civicrm_api3('Email', 'send', $emailParams); | ||
| \Civi::log()->info('Follow-up email sent', ['result' => $emailResult]); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries within the processing loop
Currently, the code performs a separate database query for each activity to check if a follow-up email has already been sent (Activity::get inside the loop). This can lead to performance issues due to the N+1 query problem, especially when processing a large number of activities.
To improve performance, consider retrieving all relevant email activities in a single query before the loop and using an in-memory lookup to check for existing emails.
Apply this diff to refactor the code:
try {
// Retrieve the email template for follow-up.
$template = MessageTemplate::get(FALSE)
->addSelect('id', 'msg_subject')
->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%')
->setLimit(1)
->execute()
->single();
+ // Retrieve all contacts who have already received the follow-up email.
+ $contactsWithEmail = Activity::get(FALSE)
+ ->addSelect('source_contact_id')
+ ->addWhere('activity_type_id:name', '=', 'Email')
+ ->addWhere('subject', '=', $template['msg_subject'])
+ ->execute()
+ ->column('source_contact_id');
+
do {
// Retrieve a batch of unscheduled induction activities older than 7 days
$unscheduledInductionActivities = Activity::get(FALSE)
->addSelect('id', 'source_contact_id', 'created_date')
->addWhere('activity_type_id:name', '=', 'Induction')
->addWhere('status_id:name', '=', 'To be scheduled')
->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp))
+ ->addWhere('source_contact_id', 'NOT IN', $contactsWithEmail)
->setLimit($batchSize)
->setOffset($offset)
->execute();
// Process each activity in the batch
foreach ($unscheduledInductionActivities 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'])
- ->setLimit(1)
- ->execute()->first();
- if (!$emailActivities) {
$emailParams = [
'contact_id' => $activity['source_contact_id'],
'template_id' => $template['id'],
];
$emailResult = civicrm_api3('Email', 'send', $emailParams);
\Civi::log()->info('Follow-up email sent', ['result' => $emailResult]);
- }
}
// Move to the next batch by increasing the offset
$offset += $batchSize;
} while (count($unscheduledInductionActivities) === $batchSize);
} catch (Exception $e) {
// Log any errors encountered during the process.
\Civi::log()->error('Error in follow-up cron: ' . $e->getMessage());
return civicrm_api3_create_error($e->getMessage());
}Committable suggestion skipped: line range outside the PR's diff.
| */ | ||
| function civicrm_api3_goonjcustom_induction_slot_booking_follow_up_cron($params) { | ||
| $returnValues = []; | ||
| \Civi::log()->info('Check for unscheduled induction activities'); |
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', '=', $template['msg_subject']) | ||
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | ||
| ->execute()->single(); | ||
|
|
||
| if (!$emailActivities) { | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| $emailResult = civicrm_api3('Email', 'send', $emailParams); | ||
| \Civi::log()->info('Follow-up email sent', ['result' => $emailResult]); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // Move to the next batch by increasing the offset | ||
| $offset += $batchSize; | ||
|
|
||
| } while (count($unscheduledInductionActivities) === $batchSize); |
There was a problem hiding this comment.
Move the entire foreach logic to the InductionService file. Use the cron file solely for setting up the cron job, and transfer all functionality to the service file. Refer to CollectionCampCron.php as an example of how this structure is implemented.
| 'template_id' => $template['id'], | ||
| ]; | ||
| $emailResult = civicrm_api3('Email', 'send', $emailParams); | ||
| \Civi::log()->info('Follow-up email sent', ['result' => $emailResult]); |
| $followUpDays = 7; | ||
|
|
||
| // Calculate the timestamp for 7 days ago from the current date | ||
| $followUpTimestamp = strtotime("-$followUpDays days"); |
There was a problem hiding this comment.
this line returns a Unix timestamp representing the date that is 7 days older than the current date.
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'To be scheduled') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) |
There was a problem hiding this comment.
If I create an unscheduled induction activity for Oct 29 and today’s date is November 5, will I receive a notification? Can you confirm this requirement and if yes then update the logic from < to <=
There was a problem hiding this comment.
As per requirement its older than 7 days not equal to 7 days
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'To be scheduled') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) |
There was a problem hiding this comment.
Have you also noted the hours here?
| ->addSelect('source_contact_id', 'activity_date_time') | ||
| ->addWhere('subject', '=', $template['msg_subject']) | ||
| ->addWhere('activity_type_id:label', '=', 'Email') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) |
| // Fetch the associated induction activity | ||
| $inductionActivity = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'status_id:name') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| if (!$inductionActivity) { | ||
| \Civi::log()->info('No induction activity found for source contact', [ | ||
| 'source_contact_id' => $activity['source_contact_id'], |
There was a problem hiding this comment.
Move the logic to the service file. The induction service as this code relates to induction
| ->addWhere('id', '=', $inductionActivity['id']) | ||
| ->execute(); | ||
|
|
||
| \Civi::log()->info('Induction activity status updated to No_show', [ |
There was a problem hiding this comment.
For monitoring logs in case any issue occur we can check logs
| return civicrm_api3_create_error($e->getMessage()); | ||
| } | ||
|
|
||
| \Civi::log()->info('Induction slot booking follow-up cron job completed successfully.'); |
There was a problem hiding this comment.
Not needed, please remove unnecessary logs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php (3)
3-5: Remove unused importsThe
ActivityandMessageTemplateclasses are no longer used directly in this file since the logic has been moved toInductionService. Consider removing these unused imports to maintain clean code.-use Civi\Api4\Activity; -use Civi\Api4\MessageTemplate; use Civi\InductionService;
7-18: Enhance API documentationWhile the documentation follows the CiviCRM standard, it could be more descriptive about the cron job's purpose and execution frequency.
/** - * Goonjcustom.InductionSlotBookingFollowUp API specification (optional) - * This is used for documentation and validation. + * Goonjcustom.InductionSlotBookingFollowUp API specification + * This API is designed to be called as a cron job to send follow-up emails + * for unscheduled induction activities. * * @param array $spec * description of fields supported by this API call. * * @return void */ function _civicrm_api3_goonjcustom_induction_slot_booking_follow_up_cron_spec(&$spec) { - // There are no parameters for the Goonjcustom cron. + // This cron job runs automatically and requires no input parameters. }
37-48: Enhance API response with meaningful informationThe API currently returns an empty array on success. Consider returning meaningful information about the operation's results.
function civicrm_api3_goonjcustom_induction_slot_booking_follow_up_cron($params) { - $returnValues = []; + $returnValues = ['processed' => false]; try { - InductionService::sendFollowUpEmails(); + $result = InductionService::sendFollowUpEmails(); + $returnValues = [ + 'processed' => true, + 'emails_sent' => $result['sent'] ?? 0, + 'timestamp' => date('Y-m-d H:i:s'), + ]; } catch (Exception $e) { // Log any errors encountered during the process. \Civi::log()->error('Error in follow-up cron: ' . $e->getMessage()); return civicrm_api3_create_error($e->getMessage()); } return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_slot_booking_follow_up_cron'); }wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (4)
505-505: Remove debug log statementThe debug log statement
cjeckaappears to be a development artifact and should be removed.- \Civi::log()->info('cjecka');
458-461: Reduce code duplication in template retrievalThe email template retrieval logic is duplicated. Consider extracting it to a private method.
+ private static function getFollowUpEmailTemplate() { + return MessageTemplate::get(FALSE) + ->addSelect('id', 'msg_subject') + ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') + ->execute()->single(); + }Then use it in both methods:
- $template = MessageTemplate::get(FALSE) - ->addSelect('id', 'msg_subject') - ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') - ->execute()->single(); + $template = self::getFollowUpEmailTemplate();Also applies to: 513-516
532-567: Simplify complex nested logicThe nested logic for processing induction activities could be simplified by extracting the processing logic into a separate method.
+ private static function processInductionActivity($activity) { + $inductionActivity = Activity::get(FALSE) + ->addSelect('id', 'source_contact_id', 'status_id:name') + ->addWhere('activity_type_id:name', '=', 'Induction') + ->addWhere('source_contact_id', '=', $activity['source_contact_id']) + ->execute() + ->single(); + + if (!$inductionActivity) { + \Civi::log()->info('No induction activity found for source contact', [ + 'source_contact_id' => $activity['source_contact_id'], + ]); + return; + } + + if (in_array($inductionActivity['status_id:name'], ['Completed', 'Scheduled'])) { + \Civi::log()->info('Induction activity is already completed or scheduled, skipping', [ + 'inductionActivityId' => $inductionActivity['id'], + 'status' => $inductionActivity['status_id:name'] + ]); + return; + } + + $updateResult = Activity::update(FALSE) + ->addValue('status_id:name', 'No_show') + ->addWhere('id', '=', $inductionActivity['id']) + ->execute(); + + \Civi::log()->info('Induction activity status updated to No_show', [ + 'inductionActivityId' => $inductionActivity['id'], + 'updateResult' => $updateResult + ]); + }Then simplify the loop:
foreach ($followUpEmailActivities as $activity) { - // Current complex nested logic + self::processInductionActivity($activity); }
450-578: Consider implementing rate limitingBoth methods process activities in batches but lack rate limiting for API calls. Consider adding a delay between batches to prevent overwhelming the system.
Consider adding a small delay between batches:
} while (count($unscheduledInductionActivities) === $batchSize); + // Add a small delay between batches + usleep(100000); // 100ms delay
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php (1)
41-45: LGTM! Robust error handling implementation.
The error handling follows best practices by:
- Using try-catch for exception handling
- Properly logging errors with context
- Returning standardized API error responses
| $followUpDays = 7; | ||
| $followUpTimestamp = strtotime("-{$followUpDays} days"); | ||
|
|
||
| // Retrieve the email template for follow-up. | ||
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | ||
| ->execute()->single(); | ||
|
|
||
| $batchSize = 25; | ||
| $offset = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert magic numbers to configurable constants
The follow-up days and batch size should be defined as class constants for better maintainability and reusability.
+ const FOLLOW_UP_DAYS = 7;
+ const BATCH_SIZE = 25;
- $followUpDays = 7;
- $batchSize = 25;Committable suggestion skipped: line range outside the PR's diff.
| if (!$emailActivities) { | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for email sending operations
The email sending operation lacks error handling, which could lead to silent failures.
if (!$emailActivities) {
$emailParams = [
'contact_id' => $activity['source_contact_id'],
'template_id' => $template['id'],
];
- civicrm_api3('Email', 'send', $emailParams);
+ try {
+ $result = civicrm_api3('Email', 'send', $emailParams);
+ if ($result['is_error']) {
+ \Civi::log()->error('Failed to send follow-up email', [
+ 'contact_id' => $activity['source_contact_id'],
+ 'error' => $result['error_message']
+ ]);
+ }
+ } catch (\Exception $e) {
+ \Civi::log()->error('Exception while sending follow-up email', [
+ 'contact_id' => $activity['source_contact_id'],
+ 'error' => $e->getMessage()
+ ]);
+ }
}📝 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.
| if (!$emailActivities) { | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| civicrm_api3('Email', 'send', $emailParams); | |
| } | |
| } | |
| if (!$emailActivities) { | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| try { | |
| $result = civicrm_api3('Email', 'send', $emailParams); | |
| if ($result['is_error']) { | |
| \Civi::log()->error('Failed to send follow-up email', [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'error' => $result['error_message'] | |
| ]); | |
| } | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Exception while sending follow-up email', [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'error' => $e->getMessage() | |
| ]); | |
| } | |
| } | |
| } |
| * | ||
| */ | ||
| public static function updateInductionStatusNoShow() { | ||
| \Civi::log()->info('cjecka'); |
There was a problem hiding this comment.
Please remove unnecessary logs.
| ->addWhere('id', '=', $inductionActivity['id']) | ||
| ->execute(); | ||
|
|
||
| \Civi::log()->info('Induction activity status updated to No_show', [ |
There was a problem hiding this comment.
If you are using the try-catch, why are you adding logs everywhere? Let's use the catch block to fetch the necessary logs you want in case the code won't run.
Whenever this cron runs every time these logs print and I expect we don't need that we only need the logs when the code won't run right?
| } while (count($followUpEmailActivities) === $batchSize); | ||
|
|
||
| } catch (\Exception $e) { | ||
| // Log any errors encountered during the process |
There was a problem hiding this comment.
Delete this comment—your code is shouting what it’s doing!
| } catch (\Exception $e) { | ||
| // Log any errors encountered during the process | ||
| \Civi::log()->error('Error in updating induction status: ' . $e->getMessage()); | ||
| throw $e; // Re-throw the exception to handle it at a higher level |
| try { | ||
| InductionService::sendFollowUpEmails(); | ||
| } catch (Exception $e) { | ||
| // Log any errors encountered during the process. |
There was a problem hiding this comment.
Delete this comment—your code is shouting what it’s doing! AND please remove these comments like add logs, Log any errors encountered during the process.
| try { | ||
| InductionService::updateInductionStatusNoShow(); | ||
| } catch (\Exception $e) { | ||
| // Log any errors encountered during the process |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3)
453-453: Add PHPDoc block for the method.The method lacks documentation describing its purpose, parameters, and return value.
Add a descriptive PHPDoc block:
/** * Sends follow-up emails to volunteers who haven't scheduled their induction. * * Processes unscheduled induction activities older than 7 days in batches, * sending follow-up emails if not already sent. * * @return void */ public static function sendFollowUpEmails() {
504-504: Add PHPDoc block for the method.The method lacks documentation describing its purpose, parameters, and return value.
Add a descriptive PHPDoc block:
/** * Updates induction status to "No_show" for activities that haven't been completed * or scheduled within 30 days after the follow-up email. * * @throws \Exception When the follow-up email template is not found or on API errors * @return void */ public static function updateInductionStatusNoShow() {
541-554: Remove unnecessary logging of normal flow conditions.The logging of normal flow conditions adds noise to the logs. Only exceptional conditions should be logged.
Remove or adjust the logging:
- if (!$inductionActivity) { - \Civi::log()->info('No induction activity found for source contact', [ - 'source_contact_id' => $activity['source_contact_id'], - ]); + if (!$inductionActivity) { continue; } // Check if the status is 'Completed' or 'Scheduled' if (in_array($inductionActivity['status_id:name'], ['Completed', 'Scheduled'])) { - \Civi::log()->info('Induction activity is already completed or scheduled, skipping', [ - 'inductionActivityId' => $inductionActivity['id'], - 'status' => $inductionActivity['status_id:name'] - ]); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionSlotBookingFollowUpCron.php
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UpdateInductionStatusNoShowCron.php
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
454-456: Convert magic numbers to class constants.
Both methods use magic numbers for follow-up days and batch size.
This issue was already identified in a previous review. The suggestion to convert these to class constants is still valid.
Also applies to: 505-507
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Add error handling for API calls.
The API calls to MessageTemplate::get and Activity::get lack error handling. If these fail, the method will throw uncaught exceptions.
Add try-catch blocks:
try {
// Retrieve the email template for follow-up.
$template = MessageTemplate::get(FALSE)
->addSelect('id', 'msg_subject')
->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%')
->execute()->single();
if (!$template) {
throw new \Exception('Follow-up email template not found.');
}
+ } catch (\Exception $e) {
+ \Civi::log()->error('Error retrieving follow-up email template: ' . $e->getMessage());
+ throw $e;
+ }
try {
// Retrieve a batch of unscheduled induction activities older than 7 days
$unscheduledInductionActivities = Activity::get(FALSE)
->addSelect('id', 'source_contact_id', 'created_date')
->addWhere('activity_type_id:name', '=', 'Induction')
->addWhere('status_id:name', '=', 'To be scheduled')
->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp))
->setLimit($batchSize)
->setOffset($offset)
->execute();
+ } catch (\Exception $e) {
+ \Civi::log()->error('Error retrieving unscheduled induction activities: ' . $e->getMessage());
+ throw $e;
+ }Also applies to: 468-475
| public static function sendFollowUpEmails() { | ||
| $followUpDays = 7; | ||
| $followUpTimestamp = strtotime("-{$followUpDays} days"); | ||
|
|
||
| // Retrieve the email template for follow-up. | ||
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | ||
| ->execute()->single(); | ||
|
|
||
| $batchSize = 25; | ||
| $offset = 0; | ||
|
|
||
| do { | ||
| // Retrieve a batch of unscheduled induction activities older than 7 days | ||
| $unscheduledInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'To be scheduled') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) | ||
| ->setLimit($batchSize) | ||
| ->setOffset($offset) | ||
| ->execute(); | ||
|
|
||
| // Process each activity in the batch | ||
| foreach ($unscheduledInductionActivities 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()->single(); | ||
|
|
||
| if (!$emailActivities) { | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
| } | ||
|
|
||
| // Move to the next batch by increasing the offset | ||
| $offset += $batchSize; | ||
|
|
||
| } while (count($unscheduledInductionActivities) === $batchSize); | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public static function updateInductionStatusNoShow() { | ||
| $followUpDays = 30; | ||
| $followUpTimestamp = strtotime("-$followUpDays days"); | ||
| $batchSize = 25; | ||
| $offset = 0; | ||
|
|
||
| try { | ||
| // Fetch the follow-up message template | ||
| $template = MessageTemplate::get(FALSE) | ||
| ->addSelect('id', 'msg_subject') | ||
| ->addWhere('msg_title', 'LIKE', 'Induction_slot_booking_follow_up_email%') | ||
| ->execute()->single(); | ||
|
|
||
| if (!$template) { | ||
| throw new \Exception('Follow-up email template not found.'); | ||
| } | ||
|
|
||
| do { | ||
| // Fetch email activities older than 30 days | ||
| $followUpEmailActivities = Activity::get(TRUE) | ||
| ->addSelect('source_contact_id', 'activity_date_time') | ||
| ->addWhere('subject', '=', $template['msg_subject']) | ||
| ->addWhere('activity_type_id:label', '=', 'Email') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) | ||
| ->setLimit($batchSize) | ||
| ->setOffset($offset)->execute(); | ||
|
|
||
| foreach ($followUpEmailActivities as $activity) { | ||
| // Fetch the associated induction activity | ||
| $inductionActivity = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'status_id:name') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('source_contact_id', '=', $activity['source_contact_id']) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| if (!$inductionActivity) { | ||
| \Civi::log()->info('No induction activity found for source contact', [ | ||
| 'source_contact_id' => $activity['source_contact_id'], | ||
| ]); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if the status is 'Completed' or 'Scheduled' | ||
| if (in_array($inductionActivity['status_id:name'], ['Completed', 'Scheduled'])) { | ||
| \Civi::log()->info('Induction activity is already completed or scheduled, skipping', [ | ||
| 'inductionActivityId' => $inductionActivity['id'], | ||
| 'status' => $inductionActivity['status_id:name'] | ||
| ]); | ||
| continue; | ||
| } | ||
|
|
||
| // Update the induction status to 'No_show' | ||
| $updateResult = Activity::update(FALSE) | ||
| ->addValue('status_id:name', 'No_show') | ||
| ->addWhere('id', '=', $inductionActivity['id']) | ||
| ->execute(); | ||
| } | ||
|
|
||
| // Increment the offset by the batch size | ||
| $offset += $batchSize; | ||
| } while (count($followUpEmailActivities) === $batchSize); | ||
|
|
||
| } catch (\Exception $e) { | ||
| \Civi::log()->error('Error in updating induction status: ' . $e->getMessage()); | ||
| throw $e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common batch processing logic.
Both methods share similar batch processing logic. This violates the DRY principle.
Create a utility method for batch processing:
+ /**
+ * Process records in batches using a callback function.
+ *
+ * @param callable $fetchBatch Function to fetch a batch of records
+ * @param callable $processBatch Function to process each batch
+ * @param int $batchSize Size of each batch
+ * @return void
+ */
+ private static function processBatches(callable $fetchBatch, callable $processBatch, int $batchSize = 25): void {
+ $maxIterations = 1000;
+ $iteration = 0;
+ $offset = 0;
+
+ do {
+ $records = $fetchBatch($batchSize, $offset);
+ $processBatch($records);
+ $offset += $batchSize;
+ $iteration++;
+ } while (count($records) === $batchSize && $iteration < $maxIterations);
+
+ if ($iteration >= $maxIterations) {
+ \Civi::log()->warning('Maximum iterations reached in batch processing');
+ }
+ }Then update both methods to use this utility:
public static function sendFollowUpEmails() {
// ... template retrieval ...
- $batchSize = 25;
- $offset = 0;
- do {
- // ... batch processing ...
- } while (count($unscheduledInductionActivities) === $batchSize);
+ self::processBatches(
+ function ($batchSize, $offset) use ($followUpTimestamp) {
+ return Activity::get(FALSE)
+ ->addSelect('id', 'source_contact_id', 'created_date')
+ // ... existing conditions ...
+ ->setLimit($batchSize)
+ ->setOffset($offset)
+ ->execute();
+ },
+ function ($activities) use ($template) {
+ foreach ($activities as $activity) {
+ // ... existing processing ...
+ }
+ }
+ );
}Committable suggestion skipped: line range outside the PR's diff.
| do { | ||
| // Retrieve a batch of unscheduled induction activities older than 7 days | ||
| $unscheduledInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('id', 'source_contact_id', 'created_date') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'To be scheduled') | ||
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) | ||
| ->setLimit($batchSize) | ||
| ->setOffset($offset) | ||
| ->execute(); | ||
|
|
||
| // Process each activity in the batch | ||
| foreach ($unscheduledInductionActivities 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()->single(); | ||
|
|
||
| if (!$emailActivities) { | ||
| $emailParams = [ | ||
| 'contact_id' => $activity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
| } | ||
|
|
||
| // Move to the next batch by increasing the offset | ||
| $offset += $batchSize; | ||
|
|
||
| } while (count($unscheduledInductionActivities) === $batchSize); |
There was a problem hiding this comment.
Add safety check to prevent infinite loop.
The do-while loop could potentially run indefinitely if the API consistently returns $batchSize records.
Add a maximum iteration limit:
+ $maxIterations = 1000; // Adjust based on expected maximum records
+ $iteration = 0;
do {
// ... existing code ...
$offset += $batchSize;
+ $iteration++;
- } while (count($unscheduledInductionActivities) === $batchSize);
+ } while (count($unscheduledInductionActivities) === $batchSize && $iteration < $maxIterations);
+
+ if ($iteration >= $maxIterations) {
+ \Civi::log()->warning('Maximum iterations reached in sendFollowUpEmails');
+ }📝 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.
| do { | |
| // Retrieve a batch of unscheduled induction activities older than 7 days | |
| $unscheduledInductionActivities = Activity::get(FALSE) | |
| ->addSelect('id', 'source_contact_id', 'created_date') | |
| ->addWhere('activity_type_id:name', '=', 'Induction') | |
| ->addWhere('status_id:name', '=', 'To be scheduled') | |
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) | |
| ->setLimit($batchSize) | |
| ->setOffset($offset) | |
| ->execute(); | |
| // Process each activity in the batch | |
| foreach ($unscheduledInductionActivities 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()->single(); | |
| if (!$emailActivities) { | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| civicrm_api3('Email', 'send', $emailParams); | |
| } | |
| } | |
| // Move to the next batch by increasing the offset | |
| $offset += $batchSize; | |
| } while (count($unscheduledInductionActivities) === $batchSize); | |
| $maxIterations = 1000; // Adjust based on expected maximum records | |
| $iteration = 0; | |
| do { | |
| // Retrieve a batch of unscheduled induction activities older than 7 days | |
| $unscheduledInductionActivities = Activity::get(FALSE) | |
| ->addSelect('id', 'source_contact_id', 'created_date') | |
| ->addWhere('activity_type_id:name', '=', 'Induction') | |
| ->addWhere('status_id:name', '=', 'To be scheduled') | |
| ->addWhere('created_date', '<', date('Y-m-d H:i:s', $followUpTimestamp)) | |
| ->setLimit($batchSize) | |
| ->setOffset($offset) | |
| ->execute(); | |
| // Process each activity in the batch | |
| foreach ($unscheduledInductionActivities 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()->single(); | |
| if (!$emailActivities) { | |
| $emailParams = [ | |
| 'contact_id' => $activity['source_contact_id'], | |
| 'template_id' => $template['id'], | |
| ]; | |
| civicrm_api3('Email', 'send', $emailParams); | |
| } | |
| } | |
| // Move to the next batch by increasing the offset | |
| $offset += $batchSize; | |
| $iteration++; | |
| } while (count($unscheduledInductionActivities) === $batchSize && $iteration < $maxIterations); | |
| if ($iteration >= $maxIterations) { | |
| \Civi::log()->warning('Maximum iterations reached in sendFollowUpEmails'); | |
| } |
2 participants
Issue 137
please refer points:
If a volunteer does not select a slot within 7 (configurable) days of receiving the scheduling email, a follow-up email shall be sent.If the volunteer does not respond to the follow-up email within 30 (configurable) days, their induction status in the CRM shall be changed to "No-show."Created new cron job
induction_slot_booking_follow_up_cronCreated new cron job
update_induction_status_no_show_cronSummary by CodeRabbit
New Features
Bug Fixes