chore: Adds deprecation warning on livechat:removeCustomField with new endpoint replacing it
#36933
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 9a22b82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 REST endpoint POST /v1/livechat/custom-fields.delete with validators and typings, logs deprecation for the legacy Meteor method Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant REST as "REST API\n/v1/livechat/custom-fields.delete"
participant DB as "LivechatCustomField\nModel"
Note right of Client #E8F6FF: Client now calls REST endpoint with object payload
Client->>REST: POST /v1/livechat/custom-fields.delete\n{ customFieldId }
REST->>DB: findOneById(customFieldId)
alt found
DB-->>REST: document
REST->>DB: removeById(customFieldId)
DB-->>REST: DeleteResult (deletedCount>0)
REST-->>Client: 200 { success: true }
else not found
REST-->>Client: 400 { error: 'Custom field not found' }
end
Note right of REST #FFF7E6: Legacy Meteor method kept (now logs deprecation)
Client--xMethod: method call (livechat:removeCustomField)
Method->>Method: methodDeprecationLogger.method(...)
Method->>DB: findOneById / removeById
DB-->>Method: result/error
Method-->>Client: result/error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
![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 (5)
apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx (1)
14-27: Polish: rename handler and stringify error for toasts.
- Rename onDeleteAgent to onConfirmDelete (or similar) for clarity.
- Ensure message is a string to avoid rendering [object Object] in toasts.
Apply this diff:
- const onDeleteAgent = async () => { + const onConfirmDelete = async () => { try { await removeCustomField({ _id: id }); dispatchToastMessage({ type: 'success', message: t('Custom_Field_Removed') }); queryClient.invalidateQueries({ queryKey: ['livechat-customFields'], }); } catch (error) { - dispatchToastMessage({ type: 'error', message: error }); + dispatchToastMessage({ type: 'error', message: (error as any)?.message ?? String(error) }); } finally { setModal(); } - }; + }; ... - setModal(<GenericModal variant='danger' onConfirm={onDeleteAgent} onCancel={() => setModal()} confirmText={t('Delete')} />); + setModal(<GenericModal variant='danger' onConfirm={onConfirmDelete} onCancel={() => setModal()} confirmText={t('Delete')} />);.changeset/cool-pets-switch.md (1)
6-6: Tighten wording in the changeset entry.
Minor grammar/clarity tweak.-Adds deprecation warning on `livechat:removeCustomField` with new endpoint replacing it; `livechat/custom-fields.remove` +Deprecates `livechat:removeCustomField` and introduces the replacement endpoint `livechat/custom-fields.remove`.packages/rest-typings/src/v1/omnichannel.ts (1)
3886-3902: Name consistency: consider singular for the remove payload.
The type/validator is named POSTLivechatRemoveCustomFields (plural) but the payload removes a single item. Consider renaming to singular to match intent; update imports accordingly.-type POSTLivechatRemoveCustomFields = { +type POSTLivechatRemoveCustomField = { _id: string; }; -const POSTLivechatRemoveCustomFieldsSchema = { +const POSTLivechatRemoveCustomFieldSchema = { type: 'object', properties: { _id: { type: 'string', }, }, required: ['_id'], additionalProperties: false, }; -export const isPOSTLivechatRemoveCustomFields = ajv.compile<POSTLivechatRemoveCustomFields>(POSTLivechatRemoveCustomFieldsSchema); +export const isPOSTLivechatRemoveCustomFields = ajv.compile<POSTLivechatRemoveCustomField>(POSTLivechatRemoveCustomFieldSchema);Note: this also requires updating the import usage in apps/meteor/app/livechat/server/api/v1/customField.ts.
apps/meteor/app/livechat/server/api/v1/customField.ts (2)
103-105: Remove inline question comment; confirm permission.
The comment “is this permission appropriate...?” should not ship. If unsure, confirm and keep code clean.- permissionsRequired: ['view-livechat-manager'], // is this permission appropriate for the targeted action? + permissionsRequired: ['view-livechat-manager'],If product expects a more granular permission (e.g., manage-livechat-custom-fields), adjust before merge.
94-106: Optional: use DELETE with path param for REST semantics.
Future improvement: expose DELETE /v1/livechat/custom-fields/:_id instead of POST ...remove.Happy to draft a follow-up PR if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/cool-pets-switch.md(1 hunks)apps/meteor/app/livechat/server/api/v1/customField.ts(2 hunks)apps/meteor/app/livechat/server/methods/removeCustomField.ts(2 hunks)apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx(1 hunks)apps/meteor/tests/data/livechat/custom-fields.ts(1 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center.spec.ts(0 hunks)packages/rest-typings/src/v1/omnichannel.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center.spec.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/tests/data/livechat/custom-fields.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/tests/e2e/utils/omnichannel/custom-field.ts (1)
removeCustomField(8-15)
apps/meteor/app/livechat/server/api/v1/customField.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatRemoveCustomFieldSuccess(3919-3919)isPOSTLivechatRemoveCustomFields(3901-3901)packages/rest-typings/src/v1/Ajv.ts (2)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)packages/models/src/index.ts (1)
LivechatCustomField(170-170)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
⏰ 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 (3)
apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx (1)
11-11: LGTM: endpoint migration and payload shape are correct.
Switch to POST /v1/livechat/custom-fields.remove with {_id} looks good.apps/meteor/app/livechat/server/methods/removeCustomField.ts (1)
19-19: Confirm deprecation target version.
Verify that '8.0.0' is the intended removal target/version shown to integrators; update if the roadmap differs.apps/meteor/tests/data/livechat/custom-fields.ts (1)
45-49: LGTM: test helper now targets the new REST endpoint.
Ordering of .set(credentials) before .send and payload {_id} are correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36933 +/- ##
===========================================
- Coverage 66.58% 66.56% -0.03%
===========================================
Files 3346 3346
Lines 114666 114666
Branches 21100 21099 -1
===========================================
- Hits 76356 76330 -26
- Misses 35618 35645 +27
+ Partials 2692 2691 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/livechat/server/api/v1/customField.ts (1)
109-112: Keep error code consistent with the deprecated method.Past feedback asked to reuse the legacy error id for parity; switch to the same error string clients already expect.
Apply:
- if (!customField) { - return API.v1.failure('custom-field-not-found'); - } + if (!customField) { + return API.v1.failure('error-invalid-custom-field'); + }
🧹 Nitpick comments (2)
apps/meteor/app/livechat/server/api/v1/customField.ts (2)
109-117: Consider idempotent delete semantics (return 200 with deletedCount: 0 when not found).Optional, but it simplifies clients and aligns with many REST designs.
Apply:
- if (!customField) { - return API.v1.failure('custom-field-not-found'); - } + if (!customField) { + return API.v1.success({ acknowledged: true, deletedCount: 0 }); + }
116-116: Minor: avoid unnecessary spread in success payload.
resultalready matches the expected shape.Apply:
- return API.v1.success({ ...result }); + return API.v1.success(result);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/livechat/server/api/v1/customField.ts(2 hunks)packages/rest-typings/src/v1/omnichannel.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rest-typings/src/v1/omnichannel.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/api/v1/customField.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatRemoveCustomFieldSuccess(3920-3920)isPOSTLivechatRemoveCustomFields(3901-3901)packages/rest-typings/src/v1/Ajv.ts (2)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)packages/models/src/index.ts (1)
LivechatCustomField(170-170)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
⏰ 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 (2)
apps/meteor/app/livechat/server/api/v1/customField.ts (2)
1-10: Imports and typings wiring look correct.Validators and ExtractRoutesFromAPI are correctly referenced and used.
Also applies to: 13-13
120-125: Run a TypeScript type-check and ensure the module augmentation is visible to packages/rest-typings.Endpoint exists at apps/meteor/app/livechat/server/api/v1/customField.ts and is consumed at apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx, but rg returned no matches under packages/rest-typings — run the repo typecheck (e.g. yarn -w tsc --noEmit) or move/add the module augmentation into packages/rest-typings so Endpoints includes the new route.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
3903-3915: Type the success schema as the actual payload.Compile the validator to
{ success: true }instead ofvoidfor better type-safety and parity with API.v1.success.export const POSTLivechatRemoveCustomFieldSuccessSchema = { type: 'object', properties: { success: { type: 'boolean', enum: [true], }, }, additionalProperties: false, }; -export const POSTLivechatRemoveCustomFieldSuccess = ajv.compile<void>(POSTLivechatRemoveCustomFieldSuccessSchema); +export const POSTLivechatRemoveCustomFieldSuccess = ajv.compile<{ success: true }>( + POSTLivechatRemoveCustomFieldSuccessSchema, +);
🧹 Nitpick comments (4)
apps/meteor/app/livechat/server/methods/removeCustomField.ts (1)
21-25: Consider aligning permission with the REST route (or a “manage” permission).This method still checks 'view-livechat-manager'. If the REST route moves to a manage-level permission, mirror it here for consistency.
apps/meteor/tests/data/livechat/custom-fields.ts (1)
45-55: Surface failures explicitly in the helper.Add an HTTP status assertion and reject when success !== true to avoid false positives in tests.
void request .post(api('livechat/custom-fields.delete')) .set(credentials) + .expect(200) .send({ _id: customFieldID, }) .end((err: Error, res: Response): void => { if (err) { return reject(err); } - resolve(res.body); + if (!res?.body?.success) { + return reject(new Error(`deleteCustomField failed: ${JSON.stringify(res.body)}`)); + } + resolve(res.body); });packages/rest-typings/src/v1/omnichannel.ts (1)
3886-3901: Nit: singularize request type name for clarity.Only a single _id is accepted; consider singular naming.
-type POSTLivechatRemoveCustomFields = { +type POSTLivechatRemoveCustomField = { _id: string; }; -const POSTLivechatRemoveCustomFieldsSchema = { +const POSTLivechatRemoveCustomFieldSchema = { type: 'object', properties: { _id: { type: 'string', }, }, required: ['_id'], additionalProperties: false, }; -export const isPOSTLivechatRemoveCustomFields = ajv.compile<POSTLivechatRemoveCustomFields>(POSTLivechatRemoveCustomFieldsSchema); +export const isPOSTLivechatRemoveCustomField = ajv.compile<POSTLivechatRemoveCustomField>(POSTLivechatRemoveCustomFieldSchema);Also update usages in the route file:
// apps/meteor/app/livechat/server/api/v1/customField.ts import { isPOSTLivechatRemoveCustomField, POSTLivechatRemoveCustomFieldSuccess } from '@rocket.chat/rest-typings'; body: isPOSTLivechatRemoveCustomField,apps/meteor/app/livechat/server/api/v1/customField.ts (1)
109-112: Use a consistent error code with the Meteor method.Return the same error code ('error-invalid-custom-field') used by the Meteor method for easier client handling.
- if (result.deletedCount === 0) { - return API.v1.failure('Custom field not found'); - } + if (result.deletedCount === 0) { + return API.v1.failure('error-invalid-custom-field'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/meteor/app/livechat/server/api/v1/customField.ts(2 hunks)apps/meteor/app/livechat/server/methods/removeCustomField.ts(2 hunks)apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx(1 hunks)apps/meteor/tests/data/livechat/custom-fields.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/omnichannel/customFields/useRemoveCustomField.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/tests/data/livechat/custom-fields.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
apps/meteor/app/livechat/server/api/v1/customField.ts (6)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatRemoveCustomFieldSuccess(3914-3914)isPOSTLivechatRemoveCustomFields(3901-3901)packages/rest-typings/src/v1/Ajv.ts (2)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)packages/models/src/index.ts (1)
LivechatCustomField(170-170)apps/meteor/app/livechat/server/methods/removeCustomField.ts (1)
_id(18-37)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/livechat/server/methods/removeCustomField.ts (1)
8-8: Deprecation hook added — LGTM; confirm sunset target.The deprecation warning and replacement path look correct. Please confirm 8.0.0 is the intended removal version for this method.
Also applies to: 19-19
apps/meteor/app/livechat/server/api/v1/customField.ts (1)
98-105: Require manage-level permission and add 403 validator
- Change permissionsRequired: ['view-livechat-manager'] to a manage-level permission for custom fields (e.g. 'manage-livechat-customfields'); if that permission doesn't exist, add it to the permissions constants and grant appropriate roles. Location: apps/meteor/app/livechat/server/api/v1/customField.ts (POST remove route).
- Add 403: validateForbiddenErrorResponse to the route responses (validator available at packages/rest-typings/src/v1/Ajv.ts).
lucas-a-pelegrino
added
stat: ready to merge
kodiakhq
bot
removed
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
lucas-a-pelegrino
dismissed stale reviews from tassoevan and sampaiodiego
via
b5f180c
lucas-a-pelegrino
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:removeCustomFieldmeteor method, as well as it adds an endpoint (livechat/custom-fields.remove) to replace it.Issue(s)
CTZ-59
Steps to test or reproduce
Further comments
Tests will be implemented on this task
Summary by CodeRabbit
New Features
Deprecations
UI
Tests
Chores
Rest typings