Dropping Center Feedback Cron Fixes#397
Conversation
WalkthroughThis pull request modifies several files related to the handling of feedback email statuses in the Dropping Center functionality. Key changes include the simplification of field references from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 1
🧹 Outside diff range and nitpick comments (5)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
Line range hint
65-67: Enhance error logging with more contextThe current error logging only includes the Dropping Center ID. Consider including more context about the failure to aid in debugging.
- \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: ' . $meta['Dropping_Center_Meta.Dropping_Center'] . + ' - Error: ' . $e->getMessage() . + ' - Initiator ID: ' . $initiatorId);
Line range hint
65-67: Consider transaction handling for batch operationsThe foreach loop processes multiple records, but there's no transaction handling. If an error occurs mid-way, some records might be processed while others fail.
Consider wrapping the loop in a transaction to ensure atomic updates:
try { $transaction = new CRM_Core_Transaction(); foreach ($droppingCenterMeta as $meta) { // ... existing code ... } $transaction->commit(); } catch (Exception $e) { $transaction->rollback(); \CRM_Core_Error::debug_log_message(...); }wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1)
Line range hint
20-63: Consider refactoring for better separation of concernsThe function
civicrm_api3_goonjcustom_biannual_dropping_center_feedback_cronhandles multiple responsibilities which makes it harder to maintain and test. Consider:
- Extracting the dropping center query logic into a separate service
- Improving error handling granularity
- Adding logging for successful operations
Would you like me to provide a detailed refactoring proposal that separates these concerns into distinct services?
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (2)
Line range hint
26-43: Consider improving theprocessDroppingCenterStatusmethod.The method could benefit from some improvements:
- The status check logic seems counterintuitive - it returns early if status is true, which means the feedback email is only sent when status is false.
- Missing PHPDoc description for the
$statusparameter.- The method is handling multiple responsibilities (getting contact details, sending email, updating status).
Consider refactoring like this:
/** * Send feedback email if it hasn't been sent yet. * * @param array $meta * Meta data for the Dropping Center. * @param string $from * Email address for 'from'. + * @param bool $status + * Current feedback email delivery status. * * @throws \CRM_Core_Exception */ public static function processDroppingCenterStatus($droppingCenterId, $initiatorId, $status, $from) { + // Skip if feedback email was already sent + if ($status) { + return; + } + // Get recipient email and name. $contactDetails = self::getContactDetails($initiatorId); - 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', 1) ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) ->execute(); } }
Let me analyze the search results. I can see that
CRM_Utils_Mail::sendreturns a boolean value indicating success/failure, and several files in the codebase handle this properly:
CollectionCampOutcomeService.phpandCollectionCampVolunteerFeedbackService.phpcheck the result and log errorsEmail/Send.phpthrows an API exception on failureVolunteerFeedbackCollectionCampCron.phpchecks the result before updating statusActionSchedule.phpreturns an error message on failureThe current implementation in
DroppingCenterFeedbackService.phpdoesn't handle the failure case, which could lead to incorrect status updates.Email sending failures should be handled before updating the status
The status should only be updated when the email is successfully sent. Consider this pattern used elsewhere in the codebase:
- self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); - - // Update status if the email is sent. - EckEntity::update('Dropping_Center_Meta', TRUE) - ->addValue('Status.Feedback_Email_Delivered', 1) - ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) - ->execute(); + $emailSendResult = self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); + if (!$emailSendResult) { + \Civi::log()->error('Failed to send dropping center feedback email', [ + 'droppingCenterId' => $droppingCenterId, + 'contactEmailId' => $contactEmailId + ]); + return; + } + + // Update status only if email was sent successfully + EckEntity::update('Dropping_Center_Meta', TRUE) + ->addValue('Status.Feedback_Email_Delivered', 1) + ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) + ->execute();🔗 Analysis chain
Line range hint
26-43: Consider adding error handling for email sending.The method should handle potential failures in email sending to prevent updating the status when email delivery fails.
Let's check if there's any error handling in the email sending utility:
Consider wrapping the email sending in a try-catch block:
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) { 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', 1) - ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) - ->execute(); + try { + self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); + + // Update status only if email is sent successfully + EckEntity::update('Dropping_Center_Meta', TRUE) + ->addValue('Status.Feedback_Email_Delivered', 1) + ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) + ->execute(); + } catch (\Exception $e) { + \Civi::log()->error('Failed to send feedback email: ' . $e->getMessage()); + throw new \CRM_Core_Exception('Failed to send feedback email'); + } } }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in CRM_Utils_Mail::send usage rg -A 5 "CRM_Utils_Mail::send"Length of output: 17455
📜 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 (2 hunks)
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
50-50: LGTM: Field selection change looks good!The simplified field reference
Status.Feedback_Email_Deliveredmaintains the same functionality while improving code clarity.
61-61: LGTM: Field access change is consistent!The simplified field access aligns with the query changes above and maintains consistency across the codebase.
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/BiannualDroppingCenterFeedbackCron.php (1)
43-46: Verify API field access pattern changeThe change from
Status.Feedback_Email_Delivered:nametoStatus.Feedback_Email_Deliveredmight affect how the data is retrieved. Let's verify this pattern is used consistently across the codebase.✅ Verification successful
Field access pattern is consistent across the codebase
The change from
:namesuffix to direct field access forStatus.Feedback_Email_Deliveredis consistent with all other usages in the codebase. The search results show that:
- No instances of the old pattern
Status.Feedback_Email_Delivered:nameexist- The new pattern
Status.Feedback_Email_Deliveredis used consistently across three files:
- BiannualDroppingCenterFeedbackCron.php
- FeedbackDroppingCenterCron.php
- DroppingCenterFeedbackService.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old pattern rg "Status\.Feedback_Email_Delivered:name" # Search for the new pattern to ensure consistency rg "Status\.Feedback_Email_Delivered[^:]"Length of output: 976
wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (1)
39-39: LGTM: Field reference simplification looks good.The change from
Status.Feedback_Email_Delivered:nametoStatus.Feedback_Email_Deliveredaligns with the standardization across other files.
| ->execute(); | ||
|
|
||
| $status = $droppingCenterMeta['Status.Feedback_Email_Delivered:name']; | ||
| $status = $droppingCenterMeta[0]['Status.Feedback_Email_Delivered']; |
There was a problem hiding this comment.
Add defensive programming checks
The direct array access $droppingCenterMeta[0] assumes the query always returns results. This could lead to undefined index errors.
Consider adding null checks:
- $status = $droppingCenterMeta[0]['Status.Feedback_Email_Delivered'];
+ $status = !empty($droppingCenterMeta) ? $droppingCenterMeta[0]['Status.Feedback_Email_Delivered'] : null;
+ if ($status === null) {
+ \CRM_Core_Error::debug_log_message('No meta data found for dropping center ID: ' . $droppingCenterId);
+ 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.
| $status = $droppingCenterMeta[0]['Status.Feedback_Email_Delivered']; | |
| $status = !empty($droppingCenterMeta) ? $droppingCenterMeta[0]['Status.Feedback_Email_Delivered'] : null; | |
| if ($status === null) { | |
| \CRM_Core_Error::debug_log_message('No meta data found for dropping center ID: ' . $droppingCenterId); | |
| continue; | |
| } |
| ->execute(); | ||
|
|
||
| $status = $droppingCenterMeta['Status.Feedback_Email_Delivered:name']; | ||
| $status = $droppingCenterMeta[0]['Status.Feedback_Email_Delivered']; |
There was a problem hiding this comment.
Consider the null check here
| ->addWhere('Dropping_Center_Meta.Dropping_Center', '=', $droppingCenterId) | ||
| ->addWhere('Status.Status:name', '=', 'Parmanently_Closed') | ||
| ->execute()->single(); | ||
| ->addWhere('Status.Status:name', '=', 'Permanently_Closed') |
There was a problem hiding this comment.
Please refactor this:
Could we add a check here to filter out Status.Feedback_Email_Delivered? By doing this, the API will be optimized to return only the dropping centers that haven't yet received emails, making it more efficient.
| ->execute(); | ||
|
|
||
| $status = $droppingCenterMeta['Status.Feedback_Email_Delivered:name']; | ||
| $status = !empty($droppingCenterMeta) ? $droppingCenterMeta[0]['Status.Feedback_Email_Delivered'] : null; |
There was a problem hiding this comment.
If this filtering is handled directly in the API, we can remove the corresponding line here as well as from the service file.
|
Please resolve conflicts before merging the PR |
2 participants
Code Cleanups
Summary by CodeRabbit
New Features
Bug Fixes
Refactor