Send feedback Form To external coordinating poc#656
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 7 minutes and 50 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)
WalkthroughThe pull request introduces enhancements to the 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: 1
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (3)
24-26: Add a code comment explaining the need for this hook registration.
Setting up a hook like '&hook_civicrm_pre' is a pivotal change since it indicates that we’re intercepting modifications before they are committed. Adding a short doc comment about why it is needed (e.g., updating visit statuses) will help future maintainers quickly grasp its purpose.
160-177: Improve grammar in the email content.
The text “Please fills out the below form” has a grammatical error. Consider revising it to “Please fill out the form below” to maintain professional communication standards.
179-214: Avoid duplicating status-check logic.
The method 'checkVisitStatusAndIds' and 'updateVisitStatusAfterAuth' both handle visit status logic. Ensure this logic is abstracted cleanly to reduce duplication and keep the code DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
135-137: Confirm the fallback mechanism for missing external PoC email.
Throwing an exception if the external PoC email is not found might cause unexpected runtime errors. Consider logging the error or using a fallback procedure if no valid email is available.
| public static function updateVisitStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { | ||
| $visitStatusDetails = self::checkVisitStatusAndIds($objectName, $objectId, $objectRef); | ||
|
|
||
| if (!$visitStatusDetails) { | ||
| return; | ||
| } | ||
|
|
||
| $newVisitStatus = $visitStatusDetails['newVisitStatus']; | ||
| $currentVisitStatus = $visitStatusDetails['currentVisitStatus']; | ||
| error_log("newVisitStatus: " . print_r($newVisitStatus, TRUE)); | ||
| error_log("currentVisitStatus: " . print_r($currentVisitStatus, TRUE)); | ||
|
|
||
| if ($currentVisitStatus !== $newVisitStatus) { | ||
| if ($newVisitStatus === 'completed') { | ||
| $visitId = $objectRef['id'] ?? NULL; | ||
| if ($visitId === NULL) { | ||
| return; | ||
| } | ||
| error_log("visitId: " . print_r($visitId, TRUE)); | ||
| $visitData = EckEntity::get('Institution_Visit', FALSE) | ||
| ->addSelect('Urban_Planned_Visit.External_Coordinating_PoC') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute()->single(); | ||
|
|
||
| $externalCoordinatingPocId = $visitData['Urban_Planned_Visit.External_Coordinating_PoC']; | ||
|
|
||
| $externalCoordinatingGoonjPOC = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $externalCoordinatingPocId) | ||
| ->execute()->single(); | ||
|
|
||
| $externalCoordinatingGoonjPOCEmail = $externalCoordinatingGoonjPOC['email.email']; | ||
| $externalCoordinatingGoonjPOCName = $externalCoordinatingGoonjPOC['display_name']; | ||
|
|
||
| if (!$externalCoordinatingGoonjPOCEmail) { | ||
| throw new \Exception('External POC email missing'); | ||
| } | ||
|
|
||
| $from = HelperService::getDefaultFromEmail(); | ||
|
|
||
| $mailParams = [ | ||
| 'subject' => 'Urban Planned Visit', | ||
| 'from' => $from, | ||
| 'toEmail' => $externalCoordinatingGoonjPOCEmail, | ||
| 'replyTo' => $from, | ||
| 'html' => self::getFeedbackEmailHtml($externalCoordinatingGoonjPOCName, $visitId), | ||
| ]; | ||
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| // if ($emailSendResult) { | ||
| // EckEntity::update('Institution_Visit', FALSE) | ||
| // ->addValue('Visit_Outcome.Feedback_Email_Sent', 1) | ||
| // ->addWhere('id', '=', $visitId) | ||
| // ->execute(); | ||
| // } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break down the method to maintain the Single Responsibility Principle.
Currently, 'updateVisitStatusAfterAuth' logs details, checks the new and old statuses, and sends an email. Splitting out responsibilities (e.g., logging, performing state checks, and sending emails) into separate smaller functions can improve readability, maintainability, and testability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (3)
167-181: Email template needs improvements
- The email content contains a grammatical error: ". Please fills out"
- HTML template is hardcoded in the code
- Similar structure to
getOutcomeEmailHtmlsuggests potential for template reuseConsider:
- Fix the grammar in the email content:
- <p>. Please fills out the below form:</p> + <p>Please fill out the form below:</p>
- Move the email template to an external file for better maintainability
- Create a shared template system for both feedback and outcome emails since they share similar structure
196-218: LGTM: Well-structured validation methodThe method is well-documented and follows single responsibility principle. Good validation checks and clear return structure.
Consider adding type hints for the return value in PHP 7.4+ style:
- public static function checkVisitStatusAndIds(string $objectName, $objectId, &$objectRef) { + public static function checkVisitStatusAndIds(string $objectName, $objectId, &$objectRef): ?array {
Line range hint
1-219: Consider architectural improvements for better maintainabilityThe class could benefit from several architectural improvements:
- Email handling is scattered across multiple methods with similar patterns
- Debug logging is inconsistent
- Error handling could be more robust
Consider:
- Creating a dedicated EmailService class to handle all email operations
- Implementing proper logging using CiviCRM's logging system
- Adding a proper exception handling strategy
- Creating interfaces for the main service components to make testing easier
Would you like me to provide a detailed example of how to implement these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(2 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (2)
24-26: LGTM: Hook subscription properly configured
The hook registration follows CiviCRM conventions for subscribing to pre-database operations.
88-162: 🛠️ Refactor suggestion
Method needs significant refactoring to improve maintainability
The method has several issues:
- Violates Single Responsibility Principle by handling status checking, email generation, and database updates
- Contains duplicate email sending logic similar to
sendOutcomeEmail - Has deep nesting which reduces readability
- Contains debug logging statements that should be removed or use proper logging system
Consider breaking this down into smaller, focused methods:
public static function updateVisitStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) {
$visitStatusDetails = self::checkVisitStatusAndIds($objectName, $objectId, $objectRef);
if (!$visitStatusDetails || $visitStatusDetails['currentVisitStatus'] === $visitStatusDetails['newVisitStatus']) {
return;
}
- // Current implementation
+ if ($visitStatusDetails['newVisitStatus'] === 'completed') {
+ self::handleCompletedVisit($objectId, $objectRef);
+ }
}
+ private static function handleCompletedVisit($objectId, $objectRef) {
+ $visitId = $objectRef['id'] ?? null;
+ if ($visitId === null) {
+ return;
+ }
+
+ $externalPOC = self::getExternalPOCDetails($objectId);
+ if (!$externalPOC['email']) {
+ throw new \Exception('External POC email missing');
+ }
+
+ if (self::sendFeedbackEmail($externalPOC, $visitId)) {
+ self::updateFeedbackEmailStatus($visitId);
+ }
+ }Also, consider:
- Using a proper logging system instead of
error_log - Creating a dedicated email service to handle email operations
- Adding error handling for the email sending operation
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
100-162: 🛠️ Refactor suggestionMethod needs refactoring to improve maintainability.
The method handles too many responsibilities:
- Status checking
- Email preparation
- Database updates
- Error handling
This makes it harder to maintain and test.
Consider breaking it down into smaller, focused methods:
public static function updateVisitStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { $visitStatusDetails = self::checkVisitStatusAndIds($objectName, $objectId, $objectRef); if (!$visitStatusDetails || $visitStatusDetails['currentVisitStatus'] === $visitStatusDetails['newVisitStatus']) { return; } if ($visitStatusDetails['newVisitStatus'] === 'completed') { self::handleCompletedVisitFeedback($objectRef); } } + private static function handleCompletedVisitFeedback(&$objectRef) { + $visitId = $objectRef['id'] ?? NULL; + if (!$visitId || self::isFeedbackAlreadySent($visitId)) { + return; + } + + $pocEmail = self::getExternalPocEmail($objectRef); + self::sendFeedbackEmail($pocEmail, $visitId); + }
🧹 Nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (4)
24-26: Consider cleaning up commented-out code.There's a TODO comment above this hook registration. Either implement the commented hook or remove it to maintain clean code.
171-179: Fix grammatical errors in email template.The email template contains grammatical errors:
- "Please fills out" should be "Please fill out"
Also, consider creating a reusable email template system since this is very similar to
getOutcomeEmailHtml.
196-218: Consider extracting magic string to a constant.The string 'Eck_Institution_Visit' should be defined as a class constant to improve maintainability and prevent typos.
+ private const INSTITUTION_VISIT_ENTITY = 'Eck_Institution_Visit'; public static function checkVisitStatusAndIds(string $objectName, $objectId, &$objectRef) { - if ($objectName != 'Eck_Institution_Visit') { + if ($objectName != self::INSTITUTION_VISIT_ENTITY) {
139-141: Improve error message with more context.The exception message could be more helpful by including the POC ID or name:
- throw new \Exception('External POC email missing'); + throw new \Exception(sprintf('External POC email missing for POC ID: %s (%s)', + $externalCoordinatingPocId, + $externalCoordinatingGoonjPOCName));
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
167-181: Fix grammatical error and consider template externalization.
- The email template contains a grammatical error: "Please fills out" should be "Please fill out"
- Consider moving the email templates to external files for better maintainability and easier editing by non-developers.
- <p>. Please fills out the below form:</p> + <p>Please fill out the below form:</p>Consider creating a template file system:
private static function getFeedbackEmailHtml($externalCoordinatingGoonjPOCName, $visitId) { $templateVars = [ 'pocName' => $externalCoordinatingGoonjPOCName, 'formUrl' => self::getVisitFeedbackFormUrl($visitId) ]; return self::renderTemplate('feedback-email', $templateVars); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(2 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (3)
24-26: LGTM: Event subscription is properly configured.
The hook registration for updateVisitStatusAfterAuth is correctly implemented.
196-218: LGTM: Well-structured validation method.
The method is well-documented, has clear responsibility, and implements proper validation checks. The error handling and return type documentation are particularly good.
168-169: Verify URL generation security.
The URL generation combines user input ($visitId) with base URL. While baseCMSURL() should be safe, verify that:
$visitIdis properly sanitized- The URL structure prevents parameter pollution
✅ Verification successful
Let me gather more information about how $visitId is obtained and validated.
Let me try another search to understand how $visitId is obtained and used in the service.
URL generation is secure in this context
The code review reveals that $visitId is obtained from trusted sources and properly validated:
- It comes from internal CiviCRM API calls (EckEntity)
- The ID is validated through
checkVisitStatusAndIds()method - The base URL is obtained through CiviCRM's secure
baseCMSURL()method - The URL structure uses fragment identifiers (#?) which prevents parameter pollution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL sanitization in the codebase
rg -A 2 "baseCMSURL|htmlspecialchars.*\$visitId"
# Look for other instances of URL generation
ast-grep --pattern 'htmlspecialchars($_, $_)'
Length of output: 20658
Script:
#!/bin/bash
# Search for visitId validation and usage in UrbanPlannedVisitService
rg -A 5 "\$visitId" wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php
# Look for validation patterns in the codebase
ast-grep --pattern 'intval($visitId)'
ast-grep --pattern 'filter_var($visitId)'
Length of output: 18721
Script:
#!/bin/bash
# Get the full content of UrbanPlannedVisitService.php to understand the context
cat wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php
# Look for any input validation or sanitization patterns in the service
rg -A 3 "sanitize|validate|clean" wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php
Length of output: 6766
| /** | ||
| * This hook is called after a db write on entities. | ||
| * | ||
| * @param string $op | ||
| * The type of operation being performed. | ||
| * @param string $objectName | ||
| * The name of the object. | ||
| * @param int $objectId | ||
| * The unique identifier for the object. | ||
| * @param object $objectRef | ||
| * The reference to the object. | ||
| */ | ||
| public static function updateVisitStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) { | ||
| $visitStatusDetails = self::checkVisitStatusAndIds($objectName, $objectId, $objectRef); | ||
|
|
||
| if (!$visitStatusDetails) { | ||
| return; | ||
| } | ||
|
|
||
| $newVisitStatus = $visitStatusDetails['newVisitStatus']; | ||
| $currentVisitStatus = $visitStatusDetails['currentVisitStatus']; | ||
|
|
||
| if ($currentVisitStatus !== $newVisitStatus) { | ||
| if ($newVisitStatus === 'completed') { | ||
| $visitId = $objectRef['id'] ?? NULL; | ||
| if ($visitId === NULL) { | ||
| return; | ||
| } | ||
|
|
||
| $visitFeedbackSent = EckEntity::get('Institution_Visit', TRUE) | ||
| ->addSelect('Visit_Feedback.Feedback_Email_Sent') | ||
| ->addWhere('id', '=', $visitId) | ||
| ->execute()->single(); | ||
|
|
||
| $isVisitFeedbackSent = $visitFeedbackSent['Visit_Feedback.Feedback_Email_Sent']; | ||
|
|
||
| if ($isVisitFeedbackSent !== NULL) { | ||
| return; | ||
| } | ||
|
|
||
| $externalCoordinatingPocId = $objectRef['Urban_Planned_Visit.External_Coordinating_PoC'] ?? ''; | ||
|
|
||
| $externalCoordinatingGoonjPOC = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $externalCoordinatingPocId) | ||
| ->execute()->single(); | ||
|
|
||
| $externalCoordinatingGoonjPOCEmail = $externalCoordinatingGoonjPOC['email.email']; | ||
| $externalCoordinatingGoonjPOCName = $externalCoordinatingGoonjPOC['display_name']; | ||
|
|
||
| if (!$externalCoordinatingGoonjPOCEmail) { | ||
| throw new \Exception('External POC email missing'); | ||
| } | ||
|
|
||
| $from = HelperService::getDefaultFromEmail(); | ||
|
|
||
| $mailParams = [ | ||
| 'subject' => 'Visit Feedback', | ||
| 'from' => $from, | ||
| 'toEmail' => $externalCoordinatingGoonjPOCEmail, | ||
| 'replyTo' => $from, | ||
| 'html' => self::getFeedbackEmailHtml($externalCoordinatingGoonjPOCName, $visitId), | ||
| ]; | ||
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($emailSendResult) { | ||
| EckEntity::update('Institution_Visit', FALSE) | ||
| ->addValue('Visit_Feedback.Feedback_Email_Sent', 1) | ||
| ->addWhere('id', '=', $visitId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method needs refactoring to improve maintainability.
The method has multiple responsibilities and contains duplicated email logic similar to sendOutcomeEmail. Consider breaking it down into smaller, focused methods:
- Status validation logic
- Email preparation logic
- Email sending logic
- Status update logic
Here's a suggested refactor:
public static function updateVisitStatusAfterAuth(string $op, string $objectName, $objectId, &$objectRef) {
$visitStatusDetails = self::checkVisitStatusAndIds($objectName, $objectId, $objectRef);
if (!$visitStatusDetails || $visitStatusDetails['currentVisitStatus'] === $visitStatusDetails['newVisitStatus']) {
return;
}
if ($visitStatusDetails['newVisitStatus'] === 'completed') {
self::sendFeedbackEmailIfNeeded($objectRef);
}
}
private static function sendFeedbackEmailIfNeeded(&$objectRef) {
$visitId = $objectRef['id'] ?? null;
if (!$visitId || self::isFeedbackAlreadySent($visitId)) {
return;
}
$pocEmail = self::getExternalPocEmail($objectRef);
if (!$pocEmail) {
throw new \Exception('External POC email missing');
}
if (self::sendFeedbackEmail($pocEmail, $visitId)) {
self::markFeedbackAsSent($visitId);
}
}
| if ($currentVisitStatus !== $newVisitStatus) { | ||
| if ($newVisitStatus === 'completed') { |
There was a problem hiding this comment.
can you combine this to only one if ?
| $externalCoordinatingGoonjPOCEmail = $externalCoordinatingGoonjPOC['email.email']; | ||
| $externalCoordinatingGoonjPOCName = $externalCoordinatingGoonjPOC['display_name']; |
There was a problem hiding this comment.
can you fix the naming here ?
changed this externalCoordinatingGoonjPOCEmail
to externalCoordinatingGoonjPocEmail
and Please check other places here
2 participants
Send feedback Form To external coordinating poc
Requirement: https://github.com/coloredcow-admin/goonj-crm/issues/170#issuecomment-2556349735
Summary by CodeRabbit