Assign group to individual for urban visit#679
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Form as AfformSubmission
participant Service as UrbanPlannedVisitService
participant GroupAPI as CiviCRM Group API
Form ->> Service: Trigger pre-hook event
Service ->> Service: Validate submission
Service ->> Service: Extract contact & state info
Service ->> GroupAPI: Query chapter group
GroupAPI -->> Service: Return group ID
Service ->> GroupAPI: Create GroupContact association
Possibly Related PRs
Poem
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: 2
🧹 Nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
330-353: Method looks good—just keep an eye on concurrency.
The fallback approach is solid, but if multiple states return different fallback groups, ensure that concurrency or parallel form submissions won’t cause group duplication or mismatched fallback. Logging the group assignment is helpful for traceability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(3 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (3)
6-7: Great use of the new Group and GroupContact imports!
These imports ensure the necessary naming references for grouping operations.
27-27: Event subscription is clear!
Hooking the new method here is a straightforward approach. No further changes needed.
292-325: Consider verifying the structure of $objectRef['data'].
Although you verify 'Eck_Institution_Visit1' at line 300, there's no similar check for 'Individual1'. If 'Individual1' is missing or not an array, it could lead to errors in the foreach. Safely handle or verify the structure for both slices of data.
| /** | ||
| * | ||
| */ | ||
| public static function assignChapterGroupToIndividualForUrbanPlannedVisit(string $op, string $objectName, $objectId, &$objectRef) { | ||
| if ($op !== 'edit' || $objectName !== 'AfformSubmission') { | ||
| return FALSE; | ||
| } | ||
|
|
||
| if (empty($objectRef['data']['Eck_Institution_Visit1'])) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| $individualData = $objectRef['data']['Individual1']; | ||
| $visitData = $objectRef['data']['Eck_Institution_Visit1']; | ||
|
|
||
| foreach ($individualData as $individual) { | ||
| $contactId = $individual['id'] ?? NULL; | ||
| } | ||
|
|
||
| foreach ($visitData as $visit) { | ||
| $fields = $visit['fields'] ?? []; | ||
| $stateProvinceId = $fields['Urban_Planned_Visit.State'] ?? NULL; | ||
| } | ||
|
|
||
| $groupId = self::getChapterGroupForState($stateProvinceId); | ||
|
|
||
| if ($groupId & $contactId) { | ||
| GroupContact::create(FALSE) | ||
| ->addValue('contact_id', $contactId) | ||
| ->addValue('group_id', $groupId) | ||
| ->addValue('status', 'Added') | ||
| ->execute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Process each contact from $individualData.
In the foreach loop starting at line 307, the code overwrites $contactId in each iteration but only uses that value once outside the loop (lines 316–324). If multiple contacts exist in $individualData, only the last one is processed. Consider moving the group assignment operation inside the loop or storing all contact IDs for subsequent group assignment.
Watch out for the bitwise operator &.
The condition uses $groupId & $contactId, which performs a bitwise AND rather than a logical check for both conditions being true. This is likely a bug and should be replaced with && for a correct boolean evaluation.
You can fix it with the following diff:
-if ($groupId & $contactId) {
+if ($groupId && $contactId) {
GroupContact::create(FALSE)
->addValue('contact_id', $contactId)
->addValue('group_id', $groupId)
->addValue('status', 'Added')
->execute();
}📝 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.
| /** | |
| * | |
| */ | |
| public static function assignChapterGroupToIndividualForUrbanPlannedVisit(string $op, string $objectName, $objectId, &$objectRef) { | |
| if ($op !== 'edit' || $objectName !== 'AfformSubmission') { | |
| return FALSE; | |
| } | |
| if (empty($objectRef['data']['Eck_Institution_Visit1'])) { | |
| return FALSE; | |
| } | |
| $individualData = $objectRef['data']['Individual1']; | |
| $visitData = $objectRef['data']['Eck_Institution_Visit1']; | |
| foreach ($individualData as $individual) { | |
| $contactId = $individual['id'] ?? NULL; | |
| } | |
| foreach ($visitData as $visit) { | |
| $fields = $visit['fields'] ?? []; | |
| $stateProvinceId = $fields['Urban_Planned_Visit.State'] ?? NULL; | |
| } | |
| $groupId = self::getChapterGroupForState($stateProvinceId); | |
| if ($groupId & $contactId) { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| } | |
| } | |
| /** | |
| * | |
| */ | |
| public static function assignChapterGroupToIndividualForUrbanPlannedVisit(string $op, string $objectName, $objectId, &$objectRef) { | |
| if ($op !== 'edit' || $objectName !== 'AfformSubmission') { | |
| return FALSE; | |
| } | |
| if (empty($objectRef['data']['Eck_Institution_Visit1'])) { | |
| return FALSE; | |
| } | |
| $individualData = $objectRef['data']['Individual1']; | |
| $visitData = $objectRef['data']['Eck_Institution_Visit1']; | |
| foreach ($individualData as $individual) { | |
| $contactId = $individual['id'] ?? NULL; | |
| } | |
| foreach ($visitData as $visit) { | |
| $fields = $visit['fields'] ?? []; | |
| $stateProvinceId = $fields['Urban_Planned_Visit.State'] ?? NULL; | |
| } | |
| $groupId = self::getChapterGroupForState($stateProvinceId); | |
| if ($groupId && $contactId) { | |
| GroupContact::create(FALSE) | |
| ->addValue('contact_id', $contactId) | |
| ->addValue('group_id', $groupId) | |
| ->addValue('status', 'Added') | |
| ->execute(); | |
| } | |
| } |
1 participant
Assign group to individual for urban visit
Summary by CodeRabbit
New Features
Bug Fixes