feat: update Livechat_enabled_when_agent_idle setting to not include offline agents in the query#39495
feat: update Livechat_enabled_when_agent_idle setting to not include offline agents in the query#39495nazabucciarelli wants to merge 52 commits intodevelopfrom
Livechat_enabled_when_agent_idle setting to not include offline agents in the query#39495Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: f332eaf 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds DDP/WebSocket test utilities and integrates WebSocket-based agent away signaling into multiple livechat tests and helpers; tests now track/close WebSocket handles. Adjusts omnichannel agent-status filter logic and adds CI env var and a changeset entry. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #39495 +/- ##
===========================================
- Coverage 70.19% 70.16% -0.04%
===========================================
Files 3281 3281
Lines 116915 116915
Branches 20671 20815 +144
===========================================
- Hits 82073 82037 -36
- Misses 31555 31594 +39
+ Partials 3287 3284 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3480932 to
15311df
Compare
15311df to
d3fbcf9
Compare
f9512c8 to
04dc75a
Compare
8a4cb7b to
6ebec42
Compare
f579500 to
77bd762
Compare
KevLehman
left a comment
There was a problem hiding this comment.
Hope gazzo is happy with the ws.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/purple-boxes-shout.md">
<violation number="1" location=".changeset/purple-boxes-shout.md:6">
P2: The changeset note is inaccurate: it says offline agents are excluded only when the setting is enabled, but this PR’s behavior is to always exclude offline agents.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…cketChat/Rocket.Chat into fix/livechat-offline-agent-assignment
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
@cubic-dev-ai review |
@nazabucciarelli I have started the AI code review. It will take a few minutes to complete. |
| await page.goto('/'); | ||
| await poOmnichannel.waitForHome(); | ||
|
|
||
| if (conversations.length === 0) { |
There was a problem hiding this comment.
I'll leave this one for a FE to take a look 👀
curious if there's a better approach to fix it.
| }); | ||
|
|
||
| test.afterEach(async ({ api }) => { | ||
| if (livechatPage) await livechatPage.context().close(); |
There was a problem hiding this comment.
Addressed in e7898f2
should we add a new linter rule?
| await setSettingValueById(api, 'Livechat_waiting_queue', false); | ||
| await setSettingValueById(api, 'Accounts_Default_User_Preferences_idleTimeLimit', 300); |
There was a problem hiding this comment.
Do u need this to happen after each test step? why?
There was a problem hiding this comment.
In the case of closing contexts, I need that after each test in order to not get multiple windows opened. In case of Accounts_Default_User_Preferences_idleTimeLimit set to 300, it's needed since we have 2 agents on this test, and if the value of the setting remains as 1 in the beginning of each test, it would make onlineAgent to be away too. Regarding the Livechat_waiting_queue setting you were right, it wasn't strictly needed in the beforeEach, but I added it on a new step at the end of the test cases that use it. Addresed in e7898f2
Proposed changes (including videos or screenshots)
The changes made in this PR were done following product team given directives.
statusis offline, regardless of theLivechat_enabled_when_agent_idlesetting. The setting now only affects agents whoseconnectionStatusis away.Issue(s)
CORE-1853 [Investigation] The Omnichannel routing system (Load Balancer) assigns new chats to offline agents if their Livechat toggle was left as "Available".
Steps to test or reproduce
Before testing these cases, you'd need to have a EE license and to create at least two agents and a department with them. Also, you'll need to go to
Settings -> Omnichannel -> Queue Management, enable theWaiting queueoption and then setMax. number of simultaneous chatsto only 1. Please check the task's comment, I've left some recordings there testing this.Further comments
IMPORTANT NOTE
This PR will introduce a new regression: CORE-1972 - Fix 'Livechat_accept_chats_with_no_agents' omnichannel setting not being respected. This happens because we weren't filtering out offline agents at all, making like the
Livechat_accept_chats_with_no_agents(setting intended to assign offline agents to visitors) was always true. So after merging this PR to develop, a new PR will be raised for this 'introduced' regression.Summary by CodeRabbit