Fix/events participants feedback#702
Conversation
WalkthroughThis pull request focuses on modifying the Changes
Sequence DiagramsequenceDiagram
participant Cron as Feedback Cron
participant Service as Events Service
participant Email as Email System
Cron->>Cron: Retrieve Eligible Events
Cron->>Service: sendGoonjInitiatedFeedbackEmail(eventsArray)
Service->>Service: Iterate Through Events
Service->>Email: Send Feedback Emails
Service->>Cron: Return Processing Result
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (3)
31-83: Method is lengthy but logically fine.
Good job wrapping the email-sending logic under a try/catch. However, it might be beneficial to split out the data retrieval and mail-sending into smaller, single-responsibility methods for better readability and reusability.
Line range hint
91-106: Grammar fix needed: "Their is one forms" --> "There is one form."
Current text has a minor grammatical issue and mentions "forms" in singular. Here's a proposed fix:-<p>Thank you for attending the goonj events <strong>$eventCode</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention... +<p>Thank you for attending the goonj events <strong>$eventCode</strong> at <strong>$eventAddress</strong>. There is one form that requires your attention...
Line range hint
349-364: Minor grammar correction: "Their is one forms" --> "There is one form."
As with the other email template, let's address the grammar:-<p>Thank you for attending the goonj events <strong>$eventCode</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention... +<p>Thank you for attending the goonj events <strong>$eventCode</strong> at <strong>$eventAddress</strong>. There is one form that requires your attention...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(4 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsFeedbackCron.php(1 hunks)
🔇 Additional comments (6)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (4)
21-24: All set with these event hooks!
They succinctly declare event-based logic, and the code is clean with no duplication or complexity.
122-170: QR code logic looks solid!
No immediate concerns spotted. Good handling of potential missing data and robust logging in case of exceptions.
178-273: Tab configuration is nicely organized.
Clear separation of responsibilities and good usage of permissions checks.
279-342: Multiple events email sending logic
Great approach to handle multiple events in one go! Quick heads-up: if your PR eventually aims to notify multiple participants per event, you might need to adapt logic to loop participants, not just the 'created_id'. For now, it works as intended for a single participant per event.
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsFeedbackCron.php (2)
41-44: Event selection limit check
You're currently capping events at 25. If you anticipate more events daily, consider raising the limit or paginating. Otherwise, some events might miss out on feedback if the list is truncated.
55-66:
Premature return in the loop
The return civicrm_api3_create_success immediately ends the function on the first iteration, so subsequent events in the loop won't be processed. Please move this return statement outside the loop to handle all targeted events properly.
}
- return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'goonj_initiated_events_feedback_cron');
}
}
+ return civicrm_api3_create_success($returnValues, $params, 'Goonjcustom', 'goonj_initiated_events_feedback_cron');Likely invalid or redundant comment.
| $eventsDetails = Event::get(TRUE) | ||
| ->addSelect('participant.status_id:name', 'participant.created_id', 'title', 'loc_block_id.address_id', 'Goonj_Events_Feedback.Last_Reminder_Sent', 'end_date') | ||
| ->addJoin('Participant AS participant', 'LEFT') | ||
| ->addWhere('participant.status_id', '=', 2) | ||
| ->addWhere('id', '=', 102) | ||
| ->setLimit(25) | ||
| ->execute(); |
There was a problem hiding this comment.
Hard-coded event ID "102"
This line effectively restricts processing to one specific event. That’s an instant showstopper if your goal is to send feedback emails for multiple events. Remove or replace this with $event['id'] or equivalent.
->addWhere('participant.status_id', '=', 2)
-->addWhere('id', '=', 102)
+->addWhere('id', '=', $event['id'])Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
Line range hint
352-363: Fix grammatical errors and improve email template structure.The email template contains several issues:
- Grammatical errors ("Their is one forms")
- Malformed HTML list (stray
</li>tag)- Redundant text ("event's event")
Apply these improvements:
$html = " <p>Dear $attendeeName,</p> -<p>Thank you for attending the goonj events <strong>$eventCode</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention during and after the goonj event:</p> +<p>Thank you for attending the Goonj event <strong>$eventCode</strong> at <strong>$eventAddress</strong>. Please complete the following feedback form:</p> <ol> - Please complete this form from the goonj event location once the goonj event ends.</li> - <li><a href=\"$eventFeedBackFormUrl\">Goonj Events Feedback Form</a><br> - This feedback form should be filled out after the goonj event ends, once you have an overview of the event's event.</li> + <li><a href=\"$eventFeedBackFormUrl\">Goonj Events Feedback Form</a><br> + This feedback form should be filled out after the event ends, once you have an overview of the outcomes.</li> </ol> <p>We appreciate your cooperation.</p> <p>Warm Regards,<br>Urban Relations Team</p>";
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (2)
122-170: LGTM! Well-structured QR code generation with proper error handling.The implementation follows best practices with:
- Early returns for invalid conditions
- Comprehensive error logging
- Clear separation of concerns
Consider enhancing the error message on line 129 to include both the object ID and the operation type:
- \Civi::log()->warning('Event ID is missing from object reference.' . $objectId); + \Civi::log()->warning("Event ID is missing from object reference. Operation: $op, ObjectId: $objectId");
279-341: Consider adding batch processing limits for better resource management.While the implementation correctly handles multiple events, processing an unlimited number of events in a single batch could lead to memory issues or timeouts.
Consider implementing batch size limits:
+ private const BATCH_SIZE = 100; public static function sendGoonjInitiatedFeedbackEmail($eventsArray) { $updatedEventIds = []; + $eventBatches = array_chunk($eventsArray, self::BATCH_SIZE); + foreach ($eventBatches as $eventBatch) { - foreach ($eventsArray as $event) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(4 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (2)
21-24: LGTM! Hook registrations are properly defined.
The event subscriptions are correctly configured for QR code generation and tabset management.
212-220: LGTM! Tab configuration is properly structured.
The feedback tab configuration maintains proper separation of concerns and correct permission settings.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor