admin setting to hide room owner on join of unauthenticated users#2844
admin setting to hide room owner on join of unauthenticated users#2844danielwujecki wants to merge 7 commits intoTHM-Health:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
WalkthroughAdds a new RoomSettings boolean (hide_owner_for_guests) with migration, admin UI toggle, validation, translations, conditional owner visibility in Room responses and UI, and corresponding backend and frontend tests and changelog entry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Thanks for your PR @danielwujecki This fix is only hiding the name (visually), I would prefer hiding it from the API response. The frontend can check if the owner name is returned and show it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/api/v1/SettingsController.php`:
- Line 186: The assignment to RoomSettings::$hide_owner_for_guests uses
$request->input(), which can return raw strings and not reliably cast to a PHP
bool; replace the call with $request->boolean('room_hide_owner_for_guests') in
the SettingsController where $roomSettings->hide_owner_for_guests is set so the
value is properly converted to a boolean consistent with the other boolean
fields.
In `@CHANGELOG.md`:
- Around line 8-11: The entry "Add admin setting to hide room owner on join of
unauthenticated users" under the "[Unreleased]" section is missing a category
header; add a "### Added" header directly beneath "## [Unreleased]" and place
that bullet under it so the changelog follows the Keep a Changelog format
(locate the "[Unreleased]" section and the line containing the bullet to move
it).
In `@lang/ar/admin.php`:
- Line 391: The translation for the 'room_hide_owner_for_guests' key is still in
English; replace its value in the lang/ar/admin.php localization array with the
Arabic translation (e.g., "إخفاء مالك الغرفة عن المستخدمين غير المسجلين") so the
string matches the rest of the Arabic locale and preserves the same array key.
In `@lang/fa/admin.php`:
- Line 389: Replace the English value for the translation key
'room_hide_owner_for_guests' with a Persian translation so the Farsi locale is
consistent; update the value for 'room_hide_owner_for_guests' in the
lang/fa/admin.php translations to a proper Farsi string (e.g., "مخفی کردن مالک
اتاق از کاربران احراز هویت نشده").
|
Please remove all non-english locale changes, as they will be removed. All non-english locales are synced from PoEditor (one-way) |
danielwujecki
force-pushed
the
2843-admin-setting-hide-room-owner-for-guests
branch
from
0e80acb to
8fc332c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/components/RoomDetailsList.vue (1)
7-14:⚠️ Potential issue | 🟠 MajorFrontend-only hiding is insufficient — owner data is still exposed in the API response.
As noted by reviewer samuelwei, the owner's name is still returned by the API and visible in network responses / browser dev tools. The backend should omit
owner.namefrom the API response for unauthenticated users when the setting is enabled, and the frontend should conditionally render based on whether the field is present in the response rather than a separate flag.This approach would also simplify this component — no need for the
showRoomOwnerprop if the API simply doesn't return the owner data.
🤖 Fix all issues with AI agents
In `@lang/de/admin.php`:
- Line 391: Remove the manual non-English locale entry by deleting the
'room_hide_owner_for_guests' => 'Eigentümer eines Raumes für Gäste verstecken'
array element from lang/de/admin.php so translations remain managed through
PoEditor; ensure no other manual German locale keys were added in the same file
and leave only keys that are synced from the canonical source or empty
placeholders if required by the loader.
danielwujecki
force-pushed
the
2843-admin-setting-hide-room-owner-for-guests
branch
from
8fc332c to
719404b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Http/Resources/Room.php (1)
86-97: Good approach: owner data is properly excluded from the API response.This correctly addresses the reviewer feedback to remove the owner from the API response rather than just hiding it in the frontend.
One minor note: the array on lines 95–96 is eagerly evaluated even when
$showOwneris false. Consider using a closure to defer evaluation:♻️ Suggested improvement
- 'owner' => $this->when($showOwner, [ - 'id' => $this->owner->id, - 'name' => $this->owner->fullname, - ]), + 'owner' => $this->when($showOwner, fn () => [ + 'id' => $this->owner->id, + 'name' => $this->owner->fullname, + ]),Since the
ownerrelationship is likely already eager-loaded, this is a minor optimization, but it avoids unnecessary property access when the value will be discarded.
Done
Good point, I improved this. |
|
Please also run the code style fixers, see https://thm-health.github.io/PILOS/docs/development/code-style |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2844 +/- ##
=============================================
+ Coverage 96.77% 97.27% +0.50%
=============================================
Files 440 177 -263
Lines 12479 6277 -6202
Branches 2073 2074 +1
=============================================
- Hits 12076 6106 -5970
+ Misses 403 171 -232 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/Http/Resources/Room.php (1)
94-97: Use a Closure for lazy owner evaluation insidewhen().PHP evaluates the array literal
['id' => $this->owner->id, ...]eagerly before passing it towhen(). When$showOwnerisfalse,$this->owner->idis still accessed (and any lazy-loaded relationship query is still fired), even though the value is immediately discarded. Wrapping in a Closure defers evaluation until the condition istrue, which is the intended behaviour and the idiomatic Laravel pattern.♻️ Proposed refactor
- 'owner' => $this->when($showOwner, [ - 'id' => $this->owner->id, - 'name' => $this->owner->fullname, - ]), + 'owner' => $this->when($showOwner, fn () => [ + 'id' => $this->owner->id, + 'name' => $this->owner->fullname, + ]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Resources/Room.php` around lines 94 - 97, The 'owner' array inside Room resource's when() is being evaluated eagerly causing $this->owner (and its lazy-loaded relation) to be accessed even when $showOwner is false; change the second argument to when() to a Closure (use function() or fn() that returns ['id' => $this->owner->id, 'name' => $this->owner->fullname]) so the $this->owner->id and $this->owner->fullname are only evaluated when $showOwner is true.tests/Frontend/e2e/RoomsViewGeneral.cy.js (1)
2109-2129:cy.interceptRoomFilesRequest()placed inside the fixture callback — inconsistent with the rest of the file.Every other call to
cy.interceptRoomFilesRequest()in this file is at the top-level of the test body (e.g., Lines 11, 439, 492). Placing it inside.then()works due to Cypress's command-queue semantics but can mislead readers into thinking intercept setup depends on the fixture contents.♻️ Suggested normalisation
it("hide room owner if not provided", function () { + cy.interceptRoomFilesRequest(); + cy.fixture("room.json").then((room) => { - cy.interceptRoomFilesRequest(); - delete room.data.owner; cy.intercept("GET", "api/v1/rooms/abc-def-123", { statusCode: 200, body: room, }).as("roomRequest"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Frontend/e2e/RoomsViewGeneral.cy.js` around lines 2109 - 2129, Move the cy.interceptRoomFilesRequest() call out of the cy.fixture("room.json").then(...) callback and place it at the top-level of the test body (before calling cy.intercept("GET", "api/v1/rooms/abc-def-123", ...) and before cy.visit(...)) so the intercept setup is consistent with other tests; update the test that uses interceptRoomFilesRequest() and the room fixture modification (delete room.data.owner) to only run inside the fixture callback while keeping interceptRoomFilesRequest() as a top-level command.tests/Frontend/e2e/AdminSettingsEdit.cy.js (1)
3078-3175: Missing 422 validation error coverage forroom_hide_owner_for_guestsin the error test.The
save changes errorstest does not include the new field in any of the three required places:
- 422 error body (lines 3078–3175):
room_hide_owner_for_guestsis absent, so the field's error-display path is never exercised.- Error display assertions (after line 3302): no
cy.get('[data-test="room-hide-owner-field"]').should("include.text", ...)check.- Error-cleared assertions (after line 3489): no corresponding
should("not.include.text", ...)check after the 500 response.Every other room-section field (
room_limit,room_token_expiration,room_file_terms_of_use, etc.) has all three. The new field should follow the same pattern:🧪 Suggested additions
// In the 422 error body object: room_file_terms_of_use: [ "The selected room file terms of use is invalid.", ], + room_hide_owner_for_guests: [ + "The selected room hide owner for guests field is invalid.", + ], // After the room-file-terms-of-use display check: cy.get('[data-test="room-file-terms-of-use-field"]').should( "include.text", "The selected room file terms of use is invalid.", ); + cy.get('[data-test="room-hide-owner-field"]').should( + "include.text", + "The selected room hide owner for guests field is invalid.", + ); // After the room-file-terms-of-use clear check (post-500): cy.get('[data-test="room-file-terms-of-use-field"]').should( "not.include.text", "The selected room file terms of use is invalid.", ); + cy.get('[data-test="room-hide-owner-field"]').should( + "not.include.text", + "The selected room hide owner for guests field is invalid.", + );Also applies to: 3282-3305, 3469-3492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Frontend/e2e/AdminSettingsEdit.cy.js` around lines 3078 - 3175, Add the missing 422 validation error for the new field and its corresponding assertions: include "room_hide_owner_for_guests": ["The selected room hide owner for guests field is invalid."] in the cy.intercept POST body that's aliased as saveChangesRequest (the big error object), add a UI assertion that checks cy.get('[data-test="room-hide-owner-field"]').should("include.text", "The selected room hide owner for guests field is invalid.") in the error-display assertions section (near the other room_* checks), and add the matching cleared assertion cy.get('[data-test="room-hide-owner-field"]').should("not.include.text", ...) in the error-cleared assertions after the 500 response so the field follows the same pattern as room_limit/room_file_terms_of_use/etc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Http/Resources/Room.php`:
- Around line 94-97: The 'owner' array inside Room resource's when() is being
evaluated eagerly causing $this->owner (and its lazy-loaded relation) to be
accessed even when $showOwner is false; change the second argument to when() to
a Closure (use function() or fn() that returns ['id' => $this->owner->id, 'name'
=> $this->owner->fullname]) so the $this->owner->id and $this->owner->fullname
are only evaluated when $showOwner is true.
In `@tests/Frontend/e2e/AdminSettingsEdit.cy.js`:
- Around line 3078-3175: Add the missing 422 validation error for the new field
and its corresponding assertions: include "room_hide_owner_for_guests": ["The
selected room hide owner for guests field is invalid."] in the cy.intercept POST
body that's aliased as saveChangesRequest (the big error object), add a UI
assertion that checks
cy.get('[data-test="room-hide-owner-field"]').should("include.text", "The
selected room hide owner for guests field is invalid.") in the error-display
assertions section (near the other room_* checks), and add the matching cleared
assertion
cy.get('[data-test="room-hide-owner-field"]').should("not.include.text", ...) in
the error-cleared assertions after the 500 response so the field follows the
same pattern as room_limit/room_file_terms_of_use/etc.
In `@tests/Frontend/e2e/RoomsViewGeneral.cy.js`:
- Around line 2109-2129: Move the cy.interceptRoomFilesRequest() call out of the
cy.fixture("room.json").then(...) callback and place it at the top-level of the
test body (before calling cy.intercept("GET", "api/v1/rooms/abc-def-123", ...)
and before cy.visit(...)) so the intercept setup is consistent with other tests;
update the test that uses interceptRoomFilesRequest() and the room fixture
modification (delete room.data.owner) to only run inside the fixture callback
while keeping interceptRoomFilesRequest() as a top-level command.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.mdapp/Http/Controllers/api/v1/SettingsController.phpapp/Http/Resources/Room.phplang/en/validation.phpresources/js/components/RoomShareButton.vueresources/js/policies/RoomPolicy.jsresources/js/views/AdminSettings.vuetests/Backend/Feature/api/v1/Room/RoomTest.phptests/Backend/Feature/api/v1/SettingsTest.phptests/Frontend/e2e/AdminSettingsEdit.cy.jstests/Frontend/e2e/AdminSettingsView.cy.jstests/Frontend/e2e/RoomsViewGeneral.cy.jstests/Frontend/fixtures/settings.json
✅ Files skipped from review due to trivial changes (1)
- resources/js/components/RoomShareButton.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- resources/js/views/AdminSettings.vue
2 participants
Fixes #2843
Type
Checklist
Changes
room.hide_owner_for_gueststoRoomSettings.AdminSettings.vueunder room settings.RoomHeader.vueandRoomDetailsList.vueto hide the owner's name for unauthenticated users if the setting is enabled.Config.phpandSettings.phpresources to expose the setting to the frontend.Other information
How to test:
Summary by CodeRabbit