feat: added enable static meeting link endpoint #4195
Conversation
📝 WalkthroughWalkthroughThis pull request introduces two new error classes to handle specific error cases encountered when enabling static meeting links. The existing meeting creation method is refactored into a new method that supports both adhoc meeting creation and static link enabling based on an additional boolean parameter. A dedicated public method is added for enabling static meeting links, which includes specific error handling logic that throws one of the new error classes depending on the error encountered. Additionally, new behavioral metric constants are defined to track the success or failure of enabling static meeting links and related error conditions. The corresponding unit tests have been updated and expanded to cover the new functionality and error scenarios, ensuring both the meeting creation and static link enabling processes are properly validated. 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.31.1)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (20)
✨ Finishing Touches
🪧 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
|
![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
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
51-54: Typo in constant names ENABLE_STATIC_METTING_LINK_*.There appears to be a typo in the constant names - "METTING" should be "MEETING".
- ENABLE_STATIC_METTING_LINK_SUCCESS: 'js_sdk_enable_static_meeting_link_success', - ENABLE_STATIC_METTING_LINK_FAILURE: 'js_sdk_enable_static_meeting_link_failure', + ENABLE_STATIC_MEETING_LINK_SUCCESS: 'js_sdk_enable_static_meeting_link_success', + ENABLE_STATIC_MEETING_LINK_FAILURE: 'js_sdk_enable_static_meeting_link_failure',packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (2)
451-458: Consider using common error handlers for consistencyWhile this implementation works correctly, consider leveraging the existing error handlers (
handlePolicyError,handleJoinWebinarError,handleForbiddenError) for consistency across the codebase..catch((err) => { if (err?.statusCode === 403) { + this.handlePolicyError(err); + this.handleJoinWebinarError(err); + this.handleForbiddenError(err); + Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.MEETING_IS_IN_PROGRESS_ERROR, { reason: err.message, stack: err.stack, }); throw new MeetingInfoV2MeetingIsInProgressError(err.body?.code, err.body?.message); }
460-470: Consider making error handling more extensibleThe current error handling implementation uses if-else statements for each status code. As more error types are added, this could become harder to maintain.
Consider using a map-based approach for error handling:
const errorHandlers = { Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.MEETING_IS_IN_PROGRESS_ERROR, { reason: err.message, stack: err.stack, }); throw new MeetingInfoV2MeetingIsInProgressError(err.body?.code, err.body?.message); }, Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.STATIC_MEETING_LINK_ALREADY_EXISTS_ERROR, { reason: err.message, stack: err.stack, }); throw new MeetingInfoV2StaticMeetingLinkAlreadyExists(err.body?.code, err.body?.message); } }; // Usage const handler = errorHandlers[err?.statusCode]; if (handler) { handler(err); } else { // Default error handling }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts(6 hunks)packages/@webex/plugin-meetings/src/meetings/index.ts(1 hunks)packages/@webex/plugin-meetings/src/metrics/constants.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js(21 hunks)packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/@webex/plugin-meetings/src/meetings/index.ts (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Meeting(540-9450)packages/@webex/plugin-meetings/src/index.ts (1)
Meeting(72-72)
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js (2)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (3)
MeetingInfoV2MeetingIsInProgressError(201-218)MeetingInfoV2StaticMeetingLinkAlreadyExists(223-240)MeetingInfoV2JoinForbiddenError(176-196)packages/@webex/plugin-meetings/src/constants.ts (2)
DESTINATION_TYPE(1400-1409)DESTINATION_TYPE(1411-1411)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (23)
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
1372-1394: Good implementation of the enableStaticMeetingLink methodThe implementation is clean, follows existing code patterns, and has appropriate error handling. This method correctly delegates the functionality to the meetingInfo object.
packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (1)
935-955: Well-structured tests for the new enableStaticMeetingLink methodThe test coverage is good and verifies both the existence of the method and that it correctly calls the meetingInfo.enableStaticMeetingLink method with the appropriate parameters.
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js (14)
10-10: Added necessary constant importsThis line adds imports for
DESTINATION_TYPEandWBXAPPAPI_SERVICEconstants that will be used in the new tests.
20-21: Added new error class importsThese new error class imports are correctly added to support the new tests for enabling static meeting links.
95-110: Well-structured setup function for test dataThe setup function properly creates an array of invitees from the conversation participants data, which will be reused across multiple tests.
112-152: Test case validates successful static meeting link enablingThis test successfully validates that:
- The correct API requests are made with the expected parameters
- The appropriate success metric is sent
- The expected result is returned with the conversation data
The test verifies that
schedule: trueis passed to enable static meeting link functionality.
154-170: Test case handles missing preferred webex site correctlyThis test correctly verifies that the method throws an appropriate error when the preferred webex site is undefined, preventing unnecessary API requests.
172-189: Test case properly validates meeting in progress error handlingThis test effectively verifies that:
- The correct error is thrown when a 403 status code with error code 33003 is received
- The appropriate error metrics are sent
- The error message is correctly constructed
The test ensures proper error handling for when a meeting is already in progress.
191-208: Test case properly validates static meeting link already exists error handlingThis test effectively verifies that:
- The correct error is thrown when a 409 status code with error code 409000 is received
- The appropriate error metrics are sent
- The error message is correctly constructed
The test ensures proper error handling for when a static meeting link already exists.
210-225: Test case properly validates generic error handlingThis test ensures that for unhandled error types, the appropriate failure metric is sent, which is important for tracking and debugging purposes.
286-291: Reformatted test case for better readabilityThe existing test cases for
fetchMeetingInfohave been reformatted for better readability and consistency, without changing functionality.Also applies to: 296-299
958-959: Minor formatting update to return statementReformatted the return statement in the
setupfunction to maintain consistent code style.
969-976: Assertion updated for createAdhocSpaceMeeting to verify correct API callsThis assertion has been properly updated to ensure the method makes the expected API requests.
987-988: Added schedule parameter to createAdhocSpaceMeetingThe
schedule: falseparameter has been added to ensure that the refactored method correctly distinguishes between adhoc meetings and static meeting links.
994-995: Updated expected result assertionThe assertion has been updated to include the expected statusCode in the result.
1104-1132: Test case for MeetingInfoV2JoinForbiddenErrorThis test properly verifies the error handling for JOIN_FORBIDDEN_CODES, ensuring that the correct error type is thrown with the appropriate message and metrics.
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (7)
19-20: Added new error message constantsWell-defined constants for the error messages that will be used by the new error classes.
198-218: New error class for meeting in progress scenariosThis class properly extends Error and follows the pattern of other error classes in the file, with appropriate properties and constructor. It will be used when attempting to enable a static meeting link for a conversation that has an active meeting.
220-240: New error class for existing static meeting link scenariosThis class properly extends Error and follows the pattern of other error classes in the file, with appropriate properties and constructor. It will be used when attempting to enable a static meeting link that already exists.
341-355: Refactored method to support both adhoc meetings and static meeting linksThe method has been well refactored to handle both use cases by adding a boolean parameter
enableStaticMeetingLink. The parameter name is clear and the default value maintains backward compatibility.The inline comment explaining the parameter purpose is helpful.
381-382: Key change to support static meeting linksThis line sets the
scheduleproperty to the value ofenableStaticMeetingLink, which determines whether to create an adhoc meeting (false) or enable a static meeting link (true).
408-430: Updated createAdhocSpaceMeeting to use refactored methodThe original method now properly checks for the preferred webex site and delegates to the refactored method with the appropriate parameter value (false) to maintain backward compatibility.
432-479: New method to enable static meeting linksThe new method provides a public API for enabling static meeting links. It includes:
- Verification of the preferred webex site
- Calling the refactored method with the appropriate parameter value (true)
- Sending appropriate metrics for success/failure
- Specific error handling for 403 (meeting in progress) and 409 (static meeting link already exists) status codes
The error handling approach is straightforward and matches the implementation in the test file.
|
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.
Pulled the code and tested it locally, it works fine for me.
4 participants
COMPLETES #< https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-645973 >
This pull request addresses
Users are not able to enable static meeting links for a conversation currently
by making the following changes
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
Make sure to have followed the contributing guidelines before submitting.