fix(): adding move back to lobby sdk api #4248
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for moving meeting participants to the lobby in the Webex meetings plugin. A new display hint Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
582-583: Consider adding JSDoc and explicit TS typing forcanMoveToLobby.To align with other
MeetingUtilmethods, adding a brief JSDoc and a TypeScript signature can improve readability:- canMoveToLobby: (displayHints) => displayHints.includes(DISPLAY_HINTS.MOVE_TO_LOBBY), + /** + * Checks if "move back to lobby" is allowed based on display hints. + * @param {string[]} displayHints - Array of display hint identifiers. + * @returns {boolean} `true` if MOVE_TO_LOBBY hint is present. + */ + canMoveToLobby(displayHints: string[]): boolean { + return displayHints.includes(DISPLAY_HINTS.MOVE_TO_LOBBY); + },
🛑 Comments failed to post (3)
packages/@webex/plugin-meetings/src/members/index.ts (1)
889-912:
⚠️ Potential issueIncorrect error message in parameter validation.
The error message for the memberId validation refers to "raise/lower the hand of the member" instead of "move the member to the lobby". This copy-paste error should be fixed.
- return Promise.reject( - new ParameterError('The member id must be defined to raise/lower the hand of the member.') - ); + return Promise.reject( + new ParameterError('The member id must be defined to move the member to the lobby.') + );📝 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./** * @param {String} memberId * @param {boolean} [raise] - to raise hand (=true) or lower (=false), default: true * @returns {Promise} * @public * @memberof Members */ public moveToLobby(memberId: string) { if (!this.locusUrl) { return Promise.reject( new ParameterError( 'The associated locus url for this meetings members object must be defined.' ) ); } if (!memberId) { - return Promise.reject( - new ParameterError('The member id must be defined to raise/lower the hand of the member.') - ); + return Promise.reject( + new ParameterError('The member id must be defined to move the member to the lobby.') + ); } const options = MembersUtil.getMoveMemberToLobbyRequestOptions(memberId); return this.membersRequest.moveToLobbyMember({locusUrl: this.locusUrl, memberId}, options); }packages/@webex/plugin-meetings/src/member/util.ts (1)
107-121: 🛠️ Refactor suggestion
Role-based utility functions are implemented correctly.
Both utility functions correctly implement the business logic:
canBeMovedToLobby: Returns true when participant is neither a cohost nor a moderatorcanMoveToLobby: Returns true when participant is either a cohost or a moderatorThe parameter validation is consistent with other utility functions.
However, the TypeScript type for the
participantparameter is missing. This should be added for type safety.-MemberUtil.canBeMovedToLobby = (participant): boolean => { +MemberUtil.canBeMovedToLobby = (participant: ParticipantWithRoles): boolean => { if (!participant) { throw new ParameterError('Move to lobby could not be processed, participant is undefined.'); } return !MemberUtil.hasCohost(participant) && !MemberUtil.hasModerator(participant); }; -MemberUtil.canMoveToLobby = (participant): boolean => { +MemberUtil.canMoveToLobby = (participant: ParticipantWithRoles): boolean => { if (!participant) { throw new ParameterError('Move to lobby could not be processed, participant is undefined.'); } return MemberUtil.hasCohost(participant) || MemberUtil.hasModerator(participant); };📝 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.MemberUtil.canBeMovedToLobby = (participant: ParticipantWithRoles): boolean => { if (!participant) { throw new ParameterError('Move to lobby could not be processed, participant is undefined.'); } return !MemberUtil.hasCohost(participant) && !MemberUtil.hasModerator(participant); }; MemberUtil.canMoveToLobby = (participant: ParticipantWithRoles): boolean => { if (!participant) { throw new ParameterError('Move to lobby could not be processed, participant is undefined.'); } return MemberUtil.hasCohost(participant) || MemberUtil.hasModerator(participant); };packages/@webex/plugin-meetings/src/members/request.ts (1)
132-146: 🛠️ Refactor suggestion
Missing JSDoc documentation for the moveToLobbyMember method.
Unlike other methods in the class,
moveToLobbyMemberlacks proper JSDoc documentation and is using an eslint-disable comment instead. Add appropriate documentation to maintain consistency and code quality.- // eslint-disable-next-line require-jsdoc - moveToLobbyMember( + /** + * Sends a request to move a member to the lobby + * @param {Object} options + * @param {String} options.locusUrl + * @param {String} options.memberId ID of member to move to lobby + * @param {Object} body + * @param {Object} body.moveToLobby + * @param {Array<String>} body.moveToLobby.participantIds Array of participant IDs to move to lobby + * @returns {Promise} + */ + moveToLobbyMember(📝 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./** * Sends a request to move a member to the lobby * @param {Object} options * @param {String} options.locusUrl * @param {String} options.memberId ID of member to move to lobby * @param {Object} body * @param {Object} body.moveToLobby * @param {Array<String>} body.moveToLobby.participantIds Array of participant IDs to move to lobby * @returns {Promise} */ moveToLobbyMember( options: {locusUrl: string; memberId: string}, body: {moveToLobby: {participantIds: string[]}} ) { if (!options || !options.locusUrl || !options.memberId) { throw new ParameterError( 'memberId must be defined, and the associated locus url for this meeting object must be defined.' ); } const requestParams = MembersUtil.getMoveMemberToLobbyRequestParams(options, body); return this.locusDeltaRequest(requestParams); }
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
stanjiawang
left a comment
stanjiawang
left a comment
There was a problem hiding this comment.
Looks good! But need test. Great work! Thanks!
shivani1211
force-pushed
the
move-to-lobby
branch
from
83cabc1 to
9af9931
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/members/request.js (1)
425-425: Remove commented-out code.This commented-out line
// await membersRequest.lowerAllHandsMember(options);appears to be leftover from development and should be removed.- // await membersRequest.lowerAllHandsMember(options);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/@webex/plugin-meetings/src/members/index.ts(1 hunks)packages/@webex/plugin-meetings/src/members/request.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/index.js(16 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/request.js(9 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/utils.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/@webex/plugin-meetings/src/members/index.ts
- packages/@webex/plugin-meetings/src/members/request.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/@webex/plugin-meetings/test/unit/spec/members/utils.js (1)
packages/@webex/plugin-meetings/src/constants.ts (3)
PARTICIPANT(59-59)CONTROLS(22-22)HTTP_VERBS(581-587)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (13)
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (2)
796-800: New test suite looks good.The test for
canMoveToLobbyproperly validates both the positive case (when 'MOVE_TO_LOBBY' is present) and negative case (empty array), following the same pattern as other display hint utility tests.
792-793: Code formatting improvement.Good consolidation of the assertion into a single line for better readability.
packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts (2)
102-102: Properly added new property to expected values.The
canMoveToLobbyproperty is correctly initialized withnullin the expected values object, consistent with other action properties.
208-208: Correctly included new property in test cases.The
canMoveToLobbyproperty is properly added to the list of properties being tested for getter/setter behavior, ensuring complete test coverage.packages/@webex/plugin-meetings/test/unit/spec/members/utils.js (2)
361-370: Good test for getMoveMemberToLobbyRequestOptions.The test properly validates that the function returns an object with the expected structure, containing the member ID in the participantIds array.
372-392: Well-structured test for getMoveMemberToLobbyRequestParams.This test thoroughly verifies all aspects of the request parameters:
- The HTTP method is PATCH
- The URI is correctly constructed
- The request body contains the proper structure with participant IDs
The test follows the established pattern for testing request parameter utility functions.
packages/@webex/plugin-meetings/test/unit/spec/members/request.js (3)
68-69: Good refactoring of the checkRequest helper.Simplifying the assertion with the
mergefunction improves code readability and maintainability.
411-437: Well-implemented test for moveToLobbyMember.The test thoroughly verifies the request parameters and utility function calls for the new method.
97-101: Formatting improvements.The comma and whitespace fixes throughout the file improve code style consistency.
Also applies to: 406-409
packages/@webex/plugin-meetings/test/unit/spec/members/index.js (4)
987-1056: Well-implemented test suite for the new moveToLobby functionality!The test suite for the
#moveToLobbymethod follows good testing practices by:
- Testing parameter validation (rejecting when member ID or locus URL is missing)
- Verifying correct utility and request method calls
- Following the same pattern as other similar test methods in the file
This ensures the new lobby movement functionality is properly tested.
367-374: Consistent formatting improvement.The code structure for stubbing error and success cases has been improved with better indentation and consistent use of trailing commas, which enhances readability.
385-393: Added proper parameter formatting for error checking.Parameters are now properly aligned and consistently formatted, making the code more readable and maintainable.
670-683: Enhanced lowerAllHands test to support roles parameter.The
checkValidfunction and associated tests have been updated to correctly handle the optionalrolesparameter, improving test coverage for the function's capabilities.
shivani1211
force-pushed
the
move-to-lobby
branch
from
6e25f98 to
71c59b2
Compare
stanjiawang
left a comment
stanjiawang
left a comment
There was a problem hiding this comment.
LGTM! Just a small naming issue.
sreenara
left a comment
sreenara
left a comment
There was a problem hiding this comment.
@shivani1211 please update the PR description with all details along with the testing done.
There was a problem hiding this comment.
Please see the comments. Also, please add the Generative AI section into the PR description.
The GAI Coding Policy And Copyright Annotation Best Practices
- GAI was not used (or, no additional notation is required)
- Code was generated entirely by GAI
- GAI was used to create a draft that was subsequently customized or modified
- Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
- Tool used for AI assistance (GitHub Copilot / Other - specify)
- Github Copilot
- Other - Please Specify
- This PR is related to
- Feature
- Defect fix
- Tech Debt
- Automation
packages/@webex/plugin-meetings/test/unit/spec/members/request.js
Outdated
Show resolved
Hide resolved
shivani1211
force-pushed
the
move-to-lobby
branch
from
9ff01d2 to
0a3a270
Compare
4 participants
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-656461
This pull request addresses
adding move to lobby receivers side API code.
by making the following changes
added a function that will call control api to locus with correct payload when move to lobby is clicked in cantina.
Change Type
The following scenarios were tested
< 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
The GAI Coding Policy And Copyright Annotation Best Practices
on manually written code)
This PR is related to
Make sure to have followed the contributing guidelines before submitting.