chore: Adds deprecation warning for livechat:saveUnit
#37255
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 1ee8f4d The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 a new payload type for business units, migrates unit save/update from the Meteor method to REST endpoints (with an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UnitEdit (client)
participant Updater as onUpdate / useEndpoint
participant API as Server (REST)
rect rgb(250,245,250)
Note over UI,API: Legacy (deprecated Meteor method)
UI->>API: Meteor method call "livechat:saveUnit" (old)
API-->>UI: result (legacy)
end
rect rgb(240,255,240)
Note over UI,API: New flow (REST + optional onUpdate)
UI->>UI: build payload { unitData, unitMonitors, unitDepartments }
alt onUpdate provided
UI->>Updater: onUpdate(payload)
Updater->>API: POST /v1/livechat/units or /v1/livechat/units/:id
API-->>Updater: 200 JSON (IOmnichannelBusinessUnit)
Updater-->>UI: response
else no onUpdate
UI->>API: POST /v1/livechat/units (payload) via useEndpoint
API-->>UI: 200 JSON (IOmnichannelBusinessUnit)
end
Note right of API: Server also logs deprecation when legacy method used
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/meteor/client/omnichannel/units/UnitEdit.tsx (2)
⏰ 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)
🔇 Additional comments (3)
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 #37255 +/- ##
===========================================
- Coverage 68.12% 68.11% -0.01%
===========================================
Files 3365 3365
Lines 115811 115808 -3
Branches 20969 20845 -124
===========================================
- Hits 78896 78888 -8
Misses 34225 34225
- Partials 2690 2695 +5
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.
Actionable comments posted: 2
🧹 Nitpick comments (8)
.changeset/cool-yaks-perform.md (1)
1-5: Clarify replacement path in the changeset note.Consider explicitly naming the replacement REST endpoints to aid downstream consumers, e.g., “Use /v1/livechat/units (create) and /v1/livechat/units/:id (update).”
apps/meteor/client/omnichannel/units/UnitEditWithData.tsx (1)
17-17: Good: onUpdate wired to REST update.Using useEndpoint('POST', '/v1/livechat/units/:id', { id: unitId }) and passing onUpdate={editUnit} is correct and aligns with the new payload.
Tiny consistency nit: since path params are pre-bound for GETs, also call the bound functions without passing params (as you already do for getUnitById) to keep a uniform style across this file.
Also applies to: 65-65
apps/meteor/tests/data/livechat/units.ts (1)
38-47: REST payload shape and unwrapped response look correct.Switch to api('livechat/units') with { unitData, unitMonitors, unitDepartments } and resolving res.body aligns with the new contract.
Rename extraMonitor → extraMonitors for clarity (array).
Also applies to: 48-53
apps/meteor/tests/e2e/utils/omnichannel/units.ts (2)
22-29: Default optional arrays to [] to match server schema.If monitors/departments are omitted, POST may fail schema checks. Default to [].
- const unitPayload = { + const unitPayload = { unitData: { name: name ?? faker.string.uuid(), visibility: visibility ?? 'public', }, - unitMonitors: monitors, - unitDepartments: departments, + unitMonitors: monitors ?? [], + unitDepartments: departments ?? [], };
31-35: Accept 200 or 201 on create/update.Some backends return 201 for create. Broaden the check.
- if (response.status() !== 200) { - throw new Error(`Failed to create or update unit [http status: ${response.status()}]`); - } + if (![200, 201].includes(response.status())) { + throw new Error(`Failed to create or update unit [http status: ${response.status()}]`); + }apps/meteor/client/omnichannel/units/UnitEdit.tsx (2)
37-48: Make onUpdate explicitly async-capable.onUpdate is awaited; declare it as possibly returning a Promise for clarity.
- onUpdate?: (params: { + onUpdate?: (params: { unitData: { name: string; visibility: string; enabled?: boolean; description?: string; email?: string; showOnOfflineForm?: boolean; }; unitMonitors: { monitorId: string; username: string }[]; unitDepartments: { departmentId: string }[]; - }) => void; + }) => void | Promise<void>;
55-55: Safer fallback for updates without onUpdate; narrow visibility typing.
- If _id exists and onUpdate is not provided, post to /v1/livechat/units/:id to update instead of creating.
- Narrow visibility to 'public' | 'private' to match options.
-const saveUnit = useEndpoint('POST', '/v1/livechat/units'); +const saveUnit = useEndpoint('POST', '/v1/livechat/units'); +const editUnit = useEndpoint('POST', '/v1/livechat/units/:id'); // ... -const payload = { +const payload = { unitData: { name, visibility }, unitMonitors: monitorsData, unitDepartments: departmentsData, }; try { - if (_id && onUpdate) { + if (_id && onUpdate) { await onUpdate(payload); - } else { - await saveUnit(payload); + } else if (_id) { + await editUnit({ id: _id, ...payload }); + } else { + await saveUnit(payload); }Optional typing tweak:
-type UnitEditFormData = { +type UnitEditFormData = { name: string; - visibility: string; + visibility: 'public' | 'private'; // ... };Also applies to: 109-114, 116-120
apps/meteor/ee/app/livechat-enterprise/server/api/units.ts (1)
19-30: Consider extracting the duplicated payload type definition.The POST parameter structure for both
/v1/livechat/unitsand/v1/livechat/units/:idis identical. Consider extracting this into a shared type to reduce duplication and improve maintainability.Example refactor:
+type SaveUnitPayload = { + unitData: { + name: string; + visibility: string; + enabled?: boolean; + description?: string; + email?: string; + showOnOfflineForm?: boolean; + }; + unitMonitors: { monitorId: string; username: string }[]; + unitDepartments: { departmentId: string }[]; +}; + declare module '@rocket.chat/rest-typings' { // eslint-disable-next-line @typescript-eslint/naming-convention interface Endpoints { '/v1/livechat/units': { GET: (params: PaginatedRequest<{ text?: string }>) => PaginatedResult & { units: IOmnichannelBusinessUnit[] }; - POST: (params: { - unitData: { - name: string; - visibility: string; - enabled?: boolean; - description?: string; - email?: string; - showOnOfflineForm?: boolean; - }; - unitMonitors: { monitorId: string; username: string }[]; - unitDepartments: { departmentId: string }[]; - }) => Omit<IOmnichannelBusinessUnit, '_updatedAt'>; + POST: (params: SaveUnitPayload) => Omit<IOmnichannelBusinessUnit, '_updatedAt'>; }; '/v1/livechat/units/:id': { GET: () => IOmnichannelBusinessUnit; - POST: (params: { - unitData: { - name: string; - visibility: string; - enabled?: boolean; - description?: string; - email?: string; - showOnOfflineForm?: boolean; - }; - unitMonitors: { monitorId: string; username: string }[]; - unitDepartments: { departmentId: string }[]; - }) => Omit<IOmnichannelBusinessUnit, '_updatedAt'>; + POST: (params: SaveUnitPayload) => Omit<IOmnichannelBusinessUnit, '_updatedAt'>; DELETE: () => number; }; } }Also applies to: 34-45
📜 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 (7)
.changeset/cool-yaks-perform.md(1 hunks)apps/meteor/client/omnichannel/units/UnitEdit.tsx(3 hunks)apps/meteor/client/omnichannel/units/UnitEditWithData.tsx(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/units.ts(1 hunks)apps/meteor/ee/app/livechat-enterprise/server/methods/saveUnit.ts(2 hunks)apps/meteor/tests/data/livechat/units.ts(1 hunks)apps/meteor/tests/e2e/utils/omnichannel/units.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/units.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/units.ts
🧬 Code graph analysis (3)
apps/meteor/tests/e2e/utils/omnichannel/units.ts (1)
apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
removeUnit(53-64)
apps/meteor/tests/data/livechat/units.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
apps/meteor/client/omnichannel/units/UnitEdit.tsx (1)
apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
saveUnit(66-128)
🔇 Additional comments (3)
apps/meteor/client/omnichannel/units/UnitEditWithData.tsx (1)
67-67: Prop pass-throughonClose forwarding preserved. No issue.
apps/meteor/ee/app/livechat-enterprise/server/api/units.ts (2)
115-120: Handler logic looks correct; same validation concern applies.The update handler correctly extracts the unit ID from URL parameters and passes it along with the payload to
LivechatEnterprise.saveUnit(). The same input validation concern mentioned for the create handler (lines 95-98) applies here as well.
95-98: The original review comment is incorrect. Runtime validation is already implemented.The
LivechatEnterprise.saveUnit()method inapps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts(lines 73–93) uses Meteor'scheck()function to validate all required fields:
unitData.nameandunitData.visibilityare validated as required stringsunitMonitorsis validated as an array of objects with requiredmonitorIdandusernamefieldsunitDepartmentsis validated as an array of objects with requireddepartmentIdfieldMeteor's
check()function provides runtime validation with clear error messages when validation fails. The handler at lines 95–98 correctly delegates tosaveUnit(), which performs comprehensive input validation before processing.Likely an incorrect or invalid review comment.
apps/meteor/ee/app/livechat-enterprise/server/methods/saveUnit.ts
Outdated
Show resolved
Hide resolved
apps/meteor/ee/app/livechat-enterprise/server/methods/saveUnit.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 (3)
apps/meteor/client/omnichannel/units/UnitEdit.tsx(3 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/units.ts(2 hunks)packages/core-typings/src/IOmnichannelBusinessUnit.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/omnichannel/units/UnitEdit.tsx (2)
packages/core-typings/src/IOmnichannelBusinessUnit.ts (1)
ISaveOmnichannelBusinessUnit(14-25)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
saveUnit(66-128)
apps/meteor/ee/app/livechat-enterprise/server/api/units.ts (1)
packages/core-typings/src/IOmnichannelBusinessUnit.ts (2)
ISaveOmnichannelBusinessUnit(14-25)IOmnichannelBusinessUnit(3-12)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/ee/app/livechat-enterprise/server/api/units.ts (2)
1-1: LGTM! Type annotations improve API contract clarity.The addition of
ISaveOmnichannelBusinessUnitas the parameter type for both POST endpoints clearly documents the expected payload structure. This addresses the previous review comment about creating a type for these endpoints.Based on past review comments.
Also applies to: 19-19, 23-23
19-19: Remove this review comment - the return type omission is correct and intentional.The POST response intentionally omits
_updatedAtand this is safe. Client code (UnitEdit.tsx line 114) callsawait saveUnit(payload)without capturing or using the response. After the POST succeeds, the client invalidates the cache and re-fetches data via GET endpoints, which return the full object including_updatedAt. This is a valid optimization pattern: POST returns minimal response while GET provides complete data.apps/meteor/client/omnichannel/units/UnitEdit.tsx (1)
1-7: LGTM! Proper type imports for the new payload structure.The import changes correctly bring in
ISaveOmnichannelBusinessUnitand add it to the component's prop types, aligning with the REST API migration.Also applies to: 43-43
lucas-a-pelegrino
added
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
dougfabris
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Co-authored-by: Douglas Fabris <[email protected]>
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning on
livechat:saveUnitmethod, and updates client to use the alternative endpoints already in place.Issue(s)
CORE-1409
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Deprecations
Refactor
New Features
Tests
Chores