Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: New livechat conversations are not assigned to contact manager #34210

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

matheusbsilva137
Copy link
Member

@matheusbsilva137 matheusbsilva137 commented Dec 18, 2024

Proposed changes (including videos or screenshots)

  • Consider the contactManager property stored in contacts when deciding the default new agent to assign to a livechat room (instead of only considering the contactManager field in visitors, which is not maintained anymore).

Issue(s)

Steps to test or reproduce

In a Premium workspace, enable the "Assign new conversations to the contact manager" setting under Settings > Omnichannel > Routing. Then create a new contact manually (in the livechat contact center) using a livechat agent and assign yourself user as the contact manager (use the contacts.get endpoint to make sure the contactManager field contains the user id of the agent who created the contact). Then start a new livechat conversation using the info from the contact that has just been created.
Expected result: the contact manager should be automatically assigned to the room if it is online.

Note: use "Auto Selection" as the "Omnichannel Routing Method" (under Settings > Omnichannel > Routing). Otherwise, only bot contact managers will be auto assigned to rooms.

Further comments

CORE-862

@matheusbsilva137 matheusbsilva137 added this to the 7.2.0 milestone Dec 18, 2024
Copy link

changeset-bot bot commented Dec 18, 2024

🦋 Changeset detected

Latest commit: d89c89e

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

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/model-typings Patch
@rocket.chat/apps Patch
@rocket.chat/models Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/omnichannel-services Patch
rocketchat-services Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/presence Patch
@rocket.chat/network-broker Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration 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

Copy link
Contributor

dionisio-bot bot commented Dec 18, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link
Contributor

github-actions bot commented Dec 18, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-34210/
on branch gh-pages at 2024-12-20 22:40 UTC

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (149c3b5) to head (d89c89e).
Report is 15 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #34210      +/-   ##
===========================================
- Coverage    75.81%   75.27%   -0.54%     
===========================================
  Files          512      516       +4     
  Lines        22221    22540     +319     
  Branches      5407     5485      +78     
===========================================
+ Hits         16846    16968     +122     
- Misses        4722     4911     +189     
- Partials       653      661       +8     
Flag Coverage Δ
unit 75.27% <ø> (-0.54%) ⬇️

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

Copy link
Contributor

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

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

Tests? :please:

@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review December 20, 2024 05:26
@matheusbsilva137 matheusbsilva137 requested review from a team as code owners December 20, 2024 05:26
apps/meteor/server/models/raw/Users.js Outdated Show resolved Hide resolved
(IS_EE ? it : it.skip)('should route to contact manager if it is online', async () => {
const room = await createLivechatRoom(visitor.token);

await sleep(5000);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing this in all other routing tests in this describe 🤔 shouldn't we keep doing this, then?

Copy link
Member

Choose a reason for hiding this comment

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

If it's not extremely necessary we shouldn't, but if we are already doing for the other cases 🤷‍♂️

Copy link
Member Author

@matheusbsilva137 matheusbsilva137 Dec 20, 2024

Choose a reason for hiding this comment

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

yeah, I'm just afraid of removing this and creating a flaky test 😢
but we could try removing all sleeps from these tests at some point. I believe we're running all callbacks sync 👀

apps/meteor/tests/end-to-end/api/livechat/24-routing.ts Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/livechat/24-routing.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Also, can we create a test case to cover the scenario found by the QA team where:
While visitor attempts to send a second message, it gets redirected to the agent, but on first attempt, it throws error.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was happening on CE and is already being caught by our tests. CI failed due to this same condition by the time our QA team reported this, so we should be fine :)

@scuciatto scuciatto added the stat: QA assured Means it has been tested and approved by a company insider label Dec 20, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 20, 2024
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.

4 participants