Add group membership and owner management#275
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds full group member and owner management: new backend endpoints with authorization and logging, expanded UsersApiClient methods, a User.isGroupOwner helper, frontend mutation hooks, and an interactive GroupPage UI for adding/removing members and owners. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (UI)
participant GP as GroupPage
participant Mut as useAddMemberMutation
participant FEAPI as Frontend API (fetch)
participant BE as Backend API (HTTP)
participant GC as GroupController
participant Client as UsersApiClient
participant DB as Users Service / DB
participant QC as Query Cache
U->>GP: choose member type/id and submit "Add"
GP->>Mut: mutate({ memberType, memberId })
Mut->>FEAPI: PUT /group/{groupName}/members
FEAPI->>BE: HTTP request
BE->>GC: GroupController::addMember(request, groupName)
GC->>GC: canManageGroup() authorization
alt forbidden
GC-->>BE: 403 Forbidden
BE-->>Mut: 403 response
Mut-->>GP: show error flash
else allowed
GC->>Client: addMemberToGroup(groupName, memberType, memberId)
Client->>DB: delegate to users API (v2/groups/...)
DB-->>Client: 200 OK
Client-->>GC: success
GC->>GC: log admin action
GC-->>BE: 200 OK
BE-->>Mut: success
Mut->>QC: invalidate group & current-user queries
Mut-->>GP: show success flash
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/src/Auth/UsersApiClient.php`:
- Around line 106-109: Remove the dead method removeUserFromGroup from the
UsersApiClient class since it has no callers; delete the public function
removeUserFromGroup(string $groupname, string $username) entirely, or if you
intend to keep it for a future feature, leave the method but add a clear
explanatory comment above it describing the planned use case and why it is
retained (including ticket/issue reference) so reviewers know it is intentional.
🧹 Nitpick comments (5)
backend/app/src/Auth/User.php (1)
298-308: Defensive access ongroupsowner_relationwould improve robustness.Line 306 already accesses
groups_relationwithout a guard (pre-existing), so this is consistent. However, if the external Users API ever returns a response missinggroupsowner_relation, line 307 will trigger an undefined-index warning/error.Consider adding a null-coalescing fallback or an
issetcheck, especially since this data comes from an external API:Proposed fix
$this->group_relations = $response->body['result']['groups_relation']; - $this->groupowner_relations = $response->body['result']['groupsowner_relation']; + $this->groupowner_relations = $response->body['result']['groupsowner_relation'] ?? [];backend/app/Http/Controllers/API/GroupController.php (2)
40-68: Consider validating the group exists before calling the upstream API.All four endpoints forward the
groupNamedirectly toUsersApiClientwithout first verifying that the group exists locally (e.g., viaGroup::find($groupName)). If a non-existent group name is provided, the upstream API will fail and the user will get a generic server error ("Kunne ikke legge til medlem.") instead of a clear 404. The existingshowmethod already does this check (line 24-27).This is minor since the upstream API should reject invalid groups anyway, but a local check would give clearer error messages.
Also applies to: 70-95, 97-125, 127-152
40-68: Duplicate structure across the four action methods — consider extracting a helper.The
addMember,removeMember,addOwner, andremoveOwnermethods follow an identical pattern: authorize → validate → call API → check response → log → return. A private helper could reduce this duplication.This is purely a maintainability suggestion and can be deferred.
Also applies to: 97-125
frontend/src/modules/groups/GroupPage.tsx (2)
354-365: NewSetobjects created on every render.
excludeUsersandexcludeGroupsare reconstructed on each render ofDetail. In practice, with typical group sizes this is negligible, but if you want to avoid unnecessary re-renders ofAddEntityForm, wrap these inuseMemo.Not a blocker — just a note for awareness.
Also applies to: 405-416
160-160: ThesetTimeoutblur-close pattern is fragile but functional.The 200ms delay before closing the dropdown on blur is a well-known workaround. It works because
onMouseDownwithpreventDefaultinSearchDropdownfires before blur. This is fine for now, but if you ever encounter intermittent issues with dropdown items not registering, consider a ref-based approach checking if the related target is within the dropdown.
Summary
🤖 Generated with Claude Code