chore: Adds deprecation warning on livechat:takeInquiry
#36993
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: d910c57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
WalkthroughCentralizes livechat takeInquiry logic into a new server library, updates the Meteor method to delegate and log deprecation, switches the client to use the REST endpoint with new parameter shape, adjusts REST typings to include an options object, updates a REST route import, and adds a changeset noting deprecations/version bumps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Agent as Client (Composer)
participant REST as REST API /v1/livechat/inquiries.take
participant Lib as Server lib/takeInquiry
participant Users as Users
participant Inq as LivechatInquiry
participant Rooms as LivechatRooms
participant Omni as Omnichannel (MAC)
participant Route as RoutingManager
Agent->>REST: POST { inquiryId, userId?, options{ clientAction, ... } }
REST->>Lib: takeInquiry(userId, inquiryId, options)
Lib->>Inq: find inquiry by ID
Inq-->>Lib: inquiry or not found
alt inquiry not found or already taken
Lib-->>REST: throw Meteor.Error
REST-->>Agent: HTTP error
else valid inquiry
Lib->>Users: findOneOnlineAgentById(userId, idleSetting)
Users-->>Lib: agent or null
alt agent unavailable
Lib-->>REST: throw Meteor.Error
REST-->>Agent: HTTP error
else
Lib->>Rooms: get room by inquiry
Rooms-->>Lib: room
Lib->>Omni: isWithinMACLimit(room)
Omni-->>Lib: boolean
alt MAC limit exceeded
Lib-->>REST: throw Meteor.Error
REST-->>Agent: HTTP error
else ok
note over Lib,Route: Optionally validate contact availability<br/>and migrate/verify contact
Lib->>Route: takeInquiry(inquiry, agent, options, room)
Route-->>Lib: success or error
alt routing error
Lib-->>REST: throw Meteor.Error
REST-->>Agent: HTTP error
else success
REST-->>Agent: 200 OK
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36993 +/- ##
===========================================
- Coverage 67.30% 66.23% -1.08%
===========================================
Files 3337 3391 +54
Lines 113326 115579 +2253
Branches 20563 21241 +678
===========================================
+ Hits 76278 76557 +279
- Misses 34444 36402 +1958
- Partials 2604 2620 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…TZ-70 # Conflicts: # apps/meteor/app/livechat/server/lib/takeInquiry.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/meteor/app/livechat/imports/server/rest/inquiries.ts (2)
60-66: Bug: options are ignored when calling takeInquiry.Client sends options (e.g., clientAction: true) but the route drops them. Forward them to the lib.
- return API.v1.success({ - inquiry: await takeInquiry(this.bodyParams.userId || this.userId, this.bodyParams.inquiryId), - }); + return API.v1.success({ + inquiry: await takeInquiry( + this.bodyParams.userId || this.userId, + this.bodyParams.inquiryId, + this.bodyParams.options, + ), + });
64-66: Bug: response returns undefined inquiry (lib returns void).Contract for /v1/livechat/inquiries.take is { inquiry: ILivechatInquiryRecord }, but takeInquiry() currently returns void. Fix one of these:
- Preferred: make server/lib/takeInquiry return ILivechatInquiryRecord and keep this route as-is after the options fix.
- Alternative (if lib stays void): fetch the inquiry after the call.
- return API.v1.success({ - inquiry: await takeInquiry(this.bodyParams.userId || this.userId, this.bodyParams.inquiryId, this.bodyParams.options), - }); + await takeInquiry(this.bodyParams.userId || this.userId, this.bodyParams.inquiryId, this.bodyParams.options); + const inquiry = await LivechatInquiry.findOneById(this.bodyParams.inquiryId); + return API.v1.success({ inquiry });apps/meteor/app/livechat/server/methods/takeInquiry.ts (2)
10-14: Align DDP options type with REST shape.Use departmentId (not oldDepartmentId) and avoid any.
- 'livechat:takeInquiry'( - inquiryId: string, - options?: { clientAction: boolean; forwardingToDepartment?: { oldDepartmentId: string; transferData: any } }, - ): unknown; + 'livechat:takeInquiry'( + inquiryId: string, + options?: { clientAction: boolean; forwardingToDepartment?: { departmentId: string; transferData: unknown } }, + ): unknown;
17-29: Return the inquiry from the lib and pass it through the Meteor method.takeInquiry (apps/meteor/app/livechat/server/lib/takeInquiry.ts) currently returns Promise, so the Meteor method returns undefined over DDP. Change the lib to return the inquiry (the updated inquiry — e.g., refetch with LivechatInquiry.findOneById(inquiryId) after RoutingManager.takeInquiry) and update the Meteor method to await and return it.
Files to change:
- apps/meteor/app/livechat/server/lib/takeInquiry.ts — change return type and return the (updated) inquiry after RoutingManager.takeInquiry.
- apps/meteor/app/livechat/server/methods/takeInquiry.ts — return the value (await) from the lib.
Suggested method diff (keep as-is):
- return takeInquiry(uid, inquiryId, options); + return await takeInquiry(uid, inquiryId, options);
🧹 Nitpick comments (2)
.changeset/nice-balloons-relax.md (1)
6-6: Fix changeset entry: wrong method name and missing leading slash.This PR deprecates livechat:takeInquiry, not livechat:removeRoom. Also prefer the canonical endpoint path with leading slash.
-Adds deprecation warning on `livechat:removeRoom`, use `livechat/inquiries.take` instead +Adds deprecation warning on `livechat:takeInquiry`; use `/v1/livechat/inquiries.take` insteadapps/meteor/client/views/room/composer/ComposerOmnichannel/ComposerOmnichannelInquiry.tsx (1)
36-39: Harden toast error handling.Ensure a string is shown instead of an object.
- } catch (error) { - dispatchToastMessage({ type: 'error', message: error }); + } catch (error) { + dispatchToastMessage({ + type: 'error', + message: error instanceof Error ? error.message : String(error), + });
📜 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/nice-balloons-relax.md(1 hunks)apps/meteor/app/livechat/imports/server/rest/inquiries.ts(1 hunks)apps/meteor/app/livechat/server/lib/takeInquiry.ts(1 hunks)apps/meteor/app/livechat/server/methods/takeInquiry.ts(2 hunks)apps/meteor/client/views/room/composer/ComposerOmnichannel/ComposerOmnichannelInquiry.tsx(3 hunks)packages/rest-typings/src/v1/omnichannel.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/lib/takeInquiry.ts (4)
apps/meteor/app/livechat/server/methods/takeInquiry.ts (1)
inquiryId(18-28)packages/models/src/index.ts (2)
LivechatInquiry(173-173)LivechatRooms(175-175)packages/core-services/src/index.ts (1)
Omnichannel(181-181)apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
RoutingManager(73-346)
🔇 Additional comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
3346-3370: Schema looks good; keep transferData typed unknown.No change requested here; just ensure server accepts this exact structure.
If you change server-side types, re-run REST schema tests that validate additionalProperties: false for options.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/livechat/server/methods/takeInquiry.ts (2)
5-6: Unify option typing with REST typings to avoid driftInline-shaping
optionscan diverge from the REST contract. Import and reuse the shared REST type for parity.Apply this diff to add the type import:
import { methodDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger'; import { takeInquiry } from '../lib/takeInquiry'; +import type { POSTLivechatInquiriesTakeParams } from '@rocket.chat/rest-typings';Then update the method signature to reuse it (outside this hunk):
// in the ServerMethods interface 'livechat:takeInquiry'(inquiryId: string, options?: POSTLivechatInquiriesTakeParams['options']): unknown;Also confirm that runtime validation now lives in lib/takeInquiry; if not, consider keeping a lightweight boundary check for
inquiryIdhere.
28-32: Permission scope: ensure we also require 'receive-livechat' (or equivalent)If lib/takeInquiry doesn’t enforce agent capabilities, gating only on
view-l-roommay be too permissive. Either rely solely on lib’s checks or strengthen this method’s guard to include the agent capability.If you choose to harden here, one option:
- if (!(await hasPermissionAsync(uid, 'view-l-room'))) { + const [canView, canReceive] = await Promise.all([ + hasPermissionAsync(uid, 'view-l-room'), + hasPermissionAsync(uid, 'receive-livechat'), + ]); + if (!canView || !canReceive) { throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'livechat:takeInquiry', }); }
📜 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/takeInquiry.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/methods/takeInquiry.ts (1)
apps/meteor/app/livechat/server/methods/getFirstRoomMessage.ts (1)
uid(17-35)
⏰ 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
🔇 Additional comments (1)
apps/meteor/app/livechat/server/methods/takeInquiry.ts (1)
20-20: Deprecation log: present — confirm 8.0.0 removal target
- methodDeprecationLogger.method('livechat:takeInquiry', '8.0.0', '/v1/livechat/inquiries.take') at apps/meteor/app/livechat/server/methods/takeInquiry.ts:20.
- Repo shows mixed removal targets for other livechat methods (e.g., livechat:transfer uses '7.0.0'); confirm 8.0.0 is the intended target in changesets/docs.
dougfabris
left a comment
dougfabris
left a comment
There was a problem hiding this comment.
On behalf of frontend, looks good to me
lucas-a-pelegrino
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning to
livechat:takeInquiry, moves thetakeInquiryfunction to its own file, as well as it replaces the use oflivechat:takeInquiryfor thelivechat/inquiries.takeendpoint in the client.Issue(s)
CTZ-70
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Deprecations
API Changes
Improvements