-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: Adds deprecation warning on livechat:removeRoom with new endpoint replacing it
#36958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b6d570a The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds REST endpoint POST livechat/rooms.delete for per-room deletion, deprecates Meteor method livechat:removeRoom with logging, updates client hook and e2e tests to use the new endpoint, tightens server-side room removal guards, and extends REST typings and route exposure. Includes changeset bump and deprecation note. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant API as REST API /v1/livechat/rooms.delete
participant Auth as Auth & Permissions
participant Rooms as RoomsService (removeOmnichannelRoom)
participant DB as DB
User->>Client: Remove room (rid)
Client->>API: POST { roomId }
API->>Auth: Validate token + permission remove-closed-livechat-room
alt Unauthorized/Forbidden
Auth-->>API: 401/403
API-->>Client: Error (401/403)
else Authorized
API->>DB: Load room by id
alt Not found / not livechat / open
API-->>Client: 400 Bad Request
else Closed livechat room
API->>Rooms: removeOmnichannelRoom(roomId)
Rooms->>DB: Delete room + artifacts
Rooms-->>API: success
API-->>Client: { success: true }
end
end
sequenceDiagram
autonumber
participant Admin as Admin Client
participant API as REST API /v1/livechat/rooms.removeAllClosedRooms
participant Rooms as RoomsService
participant DB as DB
Admin->>API: POST
API->>DB: Find closed livechat rooms
loop For each closed room
API->>Rooms: removeOmnichannelRoom(roomId)
Rooms->>DB: Delete room data
end
API-->>Admin: { removedRooms: N, success: true }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewersPoem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36958 +/- ##
===========================================
- Coverage 67.30% 66.26% -1.05%
===========================================
Files 3278 3330 +52
Lines 112000 114112 +2112
Branches 20317 20933 +616
===========================================
+ Hits 75386 75620 +234
- Misses 34017 35886 +1869
- Partials 2597 2606 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…TZ-60 # Conflicts: # apps/meteor/app/livechat/server/api/v1/room.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
4056-4687: Add POST '/v1/livechat/rooms.delete' to OmnichannelEndpointsClient code calls useEndpoint('POST', '/v1/livechat/rooms.delete') (apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts) and the server references it (apps/meteor/app/livechat/server/methods/removeRoom.ts); add a POST entry for '/v1/livechat/rooms.delete' in packages/rest-typings/src/v1/omnichannel.ts with the correct params/response types.
🧹 Nitpick comments (5)
.changeset/three-turkeys-dress.md (1)
6-7: Clean up text and include full endpoint pathClarify the endpoint with /v1 prefix and remove the stray “7” line.
-Adds deprecation warning on `livechat:removeRoom` with new endpoint replacing it; `livechat/rooms.delete` -7 +Deprecate `livechat:removeRoom`; use `/v1/livechat/rooms.delete`.packages/rest-typings/src/v1/omnichannel.ts (1)
3992-4005: Use IRoom['_id'] for roomId to match conventionsAligns with nearby types (e.g., LivechatRoomOnHold). No runtime effect; improves type accuracy.
-type POSTLivechatRemoveRoomParams = { - roomId: string; -}; +type POSTLivechatRemoveRoomParams = { + roomId: IRoom['_id']; +};apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
55-58: Assert delete status for earlier failure signalMatch the pattern used in updateRoom() for clearer test failures.
async delete() { await closeRoom(api, { roomId: room._id, visitorToken }); - return api.post('/livechat/rooms.delete', { roomId: room._id }); + const response = await api.post('/livechat/rooms.delete', { roomId: room._id }); + if (response.status() !== 200) { + throw Error(`Unable to delete room [http status: ${response.status()}]`); + } + return response; },apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
19-19: Fix deprecation log: wrong method nameIt should reference livechat:removeRoom.
- methodDeprecationLogger.method('livechat:removeCustomField', '8.0.0', '/v1/livechat/rooms.delete'); + methodDeprecationLogger.method('livechat:removeRoom', '8.0.0', '/v1/livechat/rooms.delete');apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts (1)
7-9: Prefer void over null for mutation resultEndpoint returns success with no payload; keep types as void for consistency.
-export const useRemoveCurrentChatMutation = ( - options?: Omit<UseMutationOptions<null, Error, IRoom['_id']>, 'mutationFn'>, -): UseMutationResult<null, Error, IRoom['_id']> => { +export const useRemoveCurrentChatMutation = ( + options?: Omit<UseMutationOptions<void, Error, IRoom['_id']>, 'mutationFn'>, +): UseMutationResult<void, Error, IRoom['_id']> => { const removeRoom = useEndpoint('POST', '/v1/livechat/rooms.delete');Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/three-turkeys-dress.md(1 hunks)apps/meteor/app/livechat/server/api/v1/room.ts(2 hunks)apps/meteor/app/livechat/server/methods/removeRoom.ts(2 hunks)apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts(1 hunks)apps/meteor/tests/e2e/utils/omnichannel/rooms.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
🧬 Code graph analysis (2)
apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)
apps/meteor/app/livechat/server/api/v1/room.ts (8)
apps/meteor/app/api/server/api.ts (1)
API(42-77)packages/rest-typings/src/v1/omnichannel.ts (4)
POSTLivechatRemoveRoomSuccess(4020-4020)isPOSTLivechatRemoveRoomParams(4007-4007)isPOSTLivechatRoomsCloseAllSuccessResponse(3988-3990)isPOSTLivechatRoomsCloseAll(3971-3971)packages/rest-typings/src/v1/Ajv.ts (1)
validateBadRequestErrorResponse(46-46)packages/models/src/index.ts (1)
LivechatRooms(175-175)apps/meteor/app/livechat/server/lib/rooms.ts (1)
removeOmnichannelRoom(257-292)apps/meteor/app/livechat/server/lib/logger.ts (1)
livechatLogger(5-5)apps/meteor/lib/callbacks.ts (1)
callbacks(244-252)packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(252-303)
🔇 Additional comments (4)
packages/rest-typings/src/v1/omnichannel.ts (1)
4009-4021: LGTM on success schema/validator exportPattern matches existing success validators.
apps/meteor/app/livechat/server/api/v1/room.ts (3)
22-25: LGTM: typings imports for new routeImports are correct and minimal.
444-476: LGTM: new livechat/rooms.delete routeAuth, permission, validation, and failure paths look good; aligns with existing API style.
477-502: LGTM: removeAllClosedRooms stays intactGood logging and permission split from single-delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
7-7: Import LGTM; add JSDoc deprecation on the DDP method signatureSurface the deprecation to TS consumers as well.
declare module '@rocket.chat/ddp-client' { // eslint-disable-next-line @typescript-eslint/naming-convention interface ServerMethods { - 'livechat:removeRoom'(rid: IRoom['_id']): void; + /** + * @deprecated Use POST /v1/livechat/rooms.delete instead. Scheduled for removal in 8.0.0. + */ + 'livechat:removeRoom'(rid: IRoom['_id']): void; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
19-19: No change required — deprecation style and permissions are correctRepository deprecation messages predominantly use endpoint paths without HTTP verbs; changing this one to "POST /v1/..." would be inconsistent. The API route (apps/meteor/app/livechat/server/api/v1/room.ts) registers POST 'livechat/rooms.delete' with permission 'remove-closed-livechat-room', and the Meteor method checks the same permission — permission parity verified.
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…e/v7/CTZ-60 # Conflicts: # packages/rest-typings/src/v1/omnichannel.ts
4c226f2
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithDepartmentData.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/livechat/server/lib/rooms.ts(2 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/livechat/server/lib/rooms.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
…point replacing it (#36958)
…point replacing it (#36958)
…point replacing it (#36958)
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:removeRoommeteor method, as well as it adds an endpoint (livechat/rooms.delete) to replace it.Issue(s)
CTZ-60
CORE-1410
Steps to test or reproduce
Further comments
Tests will be implemented on this task
Summary by CodeRabbit