feat: taskList and IncomingTask widgets added #348
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive enhancement to the Webex Contact Center Widgets, focusing on task management capabilities. A new Changes
Sequence DiagramsequenceDiagram
participant App as React App
participant Store as Contact Center Store
participant TaskList as TaskList Component
participant IncomingTask as IncomingTask Component
App->>Store: Initialize Store
Store-->>App: Provide Contact Center Context
App->>TaskList: Render TaskList
App->>IncomingTask: Render IncomingTask
IncomingTask->>Store: Check Current Task
TaskList->>Store: Retrieve Task List
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/contact-center/task/src/IncomingTask/styles-module.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.types.ts
Outdated
Show resolved
Hide resolved
|
Install step is failling in the pipeline. Please run a yarn install and push the yarn.lock file again |
packages/contact-center/task/tests/IncomingTask/incoming-task.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/task/tests/IncomingTask/incoming-task.presentational.tsx
Outdated
Show resolved
Hide resolved
| onAccepted: store.onAccepted, | ||
| onDeclined: store.onDeclined, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can we have another test here that checks that when the task is declined the component is hidden again.
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event |
There was a problem hiding this comment.
change the comment to say ` task:incoming' rather than TASK_INCOMING
There was a problem hiding this comment.
addressed using enum instead of actual values.
|
|
||
| waitFor(() => { | ||
| expect(result.current.currentTask).toBe(taskMock); | ||
| expect(ccMock.on).toHaveBeenCalledWith('TASK_INCOMING', expect.any(Function)); |
There was a problem hiding this comment.
Please change this in the whole file
There was a problem hiding this comment.
using enum in whole file
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(taskMock.on).toHaveBeenCalledWith('TASK_ASSIGNED', expect.any(Function)); |
There was a problem hiding this comment.
These event names are incorrect, right? Or am I missing something here?
There was a problem hiding this comment.
using enum in whole file
| expect(onDeclined).toHaveBeenCalled(); | ||
| expect(taskMock.decline).toHaveBeenCalledWith('interaction1'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We can have another test, when there is no onAccept and onDecline, we don't call it. This was an issue in station-login which caused errors.
There was a problem hiding this comment.
added testcase 'should not call onAccepted or onDeclined if they are not provided''
| expect(ccMock.on).toHaveBeenCalledTimes(1); // Event should only be registered once | ||
| expect(ccMock.off).toHaveBeenCalledTimes(1); // Event cleanup should still happen once | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests look good, we are just not checking any error scenarios here, please see if we can add a few tests for it. If possible
There was a problem hiding this comment.
added console error scenarios.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
docs/react-samples/src/App.tsx (1)
Line range hint
29-34: Security: Consider secure token handlingStoring and displaying access tokens in plain text input fields poses security risks. Consider:
- Using secure input fields (password type)
- Implementing secure token storage
- Adding token validation before usage
🧹 Nitpick comments (13)
packages/contact-center/station-login/src/helper.ts (1)
26-29: Enhance error handling and loggingThe current error handling could be improved:
- Consider using structured logging instead of console.error
- Add error types for better error handling
- Consider adding error recovery mechanisms
.catch((error: Error) => { - console.error(error); + const structuredError = { + context: 'StationLogin', + operation: 'login', + error: error.message, + timestamp: new Date().toISOString() + }; + console.error(JSON.stringify(structuredError)); setLoginFailure(error); });Also applies to: 40-43
packages/contact-center/store/src/store.ts (2)
19-30: Improve error handling in registerCCConsider enhancing the error handling with:
- Specific error types for different failure scenarios
- Proper error propagation
- Cleanup of store state on failure
+interface CCRegistrationError extends Error { + code: string; + details?: unknown; +} registerCC(webex: WithWebex['webex']): Promise<void> { this.cc = webex.cc; return this.cc .register() .then((response: Profile) => { this.teams = response.teams; this.loginOptions = response.loginVoiceOptions; this.idleCodes = response.idleCodes; this.agentId = response.agentId; }) - .catch((error) => { + .catch((error: CCRegistrationError) => { + // Reset state on registration failure + this.teams = []; + this.loginOptions = []; + this.idleCodes = []; + this.agentId = ''; console.error('Error registering contact center', error); return Promise.reject(error); }); }
33-60: Enhance initialization timeout handlingConsider making the timeout configurable and improving error handling:
- Make timeout duration configurable
- Add cleanup on timeout
- Improve error types
+const DEFAULT_INIT_TIMEOUT = 6000; -init(options: InitParams): Promise<void> { +init(options: InitParams & { timeout?: number }): Promise<void> { if ('webex' in options) { return this.registerCC(options.webex); } return new Promise((resolve, reject) => { - const timer = setTimeout(() => { + const timer = setTimeout(() => { + webex?.disconnect?.(); // Cleanup SDK reject(new Error('Webex SDK failed to initialize')); - }, 6000); + }, options.timeout || DEFAULT_INIT_TIMEOUT); const webex = Webex.init({ config: options.webexConfig, credentials: { access_token: options.access_token, }, }); webex.once('ready', () => { clearTimeout(timer); this.registerCC(webex) .then(() => { resolve(); }) .catch((error) => { + webex.disconnect(); // Cleanup SDK on error reject(error); }); }); }); }packages/contact-center/task/src/helper.ts (1)
112-114: Use optional chaining for cleaner checks.
Instead of using “onAccepted && onAccepted()” or “onDeclined && onDeclined()”, employ optional chaining methods such as “onAccepted?.()” for improved readability.Proposed changes:
- onAccepted && onAccepted(); + onAccepted?.(); - onDeclined && onDeclined(); + onDeclined?.();Also applies to: 125-126
🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/contact-center/task/src/TaskList/index.tsx (1)
14-17: Simplify props object creationSince you're only passing a single prop, you can simplify the props object creation.
- const props = { - taskList, - }; - - return <TaskListPresentational {...props} />; + return <TaskListPresentational taskList={taskList} />;packages/contact-center/task/tests/IncomingTask/incoming-task.presentational.tsx (1)
9-24: Update test description and assertionsThe "no task yet" test should be more descriptive and include accessibility testing.
-it('renders "no task yet" when there is no current task', () => { +it('displays empty state message when no current task exists', () => { const props = { currentTask: null, accept: jest.fn(), decline: jest.fn(), isBrowser: true, answered: false, ended: false, missed: false, }; render(<IncomingTaskPresentational {...props} />); - const message = screen.getByText('no task yet'); + const message = screen.getByRole('status'); expect(message).toBeInTheDocument(); + expect(message).toHaveTextContent('no task yet'); + expect(message).toHaveAttribute('aria-live', 'polite'); });packages/contact-center/task/src/IncomingTask/styles-module.scss (1)
1-98: Extract design tokens into SCSS variablesConsider extracting hard-coded values (colors, spacing, font-sizes) into SCSS variables for better maintainability and consistency.
// Design tokens $color-primary: #28a745; $color-danger: #dc3545; $color-text-primary: #333; $spacing-base: 8px; $border-radius-base: 6px; // Then use these variables in your styles .incoming-call-container { border-radius: $border-radius-base; padding: $spacing-base * 2; // ... }packages/contact-center/task/tests/TaskList/index.tsx (1)
14-20: Enhance store mock with more comprehensive stateThe current store mock is minimal. Consider adding more relevant store properties that might affect the component's behavior.
jest.mock('@webex/cc-store', () => ({ cc: {}, selectedLoginOption: 'BROWSER', onAccepted: jest.fn(), onDeclined: jest.fn(), + isLoggedIn: true, + error: null, + loading: false, + taskConfig: { + enabled: true, + maxTasks: 5 + } }));packages/contact-center/task/src/task.types.ts (2)
10-10: Consider adding type constraints for task propertiesThe
currentTaskproperty of typeITaskmight benefit from more specific type constraints to ensure type safety of the task properties being accessed in the presentational components.Consider creating a more specific task interface:
interface TaskData { interaction: { callAssociatedDetails: { ani: string; dn: string; virtualTeamName: string; }; }; } interface ITaskWithData extends ITask { data: TaskData; }
75-89: Consider more descriptive event namesSome event names in
TASK_EVENTScould be more descriptive to better indicate their purpose.Consider renaming some events for clarity:
export enum TASK_EVENTS { TASK_INCOMING = 'task:incoming', TASK_ASSIGNED = 'task:assigned', TASK_MEDIA = 'task:media', TASK_UNASSIGNED = 'task:unassigned', TASK_HOLD = 'task:hold', TASK_UNHOLD = 'task:unhold', - TASK_CONSULT = 'task:consult', - TASK_CONSULT_END = 'task:consultEnd', - TASK_CONSULT_ACCEPT = 'task:consultAccepted', + TASK_CONSULT_START = 'task:consultStart', + TASK_CONSULT_COMPLETE = 'task:consultComplete', + TASK_CONSULT_ACCEPTED = 'task:consultAccepted', TASK_PAUSE = 'task:pause', TASK_RESUME = 'task:resume', TASK_END = 'task:end', TASK_WRAPUP = 'task:wrapup', }packages/contact-center/task/tests/TaskList/task-list.presentational.tsx (1)
21-66: Enhance test coverage with additional scenariosWhile the basic functionality is tested, consider adding tests for the following scenarios:
- Edge cases with malformed task data
- Accessibility testing for screen readers
- Different virtual team names and phone number formats
- Extremely long text values that might affect layout
Example test case for malformed data:
it('handles malformed task data gracefully', () => { const props: TaskListPresentationalProps = { taskList: [{ id: '1', data: { interaction: { callAssociatedDetails: { ani: null, // Missing ANI dn: '', // Empty DN virtualTeamName: undefined // Missing team name } } } }] }; render(<TaskListPresentational {...props} />); // Assert fallback/default values are displayed });packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx (1)
4-9: Consider adding prop validation and default propsFor better type safety and debugging:
- Add PropTypes validation for runtime checking
- Consider providing default props for optional parameters
- Add JSDoc comments to document the component's purpose and props
+import PropTypes from 'prop-types'; + +/** + * Presentational component for displaying incoming task information + * @param {Object} props - Component props + * @param {Object} props.currentTask - Current task information + * @param {Function} props.accept - Handler for accepting the task + * @param {Function} props.decline - Handler for declining the task + * @param {boolean} props.isBrowser - Flag indicating browser environment + * @param {boolean} props.answered - Flag indicating if task was answered + * @param {boolean} props.ended - Flag indicating if task ended + * @param {boolean} props.missed - Flag indicating if task was missed + */ const IncomingTaskPresentational: React.FunctionComponent<IncomingTaskPresentationalProps> = (props) => {packages/contact-center/task/tests/helper.ts (1)
4-18: Enhance mock data to be more realisticThe current mock data is minimal. Consider:
- Adding more realistic task data structure
- Including all required properties that the component uses
- Adding type definitions for mocks
const taskMock = { data: { interactionId: 'interaction1', + interaction: { + callAssociatedDetails: { + ani: '+1234567890', + dn: '9876543', + virtualTeamName: 'Support Team' + } + }, + status: 'pending', + timestamp: new Date().toISOString() }, accept: jest.fn().mockResolvedValue('Accepted'), decline: jest.fn().mockResolvedValue('Declined'), on: jest.fn(), off: jest.fn(), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
docs/react-samples/src/App.tsx(2 hunks)packages/contact-center/cc-widgets/package.json(1 hunks)packages/contact-center/cc-widgets/src/index.ts(1 hunks)packages/contact-center/cc-widgets/src/wc.ts(2 hunks)packages/contact-center/station-login/src/helper.ts(3 hunks)packages/contact-center/station-login/src/station-login/station-login.types.ts(3 hunks)packages/contact-center/store/package.json(1 hunks)packages/contact-center/store/src/store.ts(1 hunks)packages/contact-center/task/.babelrc.js(1 hunks)packages/contact-center/task/package.json(1 hunks)packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx(1 hunks)packages/contact-center/task/src/IncomingTask/index.tsx(1 hunks)packages/contact-center/task/src/IncomingTask/styles-module.scss(1 hunks)packages/contact-center/task/src/TaskList/index.tsx(1 hunks)packages/contact-center/task/src/TaskList/task-list.presentational.tsx(1 hunks)packages/contact-center/task/src/helper.ts(1 hunks)packages/contact-center/task/src/index.ts(1 hunks)packages/contact-center/task/src/task.types.ts(1 hunks)packages/contact-center/task/tests/IncomingTask/incoming-task.presentational.tsx(1 hunks)packages/contact-center/task/tests/IncomingTask/index.tsx(1 hunks)packages/contact-center/task/tests/TaskList/index.tsx(1 hunks)packages/contact-center/task/tests/TaskList/task-list.presentational.tsx(1 hunks)packages/contact-center/task/tests/helper.ts(1 hunks)packages/contact-center/task/tsconfig.json(1 hunks)packages/contact-center/task/webpack.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/contact-center/task/.babelrc.js
- packages/contact-center/task/tsconfig.json
- packages/contact-center/task/src/index.ts
- packages/contact-center/station-login/src/station-login/station-login.types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/contact-center/task/src/helper.ts
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
packages/contact-center/station-login/src/helper.ts (1)
21-24: Follow established code convention for prop passing
As mentioned in previous reviews, we should follow the code convention of passing props from index.tsx rather than directly accessing store here.
Consider refactoring to receive selectedLoginOption as a prop from index.tsx.
packages/contact-center/store/package.json (2)
25-25: Question: Intentional wxcc version decrease?
The wxcc version number has decreased from .5 to .2. While this might be intentional, it's worth confirming if this was meant to be a higher number like wxcc.6 or if this specific version was chosen for a particular reason.
Let's check the version pattern in the repository:
✅ Verification successful
Let me gather more historical information about this version change.
Let me try a different approach to check for version changes.
Based on the yarn.lock analysis, I can now provide a final response:
Version 3.7.0-wxcc.2 is the correct version to use
Looking at the yarn.lock file, this version (3.7.0-wxcc.2) is the main webex package version that coordinates with all the other @webex/* dependencies. Most @webex/* packages are on 3.5.0-wxcc.1, with @webex/plugin-cc specifically on 3.5.0-wxcc.8, and @webex/calling on 3.6.0-wxcc.1. This version alignment appears intentional and correct for maintaining compatibility across the webex ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the pattern of wxcc versions used in the repository
# Search for webex version patterns in package.json files
echo "Checking webex version patterns..."
fd package.json | xargs rg "webex.*wxcc" -A 1 -B 1
Length of output: 320
Script:
#!/bin/bash
# Check git history for changes to this specific line
git log -L :webex:packages/contact-center/store/package.json
# Also check if there are any recent PRs or issues discussing this version change
fd package.json | xargs git blame | rg "webex.*wxcc"
Length of output: 108
Script:
#!/bin/bash
# Search for any commit messages or files that might mention the webex version change
rg -i "webex.*wxcc" -g '!package-lock.json'
# Also check if there are any changelog or documentation files that might explain this
fd -e md | xargs rg -i "webex.*wxcc"
Length of output: 20155
25-25: Verify webex dependency version compatibility
The update from 3.6.0-wxcc.5 to 3.7.0-wxcc.2 introduces new features that may be required for the TaskList and IncomingTask widgets.
Let's verify the version compatibility and changelog:
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information from the npm registry:
Version update to [email protected] appears stable and compatible
The version 3.7.0-wxcc.2 is officially published in the npm registry and is currently set as the wxcc dist-tag, indicating it's the recommended version for WebEx Contact Center. The decrease in the wxcc suffix (from .5 to .2) is not a concern as it's tied to a higher minor version (3.7.0 vs 3.6.0).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version compatibility and changelog for webex package
# Check if this version exists in npm registry
echo "Checking if version exists in npm registry..."
curl -s https://registry.npmjs.org/webex | jq '."dist-tags", ."versions"["3.7.0-wxcc.2"]'
# Search for any breaking changes or known issues in git history
echo "Checking for breaking changes or known issues..."
git log --oneline | rg -i "webex.*3\.7\.0-wxcc" -A 5
Length of output: 5232
packages/contact-center/task/src/helper.ts (3)
9-9: Consider using a boolean state or variable instead of a ref.
This ref only stores a boolean and is not assigned to a DOM element. If the intention is to keep track of event registration between renders, a useState or a static variable might be more idiomatic.
14-14: .some vs. .forEach
This code uses Array.prototype.some to short-circuit once a match is found, which is more efficient than using forEach to iterate over all items.
55-55: Avoid using a ref for a simple boolean check.
Similar to the earlier isEventRegistered usage, this can be replaced with a state or simple variable if you only need a boolean flag.
packages/contact-center/cc-widgets/src/index.ts (1)
3-6: Export additions look good.
The newly imported and exported components (IncomingTask, TaskList) integrate nicely.
packages/contact-center/task/webpack.config.js (1)
6-11: Configuration looks correct.
Merging the base config with the specified output settings is conventional. Confirm your library target is compatible with downstream environments.
packages/contact-center/task/src/IncomingTask/index.tsx (1)
8-18: Well-structured component.
The component properly observes the store and spreads the hook results into the presentational component. This approach promotes clean separation of logic and presentation.
packages/contact-center/task/src/TaskList/index.tsx (2)
7-7: Remove unused import useIncomingTask
The useIncomingTask hook is imported but never used in this component.
-import {useIncomingTask, useTaskList} from '../helper';
+import {useTaskList} from '../helper';10-10: Verify usage of destructured store values
The selectedLoginOption, onAccepted, and onDeclined values are destructured from the store but don't appear to be used in the component.
-const {cc, selectedLoginOption, onAccepted, onDeclined} = store;
+const {cc} = store;packages/contact-center/task/tests/IncomingTask/index.tsx (1)
3-3: Remove redundant comment
The comment is unnecessary as it duplicates the import statement.
-import * as helper from '../../src/helper'; // Import the helper where useIncomingTask is defined
+import * as helper from '../../src/helper';packages/contact-center/task/package.json (2)
4-4: Change version to 1.0.0
As mentioned in previous review, for a new component package, start with version 1.0.0.
53-53: Remove stableVersion field
As previously noted, this field will be automatically generated.
packages/contact-center/cc-widgets/package.json (1)
27-27: LGTM: Dependency addition aligns with new task widgets feature
The addition of @webex/cc-task as a workspace dependency is appropriate for integrating the new task widgets.
packages/contact-center/task/src/IncomingTask/styles-module.scss (2)
1-98: Verify SCSS functionality
Based on previous review comments indicating SCSS isn't working, please verify the build setup supports SCSS modules and that the styles are being properly applied.
38-48: Verify color contrast ratios for accessibility
Please ensure the following color combinations meet WCAG 2.1 AA standards for accessibility:
- Status background (#d4f8e8) with text color (#28a745)
- Answer button background (#28a745) with white text
- Decline button background (#dc3545) with white text
Also applies to: 67-74, 76-83
packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx (2)
13-13: Timer implementation should be handled by Call control
The TODO comment about calculating time elapsed should be removed as this will be implemented in the Call control component.
18-116: Move inline styles to a separate styles object
For better maintainability and reusability, extract all inline styles into a separate styles object at the top of the file.
| describe('IncomingTask Component', () => { | ||
| it('renders IncomingTaskPresentational with correct props', () => { | ||
| const useIncomingTaskSpy = jest.spyOn(helper, 'useIncomingTask'); | ||
|
|
||
| // Mock the return value of the useIncomingTask hook | ||
| useIncomingTaskSpy.mockReturnValue({ | ||
| currentTask: null, | ||
| setCurrentTask: jest.fn(), | ||
| answered: false, | ||
| ended: false, | ||
| missed: false, | ||
| accept: jest.fn(), | ||
| decline: jest.fn(), | ||
| isBrowser: true, | ||
| }); | ||
|
|
||
| render(<IncomingTask />); | ||
|
|
||
| // Assert that the useIncomingTask hook is called with the correct arguments | ||
| expect(useIncomingTaskSpy).toHaveBeenCalledWith({ | ||
| cc: store.cc, | ||
| selectedLoginOption: store.selectedLoginOption, | ||
| onAccepted: store.onAccepted, | ||
| onDeclined: store.onDeclined, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage
The current test only verifies hook invocation. Consider adding tests for:
- Task decline scenario
- Task acceptance scenario
- Component rendering with different task states
- Browser/non-browser rendering differences
Here's a suggested additional test case:
it('hides component when task is declined', () => {
const mockDecline = jest.fn();
const useIncomingTaskSpy = jest.spyOn(helper, 'useIncomingTask');
useIncomingTaskSpy.mockReturnValue({
currentTask: { id: '123', type: 'call' },
setCurrentTask: jest.fn(),
answered: false,
ended: false,
missed: false,
accept: jest.fn(),
decline: mockDecline,
isBrowser: true,
});
const { rerender } = render(<IncomingTask />);
// Verify task is shown
expect(screen.getByTestId('incoming-task')).toBeVisible();
// Simulate decline
mockDecline();
// Update hook return value
useIncomingTaskSpy.mockReturnValue({
...useIncomingTaskSpy.mock.results[0].value,
currentTask: null,
ended: true
});
rerender(<IncomingTask />);
// Verify task is hidden
expect(screen.queryByTestId('incoming-task')).not.toBeInTheDocument();
});There was a problem hiding this comment.
we will take care of these in UI tests after UI finalized
There was a problem hiding this comment.
@rsarika, understood. We'll hold off on those additional test scenarios until the UI is finalized. If you’d like to track these future enhancements, let me know, and we can open a follow-up GitHub issue to ensure we don’t lose sight of them.
packages/contact-center/task/src/TaskList/task-list.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(taskMock.on).toHaveBeenCalledWith('TASK_ASSIGNED', expect.any(Function)); | ||
| expect(taskMock.on).toHaveBeenCalledWith('TASK_END', expect.any(Function)); | ||
| expect(taskMock.on).toHaveBeenCalledWith('TASK_UNASSIGNED', expect.any(Function)); | ||
| expect(taskMock.on).toHaveBeenCalledWith('TASK_MEDIA', expect.any(Function)); | ||
| }); | ||
| }); | ||
|
|
||
| it('should clean up task events on task change or unmount', async () => { | ||
| const {result, unmount} = renderHook(() => | ||
| useIncomingTask({cc: ccMock, onAccepted, onDeclined, selectedLoginOption: 'BROWSER'}) | ||
| ); | ||
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| // Simulate unmount | ||
| unmount(); | ||
|
|
||
| waitFor(() => { | ||
| expect(taskMock.off).toHaveBeenCalledWith('TASK_ASSIGNED', expect.any(Function)); | ||
| expect(taskMock.off).toHaveBeenCalledWith('TASK_END', expect.any(Function)); | ||
| expect(taskMock.off).toHaveBeenCalledWith('TASK_UNASSIGNED', expect.any(Function)); | ||
| expect(taskMock.off).toHaveBeenCalledWith('TASK_MEDIA', expect.any(Function)); | ||
| expect(ccMock.off).toHaveBeenCalledWith('TASK_INCOMING', expect.any(Function)); | ||
| }); | ||
| }); | ||
|
|
||
| it('should call onAccepted callback when task is accepted', async () => { | ||
| const {result} = renderHook(() => | ||
| useIncomingTask({cc: ccMock, onAccepted, onDeclined, selectedLoginOption: 'BROWSER'}) | ||
| ); | ||
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| // Accept the task | ||
| act(() => { | ||
| result.current.accept(); | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(onAccepted).toHaveBeenCalled(); | ||
| expect(taskMock.accept).toHaveBeenCalledWith('interaction1'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should call onDeclined callback when task is declined', async () => { | ||
| const {result} = renderHook(() => | ||
| useIncomingTask({cc: ccMock, onAccepted, onDeclined, selectedLoginOption: 'BROWSER'}) | ||
| ); | ||
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| // Decline the task | ||
| act(() => { | ||
| result.current.decline(); | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(onDeclined).toHaveBeenCalled(); | ||
| expect(taskMock.decline).toHaveBeenCalledWith('interaction1'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should correctly handle task media', async () => { | ||
| const taskMediaMock = {}; | ||
| const {result} = renderHook(() => | ||
| useIncomingTask({cc: ccMock, onAccepted, onDeclined, selectedLoginOption: 'BROWSER'}) | ||
| ); | ||
|
|
||
| // Simulate an incoming task | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| // Simulate task media event | ||
| act(() => { | ||
| taskMock.on.mock.calls.find((call) => call[0] === 'task:media')[1](taskMediaMock); | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(result.current.missed).toBe(false); | ||
| expect(result.current.ended).toBe(false); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for error scenarios and edge cases
Current test coverage misses important scenarios:
- Error handling when accept/decline fails
- Edge cases when callbacks are undefined
- Network timeout scenarios
- Invalid task data handling
Example test case to add:
it('should handle accept/decline failures gracefully', async () => {
taskMock.accept.mockRejectedValueOnce(new Error('Network error'));
const {result} = renderHook(() =>
useIncomingTask({cc: ccMock, onAccepted, onDeclined, selectedLoginOption: 'BROWSER'})
);
act(() => {
ccMock.on.mock.calls[0][1](taskMock);
});
await act(async () => {
await result.current.accept();
});
expect(onAccepted).not.toHaveBeenCalled();
// Verify error handling behavior
});There was a problem hiding this comment.
addressed, added error handling tetsts
| describe('useTaskList Hook', () => { | ||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should register task:incoming event and add task to the list', async () => { | ||
| const {result} = renderHook(() => useTaskList({cc: ccMock})); | ||
|
|
||
| // Simulate the incoming task event | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); // Trigger TASK_INCOMING event | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(result.current.taskList).toContain(taskMock); | ||
| expect(ccMock.on).toHaveBeenCalledWith('task:incoming', expect.any(Function)); | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle multiple incoming tasks', async () => { | ||
| const {result} = renderHook(() => useTaskList({cc: ccMock})); | ||
|
|
||
| // Simulate multiple incoming task events | ||
| const task1 = {...taskMock, id: 'task1'}; | ||
| const task2 = {...taskMock, id: 'task2'}; | ||
|
|
||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](task1); // Trigger TASK_INCOMING for task1 | ||
| ccMock.on.mock.calls[0][1](task2); // Trigger TASK_INCOMING for task2 | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(result.current.taskList).toEqual([task1, task2]); | ||
| expect(ccMock.on).toHaveBeenCalledWith('task:incoming', expect.any(Function)); | ||
| }); | ||
| }); | ||
|
|
||
| it('should clean up task:incoming event on unmount', async () => { | ||
| const {unmount} = renderHook(() => useTaskList({cc: ccMock})); | ||
|
|
||
| // Simulate unmount | ||
| unmount(); | ||
|
|
||
| waitFor(() => { | ||
| expect(ccMock.off).toHaveBeenCalledWith('task:incoming', expect.any(Function)); | ||
| }); | ||
| }); | ||
|
|
||
| it('should not register event multiple times', async () => { | ||
| const {result} = renderHook(() => useTaskList({cc: ccMock})); | ||
|
|
||
| // Trigger TASK_INCOMING event once | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); | ||
| }); | ||
|
|
||
| // Simulate triggering the event again (should not register again) | ||
| act(() => { | ||
| ccMock.on.mock.calls[0][1](taskMock); | ||
| }); | ||
|
|
||
| waitFor(() => { | ||
| expect(ccMock.on).toHaveBeenCalledTimes(1); // Event should only be registered once | ||
| expect(ccMock.off).toHaveBeenCalledTimes(1); // Event cleanup should still happen once | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for task list management scenarios
The task list tests should additionally cover:
- Removing tasks from the list
- Updating existing tasks
- Maximum list size handling
- Task deduplication
- List sorting/ordering
Example test case to add:
it('should remove completed tasks from the list', async () => {
const {result} = renderHook(() => useTaskList({cc: ccMock}));
const task = {...taskMock, id: 'task1'};
act(() => {
ccMock.on.mock.calls[0][1](task);
});
// Simulate task completion
act(() => {
task.on.mock.calls.find(call => call[0] === 'TASK_END')[1]();
});
expect(result.current.taskList).not.toContain(task);
});There was a problem hiding this comment.
addressed, added new tests
There was a problem hiding this comment.
@rsarika, I searched the codebase for test cases covering the removal of tasks from the list or completed tasks, but no matches were found. If you’ve added those tests in a different location or file name, please let me know. Otherwise, it might be worth verifying that the new tests were pushed to the PR branch. Let me know if you need any help writing or locating the test coverage for task removal or completion scenarios.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Let's search across all test files in the repository for the newly added test cases
# related to removing tasks from the list, or specifically "should remove completed tasks"
# or for the phrase "completed tasks".
rg "should remove completed tasks" -A 10
Length of output: 42
Script:
#!/bin/bash
# Let's broaden our search to see if any new tests were added that mention removing tasks
# from the list, completed tasks, or removing tasks for any reason.
# We'll search across all .test.ts, .test.tsx, or file patterns commonly used in tests.
# We'll look for "remove.*task" or "completed"
# with some context lines around matches.
rg -g "*test*.ts*" "remove.*task|completed" -A 5
Length of output: 199
Script:
#!/bin/bash
# Let's perform a broader search across all .ts and .tsx files in the repository,
# including those in test directories, for any mention of "remove" or "completed" tasks.
# We'll include 5 lines of additional context around matches.
rg --glob "*.ts" --glob "*.tsx" "(remove.*task|completed)" -A 5
Length of output: 65
packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx
Show resolved
Hide resolved
packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/task/src/TaskList/task-list.presentational.tsx
Outdated
Show resolved
Hide resolved
| const [isAnswered, setIsAnswered] = useState(false); | ||
| const [isEnded, setIsEnded] = useState(false); | ||
| const [isMissed, setIsMissed] = useState(false); | ||
| const audioRef = useRef<HTMLAudioElement | null>(null); // Ref for the audio element |
There was a problem hiding this comment.
We don't seem to be testing the presence of this useRef and the audio ref being set
|
|
||
| const accept = () => { | ||
| const taskId = currentTask?.data.interactionId; | ||
| if (!taskId) return; |
There was a problem hiding this comment.
I don't see a test written for this condition. Is it covered in another test?
|
|
||
| const decline = () => { | ||
| const taskId = currentTask?.data.interactionId; | ||
| if (!taskId) return; |
| {isLoggedIn && <UserState />} | ||
| {isLoggedIn && <IncomingTask />} | ||
| {isLoggedIn && <TaskList />} |
There was a problem hiding this comment.
We could have this one-line
| {isLoggedIn && <UserState />} | |
| {isLoggedIn && <IncomingTask />} | |
| {isLoggedIn && <TaskList />} | |
| { | |
| isLoggedIn && ( | |
| <UserState /> | |
| <IncomingTask /> | |
| <TaskList /> | |
| ) | |
| } |
There was a problem hiding this comment.
Can we also have callbacks assigned for onAccepted and onDeclined to ensure they're called?
| }, | ||
| }); | ||
|
|
||
| const WebTaskList = r2wc(TaskList); |
There was a problem hiding this comment.
Doesn't the task list have any props? What if the user refreshes, and when we re-login, we want to show the existing task list?
| jest.mock('@webex/cc-store', () => { | ||
| return { | ||
| cc: {}, | ||
| teams, | ||
| loginOptions, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Given that we are mocking the store here, shouldn't we add a test to check if the setSelectedLoginOption method was called?
| import IncomingTaskPresentational from './incoming-task.presentational'; | ||
|
|
||
| const IncomingTask: React.FunctionComponent = observer(() => { | ||
| const {cc, selectedLoginOption, onAccepted, onDeclined} = store; |
There was a problem hiding this comment.
This shouldn't be happening. Ideally the onAccepted and onDeclined or any other actionables should be received as Widget prop and not a store prop. Plus, I don't see a declaration for these int the store. Did we test this flow?
| onAccepted: jest.fn(), | ||
| onDeclined: jest.fn(), |
There was a problem hiding this comment.
This should go away once we address the comment on the incoming task widget
PR description: Added Task List and Incoming task widgets in task package.
Added these widgets to cc-widgets.
consumed them in sample page to display incomingTask and Task list whenever new task incoming.
Tests: Dailed EP number and received incoming task in widgets and able to see incoming task and task list in sample page with details.
If browser login able to see answer and decline buttons.
If The call disconnected from customer end the IncomingTask and TaskList should dismiss.
If the agent clicks decline then IncomingTask and TaskList should dismiss.
Summary by CodeRabbit
New Features
IncomingTaskandTaskListcomponents for enhanced task management.IncomingTaskPresentationalandTaskListPresentationalcomponents for displaying task details.useIncomingTaskanduseTaskListfor managing task states.Bug Fixes
Documentation
Tests
IncomingTask,TaskList, and their presentational components, ensuring proper functionality and rendering.Chores
@webex/cc-taskpackage.