-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(cc): getBuddyAgents implementation #3
feat(cc): getBuddyAgents implementation #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with few minor comments.
Please test scenario where if agentProfileId is missing, if the scenario is handled properly. If possible share the network logs for success and agentprofileID missing case in the PR
/** | ||
* Returns the list of buddy agents in the given state and media according to agent profile settings | ||
* | ||
* @param {BuddyAgents} data - The data required to fetch buddy agents, including additional agent profile information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the JSDOC, we should have either examples of how data looks like or talk about what data properties are here itself
@@ -17,6 +17,9 @@ export const CC_EVENTS = { | |||
AGENT_STATE_CHANGE: 'AgentStateChange', | |||
AGENT_STATE_CHANGE_SUCCESS: 'AgentStateChangeSuccess', | |||
AGENT_STATE_CHANGE_FAILED: 'AgentStateChangeFailed', | |||
AGENT_BUDDY_AGENTS: 'BuddyAgents', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have 2 constants for the same string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following the convention of One for the event name and one for success event. If the event name changes in the future, it'll just be a small change here in the constants
}, | ||
}; | ||
|
||
const buddyAgentsMock = jest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a spy not mock so please update the name accordingly, also please use toHaveBeenCalledOnceWith instead of toHaveBeenCalledWith in all the places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed 1:1
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
[getBuddyAgents tests.zip](https://github.com/user-attachments/files/17717215/getBuddyAgents.tests.zip)Change Type
The following scenarios where tested
Manually tested in samples page for fetching buddy agents in following scenarios:
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.