test: add coverage for livechat rooms delete endpoint - #41162
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds end-to-end API tests for the ChangesLivechat rooms.delete test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant API
participant Room
Test->>API: POST livechat/rooms.delete (open room)
API-->>Test: 400 invalid-state
Test->>Room: close room
Test->>API: POST livechat/rooms.delete (closed room)
API-->>Test: 200 success
Test->>API: GET channels.info(roomId)
API-->>Test: error-room-not-found
Test->>API: remove permission remove-closed-livechat-room
Test->>API: POST livechat/rooms.delete
API-->>Test: 403 unauthorized
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41162 +/- ##
===========================================
- Coverage 69.33% 69.27% -0.06%
===========================================
Files 3539 3539
Lines 138832 138832
Branches 24707 24730 +23
===========================================
- Hits 96264 96181 -83
- Misses 38553 38629 +76
- Partials 4015 4022 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts (1)
1207-1249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup skipped if assertions fail.
In both
should fail if the livechat room is still openandshould delete a closed livechat room, thecloseOmnichannelRoom/deleteVisitorcleanup calls execute after therequest...expect(...)chain as plain sequential statements. If any assertion inside.expect()throws, the test fails before reaching cleanup, leaking the visitor/room and risking test-isolation issues for later runs. The permission block below (Lines 1251-1258) avoids this by usingbefore/afterhooks that always run; consider the same pattern here (e.g., wrap cleanup inafter/afterEach, or usetry/finally).♻️ Example fix using try/finally
it('should fail if the livechat room is still open', async () => { const { room, visitor } = await createRoomForDeletion(); - await request - .post(api('livechat/rooms.delete')) - .set(credentials) - .send({ roomId: room._id }) - .expect(400) - .expect((res: Response) => { - expect(res.body).to.have.property('success', false); - }); - - await closeOmnichannelRoom(room._id); - await deleteVisitor(visitor.token); + try { + await request + .post(api('livechat/rooms.delete')) + .set(credentials) + .send({ roomId: room._id }) + .expect(400) + .expect((res: Response) => { + expect(res.body).to.have.property('success', false); + }); + } finally { + await closeOmnichannelRoom(room._id); + await deleteVisitor(visitor.token); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts` around lines 1207 - 1249, The cleanup in the livechat room deletion tests is run only after the request assertions, so failures in the `request(...).expect(...)` chain skip `closeOmnichannelRoom` and `deleteVisitor`. Update the `should fail if the livechat room is still open` and `should delete a closed livechat room` cases in `00-rooms.ts` to guarantee cleanup runs even when assertions throw, using the same `before`/`after` or `try/finally` pattern already used in the nearby permission block. Keep the cleanup tied to `createRoomForDeletion`, `closeOmnichannelRoom`, and `deleteVisitor`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts`:
- Around line 1207-1249: The cleanup in the livechat room deletion tests is run
only after the request assertions, so failures in the `request(...).expect(...)`
chain skip `closeOmnichannelRoom` and `deleteVisitor`. Update the `should fail
if the livechat room is still open` and `should delete a closed livechat room`
cases in `00-rooms.ts` to guarantee cleanup runs even when assertions throw,
using the same `before`/`after` or `try/finally` pattern already used in the
nearby permission block. Keep the cleanup tied to `createRoomForDeletion`,
`closeOmnichannelRoom`, and `deleteVisitor`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8ea870a-503a-4299-b19d-4c88eb50d219
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
🔇 Additional comments (2)
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts (2)
1175-1205: LGTM!
1251-1272: LGTM!Good use of
before/afterhooks to guarantee permission restoration regardless of test outcome.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
jessicaschelly
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
jessicaschelly
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
jessicaschelly
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
Adds API test coverage for
POST /api/v1/livechat/rooms.delete.401.roomIdvalidation returns400.remove-closed-livechat-roompermission receive403.Issue(s)
QA-118
Steps to test or reproduce
Further comments
Summary by CodeRabbit