Send Feedback Email When Dropping Center is Closed or sent every 6 month#393
Send Feedback Email When Dropping Center is Closed or sent every 6 month#393belwalshubham merged 17 commits intodevelopfrom
Conversation
|
Warning Rate limit exceeded@belwalshubham has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces functionalities for managing feedback related to Dropping Centers within the CiviCRM framework. It adds two cron jobs: one for sending feedback emails for closed dropping centers and another for processing biannual feedback. Additionally, a new service class is created to encapsulate the logic for sending feedback emails, including methods for processing status and generating email content. Error handling is implemented throughout to ensure smooth operation. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
belwalshubham
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1)
73-73: Clarify the condition for feedback delivery statusThe condition
if (!$status)relies on$statusbeing falsy when feedback has not been delivered. Ensure that$statusaccurately represents the delivery status, and consider making the condition more explicit for readability.Confirm that
$statusis correctly set and reflects the intended logic.
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| // URL for the feedback form. | ||
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | ||
|
|
||
| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
|
|
||
| <p>Thank you for being such an outstanding representative of Goonj! | ||
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | ||
|
|
||
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | ||
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | ||
|
|
||
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | ||
|
|
||
| <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> | ||
|
|
||
| <p>We look forward to continuing this important journey with you!</p> | ||
|
|
||
| <p>Warm Regards,<br> | ||
| Team Goonj</p> | ||
| "; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a templating engine for email content
Constructing HTML emails by concatenating strings can lead to unreadable code and potential injection vulnerabilities. Consider using a templating engine like Smarty or CiviCRM's built-in template system for better maintainability and security.
This will separate the email content from the code logic and make it easier to manage.
| function civicrm_api3_goonjcustom_feedback_dropping_center_cron($params) { | ||
| $returnValues = []; | ||
|
|
||
| // Retrieve the Status option value. | ||
| $optionValues = OptionValue::get(FALSE) | ||
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | ||
| ->addWhere('name', '=', 'Status') | ||
| ->addWhere('grouping', '=', 'Dropping_Center_Meta') | ||
| ->setLimit(1) | ||
| ->execute()->single(); | ||
|
|
||
| $statusName = $optionValues['value']; | ||
|
|
||
| $droppingCenterMetas = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('subtype', '=', $statusName) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | ||
| ->execute(); | ||
|
|
||
| [$defaultFromName, $defaultFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(); | ||
| $from = "\"$defaultFromName\" <$defaultFromEmail>"; | ||
|
|
||
| try { | ||
| foreach ($droppingCenterMetas as $meta) { | ||
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| // Get recipient email and name. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
| if (!$status) { | ||
| $mailParams = [ | ||
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | ||
| 'from' => $from, | ||
| 'toEmail' => $contactEmailId, | ||
| 'replyTo' => $from, | ||
| 'html' => goonjcustom_volunteer_feedback_email_html($organizingContactName, $droppingCenterId), | ||
| ]; | ||
| $feedbackEmailSendResult = CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($feedbackEmailSendResult) { | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| error_log("Error processing: " . $e->getMessage()); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'feedback_dropping_center_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to adhere to Single Responsibility Principle
The function civicrm_api3_goonjcustom_feedback_dropping_center_cron handles multiple tasks: data retrieval, email preparation, sending emails, and updating records. For better maintainability, consider refactoring this function into smaller functions, each handling a specific responsibility.
Breaking down the function will improve readability and make future updates easier.
| $droppingCenterMetas = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('subtype', '=', $statusName) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') |
There was a problem hiding this comment.
Correct the typo in 'Parmanently_Closed'
The status value 'Parmanently_Closed' seems to be misspelled. The correct spelling is 'Permanently_Closed'.
Apply this diff to fix the typo:
->addWhere('Status.Status:name', '=', 'Parmanently_Closed')
+->addWhere('Status.Status:name', '=', 'Permanently_Closed')Committable suggestion was skipped due to low confidence.
| $statusName = $optionValues['value']; | ||
|
|
There was a problem hiding this comment.
Add null check before accessing 'value' key
There is a risk of an undefined index error if $optionValues does not contain the 'value' key. It's best practice to verify that $optionValues is not null and has the expected key before accessing it.
Apply this diff to add a null check:
$optionValues = OptionValue::get(FALSE)
->addWhere('option_group_id:name', '=', 'eck_sub_types')
->addWhere('name', '=', 'Status')
->addWhere('grouping', '=', 'Dropping_Center_Meta')
->setLimit(1)
->execute()->single();
+ if (!$optionValues || !isset($optionValues['value'])) {
+ throw new Exception('OptionValue for Status not found.');
+ }
$statusName = $optionValues['value'];Committable suggestion was skipped due to low confidence.
| catch (Exception $e) { | ||
| error_log("Error processing: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance exception handling in the catch block
Catching a generic Exception and merely logging it may not be sufficient. Consider rethrowing the exception or using a more robust error-handling mechanism to ensure that errors are properly propagated.
Apply this diff to improve error handling:
catch (Exception $e) {
- error_log("Error processing: " . $e->getMessage());
+ CRM_Core_Error::debug_log_message("Error in FeedbackDroppingCenterCron: " . $e->getMessage());
+ throw $e;
}📝 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.
| catch (Exception $e) { | |
| error_log("Error processing: " . $e->getMessage()); | |
| } | |
| catch (Exception $e) { | |
| CRM_Core_Error::debug_log_message("Error in FeedbackDroppingCenterCron: " . $e->getMessage()); | |
| throw $e; | |
| } |
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; |
There was a problem hiding this comment.
Verify that $campAttendedBy is not null before use
If the contact retrieval does not return any results, accessing elements of $campAttendedBy will cause an error. Ensure that $campAttendedBy is not null before proceeding.
Apply this diff to add a null check:
$campAttendedBy = Contact::get(TRUE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $initiatorId)
->execute()->single();
+ if (!$campAttendedBy) {
+ // Handle the case where the contact is not found.
+ continue;
+ }
$contactEmailId = $campAttendedBy['email.email'];Committable suggestion was skipped due to low confidence.
| if ($feedbackEmailSendResult) { | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Handle potential failure in EckEntity update
The update operation on Dropping_Center_Meta does not currently check for success. It's important to verify that the update was successful and handle any failures appropriately.
Apply this diff to check the update result:
if ($feedbackEmailSendResult) {
$updateResult = EckEntity::update('Dropping_Center_Meta', TRUE)
->addValue('Status.Feedback_Email_Delivered:name', 1)
->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId)
->execute();
+ if (!$updateResult) {
+ // Log the failure or handle it as needed.
+ error_log("Failed to update Feedback_Email_Delivered for Dropping Center ID: $droppingCenterId");
+ }
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
11-22: Enhance API documentation.While the function implementation is correct, consider adding more descriptive documentation about the cron job's purpose and execution frequency.
Add details about when this API is triggered and what it accomplishes:
/** * Goonjcustom.FeedbackDroppingCenterCron API specification (optional) * This is used for documentation and validation. + * + * This API is triggered by a cron job to send feedback request emails + * to contacts associated with permanently closed dropping centers. + * It ensures each contact receives only one feedback request. * * @param array $spec * description of fields supported by this API call. * * @return void */
108-108: Rename function to match its purpose.The function name
sendFeedbackEmailsuggests it sends an email, but it actually generates the email content.-function sendFeedbackEmail($organizingContactName, $droppingCenterId) { +function generateFeedbackEmailContent($organizingContactName, $droppingCenterId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1)
1-10: LGTM: File structure and imports are well-organized.The file follows CiviCRM conventions with proper namespace imports for API v4.
| function civicrm_api3_goonjcustom_feedback_dropping_center_cron($params) { | ||
| $returnValues = []; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return meaningful values from the API.
The API currently returns an empty array. Consider returning statistics about the processed dropping centers and sent emails.
function civicrm_api3_goonjcustom_feedback_dropping_center_cron($params) {
- $returnValues = [];
+ $returnValues = [
+ 'processed_centers' => 0,
+ 'emails_sent' => 0,
+ ];Committable suggestion was skipped due to low confidence.
| function sendFeedbackEmail($organizingContactName, $droppingCenterId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| // URL for the feedback form. | ||
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | ||
|
|
There was a problem hiding this comment.
Add input validation for function parameters.
The function should validate its input parameters before using them to generate the email content.
function sendFeedbackEmail($organizingContactName, $droppingCenterId) {
+ if (empty($organizingContactName)) {
+ throw new InvalidArgumentException('Contact name cannot be empty');
+ }
+ if (!is_numeric($droppingCenterId) || $droppingCenterId <= 0) {
+ throw new InvalidArgumentException('Invalid dropping center ID');
+ }
+
$homeUrl = \CRM_Utils_System::baseCMSURL();
// URL for the feedback form.
$volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId;📝 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.
| function sendFeedbackEmail($organizingContactName, $droppingCenterId) { | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| // URL for the feedback form. | |
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | |
| function sendFeedbackEmail($organizingContactName, $droppingCenterId) { | |
| if (empty($organizingContactName)) { | |
| throw new InvalidArgumentException('Contact name cannot be empty'); | |
| } | |
| if (!is_numeric($droppingCenterId) || $droppingCenterId <= 0) { | |
| throw new InvalidArgumentException('Invalid dropping center ID'); | |
| } | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| // URL for the feedback form. | |
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; |
| try { | ||
| foreach ($droppingCenterMetas as $meta) { | ||
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| // Get recipient email and name. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
| if (!$status) { | ||
| $mailParams = [ | ||
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | ||
| 'from' => $from, | ||
| 'toEmail' => $contactEmailId, | ||
| 'replyTo' => $from, | ||
| 'html' => sendFeedbackEmail($organizingContactName, $droppingCenterId), | ||
| ]; | ||
| $feedbackEmailSendResult = CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($feedbackEmailSendResult) { | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| error_log("Error processing: " . $e->getMessage()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Wrap database operations in a transaction.
Multiple database operations (reading contacts and updating metadata) should be wrapped in a transaction to ensure data consistency.
try {
+ $transaction = new CRM_Core_Transaction();
foreach ($droppingCenterMetas as $meta) {
// ... existing code ...
}
+ $transaction->commit();
}
catch (Exception $e) {
+ $transaction->rollback();
error_log("Error processing: " . $e->getMessage());
+ throw new CRM_Core_Exception('Failed to process dropping centers: ' . $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.
| try { | |
| foreach ($droppingCenterMetas as $meta) { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| // Get recipient email and name. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| $mailParams = [ | |
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => sendFeedbackEmail($organizingContactName, $droppingCenterId), | |
| ]; | |
| $feedbackEmailSendResult = CRM_Utils_Mail::send($mailParams); | |
| if ($feedbackEmailSendResult) { | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| } | |
| catch (Exception $e) { | |
| error_log("Error processing: " . $e->getMessage()); | |
| } | |
| try { | |
| $transaction = new CRM_Core_Transaction(); | |
| foreach ($droppingCenterMetas as $meta) { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| // Get recipient email and name. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| $mailParams = [ | |
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => sendFeedbackEmail($organizingContactName, $droppingCenterId), | |
| ]; | |
| $feedbackEmailSendResult = CRM_Utils_Mail::send($mailParams); | |
| if ($feedbackEmailSendResult) { | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| $transaction->commit(); | |
| } | |
| catch (Exception $e) { | |
| $transaction->rollback(); | |
| error_log("Error processing: " . $e->getMessage()); | |
| throw new CRM_Core_Exception('Failed to process dropping centers: ' . $e->getMessage()); | |
| } |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| // Get recipient email and name. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; |
There was a problem hiding this comment.
Could we consider relocating this logic to a dedicated service file? By creating a new service file specifically for dropping center feedback, we can simplify the current cron file and enhance its readability.
For reference please check the collectioncampcron.php
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php
🧰 Additional context used
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance API response with processing statistics.
The current implementation returns empty values, making it difficult to monitor the cron job's effectiveness.
Add processing statistics to the return values:
+ $returnValues = [
+ 'processed' => count($droppingCenters),
+ 'threshold_date' => $thresholdDate,
+ 'timestamp' => date('Y-m-d H:i:s'),
+ ];
return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron');Committable suggestion was skipped due to low confidence.
| /** | ||
| * | ||
| */ | ||
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | ||
| } |
There was a problem hiding this comment.
API specification function needs implementation.
The empty spec function violates CiviCRM API v3 best practices. This function should define the expected parameters and their types for API documentation and validation.
Add parameter specifications:
function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) {
+ // No required parameters for this API
+ $spec['limit'] = [
+ 'title' => 'Limit',
+ 'type' => CRM_Utils_Type::T_INT,
+ 'description' => 'Maximum number of dropping centers to process',
+ ];
}📝 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.
| /** | |
| * | |
| */ | |
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | |
| } | |
| /** | |
| * | |
| */ | |
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | |
| // No required parameters for this API | |
| $spec['limit'] = [ | |
| 'title' => 'Limit', | |
| 'type' => CRM_Utils_Type::T_INT, | |
| 'description' => 'Maximum number of dropping centers to process', | |
| ]; | |
| } |
| $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); | ||
|
|
||
| $droppingCenters = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id') | ||
| ->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate) | ||
| ->execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding configurable threshold and batch processing.
The 6-month threshold is hardcoded and the query fetches all matching records at once, which could cause memory issues with large datasets.
Consider these improvements:
- $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d');
+ $monthsThreshold = Civi::settings()->get('dropping_center_feedback_months_threshold') ?? 6;
+ $thresholdDate = (new \DateTime())->modify("-{$monthsThreshold} months")->format('Y-m-d');
$droppingCenters = EckEntity::get('Collection_Camp', TRUE)
->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id')
->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate)
+ ->addWhere('Feedback_Email_Delivered', 'IS NULL')
+ ->setLimit($params['limit'] ?? 50)
->execute();Committable suggestion was skipped due to low confidence.
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging granularity.
The current implementation catches errors at too high a level, potentially masking individual failures.
Implement per-record error handling:
try {
foreach ($droppingCenters as $center) {
+ try {
$droppingCenterId = $center['id'];
$initiatorId = $center['Collection_Camp_Core_Details.Contact_Id'];
$campAttendedBy = Contact::get(TRUE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $initiatorId)
->execute()->single();
if ($campAttendedBy) {
$contactEmailId = $campAttendedBy['email.email'];
$organizingContactName = $campAttendedBy['display_name'];
DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
+ // Mark as processed
+ EckEntity::update('Collection_Camp', TRUE)
+ ->addValue('Feedback_Email_Delivered', 1)
+ ->addWhere('id', '=', $droppingCenterId)
+ ->execute();
}
+ } catch (Exception $e) {
+ \CRM_Core_Error::debug_log_message(
+ "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage()
+ );
+ continue;
+ }
}
}📝 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.
| try { | |
| foreach ($droppingCenters as $center) { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| } | |
| } | |
| try { | |
| foreach ($droppingCenters as $center) { | |
| try { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Mark as processed | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addValue('Feedback_Email_Delivered', 1) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message( | |
| "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage() | |
| ); | |
| continue; | |
| } | |
| } | |
| } |
| <?php | ||
|
|
||
| /** | ||
| * @file | ||
| */ | ||
|
|
||
| use Civi\Api4\Contact; | ||
| use Civi\Api4\EckEntity; | ||
| use Civi\DroppingCenterFeedbackCron; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| function civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron($params) { | ||
| $returnValues = []; | ||
|
|
||
| $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); | ||
|
|
||
| $droppingCenters = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id') | ||
| ->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate) | ||
| ->execute(); | ||
|
|
||
| [$defaultFromName, $defaultFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(); | ||
| $from = "\"$defaultFromName\" <$defaultFromEmail>"; | ||
|
|
||
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron'); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Potential code duplication and responsibility overlap between feedback crons
After reviewing the implementation, I've identified some concerning patterns:
-
The BiannualDroppingCenterFeedbackCron and FeedbackDroppingCenterCron APIs have overlapping responsibilities - both send feedback emails to dropping center contacts but with different triggers:
- BiannualDroppingCenterFeedbackCron: Triggers based on 6-month threshold from opening date
- FeedbackDroppingCenterCron: Triggers when centers are permanently closed
-
There's duplicated code for email preparation (getting default from email/name, contact details) that should be consolidated in the DroppingCenterFeedbackCron class
Recommendations:
- Merge the two cron APIs into a single API with different trigger conditions to reduce code duplication
- Move common email preparation logic from the APIs into DroppingCenterFeedbackCron class
- Add proper error handling in BiannualDroppingCenterFeedbackCron similar to FeedbackDroppingCenterCron to track failures for specific centers
The current implementation violates the DRY principle and makes maintenance more difficult as changes need to be synchronized across multiple files.
🔗 Analysis chain
Verify integration with DroppingCenterFeedbackCron class.
The code relies heavily on the DroppingCenterFeedbackCron class. Let's verify its implementation.
Let's gather more information about the DroppingCenterFeedbackCron class implementation since we found its location.
This will help us verify:
- The implementation of sendFeedbackEmail method
- How Feedback_Email_Delivered field is used
- Any potential overlap between BiannualDroppingCenterFeedbackCron and FeedbackDroppingCenterCron
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DroppingCenterFeedbackCron class implementation
echo "Searching for DroppingCenterFeedbackCron class definition..."
ast-grep --pattern 'class DroppingCenterFeedbackCron {
$$$
static function sendFeedbackEmail($$$) {
$$$
}
$$$
}'
# Verify the Feedback_Email_Delivered field exists
echo "Verifying Feedback_Email_Delivered field definition..."
rg -l "Feedback_Email_Delivered"
Length of output: 9845
Script:
#!/bin/bash
# Get the implementation of DroppingCenterFeedbackCron class
echo "Content of DroppingCenterFeedbackCron class:"
cat wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackCron.php
# Get the implementation of FeedbackDroppingCenterCron API
echo -e "\nContent of FeedbackDroppingCenterCron API:"
cat wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php
Length of output: 6148
| /** | ||
| * Class to manage sending feedback emails for Dropping Centers. | ||
| */ | ||
| class DroppingCenterFeedbackCron { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing an interface for better testability.
The class could benefit from implementing an interface (e.g., DroppingCenterFeedbackInterface) to allow for easier mocking in tests and better dependency injection.
Consider adding:
interface DroppingCenterFeedbackInterface {
public static function processDroppingCenterStatus($meta, $from);
public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId);
}| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
|
|
||
| <p>Thank you for being such an outstanding representative of Goonj! | ||
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | ||
|
|
||
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | ||
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | ||
|
|
||
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | ||
|
|
||
| <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> | ||
|
|
||
| <p>We look forward to continuing this important journey with you!</p> | ||
|
|
||
| <p>Warm Regards,<br> | ||
| Team Goonj</p> | ||
| "; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a template system for email content.
The email content is currently hardcoded in the method. Using a template system would improve maintainability and allow for easier content updates.
Consider:
- Moving the email content to a separate template file
- Using CiviCRM's template system or a similar solution
- Making the content configurable through the admin interface
Example implementation:
public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) {
$template = \CRM_Core_Smarty::singleton();
$template->assign('organizingContactName', $organizingContactName);
$template->assign('volunteerFeedback', $volunteerFeedback);
return $template->fetch('CRM/Goonj/FeedbackEmail.tpl');
}| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | ||
| $mailParams = [ | ||
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | ||
| 'from' => $from, | ||
| 'toEmail' => $contactEmailId, | ||
| 'replyTo' => $from, | ||
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | ||
| ]; | ||
|
|
||
| // Send the email. | ||
| \CRM_Utils_Mail::send($mailParams); | ||
| } |
There was a problem hiding this comment.
Add parameter validation and error handling for email sending.
The email sending logic needs validation and error handling to ensure reliable delivery.
Implement these improvements:
public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) {
+ // Validate input parameters
+ if (empty($organizingContactName) || empty($droppingCenterId) ||
+ empty($contactEmailId) || empty($from)) {
+ throw new \CRM_Core_Exception("Missing required parameters for sending feedback email");
+ }
+
+ if (!filter_var($contactEmailId, FILTER_VALIDATE_EMAIL) ||
+ !filter_var($from, FILTER_VALIDATE_EMAIL)) {
+ throw new \CRM_Core_Exception("Invalid email format");
+ }
+
$mailParams = [
'subject' => 'Your Feedback on Managing the Goonj Dropping Center',
'from' => $from,
'toEmail' => $contactEmailId,
'replyTo' => $from,
'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId),
];
- // Send the email.
- \CRM_Utils_Mail::send($mailParams);
+ try {
+ $result = \CRM_Utils_Mail::send($mailParams);
+ if (!$result) {
+ throw new \CRM_Core_Exception("Failed to send feedback email");
+ }
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to send feedback email: ' . $e->getMessage());
+ throw $e;
+ }
}📝 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.
| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | |
| $mailParams = [ | |
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | |
| ]; | |
| // Send the email. | |
| \CRM_Utils_Mail::send($mailParams); | |
| } | |
| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | |
| // Validate input parameters | |
| if (empty($organizingContactName) || empty($droppingCenterId) || | |
| empty($contactEmailId) || empty($from)) { | |
| throw new \CRM_Core_Exception("Missing required parameters for sending feedback email"); | |
| } | |
| if (!filter_var($contactEmailId, FILTER_VALIDATE_EMAIL) || | |
| !filter_var($from, FILTER_VALIDATE_EMAIL)) { | |
| throw new \CRM_Core_Exception("Invalid email format"); | |
| } | |
| $mailParams = [ | |
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | |
| ]; | |
| try { | |
| $result = \CRM_Utils_Mail::send($mailParams); | |
| if (!$result) { | |
| throw new \CRM_Core_Exception("Failed to send feedback email"); | |
| } | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to send feedback email: ' . $e->getMessage()); | |
| throw $e; | |
| } | |
| } |
| public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | ||
|
|
||
| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
|
|
||
| <p>Thank you for being such an outstanding representative of Goonj! | ||
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | ||
|
|
||
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | ||
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | ||
|
|
||
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | ||
|
|
||
| <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> | ||
|
|
||
| <p>We look forward to continuing this important journey with you!</p> | ||
|
|
||
| <p>Warm Regards,<br> | ||
| Team Goonj</p> | ||
| "; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
Secure the HTML generation by escaping user input.
The HTML generation needs proper escaping to prevent XSS vulnerabilities.
Apply these security improvements:
public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) {
+ // Validate input parameters
+ if (empty($organizingContactName) || empty($droppingCenterId)) {
+ throw new \CRM_Core_Exception("Missing required parameters for HTML generation");
+ }
+
+ // Escape user input
+ $organizingContactName = htmlspecialchars($organizingContactName, ENT_QUOTES, 'UTF-8');
+ $droppingCenterId = (int) $droppingCenterId;
+
$homeUrl = \CRM_Utils_System::baseCMSURL();
- $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId;
+ $volunteerFeedback = htmlspecialchars(
+ $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId,
+ ENT_QUOTES,
+ 'UTF-8'
+ );
$html = "
<p>Dear $organizingContactName,</p>📝 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.
| public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) { | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | |
| $html = " | |
| <p>Dear $organizingContactName,</p> | |
| <p>Thank you for being such an outstanding representative of Goonj! | |
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | |
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | |
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | |
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | |
| <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> | |
| <p>We look forward to continuing this important journey with you!</p> | |
| <p>Warm Regards,<br> | |
| Team Goonj</p> | |
| "; | |
| return $html; | |
| } | |
| public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) { | |
| // Validate input parameters | |
| if (empty($organizingContactName) || empty($droppingCenterId)) { | |
| throw new \CRM_Core_Exception("Missing required parameters for HTML generation"); | |
| } | |
| // Escape user input | |
| $organizingContactName = htmlspecialchars($organizingContactName, ENT_QUOTES, 'UTF-8'); | |
| $droppingCenterId = (int) $droppingCenterId; | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| $volunteerFeedback = htmlspecialchars( | |
| $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId, | |
| ENT_QUOTES, | |
| 'UTF-8' | |
| ); | |
| $html = " | |
| <p>Dear $organizingContactName,</p> | |
| <p>Thank you for being such an outstanding representative of Goonj! | |
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | |
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | |
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | |
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | |
| <p>We encourage you to share any highlights, suggestions, or challenges you've encountered. Together, we can refine and enhance this process even further.</p> | |
| <p>We look forward to continuing this important journey with you!</p> | |
| <p>Warm Regards,<br> | |
| Team Goonj</p> | |
| "; | |
| return $html; | |
| } |
| public static function processDroppingCenterStatus($meta, $from) { | ||
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| // Get recipient email and name. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| if (!$status) { | ||
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update status if the email is sent. | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation and error handling.
The method has several potential issues:
- No validation of required meta fields
- Missing error handling for email operations
- Status update could fail silently after email send
Consider implementing these improvements:
public static function processDroppingCenterStatus($meta, $from) {
+ // Validate required fields
+ $requiredFields = [
+ 'Dropping_Center_Meta.Dropping_Center',
+ 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id',
+ 'Status.Feedback_Email_Delivered:name'
+ ];
+ foreach ($requiredFields as $field) {
+ if (!isset($meta[$field])) {
+ throw new \CRM_Core_Exception("Missing required field: $field");
+ }
+ }
+
$droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center'];
$initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id'];
$status = $meta['Status.Feedback_Email_Delivered:name'];
// Get recipient email and name.
$campAttendedBy = Contact::get(TRUE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $initiatorId)
->execute()->single();
+ if (empty($campAttendedBy['email.email'])) {
+ throw new \CRM_Core_Exception("No email found for contact ID: $initiatorId");
+ }
$contactEmailId = $campAttendedBy['email.email'];
$organizingContactName = $campAttendedBy['display_name'];
if (!$status) {
+ try {
self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
// Update status if the email is sent.
- EckEntity::update('Dropping_Center_Meta', TRUE)
+ $result = EckEntity::update('Dropping_Center_Meta', TRUE)
->addValue('Status.Feedback_Email_Delivered:name', 1)
->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId)
->execute();
+ if (empty($result)) {
+ throw new \CRM_Core_Exception("Failed to update feedback email status");
+ }
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to process dropping center feedback: ' . $e->getMessage());
+ throw $e;
+ }
}
}📝 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.
| public static function processDroppingCenterStatus($meta, $from) { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| // Get recipient email and name. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update status if the email is sent. | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } | |
| public static function processDroppingCenterStatus($meta, $from) { | |
| // Validate required fields | |
| $requiredFields = [ | |
| 'Dropping_Center_Meta.Dropping_Center', | |
| 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', | |
| 'Status.Feedback_Email_Delivered:name' | |
| ]; | |
| foreach ($requiredFields as $field) { | |
| if (!isset($meta[$field])) { | |
| throw new \CRM_Core_Exception("Missing required field: $field"); | |
| } | |
| } | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| // Get recipient email and name. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if (empty($campAttendedBy['email.email'])) { | |
| throw new \CRM_Core_Exception("No email found for contact ID: $initiatorId"); | |
| } | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| try { | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update status if the email is sent. | |
| $result = EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| if (empty($result)) { | |
| throw new \CRM_Core_Exception("Failed to update feedback email status"); | |
| } | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to process dropping center feedback: ' . $e->getMessage()); | |
| throw $e; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1)
3-6: Enhance the file-level PHPDoc block for better documentationThe file-level docblock is currently minimal. Providing a detailed description of the file's purpose and functionality will improve code readability and maintainability.
Consider expanding the docblock as follows:
/** + * Implements a cron job to send biannual feedback emails to dropping centers. + * + * This script retrieves dropping centers that meet certain criteria and sends + * feedback emails to associated contacts. It updates the last feedback sent date + * to prevent duplicate emails. * @file */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
| $droppingCenters = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') | ||
| ->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate) | ||
| ->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized') |
There was a problem hiding this comment.
Resolve inconsistent status checks between 'Authorized' and 'Permanently_Closed'
The query filters dropping centers with status 'Authorized' (line 31), but later checks if the status is 'Permanently_Closed' (line 58). A dropping center cannot be both 'Authorized' and 'Permanently_Closed' simultaneously. This inconsistency may result in no centers being processed.
To align the logic, consider updating the status check:
- ->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized')
+ ->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Permanently_Closed')Alternatively, if the intent is to process centers that are not permanently closed, adjust the later check accordingly.
Also applies to: 58-58
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') |
There was a problem hiding this comment.
Correct the typo in the status name 'Parmanently_Closed'
The status name 'Parmanently_Closed' appears to have a typo. It should be 'Permanently_Closed'. This error may prevent the query from matching any records.
Apply this diff to fix the typo:
- ->addWhere('Status.Status:name', '=', 'Parmanently_Closed')
+ ->addWhere('Status.Status:name', '=', 'Permanently_Closed')📝 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.
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | |
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return meaningful processing statistics
The $returnValues array is empty, so the API response does not provide any insight into what was processed. Including statistics will improve monitoring and debugging.
Populate $returnValues with processing details:
$returnValues = [];
+ $returnValues['processed_centers'] = count($droppingCenters);
+ $returnValues['timestamp'] = date('Y-m-d H:i:s');
return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron');Committable suggestion was skipped due to low confidence.
| $returnValues = []; | ||
|
|
||
| // Set threshold date to 6 months ago. | ||
| $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize the threshold date for flexibility
The threshold of 6 months is hard-coded, which reduces flexibility and reusability. Moving this value to a configuration setting or making it a parameter enhances adaptability.
Consider retrieving the threshold from a configuration setting:
- $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d');
+ $monthsThreshold = Civi::settings()->get('dropping_center_feedback_months_threshold') ?? 6;
+ $thresholdDate = (new \DateTime())->modify("-{$monthsThreshold} months")->format('Y-m-d');Committable suggestion was skipped due to low confidence.
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered']; | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| if (!$status) { | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent multiple emails due to duplicate meta records
If multiple Dropping_Center_Meta records exist for a single dropping center, the nested loop may send multiple feedback emails to the same contact. This could lead to spamming the user.
To ensure only one email is sent per dropping center, consider restructuring the code:
- Move the email-sending logic outside the inner loop.
- Check if at least one meta record meets the criteria before sending the email.
Example adjustment:
foreach ($droppingCenterMeta as $meta) {
$status = $meta['Status.Feedback_Email_Delivered'];
- if (!$status) {
- // Send the feedback email.
- DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
- // Update the last_feedback_sent_date to the current date after the email is sent.
- EckEntity::update('Collection_Camp', TRUE)
- ->addWhere('id', '=', $droppingCenterId)
- ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d'))
- ->execute();
- }
+ if ($status) {
+ // Feedback email already sent; no action needed.
+ continue;
+ }
+ // Meta record requires feedback email.
+ $needsEmail = true;
+ break; // Exit loop if email is needed.
}
+ if (isset($needsEmail)) {
+ // Send the feedback email.
+ DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
+ // Update the last_feedback_sent_date to the current date after the email is sent.
+ EckEntity::update('Collection_Camp', TRUE)
+ ->addWhere('id', '=', $droppingCenterId)
+ ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d'))
+ ->execute();
+ }📝 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.
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| if ($status) { | |
| // Feedback email already sent; no action needed. | |
| continue; | |
| } | |
| // Meta record requires feedback email. | |
| $needsEmail = true; | |
| break; // Exit loop if email is needed. | |
| } | |
| if (isset($needsEmail)) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } |
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| // Fetch the email and name of the initiator. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Check the status of the dropping center | ||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | ||
| ->execute(); | ||
|
|
||
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered']; | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| if (!$status) { | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Enhance error handling within the processing loop
Catching exceptions at the high level may cause the entire process to halt if an error occurs with one dropping center. This violates the robustness principle and affects the reliability of the cron job.
Implement per-dropping center error handling to ensure one failure doesn't impact others:
try {
foreach ($droppingCenters as $center) {
+ try {
// Existing processing logic...
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID ' . $center['id'] . ': ' . $e->getMessage());
+ // Optionally continue to the next center
+ continue;
+ }
}
- }
- catch (Exception $e) {
- \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $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.
| try { | |
| foreach ($droppingCenters as $center) { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| // Fetch the email and name of the initiator. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| // Check the status of the dropping center | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | |
| } | |
| try { | |
| foreach ($droppingCenters as $center) { | |
| try { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| // Fetch the email and name of the initiator. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| // Check the status of the dropping center | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID ' . $center['id'] . ': ' . $e->getMessage()); | |
| // Optionally continue to the next center | |
| continue; | |
| } | |
| } | |
| } |
| /** | ||
| * Class to manage sending feedback emails for Dropping Centers. | ||
| */ | ||
| class DroppingCenterFeedbackCron { |
There was a problem hiding this comment.
Can we update the name of the file so that we identify it's an service file
| * | ||
| */ | ||
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | ||
| } |
There was a problem hiding this comment.
Add comment here // There are no parameters for the Goonjcustom cron. so that it wont blank
| // Get all dropping centers where the center's open date is older than 6 months | ||
| // and the last feedback email was either never sent or sent more than 6 months ago. | ||
| $droppingCenters = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') |
There was a problem hiding this comment.
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') | |
| ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') |
remove When_do_you_wish_to_open_center_Date_ ,id as we are not using those anywhere
| [$defaultFromName, $defaultFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(); | ||
| $from = "\"$defaultFromName\" <$defaultFromEmail>"; |
There was a problem hiding this comment.
We have created a helper function for this please check on this file /wp-content/civi-extensions/goonjcustom/Civi/HelperService.php
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| // Fetch the email and name of the initiator. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Check the status of the dropping center | ||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') |
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| // Fetch the email and name of the initiator. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Check the status of the dropping center | ||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') |
There was a problem hiding this comment.
Check the spelling of Parmanently_Closed
| if (!$status) { | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackCron::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
can we return it before is there were status is there
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('subtype', '=', $statusName) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') |
| [$defaultFromName, $defaultFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(); | ||
| $from = "\"$defaultFromName\" <$defaultFromEmail>"; |
There was a problem hiding this comment.
Use helper function for those
| if (!$status) { | ||
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update status if the email is sent. | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1)
1-11: Consider splitting responsibilities into separate services.The class currently handles multiple responsibilities: email generation, sending, and status management. Consider splitting these into separate services:
DroppingCenterEmailServicefor email operationsDroppingCenterStatusServicefor status management
This would better align with the Single Responsibility Principle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php
🔇 Additional comments (7)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (7)
35-36: Enhance email sender information retrievalCurrently, the code retrieves the default domain name and email but doesn't handle the possibility of missing data, which could cause issues.
Consider adding a fallback or validation:
[$defaultFromName, $defaultFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(); +if (empty($defaultFromEmail)) { + throw new Exception('Default From Email is not configured.'); +} $from = "\"$defaultFromName\" <$defaultFromEmail>";
9-9: Ensure the DroppingCenterFeedbackService class follows single responsibilityThe
DroppingCenterFeedbackServiceclass should focus solely on sending feedback emails. Verify that it doesn't contain unrelated logic, which could violate the single responsibility principle.Run the following script to check the class methods:
✅ Verification successful
Let me try a different approach to verify the content of this class.
DroppingCenterFeedbackService follows single responsibility principle - no issues found
After reviewing the implementation, the
DroppingCenterFeedbackServiceclass strictly adheres to the single responsibility principle. It has three well-focused methods that work together for the sole purpose of managing feedback emails for Dropping Centers:
processDroppingCenterStatus: Handles the business logic for determining when to send feedbacksendFeedbackEmail: Manages the email sending mechanicsgenerateFeedbackEmailHtml: Focuses on email content generationThe class is well-documented, cohesive, and doesn't contain any unrelated functionality. Each method has a clear, specific purpose that contributes to the main responsibility of managing feedback emails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all methods in DroppingCenterFeedbackService class. # Test: Extract method names. Expect: Only methods related to feedback email. ast-grep --pattern 'class DroppingCenterFeedbackService { $$$ function $_($$) { $$$ } $$$ }' wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.phpLength of output: 184
Script:
#!/bin/bash # Let's first check if the file exists and see its content cat wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php # If the file is not found at that location, let's search for it fd DroppingCenterFeedbackService.php # Let's also try to find any references to this class in the codebase rg "DroppingCenterFeedbackService" -A 3Length of output: 5922
58-58:⚠️ Potential issueCorrect the typo in status name to prevent logic errors
There's a typo in the status name
'Parmanently_Closed'. It should be'Permanently_Closed'. This error may prevent the query from matching any records.Apply this correction:
->addWhere('Status.Status:name', '=', 'Parmanently_Closed') +->addWhere('Status.Status:name', '=', 'Permanently_Closed')Likely invalid or redundant comment.
24-24: 🛠️ Refactor suggestionExternalize the threshold date for flexibility and maintainability
The threshold of 6 months is hard-coded, which reduces flexibility. Moving this value to a configuration setting or making it a parameter enhances adaptability and makes future changes easier.
Consider retrieving the threshold from a configuration setting:
-$thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); +$monthsThreshold = Civi::settings()->get('dropping_center_feedback_months_threshold') ?? 6; +$thresholdDate = (new \DateTime())->modify("-{$monthsThreshold} months")->format('Y-m-d');Likely invalid or redundant comment.
54-60: 🛠️ Refactor suggestionConsolidate status checks to improve readability
The status check for the dropping center is split between lines 31 and 58, causing confusion.
Consider consolidating the status checks into the main query to streamline logic:
$droppingCenters = EckEntity::get('Collection_Camp', TRUE) ->addSelect('Dropping_Centre.When_do_you_wish_to_open_center_Date_', 'id', 'Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') ->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate) - ->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized') + ->addWhere('Collection_Camp_Core_Details.Status:name', 'NOT IN', ['Permanently_Closed']) ->addClause('OR', ['Dropping_Centre.last_feedback_sent_date', 'IS NULL'], ['Dropping_Centre.last_feedback_sent_date', '<=', $thresholdDate]) ->execute();Likely invalid or redundant comment.
83-83: 🛠️ Refactor suggestionReturn meaningful processing statistics
The
$returnValuesarray is empty, so the API response does not provide insight into the cron job's effectiveness. Including statistics will improve monitoring and debugging.Populate
$returnValueswith processing details:$returnValues = []; +$returnValues['processed_centers'] = count($droppingCenters); +$returnValues['timestamp'] = date('Y-m-d H:i:s'); return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron');Likely invalid or redundant comment.
31-32:⚠️ Potential issueResolve inconsistent status checks to ensure correct data retrieval
The query filters dropping centers with status
'Authorized'(line 31), but later checks if the status is'Parmanently_Closed'(line 58). A dropping center cannot be both'Authorized'and'Parmanently_Closed'simultaneously. This inconsistency may result in no centers being processed.To align the logic, consider updating the status check:
->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized') +->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Permanently_Closed')Also, correct the typo
'Parmanently_Closed'to'Permanently_Closed'in line 58.Likely invalid or redundant comment.
| public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; | ||
|
|
||
| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
|
|
||
| <p>Thank you for being such an outstanding representative of Goonj! | ||
| Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> | ||
|
|
||
| <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. | ||
| If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> | ||
|
|
||
| <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> | ||
|
|
||
| <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> | ||
|
|
||
| <p>We look forward to continuing this important journey with you!</p> | ||
|
|
||
| <p>Warm Regards,<br> | ||
| Team Goonj</p> | ||
| "; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
Move email template to a separate file and add security measures.
Several improvements needed:
- Move HTML template to a separate file
- Add XSS protection for user input
- Make URL path configurable
[security]
Here's a suggested improvement:
+ const FEEDBACK_URL_PATH = 'volunteer-feedback/#?Eck_Collection_Camp1=';
public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) {
$homeUrl = \CRM_Utils_System::baseCMSURL();
- $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId;
+ $volunteerFeedback = $homeUrl . self::FEEDBACK_URL_PATH . (int)$droppingCenterId;
+ $organizingContactName = \CRM_Utils_String::htmlEscape($organizingContactName);
- $html = "
- <p>Dear $organizingContactName,</p>
- ...
- ";
+ $templatePath = \Civi::paths()->getPath('[civicrm.root]/templates/dropping-center-feedback.html');
+ $template = file_get_contents($templatePath);
+ $html = str_replace(
+ ['{{name}}', '{{feedback_url}}'],
+ [$organizingContactName, $volunteerFeedback],
+ $template
+ );
return $html;
}Create a new template file at templates/dropping-center-feedback.html:
<p>Dear {{name}},</p>
<p>Thank you for being such an outstanding representative of Goonj!
Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p>
<p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center.
If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p>
<p><a href='{{feedback_url}}'>Click here to access the feedback form.</a></p>
<p>We encourage you to share any highlights, suggestions, or challenges you've encountered. Together, we can refine and enhance this process even further.</p>
<p>We look forward to continuing this important journey with you!</p>
<p>Warm Regards,<br>
Team Goonj</p>| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | ||
| $mailParams = [ | ||
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | ||
| 'from' => $from, | ||
| 'toEmail' => $contactEmailId, | ||
| 'replyTo' => $from, | ||
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | ||
| ]; | ||
|
|
||
| // Send the email. | ||
| \CRM_Utils_Mail::send($mailParams); | ||
| } |
There was a problem hiding this comment.
Enhance email sending robustness and configurability.
The email sending logic needs improvements:
- Add parameter validation
- Add error handling for mail sending
- Move email subject to configuration
Here's a suggested improvement:
+ const EMAIL_SUBJECT = 'Your Feedback on Managing the Goonj Dropping Center';
public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) {
+ if (empty($contactEmailId) || empty($from)) {
+ throw new \InvalidArgumentException('Missing required email parameters');
+ }
+ try {
$mailParams = [
- 'subject' => 'Your Feedback on Managing the Goonj Dropping Center',
+ 'subject' => self::EMAIL_SUBJECT,
'from' => $from,
'toEmail' => $contactEmailId,
'replyTo' => $from,
'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId),
];
- \CRM_Utils_Mail::send($mailParams);
+ $sent = \CRM_Utils_Mail::send($mailParams);
+ if (!$sent) {
+ throw new \RuntimeException('Failed to send feedback email');
+ }
+ return $sent;
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to send feedback email: ' . $e->getMessage());
+ throw $e;
+ }
}📝 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.
| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | |
| $mailParams = [ | |
| 'subject' => 'Your Feedback on Managing the Goonj Dropping Center', | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | |
| ]; | |
| // Send the email. | |
| \CRM_Utils_Mail::send($mailParams); | |
| } | |
| const EMAIL_SUBJECT = 'Your Feedback on Managing the Goonj Dropping Center'; | |
| public static function sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from) { | |
| if (empty($contactEmailId) || empty($from)) { | |
| throw new \InvalidArgumentException('Missing required email parameters'); | |
| } | |
| try { | |
| $mailParams = [ | |
| 'subject' => self::EMAIL_SUBJECT, | |
| 'from' => $from, | |
| 'toEmail' => $contactEmailId, | |
| 'replyTo' => $from, | |
| 'html' => self::generateFeedbackEmailHtml($organizingContactName, $droppingCenterId), | |
| ]; | |
| $sent = \CRM_Utils_Mail::send($mailParams); | |
| if (!$sent) { | |
| throw new \RuntimeException('Failed to send feedback email'); | |
| } | |
| return $sent; | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to send feedback email: ' . $e->getMessage()); | |
| throw $e; | |
| } | |
| } |
| public static function processDroppingCenterStatus($meta, $from) { | ||
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| // Get recipient email and name. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| if (!$status) { | ||
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update status if the email is sent. | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation and error handling.
Several improvements needed for robustness:
- Validate input parameters before processing
- Add error handling for API calls
- Handle potential null email addresses
- Replace magic number with constant
Here's a suggested improvement:
+ const STATUS_EMAIL_DELIVERED = 1;
public static function processDroppingCenterStatus($meta, $from) {
+ if (empty($meta) || empty($from)) {
+ throw new \InvalidArgumentException('Missing required parameters');
+ }
$droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center'];
$initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id'];
$status = $meta['Status.Feedback_Email_Delivered:name'];
+ try {
$campAttendedBy = Contact::get(TRUE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $initiatorId)
->execute()->single();
+ if (empty($campAttendedBy['email.email'])) {
+ throw new \RuntimeException("No email found for contact $initiatorId");
+ }
$contactEmailId = $campAttendedBy['email.email'];
$organizingContactName = $campAttendedBy['display_name'];
if (!$status) {
self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
EckEntity::update('Dropping_Center_Meta', TRUE)
- ->addValue('Status.Feedback_Email_Delivered:name', 1)
+ ->addValue('Status.Feedback_Email_Delivered:name', self::STATUS_EMAIL_DELIVERED)
->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId)
->execute();
}
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to process dropping center status: ' . $e->getMessage());
+ throw $e;
+ }
}📝 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.
| public static function processDroppingCenterStatus($meta, $from) { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| // Get recipient email and name. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update status if the email is sent. | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } | |
| const STATUS_EMAIL_DELIVERED = 1; | |
| public static function processDroppingCenterStatus($meta, $from) { | |
| if (empty($meta) || empty($from)) { | |
| throw new \InvalidArgumentException('Missing required parameters'); | |
| } | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| try { | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if (empty($campAttendedBy['email.email'])) { | |
| throw new \RuntimeException("No email found for contact $initiatorId"); | |
| } | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| if (!$status) { | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', self::STATUS_EMAIL_DELIVERED) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to process dropping center status: ' . $e->getMessage()); | |
| throw $e; | |
| } | |
| } |
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | ||
| } |
There was a problem hiding this comment.
Implement API specification function for better validation
The _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec function is currently empty. According to CiviCRM API v3 best practices, this function should define the expected parameters and their data types. This enhances validation and documentation.
Consider adding parameter specifications, even if the API does not require any parameters:
function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) {
+ // No parameters required for this API.
+ $spec['limit'] = [
+ 'title' => 'Limit',
+ 'type' => CRM_Utils_Type::T_INT,
+ 'description' => 'Maximum number of dropping centers to process',
+ 'api.default' => 50,
+ ];
}📝 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.
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | |
| } | |
| function _civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron_spec(&$spec) { | |
| // No parameters required for this API. | |
| $spec['limit'] = [ | |
| 'title' => 'Limit', | |
| 'type' => CRM_Utils_Type::T_INT, | |
| 'description' => 'Maximum number of dropping centers to process', | |
| 'api.default' => 50, | |
| ]; | |
| } |
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered']; | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| if (!$status) { | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent multiple emails due to duplicate meta records
If multiple Dropping_Center_Meta records exist for a single dropping center, the inner loop may send multiple feedback emails to the same contact, leading to spamming.
To ensure only one email is sent per dropping center, restructure the code:
foreach ($droppingCenterMeta as $meta) {
$status = $meta['Status.Feedback_Email_Delivered'];
- if (!$status) {
- // Send the feedback email.
- DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
-
- // Update the last_feedback_sent_date to the current date after the email is sent.
- EckEntity::update('Collection_Camp', TRUE)
- ->addWhere('id', '=', $droppingCenterId)
- ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d'))
- ->execute();
- }
+ if ($status) {
+ // Feedback email already sent; no action needed.
+ continue;
+ }
+ // Meta record requires feedback email.
+ $needsEmail = true;
+ break; // Exit loop if email is needed.
}
+if (isset($needsEmail)) {
+ // Send the feedback email.
+ DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
+
+ // Update the last_feedback_sent_date to the current date after the email is sent.
+ EckEntity::update('Collection_Camp', TRUE)
+ ->addWhere('id', '=', $droppingCenterId)
+ ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d'))
+ ->execute();
+}📝 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.
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| if ($status) { | |
| // Feedback email already sent; no action needed. | |
| continue; | |
| } | |
| // Meta record requires feedback email. | |
| $needsEmail = true; | |
| break; // Exit loop if email is needed. | |
| } | |
| if (isset($needsEmail)) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } |
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| // Fetch the email and name of the initiator. | ||
| $campAttendedBy = Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
|
|
||
| if ($campAttendedBy) { | ||
| $contactEmailId = $campAttendedBy['email.email']; | ||
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Check the status of the dropping center | ||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | ||
| ->execute(); | ||
|
|
||
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered']; | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| if (!$status) { | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Enhance error handling within the processing loop
Catching exceptions at a high level may cause the entire process to halt if an error occurs with one dropping center. Implement per-dropping center error handling to ensure one failure doesn't impact others.
Modify the try-catch structure:
try {
foreach ($droppingCenters as $center) {
+ try {
// Existing processing logic...
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID ' . $center['id'] . ': ' . $e->getMessage());
+ // Optionally continue to the next center
+ continue;
+ }
}
-}
-catch (Exception $e) {
- \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $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.
| try { | |
| foreach ($droppingCenters as $center) { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| // Fetch the email and name of the initiator. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| // Check the status of the dropping center | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | |
| } | |
| try { | |
| foreach ($droppingCenters as $center) { | |
| try { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| // Fetch the email and name of the initiator. | |
| $campAttendedBy = Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| if ($campAttendedBy) { | |
| $contactEmailId = $campAttendedBy['email.email']; | |
| $organizingContactName = $campAttendedBy['display_name']; | |
| // Check the status of the dropping center | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name','Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered']; | |
| // Send email only if not delivered and not permanently closed | |
| if (!$status) { | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| } | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID ' . $center['id'] . ': ' . $e->getMessage()); | |
| // Optionally continue to the next center | |
| continue; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1)
21-23: Enhance API specification documentation.While it's valid to have no parameters, it would be helpful to explicitly document why parameters aren't needed for this cron job.
function _civicrm_api3_goonjcustom_feedback_dropping_center_cron_spec(&$spec) { - // There are no parameters for the Goonjcustom cron. + // This cron job runs automatically without requiring any input parameters + // as it processes all permanently closed dropping centers that haven't received feedback emails. }wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1)
29-30: Rename$contactEmailIdto$contactEmailfor clarity.The variable
$contactEmailIdholds an email address, not an identifier. Renaming it to$contactEmailimproves readability and reduces confusion.Apply this diff:
- $contactEmailId = $contactDetails['email.email']; + $contactEmail = $contactDetails['email.email'];And update all occurrences of
$contactEmailIdto$contactEmail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
48-49:⚠️ Potential issueAdd null check before accessing array key.
The code assumes the 'value' key exists in
$optionValues. This could lead to undefined index errors.+ if (empty($optionValues) || !isset($optionValues['value'])) { + throw new CRM_Core_Exception('Required Status option value not found'); + } $statusName = $optionValues['value'];Likely invalid or redundant comment.
37-39: 🛠️ Refactor suggestionReturn meaningful statistics from the cron job.
The current implementation returns an empty array, making it difficult to monitor the cron job's effectiveness.
function civicrm_api3_goonjcustom_feedback_dropping_center_cron($params) { - $returnValues = []; + $returnValues = [ + 'processed' => [], // Successfully processed dropping centers + 'failed' => [], // Failed dropping centers with error messages + 'total_processed' => 0, + 'total_failed' => 0, + 'execution_time' => 0, + ]; + $startTime = microtime(true);And at the end of the function:
+ $returnValues['total_processed'] = count($returnValues['processed']); + $returnValues['total_failed'] = count($returnValues['failed']); + $returnValues['execution_time'] = microtime(true) - $startTime; return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'feedback_dropping_center_cron');Likely invalid or redundant comment.
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1)
66-66:⚠️ Potential issueAdd Global Namespace Prefix to
ExceptionTo ensure consistency and avoid potential namespace issues, add the global namespace prefix to the
Exceptionclass in the catch block.Apply this change:
- catch (Exception $e) { + catch (\Exception $e) {Likely invalid or redundant comment.
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('subtype', '=', $statusName) | ||
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | ||
| ->execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add pagination to prevent memory issues with large datasets.
The query fetches all dropping centers at once, which could cause memory issues if there are many records.
$droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE)
->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name')
->addWhere('subtype', '=', $statusName)
->addWhere('Status.Status:name', '=', 'Permanently_Closed')
+ ->setLimit(50) // Process in batches of 50
->execute();📝 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.
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | |
| ->addWhere('subtype', '=', $statusName) | |
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | |
| ->execute(); | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name', 'Status.Closing_Date', 'Dropping_Center_Meta.Dropping_Center', 'Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id', 'Status.Feedback_Email_Delivered:name') | |
| ->addWhere('subtype', '=', $statusName) | |
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | |
| ->setLimit(50) // Process in batches of 50 | |
| ->execute(); |
| try { | ||
| foreach ($droppingCenterMeta as $meta) { | ||
|
|
||
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | ||
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $meta['Dropping_Center_Meta.Dropping_Center']); | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve error handling and add transaction management.
The current error handling only logs the error without proper cleanup or reporting. Additionally, database operations should be wrapped in a transaction.
+ $transaction = new CRM_Core_Transaction();
try {
foreach ($droppingCenterMeta as $meta) {
+ try {
$droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center'];
$initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id'];
$status = $meta['Status.Feedback_Email_Delivered:name'];
DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from);
+ $returnValues['processed'][] = $droppingCenterId;
+ } catch (Exception $e) {
+ $returnValues['failed'][] = [
+ 'id' => $droppingCenterId,
+ 'error' => $e->getMessage()
+ ];
+ \CRM_Core_Error::debug_log_message(
+ "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage()
+ );
+ }
}
+ $transaction->commit();
}
catch (Exception $e) {
- \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $meta['Dropping_Center_Meta.Dropping_Center']);
+ $transaction->rollback();
+ throw new CRM_Core_Exception('Critical error in feedback cron: ' . $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.
| try { | |
| foreach ($droppingCenterMeta as $meta) { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| } | |
| } | |
| catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $meta['Dropping_Center_Meta.Dropping_Center']); | |
| } | |
| $transaction = new CRM_Core_Transaction(); | |
| try { | |
| foreach ($droppingCenterMeta as $meta) { | |
| try { | |
| $droppingCenterId = $meta['Dropping_Center_Meta.Dropping_Center']; | |
| $initiatorId = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Contact_Id']; | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| $returnValues['processed'][] = $droppingCenterId; | |
| } catch (Exception $e) { | |
| $returnValues['failed'][] = [ | |
| 'id' => $droppingCenterId, | |
| 'error' => $e->getMessage() | |
| ]; | |
| \CRM_Core_Error::debug_log_message( | |
| "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage() | |
| ); | |
| } | |
| } | |
| $transaction->commit(); | |
| } | |
| catch (Exception $e) { | |
| $transaction->rollback(); | |
| throw new CRM_Core_Exception('Critical error in feedback cron: ' . $e->getMessage()); | |
| } |
| function civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron($params) { | ||
| $returnValues = []; | ||
|
|
||
| // Set threshold date to 6 months ago. | ||
| $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); | ||
|
|
||
| // Get all dropping centers where the center's open date is older than 6 months | ||
| // and the last feedback email was either never sent or sent more than 6 months ago. | ||
| $droppingCenters = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date') | ||
| ->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate) | ||
| ->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized') | ||
| ->addClause('OR', ['Dropping_Centre.last_feedback_sent_date', 'IS NULL'], ['Dropping_Centre.last_feedback_sent_date', '<=', $thresholdDate]) | ||
| ->execute(); | ||
|
|
||
| $from = HelperService::getDefaultFromEmail(); | ||
|
|
||
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | ||
| ->execute(); | ||
|
|
||
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| } | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Function for Single Responsibility
The civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cron function handles multiple responsibilities, including data retrieval, processing logic, sending emails, and updating records. Refactoring the code to separate these concerns can enhance readability and maintainability.
Consider the following refactoring steps:
- Data Retrieval: Extract the logic for fetching dropping centers into a separate method or service.
- Processing Logic: Create a dedicated method to process each dropping center, handling status checks and email sending.
- Email Sending: Leverage the
DroppingCenterFeedbackServiceto encapsulate email-related operations. - Error Handling: Implement per-dropping center error handling to prevent one failure from affecting the entire process.
This adheres to the Single Responsibility Principle and makes future modifications more manageable.
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Ensure Correct Handling of $status Variable
The $status variable is assigned within the foreach loop over $droppingCenterMeta (lines 50-53), but it's used outside the loop at line 57. If $droppingCenterMeta contains multiple records, $status will retain the value from the last iteration, which may not be the intended behavior. This could lead to incorrect processing in processDroppingCenterStatus().
Consider processing each meta record individually or aggregating statuses appropriately. Here's a possible fix:
foreach ($droppingCenterMeta as $meta) {
$status = $meta['Status.Feedback_Email_Delivered:name'];
+ // Send email only if not delivered and not permanently closed
+ DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from);
}
- // Send email only if not delivered and not permanently closed
- // Send the feedback email.
- DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from);Committable suggestion was skipped due to low confidence.
| public static function processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from) { | ||
|
|
||
| // Get recipient email and name. | ||
| $contactDetails = self::getContactDetails($initiatorId); | ||
|
|
||
| if ($contactDetails) { | ||
| $contactEmailId = $contactDetails['email.email']; | ||
| $organizingContactName = $contactDetails['display_name']; | ||
|
|
||
| if (!$status) { | ||
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update status if the email is sent. | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for critical operations.
The method processDroppingCenterStatus performs database updates and email sending without error handling. If any of these operations fail, the system may not behave as expected.
Consider wrapping the email sending and status update in try-catch blocks or checking the return values to handle exceptions and ensure the system remains consistent.
| * @param array $meta | ||
| * Meta data for the Dropping Center. | ||
| * @param string $from | ||
| * Email address for 'from'. | ||
| * | ||
| * @throws \CRM_Core_Exception | ||
| */ |
There was a problem hiding this comment.
Update method documentation to reflect actual parameters.
The PHPDoc for processDroppingCenterStatus does not match the method's parameters. The documentation refers to $meta, but the method accepts $droppingCenterId, $initiatorId, $status, and $from.
Apply this diff to correct the PHPDoc:
/**
- * Send feedback email if it hasn't been sent yet.
+ * Process the dropping center status and send feedback email if it hasn't been sent yet.
*
- * @param array $meta
- * Meta data for the Dropping Center.
+ * @param int $droppingCenterId
+ * ID of the Dropping Center.
+ * @param int $initiatorId
+ * ID of the initiator.
+ * @param bool $status
+ * Feedback email delivery status.
* @param string $from
* Email address for 'from'.
*
* @throws \CRM_Core_Exception
*/📝 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.
| * @param array $meta | |
| * Meta data for the Dropping Center. | |
| * @param string $from | |
| * Email address for 'from'. | |
| * | |
| * @throws \CRM_Core_Exception | |
| */ | |
| /** | |
| * Process the dropping center status and send feedback email if it hasn't been sent yet. | |
| * | |
| * @param int $droppingCenterId | |
| * ID of the Dropping Center. | |
| * @param int $initiatorId | |
| * ID of the initiator. | |
| * @param bool $status | |
| * Feedback email delivery status. | |
| * @param string $from | |
| * Email address for 'from'. | |
| * | |
| * @throws \CRM_Core_Exception | |
| */ |
| $contactEmailId = $contactDetails['email.email']; | ||
| $organizingContactName = $contactDetails['display_name']; | ||
|
|
||
| if (!$status) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more descriptive variable name for $status.
The variable $status is ambiguous. Renaming it to $feedbackEmailDelivered or $isFeedbackEmailSent makes the code more understandable.
Apply this diff:
- public static function processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from) {
+ public static function processDroppingCenterStatus($droppingCenterId, $initiatorId, $isFeedbackEmailSent, $from) {
...
- if (!$status) {
+ if (!$isFeedbackEmailSent) {And update any other references accordingly.
Committable suggestion was skipped due to low confidence.
belwalshubham
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (2)
3-5: Add proper PHPDoc documentation blocksThe file and function documentation blocks are empty. Add comprehensive documentation to improve code maintainability.
/** + * BiannualDroppingCenterFeedbackCron.php + * + * Implements a cron job that sends feedback emails to dropping centers + * every six months based on their opening date. */ /** + * API function specification for biannual dropping center feedback cron. + * + * This API doesn't require any parameters as it processes all eligible + * dropping centers based on predefined criteria. + * + * @return array API specification */Also applies to: 11-13
21-69: Refactor for Single Responsibility PrincipleThe function handles multiple responsibilities: data retrieval, processing logic, email sending, and record updates. This violates the Single Responsibility Principle.
Consider extracting these responsibilities into separate methods in the DroppingCenterFeedbackService class:
getEligibleDroppingCenters()- Data retrievalprocessDroppingCenter()- Individual center processingupdateFeedbackSentDate()- Record updatesThis would make the code more maintainable and testable.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1)
11-11: Consider improving architecture with better separation of concerns.The class currently handles multiple responsibilities:
- Email content generation
- Email sending
- Status management
- Contact details retrieval
Consider splitting these into separate services:
DroppingCenterEmailService: For email generation and sendingDroppingCenterStatusService: For status managementContactDetailsService: For contact information retrievalThis would improve maintainability and testability.
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 (3)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php
🔇 Additional comments (5)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (3)
68-68: 🛠️ Refactor suggestionEnhance API response with processing statistics
The current implementation returns empty values, making it difficult to monitor the cron job's effectiveness.
+ $returnValues = [ + 'processed' => count($droppingCenters), + 'threshold_date' => $thresholdDate, + 'timestamp' => date('Y-m-d H:i:s'), + ]; return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'biannual_dropping_center_feedback_cron');Likely invalid or redundant comment.
48-55:⚠️ Potential issueFix status variable scope and processing logic
The status variable is set inside a loop but used outside, leading to potential bugs. Additionally, the processing logic should be inside the loop.
foreach ($droppingCenterMeta as $meta) { $status = $meta['Status.Feedback_Email_Delivered:name']; + if (!$status) { + // Send email only if not delivered + DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); + break; // Process only once per dropping center + } } - // Send email only if not delivered and not permanently closed - // Send the feedback email. - DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from);Likely invalid or redundant comment.
24-25: 🛠️ Refactor suggestionMake the threshold period configurable
The 6-month threshold is hardcoded, reducing flexibility. Move this to a configuration setting.
- $thresholdDate = (new \DateTime())->modify('-6 months')->format('Y-m-d'); + $monthsThreshold = Civi::settings()->get('dropping_center_feedback_months_threshold') ?? 6; + $thresholdDate = (new \DateTime())->modify("-{$monthsThreshold} months")->format('Y-m-d');Likely invalid or redundant comment.
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (2)
74-85: Previous review comments are still applicable.The existing review comments about enhancing email sending robustness and configurability are still valid and should be addressed.
95-119: Previous review comments are still applicable.The existing review comments about moving the email template to a separate file and adding security measures are still valid and should be addressed.
| try { | ||
| foreach ($droppingCenters as $center) { | ||
| $droppingCenterId = $center['id']; | ||
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | ||
|
|
||
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | ||
| ->addSelect('Status.Status:name', 'Status.Feedback_Email_Delivered:name') | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | ||
| ->execute(); | ||
|
|
||
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| } | ||
|
|
||
| // Send email only if not delivered and not permanently closed | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); | ||
| } | ||
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Improve error handling granularity
The current error handling catches exceptions at too high a level, potentially masking individual failures.
try {
foreach ($droppingCenters as $center) {
+ try {
$droppingCenterId = $center['id'];
// ... existing processing logic ...
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message(
+ "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage()
+ );
+ continue; // Continue with next center
+ }
}
-} catch (Exception $e) {
- \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $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.
| try { | |
| foreach ($droppingCenters as $center) { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name', 'Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| } | |
| // Send email only if not delivered and not permanently closed | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } | |
| } | |
| catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center feedback: ' . $e->getMessage()); | |
| } | |
| foreach ($droppingCenters as $center) { | |
| try { | |
| $droppingCenterId = $center['id']; | |
| $initiatorId = $center['Collection_Camp_Core_Details.Contact_Id']; | |
| $droppingCenterMeta = EckEntity::get('Dropping_Center_Meta', TRUE) | |
| ->addSelect('Status.Status:name', 'Status.Feedback_Email_Delivered:name') | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') | |
| ->execute(); | |
| foreach ($droppingCenterMeta as $meta) { | |
| $status = $meta['Status.Feedback_Email_Delivered:name']; | |
| } | |
| // Send email only if not delivered and not permanently closed | |
| // Send the feedback email. | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $droppingCenterId) | |
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | |
| ->execute(); | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message( | |
| "Error processing Dropping Center ID {$droppingCenterId}: " . $e->getMessage() | |
| ); | |
| continue; // Continue with next center | |
| } | |
| } |
| protected static function getContactDetails($initiatorId) { | ||
| return Contact::get(TRUE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $initiatorId) | ||
| ->execute()->single(); | ||
| } |
There was a problem hiding this comment.
Add error handling and parameter validation.
The method should validate input and handle potential API exceptions.
Apply this improvement:
protected static function getContactDetails($initiatorId) {
+ if (empty($initiatorId) || !is_numeric($initiatorId)) {
+ throw new \InvalidArgumentException('Invalid initiator ID provided');
+ }
+
+ try {
return Contact::get(TRUE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $initiatorId)
->execute()->single();
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to retrieve contact details: ' . $e->getMessage());
+ throw $e;
+ }
}📝 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.
| protected static function getContactDetails($initiatorId) { | |
| return Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| } | |
| protected static function getContactDetails($initiatorId) { | |
| if (empty($initiatorId) || !is_numeric($initiatorId)) { | |
| throw new \InvalidArgumentException('Invalid initiator ID provided'); | |
| } | |
| try { | |
| return Contact::get(TRUE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $initiatorId) | |
| ->execute()->single(); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to retrieve contact details: ' . $e->getMessage()); | |
| throw $e; | |
| } | |
| } |
| if ($contactDetails) { | ||
| $contactEmailId = $contactDetails['email.email']; | ||
| $organizingContactName = $contactDetails['display_name']; | ||
|
|
||
| if ($status) { | ||
| return; | ||
| } | ||
|
|
||
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | ||
|
|
||
| // Update status if the email is sent. | ||
| EckEntity::update('Dropping_Center_Meta', TRUE) | ||
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | ||
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add explicit handling for missing contact details.
The method silently continues if contactDetails is null. This could lead to dropped emails without any logging or notification.
Consider adding explicit error handling:
if ($contactDetails) {
$contactEmailId = $contactDetails['email.email'];
$organizingContactName = $contactDetails['display_name'];
if ($status) {
return;
}
self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from);
// Update status if the email is sent.
EckEntity::update('Dropping_Center_Meta', TRUE)
->addValue('Status.Feedback_Email_Delivered:name', 1)
->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId)
->execute();
+ } else {
+ \Civi::log()->warning("Could not send feedback email - contact details not found for initiator ID: $initiatorId");
}📝 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 ($contactDetails) { | |
| $contactEmailId = $contactDetails['email.email']; | |
| $organizingContactName = $contactDetails['display_name']; | |
| if ($status) { | |
| return; | |
| } | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update status if the email is sent. | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } | |
| } | |
| if ($contactDetails) { | |
| $contactEmailId = $contactDetails['email.email']; | |
| $organizingContactName = $contactDetails['display_name']; | |
| if ($status) { | |
| return; | |
| } | |
| self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); | |
| // Update status if the email is sent. | |
| EckEntity::update('Dropping_Center_Meta', TRUE) | |
| ->addValue('Status.Feedback_Email_Delivered:name', 1) | |
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | |
| ->execute(); | |
| } else { | |
| \Civi::log()->warning("Could not send feedback email - contact details not found for initiator ID: $initiatorId"); | |
| } | |
| } |
| foreach ($droppingCenterMeta as $meta) { | ||
| $status = $meta['Status.Feedback_Email_Delivered:name']; | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Should we need this loop here?
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $droppingCenterId) | ||
| ->addValue('Dropping_Centre.last_feedback_sent_date', (new \DateTime())->format('Y-m-d')) | ||
| ->execute(); |
There was a problem hiding this comment.
Move this logic to service file
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | ||
| ->addWhere('name', '=', 'Status') | ||
| ->addWhere('grouping', '=', 'Dropping_Center_Meta') | ||
| ->setLimit(1) |
There was a problem hiding this comment.
| ->setLimit(1) |
Should we need this here? If you are using ->single(); then we don't need ->setLimit(1) please remove it and test the feature
|
|
||
| try { | ||
| foreach ($droppingCenterMeta as $meta) { | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1)
105-129: Enhance readability by using HEREDOC syntax for HTML content.Embedding the HTML content using HEREDOC syntax improves readability and maintainability of the code.
Apply this diff to use HEREDOC syntax:
public static function generateFeedbackEmailHtml($organizingContactName, $droppingCenterId) { $homeUrl = \CRM_Utils_System::baseCMSURL(); $volunteerFeedback = $homeUrl . 'volunteer-feedback/#?Eck_Collection_Camp1=' . $droppingCenterId; - $html = " + $html = <<<HTML <p>Dear $organizingContactName,</p> <p>Thank you for being such an outstanding representative of Goonj! Your dedication, time, and passion are truly making a difference as we work to bring essential materials to remote villages across the country.</p> <p>As part of our commitment to constant improvement, we would greatly appreciate hearing about your experience managing the Dropping Center. If you could spare a few moments to complete our feedback form, your input would be invaluable to us!</p> <p><a href='$volunteerFeedback'>Click here to access the feedback form.</a></p> <p>We encourage you to share any highlights, suggestions, or challenges you’ve encountered. Together, we can refine and enhance this process even further.</p> <p>We look forward to continuing this important journey with you!</p> <p>Warm Regards,<br> Team Goonj</p> - "; + HTML; return $html; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (4)
23-43: MethodprocessDroppingCenterStatusadheres to best practices.The method effectively processes the Dropping Center status and sends feedback emails as required, maintaining clarity and single responsibility.
54-60: MethodgetContactDetailsis clear and efficient.The method retrieves contact details appropriately and follows best coding practices.
65-70: MethodupdateFeedbackLastSentDateis concise and accurate.The method correctly updates the
last_feedback_sent_date, aligning with coding standards.
84-95: MethodsendFeedbackEmailcorrectly constructs and sends the email.The method sets up email parameters effectively and utilizes the CRM utility to send emails appropriately.
|
@tarunnjoshi done with feedback comment. |
2 participants
Feedback_Email_Deliveredso that After the email is successfully sent, the Feedback_Email_Delivered status is updated to 1 (true) for that Dropping Center.This prevents the same email from being sent again in the next cron execution for the same center.Summary by CodeRabbit
New Features
Bug Fixes