chore: Adds deprecation warning on livechat:removeTag with new endpoint to replace it
#37136
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: 8038280 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 a POST /v1/livechat/tags.delete REST endpoint with request/response typings and i18n text, migrates client hook and E2E helper to use the new endpoint, logs deprecation for the livechat:removeTag Meteor method, and updates a changeset with package bumps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant Hook as useRemoveTag (client)
participant API as REST /v1/livechat/tags.delete
participant SVC as LivechatEnterprise
rect rgba(200,235,255,0.25)
note over UI,Hook: New REST removal flow
UI->>Hook: removeTag(tagId)
Hook->>API: POST /v1/livechat/tags.delete { id: tagId }
API->>SVC: LivechatEnterprise.removeTag(id)
SVC-->>API: success / throws Meteor.Error
API-->>Hook: success or error response
Hook-->>UI: show toast / invalidate queries
end
rect rgba(255,230,200,0.18)
note over API: Deprecated method logging (server-side)
API-->>API: methodDeprecationLogger.method('livechat:removeTag', '8.0.0', '/v1/livechat/tags.delete')
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 (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)
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 #37136 +/- ##
===========================================
+ Coverage 67.11% 67.61% +0.49%
===========================================
Files 3264 3341 +77
Lines 112926 114016 +1090
Branches 20445 20669 +224
===========================================
+ Hits 75791 77091 +1300
+ Misses 34548 34245 -303
- Partials 2587 2680 +93
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: 1
📜 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/shaggy-clocks-allow.md(1 hunks)apps/meteor/client/omnichannel/tags/useRemoveTag.tsx(1 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/methods/removeTag.ts(2 hunks)apps/meteor/tests/e2e/utils/omnichannel/tags.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(2 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/tags.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/tags.ts
🧬 Code graph analysis (1)
apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatTagsRemoveSuccessResponse(544-544)isPOSTLivechatTagsRemoveParams(531-531)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
LivechatEnterprise(13-183)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
🔇 Additional comments (9)
.changeset/shaggy-clocks-allow.md (1)
1-6: LGTM!The changeset format is correct, and patch bumps are appropriate for deprecation warnings. The description clearly summarizes the change.
apps/meteor/ee/app/livechat-enterprise/server/methods/removeTag.ts (2)
6-6: LGTM!The import is correct and necessary for logging the deprecation warning.
18-18: No remaining calls tolivechat:removeTagDeprecation warning placement is appropriate and no Meteor.call or DDP client invocations remain.packages/i18n/src/locales/en.i18n.json (1)
6219-6222: Translation addition looks goodNew error string slots in cleanly with the existing messages; no issues spotted.
apps/meteor/client/omnichannel/tags/useRemoveTag.tsx (1)
3-3: LGTM! Clean migration to REST endpoint.The transition from the deprecated Meteor method to the new REST endpoint is implemented correctly:
- Import updated to use
useEndpoint- Endpoint properly configured as POST to
/v1/livechat/tags.remove- Payload structure updated from
tagIdto{ id: tagId }matching the REST API schemaAlso applies to: 11-11, 18-18
apps/meteor/tests/e2e/utils/omnichannel/tags.ts (1)
13-13: LGTM! Test utility updated correctly.The test helper now correctly calls the new REST endpoint
/livechat/tags.removewith the proper payload structure.packages/rest-typings/src/v1/omnichannel.ts (1)
516-544: LGTM! Validation schemas properly defined.The validation schemas for the new endpoint are correctly implemented:
- Request body requires an
idstring parameter- Success response properly typed with
{ success: true }- Follows existing patterns in the codebase
The
ajv.compile<void>pattern for the success response validator is consistent with similar endpoints elsewhere in this file (e.g., lines 1451, 1796, 4289).apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts (2)
1-13: LGTM! Imports properly updated.All necessary imports are in place for the new endpoint implementation, including validation schemas, type utilities, and the LivechatEnterprise service.
100-105: LGTM! Type augmentation correctly implemented.The type declarations and module augmentation properly extend the Endpoints interface with the new route, ensuring type safety across the codebase.
lucas-a-pelegrino
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts (1)
95-95: i18n key coverage for 'error-removing-tag'Previous review asked for translations. Please confirm all locales include this key; otherwise clients will show the raw key.
Run:
#!/bin/bash # List locale files missing the 'error-removing-tag' key fd -t f -e json packages/i18n/src/locales | while read -r f; do if ! rg -q '"error-removing-tag"\s*:' "$f"; then echo "Missing in: $f"; fi done
🧹 Nitpick comments (2)
apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts (2)
73-79: Optional: return 404 for not found and validate itMap 404 and surface
tag-not-foundas Not Found to match GET behavior.Apply:
@@ - response: { + response: { + 404: validateNotFoundErrorResponse, },@@ - if (error instanceof Meteor.Error) { - return API.v1.failure(error.reason); - } + if (error instanceof Meteor.Error) { + if (error.error === 'tag-not-found') { + return API.v1.notFound('tag-not-found'); + } + return API.v1.failure(error.error || 'error-removing-tag'); + }@@ -import { +import { isPOSTLivechatTagsRemoveParams, POSTLivechatTagsRemoveSuccessResponse, validateBadRequestErrorResponse, validateForbiddenErrorResponse, validateUnauthorizedErrorResponse, + validateNotFoundErrorResponse, } from '@rocket.chat/rest-typings';As LivechatEnterprise.removeTag throws
Meteor.Error('tag-not-found', ...). Based on learningsAlso applies to: 91-96, 1-7
1-7: Meteor import for type-safety
Meteor.Erroris referenced; if globals aren’t declared in this compilation unit, import Meteor to satisfy types.Apply:
} from '@rocket.chat/rest-typings'; +import { Meteor } from 'meteor/meteor';
📜 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 (4)
apps/meteor/client/omnichannel/tags/useRemoveTag.tsx(1 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/methods/removeTag.ts(2 hunks)apps/meteor/tests/e2e/utils/omnichannel/tags.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/omnichannel/tags/useRemoveTag.tsx
- apps/meteor/ee/app/livechat-enterprise/server/methods/removeTag.ts
- apps/meteor/tests/e2e/utils/omnichannel/tags.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/ee/app/livechat-enterprise/server/api/tags.ts (6)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatTagsRemoveSuccessResponse(544-544)isPOSTLivechatTagsRemoveParams(531-531)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
LivechatEnterprise(13-183)apps/meteor/ee/app/livechat-enterprise/server/methods/removeTag.ts (1)
id(17-26)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
⏰ 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/ee/app/livechat-enterprise/server/api/tags.ts (1)
71-72: Endpoint name vs PR descriptionCode exposes
livechat/tags.delete. PR description mentionslivechat/tags.remove. Ensure docs, deprecation message, clients, and tests consistently use.delete(the deprecation logger already points to/v1/livechat/tags.delete).
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:removeTaglegacy meteor method, as well as a new endpoint to replace it;livechat/tags.removeIssue(s)
CORE-1414
Steps to test or reproduce
Further comments
Summary by CodeRabbit