Open
Conversation
WalkthroughAdds a new public static aclCollectionCamp method to CollectionBaseService that filters Institution_Visit camps by user-controlled states using API4 Address lookups and integrates it into the selectWhereClause hook; also adds an InstituteMaterialContribution tab configuration to InstitutionCollectionCampService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CiviCRM as "CiviCRM (selectWhereClause hook)"
participant Service as "CollectionBaseService::aclCollectionCamp"
participant API4 as "Civi API4 (Institution_Visit, Address)"
participant Log as "Logger"
User->>CiviCRM: Request Institution_Visit list
CiviCRM->>Service: hook_civicrm_selectWhereClause(entity=Institution_Visit)
Service->>Service: Check entity and user roles
Service->>Service: Resolve user's chapter-team group and controlled states
alt no controlled states
Service->>Log: Log missing states
Service-->>CiviCRM: Return restrictive/null clause
else has controlled states
Service->>API4: Institution_Visit.list(select processing_center_id)
API4-->>Service: Camp rows with processing_center_id
Service->>API4: Address.get(contact_id IN processing_center_ids)
API4-->>Service: Addresses with state_province_id
Service->>Service: Filter camp IDs by controlled states
alt no allowed camp IDs
Service->>Log: Log empty allowed camps
Service-->>CiviCRM: Apply clause matching NULL (no rows)
else allowed camp IDs exist
Service-->>CiviCRM: Apply clause camp_id IN (allowed IDs)
end
end
CiviCRM-->>User: Render filtered camp list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Best-practice flags
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php(4 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-11T09:53:14.458Z
Learnt from: belwalshubham
PR: ColoredCow/goonj#284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Applied to files:
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php
🪛 PHPMD (2.15.0)
wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php
63-63: Avoid unused parameters such as '$conditions'. (undefined)
(UnusedFormalParameter)
Comment on lines
+63
to
+186
| public static function aclCollectionCamp($entity, &$clauses, $userId, $conditions = []) { | ||
| if (!in_array($entity, ['Eck_Collection_Camp', 'Eck_Institution_Visit'])) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $restrictedRoles = [ | ||
| 'admin', 'urban_ops_admin', 'ho_account', | ||
| 'project_team_ho', 's2s_ho_team', 'njpc_ho_team', 'project_ho_and_accounts', | ||
| ]; | ||
|
|
||
| if (\CRM_Core_Permission::checkAnyPerm($restrictedRoles)) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Step 0: Get user's group-controlled states | ||
| $teamGroupContact = GroupContact::get(FALSE) | ||
| ->addSelect('group_id') | ||
| ->addWhere('contact_id', '=', $userId) | ||
| ->addWhere('status', '=', 'Added') | ||
| ->addWhere('group_id.Chapter_Contact_Group.Use_Case', '=', 'chapter-team') | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| if (!$teamGroupContact) { | ||
| error_log("ACL: User $userId has no chapter team group."); | ||
| return FALSE; | ||
| } | ||
|
|
||
| $groupId = $teamGroupContact['group_id']; | ||
|
|
||
| $group = Group::get(FALSE) | ||
| ->addSelect('Chapter_Contact_Group.States_controlled') | ||
| ->addWhere('id', '=', $groupId) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| $statesControlled = $group['Chapter_Contact_Group.States_controlled'] ?? []; | ||
| if (empty($statesControlled)) { | ||
| error_log("ACL: Group $groupId controls no states."); | ||
| $clauses['id'][] = 'IN (null)'; | ||
| return TRUE; | ||
| } | ||
|
|
||
| $statesControlled = array_unique($statesControlled); | ||
| error_log('ACL: Controlled states: ' . print_r($statesControlled, TRUE)); | ||
|
|
||
| // Step 1: Fetch all camps as array | ||
| try { | ||
| $campsResult = \Civi\Api4\EckEntity::get('Institution_Visit', FALSE) | ||
| ->addSelect('Urban_Planned_Visit.Which_Goonj_Processing_Center_do_you_wish_to_visit_') | ||
| ->execute(); | ||
|
|
||
| $camps = iterator_to_array($campsResult); // | ||
| } catch (\Exception $e) { | ||
| error_log("ACL: Unable to fetch Institution_Visit: " . $e->getMessage()); | ||
| $clauses['id'][] = 'IN (null)'; | ||
| return TRUE; | ||
| } | ||
|
|
||
| error_log('API4: Fetched camps: ' . print_r($camps, TRUE)); | ||
|
|
||
| // Step 2: Extract unique processing center IDs | ||
| $processingCenterIds = array_unique(array_map(function ($camp) { | ||
| return $camp['Urban_Planned_Visit.Which_Goonj_Processing_Center_do_you_wish_to_visit_'] ?? null; | ||
| }, $camps)); | ||
| error_log('API4: Processing Center IDs: ' . print_r($processingCenterIds, TRUE)); | ||
|
|
||
| if (empty($processingCenterIds)) { | ||
| $clauses['id'][] = 'IN (null)'; | ||
| return TRUE; | ||
| } | ||
|
|
||
| // Step 3: Fetch states of processing center contacts as array | ||
| $addressesResult = \Civi\Api4\Address::get(FALSE) | ||
| ->addSelect('contact_id') | ||
| ->addSelect('state_province_id') | ||
| ->addWhere('contact_id', 'IN', $processingCenterIds) | ||
| ->execute(); | ||
|
|
||
| $addresses = iterator_to_array($addressesResult); // <-- Works too | ||
| error_log('API4: Fetched addresses: ' . print_r($addresses, TRUE)); | ||
|
|
||
| $contactStates = []; | ||
| foreach ($addresses as $addr) { | ||
| $contactStates[$addr['contact_id']] = $addr['state_province_id']; | ||
| } | ||
|
|
||
| error_log('ACL: Contact states: ' . print_r($contactStates, TRUE)); | ||
|
|
||
| // Step 4: Filter camps based on group-controlled states | ||
| $allowedCampIds = []; | ||
| foreach ($camps as $camp) { | ||
| $contactId = $camp['Urban_Planned_Visit.Which_Goonj_Processing_Center_do_you_wish_to_visit_'] ?? null; | ||
| $campId = $camp['id'] ?? null; | ||
|
|
||
| if ($contactId && $campId && isset($contactStates[$contactId]) && in_array($contactStates[$contactId], $statesControlled)) { | ||
| $allowedCampIds[] = $campId; | ||
| } | ||
| } | ||
|
|
||
| if (empty($allowedCampIds)) { | ||
| $clauses['id'][] = 'IN (null)'; | ||
| return TRUE; | ||
| } | ||
|
|
||
| error_log('ACL: Allowed camp IDs: ' . print_r($allowedCampIds, TRUE)); | ||
|
|
||
| // Step 5: Add allowed camp IDs to ACL clauses | ||
| if (!empty($allowedCampIds)) { | ||
| // Instead of pushing into array, directly set the clause string: | ||
| $clauses['id'] = ['IN (' . implode(',', array_map('intval', $allowedCampIds)) . ')']; | ||
| } else { | ||
| $clauses['id'] = ['IN (null)']; | ||
| } | ||
|
|
||
|
|
||
| return TRUE; | ||
|
|
||
| } catch (\Exception $e) { | ||
| \Civi::log()->warning("ACL: Unable to apply ACL on $entity for user $userId. " . $e->getMessage()); | ||
| error_log("ACL Exception: " . $e->getMessage()); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Restore correct ACL filtering for Collection Camps
When aclCollectionCamp fires for Eck_Collection_Camp, we still fetch Institution_Visit records and push their IDs into $clauses['id']. That mismatched dataset makes the collection-camp listings collapse to an empty result (unless camp IDs coincidentally match visit IDs), effectively blocking normal access. Please either fall back to the old branch for Eck_Collection_Camp or derive the allowed IDs from the camp tables before shipping.
🧰 Tools
🪛 PHPMD (2.15.0)
63-63: Avoid unused parameters such as '$conditions'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/CollectionBaseService.php around
lines 63 to 186, the code currently fetches Institution_Visit records and uses
their IDs for ACL when entity is Eck_Collection_Camp, producing a mismatch and
empty results; change the logic so that when $entity === 'Eck_Collection_Camp'
you either 1) use the original branch that queries the camp table
(Eck_Collection_Camp) to derive allowed camp IDs directly (filter by the
processing center/contact state mapping you already compute), or 2) if camps are
related to Institution_Visit via a specific foreign key, fetch Institution_Visit
-> map to the related camp IDs (follow that relation) and build $clauses['id']
from those camp IDs; keep the existing Institution_Visit flow only for entity
=== 'Eck_Institution_Visit', ensure you set $clauses['id'] to a single string
'IN (...)' of integer IDs (or 'IN (null)' when empty), and preserve error
handling/logging as-is.
Comment on lines
+1254
to
+1260
| 'InstituteMaterialContribution' => [ | ||
| 'title' => ts('Monetary Contribution'), | ||
| 'module' => 'afsearchAutofillInstitutionMaterialContributions', | ||
| 'directive' => 'afsearch-autofill-institution-material-contributions', | ||
| 'template' => 'CRM/Goonjcustom/Tabs/MonetaryContribution.tpl', | ||
| 'permissions' => ['goonj_chapter_admin', 'urbanops', 'urban_ops_admin', 's2s_ho_team', 'project_team_ho', 'project_team_chapter', 'urban_ops_and_accounts_chapter_team', 'mmt_and_accounts_chapter_team', 'project_ho_and_accounts'], | ||
| ], |
There was a problem hiding this comment.
Differentiate the new tab’s title
This adds a second tab labeled “Monetary Contribution,” so the UI now shows two indistinguishable tabs. Users won’t know which one surfaces Institute material data vs. the monetary view. Please rename the new tab (e.g., “Institute Material Contribution”) to keep the navigation clear.
🤖 Prompt for AI Agents
In
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php
around lines 1254 to 1260, the new tab is titled "Monetary Contribution" which
duplicates an existing tab; update the 'title' value to a distinct label such as
ts('Institute Material Contribution') (or another clear name) so users can
distinguish the institute material tab from the monetary view; keep the ts()
translation wrapper and leave the rest of the array unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Urban planned visit
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.