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(plugin-cc): event emission fix for different disconnect flows #4036

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

Kesari3008
Copy link
Contributor

@Kesari3008 Kesari3008 commented Dec 19, 2024

COMPLETES #AD-HOC

This pull request addresses

Fixed emitting event from the task object for an event corresponding to CONTACT_ENDED.
Simplified the approach for different disconnect flows.

by making the following changes

We will emit 'task:end' event for all four disconnect scenarios listed below. This event will send event data as {wrapupRequired: boolean}.

  1. The caller disconnects the call before the agent answers it. wrapupRequired will be set to false.
  2. Agent declines the call. wrapupRequired will be set to false.
  3. The agent answers the call and ends it. wrapupRequired will be set to true.
  4. The agent answers the call and caller disconnects it. wrapupRequired will be set to true.

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios were tested

Tested below 4 scenarios:

  1. The caller disconnects the call before the agent answers it.
  2. Agent declines the call.
  3. The agent answers the call and ends it.
  4. The agent answers the call and the caller disconnects it.

CC-SDK-COnsole-Logs-Disconnect-Flows.txt.log

I certified that

  • I have read and followed contributing guidelines

  • I discussed changes with code owners prior to submitting this pull request

  • I have not skipped any automated checks

  • All existing and new tests passed

  • I have updated the documentation accordingly


Make sure to have followed the contributing guidelines before submitting.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new Contact Center module, enhancing the Webex SDK with contact management capabilities.
    • Added a sample project for the Contact Center, improving user navigation and access to examples.
    • Implemented a comprehensive logging system for better error tracking and reporting.
  • Bug Fixes

    • Enhanced URL parsing and matching logic for service URLs, improving reliability in service connections.
    • Improved error handling in various components to prevent crashes and ensure smoother user experiences.
  • Documentation

    • Updated README files and added documentation for the new Contact Center functionalities and usage examples.
  • Tests

    • Expanded unit test coverage for the Contact Center module, ensuring robust functionality and error handling across various scenarios.

@Kesari3008 Kesari3008 added the validated If the pull request is validated for automation. label Dec 19, 2024
@Kesari3008 Kesari3008 requested review from a team as code owners December 19, 2024 16:03
Copy link

coderabbitai bot commented Dec 19, 2024

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a comprehensive implementation of the Webex Contact Center (CC) plugin, adding a new package @webex/plugin-cc with extensive functionality for managing agent interactions, WebSocket connections, task handling, and configuration management. The changes span multiple files across the Webex SDK ecosystem, introducing TypeScript-based implementations for contact center operations, including agent registration, task management, and WebSocket communication.

Changes

File Change Summary
.github/workflows/deploy.yml Added wxcc branch to deployable branches, added conditional to prevent documentation publishing for wxcc branch
docs/index.html Added new link to Contact Center sample in the "Samples" section
packages/@webex/plugin-cc/* Comprehensive new package implementation with TypeScript files for Contact Center functionality
packages/webex/package.json Added new export for contact center module and dependency on @webex/plugin-cc
packages/webex/src/contact-center.js New module for initializing Webex Contact Center functionality
packages/webex/src/webex.js Added requirement for @webex/plugin-cc module
webpack.config.js Added new entry point for contact center module

Sequence Diagram

sequenceDiagram
    participant Agent
    participant WebexCC as Webex Contact Center
    participant WebSocket
    participant TaskManager
    participant WebCallingService

    Agent->>WebexCC: Initialize
    WebexCC->>WebSocket: Connect
    WebSocket-->>WebexCC: Welcome Event
    Agent->>WebexCC: Register
    WebexCC->>TaskManager: Set Up Listeners
    Agent->>WebexCC: Login
    WebexCC->>WebCallingService: Register Line
    WebSocket->>WebexCC: Incoming Task
    WebexCC->>TaskManager: Handle Task
    Agent->>WebexCC: Accept/Decline Task
    WebCallingService->>Agent: Manage Call
Loading

Possibly related PRs

Suggested Reviewers

  • sreenara
  • mkesavan13
  • adhmenon
  • chburket

Poem

🐰 Hop, hop, WebEx CC arrives!
A plugin that truly thrives
Agents dance, tasks take flight
Connections sparkling bright
Contact Center magic, now alive! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sreenara sreenara changed the base branch from next to wxcc December 19, 2024 16:12
Copy link

@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.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

🛑 Comments failed to post (32)
packages/@webex/plugin-cc/src/services/core/GlobalTypes.ts (1)

8-14: 🛠️ Refactor suggestion

Consider restructuring to avoid data redundancy

The Failure type includes trackingId and orgId in both the base message and data object, which could lead to inconsistencies. Consider restructuring to avoid this redundancy.

-export type Failure = Msg<{
-  agentId: string;
-  trackingId: string;
-  reasonCode: number;
-  orgId: string;
-  reason: string;
-}>;
+export type FailureData = {
+  agentId: string;
+  reasonCode: number;
+  reason: string;
+};
+export type Failure = Msg<FailureData>;

Additionally, consider adding an enum for reasonCode to improve type safety and documentation:

export enum FailureReasonCode {
  AGENT_DISCONNECT = 1,
  CALLER_DISCONNECT = 2,
  // Add other relevant codes
}
packages/@webex/plugin-cc/src/services/core/constants.ts (1)

3-3: 🛠️ Refactor suggestion

Redundant socket timeout constants

CLOSE_SOCKET_TIMEOUT_DURATION and CLOSE_SOCKET_TIMEOUT have the same value (16000ms) and seem to serve the same purpose. This could lead to confusion and maintenance issues.

Consider consolidating these into a single constant with a clear, descriptive name:

-export const CLOSE_SOCKET_TIMEOUT_DURATION = 16000;
-export const CLOSE_SOCKET_TIMEOUT = 16000;
+export const SOCKET_CLOSE_TIMEOUT = 16000; // Duration to wait before forcibly closing the socket

Also applies to: 11-11

packages/@webex/plugin-cc/src/logger-proxy.ts (2)

1-6: 🛠️ Refactor suggestion

Consider refactoring to a singleton or namespace pattern

The current implementation uses a class with only static members, which goes against object-oriented principles. Consider refactoring to either:

  1. A singleton pattern for better instance management and testability
  2. A namespace using TypeScript's namespace feature

Example singleton implementation:

export class LoggerProxy {
  private static instance: LoggerProxy;
  private logger?: Logger;

  private constructor() {}

  public static getInstance(): LoggerProxy {
    if (!LoggerProxy.instance) {
      LoggerProxy.instance = new LoggerProxy();
    }
    return LoggerProxy.instance;
  }

  public setLogger(logger: Logger): void {
    this.logger = logger;
  }
  // ... rest of the methods become instance methods
}

7-10: 🛠️ Refactor suggestion

Enhance logger initialization with validation and feedback

The initialization method lacks parameter validation and doesn't provide feedback on success.

Consider this improved implementation:

-  public static initialize(logger: Logger): void {
+  public static initialize(logger: Logger): boolean {
+    if (!logger) {
+      throw new Error('Logger instance must be provided');
+    }
     LoggerProxy.logger = logger;
+    return true;
   }
+
+  public static reset(): void {
+    LoggerProxy.logger = undefined;
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  public static initialize(logger: Logger): boolean {
    if (!logger) {
      throw new Error('Logger instance must be provided');
    }
    LoggerProxy.logger = logger;
    return true;
  }

  public static reset(): void {
    LoggerProxy.logger = undefined;
  }
packages/@webex/plugin-cc/src/services/agent/index.ts (1)

62-66: 💡 Codebase verification

Based on the search results, I can now generate the final response:

Add null check for trackingId split operation

The code should handle cases where trackingid header is missing or malformed. Looking at the codebase:

  1. The trackingid header is used in multiple places with proper null checks
  2. The Utils.ts file already has a safe handling pattern: errObj?.headers?.trackingid || errObj?.headers?.TrackingID
  3. The split operation in stationLogin is not following this safe pattern
trackingId: e.response?.headers?.trackingid?.split('_')[1],
🔗 Analysis chain

Consider verifying “trackingId” presence in stationLogin error.

Here, the code attempts to parse “trackingId” from the response headers. If the server does not return a tracking ID, it is possible that “split('_')[1]” is undefined. This may lead to undesired behavior.

Here's a script that searches the codebase for other references to “split('_')” or “trackingid” usage, aiming to confirm consistent safe parsing:


Let me analyze the usage of tracking IDs and string splitting operations in the codebase to verify the error handling consistency.


Let me analyze how tracking IDs are handled in error scenarios to verify if additional null checks are needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for split('_') or trackingid usage to ensure consistent error handling
rg "split\(\'_\'\)|trackingid"

Length of output: 137


Script:

#!/bin/bash
# Search for trackingid or tracking-id in headers with proper escaping
rg -i "tracking.?id"

# Search for string split operations
ast-grep --pattern 'split($_)'

Length of output: 82203


Script:

#!/bin/bash
# Search for trackingid header handling in the codebase
rg -A 2 "headers\.trackingid" 

# Search for error handling related to trackingid
rg -A 2 "trackingId.*split"

Length of output: 69259

packages/@webex/plugin-cc/src/services/task/index.ts (1)

100-110: ⚠️ Potential issue

Fix mismatch between the interface and implemented method signature.
In the ITask interface (lines 365–365 in types.ts), decline() requires a TaskId parameter, but the actual implementation here does not accept any parameters. To maintain alignment, either remove the parameter from the interface or accept it in this implementation.

- public async decline(): Promise<TaskResponse> {
+ public async decline(taskId?: TaskId): Promise<TaskResponse> {
    try {
      this.webCallingService.declineCall(this.data.interactionId);
      ...
    } catch (error) {
      ...
    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  public async decline(taskId?: TaskId): Promise<TaskResponse> {
    try {
      this.webCallingService.declineCall(this.data.interactionId);
      this.unregisterWebCallListeners();

      return Promise.resolve();
    } catch (error) {
      const {error: detailedError} = getErrorDetails(error, 'decline', CC_FILE);
      throw detailedError;
    }
  }
packages/@webex/plugin-cc/src/services/task/types.ts (2)

365-365: ⚠️ Potential issue

Ensure interface parameters match class implementation.
In ITask, the function signature for decline expects decline(taskId: TaskId). However, the actual implementation in index.ts does not take a parameter. Synchronize them to avoid type inconsistencies.


326-326: 🛠️ Refactor suggestion

Use undefined instead of void in union type.
Static analysis warns that void is confusing in a union. Replacing void with undefined clarifies the return type.

- export type TaskResponse = AgentContact | Error | void;
+ export type TaskResponse = AgentContact | Error | undefined;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export type TaskResponse = AgentContact | Error | undefined;
🧰 Tools
🪛 Biome (1.9.4)

[error] 326-326: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

packages/@webex/plugin-cc/src/services/task/contact.ts (1)

333-352: ⚠️ Potential issue

Align “cancelTask” with the four disconnect flows.
This function sends “CONTACT_ENDED” on success. Confirm that it covers the four disconnect cases described in the PR objective, or if some scenarios require it to also emit additional details about wrap-up requirements.

packages/@webex/plugin-cc/src/services/config/types.ts (1)

6-52: ⚠️ Potential issue

Ensure alignment with the four disconnect flows.
“CONTACT_ENDED,” “AGENT_CONTACT_END_FAILED,” and “AGENT_WRAPUP” are listed, but confirm that the new required event for “wrapupRequired” is also included or that the event payload has a boolean property.

packages/@webex/plugin-cc/src/services/task/constants.ts (1)

11-11: ⚠️ Potential issue

Fix missing leading slash in RESUME constant.

The RESUME constant is missing a leading slash, unlike other API endpoint constants.

-export const RESUME = 'record/resume';
+export const RESUME = '/record/resume';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export const RESUME = '/record/resume';
packages/@webex/plugin-cc/src/config.ts (1)

20-21: ⚠️ Potential issue

Address the TODO comment regarding dynamic domain configuration

The hard-coded production domain could cause issues when deploying to different environments. This should be configurable based on the environment.

Consider implementing environment-based configuration:

- domain: 'rtw.prod-us1.rtmsprod.net',
+ domain: process.env.CC_DOMAIN || 'rtw.prod-us1.rtmsprod.net',
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        // TODO: This should be dynamic based on the environment
        domain: process.env.CC_DOMAIN || 'rtw.prod-us1.rtmsprod.net',
packages/@webex/plugin-cc/src/services/core/HttpRequest.ts (2)

20-34: 🛠️ Refactor suggestion

Add error handling and timeout configuration to request method

The request method lacks error handling and timeout configuration.

Consider adding these improvements:

 public async request(options: {
   service: string;
   resource: string;
   method: HTTP_METHODS;
   body?: RequestBody;
+  timeout?: number;
 }): Promise<IHttpResponse> {
-  const {service, resource, method, body} = options;
+  const {service, resource, method, body, timeout = 30000} = options;

-  return this.webex.request({
-    service,
-    resource,
-    method,
-    body,
-  });
+  try {
+    return await this.webex.request({
+      service,
+      resource,
+      method,
+      body,
+      timeout,
+    });
+  } catch (error) {
+    throw createErrDetailsObject(error);
+  }
 }

Committable suggestion skipped: line range outside the PR's diff.


7-13: 🛠️ Refactor suggestion

Improve singleton implementation with proper error handling

The current singleton implementation could be enhanced:

  1. Missing null check for the instance return
  2. No error handling for invalid webex instance

Consider this improved implementation:

 public static getInstance(options?: {webex: WebexSDK}): HttpRequest {
-  if (!HttpRequest.instance && options && options.webex) {
+  if (!HttpRequest.instance) {
+    if (!options?.webex) {
+      throw new Error('WebexSDK instance is required');
+    }
     HttpRequest.instance = new HttpRequest(options);
   }
+  if (!HttpRequest.instance) {
+    throw new Error('Failed to initialize HttpRequest');
+  }
   return HttpRequest.instance;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  public static getInstance(options?: {webex: WebexSDK}): HttpRequest {
    if (!HttpRequest.instance) {
      if (!options?.webex) {
        throw new Error('WebexSDK instance is required');
      }
      HttpRequest.instance = new HttpRequest(options);
    }
    if (!HttpRequest.instance) {
      throw new Error('Failed to initialize HttpRequest');
    }
    return HttpRequest.instance;
  }
packages/@webex/plugin-cc/src/services/core/Utils.ts (1)

13-27: 🛠️ Refactor suggestion

Improve type safety and error handling in getErrorDetails

The current implementation has potential type safety issues and could be more robust.

Consider these improvements:

-export const getErrorDetails = (error: any, methodName: string, moduleName: string) => {
+export const getErrorDetails = (
+  error: Error & { details?: Failure },
+  methodName: string,
+  moduleName: string
+) => {
   const failure = error.details as Failure;
-  const reason = failure?.data?.reason ?? `Error while performing ${methodName}`;
+  const defaultError = `Error while performing ${methodName}`;
+  const reason = failure?.data?.reason || defaultError;
   if (!(reason === 'AGENT_NOT_FOUND' && methodName === 'silentReLogin')) {
     LoggerProxy.error(`${methodName} failed with trackingId: ${failure?.trackingId}`, {
       module: moduleName,
       method: methodName,
+      error: error.message,
+      trackingId: failure?.trackingId,
     });
   }

   return {
-    error: new Error(reason ?? `Error while performing ${methodName}`),
+    error: new Error(reason),
     reason,
   };
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export const getErrorDetails = (
  error: Error & { details?: Failure },
  methodName: string,
  moduleName: string
) => {
  const failure = error.details as Failure;
  const defaultError = `Error while performing ${methodName}`;
  const reason = failure?.data?.reason || defaultError;
  if (!(reason === 'AGENT_NOT_FOUND' && methodName === 'silentReLogin')) {
    LoggerProxy.error(`${methodName} failed with trackingId: ${failure?.trackingId}`, {
      module: moduleName,
      method: methodName,
      error: error.message,
      trackingId: failure?.trackingId,
    });
  }

  return {
    error: new Error(reason),
    reason,
  };
};
packages/@webex/plugin-cc/README.md (1)

16-18: ⚠️ Potential issue

TODO items need to be addressed

The documentation contains empty links that need to be completed. Please provide the actual documentation links or remove the TODO comment if the documentation is not ready yet.

🧰 Tools
🪛 Markdownlint (0.37.0)

17-17: null
No empty links

(MD042, no-empty-links)


18-18: null
No empty links

(MD042, no-empty-links)

packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (3)

42-42: ⚠️ Potential issue

Replace 'as any' type assertions with proper types

Using 'as any' bypasses TypeScript's type checking and could hide potential issues.

Define proper interface types for the request data:

interface StationLoginData {
  // Add required properties
}

interface StateChangeData {
  // Add required properties
}

interface BuddyAgentsData {
  // Add required properties
}

// Then use these types instead of 'as any'
const req = await agent.stationLogin({data: {} as StationLoginData});

Also applies to: 49-49, 55-55


25-31: 🛠️ Refactor suggestion

Enhance error handling test coverage

The logout error test could be more comprehensive and validate error handling behavior.

Consider expanding the test:

it('logout handles errors appropriately', async () => {
  const reqSpy = jest.spyOn(fakeAqm, 'reqEmpty');
  const testError = new Error('Logout failed');
  reqSpy.mockRejectedValue(testError);
  
  await expect(agent.logout({
    data: {logoutReason: 'User requested logout'}
  })).rejects.toThrow('Logout failed');
  
  expect(reqSpy).toHaveBeenCalledWith(
    expect.objectContaining({
      data: {logoutReason: 'User requested logout'}
    })
  );
});

33-58: 🛠️ Refactor suggestion

Add validation for request payloads

The tests don't verify the structure of the request payloads sent to the AQM service.

Add expectations to verify request payload structure:

it('stateChange sends correct payload', async () => {
  const reqSpy = jest.spyOn(fakeAqm, 'req');
  const stateData = {
    newState: 'AVAILABLE',
    reason: 'Agent available'
  };
  
  await agent.stateChange({data: stateData});
  
  expect(reqSpy).toHaveBeenCalledWith(
    expect.objectContaining({
      data: expect.objectContaining(stateData)
    })
  );
});
packages/@webex/plugin-cc/package.json (1)

25-32: 🛠️ Refactor suggestion

Review dependency placement

jest-html-reporters should be moved to devDependencies as it's typically only needed during testing.

 "dependencies": {
   "@types/platform": "1.3.4",
   "@webex/calling": "workspace:*",
   "@webex/internal-plugin-mercury": "workspace:*",
   "@webex/webex-core": "workspace:*",
-  "buffer": "6.0.3",
-  "jest-html-reporters": "3.0.11"
+  "buffer": "6.0.3"
 },
 "devDependencies": {
+  "jest-html-reporters": "3.0.11",
   "@babel/preset-typescript": "7.22.11",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  "dependencies": {
    "@types/platform": "1.3.4",
    "@webex/calling": "workspace:*",
    "@webex/internal-plugin-mercury": "workspace:*",
    "@webex/webex-core": "workspace:*",
    "buffer": "6.0.3"
  },
packages/@webex/plugin-cc/src/services/core/websocket/keepalive.worker.js (1)

51-74: 🛠️ Refactor suggestion

Enhance error handling for message events

The message event handler should include error handling for malformed messages and invalid event types.

 addEventListener("message", (event) => {
+  if (!event.data) {
+    console.error('Received invalid message event');
+    return;
+  }
+
   if (event.data?.type === "start") {
     intervalDuration = event.data?.intervalDuration || 4000;
     closeSocketTimeout = event.data?.closeSocketTimeout || 5000;

Committable suggestion skipped: line range outside the PR's diff.

packages/@webex/plugin-cc/src/services/agent/types.ts (2)

83-83: 🛠️ Refactor suggestion

Enhance type safety for logout reason

The logout reason should be a proper enum or union type instead of string literals.

-export type Logout = {logoutReason?: 'User requested logout' | 'Inactivity Logout'};
+export enum LogoutReason {
+  USER_REQUESTED = 'User requested logout',
+  INACTIVITY = 'Inactivity Logout'
+}
+export type Logout = {logoutReason?: LogoutReason};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export enum LogoutReason {
  USER_REQUESTED = 'User requested logout',
  INACTIVITY = 'Inactivity Logout'
}
export type Logout = {logoutReason?: LogoutReason};

85-85: 🛠️ Refactor suggestion

Define AgentState as enum instead of string union

Using string type allows any string value, which could lead to runtime errors. Consider using an enum for better type safety.

-export type AgentState = 'Available' | 'Idle' | 'RONA' | string;
+export enum AgentState {
+  AVAILABLE = 'Available',
+  IDLE = 'Idle',
+  RONA = 'RONA'
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export enum AgentState {
  AVAILABLE = 'Available',
  IDLE = 'Idle',
  RONA = 'RONA'
}
packages/@webex/plugin-cc/test/unit/spec/services/task/contact.ts (1)

140-147: ⚠️ Potential issue

Add test cases for all disconnect scenarios

The current test for contact end is basic and doesn't cover all the disconnect scenarios mentioned in the PR objectives. Please add test cases for:

  1. Caller disconnects before agent answers (wrapupRequired: false)
  2. Agent declines call (wrapupRequired: false)
  3. Agent answers and ends call (wrapupRequired: true)
  4. Agent answers and caller disconnects (wrapupRequired: true)

Example test structure:

it("should handle caller disconnect before agent answer", () => {
  fakeAqm.pendingRequests = {};
  const req = contact.end({
    interactionId: "interactionId",
    data: { wrapupRequired: false, disconnectReason: "caller_disconnect_pre_answer" }
  });
  expect(req).toBeDefined();
  // Add expectations for correct event emission
});
docs/samples/contact-center/style.css (1)

365-367: 🛠️ Refactor suggestion

Ensure sufficient color contrast for accessibility.

The color values used for headings and error states should be verified for WCAG compliance:

  • #0052bf for h2
  • #de3434 for .webex-error

Consider using a color contrast checker to ensure these meet WCAG AA standards (4.5:1 for normal text, 3:1 for large text).

Also applies to: 391-395

docs/samples/contact-center/index.html (1)

61-62: 🛠️ Refactor suggestion

Add type="password" for sensitive input fields.

The access token input should be treated as sensitive information.

- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text">
+ <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password">
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

              <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password">
              <button id="access-token-save" class="btn-code" type="submit">webex.init()</button>
packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts (1)

158-158: 🛠️ Refactor suggestion

Avoid using type assertions to access private members.

Direct access to private members using type assertions is not a recommended practice in TypeScript.

Consider exposing a protected method for testing or restructuring the test to use public APIs:

- webRTCCalling['line'] = line;
+ // Add a protected method in WebCallingService for testing
+ // protected setLine(line: ILine) { this.line = line; }
+ webRTCCalling.setLine(line);

Committable suggestion skipped: line range outside the PR's diff.

packages/@webex/plugin-cc/test/unit/spec/services/task/index.ts (1)

242-250: 🛠️ Refactor suggestion

Add test coverage for task:end event emission.

Given that the PR objectives focus on event emission for different disconnect flows, consider adding assertions to verify that the 'task:end' event is emitted with the correct wrapupRequired flag.

it('should emit task:end event with correct wrapupRequired flag', async () => {
  const taskEmitSpy = jest.spyOn(task, 'emit');
  const expectedResponse = { data: { interactionId: taskId } } as AgentContact;
  contactMock.end.mockResolvedValue(expectedResponse);

  await task.end();

  expect(taskEmitSpy).toHaveBeenCalledWith('task:end', {
    wrapupRequired: true,
    taskId: taskId
  });
});

it('should emit task:end event with wrapupRequired=false for decline', async () => {
  const taskEmitSpy = jest.spyOn(task, 'emit');
  await task.decline();

  expect(taskEmitSpy).toHaveBeenCalledWith('task:end', {
    wrapupRequired: false,
    taskId: taskId
  });
});
packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts (1)

297-340: 💡 Codebase verification

Missing test coverage for disconnect scenarios

The test file lacks coverage for the following disconnect scenarios:

  1. Caller disconnects before agent answers
  2. Agent declines the call
  3. Agent answers and ends the call
  4. Agent answers and caller disconnects

Current test coverage only includes:

  • Task end on AGENT_WRAPUP event
  • Task removal on AGENT_WRAPPEDUP event
  • Basic task management (initialization, message handling)
🔗 Analysis chain

Add test cases for all disconnect scenarios

The test suite should cover all four disconnect scenarios mentioned in the PR objectives:

  1. Caller disconnects before agent answers
  2. Agent declines the call
  3. Agent answers and ends the call
  4. Agent answers and caller disconnects

Would you like me to help generate the missing test cases?


Let me search for test cases covering the disconnect scenarios.


Let me try a different search pattern to find test cases related to call scenarios.


Let me search for all test cases in the file to get a complete picture.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing test cases covering these scenarios
rg -A 5 "describe.*disconnect" || rg -A 5 "it.*disconnect"

Length of output: 131025


Script:

#!/bin/bash
# Search for test cases covering disconnect scenarios in TaskManager.ts
rg -A 10 "it.*disconnect|it.*decline|it.*end.*call" packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts

Length of output: 123


Script:

#!/bin/bash
# Search for test cases covering call-related events in TaskManager.ts
rg -A 10 "it.*('should.*call|'should.*agent|'should.*TASK_END)" packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts

Length of output: 617


Script:

#!/bin/bash
# Search for all test cases in TaskManager.ts
rg "it\('" packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts

Length of output: 1174

packages/@webex/internal-plugin-mercury/src/mercury.js (1)

519-527: ⚠️ Potential issue

Fix event emission logic and remove duplicate emission

The event emission logic has two issues:

  1. Duplicate event emission at lines 525-526
  2. Missing check for undefined namespace

Apply this fix:

-        if (data.eventType) {
-          const [namespace] = data.eventType.split('.');
-
-          if (namespace === data.eventType) {
-            this._emit(`event:${namespace}`, envelope);
-          } else {
-            this._emit(`event:${namespace}`, envelope);
-            this._emit(`event:${data.eventType}`, envelope);
-          }
+        if (!data.eventType) {
+          return;
+        }
+        const [namespace, eventName] = data.eventType.split('.');
+        if (!namespace) {
+          return;
+        }
+        this._emit(`event:${namespace}`, envelope);
+        if (eventName) {
+          this._emit(`event:${data.eventType}`, envelope);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        if (!data.eventType) {
          return;
        }
        const [namespace, eventName] = data.eventType.split('.');
        if (!namespace) {
          return;
        }
        this._emit(`event:${namespace}`, envelope);
        if (eventName) {
          this._emit(`event:${data.eventType}`, envelope);
        }
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (2)

172-174: ⚠️ Potential issue

Add error assertion in catch block.

Empty catch blocks can hide potential issues. Consider adding an assertion to verify expected errors.

-      } catch (e) {}
+      } catch (e) {
+        // If we expect this test to never throw, fail the test
+        expect(e).toBeUndefined();
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        ]);
        expect(p).toBeDefined();
      } catch (e) {
        // If we expect this test to never throw, fail the test
        expect(e).toBeUndefined();
      }

69-70: 🛠️ Refactor suggestion

Replace Function type with explicit function type definition.

The usage of the Function type is discouraged as it's too broad and can lead to type-safety issues.

Apply this diff to improve type safety:

-const eventListeners: { [key: string]: Function[] } = {};
-webSocketManagerInstance.on = jest.fn((event: string, listener: Function) => {
+type WebSocketListener = (...args: any[]) => void;
+const eventListeners: { [key: string]: WebSocketListener[] } = {};
+webSocketManagerInstance.on = jest.fn((event: string, listener: WebSocketListener) => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    type WebSocketListener = (...args: any[]) => void;
    const eventListeners: { [key: string]: WebSocketListener[] } = {};
    webSocketManagerInstance.on = jest.fn((event: string, listener: WebSocketListener) => {
🧰 Tools
🪛 Biome (1.9.4)

[error] 69-69: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 70-70: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-4036.d3m3l2kee0btzx.amplifyapp.com

Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

Changes look good. Minor questions and comments. However, UTs are not added. Please add them and we should be good to go.

Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

LGTM. Approving with one question on the UT

@rsarika rsarika enabled auto-merge (squash) December 20, 2024 13:01
@rsarika rsarika merged commit 510a290 into webex:wxcc Dec 20, 2024
12 checks passed
@sreenara sreenara deleted the event-emission-fix branch December 20, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants