Add ability to share rooms with roles (groups)#2799
Add ability to share rooms with roles (groups)#2799hassanalitamam wants to merge 2 commits intoTHM-Health:developfrom
Conversation
Implement role-based room membership allowing room owners to share rooms with entire roles instead of only individual users. All users belonging to a shared role automatically gain access at the specified permission level (participant, moderator, or co-owner). - Add room_role pivot table and RoomRole pivot model - Update Room model with role-based membership checks - Add RoomRoleMemberController with CRUD API endpoints - Update room listing filter to include role-shared rooms - Add frontend components for role member management - Add translations for en, de, de-gender, fr, fa
WalkthroughAdds role-based room membership: DB pivot, models, API endpoints and validation, Room membership logic updates, new Vue components for managing role members, and localization strings across locales. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/api/v1/RoomRoleMemberController.php`:
- Around line 72-81: The code calls Role::find($request->role_id) after
attaching and then uses $role->getLogLabel(), which can be null if the role was
deleted concurrently; update the store method to avoid NPE by fetching the Role
safely before using it (e.g., replace Role::find(...) with
Role::findOrFail($request->role_id) or fetch the Role prior to calling
$room->roleMembers()->attach(...)), or guard the log call with null-safe
behavior (use $role?->getLogLabel() or a fallback label) so getLogLabel() is
never invoked on null; adjust the Log::info call accordingly (references: store,
roleMembers()->attach, Role::find, getLogLabel, RoomUserRole::from).
- Around line 39-43: The filter uses wherePivot with fully-qualified column
names causing double-qualification; in RoomRoleMemberController update the
$filter entries to use the bare pivot column name 'role' instead of
'room_role.role' (keep the corresponding RoomUserRole::USER, ::MODERATOR,
::CO_OWNER values) so wherePivot receives just 'role' and not a dotted name;
ensure the rest of the code that applies $filter (the wherePivot call) remains
unchanged and works with the bare column name.
In `@resources/js/components/RoomTabRoleMembers.vue`:
- Around line 69-74: The permission check used in RoomTabRoleMembers.vue is
mismatched: replace calls to userPermissions.can('manageSettings', props.room)
with userPermissions.can('manageMembers', props.room) so the frontend matches
the backend authorization; update all occurrences around the role member UI
(e.g., the RoomTabRoleMembersAddModal v-if ref="addModal" and other buttons
gated by userPermissions.can in this component) to use 'manageMembers' and
ensure the `@added`="loadData()" behavior remains unchanged.
In `@resources/js/components/RoomTabRoleMembersDeleteButton.vue`:
- Around line 99-107: In the catch block for the delete request in
RoomTabRoleMembersDeleteButton.vue, avoid calling api.error() after you've
already handled HTTP 410: detect when error.response.status === env.HTTP_GONE
(the branch that calls emit("gone") and sets modalVisible.value = false) and
return early from the catch handler so api.error(error, {
redirectOnUnauthenticated: false }) is not invoked for that case; update the
catch to short-circuit after the 410 handling so only unhandled errors reach
api.error.
In `@resources/js/components/RoomTabRoleMembersEditButton.vue`:
- Around line 160-172: In the catch handler inside
RoomTabRoleMembersEditButton.vue, after detecting a 410 (env.HTTP_GONE) and
calling emit("gone") and setting modalVisible.value = false, add an early return
to stop execution so the fallback api.error(error, { redirectOnUnauthenticated:
false }) is not invoked for "gone" responses; update the catch flow around the
error.response.status === env.HTTP_GONE branch to return immediately after
handling the gone case.
🧹 Nitpick comments (5)
database/migrations/2026_02_07_000001_create_room_role_table.php (1)
16-22: Consider the naming ambiguity of therolecolumn.The pivot table
room_rolelinks rooms to roles (groups), but therolecolumn stores the permission level (participant/moderator/co-owner). This dual use of "role" — both as the group entity and as the permission level — could be confusing for maintainers. A name likepermissionorroom_user_rolewould be clearer.This is a minor naming concern — feel free to defer if the rest of the codebase consistently uses
rolefor permission level in this context (as it does inroom_user).app/Models/Room.php (1)
286-306: Potential N+1 concern:getRoleMembershipRoleis called in each ofisModerator,isCoOwner,isMember, andgetRole.When rendering the Room resource, all four methods are invoked for the same user, each triggering an independent DB query inside
getRoleMembershipRole. Consider caching the result per-user on the model instance (e.g., a simple memoization map) to avoid redundant queries.♻️ Suggested memoization approach
+ /** `@var` array<int, ?RoomUserRole> */ + protected array $roleMembershipCache = []; + protected function getRoleMembershipRole(?User $user): ?RoomUserRole { if ($user === null) { return null; } + if (array_key_exists($user->id, $this->roleMembershipCache)) { + return $this->roleMembershipCache[$user->id]; + } + $userRoleIds = $user->roles()->pluck('roles.id'); if ($userRoleIds->isEmpty()) { + $this->roleMembershipCache[$user->id] = null; return null; } $highestRoleMember = $this->roleMembers() ->whereIn('role_id', $userRoleIds) ->orderByDesc('room_role.role') ->first(); - return $highestRoleMember?->pivot->role; + $result = $highestRoleMember?->pivot->role; + $this->roleMembershipCache[$user->id] = $result; + return $result; }app/Http/Controllers/api/v1/RoomRoleMemberController.php (1)
28-30: Thematchforsort_byonly has adefaultbranch — consider validating or documenting supported sort columns.Currently any value for
sort_bysilently falls back toLOWER(name). If more sort options are planned (as the match structure suggests), this is fine scaffolding. Otherwise, a simple hardcoded value would be clearer.resources/js/components/RoomTabRoleMembersAddModal.vue (2)
145-164: Fetching all roles withper_page: 9999— consider documenting or capping this.Loading every role in a single request works for small-to-medium installations, but could become a problem at scale. If the roles API supports returning all items without pagination (e.g., a dedicated unpaginated endpoint or a known sentinel), prefer that. Otherwise, this is an acceptable pragmatic choice — just be aware of the ceiling.
69-109: Duplicate radio-button group across Add and Edit components.The three-option role radio group (participant / moderator / co-owner) with
RoomRoleBadgelabels is copy-pasted betweenRoomTabRoleMembersAddModal.vue(Lines 69-109) andRoomTabRoleMembersEditButton.vue(Lines 52-89). Consider extracting a sharedRoomRoleSelectorcomponent to reduce duplication.Also applies to: 52-89
| $filter = match ($request->query('filter')) { | ||
| 'participant_role' => ['room_role.role', RoomUserRole::USER], | ||
| 'moderator_role' => ['room_role.role', RoomUserRole::MODERATOR], | ||
| 'co_owner_role' => ['room_role.role', RoomUserRole::CO_OWNER], | ||
| default => null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/Http/Controllers/api/v1/RoomRoleMemberController.php | head -100Repository: THM-Health/PILOS
Length of output: 4225
🏁 Script executed:
git ls-files | grep -i composerRepository: THM-Health/PILOS
Length of output: 113
🏁 Script executed:
rg -n 'belongsToMany' app/Http/Controllers/api/v1/RoomRoleMemberController.php -B5 -A10Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
rg -n "wherePivot" --type php -B2 -A2Repository: THM-Health/PILOS
Length of output: 2074
🏁 Script executed:
cat composer.json | grep -A 5 -B 5 "laravel/framework"Repository: THM-Health/PILOS
Length of output: 408
🏁 Script executed:
# Verify the Room model's relationship definition to confirm the pivot table name
rg -n 'roleMembers' app/Models/Room.php -A5Repository: THM-Health/PILOS
Length of output: 485
🏁 Script executed:
# Check how wherePivot handles column qualification in Laravel 12.3
# by looking at Laravel's source or related documentation
cat composer.lock | grep -A 10 '"name": "laravel/framework"' | head -20Repository: THM-Health/PILOS
Length of output: 592
🏁 Script executed:
# Check the database schema to understand the pivot table structure
rg -n 'room_role' database/migrations -t php -A5 -B5 | head -50Repository: THM-Health/PILOS
Length of output: 1953
🏁 Script executed:
# Look for any other usages of wherePivot with table-prefixed columns
rg -n "wherePivot\(['\"]" --type phpRepository: THM-Health/PILOS
Length of output: 650
🌐 Web query:
Laravel 12 wherePivot column qualification behavior
💡 Result:
In Laravel 12, BelongsToMany::wherePivot() is intended for pivot-table columns and internally uses pivot column qualification (via qualifyPivotColumn) so unqualified column names are scoped to the pivot/intermediate table (i.e., pivot_table.column) to avoid ambiguity in the joined query. It also accepts a Query Expression, which is the escape hatch when you don’t want Laravel to treat/qualify the input as a plain pivot column name. [1], [2]
Practical implications:
->wherePivot('approved', 1)targets the pivot table’sapprovedcolumn (not the related model’s column) and will be qualified to the pivot table in SQL. [1], [2]- If you pass something already “non-simple” (e.g., an
Expression), Laravel won’t “string-qualify” it like a normal column name—use this when you need custom SQL / explicit qualification. [2]
Sources: Laravel 12 relationship docs for pivot filtering (wherePivot*) [1] and Laravel 12 API docs showing wherePivot(string|Expression $column, …) plus qualifyPivotColumn(string|Expression $column) [2].
[1] (laravel.com)
[2] (api.laravel.com)
Citations:
- 1: https://laravel.com/docs/12.x/eloquent-relationships.?utm_source=openai
- 2: https://api.laravel.com/docs/12.x/Illuminate/Database/Eloquent/Relations/BelongsToMany.html?utm_source=openai
🏁 Script executed:
# Check if there are any test files or error logs related to this functionality
find . -name "*test*.php" -type f | xargs grep -l "wherePivot" 2>/dev/null | head -5Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
# Look at the RoomRole model to understand the pivot structure better
cat -n app/Models/RoomRole.php 2>/dev/null | head -50Repository: THM-Health/PILOS
Length of output: 414
🏁 Script executed:
# Verify the exact Laravel code for wherePivot and qualifyPivotColumn behavior
# by checking if there are any tests that show expected usage
find . -name "*Test*.php" -o -name "*test.php" | xargs grep -l "wherePivot" 2>/dev/nullRepository: THM-Health/PILOS
Length of output: 44
🏁 Script executed:
# Search for any error logs or test database files that might show this bug in action
find . -name "*.log" -o -name "*.sqlite" 2>/dev/null | head -5Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
# Double-check by looking at how the filter is being constructed vs how wherePivot is called elsewhere
echo "=== Filter construction in RoomRoleMemberController ===" && \
sed -n '39,62p' app/Http/Controllers/api/v1/RoomRoleMemberController.php && \
echo -e "\n=== Other wherePivot calls (correct pattern) ===" && \
rg "wherePivot\('role'" app/Models/Room.php -B1 -A1Repository: THM-Health/PILOS
Length of output: 1501
Bug: wherePivot() will double-qualify the column name—use bare column names only.
wherePivot() internally calls qualifyPivotColumn(), which automatically prepends the pivot table name. Passing 'room_role.role' results in the incorrect qualified name room_role.room_role.role, producing a SQL error. Compare with correct usage elsewhere in the codebase (e.g., Room.php lines 300, 318), which pass just 'role'. The filter keys should be the bare column name.
🐛 Proposed fix
// Filter by role, fallback/default is no filter
$filter = match ($request->query('filter')) {
- 'participant_role' => ['room_role.role', RoomUserRole::USER],
- 'moderator_role' => ['room_role.role', RoomUserRole::MODERATOR],
- 'co_owner_role' => ['room_role.role', RoomUserRole::CO_OWNER],
+ 'participant_role' => ['role', RoomUserRole::USER],
+ 'moderator_role' => ['role', RoomUserRole::MODERATOR],
+ 'co_owner_role' => ['role', RoomUserRole::CO_OWNER],
default => null,
};🤖 Prompt for AI Agents
In `@app/Http/Controllers/api/v1/RoomRoleMemberController.php` around lines 39 -
43, The filter uses wherePivot with fully-qualified column names causing
double-qualification; in RoomRoleMemberController update the $filter entries to
use the bare pivot column name 'role' instead of 'room_role.role' (keep the
corresponding RoomUserRole::USER, ::MODERATOR, ::CO_OWNER values) so wherePivot
receives just 'role' and not a dotted name; ensure the rest of the code that
applies $filter (the wherePivot call) remains unchanged and works with the bare
column name.
| public function store(Room $room, AddRoomRoleMember $request) | ||
| { | ||
| $room->roleMembers()->attach($request->role_id, ['role' => $request->role]); | ||
|
|
||
| $role = Role::find($request->role_id); | ||
|
|
||
| Log::info('Added role {role} with room role {roomRole} to room {room}', ['room' => $room->getLogLabel(), 'roomRole' => RoomUserRole::from($request->role)->label(), 'role' => $role->getLogLabel()]); | ||
|
|
||
| return response()->noContent(); | ||
| } |
There was a problem hiding this comment.
Role::find() can return null in a race condition, causing an NPE on getLogLabel().
If the role is deleted between the attach (Line 74) and find (Line 76), $role is null and $role->getLogLabel() throws a fatal error. Use findOrFail or reuse the role from the relation.
Proposed fix
- $role = Role::find($request->role_id);
+ $role = Role::findOrFail($request->role_id);🤖 Prompt for AI Agents
In `@app/Http/Controllers/api/v1/RoomRoleMemberController.php` around lines 72 -
81, The code calls Role::find($request->role_id) after attaching and then uses
$role->getLogLabel(), which can be null if the role was deleted concurrently;
update the store method to avoid NPE by fetching the Role safely before using it
(e.g., replace Role::find(...) with Role::findOrFail($request->role_id) or fetch
the Role prior to calling $room->roleMembers()->attach(...)), or guard the log
call with null-safe behavior (use $role?->getLogLabel() or a fallback label) so
getLogLabel() is never invoked on null; adjust the Log::info call accordingly
(references: store, roleMembers()->attach, Role::find, getLogLabel,
RoomUserRole::from).
| <RoomTabRoleMembersAddModal | ||
| v-if="userPermissions.can('manageSettings', props.room)" | ||
| ref="addModal" | ||
| :room-id="props.room.id" | ||
| @added="loadData()" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "manageSettings|manageMembers" --type vue --type js -g '*.vue' -A 2 -B 2 | head -100Repository: THM-Health/PILOS
Length of output: 87
🏁 Script executed:
rg -n "manageMembers|manageSettings" resources/ -t php -A 3 -B 1 | head -150Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
fd -e php -path "*RoomPolicy*" -o -path "*routes*" | head -20Repository: THM-Health/PILOS
Length of output: 228
🏁 Script executed:
rg "manageSettings|manageMembers" -A 2 -B 1 | head -200Repository: THM-Health/PILOS
Length of output: 13707
🏁 Script executed:
fd "RoomPolicy" --type fRepository: THM-Health/PILOS
Length of output: 123
🏁 Script executed:
fd "routes" --type f | grep -E "php|js" | head -10Repository: THM-Health/PILOS
Length of output: 98
🏁 Script executed:
find resources/js/components -name "*Room*" -type f | head -20Repository: THM-Health/PILOS
Length of output: 1000
🏁 Script executed:
cat -n app/Policies/RoomPolicy.php | grep -A 5 -B 2 "manageSettings"Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
rg "manageSettings" app/Repository: THM-Health/PILOS
Length of output: 42
🏁 Script executed:
cat app/Policies/RoomPolicy.phpRepository: THM-Health/PILOS
Length of output: 6605
🏁 Script executed:
cat resources/js/policies/RoomPolicy.jsRepository: THM-Health/PILOS
Length of output: 2321
Fix permission check mismatch: Use manageMembers instead of manageSettings for role member operations.
The API routes for role member operations (add, edit, delete) use the can:manageMembers,room middleware (routes/api.php), but the UI gates these buttons with userPermissions.can('manageSettings', props.room) (lines 70 and 154). These are different permissions with different names in the authorization policy. While both currently check equivalent conditions (owner, co-owner, or rooms.manage), the mismatch between frontend and backend permission names creates architectural confusion and potential security issues. Change the frontend permission check to match the backend: userPermissions.can('manageMembers', props.room).
🤖 Prompt for AI Agents
In `@resources/js/components/RoomTabRoleMembers.vue` around lines 69 - 74, The
permission check used in RoomTabRoleMembers.vue is mismatched: replace calls to
userPermissions.can('manageSettings', props.room) with
userPermissions.can('manageMembers', props.room) so the frontend matches the
backend authorization; update all occurrences around the role member UI (e.g.,
the RoomTabRoleMembersAddModal v-if ref="addModal" and other buttons gated by
userPermissions.can in this component) to use 'manageMembers' and ensure the
`@added`="loadData()" behavior remains unchanged.
| .catch((error) => { | ||
| if (error.response) { | ||
| if (error.response.status === env.HTTP_GONE) { | ||
| emit("gone"); | ||
| modalVisible.value = false; | ||
| } | ||
| } | ||
| api.error(error, { redirectOnUnauthenticated: false }); | ||
| }) |
There was a problem hiding this comment.
410 errors are handled twice — api.error() still fires after the specific 410 handling.
When the server returns HTTP 410, the code emits gone and closes the modal (lines 101-103), but execution continues to line 106 where api.error() is also called. This likely shows a generic error notification to the user even though the 410 was already handled gracefully.
Proposed fix — return early for handled 410
.catch((error) => {
if (error.response) {
if (error.response.status === env.HTTP_GONE) {
emit("gone");
modalVisible.value = false;
+ return;
}
}
api.error(error, { redirectOnUnauthenticated: false });
})📝 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.
| .catch((error) => { | |
| if (error.response) { | |
| if (error.response.status === env.HTTP_GONE) { | |
| emit("gone"); | |
| modalVisible.value = false; | |
| } | |
| } | |
| api.error(error, { redirectOnUnauthenticated: false }); | |
| }) | |
| .catch((error) => { | |
| if (error.response) { | |
| if (error.response.status === env.HTTP_GONE) { | |
| emit("gone"); | |
| modalVisible.value = false; | |
| return; | |
| } | |
| } | |
| api.error(error, { redirectOnUnauthenticated: false }); | |
| }) |
🤖 Prompt for AI Agents
In `@resources/js/components/RoomTabRoleMembersDeleteButton.vue` around lines 99 -
107, In the catch block for the delete request in
RoomTabRoleMembersDeleteButton.vue, avoid calling api.error() after you've
already handled HTTP 410: detect when error.response.status === env.HTTP_GONE
(the branch that calls emit("gone") and sets modalVisible.value = false) and
return early from the catch handler so api.error(error, {
redirectOnUnauthenticated: false }) is not invoked for that case; update the
catch to short-circuit after the 410 handling so only unhandled errors reach
api.error.
| .catch((error) => { | ||
| if (error.response) { | ||
| if (error.response.status === env.HTTP_GONE) { | ||
| emit("gone"); | ||
| modalVisible.value = false; | ||
| } | ||
| if (error.response.status === env.HTTP_UNPROCESSABLE_ENTITY) { | ||
| formErrors.set(error.response.data.errors); | ||
| return; | ||
| } | ||
| } | ||
| api.error(error, { redirectOnUnauthenticated: false }); | ||
| }) |
There was a problem hiding this comment.
Missing return after handling HTTP 410 — the fallback api.error() fires for "gone" responses too.
When the server returns 410, the code emits "gone" and closes the modal but does not return. Execution falls through to api.error(error, ...) on Line 171, which will display an additional (unwanted) error notification to the user.
🐛 Proposed fix
if (error.response) {
if (error.response.status === env.HTTP_GONE) {
emit("gone");
modalVisible.value = false;
+ return;
}
if (error.response.status === env.HTTP_UNPROCESSABLE_ENTITY) {
formErrors.set(error.response.data.errors);
return;
}
}📝 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.
| .catch((error) => { | |
| if (error.response) { | |
| if (error.response.status === env.HTTP_GONE) { | |
| emit("gone"); | |
| modalVisible.value = false; | |
| } | |
| if (error.response.status === env.HTTP_UNPROCESSABLE_ENTITY) { | |
| formErrors.set(error.response.data.errors); | |
| return; | |
| } | |
| } | |
| api.error(error, { redirectOnUnauthenticated: false }); | |
| }) | |
| .catch((error) => { | |
| if (error.response) { | |
| if (error.response.status === env.HTTP_GONE) { | |
| emit("gone"); | |
| modalVisible.value = false; | |
| return; | |
| } | |
| if (error.response.status === env.HTTP_UNPROCESSABLE_ENTITY) { | |
| formErrors.set(error.response.data.errors); | |
| return; | |
| } | |
| } | |
| api.error(error, { redirectOnUnauthenticated: false }); | |
| }) |
🤖 Prompt for AI Agents
In `@resources/js/components/RoomTabRoleMembersEditButton.vue` around lines 160 -
172, In the catch handler inside RoomTabRoleMembersEditButton.vue, after
detecting a 410 (env.HTTP_GONE) and calling emit("gone") and setting
modalVisible.value = false, add an early return to stop execution so the
fallback api.error(error, { redirectOnUnauthenticated: false }) is not invoked
for "gone" responses; update the catch flow around the error.response.status ===
env.HTTP_GONE branch to return immediately after handling the gone case.
|
Thank you for your PR! Here are a few notes regarding this PR
In addition we have to take care of the roles the user get's assigned for each of these options. Instead of just adding a second table on the "Room members" Tab we should instead work a broader concept.
In that context it might make sense to organise groups in a tree. Sharing a room with a higher leaf (group) in the tree shares it with all leafs below.
|
2 participants
Summary
Implements Feature Request #2190 — allows room owners to share rooms with entire roles (groups) instead of only individual users.
room_rolepivot table following the existingroom_userpatternisMember(),isModerator(),isCoOwner(),getRole()now check both individual and role-based membership, with highest role winningAPI Endpoints
rooms/{room}/role-memberrooms/{room}/role-memberrooms/{room}/role-member/{role}rooms/{room}/role-member/{role}Precedence
When a user has both individual and role-based membership, the highest permission level wins.
Closes #2190
Test plan
php artisan migrate— verifyroom_roletable is createdphp artisan test— all existing tests passnpm run build— frontend builds without errorsSummary by CodeRabbit
New Features
Behavior Changes
Documentation / Localization