feat(cc): getBuddyAgents() implementation #1

bhabalan · 2024-10-29T04:14:51Z

COMPLETES SPARK-567680

This pull request addresses

Fetching Buddy Agents

by making the following changes

introducing webex.cc.getBuddyAgents() API

Vidcast: https://app.vidcast.io/share/ae742d0a-126f-4e4c-90f0-0cf596473063

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 where tested

Manually tested in samples page for fetching buddy agents in following scenarios:

Station login not done
Station login done, buddy agents available
Station login done, buddy agents not available
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

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.

Kesari3008

Have done a quick review, please check comments

Kesari3008 · 2024-10-29T11:49:09Z

packages/@webex/plugin-cc/src/services/AgentService.ts

@@ -56,4 +70,31 @@ export default class AgentService {
      return Promise.reject(error);
    }
  }
+
+  public async getBuddyAgents(options: GetBuddyAgentsOptions): Promise<BuddyAgentsResponse> {


Let's call it data instead of options and please change the name from GetBuddyAgentsOptions to BuddyAgents

This comment is not addressed

Kesari3008 · 2024-10-29T11:50:03Z

packages/@webex/plugin-cc/src/services/AgentService.ts

+        resource: GET_BUDDY_AGENTS_API,
+        method: HTTP_METHODS.POST,
+        payload,
+        eventType: GET_BUDDY_AGENTS_EVENT,


Let's not use GET and EVENT in the naming convention

Removed the GET, But EVENT is required to differentiate it from an other type named BUDDY_AGENTS which defines the param shape for getBuddyAgents()

Kesari3008 · 2024-10-29T11:54:46Z

packages/@webex/plugin-cc/src/services/constants.ts

+
+export const AgentDesktopMessage = 'AgentDesktopMessage';
+
+export const GET_BUDDY_AGENTS_EVENT = 'BuddyAgents';


These 2 are the same string values...we can keep single constant here

Kesari3008 · 2024-10-29T12:01:14Z

packages/@webex/plugin-cc/src/features/Agent.ts

+    mediaType: BuddyAgentMediaType;
+    state?: BuddyAgentState;
+  }): Promise<BuddyAgentsResponse> {
+    const {agentProfileId, mediaType, state} = options;


Why are we fetching it here and then passing it one by one. We can pass the data as it is. We are not doing anything with the data herfe

Kesari3008 · 2024-10-29T12:01:35Z

packages/@webex/plugin-cc/src/features/Agent.ts

@@ -34,4 +34,25 @@ export default class Agent {
      return Promise.reject(new Error('Error while performing agent login', error));
    }
  }
+
+  public async getBuddyAgents(options: {
+    agentProfileId?: string;


agentProfileId should not be optional

Kesari3008 · 2024-10-29T12:03:06Z

packages/@webex/plugin-cc/src/services/HttpRequest.ts

      // Send the service request
-      const response = await this.webex.request({
+      const response: IHttpResponse = await this.webex.request({


Do we need to specify the type here? Based on the value assigned, type will be inferred automatically

Kesari3008 · 2024-10-29T12:04:42Z

packages/@webex/plugin-cc/test/unit/spec/features/Agent.ts

+      expect(agentServiceMock.getBuddyAgents).toHaveBeenCalledWith(options);
+    });
+
+    it('should throw an error when getBuddyAgents fails', async () => {


We should have one testcase for error response as well

Kesari3008 · 2024-10-29T12:05:29Z

packages/@webex/plugin-cc/src/services/HttpRequest.ts

@@ -96,14 +96,24 @@ class HttpRequest {
    try {
      const {service, resource, method, payload, eventType, success, failure} = options;

+      // prune undefined values
+      Object.keys(payload).forEach((key) => payload[key] === undefined && delete payload[key]);


What is this code supposed to be doing ?

{ "someKey": "someValue", "someOtherKey": undefined }

will get transformed to

{ "someKey": "someValue" }

In which case we are going to pass a key with undefined value?

Kesari3008 · 2024-10-29T12:06:23Z

packages/@webex/plugin-cc/src/cc.ts

+   *  state: BuddyAgentState.AVAILABLE,
+   *  });
+   */
+  public async getBuddyAgents({


Why have we created 3 functions for this?

Discussed 1:1

Kesari3008 · 2024-10-29T12:07:34Z

docs/samples/contact-center/index.html

+            <fieldset id="buddyAgentsBox">
+              <legend>Buddy Agents</legend>
+              <button id="fetchBuddyAgents" class="btn btn-primary my-3" onclick="fetchBuddyAgents()">Get Buddy Agents</button>
+              <ul id="buddyAgentsList"></ul>


Please use a dropdown to display the list of buddy agent list

rarajes2 · 2024-10-30T07:48:09Z

docs/samples/contact-center/app.js

+      buddyAgentsDropdown.add(option);
+    });
+  } catch (error) {
+    const buddyAgentsDropdown = document.getElementById('buddyAgentsDropdown');


Please add this to the top of file. Please remove from the try block as well.

rarajes2 · 2024-10-30T07:49:57Z

docs/samples/contact-center/app.js

+    });
+  } catch (error) {
+    const buddyAgentsDropdown = document.getElementById('buddyAgentsDropdown');
+    buddyAgentsDropdown.innerHTML = ''; // Clear previous options


This line can be moved to finally { } block. Can be removed from both try and catch

rarajes2 · 2024-10-30T07:52:48Z

docs/samples/contact-center/app.js

+    buddyAgentsDropdown.innerHTML = ''; // Clear previous options
+    const option = document.createElement('option');
+    option.text = `Failed to fetch buddy agents, ${error}`;
+    option.disabled = true;


All the three lines can be written in simple way

From

const option = document.createElement('option'); option.text = `Failed to fetch buddy agents, ${error}`; option.disabled = true;

To

const option = `<option disabled="true">Failed to fetch buddy agents, ${error}<option>`

Please check all other places as well.

rarajes2 · 2024-10-30T07:57:08Z

packages/@webex/plugin-cc/src/cc.ts

+        channelName,
+        state,
+      });
+      this.$webex.logger.log('Get Buddy Agents API SUCCESS');


Agent code is already logging the success message. This will add redundant log

rarajes2 · 2024-10-30T08:01:14Z

packages/@webex/plugin-cc/src/services/AgentService.ts

+
+  public async getBuddyAgents(options: BuddyAgents): Promise<BuddyAgentsSuccess> {
+    try {
+      const data = await this.httpRequest.sendRequestWithEvent({


Suggestion (Not to be fixed in this PR) - We should allow sendRequestWithEvent to accept a response type so that we don't have to typecast the response while returning.

cc - @Kesari3008, Ravi

rarajes2 · 2024-10-30T08:05:08Z

packages/@webex/plugin-cc/src/services/HttpRequest.ts

@@ -96,14 +96,24 @@ class HttpRequest {
    try {
      const {service, resource, method, payload, eventType, success, failure} = options;

+      // prune undefined values
+      Object.keys(payload).forEach((key) => payload[key] === undefined && delete payload[key]);


In which case we are going to pass a key with undefined value?

rarajes2 · 2024-10-30T08:07:17Z

packages/@webex/plugin-cc/src/types.ts

+  CHAT: 'chat',
+  SOCIAL: 'social',
+  EMAIL: 'email',
+} as const;


Why are we using as const ? What does it do?

For this we can create enum.

@rarajes2 This is the new way of creating enum. Please refer to this document: https://confluence-eng-gpk2.cisco.com/conf/display/WTWC/How+to+use+enums+and+alternatives

rarajes2 · 2024-10-30T08:08:52Z

packages/@webex/plugin-cc/src/types.ts

+  EMAIL: 'email',
+} as const;
+
+export type CHANNEL_NAME = Enum<typeof CHANNEL_NAME>;


We don't have to create a separate type like this. The constant defined above can be converted to an enum, no need to create a const variable.

@rarajes2 , Can you please provide an example on how to do this?

Kesari3008 · 2024-11-04T12:31:42Z

packages/@webex/plugin-cc/src/cc.ts

+   *  });
+   */
+  public async getBuddyAgents(
+    options: GetBuddyAgentsRequest = {


We still have Get in the name here

Kesari3008 · 2024-11-04T12:40:02Z

packages/@webex/plugin-cc/src/services/AgentService.ts

@@ -56,4 +64,28 @@ export default class AgentService {
      return Promise.reject(error);
    }
  }
+
+  public async getBuddyAgents(options: BuddyAgents): Promise<BuddyAgentsSuccess> {


Return type for all the methods should be generic not just associated with success

Kesari3008 · 2024-11-04T12:40:49Z

packages/@webex/plugin-cc/src/services/constants.ts

@@ -6,4 +6,10 @@ export const AGENT = 'agent';
 // CC GATEWAY API URL PATHS
 export const SUBSCRIBE_API = 'v1/notification/subscribe';
 export const LOGIN_API = 'v1/agents/login';
+export const GET_BUDDY_AGENTS_API = 'v1/agents/buddyList';


Remove Get from here as well

bhabalan · 2024-11-12T12:12:46Z

Closed this in favor of rsarika#3

bhabalan force-pushed the cc/getBuddyAgents branch from b6d5300 to e453b09 Compare October 29, 2024 04:22

bhabalan self-assigned this Oct 29, 2024

bhabalan added the validated label Oct 29, 2024

rsarika force-pushed the AgentLogin branch from 7a58a50 to c45ab61 Compare October 29, 2024 08:36

feat(cc): getBuddyAgents() implementation

3c9f2d0

bhabalan force-pushed the cc/getBuddyAgents branch from 7156ee3 to 3c9f2d0 Compare October 29, 2024 11:08

Merge branch 'AgentLogin' into cc/getBuddyAgents

b956fd9

Kesari3008 suggested changes Oct 29, 2024

View reviewed changes

bhabalan added 2 commits October 29, 2024 18:52

Merge branch 'AgentLogin' into cc/getBuddyAgents

f6fb68e

feat(cc): address feedback

371e50b

rarajes2 reviewed Oct 30, 2024

View reviewed changes

arun3528 force-pushed the AgentLogin branch 2 times, most recently from 2ece117 to ac5285e Compare October 31, 2024 13:30

Kesari3008 reviewed Nov 4, 2024

View reviewed changes

bhabalan closed this Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cc): getBuddyAgents() implementation #1

feat(cc): getBuddyAgents() implementation #1

bhabalan commented Oct 29, 2024 •

edited

Loading

Kesari3008 left a comment

Kesari3008 Oct 29, 2024

Kesari3008 Nov 4, 2024

Kesari3008 Oct 29, 2024

bhabalan Oct 29, 2024

Kesari3008 Oct 29, 2024

Kesari3008 Oct 29, 2024

Kesari3008 Oct 29, 2024

Kesari3008 Oct 29, 2024

Kesari3008 Oct 29, 2024

Kesari3008 Oct 29, 2024

bhabalan Oct 29, 2024

rarajes2 Oct 30, 2024

Kesari3008 Oct 29, 2024

bhabalan Oct 29, 2024

Kesari3008 Oct 29, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

rarajes2 Oct 30, 2024

Kesari3008 Nov 4, 2024

rarajes2 Oct 30, 2024

bhabalan Oct 30, 2024

Kesari3008 Nov 4, 2024

Kesari3008 Nov 4, 2024

Kesari3008 Nov 4, 2024

bhabalan commented Nov 12, 2024


		export const AgentDesktopMessage = 'AgentDesktopMessage';

		export const GET_BUDDY_AGENTS_EVENT = 'BuddyAgents';

feat(cc): getBuddyAgents() implementation #1

feat(cc): getBuddyAgents() implementation #1

Conversation

bhabalan commented Oct 29, 2024 • edited Loading

COMPLETES SPARK-567680

This pull request addresses

by making the following changes

Vidcast: https://app.vidcast.io/share/ae742d0a-126f-4e4c-90f0-0cf596473063

Change Type

The following scenarios where tested

I certified that

Kesari3008 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhabalan commented Nov 12, 2024

bhabalan commented Oct 29, 2024 •

edited

Loading