Skip to content

Conversation

@lucas-a-pelegrino
Copy link
Contributor

@lucas-a-pelegrino lucas-a-pelegrino commented Sep 24, 2025

Proposed changes (including videos or screenshots)

This PR adds a deprecation warning for removeCannedResponse meteor method, as well as it updates useRemoveCannedResponse to use corresponding endpoint.

Issue(s)

CTZ-72
CORE-1408

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Refactor
    • Unified canned-response deletion UI with a dedicated remove button and on-delete callback for edit flows; deletion behavior centralized.
  • API
    • Delete now uses path-based endpoint: DELETE /v1/canned-responses/:_id; collection-level DELETE is deprecated.
  • Deprecation
    • Removal method now emits a deprecation warning directing to the new endpoint.
  • Tests
    • End-to-end tests updated for path-based deletion.
  • Chores
    • Patch version bump.

@lucas-a-pelegrino lucas-a-pelegrino added this to the 7.12.0 milestone Sep 24, 2025
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 24, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Sep 24, 2025

🦋 Changeset detected

Latest commit: 8b26481

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/federation-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Moves canned-response deletion from a body-based Meteor method to a RESTful DELETE /v1/canned-responses/:_id endpoint; refactors client to use a new useRemoveCannedResponse(id) hook, adds RemoveCannedResponseButton, wires onDelete props through edit components, logs deprecation for the old method, and updates E2E tests.

Changes

Cohort / File(s) Summary
Changeset
.changeset/late-papayas-swim.md
Adds a patch changeset noting a deprecation warning on removeCannedResponse.
Client — edit components
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEdit.tsx, apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithData.tsx, apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithDepartmentData.tsx
CannedResponseEdit gains an optional onDelete?: () => void prop and no longer calls an internal delete hook; parent components obtain a delete handler from useRemoveCannedResponse(id) and pass it down.
Client — table & button
apps/meteor/client/omnichannel/cannedResponses/CannedResponsesTable.tsx, apps/meteor/client/omnichannel/cannedResponses/RemoveCannedResponseButton.tsx
Replaces inline delete cell with new RemoveCannedResponseButton that uses useRemoveCannedResponse(id), stops event propagation, and invokes deletion.
Client — deletion hook
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx
Adds useRemoveCannedResponse(id: IOmnichannelCannedResponse['_id']), switches to useEndpoint for DELETE /v1/canned-responses/:_id, and preserves modal/toast/cache invalidation/navigation side effects.
Server (EE) — API routes
apps/meteor/ee/app/api-enterprise/server/canned-responses.ts
Removes body-based DELETE from /v1/canned-responses; adds DELETE on /v1/canned-responses/:_id with explicit permissions and handler calling removeCannedResponse(userId, _id); marks the old route deprecated and updates endpoint typings.
Server (EE) — method deprecation
apps/meteor/ee/app/canned-responses/server/methods/removeCannedResponse.ts
Adds a deprecation log call methodDeprecationLogger.method('removeCannedResponse', '8.0.0', 'DELETE /v1/canned-responses/:_id') to the legacy Meteor method.
E2E tests
apps/meteor/tests/end-to-end/api/livechat/15-canned-responses.ts
Updates tests to use path-based DELETE (/v1/canned-responses/:_id) instead of sending _id in the body; asserts permission/validation behavior (e.g., 403 for unauthorized, 400 for missing id, 200 on success).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant UI as Client UI (Table / Edit)
  participant Hook as useRemoveCannedResponse(id)
  participant API as DELETE /v1/canned-responses/:_id
  participant S as Server Handler
  participant Store as Client Cache / Router

  U->>UI: Click "Delete"
  UI->>Hook: onDelete()
  Note right of Hook #DDEBF7: show confirm modal → call endpoint
  Hook->>API: DELETE /v1/canned-responses/:_id
  API->>S: validate auth & permissions
  S-->>API: 200 success
  API-->>Hook: success
  Hook->>Store: invalidate getCannedResponses, show toast, navigate list
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo
  • dougfabris

Poem

I nibble paths and hop along the trail,
IDs move to URLs on the breeze of the night.
A button taps, a hook hums, the old call logs farewell,
Tests march forward, tidy and bright.
I twitch my whiskers — deletion feels right. 🐇

Pre-merge checks and finishing touches

❌ Failed checks
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title accurately references the deprecation warning for removeCannedResponse, which is a genuine part of the changes, but it does not convey the broader migration of deletion logic to the new endpoint and associated UI and hook updates. As a result, it only partially reflects the scope of this changeset, although it remains concise and clear. Overall, the title is sufficiently related but could be more descriptive of key refactorings. Consider expanding the title to mention the migration to the new deletion endpoint and useRemoveCannedResponse hook to better reflect all major changes. This will help teammates quickly understand the full scope of refactoring and UI updates.
Linked Issues Check ⚠️ Warning The pull request implements all core objectives of CORE-1408 by adding a deprecation warning in the removeCannedResponse Meteor method, updating the useRemoveCannedResponse hook to call the new endpoint, adjusting UI components to use the hook, updating backend routes with endpoint deprecation metadata, and updating end-to-end tests to reflect the new RESTful delete paths. These changes fully satisfy the requirements to deprecate the old method and migrate deletion logic to the endpoint. No additional coding requirements from the linked issue appear outstanding.
Out of Scope Changes Check ⚠️ Warning All code changes directly relate to deprecating the Meteor method removeCannedResponse and migrating deletion functionality to the new endpoint via useRemoveCannedResponse and corresponding UI updates. There are no unrelated or extraneous modifications outside of the context of removing and centralizing deletion logic. Thus the changes remain within the objectives of CORE-1408.
Docstring Coverage ⚠️ Warning No functions found in the changes. Docstring coverage check skipped.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit's high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/v7/CTZ-72

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 23.52941% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.31%. Comparing base (46f8110) to head (8b26481).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37051      +/-   ##
===========================================
+ Coverage    67.30%   67.31%   +0.01%     
===========================================
  Files         3277     3276       -1     
  Lines       111998   111860     -138     
  Branches     20315    20312       -3     
===========================================
- Hits         75376    75299      -77     
+ Misses       34018    33963      -55     
+ Partials      2604     2598       -6     
Flag Coverage Δ
e2e 57.30% <23.52%> (-0.04%) ⬇️
unit 71.09% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lucas-a-pelegrino lucas-a-pelegrino marked this pull request as ready for review September 25, 2025 21:00
@lucas-a-pelegrino lucas-a-pelegrino requested review from a team as code owners September 25, 2025 21:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (4)
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)

21-26: Also invalidate the detail query to prevent stale caches

When deleting from the edit view, invalidate the per-id cache too.

       await removeCannedResponse();
       queryClient.invalidateQueries({
         queryKey: ['getCannedResponses'],
       });
+      queryClient.invalidateQueries({
+        queryKey: ['getCannedResponseById', id],
+      });
       router.navigate('/omnichannel/canned-responses');
       dispatchToastMessage({ type: 'success', message: t('Canned_Response_Removed') });
apps/meteor/tests/end-to-end/api/livechat/15-canned-responses.ts (1)

255-258: Rename test to reflect path-based 404

The title references body param; now it’s 404 due to missing path param.

-it('should fail if _id is not on the request', async () => {
+it('should 404 when DELETE /canned-responses has no :_id path parameter', async () => {
   await updatePermission('remove-canned-responses', ['livechat-agent', 'livechat-monitor', 'livechat-manager', 'admin']);
   return request.delete(api('canned-responses')).set(credentials).expect(404);
 });
apps/meteor/client/omnichannel/cannedResponses/RemoveCannedResponseButton.tsx (1)

15-23: Add aria-label for accessibility

Provide an explicit accessible name for the icon button.

-      <IconButton
+      <IconButton
         icon='trash'
         small
+        aria-label={t('Remove')}
         title={t('Remove')}
         onClick={(e) => {
           e.stopPropagation();
           handleDelete();
         }}
       />
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEdit.tsx (1)

81-86: Guard Delete button rendering when onDelete is not provided

Prevents a runtime error if a caller omits onDelete.

-        {cannedResponseData?._id && (
+        {cannedResponseData?._id && onDelete && (
           <ButtonGroup>
             <Button danger onClick={onDelete}>
               {t('Delete')}
             </Button>
           </ButtonGroup>
         )}
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 65b57cc and 4abfe98.

📒 Files selected for processing (10)
  • .changeset/late-papayas-swim.md (1 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/CannedResponseEdit.tsx (3 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithData.tsx (2 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithDepartmentData.tsx (2 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/CannedResponsesTable.tsx (2 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/RemoveCannedResponseButton.tsx (1 hunks)
  • apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1 hunks)
  • apps/meteor/ee/app/api-enterprise/server/canned-responses.ts (5 hunks)
  • apps/meteor/ee/app/canned-responses/server/methods/removeCannedResponse.ts (2 hunks)
  • apps/meteor/tests/end-to-end/api/livechat/15-canned-responses.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/meteor/client/omnichannel/cannedResponses/RemoveCannedResponseButton.tsx (1)
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)
  • useRemoveCannedResponse (8-45)
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEdit.tsx (1)
packages/livechat/src/components/Button/index.tsx (1)
  • Button (34-99)
apps/meteor/tests/end-to-end/api/livechat/15-canned-responses.ts (2)
apps/meteor/tests/data/api-data.ts (2)
  • request (10-10)
  • credentials (39-42)
apps/meteor/tests/data/livechat/canned-responses.ts (1)
  • createCannedResponse (7-25)
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)
packages/ui-contexts/src/index.ts (1)
  • useEndpoint (32-32)
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithData.tsx (2)
packages/ui-contexts/src/index.ts (1)
  • useEndpoint (32-32)
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)
  • useRemoveCannedResponse (8-45)
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithDepartmentData.tsx (1)
apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)
  • useRemoveCannedResponse (8-45)
apps/meteor/ee/app/api-enterprise/server/canned-responses.ts (1)
packages/rest-typings/src/v1/omnichannel.ts (2)
  • isPOSTCannedResponsesProps (2883-2883)
  • isCannedResponsesProps (998-998)
🔇 Additional comments (7)
.changeset/late-papayas-swim.md (1)

1-5: Changelog entry looks good.

Patch bump and deprecation note are captured clearly.

apps/meteor/client/omnichannel/cannedResponses/useRemoveCannedResponse.tsx (1)

15-15: LGTM: using DELETE /v1/canned-responses/:_id

Endpoint binding with path params is correct.

apps/meteor/client/omnichannel/cannedResponses/CannedResponsesTable.tsx (1)

171-171: LGTM: centralized delete via RemoveCannedResponseButton

Good consolidation and event propagation stop handled in the button.

apps/meteor/tests/end-to-end/api/livechat/15-canned-responses.ts (2)

253-253: LGTM: unauthorized delete now targets path-based endpoint

403 expectation remains correct.


262-265: LGTM: switched to DELETE /canned-responses/:_id

Matches new API contract and keeps response assertions intact.

apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithData.tsx (1)

39-39: LGTM: delegate delete via onDelete

Wires the new hook through props cleanly.

apps/meteor/ee/app/api-enterprise/server/canned-responses.ts (1)

112-135: LGTM!

Splitting the permissions per verb and moving the delete handler onto the id-scoped route aligns with the updated endpoint contract without introducing regressions.

@dougfabris dougfabris requested a review from a team September 25, 2025 21:19
dougfabris
dougfabris previously approved these changes Sep 25, 2025
Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between f71fbf4 and 6b7cd03.

📒 Files selected for processing (1)
  • apps/meteor/ee/app/api-enterprise/server/canned-responses.ts (6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/ee/app/api-enterprise/server/canned-responses.ts (1)
packages/rest-typings/src/v1/omnichannel.ts (3)
  • isPOSTCannedResponsesProps (2887-2887)
  • isDELETECannedResponsesProps (2904-2904)
  • isCannedResponsesProps (998-998)
⏰ 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

Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frontend looks good to me!

@tassoevan tassoevan added the stat: QA assured Means it has been tested and approved by a company insider label Oct 6, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants