Feat/institution contribution receipt#634
Conversation
WalkthroughThis pull request introduces two new service classes, Changes
Sequence DiagramsequenceDiagram
participant Event as CiviCRM Event
participant Service as InstitutionCampaignService
participant API as CiviCRM API
participant Activity as Activity Creation
Event ->> Service: Trigger post-event hook
Service ->> API: Retrieve Campaign Details
API -->> Service: Return Campaign Information
Service ->> Activity: Create Campaign Organization Activity
Activity -->> Service: Confirm Activity Creation
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
🔭 Outside diff range comments (1)
wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php (1)
Line range hint
147-157: Consider consistent refactoring patternThe address field determination uses nested ternary operators, similar to what was previously refactored with the campField mapping. Consider applying the same mapping pattern here for consistency and readability.
- $addressField = ($subtype == 'Collection_Camp') - ? 'Collection_Camp_Intent_Details.Location_Area_of_camp' - : (($subtype == 'Dropping_Center') - ? 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_' - : (($subtype == 'Institution_Collection_Camp') - ? 'Institution_Collection_Camp_Intent.Collection_Camp_Address' - : (($subtype == 'Institution_Dropping_Center') - ? 'Institution_Dropping_Center_Intent.Dropping_Center_Address' - : NULL))); + $addressFieldMapping = [ + 'Collection_Camp' => 'Collection_Camp_Intent_Details.Location_Area_of_camp', + 'Dropping_Center' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', + 'Institution_Collection_Camp' => 'Institution_Collection_Camp_Intent.Collection_Camp_Address', + 'Institution_Dropping_Center' => 'Institution_Dropping_Center_Intent.Dropping_Center_Address' + ]; + + $addressField = $addressFieldMapping[$subtype] ?? NULL;
🧹 Nitpick comments (7)
wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php (2)
36-37: Documentation needs improvementThe current documentation "Er." is insufficient. Please provide meaningful documentation describing the purpose of
getSubscribedEvents()and its role in the event subscription system.- /** - * Er. - */ + /** + * {@inheritdoc} + * + * Subscribes to the alterMailParams hook to attach contribution receipts to emails. + * + * @return array + * An array of event subscriber configurations. + */
Line range hint
183-350: Consider template externalization and configuration improvementsThe HTML generation method could benefit from several architectural improvements:
- Move the HTML template to a separate file
- Extract social media URLs to configuration
- Make image paths configurable
- Consider using a template engine for better separation of concerns
Would you like me to provide a detailed implementation plan for these improvements?
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCampaignService.php (4)
21-23: Provide meaningful method documentation forgetSubscribedEvents.The method
getSubscribedEventslacks a descriptive doc comment. Adding a meaningful description will enhance code readability and maintainability.
26-28: Provide meaningful method documentation forlinkCollectionCampToContact.The method
linkCollectionCampToContactdoes not have a descriptive doc comment. Including details about its purpose, parameters, and expected behavior will improve code understanding.
73-75: Provide meaningful method documentation forcreateCollectionCampOrganizeActivity.The method
createCollectionCampOrganizeActivityhas minimal documentation. Elaborating on its functionality and parameters will assist in code comprehension.
76-76: Add type hints to method parameters forcreateCollectionCampOrganizeActivity.Adding type declarations improves code robustness and readability. Consider specifying parameter types and return type:
-private static function createCollectionCampOrganizeActivity($currentInstitutionId, $campaignTitle, $campaignId) { +private static function createCollectionCampOrganizeActivity(int $currentInstitutionId, string $campaignTitle, int $campaignId): void {wp-content/civi-extensions/goonjcustom/Civi/InstitutionMaterialContributionService.php (1)
175-193: Consider using a templating system for generating the email body ingenerateEmailBody.Embedding HTML directly within the code reduces readability. Utilizing a templating system or external template files can improve code organization and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCampaignService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionMaterialContributionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php(3 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php (1)
125-135: Well-structured mapping implementation
Good refactoring of the subtype-to-field mapping. The associative array improves readability and maintainability compared to nested ternary operators. The early return pattern is also well applied.
| error_log("activitiesmaterial: " . print_r($activities, TRUE)); | ||
|
|
There was a problem hiding this comment.
Remove or improve debug logging
The current error logging using print_r could expose sensitive data and should not be in production code. If logging is needed, consider:
- Using structured logging with appropriate log levels
- Logging only specific fields rather than the entire array
- Sanitizing sensitive information
- error_log("activitiesmaterial: " . print_r($activities, TRUE));
+ if (\Civi::settings()->get('debug_enabled')) {
+ \Civi::log()->debug('Material contribution activities processed', [
+ 'activity_count' => $activities->count(),
+ 'activity_ids' => array_column((array) $activities, 'id')
+ ]);
+ }📝 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("activitiesmaterial: " . print_r($activities, TRUE)); | |
| if (\Civi::settings()->get('debug_enabled')) { | |
| \Civi::log()->debug('Material contribution activities processed', [ | |
| 'activity_count' => $activities->count(), | |
| 'activity_ids' => array_column((array) $activities, 'id') | |
| ]); | |
| } |
| error_log("objectName " . print_r($objectName, TRUE)); | ||
| error_log("objectId " . print_r($objectId, TRUE)); | ||
| error_log("objectRef " . print_r($objectRef, TRUE)); |
There was a problem hiding this comment.
Avoid logging sensitive information using error_log.
Logging variables like $objectRef, $institutionCampaign, $campaignTitle, and $campaignId may expose sensitive data. Use a proper logging mechanism with appropriate log levels and avoid logging sensitive information in production.
Also applies to: 55-55, 63-63, 65-65
| $currentInstitutionCampaign = $institutionCampaign->first(); | ||
| $currentInstitutionId= $currentInstitutionCampaign['Additional_Details.Institution']; |
There was a problem hiding this comment.
Add null check before accessing $currentInstitutionCampaign to prevent errors.
If $institutionCampaign->first() returns null, accessing its properties will cause an error. Ensure $currentInstitutionCampaign is not null before proceeding.
Apply this diff to add the null check:
$currentInstitutionCampaign = $institutionCampaign->first();
+if (!$currentInstitutionCampaign) {
+ return;
+}
$currentInstitutionId = $currentInstitutionCampaign['Additional_Details.Institution'];📝 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.
| $currentInstitutionCampaign = $institutionCampaign->first(); | |
| $currentInstitutionId= $currentInstitutionCampaign['Additional_Details.Institution']; | |
| $currentInstitutionCampaign = $institutionCampaign->first(); | |
| if (!$currentInstitutionCampaign) { | |
| return; | |
| } | |
| $currentInstitutionId = $currentInstitutionCampaign['Additional_Details.Institution']; |
|
|
||
| <div style="width: 100%; font-size: 14px;"> | ||
| <div style="float: left; text-align: left;"> | ||
| <!-- Material Acknowledgment# {$activity['id']} --> |
There was a problem hiding this comment.
Fix undefined variable $activity in generateContributionReceiptHtml.
The variable $activity is used but not defined within this method. This will cause an undefined variable error. Ensure that $activity is defined or passed as a parameter if needed.
| * | ||
| */ | ||
| private static function prepareEmailParams(string $emailSubject, string $emailBody, array $attachments, string $recipientEmail) { | ||
| $from = HelperService::getDefaultFromEmail(); |
There was a problem hiding this comment.
Undefined class HelperService used in prepareEmailParams.
The class HelperService is not imported or defined. This will result in an error when calling HelperService::getDefaultFromEmail(). Ensure HelperService is properly imported or defined.
| private static function getBranchWisePOCs($campaignId) { | ||
| $contacts = []; | ||
| $campaign = Campaign::get(TRUE) | ||
| ->addSelect('Additional_Details.Branch_Wise_POCs') | ||
| ->addWhere('id', '=', $campaignId) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| $branchWisePOCs = $campaign['Additional_Details.Branch_Wise_POCs'] ?? []; | ||
| foreach ($branchWisePOCs as $contactId) { | ||
| $contact = self::getContactDetails($contactId); | ||
| if ($contact) { | ||
| $contacts[$contact['email']] = $contact; | ||
| } | ||
| } | ||
| return $contacts; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate duplicate code in contact-fetching methods.
The methods getBranchWisePOCs, getCampaignInstitutionPOC, and getInstitutionRelationships contain similar logic for fetching contacts. Refactoring to extract common functionality will reduce code duplication and enhance maintainability.
Also applies to: 99-114, 119-134
| private static function generateContributionReceiptHtml($email, $contactPhone, $description, $contactName, $deliveredBy, $deliveredByContact, $activityDate) { | ||
|
|
||
| $baseDir = plugin_dir_path(__FILE__) . '../../../themes/goonj-crm/'; | ||
|
|
||
| $paths = [ | ||
| 'logo' => $baseDir . 'images/goonj-logo.png', | ||
| 'qrCode' => $baseDir . 'images/qr-code.png', | ||
| 'callIcon' => $baseDir . 'Icon/call.png', | ||
| 'domainIcon' => $baseDir . 'Icon/domain.png', | ||
| 'emailIcon' => $baseDir . 'Icon/email.png', | ||
| 'facebookIcon' => $baseDir . 'Icon/facebook.webp', | ||
| 'instagramIcon' => $baseDir . 'Icon/instagram.png', | ||
| 'twitterIcon' => $baseDir . 'Icon/twitter.webp', | ||
| 'youtubeIcon' => $baseDir . 'Icon/youtube.webp', | ||
| ]; | ||
|
|
||
| $imageData = array_map(fn ($path) => base64_encode(file_get_contents($path)), $paths); | ||
|
|
||
| $html = <<<HTML | ||
| <html> | ||
| <body style="font-family: Arial, sans-serif;"> | ||
| <div style="text-align: center; margin-bottom: 16px;"> | ||
| <img src="data:image/png;base64,{$imageData['logo']}" alt="Goonj Logo" style="width: 95px; height: 80px;"> | ||
| </div> | ||
|
|
||
| <div style="width: 100%; font-size: 14px;"> | ||
| <div style="float: left; text-align: left;"> | ||
| <!-- Material Acknowledgment# {$activity['id']} --> | ||
| </div> | ||
| <div style="float: right; text-align: right;"> | ||
| Goonj, C-544, Pocket C, Sarita Vihar, Delhi, India | ||
| </div> | ||
| </div> | ||
| <br><br> | ||
| <div style="font-weight: bold; font-style: italic; margin-top: 6px; margin-bottom: 6px;"> | ||
| "We appreciate your contribution of pre-used/new material. Goonj makes sure that the material reaches people with dignity and care." | ||
| </div> | ||
| <table border="1" cellpadding="10" cellspacing="0" style="width: 100%; border-collapse: collapse;"> | ||
| <style> | ||
| .table-header { | ||
| text-align: left; | ||
| font-weight: bold; | ||
| } | ||
| </style> | ||
| <!-- Table rows for each item --> | ||
| <tr> | ||
| <td class="table-header">Description of Material *</td> | ||
| <td style="text-align: center;">{$description}</td> | ||
| </tr> | ||
| <tr> | ||
| <td class="table-header">Received On</td> | ||
| <td style="text-align: center;">{$activityDate}</td> | ||
| </tr> | ||
| <tr> | ||
| <td class="table-header">From</td> | ||
| <td style="text-align: center;">{$contactName}</td> | ||
| </tr> | ||
| <tr> | ||
| <td class="table-header">Email</td> | ||
| <td style="text-align: center;">{$email}</td> | ||
| </tr> | ||
| <tr> | ||
| <td class="table-header">Phone</td> | ||
| <td style="text-align: center;">{$contactPhone}</td> | ||
| </tr> | ||
| <tr> | ||
| <td class="table-header">Delivered by (Name & contact no.)</td> | ||
| <td style="text-align: center;"> | ||
| {$deliveredBy}<br> | ||
| {$deliveredByContact} | ||
| </td> | ||
| </tr> | ||
|
|
||
| </table> | ||
| <div style="text-align: right; font-size: 14px;"> | ||
| Team Goonj | ||
| </div> | ||
| <div style="width: 100%; margin-top: 16px;"> | ||
| <div style="float: left; width: 60%; font-size: 14px;"> | ||
| <p>Join us, by encouraging your friends, relatives, colleagues, and neighbours to join the journey as all of us have a lot to give.</p> | ||
| <p style="margin-top: 8px;"> | ||
| <strong>With Material Money Matters</strong> Your monetary contribution is needed too for sorting, packing, transportation to implementation. (Financial contributions are tax-exempted u/s 80G of IT Act) | ||
| </p> | ||
| <p style="margin-top: 10px; font-size: 12px; float: left">* Received material has 'No Commercial Value' for Goonj.</p> | ||
| </div> | ||
| <div style="float: right; width: 40%; text-align: right; font-size: 12px; font-style: italic;"> | ||
| <p>To contribute, please scan the code.</p> | ||
| <img src="data:image/png;base64,{$imageData['qrCode']}" alt="QR Code" style="width: 80px; height: 70px; margin-top: 2px"></div> | ||
| </div> | ||
| <div style="clear: both; margin-top: 20px;"></div> | ||
| <div style="width: 100%; margin-top: 15px; background-color: #f2f2f2; padding: 16px; font-weight: 300; color: #000000"> | ||
| <div style="font-size: 14px; margin-bottom: 20px;"> | ||
| <div style="position: relative; height: 24px;"> | ||
| <div style="font-size: 14px; float: left; color:"> | ||
| Goonj, C-544, 1st Floor, C-Pocket, Sarita Vihar,<br> | ||
| New Delhi-110076 | ||
| </div> | ||
| <div style="font-size: 14px; float: right;"> | ||
| <img src="data:image/png;base64,{$imageData['callIcon']}" alt="Phone" style="width: 16px; height: 16px; margin-right: 5px;"> | ||
| 011-26972351/41401216 | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div style="text-align: center; width: 100%; font-size: 14px; margin-bottom: 20px;"> | ||
| <div style="font-size: 14px;"> | ||
| <img src="data:image/png;base64,{$imageData['emailIcon']}" alt="Email" style="width: 16px; height: 16px; display: inline;"> | ||
| <span style="display: inline; margin-left: 0;">mail@goonj.org</span> | ||
| <img src="data:image/png;base64,{$imageData['domainIcon']}" alt="Website" style="width: 16px; height: 16px; margin-right: 5px;"> | ||
| <span style="display: inline; margin-left: 0;">www.goonj.org</span> | ||
| </div> | ||
| </div> | ||
|
|
||
| <!-- Social Media Icons --> | ||
| <div style="text-align: center; width: 100%; margin-top: 28px;"> | ||
| <a href="https://www.facebook.com/goonj.org" target="_blank"><img src="data:image/webp;base64,{$imageData['facebookIcon']}" alt="Facebook" style="width: 24px; height: 24px; margin-right: 10px;"></a> | ||
| <a href="https://www.instagram.com/goonj/" target="_blank"><img src="data:image/webp;base64,{$imageData['instagramIcon']}" alt="Instagram" style="width: 24px; height: 24px; margin-right: 10px;"></a> | ||
| <a href="https://x.com/goonj" target="_blank"><img src="data:image/webp;base64,{$imageData['twitterIcon']}" alt="Twitter" style="width: 24px; height: 24px; margin-right: 10px;"></a> | ||
| <a href="https://www.youtube.com/channel/UCCq8iYlmjT7rrgPI1VHzIHg" target="_blank"><img src="data:image/webp;base64,{$imageData['youtubeIcon']}" alt="YouTube" style="width: 24px; height: 24px; margin-right: 10px;"></a> | ||
| </div> | ||
| </div> | ||
| <p style="margin-bottom: 2px; text-align: center; font-size: 12px;">* This is a computer generated receipt, signature is not required.</p> | ||
| </body> | ||
| </html> | ||
| HTML; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor generateContributionReceiptHtml to adhere to the single responsibility principle.
The method is lengthy and handles multiple responsibilities, making it hard to read and maintain. Consider breaking it down into smaller methods or using a templating system to improve clarity.
1 participant
Summary by CodeRabbit
New Features
InstitutionCampaignServicefor handling campaign-related events and logging activities.InstitutionMaterialContributionServiceto process material contributions and send acknowledgment emails.Bug Fixes
Refactor
MaterialContributionServicefor determining campaign fields and address retrieval.Documentation
getSubscribedEventsmethod inMaterialContributionService.