fix: Agents unable to assign outbound message replies to themselves #36989
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 |
|
Pre-merge checks and finishing touches✅ Passed checks (5 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 (2)
🧰 Additional context used🧬 Code graph analysis (2)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.ts (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.ts (2)
⏰ 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). (7)
🔇 Additional comments (16)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor/outbound-forms #36989 +/- ##
===========================================================
- Coverage 67.13% 67.13% -0.01%
===========================================================
Files 3407 3409 +2
Lines 117870 117929 +59
Branches 21473 21483 +10
===========================================================
+ Hits 79136 79174 +38
- Misses 36030 36054 +24
+ Partials 2704 2701 -3
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: 0
🧹 Nitpick comments (4)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (1)
48-48: Consider adding null check for edge cases.While the logic is correct, consider whether
!canAssignAgentshould be explicitly checked ascanAssignAgent === falseto handle potential undefined values more predictably.- disabled={disabled || isLoading || !canAssignAgent} + disabled={disabled || isLoading || canAssignAgent !== true}apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
14-14: Consider using a stable timestamp for testing.Using
new Date().toISOString()creates non-deterministic values that could make testing harder. Consider accepting an optional timestamp parameter or using a factory pattern for better testability.-export const getAgentDerivedFromUser = (user: IUser | null, departmentId: string): Serialized<ILivechatDepartmentAgents> => { +export const getAgentDerivedFromUser = (user: IUser | null, departmentId: string, timestamp?: string): Serialized<ILivechatDepartmentAgents> => { if (!isOmnichannelAgent(user)) { throw new Error('User is not a livechat agent'); } return { agentId: user._id, username: user.username || '', _id: user._id, - _updatedAt: new Date().toISOString(), + _updatedAt: timestamp || new Date().toISOString(), departmentId, departmentEnabled: true, count: 0, order: 0, }; };apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx (1)
34-36: Silent error handling might hide issues.The catch block silently returns an empty array, which could mask legitimate errors. Consider logging errors for debugging purposes while maintaining the graceful fallback.
} catch { + // Log error for debugging while gracefully falling back + console.warn('Failed to derive allowed agents:', error); return []; }apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
92-92: Potential undefined reference in agent lookup.The
agentsvariable could be undefined ifuseAllowedAgentsreturns undefined. While the hook implementation suggests it always returns an array, consider adding a defensive check.- const agent = agents?.find((agent) => agent.agentId === agentId); + const agent = agents?.find((agent) => agent.agentId === agentId); + // Or more defensively: + // const agent = (agents || []).find((agent) => agent.agentId === agentId);
📜 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)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx(6 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx(2 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts(1 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.tsx (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)apps/meteor/tests/mocks/data.ts (1)
createFakeUser(32-44)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx (1)
useAllowedAgents(14-38)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)packages/core-typings/src/IUser.ts (1)
IUser(186-252)apps/meteor/tests/mocks/data.ts (1)
createFakeUser(32-44)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
getAgentDerivedFromUser(5-20)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx (1)
useAllowedAgents(14-38)
🔇 Additional comments (9)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.spec.tsx (2)
21-21: LGTM! Role addition aligns with permission requirements.Adding the
'livechat-agent'role to the mock user ensures consistency with the new agent validation logic that requires users to have this role.
222-222: Confirmed — appRoot([]) intentionally removes permissions and matches the test intent.appRoot is defined to accept a permissions array (defaults to ['outbound.can-assign-self-only','outbound.can-assign-any-agent']); passing [] clears permissions, so using appRoot([]).build() for tests that assert agent-not-found / agent field disabled is correct.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.spec.ts (1)
1-39: Well-structured test coverage for agent derivation utility.The tests comprehensively cover all edge cases: null user, non-livechat-agent user, and valid livechat-agent user. The assertions correctly validate the expected error messages and object structure.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/components/AgentField.tsx (1)
13-20: Good API simplification with unified permission flag.Consolidating
canAssignAnyandcanAssignSelfOnlyinto a singlecanAssignAgentprop simplifies the component interface and makes the permission logic clearer.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/utils/getAgentDerivedFromUser.ts (1)
3-3: Type guard correctly validates livechat-agent role.The
isOmnichannelAgentfunction properly checks for null users and validates the required role.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.tsx (2)
16-36: Well-structured permission logic with clear fallback patterns.The hook correctly implements the fallback rules as described in the PR objectives. The logic flow is clear and handles all permission scenarios appropriately.
28-30: Verify emptyqueryAgentsedge case.The condition
canAssignAnyAgent && queryAgents?.lengthis false whenqueryAgentsis an empty array, causing fall-through to the derived-agent path. Confirm whether agents withcanAssignAnyAgentbut no department agents should be allowed to assign to themselves.if (canAssignAnyAgent && queryAgents?.length) { return queryAgents; }apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/hooks/useAllowedAgents.spec.tsx (1)
1-169: Comprehensive test coverage with proper mocking.The test suite thoroughly covers all permission scenarios and edge cases, including error handling. The mock setup and assertions are well-structured.
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RepliesForm/RepliesForm.tsx (1)
77-83: Clean integration of user-aware agent selection.The integration of
useAllowedAgentshook properly centralizes the agent filtering logic based on user permissions and department selection.
Proposed changes (including videos or screenshots)
This PR adds fallbacks so agents are be able to assign an outbound message reply to themselves according to the outbound permissions.
outbound.can-assign-self-onlynoroutbound.can-assign-any-agent, no agents should be available.outbound.can-assign-any-agentbut doesn't haveview-livechat-department, only themselves must be available.outbound.can-assign-self-onlyonly themselves must be available.Issue(s)
CTZ-330
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests