Reminder for outcome 2 emails will be sent#392
Conversation
WalkthroughThe pull request introduces significant changes to 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 (2)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1)
50-50: Consider improving query readability and documentation.While the addition of
Camp_Outcome.Final_Reminder_Sentaligns with the new two-reminder system, consider these improvements:
- Group related fields together for better readability
- Add comments explaining the reminder tracking fields
Here's a suggested refactor:
->addSelect( 'title', // Camp details 'Collection_Camp_Intent_Details.Location_Area_of_camp', 'Collection_Camp_Intent_Details.End_Date', 'Logistics_Coordination.Camp_to_be_attended_by', // Reminder tracking fields - 'Camp_Outcome.Last_Reminder_Sent', - 'Camp_Outcome.Final_Reminder_Sent', + 'Camp_Outcome.Last_Reminder_Sent', // Tracks the first reminder (sent 48h after event) + 'Camp_Outcome.Final_Reminder_Sent' // Tracks the second/final reminder (sent 48h after first) )wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1)
Line range hint
16-16: Add type hint for the $from parameter.The method signature should include type hints for all parameters to ensure type safety and improve code maintainability.
- public static function processCampReminder($camp, $now, $from) { + public static function processCampReminder(array $camp, \DateTimeImmutable $now, string $from) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (2 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1)
Line range hint
50-71: Verify the integration with CollectionCampOutcomeService.The query now fetches the
Final_Reminder_Sentfield which is used byCollectionCampOutcomeService::processCampReminder(). Let's verify this integration.✅ Verification successful
Let me generate additional scripts to verify the complete implementation of
processCampRemindermethod since the previous results don't show its full implementation.The previous results show that
Final_Reminder_Sentis being used inCollectionCampOutcomeService, but we need to see the complete implementation ofprocessCampReminderto verify how it's being handled in the reminder logic.
Let me try one more search with a different pattern to locate the complete implementation.
The previous results showed that
Final_Reminder_Sentis being used in the service class, but we need to see the complete implementation to verify the reminder logic and timing calculations.
Integration with CollectionCampOutcomeService is properly implemented
The verification shows that the
Final_Reminder_Sentfield is correctly integrated in the reminder logic:
- The field is fetched in the query and passed to
processCampReminder- The service properly handles both
Last_Reminder_SentandFinal_Reminder_Sentfields- The reminder logic follows a clear sequence:
- First reminder after 48 hours of camp end
- Second (final) reminder after another 48 hours
- No more reminders after
Final_Reminder_Sentis setThe implementation maintains good separation of concerns with dedicated methods for email sending and HTML generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Final_Reminder_Sent field in processCampReminder method # Test: Search for the method implementation to confirm it handles both reminder fields ast-grep --pattern 'function processCampReminder($camp, $now, $from) { $$$ }' # Test: Search for any direct usage of the Final_Reminder_Sent field rg -A 5 'Final_Reminder_Sent'Length of output: 11688
Script:
#!/bin/bash # Search for the complete implementation of processCampReminder method fd -e php -x cat {} \; | rg -A 30 "function processCampReminder\(\$camp, \$now, \$from\)" # Search for any other methods that might be using Final_Reminder_Sent fd -e php -x cat {} \; | rg -B 5 -A 5 "Final_Reminder_Sent"Length of output: 154
Script:
#!/bin/bash # Search for the complete implementation in the CollectionCampOutcomeService file cat wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php # Alternative search for the method rg -U "public static function processCampReminder.*?\{.*?\}" -A 50Length of output: 4658
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (3)
43-46: LGTM! Early return prevents additional reminders.This check effectively implements the requirement that no additional emails should be sent after the second reminder.
47-56: Improve comment clarity and verify timing logic.The comment could be more specific about the 48-hour requirement.
- // If no reminders have been sent yet, send the first reminder. + // Send first reminder if no previous reminders and 48 hours have passed since camp end.Let's verify that this is the only place where Last_Reminder_Sent is updated:
✅ Verification successful
Let me try a different search strategy to verify the Last_Reminder_Sent updates.
This should help us find any occurrences of Last_Reminder_Sent being updated, regardless of the exact syntax or casing used.
Let me also check for the entity definition to understand the field better:
Based on the search results, I can now verify the Last_Reminder_Sent field usage. The code shows that:
The Last_Reminder_Sent field is only updated in two specific scenarios:
- In CollectionCampOutcomeService.php for outcome reminders
- In CollectionCampVolunteerFeedbackService.php for volunteer feedback reminders
The timing logic is correctly implemented:
- First checks if 48 hours have passed since camp end (line 38)
- Then checks if no reminders have been sent (Last_Reminder_Sent is NULL)
- Updates Last_Reminder_Sent timestamp when sending the first reminder
The comment could indeed be more specific about the timing requirement, but it's not technically incorrect since it accurately describes the immediate condition being checked.
The comment is accurate but could be more informative. The current implementation correctly handles the reminder timing requirements.
The suggested comment improvement would add helpful context, but the existing comment is not incorrect - it accurately describes the specific condition being checked. The 48-hour requirement is already enforced by the previous check at line 38.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other updates to Last_Reminder_Sent to ensure consistent reminder tracking rg -l "Last_Reminder_Sent" | xargs rg "Last_Reminder_Sent.*="Length of output: 61
Script:
#!/bin/bash # Search for Last_Reminder_Sent pattern in different variations rg -i "last.?reminder.?sent" -A 3 -B 3 # Also search using ast-grep to find any assignments ast-grep --pattern '$_->addValue("Last_Reminder_Sent", $_)'Length of output: 18398
Script:
#!/bin/bash # Look for Collection_Camp entity definition rg -i "collection.?camp" -A 5 -B 5Length of output: 279258
Line range hint
16-67: Verify the complete reminder flow timing.The implementation aims to:
- Send first reminder 48 hours after camp end
- Send second reminder 48 hours after first reminder
- Never send more reminders
Let's verify this flow by checking the database schema and any related cron jobs:
✅ Verification successful
Let me gather more information about the cron job implementation to verify the complete reminder flow.
The first command will help us understand how the cron job is scheduled and triggered. The ast-grep patterns will help us verify if there are any other places in the code that might modify these reminder fields, which could potentially interfere with the intended flow.
Let me try one more search to find the cron implementation:
This should help us see the complete cron implementation and any API routes that might affect the reminder flow.
Reminder flow implementation is correct and well-structured
After analyzing the complete implementation, the reminder flow is correctly implemented with proper timing and controls:
- The cron job correctly filters for completed camps with unfilled outcome forms
- The service implementation ensures:
- First reminder after 48 hours of camp end
- Second reminder after another 48 hours
- No more reminders after Final_Reminder_Sent is set
- The flow has proper error handling and logging
The implementation follows single responsibility principle with:
- Cron job handling scheduling and camp filtering
- Service class handling reminder business logic
- Clear separation of database operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check database schema for reminder-related fields rg -l "CREATE TABLE.*Collection_Camp" | xargs rg "Last_Reminder_Sent|Final_Reminder_Sent" # Check cron job scheduling rg -l "collection_camp_outcome_reminder_cron" | xargs rg "job.*frequency"Length of output: 2044
Script:
#!/bin/bash # Check the cron job implementation rg -A 10 "class.*CollectionCampOutcomeReminderCron" # Check for any API endpoints or services that might modify these reminder fields ast-grep --pattern 'function $FUNC($$$) { $$$ ->addValue("Camp_Outcome.Last_Reminder_Sent", $$$) $$$ }' ast-grep --pattern 'function $FUNC($$$) { $$$ ->addValue("Camp_Outcome.Final_Reminder_Sent", $$$) $$$ }'Length of output: 18822
Script:
#!/bin/bash # Search for the cron implementation file directly rg -l "CollectionCampOutcomeReminderCron" | xargs cat # Also check for any API routes that might trigger this rg -A 5 "api.*collection.*camp.*outcome.*reminder"Length of output: 4232
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (4)
Line range hint
16-21: Update method signature documentation.The method signature has been updated to include the
$fromparameter, but the PHPDoc block hasn't been updated accordingly. Consider adding type hints and parameter documentation:/** * Process camp reminder logic. * * @param array $camp * @param \DateTimeImmutable $now + * @param string $from Email address to send reminders from * * @throws \Exception */ - public static function processCampReminder($camp, $now, $from) { + public static function processCampReminder(array $camp, \DateTimeImmutable $now, string $from): bool {
42-51: Document the business logic for reminder flow.While the implementation is correct, adding a comment block explaining the reminder flow would improve maintainability:
+ // Business Logic: + // 1. First reminder is sent 48 hours after camp end + // 2. Second reminder is sent 48 hours after first reminder + // 3. No more reminders are sent after the second one + // If no reminders have been sent yet, send the first reminder. if ($lastReminderSent === NULL) {
Line range hint
69-70: Add reminder type to error logs.Enhance error logging by including which reminder (first/final) failed to send:
\Civi::log()->error('Failed to send reminder email', [ 'campAttendedById' => $campAttendedById, 'attendeeEmail' => $attendeeEmail, + 'reminderType' => $camp['Camp_Outcome.Final_Reminder_Sent'] === NULL ? 'first' : 'final', ]);
Based on the investigation of CiviCRM's codebase, I can now provide a final response:
Add email validation using CiviCRM's standard validation approach
The
$fromparameter should be validated using CiviCRM's built-in validation to maintain consistency with the framework. Here's the recommended approach:public static function processCampReminder($camp, $now, $from) { + if (!CRM_Utils_Rule::email($from)) { + throw new CRM_Core_Exception('Invalid from email address'); + }
- CiviCRM provides
CRM_Utils_Rule::email()as the standard way to validate email addresses across the framework- This maintains consistency with other CiviCRM components and ensures uniform email validation behavior
- The error message follows CiviCRM's exception handling patterns
🔗 Analysis chain
Line range hint
16-21: Validate email format for $from parameter.The new
$fromparameter should be validated to ensure it's a valid email address format.Let's check if email validation is implemented elsewhere:
Add validation at the start of the method:
public static function processCampReminder($camp, $now, $from) { + if (!filter_var($from, FILTER_VALIDATE_EMAIL)) { + throw new \CRM_Core_Exception('Invalid from email address format'); + }
Let me check the CiviCRM email validation implementations to see if there's a standard way to validate emails in this codebase.
Let me check if there's a standard CiviCRM utility class for email validation.
Let me check one more thing - the CiviCRM validation rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for email validation patterns rg -l "email.*valid" --type phpLength of output: 3593
Script:
#!/bin/bash # Check CiviCRM's email validation implementations rg -A 5 "email.*valid" wp-content/plugins/civicrm/civicrm/CRM/Utils/Length of output: 2089
Script:
#!/bin/bash # Search for email validation utility classes in CiviCRM rg -A 5 "class.*Mail|validateEmail" wp-content/plugins/civicrm/civicrm/CRM/Utils/Length of output: 5595
Script:
#!/bin/bash # Check for email validation rules in CiviCRM rg -A 10 "validate.*email" wp-content/plugins/civicrm/civicrm/CRM/Core/BAO/Length of output: 77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampOutcomeService.php (1 hunks)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/CollectionCampOutcomeReminderCron.php
🧰 Additional context used
| // Return if 48 hours have not passed since the last reminder was sent. | ||
| if ($lastReminderSent !== NULL && $hoursSinceLastReminder < 48) { | ||
| return FALSE; | ||
| } | ||
| self::sendOutcomeReminderEmail($campAttendedById, $from, $campCode, $campAddress, $collectionCampId, $endDateString); | ||
|
|
||
| EckEntity::update('Collection_Camp', TRUE) | ||
| ->addWhere('id', '=', $camp['id']) | ||
| ->addValue('Camp_Outcome.Last_Reminder_Sent', $now->format('Y-m-d H:i:s')) | ||
| ->addValue('Camp_Outcome.Final_Reminder_Sent', $now->format('Y-m-d H:i:s')) | ||
| ->execute(); |
There was a problem hiding this comment.
Prevent possibility of third reminder.
The current implementation doesn't explicitly check if a final reminder has already been sent. This could potentially allow a third reminder to be sent.
Add a check for Final_Reminder_Sent:
// Return if 48 hours have not passed since the last reminder was sent.
if ($lastReminderSent !== NULL && $hoursSinceLastReminder < 48) {
return FALSE;
}
+
+ // Don't send more than two reminders
+ if ($camp['Camp_Outcome.Final_Reminder_Sent'] !== NULL) {
+ return FALSE;
+ }📝 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.
| // Return if 48 hours have not passed since the last reminder was sent. | |
| if ($lastReminderSent !== NULL && $hoursSinceLastReminder < 48) { | |
| return FALSE; | |
| } | |
| self::sendOutcomeReminderEmail($campAttendedById, $from, $campCode, $campAddress, $collectionCampId, $endDateString); | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $camp['id']) | |
| ->addValue('Camp_Outcome.Last_Reminder_Sent', $now->format('Y-m-d H:i:s')) | |
| ->addValue('Camp_Outcome.Final_Reminder_Sent', $now->format('Y-m-d H:i:s')) | |
| ->execute(); | |
| // Return if 48 hours have not passed since the last reminder was sent. | |
| if ($lastReminderSent !== NULL && $hoursSinceLastReminder < 48) { | |
| return FALSE; | |
| } | |
| // Don't send more than two reminders | |
| if ($camp['Camp_Outcome.Final_Reminder_Sent'] !== NULL) { | |
| return FALSE; | |
| } | |
| self::sendOutcomeReminderEmail($campAttendedById, $from, $campCode, $campAddress, $collectionCampId, $endDateString); | |
| EckEntity::update('Collection_Camp', TRUE) | |
| ->addWhere('id', '=', $camp['id']) | |
| ->addValue('Camp_Outcome.Final_Reminder_Sent', $now->format('Y-m-d H:i:s')) | |
| ->execute(); |
2 participants
There were some changes to the requirement where we need 2 emails to be sent, once after 48 hours and if that is not filled then 48 hours after that. We don't want the emails to go after that.
Target issue: https://github.com/coloredcow-admin/goonj-crm/issues/149#issuecomment-2431109084
Summary by CodeRabbit
New Features
Bug Fixes