Add error logging for feedback email sending issue#419
Add error logging for feedback email sending issue#419belwalshubham merged 2 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance logging capabilities within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Add one simple log below the update query |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (2)
68-74: Consider using debug level for detailed loggingWhile the detailed logging is valuable for troubleshooting, consider using
debuglevel for per-record details in production:- Civi::log()->info('Dropping Center Meta details', [ + Civi::log()->debug('Dropping Center Meta details', [
Line range hint
82-83: Enhance error handling granularity and structureThe current error handling could be improved in several ways:
- Move try-catch inside the loop to handle each dropping center independently
- Use structured logging for errors
- catch (Exception $e) { - \CRM_Core_Error::debug_log_message('Error processing Dropping Center ID: ' . $droppingCenterId . ' - ' . $e->getMessage()); + catch (Exception $e) { + Civi::log()->error('Failed to process dropping center feedback', [ + 'dropping_center_id' => $droppingCenterId, + 'initiator_id' => $initiatorId, + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString(), + ]);wp-content/civi-extensions/goonjcustom/Civi/DroppingCenterFeedbackService.php (2)
Line range hint
36-45: Add error handling around email operations.The email sending and status update operations lack error handling, which could lead to silent failures in production. This makes it difficult to track and debug issues when they occur.
Consider wrapping these operations in try-catch blocks with proper error logging:
+ try { self::sendFeedbackEmail($organizingContactName, $droppingCenterId, $contactEmailId, $from); + } catch (\Exception $e) { + Civi::log()->error('Failed to send feedback email', [ + 'dropping_center_id' => $droppingCenterId, + 'initiator_id' => $initiatorId, + 'error' => $e->getMessage(), + 'trace' => $e->getTraceAsString() + ]); + throw $e; + } + try { // 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(); + Civi::log()->info('Successfully updated feedback email status', [ + 'dropping_center_id' => $droppingCenterId + ]); + } catch (\Exception $e) { + Civi::log()->error('Failed to update feedback email status', [ + 'dropping_center_id' => $droppingCenterId, + 'error' => $e->getMessage() + ]); + throw $e; + }
Line range hint
89-93: Add logging in sendFeedbackEmail method.The email sending method should include logging to track successful email delivery.
Consider adding a success log after email sending:
// Send the email. \CRM_Utils_Mail::send($mailParams); + Civi::log()->info('Successfully sent feedback email', [ + 'recipient' => $contactEmailId, + 'dropping_center_id' => $droppingCenterId + ]);
📜 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/FeedbackDroppingCenterCron.php(2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/FeedbackDroppingCenterCron.php (1)
49-54: Verify the impact of disabling caching in EckEntity::get
The change from TRUE to FALSE in EckEntity::get() disables caching. While this ensures fresh data, it might impact performance if this cron runs frequently with large datasets.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Disabling caching in EckEntity::get is consistent with codebase patterns
Looking at the codebase usage patterns of EckEntity::get(), the caching parameter (TRUE/FALSE) is consistently chosen based on the use case:
- Caching is disabled (FALSE) when retrieving data that needs to be fresh, especially in user-facing operations and data mutations
- Caching is enabled (TRUE) for background operations like cron jobs that process bulk data
In this case, disabling caching for feedback email delivery status is appropriate since:
- The feedback status needs to be accurate and fresh to avoid duplicate emails
- The query is targeted with specific conditions, so performance impact is minimal
- Other similar feedback-related cron jobs in the codebase also use caching strategically
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of EckEntity::get to understand caching patterns
rg -A 2 "EckEntity::get\\(" --type php
# Look for cron schedule configuration
fd -e php -e json -H cron
Length of output: 19594
| Civi::log()->info('Dropping Center Meta data retrieved', [ | ||
| 'status_name' => $statusName, | ||
| 'data' => $droppingCenterMeta, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider logging volume and data sensitivity
The current logging statement dumps the entire $droppingCenterMeta dataset, which could:
- Generate excessive log volume in production
- Potentially expose sensitive contact information
Consider logging only essential metadata:
- 'data' => $droppingCenterMeta,
+ 'count' => count($droppingCenterMeta),
+ 'dropping_center_ids' => array_column($droppingCenterMeta, 'Dropping_Center_Meta.Dropping_Center'),📝 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.
| Civi::log()->info('Dropping Center Meta data retrieved', [ | |
| 'status_name' => $statusName, | |
| 'data' => $droppingCenterMeta, | |
| ]); | |
| Civi::log()->info('Dropping Center Meta data retrieved', [ | |
| 'status_name' => $statusName, | |
| 'count' => count($droppingCenterMeta), | |
| 'dropping_center_ids' => array_column($droppingCenterMeta, 'Dropping_Center_Meta.Dropping_Center'), | |
| ]); |
| Civi::log()->info('trigger email'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logging with contextual information.
The current logging statement is too basic and lacks important context that would be valuable for debugging production issues.
Consider enhancing the log message with relevant metadata:
- Civi::log()->info('trigger email');
+ Civi::log()->info('Triggering feedback email', [
+ 'dropping_center_id' => $droppingCenterId,
+ 'initiator_id' => $initiatorId,
+ 'recipient_email' => $contactEmailId,
+ 'recipient_name' => $organizingContactName
+ ]);Committable suggestion skipped: line range outside the PR's diff.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
These updates improve traceability and debugging capabilities for users interacting with the Dropping Center functionalities.