Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 3
🧹 Outside diff range and nitpick comments (9)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (5)
Line range hint
1-24: Consider splitting this class to follow Single Responsibility PrincipleThe class handles multiple distinct responsibilities:
- QR code generation
- Email notifications
- Office management
- UI tabset management
- Event subscription
This violates the Single Responsibility Principle and makes the code harder to maintain.
Consider splitting this into smaller, focused service classes:
InstitutionCampQRServiceInstitutionCampNotificationServiceInstitutionCampOfficeServiceInstitutionCampUIService
Line range hint
266-397: Refactor email-related methods to reduce duplicationThe email sending logic is duplicated across multiple methods with similar patterns. This makes maintenance difficult and increases the chance of bugs.
Consider:
- Creating a reusable email template system
- Extracting common email sending logic into a separate method
- Using a configuration file for email subjects and content
Example refactor:
+ private static function sendEmail($recipient, $template, $params) { + $from = HelperService::getDefaultFromEmail(); + $mailParams = [ + 'subject' => $params['subject'], + 'from' => $from, + 'toEmail' => $recipient['email'], + 'replyTo' => $from, + 'html' => self::renderTemplate($template, $params), + ]; + return \CRM_Utils_Mail::send($mailParams); + }
Line range hint
266-284: Improve error handling in email sending logicThe error handling in the email sending logic is inconsistent. Some errors are logged while others are silently ignored.
Consider:
- Adding proper error handling for all email operations
- Using a consistent logging pattern
- Implementing retry logic for failed email attempts
Example improvement:
- if (!$recipientEmail) { - \Civi::log()->info("Recipient email missing: $campId"); - } + if (!$recipientEmail) { + \Civi::log()->error("Failed to send email - Recipient email missing for camp: $campId"); + throw new \CRM_Core_Exception("Recipient email not found"); + }
Line range hint
266-397: Break down long methods and improve documentationThe
sendLogisticsEmailmethod is too long (130+ lines) and handles multiple responsibilities. This makes it difficult to understand and maintain.Consider:
- Breaking down the method into smaller, focused methods
- Adding proper PHPDoc blocks
- Using meaningful variable names
Example refactor:
+ /** + * Sends logistics email for self-managed camps + * + * @param array $camp Camp details + * @param string $recipientEmail Recipient's email + * @return bool Success status + */ + private static function sendSelfManagedCampEmail($camp, $recipientEmail) { + // Implementation + } + /** + * Sends logistics email for regular camps + * + * @param array $camp Camp details + * @param string $recipientEmail Recipient's email + * @return bool Success status + */ + private static function sendRegularCampEmail($camp, $recipientEmail) { + // Implementation + }
Line range hint
398-428: Fix potential XSS vulnerabilities in email templatesThe email templates directly inject variables into HTML without proper escaping, which could lead to XSS vulnerabilities.
Apply proper escaping:
- <p>Dear $contactName,</p> - <p>Thank you for attending the camp <strong>$campCode</strong> at <strong>$campAddress</strong>. + <p>Dear <?php echo htmlspecialchars($contactName, ENT_QUOTES, 'UTF-8'); ?></p> + <p>Thank you for attending the camp <strong><?php echo htmlspecialchars($campCode, ENT_QUOTES, 'UTF-8'); ?></strong> at <strong><?php echo htmlspecialchars($campAddress, ENT_QUOTES, 'UTF-8'); ?></strong>.wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InstitutionCampFeedbackCron.php (4)
109-111: Provide PHPDoc comments for 'sendFeedbackEmail'The
sendFeedbackEmailfunction lacks a descriptive PHPDoc block. Adding appropriate documentation enhances code maintainability and provides clarity on the function's purpose and parameters.Example:
/** * Generates the HTML content for the feedback email to the organizing contact. * * @param string $organizingContactName * The name of the organizing contact. * @param int $collectionCampId * The ID of the collection camp. * @param string $campAddress * The address of the camp. * * @return string * The HTML content of the feedback email. */ function generateFeedbackEmailContent($organizingContactName, $collectionCampId, $campAddress) { // ... function content ... }
118-125: Externalize email content for maintainability and localizationThe email subject and body are hardcoded within the code. To facilitate easier updates and support localization, consider using a templating system or storing the content in external configuration files.
For example, use the CiviCRM messaging templates or a template engine to manage email content:
// Example using CiviCRM template $html = CRM_Core_Smarty::singleton()->fetch('YourTemplate.tpl', $templateVars);This approach allows non-developers to modify email content without changing the code.
78-79: Standardize variable naming for clarityThe variable
$contactEmailIdholds an email address rather than an identifier. Renaming it to$contactEmailimproves readability and accurately reflects its content.Apply this diff:
-$contactEmailId = $campAttendedBy['email.email']; +$contactEmail = $campAttendedBy['email.email']; $organizingContactName = $campAttendedBy['display_name']; $mailParams = [ // ... other parameters ... - 'toEmail' => $contactEmailId, + 'toEmail' => $contactEmail, // ... other parameters ... ];
20-22: Remove or properly implement the empty specification functionThe
_civicrm_api3_goonjcustom_institution_camp_feedback_cron_specfunction is currently empty. If no specification is needed, it can be removed. Otherwise, provide proper documentation or validations as required.If removing:
-function _civicrm_api3_goonjcustom_institution_camp_feedback_cron_spec(&$spec) { - // There are no parameters for the Goonjcustom cron. -}If keeping, consider adding meaningful content or documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InstitutionCampFeedbackCron.php(1 hunks)
| catch (Exception $e) { | ||
| \Civi::log()->info("Error processing camp ID $collectionCampId: " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Fix potential undefined variable in exception handling
In the catch block, the variable $collectionCampId may be undefined if an exception occurs before it is assigned. This could lead to an undefined variable notice or incorrect logging information.
Apply this diff to initialize $collectionCampId before the try block:
+ $collectionCampId = null;
try {
$endDate = new DateTime($camp['Institution_Collection_Camp_Intent.Collections_will_end_on_Date_']);
+ $collectionCampId = $camp['id'];
// ... rest of the code ...
} catch (Exception $e) {
\Civi::log()->info("Error processing camp ID $collectionCampId: " . $e->getMessage());
}Committable suggestion skipped: line range outside the PR's diff.
| $organizingContactName = $campAttendedBy['display_name']; | ||
|
|
||
| // Send email if the end date is today or earlier. | ||
| if (!$feedbackEmailSent && $endDateFormatted <= $todayFormatted) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compare DateTime objects directly for accuracy
Comparing dates as formatted strings may lead to subtle bugs due to formatting differences. It's more reliable to compare DateTime objects directly.
Modify the condition to compare DateTime objects:
-$endDateFormatted = $endDate->format('Y-m-d');
-$todayFormatted = $today->format('Y-m-d');
// ...
-if (!$feedbackEmailSent && $endDateFormatted <= $todayFormatted) {
+if (!$feedbackEmailSent && $endDate <= $today) {Committable suggestion skipped: line range outside the PR's diff.
| function sendFeedbackEmail($organizingContactName, $collectionCampId, $campAddress) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| // URL for the volunteer feedback form. | ||
| $campVolunteerFeedback = $homeUrl . 'volunteer-institution-camp-feedback/#?Eck_Collection_Camp1=' . $collectionCampId; | ||
|
|
||
| $html = " | ||
| <p>Dear $organizingContactName,</p> | ||
| <p>Thank you for stepping up and organising the recent collection drive at <strong>$campAddress</strong>! Your time, effort, and enthusiasm made all the difference, and we hope that it was a meaningful effort for you as well.</p> | ||
| <p>To help us improve, we’d love to hear your thoughts and experiences. Kindly take a few minutes to fill out our feedback form. Your input will be valuable to us:</p> | ||
| <p><a href=\"$campVolunteerFeedback\">Feedback Form Link</a></p> | ||
| <p>Feel free to share any highlights, suggestions, or challenges you faced. We're eager to learn how we can make it better together!</p> | ||
| <p>We look forward to continuing this journey together!</p> | ||
| <p>Warm Regards,<br>Team Goonj</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename 'sendFeedbackEmail' function for clarity
The function sendFeedbackEmail generates the email content but does not send the email. To adhere to the single responsibility principle and improve code readability, consider renaming the function to better reflect its purpose, such as generateFeedbackEmailContent.
Apply this diff to rename the function:
-function sendFeedbackEmail($organizingContactName, $collectionCampId, $campAddress) {
+function generateFeedbackEmailContent($organizingContactName, $collectionCampId, $campAddress) {
// ... function content ...
}
// Update the function call:
$mailParams = [
// ... other parameters ...
- 'html' => sendFeedbackEmail($organizingContactName, $collectionCampId, $campAddress),
+ 'html' => generateFeedbackEmailContent($organizingContactName, $collectionCampId, $campAddress),
];Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
583-588: Consider refactoring to eliminate duplicate code in tab configurationsThe
'template'parameter is the same for all tab configurations. You can reduce code duplication and improve maintainability by setting this default value within the loop instead of repeating it in each configuration.Apply this diff to refactor the code:
$tabConfigs = [ 'logistics' => [ 'title' => ts('Logistics'), 'module' => 'afsearchInstitutionCollectionCampLogistics', 'directive' => 'afsearch-institution-collection-camp-logistics', - 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', ], // ... other tab configurations ... 'campFeedback' => [ 'title' => ts('Volunteer Feedback'), 'module' => 'afsearchInstitutionCollectionCampFeedback', 'directive' => 'afsearch-institution-collection-camp-feedback', - 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', ], ]; foreach ($tabConfigs as $key => $config) { $tabs[$key] = [ 'id' => $key, 'title' => $config['title'], 'is_active' => 1, + 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', 'module' => $config['module'], 'directive' => $config['directive'], ]; \Civi::service('angularjs.loader')->addModules($config['module']); }
583-588: Simplify generation of 'module' and 'directive' valuesThe
'module'and'directive'values in your tab configurations follow a consistent pattern based on the key. You can simplify the code and reduce duplication by generating these values dynamically.Apply this diff to refactor the code:
$tabConfigs = [ 'logistics' => [ 'title' => ts('Logistics'), ], // ... other tab configurations ... 'campFeedback' => [ 'title' => ts('Volunteer Feedback'), ], ]; foreach ($tabConfigs as $key => $config) { + $moduleName = 'afsearchInstitutionCollectionCamp' . ucfirst($key); + $directiveName = 'afsearch-institution-collection-camp-' . $key; $tabs[$key] = [ 'id' => $key, 'title' => $config['title'], 'is_active' => 1, 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', - 'module' => $config['module'], - 'directive' => $config['directive'], + 'module' => $moduleName, + 'directive' => $directiveName, ]; \Civi::service('angularjs.loader')->addModules($tabs[$key]['module']); }
|
Self Review Done, Due to Priority Merging this PR for now. |
1 participant
Send the feedback email to institution POC on the day of camp
Summary by CodeRabbit
New Features
Bug Fixes