Added Token and Cron to send email to volunteer on induction#479
Added Token and Cron to send email to volunteer on induction#479nishant22029 merged 4 commits intodevelopfrom
Conversation
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: 10
🧹 Outside diff range and nitpick comments (5)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php (1)
3-5: Remove unused importsThe
ActivityandMessageTemplateclasses are imported but never used in this file. Consider removing these imports to maintain clean code.-use Civi\Api4\Activity; -use Civi\Api4\MessageTemplate; use Civi\InductionService;wp-content/civi-extensions/goonjcustom/goonjcustom.php (2)
138-138: Add error logging for activity fetch failuresConsider adding error logging when the activity fetch fails to help with debugging issues in production.
$inductionActivities = \Civi\Api4\Activity::get(FALSE) ->addSelect('activity_date_time', 'Induction_Fields.Goonj_Office', 'Induction_Fields.Assign') ->addWhere('activity_type_id:name', '=', 'Induction') ->addWhere('source_contact_id', '=', $contactId) + ->setCheckPermissions(FALSE) ->execute(); +if ($inductionActivities === NULL) { + \Civi::log()->error('Failed to fetch induction activities for contact: ' . $contactId); +}
198-201: Add value validation before token assignmentConsider adding validation for the values before assigning them to tokens to prevent potential issues with empty or invalid data.
+ // Sanitize and validate values before assignment + $completeAddress = strip_tags($completeAddress); + $inductionOfficeCity = strip_tags($inductionOfficeCity); + $inductionAssigneeFirstName = strip_tags($inductionAssigneeFirstName); + $inductionAssigneePhone = preg_replace('/[^0-9]/', '', $inductionAssigneePhone); + $row->tokens('contact', 'inductionOfficeAddress', $completeAddress); $row->tokens('contact', 'inductionOfficeCity', $inductionOfficeCity); $row->tokens('contact', 'inductionAssigneeFirstName', $inductionAssigneeFirstName); $row->tokens('contact', 'inductionAssigneePhone', $inductionAssigneePhone);wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (2)
192-197: Use Constants for Template Names to Enhance MaintainabilityHardcoding template names can lead to errors and complicate future updates. Consider defining constants for
'New_Volunteer_Registration%'and'New_Volunteer_Registration_Online%'. This practice aligns with the DRY (Don't Repeat Yourself) principle and makes the codebase easier to manage.
763-808: SimplifyfetchTypeOfInductionLogic for ClarityThe
fetchTypeOfInductionmethod contains complex conditional logic and multiple return statements, which can be difficult to follow. Consider refactoring the method to improve readability:
- Consolidate conditions using clear boolean expressions.
- Use early returns to reduce nested conditions.
- Extract sections of code into well-named helper methods if necessary.
Refactoring will enhance understandability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(3 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(3 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (2)
70-74: LGTM: Token registration aligns with requirements
The new tokens for induction office details and assignee information are well-defined and match the PR objectives for including dynamic data in emails.
Line range hint 1-204: Verify integration with the reminder email cron job
The PR objectives mention implementing a cron job for sending reminder emails, but this file only handles token registration. Let's verify the integration between this token system and the cron job implementation.
✅ Verification successful
Token registration is properly integrated with the reminder system
The verification shows that the token system is correctly integrated with the reminder functionality:
- Tokens are properly registered in
goonjcustom.php - The cron job implementation in
InductionRemainderEmailCron.phpuses theInductionServiceclass - The token values are populated with relevant induction details including office address, city, and assignee information
- The reminder system is implemented as an API endpoint that can be scheduled via CiviCRM's job scheduler
The implementation follows CiviCRM's best practices for token registration and cron job implementation. The tokens are available for use in message templates, and the reminder system can utilize them through the standard token replacement mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for cron job implementation and its usage of these tokens
# Look for cron job implementation files
echo "Searching for cron job implementation files..."
fd -e php -e inc "cron|reminder|schedule"
# Look for token usage in those files
echo "Checking token usage in relevant files..."
rg -l "inductionOfficeAddress|inductionOfficeCity|inductionAssigneeFirstName|inductionAssigneePhone" $(fd -e php -e inc "cron|reminder|schedule")
Length of output: 24947
Script:
#!/bin/bash
# Let's check the content of the induction reminder cron file specifically
echo "Checking induction reminder cron implementation..."
cat wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php
# Also check for any message templates that might be using these tokens
echo -e "\nChecking for message templates using the tokens..."
rg "inductionOfficeAddress|inductionOfficeCity|inductionAssigneeFirstName|inductionAssigneePhone" wp-content/civi-extensions/goonjcustom/
Length of output: 3589
Script:
#!/bin/bash
# Let's check the InductionService class to see how these tokens are used in the reminder emails
echo "Checking InductionService implementation..."
rg -A 10 "class InductionService" wp-content/civi-extensions/goonjcustom/
# Also check for any scheduled job configurations that might be using this cron
echo -e "\nChecking for scheduled job configurations..."
fd -e xml -e php "job" wp-content/civi-extensions/goonjcustom/
Length of output: 1434
Script:
#!/bin/bash
# Let's check the managed directory for job definitions and the InductionService's sendRemainderEmails method
echo "Checking managed directory for job definitions..."
fd . wp-content/civi-extensions/goonjcustom/managed/
echo -e "\nChecking InductionService's sendRemainderEmails implementation..."
rg -A 20 "sendRemainderEmails" wp-content/civi-extensions/goonjcustom/
# Also check for message template definitions
echo -e "\nChecking for message template definitions..."
fd . wp-content/civi-extensions/goonjcustom/xml/
Length of output: 1851
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
10-10: Import Statement Confirmed
The StateProvince class is appropriately imported and utilized in the code.
| function _civicrm_api3_goonjcustom_induction_remainder_email_cron_spec(&$spec) { | ||
| // There are no parameters for the Goonjcustom cron. | ||
| } |
There was a problem hiding this comment.
Fix typo in function names: "Remainder" should be "Reminder"
The word "Remainder" is consistently misspelled throughout the file. It should be "Reminder" as we're dealing with reminder emails, not mathematical remainders.
Apply these changes:
-function _civicrm_api3_goonjcustom_induction_remainder_email_cron_spec(&$spec)
+function _civicrm_api3_goonjcustom_induction_reminder_email_cron_spec(&$spec)
-function civicrm_api3_goonjcustom_induction_remainder_email_cron($params)
+function civicrm_api3_goonjcustom_induction_reminder_email_cron($params)
- \Civi::log()->error('Error in Remainder Email cron: ' . $e->getMessage());
+ \Civi::log()->error('Error in Reminder Email cron: ' . $e->getMessage());
- return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_remainder_email_cron');
+ return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_reminder_email_cron');Also applies to: 37-47
| function civicrm_api3_goonjcustom_induction_remainder_email_cron($params) { | ||
| $returnValues = []; | ||
| try { | ||
| InductionService::sendRemainderEmails(); | ||
| } catch (Exception $e) { | ||
| \Civi::log()->error('Error in Remainder Email cron: ' . $e->getMessage()); | ||
| return civicrm_api3_create_error($e->getMessage()); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_remainder_email_cron'); | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and return values
The current implementation could be improved in several ways:
- The return values array is empty, making it difficult to track what happened
- Error handling could be more specific about what failed
Consider this improved implementation:
function civicrm_api3_goonjcustom_induction_reminder_email_cron($params) {
- $returnValues = [];
+ $returnValues = [
+ 'processed' => 0,
+ 'sent' => 0,
+ ];
try {
- InductionService::sendRemainderEmails();
+ $result = InductionService::sendReminderEmails();
+ $returnValues['processed'] = $result['processed'] ?? 0;
+ $returnValues['sent'] = $result['sent'] ?? 0;
} catch (Exception $e) {
- \Civi::log()->error('Error in Remainder Email cron: ' . $e->getMessage());
+ \Civi::log()->error('Failed to send induction reminder emails: ' . $e->getMessage(), [
+ 'exception' => $e,
+ 'params' => $params,
+ ]);
return civicrm_api3_create_error($e->getMessage());
}
- return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_remainder_email_cron');
+ return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'induction_reminder_email_cron');
}Committable suggestion skipped: line range outside the PR's diff.
| function civicrm_api3_goonjcustom_induction_remainder_email_cron($params) { | ||
| $returnValues = []; | ||
| try { | ||
| InductionService::sendRemainderEmails(); |
There was a problem hiding this comment.
💡 Codebase verification
Fix typo: "Remainder" should be "Reminder" across the codebase
The code inspection reveals several instances where "Remainder" is incorrectly used instead of "Reminder":
-
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php:- File name should be renamed to
InductionReminderEmailCron.php - Function
_civicrm_api3_goonjcustom_induction_remainder_email_cron_spec - Function
civicrm_api3_goonjcustom_induction_remainder_email_cron - Method call
InductionService::sendRemainderEmails() - Error message "Error in Remainder Email cron"
- API endpoint name "induction_remainder_email_cron"
- File name should be renamed to
-
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php:- Method name
sendRemainderEmails() - Template name
'Remainder_For_Volunteer_Induction_Scheduled_for_Today%' - Template name
'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%'
- Method name
All these instances should be updated to use "Reminder" instead of "Remainder" for consistency and correctness.
🔗 Analysis chain
Verify consistent naming across the codebase
Let's ensure the "Remainder" to "Reminder" rename is applied consistently and that the InductionService class is properly implemented.
Fix typo: "Remainder" should be "Reminder" across the codebase
The search results show multiple instances of "Remainder" that need to be corrected to "Reminder" for consistency:
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php:- File name should be renamed to
InductionReminderEmailCron.php - Function
_civicrm_api3_goonjcustom_induction_remainder_email_cron_spec - Function
civicrm_api3_goonjcustom_induction_remainder_email_cron - Method call
InductionService::sendRemainderEmails() - Error message "Error in Remainder Email cron"
- API endpoint name "induction_remainder_email_cron"
- File name should be renamed to
Let's verify the InductionService implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of "Remainder" that need to be fixed
rg -i "remainder.*email" --type php
# Verify the InductionService implementation
ast-grep --pattern 'class InductionService {
$$$
sendRemainderEmails() {
$$$
}
$$$
}'
Length of output: 10588
Script:
# Find the InductionService class definition
fd -e php InductionService
Length of output: 93
Script:
# Check the implementation of InductionService
cat wp-content/civi-extensions/goonjcustom/Civi/InductionService.php
Length of output: 26524
| $completeAddress = ''; | ||
| $inductionOfficeCity = ''; | ||
| $inductionAssigneeFirstName = ''; | ||
| $inductionAssigneePhone = '70423 99564'; |
There was a problem hiding this comment.
Remove hardcoded phone number
The fallback phone number '70423 99564' should not be hardcoded. Consider moving this to a configuration setting.
- $inductionAssigneePhone = '70423 99564';
+ $inductionAssigneePhone = Civi::settings()->get('default_induction_coordinator_phone');Committable suggestion skipped: line range outside the PR's diff.
| $addresses = \Civi\Api4\Address::get( false ) | ||
| ->addWhere( 'contact_id', '=', $officeDetails['id'] ) | ||
| ->addWhere( 'is_primary', '=', true ) | ||
| ->setLimit( 1 ) | ||
| ->execute(); | ||
|
|
||
| $address = $addresses->count() > 0 ? $addresses->first() : null; | ||
| $completeAddress = CRM_Utils_Address::format($address); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract address formatting logic
Consider extracting the address fetching and formatting logic into a separate method to improve readability and reusability.
+/**
+ * Fetch and format the primary address for a contact
+ *
+ * @param int $contactId The ID of the contact
+ * @return string The formatted address
+ */
+private function getFormattedPrimaryAddress($contactId) {
+ $addresses = \Civi\Api4\Address::get(false)
+ ->addWhere('contact_id', '=', $contactId)
+ ->addWhere('is_primary', '=', true)
+ ->setLimit(1)
+ ->execute();
+
+ $address = $addresses->count() > 0 ? $addresses->first() : null;
+ return CRM_Utils_Address::format($address);
+}
- $addresses = \Civi\Api4\Address::get( false )
- ->addWhere( 'contact_id', '=', $officeDetails['id'] )
- ->addWhere( 'is_primary', '=', true )
- ->setLimit( 1 )
- ->execute();
-
- $address = $addresses->count() > 0 ? $addresses->first() : null;
- $completeAddress = CRM_Utils_Address::format($address);
+ $completeAddress = $this->getFormattedPrimaryAddress($officeDetails['id']);📝 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.
| $addresses = \Civi\Api4\Address::get( false ) | |
| ->addWhere( 'contact_id', '=', $officeDetails['id'] ) | |
| ->addWhere( 'is_primary', '=', true ) | |
| ->setLimit( 1 ) | |
| ->execute(); | |
| $address = $addresses->count() > 0 ? $addresses->first() : null; | |
| $completeAddress = CRM_Utils_Address::format($address); | |
| /** | |
| * Fetch and format the primary address for a contact | |
| * | |
| * @param int $contactId The ID of the contact | |
| * @return string The formatted address | |
| */ | |
| private function getFormattedPrimaryAddress($contactId) { | |
| $addresses = \Civi\Api4\Address::get(false) | |
| ->addWhere('contact_id', '=', $contactId) | |
| ->addWhere('is_primary', '=', true) | |
| ->setLimit(1) | |
| ->execute(); | |
| $address = $addresses->count() > 0 ? $addresses->first() : null; | |
| return CRM_Utils_Address::format($address); | |
| } | |
| $completeAddress = $this->getFormattedPrimaryAddress($officeDetails['id']); |
| public static function sendReminderEmails() | ||
| { | ||
| $startOfDay = (new \DateTime())->setTime(0, 0, 0); | ||
| $endOfDay = (new \DateTime())->setTime(23, 59, 59); | ||
|
|
||
| $scheduledInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('source_contact_id', 'Induction_Fields.Assign') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'Scheduled') | ||
| ->addWhere('activity_date_time', '>=', $startOfDay->format('Y-m-d H:i:s')) | ||
| ->addWhere('activity_date_time', '<=', $endOfDay->format('Y-m-d H:i:s')) | ||
| ->execute(); | ||
|
|
||
| foreach ($scheduledInductionActivities as $scheduledInductionActivity) { | ||
| try { | ||
| $inductionType = self::fetchTypeOfInduction($scheduledInductionActivity['source_contact_id']); | ||
|
|
||
| $templateName = ($inductionType === 'Offline') | ||
| ? 'Remainder_For_Volunteer_Induction_Scheduled_for_Today%' | ||
| : 'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%'; | ||
|
|
||
| $template = MessageTemplate::get(FALSE) | ||
| ->addWhere('msg_title', 'LIKE', $templateName) | ||
| ->setLimit(1) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| $searchableSubject = str_replace('{contact.first_name}', '%', $template['msg_subject']); | ||
|
|
||
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', 'LIKE', $searchableSubject) | ||
| ->addWhere('source_contact_id', '=', $scheduledInductionActivity['source_contact_id']) | ||
| ->execute(); | ||
|
|
||
| $emailActivity = $emailActivities->first(); | ||
|
|
||
| if (!$emailActivity) { | ||
| $emailParams = [ | ||
| 'contact_id' => $scheduledInductionActivity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
| $result = civicrm_api3('Email', 'send', $emailParams); | ||
| } | ||
| } catch (\Exception $e) { | ||
| // Log the error for debugging purposes | ||
| \Civi::log()->error('Failed to send reminder email', [ | ||
| 'contact_id' => $scheduledInductionActivity['source_contact_id'], | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor sendReminderEmails for Single Responsibility
The sendReminderEmails method is substantial and combines multiple responsibilities, such as fetching activities, processing templates, and sending emails. To adhere to the Single Responsibility Principle, consider breaking this method into smaller, focused methods. For example:
- A method to retrieve scheduled induction activities.
- A method to fetch the appropriate email template.
- A method to check if a reminder email has already been sent.
- A method to send the email.
Refactoring will improve readability and maintainability.
| $searchableSubject = str_replace('{contact.first_name}', '%', $template['msg_subject']); | ||
|
|
||
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', 'LIKE', $searchableSubject) | ||
| ->addWhere('source_contact_id', '=', $scheduledInductionActivity['source_contact_id']) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Email Sent Verification Logic
Replacing '{contact.first_name}' with '%' in the subject line assumes a specific token structure. If additional tokens are introduced, this method may fail. Consider a more reliable approach:
- Use a unique identifier or a flag in the activity to track sent emails.
- Store the email activity ID after sending and check against it.
- Utilize a dedicated field or status to indicate that the reminder has been sent.
💡 Codebase verification
Subject Line Token Replacement Needs Enhancement
The current implementation of replacing only {contact.first_name} with % in the subject line is insufficient. Based on the codebase analysis, there are multiple contact-related tokens that can be used in message templates. The code should handle all possible contact tokens in the subject line to ensure reliable email activity matching.
Key findings:
- The codebase shows extensive token support including various contact-related tokens like
prefix_id,suffix_id, and custom tokens - Message templates can use any of these tokens in subject lines
- Current implementation only handles
{contact.first_name}, which could lead to false negatives in email activity matching
Suggested approach:
- Replace all
{contact.*}tokens with%in the subject line search pattern - Or use a more robust activity matching strategy that doesn't rely on subject line pattern matching
🔗 Analysis chain
Verify Potential Impact of Subject Line Tokens
The method of checking if an email has been sent relies on replacing '{contact.first_name}' with '%' in the subject. This assumes that only the first name token is present. If additional tokens are introduced, this check may fail.
Run the following script to search for other tokens used in email subjects:
Please review the results to ensure the current approach safely handles all possible tokens.
Let me try a different approach to find token usage patterns in the codebase. This will help us identify if there are other tokens being used in subject lines or message templates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all tokens used in email subject lines
# Search for templates with tokens in the subject
ast-grep --pattern '$subject = $'MessageTemplate::get(FALSE)->addSelect('msg_subject')->addWhere('msg_subject', 'LIKE', '%{%}%')->execute();'
# Expected result: List of subjects with all tokens used
Length of output: 324
Script:
#!/bin/bash
# Search for message templates with tokens in the subject line
rg -g '*.php' '{.*?}' -A 2 -B 2
Length of output: 114
Script:
#!/bin/bash
# Search specifically in the InductionService file for token replacements
rg -g 'InductionService.php' "str_replace.*?\{.*?\}"
Length of output: 218
Script:
#!/bin/bash
# Look for message template definitions or configurations
fd -e php -e json -e xml --exec grep -l "msg_subject" {}
Length of output: 2385
Script:
#!/bin/bash
# Let's check the message template files for subject line token patterns
rg -g '*.php' "msg_subject.*?{" -B 2 -A 2
# Also search for token replacement patterns in message templates
rg -g '*.php' "MessageTemplate.*?subject.*?{.*?}" -B 2 -A 2
Length of output: 344
Script:
#!/bin/bash
# Check the specific files that might contain message template definitions
cat "./wp-content/plugins/civicrm/civicrm/CRM/Core/BAO/MessageTemplate.php" | grep -A 5 -B 5 "msg_subject"
Length of output: 2089
Script:
#!/bin/bash
# Look for any token-related configurations or definitions
rg -g '*.php' "token.*?contact" -B 2 -A 2
Length of output: 73597
| ->addWhere('name', 'IN', ['Bihar', 'Jharkhand', 'Orissa']) | ||
| ->execute() |
There was a problem hiding this comment.
Update State and City Names for Accurate Matching
- Line 780: The state
'Orissa'should be updated to'Odisha'to reflect its official name. - Line 787: The city
'bhubaneshwar'should be corrected to'bhubaneswar'.
Using accurate names ensures correct matching in queries and prevents potential bugs due to misspellings.
Apply this diff to correct the names:
- ->addWhere('name', 'IN', ['Bihar', 'Jharkhand', 'Orissa'])
+ ->addWhere('name', 'IN', ['Bihar', 'Jharkhand', 'Odisha'])- if (in_array($contactCity, ['patna', 'ranchi', 'bhubaneshwar'])) {
+ if (in_array($contactCity, ['patna', 'ranchi', 'bhubaneswar'])) {Also applies to: 787-787
| ? 'Remainder_For_Volunteer_Induction_Scheduled_for_Today%' | ||
| : 'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%'; |
There was a problem hiding this comment.
Correct Typographical Error in Template Names
The template names contain a typo: 'Remainder' should be 'Reminder'. This typo could lead to failed template retrievals and emails not being sent, impacting the communication with volunteers.
Apply this diff to correct the typo:
- ? 'Remainder_For_Volunteer_Induction_Scheduled_for_Today%'
- : 'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%';
+ ? 'Reminder_For_Volunteer_Induction_Scheduled_for_Today%'
+ : 'Reminder_For_Volunteer_Induction_Scheduled_Online_for_Today%';📝 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.
| ? 'Remainder_For_Volunteer_Induction_Scheduled_for_Today%' | |
| : 'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%'; | |
| ? 'Reminder_For_Volunteer_Induction_Scheduled_for_Today%' | |
| : 'Reminder_For_Volunteer_Induction_Scheduled_Online_for_Today%'; |
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/InductionRescheduleCron.php (3)
Line range hint
1-18: Enhance API specification documentationThe specification function could be more explicit about why there are no parameters. Consider adding a comment explaining that this is an automated cron job that doesn't require user input.
function _civicrm_api3_goonjcustom_induction_reschedule_cron_spec(&$spec) { - // There are no parameters for the Goonjcustom cron. + // This API is triggered automatically by the cron system and doesn't require any input parameters. + // It processes all eligible induction activities that need rescheduling. }
Line range hint
21-35: Fix grammar and enhance documentation clarityThe documentation has a minor grammatical error and could be more specific about the timing criteria.
* This function checks for not visited induction activities older than 1 days - * and sends reschedule emails to the respective contacts if they haven't already - * received one. + * (more than 24 hours old) and sends reschedule emails to the respective contacts + * if they haven't already received a reschedule notification.
Line range hint
36-46: Consider operational monitoring and rate limitingFor production reliability, consider these enhancements:
- Add detailed logging of the number of emails processed/sent for monitoring.
- Implement rate limiting or batching if processing large numbers of emails.
- Add metrics for monitoring cron execution success/failure rates.
wp-content/civi-extensions/goonjcustom/goonjcustom.php (3)
100-103: Remove debug logging statementsDebug logging statements should not be committed to production code. Consider using debug logging only during development or implementing proper logging levels.
- \Civi::log()->info('office', ['office'=>$office]); - \Civi::log()->info('coordinatorId', ['coordinatorId'=>$coordinatorId]);
Line range hint
143-157: Improve error handling and readabilityThe activity fetching and processing logic could be more robust with better error handling and early returns.
Consider restructuring like this:
- if ($inductionActivities->count() === 0) { - $row->tokens('contact', 'inductionDateTime', 'Not Scheduled'); - $row->tokens('contact', 'inductionOnlineMeetlink', ''); - return; - } - - $inductionActivity = $inductionActivities->first(); - $inductionDateTime = $inductionActivity['activity_date_time'] ?? 'Not Scheduled'; - $inductionGoonjOffice = $inductionActivity['Induction_Fields.Goonj_Office'] ?? ''; - $inductionAssigneeId = $inductionActivity['Induction_Fields.Assign']?? ''; + $defaultTokens = [ + 'inductionDateTime' => 'Not Scheduled', + 'inductionOnlineMeetlink' => '', + ]; + + if ($inductionActivities->count() === 0) { + foreach ($defaultTokens as $token => $value) { + $row->tokens('contact', $token, $value); + } + return; + } + + $inductionActivity = $inductionActivities->first(); + $inductionDateTime = $inductionActivity['activity_date_time'] ?? $defaultTokens['inductionDateTime']; + $inductionGoonjOffice = $inductionActivity['Induction_Fields.Goonj_Office'] ?? ''; + $inductionAssigneeId = $inductionActivity['Induction_Fields.Assign'] ?? '';
166-177: Improve assignee details fetchingThe assignee details fetching could be more concise and handle edge cases better.
Consider this approach:
- if( $inductionAssigneeId){ - $assigneeDetails = \Civi\Api4\Contact::get(FALSE) - ->addSelect('first_name', 'phone.phone') - ->addJoin('Phone AS phone', 'LEFT') - ->addWhere('id', '=', $inductionAssigneeId) - ->execute()->single(); - - $inductionAssigneeFirstName = $assigneeDetails['first_name'] ?? ''; - - $inductionAssigneePhone = $assigneeDetails['phone.phone'] ?? ''; - - } + $defaultAssigneeDetails = [ + 'first_name' => '', + 'phone.phone' => Civi::settings()->get('default_induction_coordinator_phone'), + ]; + + $assigneeDetails = $inductionAssigneeId + ? \Civi\Api4\Contact::get(FALSE) + ->addSelect('first_name', 'phone.phone') + ->addJoin('Phone AS phone', 'LEFT') + ->addWhere('id', '=', $inductionAssigneeId) + ->execute() + ->first() ?? $defaultAssigneeDetails + : $defaultAssigneeDetails; + + $inductionAssigneeFirstName = $assigneeDetails['first_name']; + $inductionAssigneePhone = $assigneeDetails['phone.phone'];wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
Line range hint
192-221: Improve email template selection logicThe template selection logic could be more robust:
- Template names are hardcoded strings
- No fallback handling if template is not found
- The email sent flag update should be in a transaction with email sending
Consider these improvements:
- Move template names to constants
- Add fallback handling
- Wrap email sending and flag update in a transaction
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(13 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRemainderEmailCron.php
🔇 Additional comments (8)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InductionRescheduleCron.php (1)
Line range hint 36-46: Verify InductionService implementation
The implementation looks good and follows best practices. Let's verify the existence and implementation of the sendInductionRescheduleEmail method.
✅ Verification successful
The previous results show the method exists but we need to see its implementation to verify it properly handles the induction rescheduling email functionality.
The previous output shows the beginning of the method with email template retrieval, but we need to see the complete implementation to verify it properly handles the email sending logic and follows best practices.
We need to see the complete implementation, particularly how it handles the email sending after checking for existing follow-up emails. The current output shows good practices with batch processing and duplicate email checks, but we need to verify the actual email sending logic.
The previous command to get the rest of the implementation failed. Let's try to view the complete file to see how the email sending logic is implemented after the follow-up email check.
Implementation verified and follows best practices
The sendInductionRescheduleEmail method is well-implemented with proper:
- Batch processing to handle large datasets efficiently
- Error handling and logging
- Email template verification
- Duplicate email prevention using custom fields
- Status updates for activities
- Clear business logic for rescheduling workflow
The implementation aligns with the API endpoint's purpose and maintains consistency with other email-sending methods in the service class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of InductionService::sendInductionRescheduleEmail
# Search for the InductionService class and its method
ast-grep --pattern 'class InductionService {
$$$
public function sendInductionRescheduleEmail() {
$$$
}
$$$
}'
# Backup: Search using ripgrep if the class is defined differently
rg -l "class.*InductionService" | xargs rg "function.*sendInductionRescheduleEmail"
Length of output: 9538
Script:
#!/bin/bash
# Let's get more context about the InductionService implementation
rg -A 10 "function.*sendInductionRescheduleEmail"
# Also find the file containing InductionService
fd -t f "InductionService"
Length of output: 1281
Script:
#!/bin/bash
# Let's see the complete implementation of sendInductionRescheduleEmail
rg -B 2 -A 30 "function.*sendInductionRescheduleEmail" wp-content/civi-extensions/goonjcustom/Civi/InductionService.php
Length of output: 1464
Script:
#!/bin/bash
# Let's see the rest of the implementation to verify email sending logic
rg -A 40 "source_contact_id', '=', \$activity\['source_contact_id'\]\)" wp-content/civi-extensions/goonjcustom/Civi/InductionService.php
Length of output: 152
Script:
#!/bin/bash
# Let's try a simpler approach to get the full file content
cat wp-content/civi-extensions/goonjcustom/Civi/InductionService.php
Length of output: 28518
wp-content/civi-extensions/goonjcustom/goonjcustom.php (4)
9-9: LGTM: Clean token registration
The new imports and token registrations are well-structured and follow the existing pattern.
Also applies to: 71-75
164-164: Previous comment still valid: Remove hardcoded phone number
As mentioned in the previous review, the hardcoded phone number should be moved to a configuration setting.
189-197: Previous comment still valid: Extract address formatting logic
As suggested in the previous review, the address fetching and formatting logic should be extracted into a separate method.
203-206: Verify token usage in email templates
The token implementation looks correct, but we should verify that these tokens are properly used in the email templates.
✅ Verification successful
Token implementation is properly integrated with email templates
The verification shows that:
- The tokens are properly registered and documented in the codebase
- The tokens are correctly populated with data from the InductionService
- The tokens are used in conjunction with CiviCRM's MessageTemplate system
- Multiple email templates are configured to use these tokens for induction-related communications (follow-up, rescheduling, reminders)
The implementation follows CiviCRM's token system best practices and maintains clean separation of concerns between token registration, data population, and template usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for token usage in email templates
rg -l "inductionOffice(Address|City)|inductionAssignee(FirstName|Phone)" \
--type html \
--type php
Length of output: 152
Script:
#!/bin/bash
# Search for email templates that might use these tokens
fd -e html -e msg -e tpl -e template -e email --exec rg -l "token|{contact\." {}
# Also check for any CiviCRM message template configurations
rg -l "MessageTemplate" --type php
Length of output: 107639
Script:
#!/bin/bash
# Let's check the content of email templates and message templates for token usage
rg -A 5 "inductionOffice(Address|City)|inductionAssignee(FirstName|Phone)" wp-content/civi-extensions/goonjcustom/
# Also check for message template definitions
rg -A 5 "MessageTemplate" wp-content/civi-extensions/goonjcustom/
Length of output: 13355
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (3)
10-10: LGTM: Import addition is well-placed
The new StateProvince API import is correctly placed with other imports and is used appropriately in the new methods.
418-428: LGTM: Email sent check is improved
The modification to use a custom field for tracking sent emails is a good improvement over the previous implementation.
Line range hint 159-171: Consider keeping methods private to maintain encapsulation
The methods findOfficeForState and findCoordinatorForOffice contain core business logic for office and coordinator assignment. Making them public exposes internal implementation details and violates encapsulation. Unless there's a specific requirement to access these methods from outside the class, they should remain private.
| public static function fetchTypeOfInduction($volunteerId) | ||
| { | ||
| $inductionType = 'Offline'; | ||
|
|
||
| // Fetch contact data | ||
| $contactData = Contact::get(FALSE) | ||
| ->addSelect('address_primary.state_province_id', 'address_primary.city', 'Individual_fields.Created_Date') | ||
| ->addWhere('id', '=', $volunteerId) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| $contactStateId = intval($contactData['address_primary.state_province_id']); | ||
| $contactCityFormatted = ucwords(strtolower($contactData['address_primary.city'])); | ||
|
|
||
| // States with mixed induction types | ||
| $statesWithMixedInductionTypes = StateProvince::get(FALSE) | ||
| ->addWhere('country_id.name', '=', 'India') | ||
| ->addWhere('name', 'IN', ['Bihar', 'Jharkhand', 'Orissa']) | ||
| ->execute() | ||
| ->column('id'); | ||
|
|
||
| // Check if the contact's state and city match special conditions | ||
| if (in_array($contactStateId, $statesWithMixedInductionTypes)) { | ||
| $contactCity = isset($contactData['address_primary.city']) ? strtolower($contactData['address_primary.city']) : ''; | ||
| if (in_array($contactCity, ['patna', 'ranchi', 'bhubaneshwar'])) { | ||
| return $inductionType; | ||
| } | ||
| $inductionType = 'Online'; | ||
| return $inductionType; | ||
| } | ||
|
|
||
| // Check if a Goonj office exists in the contact's state | ||
| $officeContact = Contact::get(FALSE) | ||
| ->addSelect('id', 'display_name') | ||
| ->addWhere('contact_sub_type', 'CONTAINS', 'Goonj_Office') | ||
| ->addWhere('address_primary.state_province_id', '=', $contactStateId) | ||
| ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%') | ||
| ->execute(); | ||
|
|
||
| // If no Goonj office exists, induction is online | ||
| if ($officeContact->count() === 0) { | ||
| $inductionType = 'Online'; | ||
| return $inductionType; | ||
| } | ||
|
|
||
| return $inductionType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve state and city handling in induction type determination
The method has several areas for improvement:
- Hard-coded state and city names should be moved to configuration
- City name comparison is case-sensitive and could be problematic
- The method returns early in some cases but continues in others, making the flow hard to follow
Consider these improvements:
- Move state and city lists to configuration
- Use consistent case-insensitive city name comparison
- Standardize the return flow
Example refactor:
- if (in_array($contactStateId, $statesWithMixedInductionTypes)) {
- $contactCity = isset($contactData['address_primary.city']) ? strtolower($contactData['address_primary.city']) : '';
- if (in_array($contactCity, ['patna', 'ranchi', 'bhubaneshwar'])) {
- return $inductionType;
- }
- $inductionType = 'Online';
- return $inductionType;
- }
+ if (in_array($contactStateId, $statesWithMixedInductionTypes)) {
+ $contactCity = strtolower($contactData['address_primary.city'] ?? '');
+ return in_array($contactCity, self::OFFLINE_INDUCTION_CITIES)
+ ? 'Offline'
+ : 'Online';
+ }Committable suggestion skipped: line range outside the PR's diff.
| public static function sendRemainderEmails() | ||
| { | ||
| $startOfDay = (new \DateTime())->setTime(0, 0, 0); | ||
| $endOfDay = (new \DateTime())->setTime(23, 59, 59); | ||
|
|
||
| $scheduledInductionActivities = Activity::get(FALSE) | ||
| ->addSelect('source_contact_id', 'Induction_Fields.Assign') | ||
| ->addWhere('activity_type_id:name', '=', 'Induction') | ||
| ->addWhere('status_id:name', '=', 'Scheduled') | ||
| ->addWhere('activity_date_time', '>=', $startOfDay->format('Y-m-d H:i:s')) | ||
| ->addWhere('activity_date_time', '<=', $endOfDay->format('Y-m-d H:i:s')) | ||
| ->execute(); | ||
|
|
||
| foreach ($scheduledInductionActivities as $scheduledInductionActivity) { | ||
| try { | ||
| $inductionType = self::fetchTypeOfInduction($scheduledInductionActivity['source_contact_id']); | ||
|
|
||
| $templateName = ($inductionType === 'Offline') | ||
| ? 'Remainder_For_Volunteer_Induction_Scheduled_for_Today%' | ||
| : 'Remainder_For_Volunteer_Induction_Scheduled_Online_for_Today%'; | ||
|
|
||
| $template = MessageTemplate::get(FALSE) | ||
| ->addWhere('msg_title', 'LIKE', $templateName) | ||
| ->setLimit(1) | ||
| ->execute() | ||
| ->single(); | ||
|
|
||
| $searchableSubject = str_replace('{contact.first_name}', '%', $template['msg_subject']); | ||
|
|
||
| $emailActivities = Activity::get(FALSE) | ||
| ->addWhere('activity_type_id:name', '=', 'Email') | ||
| ->addWhere('subject', 'LIKE', $searchableSubject) | ||
| ->addWhere('source_contact_id', '=', $scheduledInductionActivity['source_contact_id']) | ||
| ->execute(); | ||
|
|
||
| $emailActivity = $emailActivities->first(); | ||
|
|
||
| $contacts = Contact::get(FALSE) | ||
| ->addSelect('Individual_fields.Induction_Remainder_Email_Sent_on_Induction_Day') | ||
| ->addWhere('id', '=',$scheduledInductionActivity['source_contact_id'] ) | ||
| ->execute()->single(); | ||
|
|
||
| $isMailSent = $contacts['Individual_fields.Induction_Remainder_Email_Sent_on_Induction_Day']?? null; | ||
|
|
||
| if (empty($isMailSent)) { | ||
| $emailParams = [ | ||
| 'contact_id' => $scheduledInductionActivity['source_contact_id'], | ||
| 'template_id' => $template['id'], | ||
| ]; | ||
|
|
||
| $result = civicrm_api3('Email', 'send', $emailParams); | ||
|
|
||
| $contact = Contact::update(FALSE) | ||
| ->addValue('Individual_fields.Induction_Remainder_Email_Sent_on_Induction_Day', 1) | ||
| ->addWhere('id', '=', $scheduledInductionActivity['source_contact_id']) | ||
| ->execute(); | ||
|
|
||
| } | ||
| } catch (\Exception $e) { | ||
| // Log the error for debugging purposes | ||
| \Civi::log()->error('Failed to send reminder email', [ | ||
| 'contact_id' => $scheduledInductionActivity['source_contact_id'], | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Critical: Fix typos and improve error handling in reminder email logic
- Method name has a typo:
sendRemainderEmailsshould besendReminderEmails - The error handling could be improved by adding specific exception types
- The method is quite long and could benefit from extraction of smaller methods
Apply this diff to fix the typo:
-public static function sendRemainderEmails()
+public static function sendReminderEmails()Consider breaking down the method into smaller, focused methods:
getScheduledInductionsForToday()sendReminderEmail()updateReminderStatus()
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (3)
71-75: Add documentation for the new tokensWhile the token registration is clean, it would be helpful to add PHPDoc comments describing:
- The purpose and expected format of each new token
- Any dependencies or requirements for the token values
- Example usage in email templates
156-160: Initialize variables with appropriate default valuesConsider using more descriptive default values that indicate why the value is empty, for example:
- $completeAddress = ''; - $inductionOfficeCity = ''; - $urbanOpsName = ''; - $urbanOpsPhone = ''; + $completeAddress = 'Address not available'; + $inductionOfficeCity = 'City not specified'; + $urbanOpsName = 'Coordinator not assigned'; + $urbanOpsPhone = 'Phone not available';
Line range hint
80-204: Consider refactoring token evaluation into smaller, focused classesThe token evaluation logic has grown complex with multiple responsibilities. Consider splitting it into separate classes:
InductionTokenProvider: Handle induction-specific tokensCoordinatorTokenProvider: Handle coordinator-related tokensAddressTokenProvider: Handle address-related tokensThis would improve maintainability and make the code easier to test.
Would you like me to provide a detailed implementation plan for this refactoring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/goonjcustom.php(3 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (2)
172-180: 🛠️ Refactor suggestion
Extract duplicate address fetching logic
This address fetching logic is duplicated. Consider extracting it into a reusable method:
+ /**
+ * Fetches and formats the primary address for a contact
+ * @param int $contactId Contact ID
+ * @return string Formatted address or default message
+ */
+ private function getPrimaryAddress($contactId) {
+ try {
+ $addresses = \Civi\Api4\Address::get(false)
+ ->addWhere('contact_id', '=', $contactId)
+ ->addWhere('is_primary', '=', true)
+ ->setLimit(1)
+ ->execute();
+ $address = $addresses->count() > 0 ? $addresses->first() : null;
+ return $address ? CRM_Utils_Address::format($address) : 'Address not available';
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to fetch address: ' . $e->getMessage());
+ return 'Address temporarily unavailable';
+ }
+ }Likely invalid or redundant comment.
201-204: Verify token usage in email templates
These new tokens are being added to support induction emails. Let's verify that all email templates using these tokens handle empty/null values gracefully.
✅ Verification successful
Let's search for template files and examine how these tokens are used in the codebase. The previous search confirmed the tokens are defined in goonjcustom.php, but we need to see where and how they're actually used in templates and any validation logic around them.
Let's check if these tokens are used in any message templates and examine how they're processed in the code. The previous search found where they're defined but we need to understand their usage and validation.
Token handling appears safe with proper null checks
The code shows that these tokens are properly handled with null coalescing operators and default values:
urbanOpsNamefalls back to empty string if coordinator detail is missingurbanOpsPhonefalls back to empty string if phone not foundinductionDateTimedefaults to 'Not Scheduled' if activity date missing- Empty contact ID case is handled by setting empty token values
The token registration and evaluation functions follow CiviCRM's token handling patterns and include appropriate error handling. No issues found with how empty/null values are managed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for email templates using the new tokens
rg -l "inductionOfficeAddress|inductionOfficeCity|urbanOpsName|urbanOpsPhone" \
--type html \
--type php
# Search for message templates in the database
echo "SELECT msg_title, msg_subject, msg_text
FROM civicrm_msg_template
WHERE msg_text LIKE '%inductionOffice%'
OR msg_text LIKE '%urbanOps%';" | mysql -N civicrm
Length of output: 437
Script:
#!/bin/bash
# Search for message template files that might contain these tokens
fd -e html -e tpl -e msg
# Search for token usage in PHP files with context
rg -A 5 -B 5 "inductionOfficeAddress|inductionOfficeCity|urbanOpsName|urbanOpsPhone" \
--type php
# Look for any token validation or processing logic
ast-grep --pattern 'tokens($_, $_, $_)'
Length of output: 123533
Script:
#!/bin/bash
# Search for message templates that might use these tokens
rg -l "inductionOffice|urbanOps" \
--type-add 'template:*.{tpl,html,msg,txt}' \
--type template
# Look for token validation/processing code
rg -A 5 -B 5 "tokens\(" \
--type php \
wp-content/civi-extensions/goonjcustom/goonjcustom.php
Length of output: 2533
| $office = InductionService::findOfficeForState($stateId); | ||
|
|
||
| $coordinatorId = InductionService::findCoordinatorForOffice($office['id']); | ||
|
|
||
|
|
||
| $coordinatorDetails = Contact::get(FALSE) | ||
| ->addSelect('phone.phone', 'display_name') | ||
| ->addJoin('Phone AS phone', 'LEFT') | ||
| ->addWhere('id', '=', $coordinatorId) | ||
| ->execute(); | ||
| $coordinatorDetail = $coordinatorDetails->first(); | ||
|
|
||
| $urbanOpsName = $coordinatorDetail['display_name'] ?? ''; | ||
| $urbanOpsPhone = $coordinatorDetail['phone.phone'] ?? ''; | ||
|
|
There was a problem hiding this comment.
Add error handling for coordinator lookup
The coordinator lookup chain lacks proper error handling. A failure in any of these API calls could cause the entire token evaluation to fail:
+ try {
$office = InductionService::findOfficeForState($stateId);
+ if (empty($office)) {
+ throw new \Exception('No office found for state');
+ }
$coordinatorId = InductionService::findCoordinatorForOffice($office['id']);
+ if (empty($coordinatorId)) {
+ throw new \Exception('No coordinator assigned to office');
+ }
$coordinatorDetails = Contact::get(FALSE)
->addSelect('phone.phone', 'display_name')
->addJoin('Phone AS phone', 'LEFT')
->addWhere('id', '=', $coordinatorId)
->execute();
$coordinatorDetail = $coordinatorDetails->first();
$urbanOpsName = $coordinatorDetail['display_name'] ?? '';
$urbanOpsPhone = $coordinatorDetail['phone.phone'] ?? '';
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to fetch coordinator details: ' . $e->getMessage());
+ $urbanOpsName = 'Coordinator information unavailable';
+ $urbanOpsPhone = 'Phone number unavailable';
+ }📝 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.
| $office = InductionService::findOfficeForState($stateId); | |
| $coordinatorId = InductionService::findCoordinatorForOffice($office['id']); | |
| $coordinatorDetails = Contact::get(FALSE) | |
| ->addSelect('phone.phone', 'display_name') | |
| ->addJoin('Phone AS phone', 'LEFT') | |
| ->addWhere('id', '=', $coordinatorId) | |
| ->execute(); | |
| $coordinatorDetail = $coordinatorDetails->first(); | |
| $urbanOpsName = $coordinatorDetail['display_name'] ?? ''; | |
| $urbanOpsPhone = $coordinatorDetail['phone.phone'] ?? ''; | |
| try { | |
| $office = InductionService::findOfficeForState($stateId); | |
| if (empty($office)) { | |
| throw new \Exception('No office found for state'); | |
| } | |
| $coordinatorId = InductionService::findCoordinatorForOffice($office['id']); | |
| if (empty($coordinatorId)) { | |
| throw new \Exception('No coordinator assigned to office'); | |
| } | |
| $coordinatorDetails = Contact::get(FALSE) | |
| ->addSelect('phone.phone', 'display_name') | |
| ->addJoin('Phone AS phone', 'LEFT') | |
| ->addWhere('id', '=', $coordinatorId) | |
| ->execute(); | |
| $coordinatorDetail = $coordinatorDetails->first(); | |
| $urbanOpsName = $coordinatorDetail['display_name'] ?? ''; | |
| $urbanOpsPhone = $coordinatorDetail['phone.phone'] ?? ''; | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to fetch coordinator details: ' . $e->getMessage()); | |
| $urbanOpsName = 'Coordinator information unavailable'; | |
| $urbanOpsPhone = 'Phone number unavailable'; | |
| } |
|
Approved, Please test all cases before merging this PR. |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Enhancements