Initial Draft: Institution Dropping Center Implementation#528
Initial Draft: Institution Dropping Center Implementation#528
Conversation
WalkthroughThe changes in this pull request introduce a new class, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (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: 4
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
12-14: Provide meaningful documentation for class and methodsThe class and several methods have empty docblocks. Adding concise and descriptive documentation will improve code readability and maintainability.
Also applies to: 17-20, 24-26, 36-38, 73-75, 85-87, 110-112
113-150: Improve code modularity by extracting logic into separate methodsThe
setOfficeDetailsmethod is handling multiple responsibilities, making it lengthy and harder to maintain. Consider extracting the logic for finding the state office and updating the entity into separate private methods. This will enhance readability and adhere to the single responsibility principle.
| $intentStateFields = CustomField::get(FALSE) | ||
| ->addWhere('custom_group_id:name', '=', 'Institution_Dropping_Center_Intent') | ||
| ->addWhere('name', '=', 'State') | ||
| ->execute(); | ||
|
|
||
| $stateField = $intentStateFields->first(); | ||
|
|
||
| $statefieldId = $stateField['id']; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code for retrieving state field ID
The logic to retrieve the state field ID is duplicated in both setIndianStateOptions and findStateField methods. Consider extracting this logic into a dedicated private method to enhance code reusability and maintainability.
Also applies to: 89-94
| $stateField = $intentStateFields->first(); | ||
|
|
||
| $statefieldId = $stateField['id']; |
There was a problem hiding this comment.
Add null check for $stateField before accessing its id
If no state field is found, $stateField may be null, leading to an undefined index error when accessing $stateField['id']. Add a check to ensure $stateField is not null before accessing its properties.
Apply this diff to fix the issue:
$stateField = $intentStateFields->first();
+if (!$stateField) {
+ return;
+}
$statefieldId = $stateField['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.
| $stateField = $intentStateFields->first(); | |
| $statefieldId = $stateField['id']; | |
| $stateField = $intentStateFields->first(); | |
| if (!$stateField) { | |
| return; | |
| } | |
| $statefieldId = $stateField['id']; |
| if (!$stateOffice) { | ||
| $stateOffice = self::getFallbackOffice(); | ||
| } | ||
|
|
||
| $stateOfficeId = $stateOffice['id']; | ||
|
|
There was a problem hiding this comment.
Ensure $stateOffice is not null before accessing its id
After attempting to get the fallback office, $stateOffice may still be null if no office was found. Accessing $stateOffice['id'] without checking could lead to errors. Add a null check before using $stateOffice.
Apply this diff to fix the issue:
// If no state office is found, assign the fallback state office.
if (!$stateOffice) {
$stateOffice = self::getFallbackOffice();
}
+if (!$stateOffice) {
+ \CRM_Core_Error::debug_log_message('No office found, unable to assign Goonj Office.');
+ return false;
+}
$stateOfficeId = $stateOffice['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.
| if (!$stateOffice) { | |
| $stateOffice = self::getFallbackOffice(); | |
| } | |
| $stateOfficeId = $stateOffice['id']; | |
| if (!$stateOffice) { | |
| $stateOffice = self::getFallbackOffice(); | |
| } | |
| if (!$stateOffice) { | |
| \CRM_Core_Error::debug_log_message('No office found, unable to assign Goonj Office.'); | |
| return false; | |
| } | |
| $stateOfficeId = $stateOffice['id']; |
| private static function getFallbackOffice() { | ||
| $fallbackOffices = Contact::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('organization_name', 'CONTAINS', self::FALLBACK_OFFICE_NAME) | ||
| ->execute(); | ||
|
|
||
| return $fallbackOffices->first(); | ||
| } |
There was a problem hiding this comment.
Handle case when fallback office is not found
The method getFallbackOffice may return null if no office matches the fallback office name. Accessing properties of a null value can lead to errors. Ensure that you handle the scenario where no fallback office is found.
Apply this diff to address the issue:
return $fallbackOffices->first();
+if (!$fallbackOffices->first()) {
+ \CRM_Core_Error::debug_log_message('Fallback office not found.');
+ return null;
+}
return $fallbackOffices->first();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/config/eventCode.php (1)
Line range hint
1-30: Consider externalizing the event code configurationThe event codes are currently hard-coded in the PHP file. This makes it difficult to manage and update configurations across different environments.
Consider:
- Moving these configurations to a database table or external configuration file
- Implementing a consistent pattern for code generation (e.g., always use 2-3 letter codes)
- Adding validation to ensure code uniqueness
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
Line range hint
1-1000: Class violates Single Responsibility PrincipleThe
CollectionCampServiceclass has grown too large and handles multiple responsibilities including:
- Email notifications
- QR code generation
- Status management
- Activity creation
- Form handling
- Address management
Consider breaking this class into smaller, focused service classes:
CollectionCampEmailServiceCollectionCampQRCodeServiceCollectionCampStatusServiceCollectionCampActivityServiceCollectionCampFormServiceCollectionCampAddressService
Line range hint
472-520: Refactor getStateIdForSubtype methodThe method has grown complex with multiple similar conditions. This pattern suggests a need for refactoring.
Consider using a mapping approach:
private static $STATE_FIELD_MAPPING = [ 'Institution Collection Camp' => 'Institution_Collection_Camp_Intent.State', 'Goonj Activities' => 'Goonj_Activities.State', 'Institution Dropping Center' => 'Institution_Dropping_Center_Intent.State', 'Dropping Center' => 'Dropping_Centre.State' ]; public static function getStateIdForSubtype(array $objectRef, int $subtypeId, ?string $campTitle): ?int { if (!$campTitle || !isset(self::$STATE_FIELD_MAPPING[$campTitle])) { return $objectRef['Collection_Camp_Intent_Details.State'] ?? NULL; } $optionValue = self::getOptionValue(str_replace(' ', '_', $campTitle)); if ($subtypeId == $optionValue['value']) { return $objectRef[self::$STATE_FIELD_MAPPING[$campTitle]] ?? NULL; } return NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/config/eventCode.php(1 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
490-495:
New code block lacks error handling
The state ID retrieval for 'Institution Dropping Center' doesn't handle the case where Institution_Dropping_Center_Intent.State is undefined.
Add null checks and error handling:
if ($campTitle === 'Institution Dropping Center') {
$institutionDroppingCenterOptionValue = self::getOptionValue('Institution_Dropping_Center');
if ($subtypeId == $institutionDroppingCenterOptionValue['value']) {
+ if (!isset($objectRef['Institution_Dropping_Center_Intent.State'])) {
+ \CRM_Core_Error::debug_log_message('State not set for Institution Dropping Center: ' . $objectRef['id']);
+ return NULL;
+ }
return $objectRef['Institution_Dropping_Center_Intent.State'] ?? NULL;
}
}Likely invalid or redundant comment.
Line range hint 32-32: Verify the impact of new Institution Dropping Center code
The addition of Institution Dropping Center handling might affect existing code paths.
Let's verify the usage and impact:
Also applies to: 490-495
✅ Verification successful
Let me gather more context about the specific implementation to understand the impact better.
No impact concerns with Institution Dropping Center implementation
The implementation follows the existing pattern used for other camp types (Institution Collection Camp, Goonj Activities) and properly handles state information through dedicated fields. The code:
- Uses consistent state handling via
getStateIdForSubtype - Maintains separation between different camp types
- Follows the established pattern for state field access
- Has proper integration in the event code configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to Institution Dropping Center in the codebase
# Check for references to the new state field
rg "Institution_Dropping_Center_Intent.State"
# Check for references to the new camp type
rg "Institution Dropping Center"
# Check for any potential impact on existing dropping centers
rg "Dropping_Centre.State"
Length of output: 1240
Script:
#!/bin/bash
# Check the implementation details around Institution Dropping Center handling
ast-grep --pattern 'if ($campTitle === "Institution Dropping Center") {
$$$
}'
# Check for any state-related processing in CollectionCampService
rg -A 5 "getState" wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
# Look for any other camp type handling
rg -A 2 "campTitle === " wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
Length of output: 10739
| 'Chuppi Todo Baithak-NJPC' => 'CTB', | ||
| 'Collection Camp' => 'CC', | ||
| 'Institution Collection Camp' => 'CC', | ||
| 'Institution Dropping Center' => 'DC', |
There was a problem hiding this comment.
Avoid duplicate event codes
The code 'DC' is used for both 'Institution Dropping Center' and 'Dropping Center'. This could lead to ambiguity in identifying the source of events.
Consider using a unique code for 'Institution Dropping Center', such as 'IDC'.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
187-197: Improve single responsibility by separating QR code generationThe method
generateInstitutionDroppingCenterQrCodeis responsible for constructing the URL and generating the QR code. To adhere to the Single Responsibility Principle, consider separating the URL construction and QR code generation into different methods or classes. This separation enhances code maintainability and readability.
16-199: Add method-level documentation for clarityThe methods lack descriptive docblocks, which can hinder understanding and maintenance of the code. Consider adding meaningful documentation to each method to explain their purpose and usage.
Apply this change to add documentation:
/** - * + * Class responsible for managing Institution Dropping Center services. */ class InstitutionDroppingCenterService extends AutoSubscriber { + /** + * Get the events to which the service should subscribe. + * + * @return array + */ public static function getSubscribedEvents() { + /** + * Populate state options for Institution Dropping Center. + * + * @param string $entity + * @param string $field + * @param array &$options + * @param array $params + */ public static function setIndianStateOptions(string $entity, string $field, array &$options, array $params) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(0 hunks)
💤 Files with no reviewable changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (4)
48-56: 🛠️ Refactor suggestion
Refactor duplicate code for retrieving state field ID
The logic to retrieve the state field ID is duplicated in both setIndianStateOptions (lines 48-56) and findStateField (lines 92-99) methods. Consider extracting this logic into a dedicated private method to enhance code reusability and maintainability.
Also applies to: 92-99
53-55:
Add null check for $stateField before accessing its id
If no state field is found, $stateField may be null, leading to an undefined index error when accessing $stateField['id'] at line 55. Add a check to ensure $stateField is not null before accessing its properties.
Apply this diff to fix the issue:
$stateField = $intentStateFields->first();
+if (!$stateField) {
+ return;
+}
$statefieldId = $stateField['id'];80-87:
Handle case when fallback office is not found
The method getFallbackOffice may return null if no office matches the fallback office name. Accessing properties of a null value can lead to errors in methods that rely on this return value. Ensure that you handle the scenario where no fallback office is found.
Apply this diff to address the issue:
$fallbackOffice = $fallbackOffices->first();
+if (!$fallbackOffice) {
+ \CRM_Core_Error::debug_log_message('Fallback office not found.');
+ return null;
+}
return $fallbackOffice;145-149:
Ensure $stateOffice is not null before accessing its id
After attempting to get the fallback office, $stateOffice may still be null if no office was found. Accessing $stateOffice['id'] without checking could lead to errors. Add a null check before using $stateOffice.
Apply this diff to fix the issue:
// If no state office is found, assign the fallback state office.
if (!$stateOffice) {
$stateOffice = self::getFallbackOffice();
}
+if (!$stateOffice) {
+ \CRM_Core_Error::debug_log_message('No office found, unable to assign Goonj Office.');
+ return false;
+}
$stateOfficeId = $stateOffice['id'];| if (!($stateField = self::findStateField($params))) { | ||
| return; | ||
| } | ||
|
|
||
| $stateId = $stateField['value']; | ||
| $institutionDroppingCenterId = $stateField['entity_id']; |
There was a problem hiding this comment.
Check for missing value key in $stateField
When accessing $stateField['value'] at line 126, ensure that the 'value' key exists to prevent possible undefined index errors. Add a check to confirm that $stateField['value'] is set before using it.
Apply this diff to fix the issue:
$stateId = $stateField['value'];
+if (!$stateId) {
+ \CRM_Core_Error::debug_log_message('State value not found in state field.');
+ return false;
+}Committable suggestion skipped: line range outside the PR's diff.
pokhiii
force-pushed
the
feat/institution-dropping-center-implementation
branch
from
407394f to
4ba0ef9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
206-215: Add error handling for file inclusion ingetConfigThe
getConfigmethod includes files without checking if they exist, which could lead to warnings or errors if the files are missing.Implement a check to verify that the files exist before including them:
$stateCodesPath = $extensionPath . 'constants.php'; $eventCodesPath = $extensionPath . 'eventCode.php'; if (!file_exists($stateCodesPath) || !file_exists($eventCodesPath)) { // Handle the error, e.g., log a message or throw an exception \Civi::log()->error('Configuration files missing in getConfig method.'); return null; } return [ 'state_codes' => include $stateCodesPath, 'event_codes' => include $eventCodesPath, ];This will prevent unexpected errors and improve the reliability of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(0 hunks)wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(1 hunks)wp-content/civi-extensions/goonjcustom/config/eventCode.php(1 hunks)
💤 Files with no reviewable changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
🚧 Files skipped from review as they are similar to previous changes (2)
- wp-content/civi-extensions/goonjcustom/config/eventCode.php
- wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (1)
866-867: LGTM!
The formatting changes look good and maintain consistency with the rest of the code.
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (1)
40-40: Ensure generateCollectionSourceCode method is appropriately integrated
The generateCollectionSourceCode method is added to the hook_civicrm_pre event subscribers. Verify that this method is correctly defined and accessible via the CollectionSource trait used in CollectionBaseService.
This addition seems appropriate and aligns with extending functionality using traits.
| // Check if the result has any rows. | ||
| if ($officeContact->count() > 0) { | ||
| // Extract the first row (assuming one result, based on rowCount => 1) | ||
| $officeContactData = $officeContact->first(); | ||
| // Add the primary city to the array | ||
| if (!empty($officeContactData['address_primary.city'])) { | ||
| $officeContactInductionCities[] = $officeContactData['address_primary.city']; | ||
| } | ||
| // Add the other induction cities to the array | ||
| if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) { | ||
| // Split the string into an array and merge it | ||
| $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); | ||
| $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); | ||
| } | ||
| // Convert all cities to lowercase, remove duplicates, and re-index the array | ||
| $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities); | ||
| $officeContactInductionCities = array_unique($officeContactInductionCities); | ||
| $officeContactInductionCities = array_values($officeContactInductionCities); | ||
| // Extract the first row (assuming one result, based on rowCount => 1) | ||
| $officeContactData = $officeContact->first(); | ||
|
|
||
| // Add the primary city to the array. | ||
| if (!empty($officeContactData['address_primary.city'])) { | ||
| $officeContactInductionCities[] = $officeContactData['address_primary.city']; | ||
| } | ||
|
|
||
| // Add the other induction cities to the array. | ||
| if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) { | ||
| // Split the string into an array and merge it. | ||
| $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); | ||
| $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); | ||
| } | ||
|
|
||
| // Convert all cities to lowercase, remove duplicates, and re-index the array. | ||
| $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities); | ||
| $officeContactInductionCities = array_unique($officeContactInductionCities); | ||
| $officeContactInductionCities = array_values($officeContactInductionCities); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and code organization
While the changes improve the handling of office contact data, there are a few suggestions for better maintainability and robustness:
- Extract city processing logic into a separate method for better readability and reusability:
- // Check if the result has any rows.
- if ($officeContact->count() > 0) {
- // Extract the first row (assuming one result, based on rowCount => 1)
- $officeContactData = $officeContact->first();
-
- // Add the primary city to the array.
- if (!empty($officeContactData['address_primary.city'])) {
- $officeContactInductionCities[] = $officeContactData['address_primary.city'];
- }
-
- // Add the other induction cities to the array.
- if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) {
- // Split the string into an array and merge it.
- $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities']));
- $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities);
- }
-
- // Convert all cities to lowercase, remove duplicates, and re-index the array.
- $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities);
- $officeContactInductionCities = array_unique($officeContactInductionCities);
- $officeContactInductionCities = array_values($officeContactInductionCities);
- }
+ if ($officeContact->count() > 0) {
+ $officeContactInductionCities = self::processInductionCities($officeContact->first());
+ }
+ private static function processInductionCities($officeContactData) {
+ $cities = [];
+
+ // Add primary city if available
+ if (!empty($officeContactData['address_primary.city'])) {
+ $cities[] = $officeContactData['address_primary.city'];
+ }
+
+ // Add other induction cities if available
+ if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) {
+ $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities']));
+ $cities = array_merge($cities, $otherCities);
+ }
+
+ // Normalize cities: convert to lowercase, remove duplicates, and re-index
+ return array_values(array_unique(array_map('strtolower', $cities)));
+ }- Add validation for the office contact data:
+ // Validate office contact data
+ if (empty($officeContactData)) {
+ \Civi::log()->warning('Invalid office contact data received', [
+ 'contact_id' => $volunteerId,
+ 'state_id' => $contactStateId
+ ]);
+ return 'Online'; // Default to online if data is invalid
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| $sourceStateFieldMapper = [ | ||
| 'Collection_Camp' => 'Collection_Camp_Intent_Details.State', | ||
| 'Dropping_Center' => 'Dropping_Centre.State', | ||
| 'Institution_Collection_Camp' => 'Institution_Collection_Camp_Intent.State', | ||
| 'Goonj_Activities' => 'Goonj_Activities.State', | ||
| 'Institution_Dropping_Center' => 'Institution_Dropping_Center_Intent.State', | ||
| ]; | ||
|
|
||
| $sourceTypeName = self::getSourceTypeName($subtypeId); | ||
|
|
||
| $stateFieldName = $sourceStateFieldMapper[$sourceTypeName]; | ||
|
|
||
| return $objectRef[$stateFieldName]; | ||
|
|
||
| } |
There was a problem hiding this comment.
Prevent potential undefined index errors in getStateIdForSourceType
In the getStateIdForSourceType method, accessing $sourceStateFieldMapper[$sourceTypeName] without checking if the key exists may lead to an undefined index error if $sourceTypeName is not present in the array.
Add a check to ensure $sourceTypeName exists in $sourceStateFieldMapper before accessing it:
if (!isset($sourceStateFieldMapper[$sourceTypeName])) {
// Handle the error or return a default value
return null;
}
$stateFieldName = $sourceStateFieldMapper[$sourceTypeName];This will prevent runtime errors and enhance code robustness.
| private static function getSourceTypeName(string $sourceTypeId): ?array { | ||
|
|
||
| $sourceType = OptionValue::get(TRUE) | ||
| ->addSelect('name') | ||
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | ||
| ->addWhere('grouping', '=', 'Collection_Camp') | ||
| ->addWhere('value', '=', $sourceTypeId) | ||
| ->single(); | ||
| return $sourceType['name']; | ||
| } |
There was a problem hiding this comment.
Correct the return type declaration in getSourceTypeName
The getSourceTypeName method is declared to return ?array, but it returns $sourceType['name'], which is a string. This mismatch can cause confusion and potential type errors.
Update the return type to ?string to reflect the actual return value:
-private static function getSourceTypeName(string $sourceTypeId): ?array {
+private static function getSourceTypeName(string $sourceTypeId): ?string {This ensures consistency between the method's declaration and its return value.
📝 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.
| private static function getSourceTypeName(string $sourceTypeId): ?array { | |
| $sourceType = OptionValue::get(TRUE) | |
| ->addSelect('name') | |
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | |
| ->addWhere('grouping', '=', 'Collection_Camp') | |
| ->addWhere('value', '=', $sourceTypeId) | |
| ->single(); | |
| return $sourceType['name']; | |
| } | |
| private static function getSourceTypeName(string $sourceTypeId): ?string { | |
| $sourceType = OptionValue::get(TRUE) | |
| ->addSelect('name') | |
| ->addWhere('option_group_id:name', '=', 'eck_sub_types') | |
| ->addWhere('grouping', '=', 'Collection_Camp') | |
| ->addWhere('value', '=', $sourceTypeId) | |
| ->single(); | |
| return $sourceType['name']; | |
| } |
pokhiii
left a comment
pokhiii
left a comment
There was a problem hiding this comment.
Please test the cases Ive shared in the chat. Make any updates for changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
181-203: Add type hints to improve code safetyThe method has good structure but could benefit from stronger type hints.
Consider adding type hints:
- public static function checkCampStatusAndIds(string $objectName, $objectId, &$objectRef) { + public static function checkCampStatusAndIds( + string $objectName, + int $objectId, + array &$objectRef + ): ?array {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(2 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (2)
222-238: 🛠️ Refactor suggestion
Move field mapping to configuration and add error handling
The method has two issues:
- No validation for array access (previous review still applies)
- Hard-coded field mappings that should be in configuration
Consider this improvement:
public static function getStateIdForSourceType(array $objectRef, int $subtypeId, ?string $campTitle): ?int {
- $sourceStateFieldMapper = [
- 'Collection_Camp' => 'Collection_Camp_Intent_Details.State',
- // ... other mappings
- ];
+ $config = self::getConfig();
+ $sourceStateFieldMapper = $config['source_state_field_mapper'] ?? [];
$sourceTypeName = self::getSourceTypeName($subtypeId);
+ if (!$sourceTypeName || !isset($sourceStateFieldMapper[$sourceTypeName])) {
+ throw new \InvalidArgumentException("Invalid source type: $sourceTypeName");
+ }
$stateFieldName = $sourceStateFieldMapper[$sourceTypeName];
+ if (!isset($objectRef[$stateFieldName])) {
+ throw new \InvalidArgumentException("Missing state field: $stateFieldName");
+ }
return $objectRef[$stateFieldName];
}Likely invalid or redundant comment.
243-253: Verify permission requirements for option value retrieval
The method uses checkPermissions => TRUE which might be more permissive than necessary.
Let's check if this method is used in any public-facing contexts:
✅ Verification successful
Permission bypass is acceptable in this context
The getSourceTypeName method is:
- Used privately within the
CollectionSourcetrait - Only called by
getStateIdForSourceTypewhich is part of internal service logic - All services using this trait extend
AutoSubscriber, suggesting a controlled access pattern - The queried data (
eck_sub_types) appears to be configuration/metadata rather than sensitive data
Since this is an internal utility method accessing static configuration data, bypassing permissions with checkPermissions => TRUE is a reasonable optimization that doesn't introduce security risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to getSourceTypeName
rg -l "getSourceTypeName" --type php
# Search for routes or endpoints that might expose this functionality
rg -l "api4.*OptionValue.*get" --type php
Length of output: 152
Script:
#!/bin/bash
# Let's check the usage context of this method within the trait
rg -A 10 -B 10 "getSourceTypeName" wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php
# Let's also check if this trait is used in any public-facing contexts
rg -l "use.*CollectionSource" --type php
# Check the class/trait definition to understand access levels
rg "trait CollectionSource" -A 5 wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php
Length of output: 1937
Script:
#!/bin/bash
# Let's check if these services are exposed via API endpoints
rg -A 5 "class (InstitutionDroppingCenterService|DroppingCenterService|CollectionBaseService|CollectionCampService|GoonjActivitiesService|InstitutionService|InstitutionCollectionCampService)" --type php
# Check if getStateIdForSourceType (which uses our method) is called from public contexts
rg "getStateIdForSourceType" -A 5 -B 5 --type php
# Check if the option group being queried has any access restrictions
rg "eck_sub_types.*Collection_Camp" --type php
Length of output: 6937
| private static function getConfig() { | ||
| $extensionsDir = \CRM_Core_Config::singleton()->extensionsDir; | ||
|
|
||
| $extensionPath = $extensionsDir . 'goonjcustom/config/'; | ||
|
|
||
| return [ | ||
| 'state_codes' => include $extensionPath . 'constants.php', | ||
| 'event_codes' => include $extensionPath . 'eventCode.php', | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Add error handling for configuration file loading
The current implementation might fail silently if configuration files are missing or inaccessible.
Add proper error handling:
private static function getConfig() {
$extensionsDir = \CRM_Core_Config::singleton()->extensionsDir;
$extensionPath = $extensionsDir . 'goonjcustom/config/';
+ $stateCodesFile = $extensionPath . 'constants.php';
+ $eventCodesFile = $extensionPath . 'eventCode.php';
+
+ if (!file_exists($stateCodesFile) || !file_exists($eventCodesFile)) {
+ throw new \RuntimeException('Required configuration files are missing');
+ }
+
return [
- 'state_codes' => include $extensionPath . 'constants.php',
- 'event_codes' => include $extensionPath . 'eventCode.php',
+ 'state_codes' => require $stateCodesFile,
+ 'event_codes' => require $eventCodesFile,
];
}📝 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.
| private static function getConfig() { | |
| $extensionsDir = \CRM_Core_Config::singleton()->extensionsDir; | |
| $extensionPath = $extensionsDir . 'goonjcustom/config/'; | |
| return [ | |
| 'state_codes' => include $extensionPath . 'constants.php', | |
| 'event_codes' => include $extensionPath . 'eventCode.php', | |
| ]; | |
| } | |
| private static function getConfig() { | |
| $extensionsDir = \CRM_Core_Config::singleton()->extensionsDir; | |
| $extensionPath = $extensionsDir . 'goonjcustom/config/'; | |
| $stateCodesFile = $extensionPath . 'constants.php'; | |
| $eventCodesFile = $extensionPath . 'eventCode.php'; | |
| if (!file_exists($stateCodesFile) || !file_exists($eventCodesFile)) { | |
| throw new \RuntimeException('Required configuration files are missing'); | |
| } | |
| return [ | |
| 'state_codes' => require $stateCodesFile, | |
| 'event_codes' => require $eventCodesFile, | |
| ]; | |
| } |
| public static function generateCollectionSourceCode(string $op, string $objectName, $objectId, &$objectRef) { | ||
| $statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef); | ||
|
|
||
| if (!$statusDetails) { | ||
| return; | ||
| } | ||
|
|
||
| $newStatus = $statusDetails['newStatus']; | ||
| $currentStatus = $statusDetails['currentStatus']; | ||
|
|
||
| if ($currentStatus !== $newStatus) { | ||
| if ($newStatus === 'authorized') { | ||
| $subtypeId = $objectRef['subtype'] ?? NULL; | ||
| if (!$subtypeId) { | ||
| return; | ||
| } | ||
|
|
||
| $sourceId = $objectRef['id'] ?? NULL; | ||
| if (!$sourceId) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionSource = EckEntity::get('Collection_Camp', FALSE) | ||
| ->addWhere('id', '=', $sourceId) | ||
| ->execute()->single(); | ||
|
|
||
| $collectionSourceCreatedDate = $collectionSource['created_date'] ?? NULL; | ||
|
|
||
| $sourceTitle = $collectionSource['title'] ?? NULL; | ||
|
|
||
| $year = date('Y', strtotime($collectionSourceCreatedDate)); | ||
|
|
||
| $stateId = self::getStateIdForSourceType($objectRef, $subtypeId, $sourceTitle); | ||
|
|
||
| if (!$stateId) { | ||
| return; | ||
| } | ||
|
|
||
| $stateProvince = StateProvince::get(FALSE) | ||
| ->addWhere('id', '=', $stateId) | ||
| ->execute()->single(); | ||
|
|
||
| if (empty($stateProvince)) { | ||
| return; | ||
| } | ||
|
|
||
| $stateAbbreviation = $stateProvince['abbreviation'] ?? NULL; | ||
| if (!$stateAbbreviation) { | ||
| return; | ||
| } | ||
|
|
||
| // Fetch the Goonj-specific state code. | ||
| $config = self::getConfig(); | ||
| $stateCode = $config['state_codes'][$stateAbbreviation] ?? 'UNKNOWN'; | ||
|
|
||
| // Get the current event title. | ||
| $currentTitle = $objectRef['title'] ?? 'Collection Camp'; | ||
|
|
||
| // Fetch the event code. | ||
| $eventCode = $config['event_codes'][$currentTitle] ?? 'UNKNOWN'; | ||
|
|
||
| // Modify the title to include the year, state code, event code, and camp Id. | ||
| $newTitle = $year . '/' . $stateCode . '/' . $eventCode . '/' . $sourceId; | ||
| $objectRef['title'] = $newTitle; | ||
|
|
||
| // Save the updated title back to the Collection Camp entity. | ||
| EckEntity::update('Collection_Camp') | ||
| ->addWhere('id', '=', $sourceId) | ||
| ->addValue('title', $newTitle) | ||
| ->execute(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor long method to improve maintainability
The generateCollectionSourceCode method is too complex and handles multiple responsibilities:
- Status validation
- Data retrieval from multiple sources
- Title generation logic
- Database updates
Consider breaking this down into smaller, focused methods:
public static function generateCollectionSourceCode(string $op, string $objectName, $objectId, &$objectRef) {
- // Current implementation
+ $statusDetails = self::checkCampStatusAndIds($objectName, $objectId, $objectRef);
+ if (!$statusDetails || !self::isStatusTransitionValid($statusDetails)) {
+ return;
+ }
+
+ if ($statusDetails['newStatus'] === 'authorized') {
+ $sourceData = self::validateAndGetSourceData($objectRef);
+ if (!$sourceData) {
+ return;
+ }
+
+ $titleComponents = self::generateTitleComponents($sourceData);
+ if (!$titleComponents) {
+ return;
+ }
+
+ $newTitle = self::formatTitle($titleComponents);
+ self::updateSourceTitle($objectRef, $sourceData['sourceId'], $newTitle);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
belwalshubham
changed the title
2 participants
Description
hook_civicrm_fieldOptionshook to display Indian states in the state field.hook_civicrm_customhook to assign the Goonj office based on the state ID.hook_civicrm_prehook to generate the QR codeSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation