feat(requests): allow users to view requester on media details page - #2866
feat(requests): allow users to view requester on media details page#2866danielkinahan wants to merge 11 commits into
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:
📝 WalkthroughWalkthroughMovie and TV detail pages now render a media request summary with permission-gated request data and the current user id. English locale strings were added for the summary’s date, requester, and status labels. ChangesMedia request detail integration
Sequence Diagram(s)sequenceDiagram
participant Browser
participant MovieDetails
participant TvDetails
participant AuthPermissions
participant MediaRequestSummary
Browser->>MovieDetails: open movie page
Browser->>TvDetails: open TV page
MovieDetails->>AuthPermissions: check MANAGE_REQUESTS / REQUEST_VIEW
TvDetails->>AuthPermissions: check MANAGE_REQUESTS / REQUEST_VIEW
AuthPermissions-->>MovieDetails: permission scope
AuthPermissions-->>TvDetails: permission scope
MovieDetails->>MediaRequestSummary: render requests + currentUserId
TvDetails->>MediaRequestSummary: render requests + currentUserId
MediaRequestSummary-->>Browser: display requester, date, and status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. 🐇 I hopped to the media page with cheer, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/MediaRequestSummary/index.tsx (1)
44-70: Consolidate status mapping to prevent future drift.
statusMessageandstatusBadgeTypeare derived from duplicateswitchlogic on the same enum. Centralizing this in one map/helper will keep status text and badge color in sync as statuses evolve.♻️ Suggested refactor
+const requestStatusMeta = { + [MediaRequestStatus.APPROVED]: { + message: globalMessages.approved, + badgeType: 'success', + }, + [MediaRequestStatus.DECLINED]: { + message: globalMessages.declined, + badgeType: 'danger', + }, + [MediaRequestStatus.FAILED]: { + message: globalMessages.failed, + badgeType: 'danger', + }, + [MediaRequestStatus.COMPLETED]: { + message: globalMessages.completed, + badgeType: 'success', + }, + [MediaRequestStatus.PENDING]: { + message: globalMessages.pending, + badgeType: 'warning', + }, +} as const; ... - const statusMessage = (() => { - switch (request.status) { - case MediaRequestStatus.APPROVED: - return intl.formatMessage(globalMessages.approved); - case MediaRequestStatus.DECLINED: - return intl.formatMessage(globalMessages.declined); - case MediaRequestStatus.FAILED: - return intl.formatMessage(globalMessages.failed); - case MediaRequestStatus.COMPLETED: - return intl.formatMessage(globalMessages.completed); - default: - return intl.formatMessage(globalMessages.pending); - } - })(); - - const statusBadgeType = (() => { - switch (request.status) { - case MediaRequestStatus.APPROVED: - case MediaRequestStatus.COMPLETED: - return 'success'; - case MediaRequestStatus.DECLINED: - case MediaRequestStatus.FAILED: - return 'danger'; - default: - return 'warning'; - } - })(); + const statusMeta = + requestStatusMeta[request.status] ?? requestStatusMeta[MediaRequestStatus.PENDING]; + const statusMessage = intl.formatMessage(statusMeta.message); + const statusBadgeType = statusMeta.badgeType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MediaRequestSummary/index.tsx` around lines 44 - 70, The duplicate switch logic for computing statusMessage and statusBadgeType from MediaRequestStatus should be centralized: create a single helper (e.g., getMediaRequestStatusInfo or STATUS_MAP) that accepts a MediaRequestStatus and returns both the localized message (using intl + globalMessages) and the badge type string; replace the current statusMessage and statusBadgeType computed blocks with calls to that helper (use its .message and .badgeType), ensuring you reference MediaRequestStatus, intl, and globalMessages inside the helper so both values stay in sync as statuses change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/MediaRequestSummary/index.tsx`:
- Around line 44-70: The duplicate switch logic for computing statusMessage and
statusBadgeType from MediaRequestStatus should be centralized: create a single
helper (e.g., getMediaRequestStatusInfo or STATUS_MAP) that accepts a
MediaRequestStatus and returns both the localized message (using intl +
globalMessages) and the badge type string; replace the current statusMessage and
statusBadgeType computed blocks with calls to that helper (use its .message and
.badgeType), ensuring you reference MediaRequestStatus, intl, and globalMessages
inside the helper so both values stay in sync as statuses change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f10119d-f175-4952-a1a5-27805527278f
📒 Files selected for processing (3)
src/components/MediaRequestSummary/index.tsxsrc/components/MovieDetails/index.tsxsrc/components/TvDetails/index.tsx
gauthier-th
left a comment
gauthier-th
left a comment
There was a problem hiding this comment.
What's the point of this PR?
You can already see the requests on the right sidebar (displayed when you click on the settings icon).
|
Only admins have access to that settings icon. My users are often asking who requested the movies they go to look up. |
You can already grant a user to view requests from other users... |
|
I'm aware of that user privilege, which grants view access to the requests tab. I used the same one in the code in this PR. However there is no search bar on that tab. A user would need to scroll to the approximate date when it was created which they wouldnt know. I have over 1300 requests on my instance and scrolling through those is not convenient. |
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
|
I've addressed the feedback and im waiting for the maintainers response |
There was a problem hiding this comment.
Pull request overview
Adds a “Media Request Summary” section to Movie/TV detail pages that displays requester, request date, and status (including a 4K badge), with visibility intended to depend on the viewer’s permissions.
Changes:
- Introduces a new
MediaRequestSummarycomponent that renders per-request requester/date/status rows. - Renders
MediaRequestSummaryon both Movie and TV detail pages with permission-based filtering for what is displayed. - Adds new English i18n keys for the summary labels.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/i18n/locale/en.json | Adds i18n strings for the new request summary labels. |
| src/components/TvDetails/index.tsx | Renders the new request summary card on TV detail pages with permission-based display filtering. |
| src/components/MovieDetails/index.tsx | Renders the new request summary card on Movie detail pages with permission-based display filtering. |
| src/components/MediaRequestSummary/index.tsx | New UI component that lists requests with requester info, request date, and status badges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hasPermission( | ||
| [Permission.MANAGE_REQUESTS, Permission.REQUEST_VIEW], | ||
| { type: 'or' } | ||
| ) | ||
| ? data.mediaInfo?.requests |
| hasPermission( | ||
| [Permission.MANAGE_REQUESTS, Permission.REQUEST_VIEW], | ||
| { type: 'or' } | ||
| ) | ||
| ? data.mediaInfo?.requests |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/components/TvDetails/index.tsx:1338
- This permission check only hides other users’ requests in the UI. The movie/TV details APIs still return
mediaInfo.requests(andrequestedBy) for everyone viaMedia.getMedia(..., { relations: { requests: true } }), andMediaRequest.requestedByis eager-loaded, so a user without REQUEST_VIEW can still see other requesters by inspecting the network response. Consider enforcing this authorization server-side (e.g., filtermediaInfo.requeststo the current user unless the requester has MANAGE_REQUESTS or REQUEST_VIEW, or omitrequestedByentirely when not authorized).
requests={
hasPermission(
[Permission.MANAGE_REQUESTS, Permission.REQUEST_VIEW],
{ type: 'or' }
)
? data.mediaInfo?.requests
: data.mediaInfo?.requests?.filter(
(r) => r.requestedBy?.id === user?.id
)
}
src/components/MovieDetails/index.tsx:1118
- This permission check only hides other users’ requests in the UI. The movie/TV details APIs still return
mediaInfo.requests(andrequestedBy) for everyone viaMedia.getMedia(..., { relations: { requests: true } }), andMediaRequest.requestedByis eager-loaded, so a user without REQUEST_VIEW can still see other requesters by inspecting the network response. Consider enforcing this authorization server-side (e.g., filtermediaInfo.requeststo the current user unless the requester has MANAGE_REQUESTS or REQUEST_VIEW, or omitrequestedByentirely when not authorized).
requests={
hasPermission(
[Permission.MANAGE_REQUESTS, Permission.REQUEST_VIEW],
{ type: 'or' }
)
? data.mediaInfo?.requests
: data.mediaInfo?.requests?.filter(
(r) => r.requestedBy?.id === user?.id
)
}
Description
This change adds the ability for a user with View Requests permission to view the requests of other users on the media details page (both TV and movies). I added it as a seperate card under the media-facts card, but I'd like feedback on the placement. I will include screenshots.
Disclaimer on AI
I used AI to search the codebase and review this code for feedback.
How Has This Been Tested?
I copied my db of my instance and ran tests with it, so I could have data. I have only run this in dev mode. I have tried this with a piece of media that has no requests, one and multiple (creates new cards for each).
Screenshots / Logs (if applicable)
One request
Multiple requests
No requests (card doesnt show)
Mobile view of multiple requests
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit