Conversation
|
Warning Rate limit exceeded@tarunnjoshi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces two new PHP files in the CiviCRM extension: Changes
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: 7
🧹 Nitpick comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php (1)
3-21: Enhance API documentation for better maintainabilityThe documentation should provide more context about:
- The purpose and frequency of this cron job
- Expected behavior and side effects
- Potential exceptions that might be thrown
/** * Goonjcustom.UrbanPlannedVisitOutcomeCron API specification (optional) * This is used for documentation and validation. + * + * This cron job processes urban planned visits that have occurred up to the current day + * and sends outcome emails to the coordinating POCs. * * @param array $spec * description of fields supported by this API call. * + * @throws \Exception When email sending fails * * @return void */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php(1 hunks)
| ->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay) | ||
| ->execute(); | ||
|
|
||
| error_log("institutionVisit: " . print_r($institutionVisit, TRUE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace error_log with CiviCRM logging
Using error_log for debugging in production code is not recommended. Use CiviCRM's logging system consistently.
- error_log("institutionVisit: " . print_r($institutionVisit, TRUE));
+ \Civi::log()->debug('Processing institution visits', [
+ 'visits' => $institutionVisit,
+ ]);📝 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.
| error_log("institutionVisit: " . print_r($institutionVisit, TRUE)); | |
| \Civi::log()->debug('Processing institution visits', [ | |
| 'visits' => $institutionVisit, | |
| ]); |
| foreach ($institutionVisit as $visit) { | ||
| try { | ||
| UrbanPlannedVisitService::sendOutcomeEmail($visit); | ||
| } | ||
| catch (\Exception $e) { | ||
| \Civi::log()->info('Error processing visit', [ | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add visit processing status tracking
The cron job should track which visits have been processed to prevent duplicate emails.
Consider adding a status field to track processed visits:
foreach ($institutionVisit as $visit) {
try {
+ // Check if already processed
+ if ($visit['Urban_Planned_Visit.outcome_email_sent'] ?? false) {
+ continue;
+ }
UrbanPlannedVisitService::sendOutcomeEmail($visit);
+ // Mark as processed
+ EckEntity::update('Institution_Visit', FALSE)
+ ->addValue('Urban_Planned_Visit.outcome_email_sent', true)
+ ->addWhere('id', '=', $visit['id'])
+ ->execute();
}
catch (\Exception $e) {
\Civi::log()->info('Error processing visit', [
'error' => $e->getMessage(),
+ 'visit_id' => $visit['id'],
]);
}
}Committable suggestion skipped: line range outside the PR's diff.
| $institutionVisit = EckEntity::get('Institution_Visit', FALSE) | ||
| ->addSelect('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', 'Urban_Planned_Visit.Coordinating_Goonj_POC') | ||
| ->addWhere('Urban_Planned_Visit.Coordinating_Goonj_POC', 'IS NOT NULL') | ||
| ->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay) | ||
| ->execute(); | ||
|
|
There was a problem hiding this comment.
Add pagination to prevent timeout on large datasets
The query fetches all matching visits without any limit, which could cause performance issues or timeouts if there are many visits.
$institutionVisit = EckEntity::get('Institution_Visit', FALSE)
->addSelect('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', 'Urban_Planned_Visit.Coordinating_Goonj_POC')
->addWhere('Urban_Planned_Visit.Coordinating_Goonj_POC', 'IS NOT NULL')
->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay)
+ ->setLimit(50) // Process in batches
->execute();📝 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.
| $institutionVisit = EckEntity::get('Institution_Visit', FALSE) | |
| ->addSelect('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', 'Urban_Planned_Visit.Coordinating_Goonj_POC') | |
| ->addWhere('Urban_Planned_Visit.Coordinating_Goonj_POC', 'IS NOT NULL') | |
| ->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay) | |
| ->execute(); | |
| $institutionVisit = EckEntity::get('Institution_Visit', FALSE) | |
| ->addSelect('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', 'Urban_Planned_Visit.Coordinating_Goonj_POC') | |
| ->addWhere('Urban_Planned_Visit.Coordinating_Goonj_POC', 'IS NOT NULL') | |
| ->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay) | |
| ->setLimit(50) // Process in batches | |
| ->execute(); |
| private static function getOutcomeEmailHtml($coordinatingGoonjPOCName) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $visitOutcomeFormUrl = $homeUrl . '/visit-outcome-form/'; | ||
|
|
||
| $html = " | ||
| <p>Dear $coordinatingGoonjPOCName,</p> | ||
| <p>Thank you for attending the visit. Please fills out the below form:</p> | ||
| <ol> | ||
| <li><a href=\"$visitOutcomeFormUrl\">Camp Outcome Form</a><br> | ||
| This feedback form should be filled out after the camp/drive ends, once you have an overview of the event's outcomes.</li> | ||
| </ol> | ||
| <p>We appreciate your cooperation.</p> | ||
| <p>Warm Regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CiviCRM's message template system
Instead of hardcoding the email HTML, use CiviCRM's message template system for better maintainability and consistency.
Consider:
- Creating a message template in CiviCRM
- Using token replacement for dynamic content
- Supporting multilingual content
Example implementation:
private static function getOutcomeEmailHtml($coordinatingGoonjPOCName) {
$template = \CRM_Core_Smarty::singleton();
$template->assign('coordinatingGoonjPOCName', $coordinatingGoonjPOCName);
$template->assign('visitOutcomeFormUrl',
\CRM_Utils_System::baseCMSURL() . '/visit-outcome-form/');
return $template->fetch('string:' . \CRM_Core_DAO::getFieldValue(
'CRM_Core_DAO_MessageTemplate',
'urban_planned_visit_outcome',
'msg_html',
'msg_title'
));
}| error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE)); | ||
| error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace error_log with CiviCRM logging
Similar to the cron file, replace error_log with CiviCRM's logging system.
- error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE));
- error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE));
+ \Civi::log()->debug('Sending outcome email', [
+ 'poc_email' => $coordinatingGoonjPOCEmail,
+ 'poc_name' => $coordinatingGoonjPOCName,
+ ]);📝 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.
| error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE)); | |
| error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE)); | |
| \Civi::log()->debug('Sending outcome email', [ | |
| 'poc_email' => $coordinatingGoonjPOCEmail, | |
| 'poc_name' => $coordinatingGoonjPOCName, | |
| ]); | |
| class UrbanPlannedVisitService extends AutoSubscriber { | ||
| private static $individualId = NULL; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review class architecture and remove unused code
The class extends AutoSubscriber but doesn't seem to use its features. Also, there's an unused private static property.
- Consider if extending AutoSubscriber is necessary
- Remove the unused $individualId property unless it's planned for future use
-class UrbanPlannedVisitService extends AutoSubscriber {
- private static $individualId = NULL;
+class UrbanPlannedVisitService {Committable suggestion skipped: line range outside the PR's diff.
| public static function sendOutcomeEmail($visit) { | ||
| $coordinatingGoonjPOCId = $visit['Urban_Planned_Visit.Coordinating_Goonj_POC']; | ||
|
|
||
| $coordinatingGoonjPOC = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $coordinatingGoonjPOCId) | ||
| ->execute()->single(); | ||
|
|
||
| $coordinatingGoonjPOCEmail = $coordinatingGoonjPOC['email.email']; | ||
| $coordinatingGoonjPOCName = $coordinatingGoonjPOC['display_name']; | ||
| error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE)); | ||
| error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE)); | ||
|
|
||
| if (!$coordinatingGoonjPOCEmail) { | ||
| throw new \Exception('POC email missing'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and data validation
The current error handling could be more robust with better validation and error messages.
public static function sendOutcomeEmail($visit) {
+ // Validate visit data structure
+ if (!isset($visit['Urban_Planned_Visit.Coordinating_Goonj_POC'])) {
+ throw new \Exception('Invalid visit data: missing Coordinating_Goonj_POC');
+ }
+
$coordinatingGoonjPOCId = $visit['Urban_Planned_Visit.Coordinating_Goonj_POC'];
+ if (!$coordinatingGoonjPOCId) {
+ throw new \Exception('Invalid visit data: Coordinating_Goonj_POC is empty');
+ }
$coordinatingGoonjPOC = Contact::get(FALSE)
->addSelect('email.email', 'display_name')
->addJoin('Email AS email', 'LEFT')
->addWhere('id', '=', $coordinatingGoonjPOCId)
->execute()->single();
+ if (!$coordinatingGoonjPOC) {
+ throw new \Exception(
+ sprintf('Contact not found for POC ID: %d', $coordinatingGoonjPOCId)
+ );
+ }
$coordinatingGoonjPOCEmail = $coordinatingGoonjPOC['email.email'];
$coordinatingGoonjPOCName = $coordinatingGoonjPOC['display_name'];
if (!$coordinatingGoonjPOCEmail) {
- throw new \Exception('POC email missing');
+ throw new \Exception(
+ sprintf('Email missing for POC: %s (ID: %d)',
+ $coordinatingGoonjPOCName,
+ $coordinatingGoonjPOCId
+ )
+ );
}📝 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.
| public static function sendOutcomeEmail($visit) { | |
| $coordinatingGoonjPOCId = $visit['Urban_Planned_Visit.Coordinating_Goonj_POC']; | |
| $coordinatingGoonjPOC = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $coordinatingGoonjPOCId) | |
| ->execute()->single(); | |
| $coordinatingGoonjPOCEmail = $coordinatingGoonjPOC['email.email']; | |
| $coordinatingGoonjPOCName = $coordinatingGoonjPOC['display_name']; | |
| error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE)); | |
| error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE)); | |
| if (!$coordinatingGoonjPOCEmail) { | |
| throw new \Exception('POC email missing'); | |
| } | |
| public static function sendOutcomeEmail($visit) { | |
| // Validate visit data structure | |
| if (!isset($visit['Urban_Planned_Visit.Coordinating_Goonj_POC'])) { | |
| throw new \Exception('Invalid visit data: missing Coordinating_Goonj_POC'); | |
| } | |
| $coordinatingGoonjPOCId = $visit['Urban_Planned_Visit.Coordinating_Goonj_POC']; | |
| if (!$coordinatingGoonjPOCId) { | |
| throw new \Exception('Invalid visit data: Coordinating_Goonj_POC is empty'); | |
| } | |
| $coordinatingGoonjPOC = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $coordinatingGoonjPOCId) | |
| ->execute()->single(); | |
| if (!$coordinatingGoonjPOC) { | |
| throw new \Exception( | |
| sprintf('Contact not found for POC ID: %d', $coordinatingGoonjPOCId) | |
| ); | |
| } | |
| $coordinatingGoonjPOCEmail = $coordinatingGoonjPOC['email.email']; | |
| $coordinatingGoonjPOCName = $coordinatingGoonjPOC['display_name']; | |
| error_log("coordinatingGoonjPOCEmail: " . print_r($coordinatingGoonjPOCEmail, TRUE)); | |
| error_log("coordinatingGoonjPOCName: " . print_r($coordinatingGoonjPOCName, TRUE)); | |
| if (!$coordinatingGoonjPOCEmail) { | |
| throw new \Exception( | |
| sprintf('Email missing for POC: %s (ID: %d)', | |
| $coordinatingGoonjPOCName, | |
| $coordinatingGoonjPOCId | |
| ) | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php (1)
64-64: Return meaningful data for debugging or monitoring.
Currently, $returnValues is always empty. Consider including the number of emails sent or any relevant status details. This makes monitoring results simpler and enables better troubleshooting if issues arise.Example:
$sentCount = 0; ... foreach ($institutionVisit as $visit) { try { UrbanPlannedVisitService::sendOutcomeEmail($visit); + $sentCount++; } catch (\Exception $e) { ... } } ... return civicrm_api3_create_success(['emailsSent' => $sentCount], $params, 'Goonjcustom', 'urban_planned_visit_outcome_cron');wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (2)
13-13: Assess necessity of extending AutoSubscriber.
If none of the AutoSubscriber functionalities are leveraged, consider removing this inheritance for a more streamlined design.
73-80: Fix grammatical error in the email body.
“Please fills out the below form” should be “Please fill out the form below” to improve readability and professionalism.Possible fix:
-<p>. Please fills out the below form:</p> +<p>Please fill out the form below:</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php(1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php (2)
51-51: Replace error_log with CiviCRM logging.
41-49: 🛠️ Refactor suggestion
Consider pagination to avoid potential timeouts.
When fetching a large number of records, consider batching or applying limits to prevent timeouts and performance degradation. Handling these records in chunks ensures smoother operation and predictable resource usage.
Example implementation:
$institutionVisit = EckEntity::get('Institution_Visit', FALSE)
->addSelect('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', 'Urban_Planned_Visit.Coordinating_Goonj_POC')
->addWhere('Urban_Planned_Visit.Coordinating_Goonj_POC', 'IS NOT NULL')
->addWhere('Urban_Planned_Visit.When_do_you_wish_to_visit_Goonj', '<=', $endOfDay)
->addClause('OR',
['Visit_Outcome.Outcome_Email_Sent', 'IS NULL'],
['Visit_Outcome.Outcome_Email_Sent', '=', 0]
)
+ ->setLimit(50)
->execute();Likely invalid or redundant comment.
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
43-44: Use CiviCRM logging system.
2 participants
Add Outcome email for visit
Summary by CodeRabbit
New Features
Bug Fixes