Refactor: Institution Relationship#602
Conversation
WalkthroughThe changes in this pull request involve 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: 1
🧹 Outside diff range and nitpick comments (5)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (1)
298-300: Consider using constants for relationship type names.The relationship type mappings use string literals which could lead to maintenance issues if these values need to be changed across the codebase.
Consider defining these as class constants:
+ const RELATIONSHIP_TYPE_FOUNDATION = 'Foundation Coordinator of'; + const RELATIONSHIP_TYPE_EDUCATION = 'Education Institute Coordinator of'; + const RELATIONSHIP_TYPE_ASSOCIATIONS = 'Associations Coordinator of'; private static function getRelationshipTypeName($contactId) { // ... $typeToRelationshipMap = [ - 'Foundation' => 'Foundation Coordinator of', - 'Education' => 'Education Institute Coordinator of', - 'Associations' => 'Associations Coordinator of', + 'Foundation' => self::RELATIONSHIP_TYPE_FOUNDATION, + 'Education' => self::RELATIONSHIP_TYPE_EDUCATION, + 'Associations' => self::RELATIONSHIP_TYPE_ASSOCIATIONS, // ... ];wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)
Line range hint
556-577: Refactor method to improve separation of concerns.The
assignCoordinatorByRelationshipTypemethod has multiple responsibilities: mapping types, finding coordinators, and updating entities. This makes it harder to maintain and test.Consider breaking it down into smaller, focused methods:
private function getRelationshipTypeForRegistration($registrationType) { $registrationCategory = trim($registrationType['Institution_Collection_Camp_Intent.You_wish_to_register_as:name']); return $this->relationshipTypeMap[$registrationCategory] ?? 'Other Entities Coordinator of'; } private function updateCollectionCampCoordinator($collectionCampId, $coordinatorId) { return EckEntity::update('Collection_Camp', FALSE) ->addValue('Institution_collection_camp_Review.Coordinating_POC', $coordinatorId) ->addWhere('id', '=', $collectionCampId) ->execute(); } public function assignCoordinatorByRelationshipType($stateOfficeId, $registrationType, $collectionCampId) { $relationshipTypeName = $this->getRelationshipTypeForRegistration($registrationType); $coordinator = $this->getCoordinator($stateOfficeId, $relationshipTypeName); if (!$coordinator) { \CRM_Core_Error::debug_log_message( sprintf('No coordinator available for relationship type %s and office %d', $relationshipTypeName, $stateOfficeId ) ); return FALSE; } return $this->updateCollectionCampCoordinator($collectionCampId, $coordinator['contact_id_a']); }wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InstitutionDroppingCenterOutcomeCron.php (3)
Line range hint
14-77: Consider refactoring for better maintainability and readability.The main function is handling multiple responsibilities and could benefit from being split into smaller, focused functions. Consider these improvements:
- Extract data collection logic into separate methods:
+ private function collectCashContributions() { + $cashContributionById = []; + $droppingCenterMetas = EckEntity::get('Dropping_Center_Meta', TRUE) + ->addSelect('Donation.Cash_Contribution', 'Dropping_Center_Meta.Institution_Dropping_Center') + ->addClause('OR', ['Donation.Cash_Contribution', 'IS NOT EMPTY']) + ->execute(); + foreach ($droppingCenterMetas as $center) { + $id = $center['Dropping_Center_Meta.Institution_Dropping_Center']; + $cashContributionById[$id] = ($cashContributionById[$id] ?? 0) + (float) ($center['Donation.Cash_Contribution'] ?? 0); + } + return $cashContributionById; + }
- Define constants for frequently used column names:
+ private const COLUMN_INSTITUTION_DROPPING_CENTER = 'Material_Contribution.Institution_Dropping_Center'; + private const COLUMN_CASH_CONTRIBUTION = 'Donation.Cash_Contribution';
- Add error handling for database operations:
+ try { + $activities = Activity::get(TRUE) + ->addSelect(self::COLUMN_INSTITUTION_DROPPING_CENTER) + ->addWhere('activity_type_id:name', '=', 'Material Contribution') + ->addWhere(self::COLUMN_INSTITUTION_DROPPING_CENTER, 'IS NOT NULL') + ->execute(); + } catch (Exception $e) { + \Civi::log()->error('Failed to fetch activities: ' . $e->getMessage()); + return civicrm_api3_create_error('Failed to process dropping center outcomes'); + }
Line range hint
1-13: Improve documentation for better maintainability.The file lacks comprehensive documentation about its purpose and behavior.
Add PHPDoc blocks explaining:
/** * @file + * Provides cron functionality for updating dropping center metrics. */ use Civi\Api4\Activity; use Civi\Api4\EckEntity; /** * Cron job to update Dropping Center outcomes. + * + * @param array $params Parameters passed to the API + * @return array API result array + * + * This cron job aggregates various metrics for dropping centers including: + * - Cash contributions + * - Product sales + * - Vehicle dispatch counts + * - Center footfall + * - Bags received */
Line range hint
58-68: Add return type hints and parameter documentation.The helper function lacks proper type hints and parameter documentation.
/** - * + * Updates a specific metric for institution dropping centers. + * + * @param string $metricName The name of the metric to update + * @param array<int,float|int> $data Array of values indexed by dropping center ID + * @return void */ - function updateInstitutionDroppingCenterMetric($metricName, $data) { + function updateInstitutionDroppingCenterMetric(string $metricName, array $data): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InstitutionDroppingCenterOutcomeCron.php(1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/InstitutionDroppingCenterOutcomeCron.php (1)
47-47: Verify the column name change impact.
The change from using Dropping_Center to Institution_Dropping_Center for footfall calculation appears correct, but let's verify this doesn't miss any existing data.
✅ Verification successful
The column name change is consistent with the codebase structure
The codebase shows a clear distinction between regular dropping centers (Dropping_Center) and institution dropping centers (Institution_Dropping_Center). The change from Material_Contribution.Dropping_Center to Material_Contribution.Institution_Dropping_Center in the footfall calculation is correct because:
- The file
InstitutionDroppingCenterOutcomeCron.phpspecifically handles institution dropping centers, as evidenced by:- All other queries in the file use
Institution_Dropping_Center - The entity subtype is explicitly checked as
Institution_Dropping_Center - The corresponding service class
InstitutionDroppingCenterService.phpuses the same naming
- All other queries in the file use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old column name
rg -i "dropping_center[^_]" --type php
# Search for the new column name pattern to confirm consistency
rg -i "institution_dropping_center" --type php
Length of output: 22050
| 'Corporate' => 'Corporate Coordinator of', | ||
| 'School' => 'School Coordinator of', | ||
| 'College' => 'College Coordinator of', | ||
| 'Associations' => 'Associations Coordinator of', | ||
| 'Others' => 'Others Coordinator of', |
There was a problem hiding this comment.
Eliminate duplicate relationship type mappings.
The relationship type mappings are duplicated between InstitutionService and InstitutionCollectionCampService. This violates the DRY principle and could lead to inconsistencies if one mapping is updated but not the other.
Consider extracting these mappings to a shared configuration class:
class RelationshipTypeConfig {
const COORDINATOR_TYPES = [
'Corporate' => 'Corporate Coordinator of',
'School' => 'School Coordinator of',
'College' => 'College Coordinator of',
'Associations' => 'Associations Coordinator of',
'Others' => 'Others Coordinator of',
];
}Then use this shared configuration in both services:
- $relationshipTypeMap = [
- 'Corporate' => 'Corporate Coordinator of',
- 'School' => 'School Coordinator of',
- 'College' => 'College Coordinator of',
- 'Associations' => 'Associations Coordinator of',
- 'Others' => 'Others Coordinator of',
- ];
+ $relationshipTypeMap = RelationshipTypeConfig::COORDINATOR_TYPES;
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
Line range hint
302-344: Consider refactoring for better maintainabilityThe
mailNotificationToMmtmethod could benefit from:
- Separation of concerns (data fetching, email composition, sending)
- Improved error handling for database operations
- Early returns to reduce nesting
Consider splitting the method into smaller, focused methods:
private function fetchInstitutionData($institutionDroppingCenterId) { return EckEntity::get('Collection_Camp', FALSE) ->addSelect('Institution_Dropping_Center_Intent.Dropping_Center_Address', 'title') ->addWhere('id', '=', $institutionDroppingCenterId) ->execute() ->single(); } private function fetchMmtCoordinator($goonjFieldId) { $coordinators = Relationship::get(FALSE) ->addWhere('contact_id_b', '=', $goonjFieldId) ->addWhere('relationship_type_id:name', '=', self::MATERIAL_RELATIONSHIP_TYPE_NAME) ->addWhere('is_current', '=', TRUE) ->execute() ->first(); return $coordinators['contact_id_a'] ?? null; } private function sendMmtNotification($mmtId, $institutionData, $vehicleDispatchId) { // Email composition and sending logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(3 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (2)
341-341:
Fix undefined variable usage
The $fromEmail variable used in the replyTo field is undefined, which could cause runtime errors.
- 'replyTo' => $fromEmail['label'],
+ 'replyTo' => $from,Likely invalid or redundant comment.
302-302: Verify field name change consistency
The field name has been changed from 'Collection_Camp' to 'Institution_Dropping_Center'. Let's verify this change is consistent across the codebase.
| ->execute()->single(); | ||
|
|
||
| $institutionDroppingCenter = $institutionDroppingCenter['title']; | ||
| $InstitutionDroppingCenterCode = $institutionDroppingCenter['title']; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow PHP variable naming conventions
The variable name InstitutionDroppingCenterCode uses PascalCase, which violates PHP naming conventions. Variables should use camelCase.
- $InstitutionDroppingCenterCode = $institutionDroppingCenter['title'];
+ $institutionDroppingCenterCode = $institutionDroppingCenter['title'];📝 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.
| $InstitutionDroppingCenterCode = $institutionDroppingCenter['title']; | |
| $institutionDroppingCenterCode = $institutionDroppingCenter['title']; |
1 participant
Description