Do Not Send Feedback Emails for Closed Status in Biannual Cron#399
Do Not Send Feedback Emails for Closed Status in Biannual Cron#399belwalshubham merged 2 commits intodevelopfrom
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (3)
Line range hint 25-35: Consider optimizing the dropping centers query for better performance.
The current implementation fetches all authorized centers first and then performs additional queries for each center to check their status. This could be inefficient for large datasets.
Consider combining the queries using a JOIN operation:
$droppingCenters = EckEntity::get('Collection_Camp', TRUE)
->addSelect('Collection_Camp_Core_Details.Contact_Id', 'Dropping_Centre.last_feedback_sent_date')
+ ->addJoin('Dropping_Center_Meta AS dcm', 'LEFT', ['id', '=', 'dcm.Dropping_Center'])
->addWhere('Dropping_Centre.When_do_you_wish_to_open_center_Date_', '<=', $thresholdDate)
->addWhere('Collection_Camp_Core_Details.Status:name', '=', 'Authorized')
+ ->addWhere('dcm.Status.Status:name', '!=', 'Permanently_Closed')
->addClause('OR', ['Dropping_Centre.last_feedback_sent_date', 'IS NULL'], ['Dropping_Centre.last_feedback_sent_date', '<=', $thresholdDate])
->execute();Line range hint 38-45: Remove unused status variable.
The $status variable is fetched from $droppingCenterMeta but never used in the actual processing logic.
Remove the unused code:
- $status = !empty($droppingCenterMeta) ? $droppingCenterMeta[0]['Status.Feedback_Email_Delivered'] : null;
// Send email only if not delivered and not permanently closed60-60: Enhance debug logging with additional context.
The current debug message could provide more useful information for troubleshooting.
Consider enhancing the log message:
- \CRM_Core_Error::debug_log_message("Feedback cron: Dropping Center ID {$droppingCenterId} is marked as 'Permanently Closed'. As a result, no email will be sent by the biannual feedback cron.");
+ \CRM_Core_Error::debug_log_message("Feedback cron: Skipping email for Dropping Center ID {$droppingCenterId} (Contact: {$initiatorId}). Reason: Center is marked as 'Permanently Closed' since " . $droppingCenterMeta[0]['Status.Feedback_Email_Delivered']);📜 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)
| if (empty($droppingCenterMeta[0])) { | ||
| // Proceed with sending the feedback email. | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
|
|
||
| // Update the last_feedback_sent_date to the current date after the email is sent. | ||
| DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId); | ||
| // Update the last_feedback_sent_date to the current date after the email is sent | ||
| DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId); | ||
| } |
There was a problem hiding this comment.
Improve error handling for email processing and date updates.
The email sending and date update operations should be wrapped in proper error handling to ensure data consistency.
Consider implementing the following changes:
if (empty($droppingCenterMeta[0])) {
+ try {
// Proceed with sending the feedback email.
DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from);
// Update the last_feedback_sent_date to the current date after the email is sent
DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId);
+ } catch (Exception $e) {
+ \CRM_Core_Error::debug_log_message("Failed to process feedback for center {$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.
| if (empty($droppingCenterMeta[0])) { | |
| // Proceed with sending the feedback email. | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent. | |
| DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId); | |
| // Update the last_feedback_sent_date to the current date after the email is sent | |
| DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId); | |
| } | |
| if (empty($droppingCenterMeta[0])) { | |
| try { | |
| // Proceed with sending the feedback email. | |
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | |
| // Update the last_feedback_sent_date to the current date after the email is sent | |
| DroppingCenterFeedbackService::updateFeedbackLastSentDate($droppingCenterId); | |
| } catch (Exception $e) { | |
| \CRM_Core_Error::debug_log_message("Failed to process feedback for center {$droppingCenterId}: " . $e->getMessage()); | |
| continue; | |
| } | |
| } |
| // Send email only if not delivered and not permanently closed | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
| if (empty($droppingCenterMeta[0])) { |
There was a problem hiding this comment.
| if (empty($droppingCenterMeta[0])) { | |
| if (!$droppingCenterMeta)) { |
is that not work, why we are fetching the 0th index and then check it's empty or not
There was a problem hiding this comment.
I only want to check the zeroth index. If this index is not present, it indicates that there are no entries for 'Permanently Closed' centers, which means we can proceed with triggering the biannual cron. However, if an entry is present, we do not want to run this cron.
There was a problem hiding this comment.
I think i have added a feedback on your previous PR where we need to change this to single so that we don't need this type of problem, please check that out and try to implement that on another PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1)
50-50: Consider documenting the complex field relationships
The query includes deeply nested field selections which could be difficult to maintain. Adding comments explaining the relationship between Dropping_Center_Meta, Collection_Camp_Core_Details, and their respective status fields would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
Line range hint 50-70: Implementation looks good overall
The changes effectively implement the requirement to prevent feedback emails for closed dropping centers through a combination of query filtering and status checks. The code is well-structured and includes proper error handling.
64-66: Verify the business logic alignment
The PR's objective is to prevent feedback emails for closed centers, but this condition checks for 'authorized' status. Please verify:
- Are there other valid statuses besides 'authorized' that should receive feedback?
- Is this condition redundant with the 'Permanently_Closed' check in the query?
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the status checks in the codebase:
'authorized' status check is consistent with codebase patterns
The 'authorized' status check in the code is correct and aligns with the codebase's business logic. Evidence:
- All other cron jobs consistently check for 'authorized' status:
- BiannualDroppingCenterFeedbackCron.php
- VolunteerFeedbackReminderCron.php
- CollectionCampOutcomeReminderCron.php
- CollectionCampCron.php
- VolunteerFeedbackCollectionCampCron.php
The 'Permanently_Closed' check in the query and the 'authorized' status check serve different purposes:
- The query filters dropping centers that are permanently closed
- The 'authorized' check verifies if the dropping center itself is in an authorized state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for status definitions and usage
ast-grep --pattern 'Status.Status:name'
rg -A 2 'Collection_Camp_Core_Details.Status'
Length of output: 20249
| } | ||
| catch (Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $meta['Dropping_Center_Meta.Dropping_Center']); | ||
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $droppingCenterId . ' - ' . $e->getMessage()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using structured logging
While the error logging has been improved, consider using a more structured format for better log parsing and monitoring:
- \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $droppingCenterId . ' - ' . $e->getMessage());
+ \CRM_Core_Error::debug_log_message(sprintf(
+ 'Dropping center feedback processing failed. Center ID: %d, Error: %s, Stack: %s',
+ $droppingCenterId,
+ $e->getMessage(),
+ $e->getTraceAsString()
+ ));📝 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.
| \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $droppingCenterId . ' - ' . $e->getMessage()); | |
| \CRM_Core_Error::debug_log_message(sprintf( | |
| 'Dropping center feedback processing failed. Center ID: %d, Error: %s, Stack: %s', | |
| $droppingCenterId, | |
| $e->getMessage(), | |
| $e->getTraceAsString() | |
| )); |
| $authorizedStatus = $meta['Dropping_Center_Meta.Dropping_Center.Collection_Camp_Core_Details.Status']; | ||
| // Trigger only if the status is 'authorized'. | ||
| if ($authorizedStatus === 'authorized') { | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract 'authorized' as a constant
The hardcoded string 'authorized' should be defined as a constant at the class/module level for better maintainability and reusability.
+ const STATUS_AUTHORIZED = 'authorized';
...
- if ($authorizedStatus === 'authorized') {
+ if ($authorizedStatus === self::STATUS_AUTHORIZED) {Committable suggestion was skipped due to low confidence.
| // Send email only if not delivered and not permanently closed | ||
| // Send the feedback email. | ||
| DroppingCenterFeedbackService::processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from); | ||
| if (empty($droppingCenterMeta[0])) { |
There was a problem hiding this comment.
I think i have added a feedback on your previous PR where we need to change this to single so that we don't need this type of problem, please check that out and try to implement that on another PR.
2 participants
Description
The biannual cron is responsible for sending feedback emails when a dropping center is active. Currently, it sends an email if the dropping center has been open for six months, even if the status is closed. To address this issue, the code has been updated to include a check that prevents emails from being sent to dropping centers that are closed.
Send the feedback cron email only if the status is authorized
Summary by CodeRabbit
New Features
Bug Fixes