feat(plugin-cc): add getQueues() method #4176
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 📝 WalkthroughWalkthroughThis pull request introduces a new asynchronous function in the contact center’s application code that retrieves a list of telephony queues via the Possibly related PRs
🪧 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
Documentation and Community
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| @@ -487,4 +487,10 @@ export default class ContactCenter extends WebexPlugin implements IContactCenter | |||
| throw detailedError; | |||
| } | |||
| } | |||
|
|
|||
| public async getQueues() { | |||
| const orgId = this.$webex.credentials.getOrgId(); | |||
There was a problem hiding this comment.
What happens if we fail to fetch orgId here ?
There was a problem hiding this comment.
Added a check and throwing an error if not found.
packages/@webex/plugin-cc/src/cc.ts
Outdated
| @@ -487,4 +487,10 @@ export default class ContactCenter extends WebexPlugin implements IContactCenter | |||
| throw detailedError; | |||
| } | |||
| } | |||
|
|
|||
| public async getQueues() { | |||
There was a problem hiding this comment.
Please add the description for this public method
| @@ -225,6 +225,18 @@ function toggleTransferOptions() { | |||
| transferOptionsElm.style.display = isTransferOptionsShown ? 'block' : 'none'; | |||
| } | |||
|
|
|||
| async function getQueueListForTelephonyChannel() { | |||
There was a problem hiding this comment.
Where are we checking for consult to queue feature flag to idetify if the queue should be show in the dropdown for consult?
There was a problem hiding this comment.
Couldn't find the feature flag API. Not sure how we can add that check. Please let me know if you have more info on that, I will add that check here. For now it will throw error on the samples page if it's not enabled from control hub.
|
|
||
| LoggerProxy.log('getQueues api success.', {module: CONFIG_FILE_NAME, method: 'getQueues'}); | ||
|
|
||
| return Promise.resolve(response.body?.data); |
There was a problem hiding this comment.
Just a thought: If body is undefined here then promise.resolve will happen with undefined data. Shouldn't we ensure that body is present before resolving the promise or if no queues are present. empty array is returned instead of undefined
There was a problem hiding this comment.
body can't be undefined if the status code is 200. I had added the check just for any rare unknown case so that it does not break the application. We are not adding this check in any other methods. Please let me know if any change is required here.
There was a problem hiding this comment.
Also why do we need to promise resolve? Just returning the data is sufficient right? It is an async method anyway
There was a problem hiding this comment.
@mkesavan13 True, all other methods are doing the same. Kept it consistent with them.
There was a problem hiding this comment.
I think it's okay to break the uniformity here or for future methods like this.
| @@ -1003,4 +1013,26 @@ describe('webex.cc', () => { | |||
| expect(getErrorDetailsSpy).toHaveBeenCalledWith(error, 'startOutdial', CC_FILE); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('getQueues', () => { | |||
There was a problem hiding this comment.
What about failed scenario testcase?
|
|
||
| const result = await webex.cc.getQueues(); | ||
|
|
||
| expect(webex.cc.services.config.getQueues).toHaveBeenCalled(); |
There was a problem hiding this comment.
Please use toHaveBeenCalledWith and mention the orgId sent as a parameter
packages/@webex/plugin-cc/src/cc.ts
Outdated
| @@ -487,4 +487,10 @@ export default class ContactCenter extends WebexPlugin implements IContactCenter | |||
| throw detailedError; | |||
| } | |||
| } | |||
|
|
|||
| public async getQueues() { | |||
There was a problem hiding this comment.
This is the public method from the SDK that external developers will use. Shouldn't this method have a return type mentioned.
|
|
||
| const result = await agentConfigService.getQueues(mockOrgId); | ||
|
|
||
| expect(mockHttpRequest.request).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
This expect statement should be present in failure testcases below too
|
@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: 0
♻️ Duplicate comments (2)
packages/@webex/plugin-cc/src/cc.ts (1)
491-495: 🛠️ Refactor suggestionAdd JSDoc and return type to public method.
As a public method in the SDK, this function should include JSDoc documentation and a return type annotation. It's also missing error handling that's present in other public methods.
Consider implementing like this:
- public async getQueues() { + /** + * Retrieves a list of queues for the organization. + * This is used for consult or transfer operations. + * + * @returns {Promise<Array<ContactServiceQueue>>} A promise that resolves to a list of queues. + * @throws Error if retrieving queues fails + */ + public async getQueues(): Promise<Array<ContactServiceQueue>> { + try { const orgId = this.$webex.credentials.getOrgId(); + if (!orgId) { + throw new Error('Failed to retrieve organization ID'); + } return this.services.config.getQueues(orgId); + } catch (error) { + const {error: detailedError} = getErrorDetails(error, 'getQueues', CC_FILE); + throw detailedError; + } }packages/@webex/plugin-cc/test/unit/spec/cc.ts (1)
1017-1037: 🛠️ Refactor suggestionAdd test for failure scenario.
The test suite only covers the success case for the
getQueuesmethod. Add a test case for error handling to match the pattern of other test suites in this file.Also, use
toHaveBeenCalledWithto verify the orgId parameter is being passed correctly:expect(webex.cc.services.config.getQueues).toHaveBeenCalled(); + expect(webex.cc.services.config.getQueues).toHaveBeenCalledWith('mockOrgId');Consider adding a failure test case:
it('should handle error during getQueues', async () => { const error = { details: { trackingId: '1234', data: { reason: 'Error retrieving queues', }, }, }; webex.cc.services.config.getQueues = jest.fn().mockRejectedValue(error); await expect(webex.cc.getQueues()).rejects.toThrow(error.details.data.reason); expect(LoggerProxy.error).toHaveBeenCalledWith( `getQueues failed with trackingId: ${error.details.trackingId}`, {module: CC_FILE, method: 'getQueues'} ); expect(getErrorDetailsSpy).toHaveBeenCalledWith(error, 'getQueues', CC_FILE); });
🧹 Nitpick comments (1)
docs/samples/contact-center/index.html (1)
183-184: Capitalization changed in placeholder text.The placeholder text has been capitalized from "Enter destination" to "Enter Destination". Consider maintaining consistent capitalization with the transfer input field on line 198 which still uses "Enter destination".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/samples/contact-center/app.js(3 hunks)docs/samples/contact-center/index.html(2 hunks)packages/@webex/plugin-cc/src/cc.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/constants.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/index.ts(2 hunks)packages/@webex/plugin-cc/src/services/config/types.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/cc.ts(4 hunks)packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/@webex/plugin-cc/src/services/config/index.ts (5)
packages/@webex/plugin-cc/src/services/config/types.ts (1)
ContactServiceQueue(594-624)packages/@webex/plugin-cc/src/services/config/constants.ts (1)
endPointMap(16-47)packages/@webex/plugin-cc/src/services/constants.ts (1)
WCC_API_GATEWAY(2-2)packages/@webex/plugin-cc/src/types.ts (2)
HTTP_METHODS(9-15)HTTP_METHODS(18-18)packages/@webex/plugin-cc/src/constants.ts (1)
CONFIG_FILE_NAME(8-8)
packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts (2)
packages/@webex/plugin-cc/src/logger-proxy.ts (1)
LoggerProxy(4-48)packages/@webex/plugin-cc/src/constants.ts (1)
CONFIG_FILE_NAME(8-8)
docs/samples/contact-center/app.js (2)
packages/@webex/test-helper-mock-webex/src/index.js (1)
webex(222-222)packages/@webex/plugin-cc/src/logger-proxy.ts (1)
error(35-39)
🔇 Additional comments (15)
docs/samples/contact-center/index.html (2)
177-180: Default dropdown selection now set to "Dial Number".The default selection for the consult destination type has been changed from "Queue" to "Dial Number", which aligns with the changes made to the transfer options below.
193-196: Default dropdown selection now set to "Dial Number".The default selection for the transfer destination type has been changed from "Queue" to "Dial Number".
packages/@webex/plugin-cc/src/services/config/constants.ts (1)
46-46: Added new endpoint mapping for queue list.The new endpoint mapping for retrieving the list of queues follows the existing pattern and facilitates the new functionality for displaying available queues.
docs/samples/contact-center/app.js (4)
228-237: LGTM: Good implementation of the queue retrieval function.The implementation of
getQueueListForTelephonyChannelis well-structured with proper error handling through a try-catch block and appropriate logging of failures.
228-228: Check for feature flag for the consult to queue capability.Where are we checking for consult to queue feature flag to identify if the queue should be shown in the dropdown for consult? This was previously noted in a review comment.
#!/bin/bash # Search for feature flag checks related to consultToQueue rg -A 2 "consultToQueue" --glob "*.ts" --glob "*.js" --glob "*.json"
261-275: Properly handles queue selection in consult options.The implementation correctly fetches the queue list and creates a dropdown with options based on queue names and IDs when the destination type is set to 'queue'.
304-317: Properly handles queue selection in transfer options.The implementation correctly fetches the queue list and creates a dropdown with options based on queue names and IDs when the transfer destination type is set to 'queue'.
packages/@webex/plugin-cc/src/services/config/index.ts (3)
536-564: Consider handling undefined response data.When Promise.resolve is called with
response.body?.data, there's no fallback ifbodyordatais undefined. The method is typed to returnPromise<ContactServiceQueue[]>, but could potentially returnPromise<undefined>.Consider modifying line 556 to return an empty array instead of undefined in such cases:
- return Promise.resolve(response.body?.data); + return Promise.resolve(response.body?.data || []);
536-540: Good method documentation.The method has appropriate JSDoc comments explaining its purpose, parameters, and return type.
541-564: LGTM: Implementation follows established patterns.The implementation follows the same pattern as other API methods in the class with proper error handling, logging, and response status checking.
packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts (3)
931-959: Good test coverage for successful API response.The test correctly verifies that the method makes the appropriate API call and processes the response as expected.
961-969: Complete error testing.The test properly verifies error handling when the API call fails.
971-982: Complete non-200 response testing.The test properly verifies error handling when the API returns a non-200 status code.
packages/@webex/plugin-cc/src/services/config/types.ts (2)
588-592: Well-defined CallDistributionGroup type.The type definition for CallDistributionGroup includes all necessary properties with appropriate types.
594-624: Comprehensive ContactServiceQueue type definition.The ContactServiceQueue type is well-structured with a comprehensive list of properties that cover all aspects of a queue configuration.
|
|
||
| LoggerProxy.log('getQueues api success.', {module: CONFIG_FILE_NAME, method: 'getQueues'}); | ||
|
|
||
| return Promise.resolve(response.body?.data); |
There was a problem hiding this comment.
Also why do we need to promise resolve? Just returning the data is sufficient right? It is an async method anyway
There was a problem hiding this comment.
Looks good to me mostly. Just one nitpick about the promise resolution step. I've resolved the rest.
Thanks for the PR, @rarajes2
COMPLETES #< SPARK-567681 >
This pull request addresses
While performing consult or transfer we have option to choose the type of consult such as agent, DN, queue etc.
We needed to fetch the list of queues to perform the consult/transfer operation for queue as a result and for that we have added a method inside
cc.tsfile asgetQueues().by making the following changes
Added a method
getQueues(orgId)inside thepackages/@webex/plugin-cc/src/services/config/index.tsand exposed to the developer throughcc.tsfile with the same name.We are able to get the list of queue.
Vidcast - https://app.vidcast.io/share/abe60753-715a-4811-b655-e0a40cb7c303
Change Type
The following scenarios were tested
Note: Based on the requirement mentioned on the ticket, the
getQueues()method is working as expected.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.