Feat/institute goonj activities #591
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (6)
wp-content/civi-extensions/goonjcustom/Civi/IntitutionGoonjActivitiesService.php (4)
869-911: Code Duplication in Address HandlingThe methods
setGoonjActivitiesAddressandsetActivitiesVolunteersAddresscontain similar logic for setting addresses.Consider consolidating the shared logic into a private helper method to avoid duplication and enhance maintainability.
625-628: Consider Using 'mt_rand()' Instead of 'rand()'The method
getCoordinatorusesrand()for random selection, which may have lower randomness quality.Replace
rand()withmt_rand()for better randomness performance:- ? $existingCoordinators->itemAt(rand(0, $coordinatorCount - 1)) + ? $existingCoordinators->itemAt(mt_rand(0, $coordinatorCount - 1))
110-120: Enhance Error Handling in 'addContactToGroup'Currently, exceptions are caught, and errors are only logged without notifying the caller.
Consider rethrowing the exception or returning a failure status to ensure that calling methods are aware of the issue.
catch (Exception $e) { \Civi::log()->error("Error adding contact_id: $contactId to group_id: $groupId. Exception: " . $e->getMessage()); throw $e; }
24-1037: Lack of Method DocumentationSeveral methods lack docblocks or have empty docblocks, which reduces code understandability.
Add meaningful docblocks to each method, describing its purpose, parameters, and return values.
Example:
/** * Assigns the chapter group to an individual based on state and organization. * * @param string $op * @param string $objectName * @param int $objectId * @param array &$objectRef */ public static function assignChapterGroupToIndividual(string $op, string $objectName, $objectId, &$objectRef) { // Method implementation... }wp-content/civi-extensions/goonjcustom/config/eventCode.php (1)
Line range hint
1-35: Improve code organization and documentation
- The @file docblock is empty. Consider adding meaningful documentation about the purpose of these event codes and their usage.
- The array entries are not alphabetically sorted, which makes it harder to maintain and locate specific entries.
- There are multiple pairs of similar entries sharing the same codes:
- 'Collection Camp' and 'Institution Collection Camp' both use 'CC'
- 'Dropping Center' and 'Institution Dropping Center' both use 'DC'
Consider:
- Adding proper documentation
- Organizing entries alphabetically
- Using distinct codes for institutional vs non-institutional events
Example improvement:
<?php /** - * @file + * @file + * Event codes configuration for the Goonj system. + * + * This file defines the mapping between event names and their corresponding + * short codes used throughout the system. Each event type has a unique + * identifier used for tracking and reporting purposes. */ return [ + 'Activity' => 'AY', 'Book Fair' => 'BK', + 'Capacity Building Session- NJPC' => 'CBS', 'Chaupal' => 'Chaupal', // ... (rest of the entries in alphabetical order) ];wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (1)
523-526: Consider improving naming consistencyThere's a slight inconsistency in the naming pattern:
Institute_Goonj_ActivitiesvsInstitution_Collection_Camp- One uses "Institute" while others use "Institution"
This could lead to confusion. Consider standardizing the naming convention across all entities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/IntitutionGoonjActivitiesService.php(1 hunks)wp-content/civi-extensions/goonjcustom/config/eventCode.php(1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/config/eventCode.php (1)
Line range hint 1-35: Verify the impact of duplicate event codes
Let's check how these event codes are used in the codebase to understand the potential impact of duplicate codes.
✅ Verification successful
Let me search for more specific usage patterns of event codes to better understand their impact.
Let me check the CollectionSource trait to understand how these event codes are used.
Duplicate event codes are intentionally used for grouping similar events
After analyzing the codebase, I can confirm that the duplicate event codes ('DC' for both 'Institution Dropping Center' and 'Dropping Center', and 'GA' for both 'Goonj Activities' and 'Institution Goonj Activities') are intentionally used to group similar types of events. The CollectionSource trait shows that these codes are used to generate standardized titles in the format year/state_code/event_code/source_id, where similar events should share the same event code.
The implementation in generateCollectionSourceCode() method demonstrates that these codes are used for categorization purposes, and the uniqueness of each record is maintained by the source_id in the final title format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of event codes to understand potential conflicts
echo "Searching for GA event code usage..."
rg -A 5 "'GA'" --type php
echo "Searching for event code comparisons..."
ast-grep --pattern 'if ($_ === "GA")'
Length of output: 11235
Script:
#!/bin/bash
# Search for any usage of event codes in PHP files
echo "Searching for event code usage patterns..."
rg -A 5 "=> 'DC'" --type php
# Search for files that might be using these event codes
echo "Searching for files that might process event codes..."
fd -e php | xargs rg -l "eventCode"
# Search for any comparison or switch statements using event codes
echo "Searching for event code processing logic..."
ast-grep --pattern 'switch ($_) { $$$case$$$: }'
Length of output: 11062
Script:
#!/bin/bash
# Check the implementation in CollectionSource.php
echo "Checking CollectionSource.php implementation..."
cat "wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php"
# Look for any other files that might be using these event codes
echo "Searching for files referencing specific event codes..."
rg -l "'DC'|'GA'" --type php
Length of output: 8836
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (2)
523-526: Verify state group mappings for new institution entities
The addition of new institution-related mappings looks good, but let's verify that the corresponding custom groups exist in the system.
Let's verify the existence of these custom groups:
✅ Verification successful
✓ State group mappings are correctly implemented and used
The verification shows that all the custom group mappings in getStateGroupNameMapper are actively used in the codebase:
Institution_Collection_Camp_Intentis used in InstitutionCollectionCampService.php for managing collection camp dataInstitution_Dropping_Center_Intentis used in InstitutionDroppingCenterService.php for dropping center operations- Both services properly reference these custom groups in their API queries and field operations
The mappings are consistent with the actual implementation and usage patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the custom groups referenced in the mappings exist
# Search for custom group definitions in PHP files
rg -l "Institution_Collection_Camp_Intent|Goonj_Activities|Institution_Dropping_Center_Intent|Institute_Goonj_Activities" --type php
# Search for any XML installer files that might define these custom groups
fd -e xml | xargs grep -l "Institution_Collection_Camp_Intent\|Goonj_Activities\|Institution_Dropping_Center_Intent\|Institute_Goonj_Activities"
Length of output: 1901
Script:
#!/bin/bash
# Let's check the actual implementation and usage of these custom groups
# First, let's look at the service implementations
rg -A 5 "Institution_Collection_Camp_Intent|Institution_Dropping_Center_Intent" wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php
# Check for custom group definitions in XML installer files
fd "xml$" --full-path wp-content/civi-extensions/goonjcustom | xargs cat
# Check for any database schema or installation related files
fd "(sql|install|upgrade)" --full-path wp-content/civi-extensions/goonjcustom
# Look for any CiviCRM API calls or definitions related to these custom groups
rg "custom_group.*(_name|_id).*=.*('|\").*Intent" wp-content/civi-extensions/goonjcustom
Length of output: 16101
523-526: Verify ACL impact for new mappings
The aclCollectionCamp method uses these mappings indirectly through getStateFieldDbDetails. Ensure that ACL restrictions work correctly for the new institution entities.
Let's check for any ACL-related code that might need updates:
✅ Verification successful
Let me gather more information about the ACL implementation to verify its impact.
Let me try one more search to understand the complete ACL implementation.
ACL implementation verified for new institution entities
The ACL implementation in aclCollectionCamp method correctly handles the new institution mappings through:
- Dynamic state field resolution via
getStateFieldDbDetails() - Proper mapping of institution entities in
getStateGroupNameMapper() - Unified ACL query generation for all collection camp subtypes
The changes maintain consistent ACL restrictions across all institution types by using the same state-based access control mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for ACL-related code that might need updates for new entities
# Search for ACL-related code referencing these entities
rg "aclCollectionCamp|ACL.*Institution|Institution.*ACL" --type php
# Search for any permission checks related to these entities
ast-grep --pattern 'checkPermission($$$Institution$$$)'
Length of output: 9690
Script:
#!/bin/bash
# Let's examine the aclCollectionCamp method implementation and its usage
# First, get the implementation of aclCollectionCamp
ast-grep --pattern 'function aclCollectionCamp($$$) {
$$$
}'
# Check for getStateFieldDbDetails usage
rg "getStateFieldDbDetails" -A 5 --type php
# Look for any other ACL-related hooks or implementations
rg "hook_civicrm_selectWhereClause" --type php
Length of output: 11598
Script:
#!/bin/bash
# Get the complete implementation of CollectionBaseService.php to understand ACL flow
cat wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php
# Also check for any state-related configurations
rg "getStateGroupNameMapper" -A 5 --type php
Length of output: 18582
| private static function sendDispatchEmail($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $pocEmail, $pocContactNumber, $nameOfInstitution, $addressOfInstitution) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| $campVehicleDispatchFormUrl = $homeUrl . 'institution-camp-vehicle-dispatch-form/#?Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId | ||
| . '&Camp_Vehicle_Dispatch.Filled_by=' . $campAttendedById | ||
| . '&Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent=' . $collectionCampGoonjOffice | ||
| . '&Eck_Collection_Camp1=' . $collectionCampId | ||
| . '&Camp_Institution_Data.Name_of_the_institution=' . $nameOfInstitution | ||
| . '&Camp_Institution_Data.Address=' . $addressOfInstitution | ||
| . '&Camp_Institution_Data.Email=' . $pocEmail | ||
| . '&Camp_Institution_Data.Contact_Number=' . $pocContactNumber; | ||
|
|
||
| $html = " | ||
| <p>Dear $contactName,</p> | ||
| <p>Thank you for attending the camp <strong>$campCode</strong> at <strong>$campAddress</strong>. There is a form that requires your attention during the camp:</p> | ||
| <ol> | ||
| <li><a href=\"$campVehicleDispatchFormUrl\">Dispatch Form</a><br> | ||
| Please complete this form from the camp location once the vehicle is being loaded and ready for dispatch to Goonj's processing center.</li> | ||
| </ol> | ||
| <p>We appreciate your cooperation.</p> | ||
| <p>Warm Regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| private static function sendOutcomeEmail($contactName, $collectionCampId, $coordinatingPOCId, $campCode, $campAddress) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| $campOutcomeFormUrl = $homeUrl . '/institution-camp-outcome-form/#?Eck_Collection_Camp1=' . $collectionCampId . '&Camp_Outcome.Filled_By=' . $campAttendedById; | ||
|
|
||
| $html = " | ||
| <p>Dear $contactName,</p> | ||
| <p>Thank you for attending the camp <strong>$campCode</strong> at <strong>$campAddress</strong>. There is one form that requires your attention after the camp:</p> | ||
| <ol> | ||
| <li><a href=\"$campOutcomeFormUrl\">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; | ||
| } | ||
|
|
||
| /** | ||
| * Generates the logistics email HTML content. | ||
| */ | ||
| private static function getLogisticsEmailHtml($contactName, $collectionCampId, $campAttendedById, $collectionCampGoonjOffice, $campCode, $campAddress, $pocEmail, $pocContactNumber, $nameOfInstitution, $addressOfInstitution) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
|
|
||
| $campVehicleDispatchFormUrl = $homeUrl | ||
| . 'institution-camp-vehicle-dispatch-form/#?Camp_Vehicle_Dispatch.Collection_Camp=' . $collectionCampId | ||
| . '&Camp_Vehicle_Dispatch.Filled_by=' . $campAttendedById | ||
| . '&Camp_Vehicle_Dispatch.To_which_PU_Center_material_is_being_sent=' . $collectionCampGoonjOffice | ||
| . '&Eck_Collection_Camp1=' . $collectionCampId | ||
| . '&Camp_Institution_Data.Name_of_the_institution=' . $nameOfInstitution | ||
| . '&Camp_Institution_Data.Address=' . $addressOfInstitution | ||
| . '&Camp_Institution_Data.Email=' . $pocEmail | ||
| . '&Camp_Institution_Data.Contact_Number=' . $pocContactNumber; | ||
|
|
||
| $campOutcomeFormUrl = $homeUrl . '/institution-camp-outcome-form/#?Eck_Collection_Camp1=' . $collectionCampId . '&Camp_Outcome.Filled_By=' . $campAttendedById; | ||
|
|
||
| $html = " | ||
| <p>Dear $contactName,</p> | ||
| <p>Thank you for attending the camp <strong>$campCode</strong> at <strong>$campAddress</strong>. There are two forms that require your attention during and after the camp:</p> | ||
| <ol> | ||
| <li><a href=\"$campVehicleDispatchFormUrl\">Dispatch Form</a><br> | ||
| Please complete this form from the camp location once the vehicle is being loaded and ready for dispatch to Goonj's processing center.</li> | ||
| <li><a href=\"$campOutcomeFormUrl\">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
Duplicate Code in Email Sending Methods
The methods sendDispatchEmail, sendOutcomeEmail, and getLogisticsEmailHtml contain similar code for generating email HTML content. This duplication violates the DRY (Don't Repeat Yourself) principle.
Consider refactoring these methods by extracting common code into a reusable method or using a templating system for generating emails. This will enhance maintainability and reduce the risk of inconsistencies.
| ->addWhere('id', '=', $campAttendedById) | ||
| ->execute()->single(); | ||
|
|
||
| $recipientEmail = $recipient['email.email']; | ||
| $recipientName = $recipient['display_name']; | ||
|
|
||
| if (!$recipientEmail) { | ||
| \Civi::log()->info("Recipient email missing: $campId"); | ||
| } | ||
| $from = HelperService::getDefaultFromEmail(); | ||
| $mailParams = [ | ||
| 'subject' => 'Collection Camp Notification: ' . $campCode . ' at ' . $campAddress, | ||
| 'from' => $from, | ||
| 'toEmail' => $recipientEmail, | ||
| 'replyTo' => $from, | ||
| 'html' => self::getLogisticsEmailHtml($recipientName, $campId, $campAttendedById, $campOffice, $campCode, $campAddress, $pocEmail, $pocContactNumber, $nameOfInstitution, $addressOfInstitution), | ||
| ]; | ||
|
|
||
| // Send logistics email. | ||
| $emailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($emailSendResult) { | ||
| \Civi::log()->info("Logistics email sent for collection camp: $campId"); | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Institution_Collection_Camp_Logistics.Email_Sent', 1) | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| else { | ||
| $recipient = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $institutionPOCId) | ||
| ->execute()->single(); | ||
|
|
||
| $recipientEmail = $recipient['email.email']; | ||
| $recipientName = $recipient['display_name']; | ||
|
|
||
| if (!$recipientEmail) { | ||
| \Civi::log()->info("Recipient email missing for institution POC: $campId"); | ||
| return; | ||
| } | ||
|
|
||
| $from = HelperService::getDefaultFromEmail(); | ||
| $mailParams = [ | ||
| 'subject' => 'Dispatch Notification for Self Managed Camp: ' . $campCode, | ||
| 'from' => $from, | ||
| 'toEmail' => $recipientEmail, | ||
| 'replyTo' => $from, | ||
| 'html' => self::sendDispatchEmail($recipientName, $campId, $coordinatingPOCId, $campOffice, $campCode, $campAddress, $pocEmail, $pocContactNumber, $nameOfInstitution, $addressOfInstitution), | ||
| ]; | ||
|
|
||
| $dispatchEmailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
|
|
||
| if ($dispatchEmailSendResult) { | ||
| \Civi::log()->info("dispatch email sent for collection camp: $campId"); | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Institution_Collection_Camp_Logistics.Email_Sent', 1) | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
|
|
||
| $recipient = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $coordinatingPOCId) | ||
| ->execute()->single(); | ||
|
|
||
| $recipientEmail = $recipient['email.email']; | ||
| $recipientName = $recipient['display_name']; | ||
|
|
||
| if (!$recipientEmail) { | ||
| \Civi::log()->info("Recipient email missing for coordinating POC': $campId"); | ||
| return; | ||
| } | ||
|
|
||
| $mailParams = [ | ||
| 'subject' => 'Outcome Notification for Self Managed Camp: ' . $campCode, | ||
| 'from' => $from, | ||
| 'toEmail' => $recipientEmail, | ||
| 'replyTo' => $from, | ||
| 'html' => self::sendOutcomeEmail($recipientName, $campId, $coordinatingPOCId, $campCode, $campAddress,), | ||
| ]; | ||
|
|
||
| $outcomeEmailSendResult = \CRM_Utils_Mail::send($mailParams); | ||
| if ($outcomeEmailSendResult) { | ||
| \Civi::log()->info("dispatch email sent for collection camp: $campId"); | ||
| EckEntity::update('Collection_Camp', FALSE) | ||
| ->addValue('Institution_Collection_Camp_Logistics.Email_Sent', 1) | ||
| ->addWhere('id', '=', $campId) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| catch (\Exception $e) { | ||
| \Civi::log()->error("Error in sendLogisticsEmail for $campId: " . $e->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method 'sendLogisticsEmail' Is Too Long and Complex
The sendLogisticsEmail method is lengthy with deeply nested conditionals, making it difficult to read and maintain.
Refactor the method by:
- Extracting logical blocks into separate methods (e.g., email preparation, contact retrieval).
- Reducing nesting by returning early where possible.
- Using guard clauses to simplify conditional logic.
This will enhance clarity and adhere to the Single Responsibility Principle.
🛠️ Refactor suggestion
Single Responsibility Principle Violation in 'sendLogisticsEmail'
This method handles multiple tasks including data retrieval, email content generation, and sending emails.
Split the method into smaller, focused methods:
- One for retrieving data.
- One for preparing email content.
- One for sending emails.
This adheres to the Single Responsibility Principle and improves code clarity.
| /** | ||
| * | ||
| */ | ||
| class IntitutionGoonjActivitiesService extends AutoSubscriber { |
There was a problem hiding this comment.
Typographical Error in Class Name
The class name IntitutionGoonjActivitiesService appears to be misspelled. Correcting it to InstitutionGoonjActivitiesService will improve code readability and prevent potential issues.
Apply this diff to fix the class name:
-class IntitutionGoonjActivitiesService extends AutoSubscriber {
+class InstitutionGoonjActivitiesService extends AutoSubscriber {📝 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.
| class IntitutionGoonjActivitiesService extends AutoSubscriber { | |
| class InstitutionGoonjActivitiesService extends AutoSubscriber { |
| 'is_primary' => 1, | ||
| ]; | ||
| self::$addressAdded = TRUE; | ||
| \Civi::log()->info('instituteGoonjActivitiesA12312312ddress', ['instit21212uteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress, 'institutePocAddress' => self::$institutePocAddress]); |
There was a problem hiding this comment.
Garbled Text in Log Message
The log message contains typographical errors and seems to include unintended characters.
Apply this diff to correct the log message:
- \Civi::log()->info('instituteGoonjActivitiesA12312312ddress', ['instit21212uteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress, 'institutePocAddress' => self::$institutePocAddress]);
+ \Civi::log()->info('instituteGoonjActivitiesAddress', ['instituteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress, 'institutePocAddress' => self::$institutePocAddress]);📝 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.
| \Civi::log()->info('instituteGoonjActivitiesA12312312ddress', ['instit21212uteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress, 'institutePocAddress' => self::$institutePocAddress]); | |
| \Civi::log()->info('instituteGoonjActivitiesAddress', ['instituteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress, 'institutePocAddress' => self::$institutePocAddress]); |
| $relationshipTypeName = 'Other Entities Coordinator of'; | ||
| } | ||
| \Civi::log()->info('relationshipTypeName', ['relationshipTypeName' => $relationshipTypeName, '------', 'stateOfficeId' => $stateOfficeId]); | ||
|
|
There was a problem hiding this comment.
Remove Debug Logging Statements from Production Code
The logging statement includes detailed internal data, which may not be suitable for production environments.
Apply this diff to remove or adjust the logging statement:
- \Civi::log()->info('registrationCategorySelection', ['registrationCategorySelection' => $registrationCategorySelection, 'relationshipTypeMap' => $relationshipTypeMap]);If logging is necessary, ensure it's appropriately leveled and sanitized.
Committable suggestion skipped: line range outside the PR's diff.
| '&hook_civicrm_pre' => [ | ||
| ['assignChapterGroupToIndividual'], | ||
| ['generateInstitutionCollectionCampQr'], | ||
| ], | ||
| 'civi.afform.submit' => [ | ||
| ['setGoonjActivitiesAddress', 10], | ||
| ['setActivitiesVolunteersAddress', 20], | ||
| ], | ||
| '&hook_civicrm_custom' => [ | ||
| ['setOfficeDetails'], | ||
| ], | ||
| '&hook_civicrm_pre' => [ | ||
| ['createActivityForGoonjActivityCollectionCamp'], | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate Event Subscription for 'hook_civicrm_pre'
The getSubscribedEvents method subscribes to &hook_civicrm_pre twice, which may lead to confusion or unintended behavior.
Combine the event handlers under a single subscription:
return [
'&hook_civicrm_pre' => [
['assignChapterGroupToIndividual'],
['generateInstitutionCollectionCampQr'],
['createActivityForGoonjActivityCollectionCamp'],
],
// Other event subscriptions...
];Committable suggestion skipped: line range outside the PR's diff.
| \Civi::log()->info('joined', ['event2' => $joins, 'contacinstituteGoonjActivitiesAddresst' => $instituteGoonjActivitiesAddress]); | ||
| // Add if not present. |
There was a problem hiding this comment.
Misspelled Variable Names in Logging
Variable names in the logging statements are misspelled, which can lead to confusion during debugging.
Apply this diff to correct the variable names:
- \Civi::log()->info('joined', ['event2' => $joins, 'contacinstituteGoonjActivitiesAddresst' => $instituteGoonjActivitiesAddress]);
+ \Civi::log()->info('joined', ['event2' => $joins, 'instituteGoonjActivitiesAddress' => self::$instituteGoonjActivitiesAddress]);Committable suggestion skipped: line range outside the PR's diff.
| @@ -15,6 +15,7 @@ | |||
| 'Disaster Exhibition' => 'DE', | |||
| 'Dropping Center' => 'DC', | |||
| 'Goonj Activities' => 'GA', | |||
| 'Institution Goonj Activities' => 'GA', | |||
There was a problem hiding this comment.
Potential ambiguity with duplicate event code 'GA'
The newly added 'Institution Goonj Activities' uses the same code 'GA' as the existing 'Goonj Activities' entry. This could cause confusion or issues in event identification and reporting. Consider using a distinct code for institutional activities, such as 'IGA'.
Apply this diff to use a unique code:
- 'Institution Goonj Activities' => 'GA',
+ 'Institution Goonj Activities' => 'IGA',📝 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.
| 'Institution Goonj Activities' => 'GA', | |
| 'Institution Goonj Activities' => 'IGA', |
Summary by CodeRabbit
New Features
Institute_Goonj_Activitiesentity to enhance data processing.Bug Fixes