Conversation
WalkthroughThis pull request introduces a new class 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: 5
🧹 Outside diff range and nitpick comments (7)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (2)
Line range hint
19-28: Update function documentation to reflect new token handlerThe hook documentation should be updated to mention the InstitutionCollectionCamp token handler in the function's docblock.
/** * Implements hook_civicrm_config(). * + * Registers token handlers for: + * - Collection Camp + * - Dropping Center + * - Institution Collection Camp * * @link https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_config/ */
Line range hint
71-189: Consider refactoring token evaluation for better maintainabilityThe
goonjcustom_evaluate_tokensfunction has grown quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods:+ /** + * Service class for handling token evaluation + */ + class CRM_Goonjcustom_TokenEvaluationService { + /** + * Process induction center details + */ + public function getInductionCenterDetails($contactId) { + // Extract processing center logic here + } + + /** + * Process induction activity details + */ + public function getInductionActivityDetails($contactId) { + // Extract induction activity logic here + } + + /** + * Process office details + */ + public function getOfficeDetails($inductionGoonjOffice) { + // Extract office details logic here + } + } function goonjcustom_evaluate_tokens(TokenValueEvent $e) { $service = new CRM_Goonjcustom_TokenEvaluationService(); foreach ($e->getRows() as $row) { // Delegate to service methods } }This would:
- Improve testability by isolating different concerns
- Make the code more maintainable
- Make it easier to add new token processing logic
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionCollectionCamp.php (3)
3-6: Add meaningful documentation for the class and methodsThe docblocks for the class and methods are currently empty or minimal. Providing detailed documentation enhances code readability and maintainability.
Also applies to: 14-16
58-58: Remove debug logging statementThe
error_logcall at line 58 seems to be a leftover from debugging. It's best practice to remove such statements from production code to avoid unnecessary logging.Suggested change:
- error_log("collectionSource: " . print_r($collectionSource, TRUE));
169-171: Ensure proper checking of 'phone.is_primary' fieldIn the
array_filterfunction at lines 169-171, ensure that the'phone.is_primary'field exists and is correctly checked to avoid potential notices or incorrect filtering.Suggested change:
$primaryVolunteer = array_filter($volunteersArray, function ($volunteer) { - return $volunteer['phone.is_primary']; + return isset($volunteer['phone.is_primary']) && $volunteer['phone.is_primary']; });wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php (2)
61-61: OptimizeaddSelectto include only necessary fieldsIncluding
'*'in youraddSelectcall along with specific fields may lead to unnecessary data retrieval and impact performance. Consider selecting only the required fields to improve efficiency.Apply this diff to refine the
addSelectstatement:->addSelect('*', +->addSelect( 'contact.display_name', 'Material_Contribution.Delivered_By', 'Material_Contribution.Delivered_By_Contact', 'Material_Contribution.Goonj_Office', 'Material_Contribution.Collection_Camp.subtype:name', 'Material_Contribution.Institution_Collection_Camp.subtype:name', 'Material_Contribution.Dropping_Center.subtype:name')
74-84: Adhere to Single Responsibility PrincipleThe methods
attachContributionReceiptToEmailandgetContributionCityhandle multiple responsibilities, including data fetching, business logic, and logging. Refactoring these methods to separate concerns will enhance code maintainability.Consider extracting the subtype determination and field assignments into separate methods:
private static function determineSubtype($contribution) { /* ... */ } private static function getCampField($subtype) { /* ... */ } private static function getAddressField($subtype) { /* ... */ }Also applies to: 119-125, 137-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionCollectionCamp.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/MaterialContributionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/goonjcustom.php(1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/goonjcustom.php (1)
27-27: Verify the existence of InstitutionCollectionCamp token handler
The integration follows the established pattern, but let's ensure the referenced class exists and is properly implemented.
✅ Verification successful
Token handler implementation looks solid and follows best practices
The InstitutionCollectionCamp token handler:
- Properly extends
AbstractTokenSubscriber - Follows CiviCRM naming conventions
- Has well-organized token definitions with translations
- Implements clean, focused methods for each token type
- Includes proper error handling and logging
- Uses appropriate API calls for data retrieval
The implementation maintains single responsibility principle with separate formatting methods for each token type and follows established CiviCRM token handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the token handler class file exists and follows CiviCRM conventions
fd -e php "InstitutionCollectionCamp.php"
# Check for potential token name collisions
ast-grep --pattern 'function registerTokens($e) {
$$$
}'
Length of output: 9465
Script:
#!/bin/bash
# Let's check the content of the token handler class
cat "wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionCollectionCamp.php"
# Let's also check for any similar token handlers to understand the pattern
rg -l "implements TokenSubscriberInterface" --type php
Length of output: 6392
wp-content/civi-extensions/goonjcustom/CRM/Goonjcustom/Token/InstitutionCollectionCamp.php (1)
41-42:
Syntax Error: Remove trailing comma in function declaration
The evaluateToken function has a trailing comma after the last parameter at line 41, which is invalid in PHP function declarations. Please remove the comma to fix the syntax error.
Suggested change:
$field,
- $prefetch = NULL,
+ $prefetch = NULL
) {Likely invalid or redundant comment.
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (1)
351-353: Ensure safe access to 'contact_id_b' in relationships array
Accessing $relationships[0]['contact_id_b'] without confirming the array structure may lead to undefined index errors. Ensure that $relationships is properly indexed and that 'contact_id_b' exists before accessing it. Consider using the first() method or checking the existence explicitly.
Run the following script to verify the structure of the $relationships array:
✅ Verification successful
Accessing array index is safe with current null and empty checks
The current implementation already has proper checks in place before accessing the array index:
if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) {
return $relationships[0]['contact_id_b'];
}The code follows best practices by:
- Checking if
$relationshipsis not empty using!empty() - Explicitly verifying the existence of the 'contact_id_b' key using
isset()
The codebase shows consistent usage of this pattern when dealing with relationship data, particularly in the CiviCRM core and extensions. The Relationship::get() API consistently returns relationships with 'contact_id_b' as a standard field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'contact_id_b' exists in the relationships fetched.
# Test: Fetch relationships and check for 'contact_id_b'.
php -r "
\$relationships = Civi\Api4\Relationship::get(FALSE)
->addWhere('contact_id_a', '=', $organizationId)
->addWhere('relationship_type_id:name', 'IN', ['Primary Institution POC of', 'Secondary Institution POC of'])
->execute();
if (!empty(\$relationships) && isset(\$relationships[0]['contact_id_b'])) {
echo 'contact_id_b exists.';
} else {
echo 'contact_id_b not found in relationships.';
}
"
Length of output: 552
Script:
#!/bin/bash
# Let's analyze the codebase to understand the relationship structure and usage
# Search for similar patterns of relationship access
rg -U "contact_id_b.*relationships" -A 3 -B 3
# Also check the schema or relationship type definitions
ast-grep --pattern 'contact_id_b'
# Look for similar array access patterns
rg "\\\$relationships\[.*?\]\[" -A 2 -B 2
Length of output: 9353
Script:
#!/bin/bash
# Let's try a different approach to find relationship-related code
# Search for files that might contain relationship definitions or handling
fd -e php -e xml -e json | xargs rg -l "relationship"
# Look specifically in the CiviCRM extensions directory
fd -e php . wp-content/civi-extensions/ | xargs rg -l "Relationship::"
# Search for API calls or relationship handling patterns
rg -g "*.php" "(\\\$relationships|Relationship::)" -A 2 -B 2
Length of output: 102264
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of') | ||
| ->execute(); | ||
|
|
||
| // If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'. | ||
| if (empty($relationships)) { | ||
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of') | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated relationship queries and avoid hard-coded strings
The code querying relationships for 'Primary Institution POC of' and 'Secondary Institution POC of' is duplicated. Consider refactoring to eliminate duplication and improve maintainability.
Additionally, hard-coding relationship type names may cause issues if the names change. It's better to use relationship type IDs or define constants.
Suggested refactor:
$relationships = Relationship::get(FALSE)
->addWhere('contact_id_a', '=', $organizationId)
- ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of')
- ->execute();
-// If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'.
-if (empty($relationships)) {
- $relationships = Relationship::get(FALSE)
- ->addWhere('contact_id_a', '=', $organizationId)
- ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of')
- ->execute();
-}
+ ->addWhere('relationship_type_id:name', 'IN', ['Primary Institution POC of', 'Secondary Institution POC of'])
+ ->execute();If possible, retrieve the relationship type IDs dynamically or define them as constants to avoid hard-coding.
Committable suggestion skipped: line range outside the PR's diff.
| $subtype = NULL; | ||
| if (!empty($contribution['Material_Contribution.Collection_Camp.subtype:name'])) { | ||
| $subtype = $contribution['Material_Contribution.Collection_Camp.subtype:name']; | ||
| } | ||
| elseif (!empty($contribution['Material_Contribution.Institution_Collection_Camp.subtype:name'])) { | ||
| $subtype = $contribution['Material_Contribution.Institution_Collection_Camp.subtype:name']; | ||
| } | ||
| elseif (!empty($contribution['Material_Contribution.Dropping_Center.subtype:name'])) { | ||
| $subtype = $contribution['Material_Contribution.Dropping_Center.subtype:name']; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicate code in $subtype assignment
The logic for assigning $subtype repeats similar patterns. Refactoring this code will enhance readability and maintainability.
Consider using a loop or an array to streamline the assignment:
$subtypeFields = [
'Material_Contribution.Collection_Camp.subtype:name',
'Material_Contribution.Institution_Collection_Camp.subtype:name',
'Material_Contribution.Dropping_Center.subtype:name',
];
$subtype = NULL;
foreach ($subtypeFields as $field) {
if (!empty($contribution[$field])) {
$subtype = $contribution[$field];
break;
}
}| ? 'Material_Contribution.Collection_Camp' | ||
| : (($subtype == 'Dropping_Center') | ||
| ? 'Material_Contribution.Dropping_Center' | ||
| : (($subtype == 'Institution_Collection_Camp') | ||
| ? 'Material_Contribution.Institution_Collection_Camp' | ||
| : NULL)); | ||
| error_log("campField: " . print_r($campField, TRUE)); |
There was a problem hiding this comment.
Fix syntax errors in nested ternary operators
The nested ternary operators in $campField assignment contain syntax errors, making the code unreadable and prone to bugs. Rewriting this logic using if-elseif-else statements will improve clarity and correctness.
Here's the corrected code:
if ($subtype == 'Collection_Camp') {
$campField = 'Material_Contribution.Collection_Camp';
} elseif ($subtype == 'Dropping_Center') {
$campField = 'Material_Contribution.Dropping_Center';
} elseif ($subtype == 'Institution_Collection_Camp') {
$campField = 'Material_Contribution.Institution_Collection_Camp';
} else {
$campField = NULL;
}
error_log("campField: " . print_r($campField, TRUE));| ? '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' | ||
| : NULL)); | ||
| error_log("addressField: " . print_r($addressField, TRUE)); |
There was a problem hiding this comment.
Correct syntax and improve readability in $addressField assignment
Similar to the previous issue, the nested ternary operators for $addressField have syntax errors and reduce code readability. Refactor using if-elseif-else statements for better comprehension.
Updated code:
if ($subtype == 'Collection_Camp') {
$addressField = 'Collection_Camp_Intent_Details.Location_Area_of_camp';
} elseif ($subtype == 'Dropping_Center') {
$addressField = 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_';
} elseif ($subtype == 'Institution_Collection_Camp') {
$addressField = 'Institution_Collection_Camp_Intent.Collection_Camp_Address';
} else {
$addressField = NULL;
}
error_log("addressField: " . print_r($addressField, TRUE));| /** | ||
| * | ||
| */ | ||
| private static function getInitiatorId(array $collectionCamp) { | ||
| $subtypeName = $collectionCamp['subtype:name']; | ||
|
|
||
| $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; | ||
|
|
||
| if ($subtypeName === 'Institution_Collection_Camp') { | ||
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of') | ||
| ->execute(); | ||
| // If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'. | ||
| if (empty($relationships)) { | ||
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of') | ||
| ->execute(); | ||
| } | ||
|
|
||
| // Return contact_id_b as initiator if found. | ||
| if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) { | ||
| return $relationships[0]['contact_id_b']; | ||
| } | ||
| } | ||
|
|
||
| // If the subtype is not 'Institution_Collection_Camp', or no relationship was found, use the default contact_id. | ||
| return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor getInitiatorId to eliminate duplicate relationship queries
The getInitiatorId method contains duplicated code when querying relationships for 'Primary Institution POC of' and 'Secondary Institution POC of'. Refactor the code to combine these queries into a single call to improve maintainability and readability.
Apply this diff to refactor the method:
private static function getInitiatorId(array $collectionCamp) {
$subtypeName = $collectionCamp['subtype:name'];
$organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id'];
if ($subtypeName === 'Institution_Collection_Camp') {
+ $relationshipTypes = ['Primary Institution POC of', 'Secondary Institution POC of'];
$relationships = Relationship::get(FALSE)
->addWhere('contact_id_a', '=', $organizationId)
- ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of')
+ ->addWhere('relationship_type_id:name', 'IN', $relationshipTypes)
->execute();
- // If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'.
- if (empty($relationships)) {
- $relationships = Relationship::get(FALSE)
- ->addWhere('contact_id_a', '=', $organizationId)
- ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of')
- ->execute();
- }
// Return contact_id_b as initiator if found.
if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) {
return $relationships[0]['contact_id_b'];
}
}
// If the subtype is not 'Institution_Collection_Camp', or no relationship was found, use the default contact_id.
return $collectionCamp['Collection_Camp_Core_Details.Contact_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.
| /** | |
| * | |
| */ | |
| private static function getInitiatorId(array $collectionCamp) { | |
| $subtypeName = $collectionCamp['subtype:name']; | |
| $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; | |
| if ($subtypeName === 'Institution_Collection_Camp') { | |
| $relationships = Relationship::get(FALSE) | |
| ->addWhere('contact_id_a', '=', $organizationId) | |
| ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of') | |
| ->execute(); | |
| // If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'. | |
| if (empty($relationships)) { | |
| $relationships = Relationship::get(FALSE) | |
| ->addWhere('contact_id_a', '=', $organizationId) | |
| ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of') | |
| ->execute(); | |
| } | |
| // Return contact_id_b as initiator if found. | |
| if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) { | |
| return $relationships[0]['contact_id_b']; | |
| } | |
| } | |
| // If the subtype is not 'Institution_Collection_Camp', or no relationship was found, use the default contact_id. | |
| return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; | |
| } | |
| /** | |
| * | |
| */ | |
| private static function getInitiatorId(array $collectionCamp) { | |
| $subtypeName = $collectionCamp['subtype:name']; | |
| $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; | |
| if ($subtypeName === 'Institution_Collection_Camp') { | |
| $relationshipTypes = ['Primary Institution POC of', 'Secondary Institution POC of']; | |
| $relationships = Relationship::get(FALSE) | |
| ->addWhere('contact_id_a', '=', $organizationId) | |
| ->addWhere('relationship_type_id:name', 'IN', $relationshipTypes) | |
| ->execute(); | |
| // Return contact_id_b as initiator if found. | |
| if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) { | |
| return $relationships[0]['contact_id_b']; | |
| } | |
| } | |
| // If the subtype is not 'Institution_Collection_Camp', or no relationship was found, use the default contact_id. | |
| return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (3)
245-256: Consider extracting tab configurations to a separate configuration file.The tab configurations are growing, and they all share similar structure. Consider moving these configurations to a separate configuration file to improve maintainability and reduce the risk of configuration errors.
Example structure:
// config/tabs.php return [ 'vehicleDispatch' => [ 'title' => ts('Dispatch'), 'module' => 'afsearchInstitutionCampVehicleDispatchData', 'directive' => 'afsearch-institution-camp-vehicle-dispatch-data', 'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl', ], // ... other configurations ];
247-249: Standardize naming conventions across directives and modules.The naming conventions between tabs are inconsistent:
afsearchInstitutionCampVehicleDispatchDatavsafsearchInstitutionCampAcknowledgementDispatchafsearch-institution-camp-vehicle-dispatch-datavsafsearch-institution-camp-acknowledgement-dispatchConsider standardizing these names for better maintainability.
Also applies to: 253-255
Line range hint
1-300: Consider splitting the class into smaller, focused services.The
InstitutionCollectionCampServiceclass currently handles multiple responsibilities:
- QR code generation
- State options management
- Office details management
- Tab configuration
Consider splitting these into separate services to improve maintainability and testability:
InstitutionCollectionCampQRServiceInstitutionCollectionCampStateServiceInstitutionCollectionCampOfficeServiceInstitutionCollectionCampTabServiceThis would:
- Make the code more maintainable
- Make it easier to test individual components
- Follow the Single Responsibility Principle
- Make it easier to modify individual features without affecting others
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)
249-249: Verify if using the same template for all tabs is intentional.
All tabs are using the same template 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl'. Please verify if this is intentional or if each tab should have its own specialized template.
Also applies to: 255-255
✅ Verification successful
Let me gather more information about how this template is being used.
Let me check the content of the other template file to understand if it's being used and how it differs.
Using the same template across all tabs is intentional and follows best practices
The shared template CollectionCamp.tpl is correctly used across all tabs because:
- It serves as a generic wrapper that dynamically loads Angular modules and directives based on the tab configuration
- Each tab's unique functionality is encapsulated in its own directive and module, making the template reusable
- The template structure is identical for all tabs, following the DRY (Don't Repeat Yourself) principle
The existence of CollectionCampService.tpl appears to be legacy code as it's only referenced in DroppingCenterService.php but not actively used in the tab configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the template file exists and look for tab-specific content
cat wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl
# Check if there are any other similar templates that might be more appropriate
fd CollectionCamp.tpl wp-content/civi-extensions/goonjcustom/templates/
Length of output: 460
Script:
#!/bin/bash
# Check for the usage of CollectionCamp.tpl in PHP files
rg -l "CollectionCamp.tpl" wp-content/civi-extensions/goonjcustom/
# Check the context of template usage
rg -B 5 -A 5 "CollectionCamp.tpl" wp-content/civi-extensions/goonjcustom/
# Look for other similar templates
fd .tpl wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/
Length of output: 22051
Script:
#!/bin/bash
# Check the content of CollectionCampService.tpl
cat wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCampService.tpl
# Check if CollectionCampService.tpl is referenced anywhere
rg -l "CollectionCampService.tpl" wp-content/civi-extensions/goonjcustom/
Length of output: 464
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (1)
361-361: Improve error logging for queue failuresThe error logging should be enhanced to include more debugging information.
Suggested improvement:
- \Civi::log()->debug('Cannot queue authorization email for initiator.', [ + \Civi::log()->error('Cannot queue authorization email for initiator.', [ 'initiatorId' => $initiatorId, 'status' => $status, 'entityId' => $objectRef['id'], 'error' => $ex->getMessage(), + 'stack_trace' => $ex->getTraceAsString(), ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(5 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php (2)
13-13: LGTM!
The Relationship API import is properly placed with other Civi API imports.
70-73: LGTM!
The change improves flexibility by generating filenames based on entity subtype while maintaining consistent naming conventions.
| private static function getInitiatorId(array $collectionCamp) { | ||
| $subtypeName = $collectionCamp['subtype:name']; | ||
|
|
||
| $organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id']; | ||
|
|
||
| if ($subtypeName === 'Institution_Collection_Camp') { | ||
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Primary Institution POC of') | ||
| ->execute(); | ||
| // If no relationships found for 'Primary Institution POC of', check for 'Secondary Institution POC of'. | ||
| if (empty($relationships)) { | ||
| $relationships = Relationship::get(FALSE) | ||
| ->addWhere('contact_id_a', '=', $organizationId) | ||
| ->addWhere('relationship_type_id:name', '=', 'Secondary Institution POC of') | ||
| ->execute(); | ||
| } | ||
|
|
||
| // Return contact_id_b as initiator if found. | ||
| if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) { | ||
| return $relationships[0]['contact_id_b']; | ||
| } | ||
| } | ||
|
|
||
| // If the subtype is not 'Institution_Collection_Camp', or no relationship was found, use the default contact_id. | ||
| return $collectionCamp['Collection_Camp_Core_Details.Contact_Id']; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to improve maintainability and reduce duplication
The method has several issues:
- Duplicate API calls for different relationship types
- Hardcoded relationship type names
- Missing error handling
Suggested improvements:
+ private const PRIMARY_POC_RELATIONSHIP = 'Primary Institution POC of';
+ private const SECONDARY_POC_RELATIONSHIP = 'Secondary Institution POC of';
private static function getInitiatorId(array $collectionCamp) {
$subtypeName = $collectionCamp['subtype:name'];
$organizationId = $collectionCamp['Institution_Collection_Camp_Intent.Organization_Name.id'];
if ($subtypeName === 'Institution_Collection_Camp') {
+ try {
+ $relationshipTypes = [self::PRIMARY_POC_RELATIONSHIP, self::SECONDARY_POC_RELATIONSHIP];
+ $relationships = Relationship::get(FALSE)
+ ->addWhere('contact_id_a', '=', $organizationId)
+ ->addWhere('relationship_type_id:name', 'IN', $relationshipTypes)
+ ->execute();
+ if (!empty($relationships) && isset($relationships[0]['contact_id_b'])) {
+ return $relationships[0]['contact_id_b'];
+ }
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to retrieve relationships', [
+ 'error' => $e->getMessage(),
+ 'organizationId' => $organizationId
+ ]);
+ }
}
return $collectionCamp['Collection_Camp_Core_Details.Contact_Id'];
}Committable suggestion skipped: line range outside the PR's diff.
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'subtype:name') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute()->single(); | ||
|
|
||
| $initiatorId = self::getInitiatorId($currentCollectionCamp); | ||
|
|
There was a problem hiding this comment.
Add error handling for the API call
The EckEntity::get call could fail, but there's no try-catch block to handle potential exceptions.
Add error handling:
- $currentCollectionCamp = EckEntity::get('Collection_Camp', FALSE)
- ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'subtype:name')
- ->addWhere('id', '=', $objectId)
- ->execute()->single();
-
- $initiatorId = self::getInitiatorId($currentCollectionCamp);
+ try {
+ $currentCollectionCamp = EckEntity::get('Collection_Camp', FALSE)
+ ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'subtype:name')
+ ->addWhere('id', '=', $objectId)
+ ->execute()->single();
+
+ $initiatorId = self::getInitiatorId($currentCollectionCamp);
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to retrieve collection camp details', [
+ 'error' => $e->getMessage(),
+ 'objectId' => $objectId
+ ]);
+ return;
+ }📝 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.
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'subtype:name') | |
| ->addWhere('id', '=', $objectId) | |
| ->execute()->single(); | |
| $initiatorId = self::getInitiatorId($currentCollectionCamp); | |
| try { | |
| $currentCollectionCamp = EckEntity::get('Collection_Camp', FALSE) | |
| ->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'Institution_Collection_Camp_Intent.Organization_Name.id', 'subtype:name') | |
| ->addWhere('id', '=', $objectId) | |
| ->execute()->single(); | |
| $initiatorId = self::getInitiatorId($currentCollectionCamp); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to retrieve collection camp details', [ | |
| 'error' => $e->getMessage(), | |
| 'objectId' => $objectId | |
| ]); | |
| return; | |
| } |
| * | ||
| */ | ||
| private function formatDate($start, $end) { | ||
|
|
There was a problem hiding this comment.
will do this in new pr
| * | ||
| */ | ||
| private function formatTime($start, $end) { | ||
|
|
There was a problem hiding this comment.
will do this in new pr
belwalshubham
deleted the
feat/institution-collection-camp-functionality
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes