Feat/assign to chapter group #617
Conversation
…at/assign-to-chapter-group
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: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (3)
94-105: Replace magic strings with constants in the$assignmentsarrayUsing hardcoded strings for assignment keys and field names can lead to errors and makes the code harder to maintain. Consider defining constants for the keys like
'Institution Collection Camp'and field names within the class or in a configuration file. This will enhance readability and make future modifications easier.
130-135: Eliminate code duplication when adding contacts to the groupTo adhere to the DRY (Don't Repeat Yourself) principle, consider collecting the contact IDs into an array and iterating over them. This reduces code duplication and simplifies the addition of more contacts in the future.
Apply this diff to refactor the code:
if ($groupId) { - self::addContactToGroup($contactId, $groupId); - if ($organizationId) { - self::addContactToGroup($organizationId, $groupId); - } + $contactIds = [$contactId]; + if ($organizationId) { + $contactIds[] = $organizationId; + } + foreach ($contactIds as $id) { + self::addContactToGroup($id, $groupId); + } }
92-137: RefactorassignChapterGroupToIndividualto follow the Single Responsibility PrincipleThe
assignChapterGroupToIndividualmethod handles multiple responsibilities, including validation, data retrieval, error logging, and group assignment. Breaking this method into smaller, focused functions will enhance readability, promote reusability, and adhere to the Single Responsibility Principle.
| return; | ||
| if ($objectName !== 'Eck_Collection_Camp' || empty($objectRef['title']) || $objectRef['title'] !== 'Institution Collection Camp') { | ||
| return FALSE; | ||
| public static function assignChapterGroupToIndividual(string $op, string $objectName, $objectId, &$objectRef) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent parameter type for $objectId
The type hint for $objectId has been changed from int to untyped. This could lead to type safety issues or unexpected behavior if non-integer values are passed. Consider specifying the expected type(s) for $objectId or using appropriate type declarations to maintain type safety.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
53-78: Define status constants for better maintainabilityMagic strings for status values make the code harder to maintain and more prone to errors.
+ private const STATUS_AUTHORIZED = 'authorized'; + public static function linkCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) { // ... - if ($currentStatus !== $newStatus && $newStatus === 'authorized') { + if ($currentStatus !== $newStatus && $newStatus === self::STATUS_AUTHORIZED) {
81-103: Enhance error handling in createCollectionCampOrganizeActivityThe catch block only logs the error. Consider:
- Using more specific exception types
- Providing more context in the error message
- Potentially rethrowing critical errors
- } catch (\CiviCRM_API4_Exception $ex) { - \Civi::log()->debug("Exception while creating Organize Collection Camp activity: " . $ex->getMessage()); + } catch (\CiviCRM_API4_Exception $ex) { + $context = [ + 'collection_camp_id' => $collectionCampId, + 'poc_id' => $PocId, + 'organization_id' => $organizationId + ]; + \Civi::log()->error( + "Failed to create Collection Camp activity: " . $ex->getMessage(), + ['context' => $context] + ); + // Optionally rethrow if this is a critical error + throw $ex;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(3 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
9-10: LGTM: Clean import additions
The new imports for Group and GroupContact APIs are properly organized and follow the existing pattern.
92-92: Verify impact of parameter type change
The type hint for $objectId has been removed. This could impact type safety if non-integer values are passed.
Let's verify all callers of this method:
#!/bin/bash
# Search for all calls to assignChapterGroupToIndividual
rg -l "assignChapterGroupToIndividual" | while read -r file; do
echo "=== $file ==="
rg -A 2 "assignChapterGroupToIndividual" "$file"
donewp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)
38-38: LGTM: Event subscription addition is well-placed
The new event handler is correctly added to the existing pre-hook subscriptions.
| @@ -26,7 +28,7 @@ public static function getSubscribedEvents() { | |||
| '&hook_civicrm_post' => [ | |||
| ['organizationCreated'], | |||
| ['setOfficeDetails'], | |||
| ['assignChapterGroupToIndividual'], | |||
| // ['assignChapterGroupToIndividual'], | |||
There was a problem hiding this comment.
Avoid commenting out event subscriptions without proper cleanup
The commented-out event subscription could lead to:
- Dead code if not properly cleaned up
- Breaking changes if other parts of the system depend on this hook
- Confusion about the intended state of the feature
Either remove the line completely or document why it's temporarily disabled.
| // public static function assignChapterGroupToIndividual(string $op, string $objectName, $objectId, &$objectRef) { | ||
| // // Define the assignments array | ||
| // $assignments = [ | ||
| // 'Institution Collection Camp' => [ | ||
| // 'stateField' => 'Institution_Collection_Camp_Intent.State', | ||
| // 'contactField' => 'Institution_Collection_Camp_Intent.Institution_POC', | ||
| // 'organizationField' => 'Institution_Collection_Camp_Intent.Organization_Name' | ||
| // ], | ||
| // 'Institution Dropping Center' => [ | ||
| // 'stateField' => 'Institution_Dropping_Center_Intent.State', | ||
| // 'contactField' => 'Institution_Dropping_Center_Intent.Institution_POC', | ||
| // 'organizationField' => 'Institution_Dropping_Center_Intent.Organization_Name' | ||
| // ] | ||
| // ]; | ||
|
|
||
|
|
||
|
|
||
| // // Validate objectName and objectRef | ||
| // if ($objectName !== 'Eck_Collection_Camp' || empty($objectRef['title']) || !isset($assignments[$objectRef['title']])) { | ||
| // return FALSE; | ||
| // } | ||
|
|
||
| // $assignment = $assignments[$objectRef['title']]; | ||
|
|
||
| // // Access fields using array syntax | ||
| // $stateId = $objectRef[$assignment['stateField']] ?? NULL; | ||
| // $contactId = $objectRef[$assignment['contactField']] ?? NULL; | ||
| // $organizationId = $objectRef[$assignment['organizationField']] ?? NULL; | ||
|
|
||
| // if (!$stateId || !$contactId) { | ||
| // \Civi::log()->info("Missing Contact ID or State ID for " . $objectRef['title']); | ||
| // return FALSE; | ||
| // } | ||
|
|
||
| // // Get the group and add contacts | ||
| // $groupId = self::getChapterGroupForState($stateId); | ||
|
|
||
| // if ($groupId) { | ||
| // self::addContactToGroup($contactId, $groupId); | ||
| // if ($organizationId) { | ||
| // self::addContactToGroup($organizationId, $groupId); | ||
| // } | ||
| // } | ||
|
|
||
| // return TRUE; | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor commented implementation for better maintainability
The commented-out implementation has several areas for improvement:
- Move the assignments array to a configuration file or class constant to improve maintainability
- Break down the method into smaller, focused methods following Single Responsibility Principle
- Consider using enums or constants for the title strings to prevent typos
Here's a suggested refactor:
private const INSTITUTION_ASSIGNMENTS = [
self::INSTITUTION_COLLECTION_CAMP => [
'stateField' => 'Institution_Collection_Camp_Intent.State',
'contactField' => 'Institution_Collection_Camp_Intent.Institution_POC',
'organizationField' => 'Institution_Collection_Camp_Intent.Organization_Name'
],
self::INSTITUTION_DROPPING_CENTER => [
'stateField' => 'Institution_Dropping_Center_Intent.State',
'contactField' => 'Institution_Dropping_Center_Intent.Institution_POC',
'organizationField' => 'Institution_Dropping_Center_Intent.Organization_Name'
]
];
public static function assignChapterGroupToIndividual(string $op, string $objectName, $objectId, &$objectRef) {
if (!self::isValidAssignment($objectName, $objectRef)) {
return FALSE;
}
$assignment = self::INSTITUTION_ASSIGNMENTS[$objectRef['title']];
$ids = self::extractAssignmentIds($objectRef, $assignment);
if (!self::areIdsValid($ids)) {
return FALSE;
}
return self::processGroupAssignment($ids);
}
private static function isValidAssignment(string $objectName, array $objectRef): bool {
return $objectName === 'Eck_Collection_Camp'
&& !empty($objectRef['title'])
&& isset(self::INSTITUTION_ASSIGNMENTS[$objectRef['title']]);
}
private static function extractAssignmentIds(array $objectRef, array $assignment): array {
return [
'stateId' => $objectRef[$assignment['stateField']] ?? NULL,
'contactId' => $objectRef[$assignment['contactField']] ?? NULL,
'organizationId' => $objectRef[$assignment['organizationField']] ?? NULL,
];
}
private static function areIdsValid(array $ids): bool {
if (!$ids['stateId'] || !$ids['contactId']) {
\Civi::log()->info("Missing Contact ID or State ID");
return FALSE;
}
return TRUE;
}
private static function processGroupAssignment(array $ids): bool {
$groupId = self::getChapterGroupForState($ids['stateId']);
if (!$groupId) {
return FALSE;
}
self::addContactToGroup($ids['contactId'], $groupId);
if ($ids['organizationId']) {
self::addContactToGroup($ids['organizationId'], $groupId);
}
return TRUE;
}| private static function createActivity($contactId, $activityData) { | ||
| Activity::create(FALSE) | ||
| ->addValue('source_contact_id', $contactId) | ||
| ->addValue('target_contact_id', $contactId) | ||
| ->addValues($activityData) | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Add error handling to createActivity method
The method should handle potential API exceptions to prevent silent failures.
private static function createActivity($contactId, $activityData) {
+ try {
Activity::create(FALSE)
->addValue('source_contact_id', $contactId)
->addValue('target_contact_id', $contactId)
->addValues($activityData)
->execute();
+ } catch (\CiviCRM_API4_Exception $ex) {
+ \Civi::log()->error(
+ "Failed to create activity for contact {$contactId}: " . $ex->getMessage(),
+ ['activity_data' => $activityData]
+ );
+ throw $ex;
+ }
}📝 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 createActivity($contactId, $activityData) { | |
| Activity::create(FALSE) | |
| ->addValue('source_contact_id', $contactId) | |
| ->addValue('target_contact_id', $contactId) | |
| ->addValues($activityData) | |
| ->execute(); | |
| } | |
| private static function createActivity($contactId, $activityData) { | |
| try { | |
| Activity::create(FALSE) | |
| ->addValue('source_contact_id', $contactId) | |
| ->addValue('target_contact_id', $contactId) | |
| ->addValues($activityData) | |
| ->execute(); | |
| } catch (\CiviCRM_API4_Exception $ex) { | |
| \Civi::log()->error( | |
| "Failed to create activity for contact {$contactId}: " . $ex->getMessage(), | |
| ['activity_data' => $activityData] | |
| ); | |
| throw $ex; | |
| } | |
| } |
| public static function linkCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) { | ||
| if ($objectName !== 'Eck_Collection_Camp' || !$objectId) { | ||
| return; | ||
| } | ||
|
|
||
| $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; | ||
| if (!$newStatus) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionCamps = EckEntity::get('Collection_Camp', FALSE) | ||
| ->addSelect('Collection_Camp_Core_Details.Status', 'Institution_Collection_Camp_Intent.Organization_Name', 'title', 'Institution_Collection_Camp_Intent.Institution_POC') | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute(); | ||
|
|
||
| $currentCollectionCamp = $collectionCamps->first(); | ||
| $currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status']; | ||
| $PocId = $currentCollectionCamp['Institution_Collection_Camp_Intent.Institution_POC']; | ||
| $organizationId = $currentCollectionCamp['Institution_Collection_Camp_Intent.Organization_Name']; | ||
|
|
||
| if (!$PocId && !$organizationId) { | ||
| return; | ||
| } | ||
|
|
||
| $collectionCampTitle = $currentCollectionCamp['title']; | ||
| $collectionCampId = $currentCollectionCamp['id']; | ||
|
|
||
| // Check for status change and handle activity creation | ||
| if ($currentStatus !== $newStatus && $newStatus === 'authorized') { | ||
| self::createCollectionCampOrganizeActivity($PocId, $organizationId, $collectionCampTitle, $collectionCampId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to improve separation of concerns
The linkCollectionCampToContact method handles multiple responsibilities. Consider splitting it into smaller, focused methods:
- Status validation
- Collection camp data retrieval
- Activity creation orchestration
-public static function linkCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) {
+private static function validateCollectionCampStatus(string $objectName, $objectId, $newStatus): bool {
+ return $objectName === 'Eck_Collection_Camp' && $objectId && $newStatus;
+}
+
+private static function getCollectionCampData($objectId) {
+ return EckEntity::get('Collection_Camp', FALSE)
+ ->addSelect(
+ 'Collection_Camp_Core_Details.Status',
+ 'Institution_Collection_Camp_Intent.Organization_Name',
+ 'title',
+ 'Institution_Collection_Camp_Intent.Institution_POC'
+ )
+ ->addWhere('id', '=', $objectId)
+ ->execute()
+ ->first();
+}
+
+public static function linkCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) {
+ $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? '';
+ if (!self::validateCollectionCampStatus($objectName, $objectId, $newStatus)) {
+ return;
+ }
+
+ $currentCollectionCamp = self::getCollectionCampData($objectId);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
51-81: Consider breaking down the method for better maintainabilityThe method handles multiple responsibilities:
- Validation
- Data retrieval
- Status comparison
- Activity creation
Consider extracting these into separate private methods for better maintainability and testability.
-public static function linkInstitutionCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) { +private static function validateCollectionCampInput(string $objectName, $objectId, $newStatus): bool { + return $objectName === 'Eck_Collection_Camp' && $objectId && $newStatus; +} + +private static function getCollectionCampDetails($objectId) { + return EckEntity::get('Collection_Camp', FALSE) + ->addSelect( + 'Collection_Camp_Core_Details.Status', + 'Institution_Collection_Camp_Intent.Organization_Name', + 'title', + 'Institution_Collection_Camp_Intent.Institution_POC' + ) + ->addWhere('id', '=', $objectId) + ->execute() + ->first(); +} + +public static function linkInstitutionCollectionCampToContact(string $op, string $objectName, $objectId, &$objectRef) { + $newStatus = $objectRef['Collection_Camp_Core_Details.Status'] ?? ''; + if (!self::validateCollectionCampInput($objectName, $objectId, $newStatus)) { + return; + } + + $currentCollectionCamp = self::getCollectionCampDetails($objectId);
Line range hint
1-1000: Consider splitting the class into smaller servicesThe
InstitutionCollectionCampServiceclass has grown to handle multiple concerns:
- Collection camp management
- Email notifications
- Activity creation
- Coordinator assignment
- Office management
Consider splitting these into separate services:
CollectionCampActivityServiceCollectionCampNotificationServiceCollectionCampCoordinatorServiceThis would improve maintainability and adhere better to the Single Responsibility Principle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(2 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
106-118:
Add error handling to createActivity method
The method should handle potential API exceptions to prevent silent failures.
private static function createActivity($contactId, $collectionCampTitle, $collectionCampId) {
+ try {
Activity::create(FALSE)
->addValue('subject', $collectionCampTitle)
->addValue('activity_type_id:name', 'Organize Collection Camp')
->addValue('status_id:name', 'Authorized')
->addValue('activity_date_time', date('Y-m-d H:i:s'))
->addValue('source_contact_id', $contactId)
->addValue('target_contact_id', $contactId)
->addValue('Collection_Camp_Data.Collection_Camp_ID', $collectionCampId)
->execute();
\Civi::log()->info("Activity created for contact {$contactId} for Institution Collection Camp {$collectionCampTitle}");
+ } catch (\CiviCRM_API4_Exception $ex) {
+ \Civi::log()->error(
+ "Failed to create activity for contact {$contactId}: " . $ex->getMessage(),
+ ['collection_camp' => $collectionCampTitle]
+ );
+ throw $ex;
+ }
}Likely invalid or redundant comment.
38-38: Verify hook implementation order
The new hook implementation is added to the hook_civicrm_pre event. Ensure that the order of execution doesn't affect other hooks in the chain.
Summary by CodeRabbit
New Features
Bug Fixes