fix(requests): refresh modified timestamps#2729
Open
saudademjj wants to merge 6 commits intoseerr-team:developfrom
Open
fix(requests): refresh modified timestamps#2729saudademjj wants to merge 6 commits intoseerr-team:developfrom
saudademjj wants to merge 6 commits intoseerr-team:developfrom
Conversation
2 tasks
|
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:
📝 WalkthroughWalkthroughAdds an integration test suite for request routes and ensures modified requests explicitly set Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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. 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)
server/routes/request.test.ts (1)
81-114: Consider adding a test for the decline flow as well.The current test covers approval but the
@BeforeUpdate()hook should also refreshupdatedAtwhen a request is declined. Adding a similar test case would provide more comprehensive coverage.📝 Example test case for decline
it('refreshes updatedAt when a request is declined', async () => { const requestRepository = getRepository(MediaRequest); const pendingRequest = await createPendingRequest(); const previousUpdatedAt = pendingRequest.updatedAt.toISOString(); const agent = await authenticatedAgent('admin@seerr.dev', 'test1234'); const res = await agent.post(`/request/${pendingRequest.id}/decline`); assert.strictEqual(res.status, 200); assert.ok( new Date(res.body.updatedAt).getTime() > new Date(previousUpdatedAt).getTime() ); assert.strictEqual(savedRequest.status, MediaRequestStatus.DECLINED); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/request.test.ts` around lines 81 - 114, Add a parallel test for the decline flow that mirrors the approve test: use createPendingRequest() to seed a pending MediaRequest, capture previousUpdatedAt from pendingRequest.updatedAt, authenticate as admin via authenticatedAgent('admin@seerr.dev','test1234'), POST to /request/{id}/decline, assert response status is 200, assert the returned updatedAt is greater than previousUpdatedAt, and assert the request's status is MediaRequestStatus.DECLINED (use the same repository/getRepository(MediaRequest) or the response body to validate status); this ensures the `@BeforeUpdate`() hook refreshes updatedAt on decline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/request.test.ts`:
- Around line 81-114: Add a parallel test for the decline flow that mirrors the
approve test: use createPendingRequest() to seed a pending MediaRequest, capture
previousUpdatedAt from pendingRequest.updatedAt, authenticate as admin via
authenticatedAgent('admin@seerr.dev','test1234'), POST to /request/{id}/decline,
assert response status is 200, assert the returned updatedAt is greater than
previousUpdatedAt, and assert the request's status is
MediaRequestStatus.DECLINED (use the same repository/getRepository(MediaRequest)
or the response body to validate status); this ensures the `@BeforeUpdate`() hook
refreshes updatedAt on decline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 659a9f27-f88b-4f0c-9bf4-d2cdfc3a77bb
📒 Files selected for processing (2)
server/entity/MediaRequest.tsserver/routes/request.test.ts
fallenbagel
requested changes
Mar 20, 2026
Collaborator
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
Please update the PR description to follow the PR template.
Author
|
Updated the PR description to match the repository template and pushed |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/routes/request.test.ts (1)
117-171: Coverage is solid; consider extracting shared transition assertions.Approve/decline tests duplicate setup and verification steps. A small helper would reduce drift and make future status-flow extensions easier.
♻️ Optional refactor sketch
+async function assertRequestTransition( + action: 'approve' | 'decline', + expectedStatus: MediaRequestStatus +) { + const requestRepository = getRepository(MediaRequest); + const pendingRequest = await createPendingRequest(); + const previousUpdatedAt = pendingRequest.updatedAt.toISOString(); + const agent = await authenticatedAgent('admin@seerr.dev', 'test1234'); + + const res = await agent.post(`/request/${pendingRequest.id}/${action}`); + + assert.strictEqual(res.status, 200); + assert.notStrictEqual(res.body.updatedAt, previousUpdatedAt); + assert.ok(new Date(res.body.updatedAt).getTime() > new Date(previousUpdatedAt).getTime()); + assert.strictEqual(res.body.modifiedBy.email, 'admin@seerr.dev'); + + const savedRequest = await requestRepository.findOneOrFail({ + where: { id: pendingRequest.id }, + relations: { modifiedBy: true }, + }); + + assert.strictEqual(savedRequest.status, expectedStatus); + assert.strictEqual(savedRequest.modifiedBy?.email, 'admin@seerr.dev'); + assert.ok(savedRequest.updatedAt.getTime() > pendingRequest.updatedAt.getTime()); +} + describe('POST /request/:requestId/:status', () => { it('refreshes updatedAt when a request is approved', async () => { - // duplicated body... + await assertRequestTransition('approve', MediaRequestStatus.APPROVED); }); it('refreshes updatedAt when a request is declined', async () => { - // duplicated body... + await assertRequestTransition('decline', MediaRequestStatus.DECLINED); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/request.test.ts` around lines 117 - 171, The two tests in request.test.ts ('refreshes updatedAt when a request is approved' and 'refreshes updatedAt when a request is declined') duplicate setup and verification; extract a helper (e.g., assertRequestTransition or runRequestTransition) that accepts the action ('approve' | 'decline') and expected MediaRequestStatus to perform: createPendingRequest, capture previousUpdatedAt, call authenticatedAgent, post to `/request/${id}/${action}`, assert status 200, assert res.body.updatedAt changed and is newer, assert res.body.modifiedBy.email is 'admin@seerr.dev', load savedRequest via getRepository(MediaRequest).findOneOrFail with relations, assert savedRequest.status equals expected MediaRequestStatus and modifiedBy.email and that savedRequest.updatedAt is newer than pendingRequest.updatedAt; then replace the two tests with calls to that helper for approve and decline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/request.test.ts`:
- Around line 117-171: The two tests in request.test.ts ('refreshes updatedAt
when a request is approved' and 'refreshes updatedAt when a request is
declined') duplicate setup and verification; extract a helper (e.g.,
assertRequestTransition or runRequestTransition) that accepts the action
('approve' | 'decline') and expected MediaRequestStatus to perform:
createPendingRequest, capture previousUpdatedAt, call authenticatedAgent, post
to `/request/${id}/${action}`, assert status 200, assert res.body.updatedAt
changed and is newer, assert res.body.modifiedBy.email is 'admin@seerr.dev',
load savedRequest via getRepository(MediaRequest).findOneOrFail with relations,
assert savedRequest.status equals expected MediaRequestStatus and
modifiedBy.email and that savedRequest.updatedAt is newer than
pendingRequest.updatedAt; then replace the two tests with calls to that helper
for approve and decline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dbb025b-5588-4422-8084-ce2d5306a9f7
📒 Files selected for processing (1)
server/routes/request.test.ts
Author
|
Updated the PR template again after finishing the full validation run on 100.92.89.12. Prettier, ESLint, tests, server typecheck, build, and i18n extraction all passed on Node 22.18.0 / pnpm 10.24.0. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
saudademjj
force-pushed
the
saudademjj/fix-request-modified-date
branch
from
f222e83 to
3bb9f9e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/routes/request.ts (1)
647-650: Consider settingmodifiedByin retry flow for audit consistency.
POST /:requestId/:statusrecords the actor (modifiedBy), but retry currently does not. Keeping both paths aligned will make audit metadata consistent.Suggested patch
// this also triggers updating the parent media's status & sending to *arr request.status = MediaRequestStatus.APPROVED; + request.modifiedBy = req.user; request.updatedAt = new Date(); await requestRepository.save(request);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/request.ts` around lines 647 - 650, The retry flow updates request.status and request.updatedAt and saves via requestRepository.save(request) but does not set request.modifiedBy, causing inconsistent audit metadata; update the retry code path to assign the current actor to request.modifiedBy (same source used in POST /:requestId/:status), then set request.updatedAt and save with requestRepository.save(request) so both approval and retry flows record the modifier consistently.server/routes/request.test.ts (1)
122-176: Optional: parameterize approve/decline test bodies to reduce duplication.The two status tests are nearly identical aside from endpoint and expected status, so a small table-driven helper would simplify future maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/request.test.ts` around lines 122 - 176, The two tests in request.test.ts ("refreshes updatedAt when a request is approved" and "refreshes updatedAt when a request is declined") are duplicated; refactor into a single table-driven helper that accepts the endpoint suffix ('approve'/'decline') and the expected status (MediaRequestStatus.APPROVED / MediaRequestStatus.DECLINED). Extract the common setup (createRequest(), previousUpdatedAt, authenticatedAgent('admin@seerr.dev', 'test1234'), requestRepository) and common assertions (res.status === 200, res.body.updatedAt changed and newer, res.body.modifiedBy.email, fetching savedRequest and checking savedRequest.updatedAt) into a helper function (e.g., runStatusChangeTest(endpointSuffix, expectedStatus)) and call it for both 'approve' and 'decline' to remove duplication while keeping the existing assertions and relations lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/request.test.ts`:
- Around line 122-176: The two tests in request.test.ts ("refreshes updatedAt
when a request is approved" and "refreshes updatedAt when a request is
declined") are duplicated; refactor into a single table-driven helper that
accepts the endpoint suffix ('approve'/'decline') and the expected status
(MediaRequestStatus.APPROVED / MediaRequestStatus.DECLINED). Extract the common
setup (createRequest(), previousUpdatedAt, authenticatedAgent('admin@seerr.dev',
'test1234'), requestRepository) and common assertions (res.status === 200,
res.body.updatedAt changed and newer, res.body.modifiedBy.email, fetching
savedRequest and checking savedRequest.updatedAt) into a helper function (e.g.,
runStatusChangeTest(endpointSuffix, expectedStatus)) and call it for both
'approve' and 'decline' to remove duplication while keeping the existing
assertions and relations lookups.
In `@server/routes/request.ts`:
- Around line 647-650: The retry flow updates request.status and
request.updatedAt and saves via requestRepository.save(request) but does not set
request.modifiedBy, causing inconsistent audit metadata; update the retry code
path to assign the current actor to request.modifiedBy (same source used in POST
/:requestId/:status), then set request.updatedAt and save with
requestRepository.save(request) so both approval and retry flows record the
modifier consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2029c61a-3fc4-4e27-8067-6b94a8a09a3a
📒 Files selected for processing (2)
server/routes/request.test.tsserver/routes/request.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MediaRequest.updatedAtwhenever a request entity is updated so the request list's Modified time stays aligned with approval and decline status changes.How Has This Been Tested?
100.92.89.12usingNode v22.18.0andpnpm v10.24.0.pnpm exec prettier --check server/entity/MediaRequest.ts server/routes/request.test.tspnpm exec eslint server/entity/MediaRequest.ts server/routes/request.test.tspnpm test server/routes/auth.test.ts server/routes/request.test.tspnpm typecheck:serverpnpm buildpnpm i18n:extractpnpm i18n:extractcompleted without introducing any tracked file changes in the validation checkout.Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Tests
Bug Fixes