feat(webinar): spark-579207 return registration info #3968
Conversation
WalkthroughThe changes introduce a new constant Changes
Possibly related PRs
Suggested reviewers
Poem
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. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (1)
21-21: Enhance error documentation with specific error code descriptions.The error class implementation looks good, but the documentation could be more helpful by describing what each error code (403021, 403022, 403024) represents.
Add JSDoc descriptions for the error codes:
+/** + * Error codes for webinar registration: + * - 403021: Registration required for this webinar + * - 403022: Registration closed for this webinar + * - 403024: Registration pending approval for this webinar + */ const WEBINAR_REGISTRATION_ERROR_CODES = [403021, 403022, 403024];Also applies to: 128-151
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js (1)
893-927: Consider enhancing test coverage and data.A few suggestions to make the tests more robust:
- Add test cases for different registration URL formats
- Include additional relevant meeting info fields that might be present in a real webinar response
- Consider testing the error handling with missing or malformed registration URLs
Example enhancement:
forEach( [ - {errorCode: 403021}, - {errorCode: 403022}, - {errorCode: 403024}, + { + errorCode: 403021, + meetingInfo: { + registrationUrl: 'https://webex.com/register/123', + topic: 'Test Webinar', + webinarType: 'regular' + } + }, + { + errorCode: 403022, + meetingInfo: { + registrationUrl: 'https://custom-domain.webex.com/register/456', + topic: 'Advanced Webinar', + webinarType: 'premium' + } + }, + { + errorCode: 403024, + meetingInfo: { + registrationUrl: null // Test missing registration URL + } + }, ],packages/@webex/plugin-meetings/src/constants.ts (1)
Line range hint
1267-1273: Consider adding JSDoc comments for better documentation.While the implementation is correct, adding JSDoc comments would improve the documentation of the
MEETING_INFO_FAILURE_REASONobject, making it easier for developers to understand each failure reason.Example improvement:
+/** + * Represents possible failure reasons when retrieving meeting information. + * @enum {string} + */ export const MEETING_INFO_FAILURE_REASON = { NONE: 'NONE', // meeting info was retrieved succesfully WRONG_PASSWORD: 'WRONG_PASSWORD', // meeting requires password and no password or wrong one was provided WRONG_CAPTCHA: 'WRONG_CAPTCHA', // wbxappapi requires a captcha code or a wrong captcha code was provided POLICY: 'POLICY', // meeting info request violates some meeting policy WEBINAR_REGISTRATION: 'WEBINAR_REGISTRATION', // webinar need registration OTHER: 'OTHER', // any other error (network, etc) };packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint
4-4: Add unit tests for the formula function.The TODO comment indicates missing test coverage. Tests are important to verify the function behavior, especially with the recent parameter changes.
Would you like me to help generate unit tests for this function?
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (5)
6254-6269: LGTM! Good test coverage for webinar registration error handling.The test case properly verifies:
- Error code (403021) is set correctly
- Error reason (WEBINAR_REGISTRATION) is set correctly
- Error propagation through PermissionError
- Meeting info is preserved in the error case
Consider adding additional test cases to verify:
- The error message content matches expectations
- The error properly bubbles up through the promise chain
- Edge cases like missing error message or code
Example:
it('should include the error message in the rejection', async () => { const errorMessage = 'Registration required for this webinar'; meeting.attrs.meetingInfoProvider = { fetchMeetingInfo: sinon .stub() .throws(new MeetingInfoV2WebinarRegistrationError(403021, FAKE_MEETING_INFO, errorMessage)) }; const error = await assert.isRejected(meeting.fetchMeetingInfo()); assert.equal(error.message, errorMessage); });
Line range hint
1-6269: Consider splitting this large test file into multiple focused test filesThe test file is quite large (>6000 lines) which can make it harder to maintain and navigate. Consider splitting it into multiple smaller test files organized by functionality, such as:
- meeting-join.test.js - Tests for join/leave functionality
- meeting-media.test.js - Tests for media handling
- meeting-info.test.js - Tests for meeting info and registration
- meeting-reactions.test.js - Tests for reactions functionality
- meeting-transcription.test.js - Tests for transcription features
This would improve:
- Code organization and maintainability
- Test file navigation
- Parallel test execution
- Focused testing of specific features
Line range hint
1-6269: Add documentation for complex test scenariosSome of the more complex test scenarios (like media negotiation and share scenarios) would benefit from better documentation explaining:
- The test setup and preconditions
- The expected behavior being tested
- Why certain assertions are important
- The real-world scenarios these tests simulate
For example, in the share scenarios tests, add comments like:
describe('Desktop A --> Desktop B', () => { /** * Tests the scenario where a user switches from web client screen share * to native client screen share. This verifies: * 1. The first share is properly released * 2. The second share is properly granted * 3. Share status is updated correctly * 4. Proper events are emitted in the correct order */ it('Scenario #1: you share desktop using web client and then share using native client', () => { // Test implementation }); });
Line range hint
1-6269: Improve error handling test assertionsThe error handling tests could be strengthened by:
- Being more specific in error assertions:
// Instead of: await assert.isRejected(meeting.someMethod()); // Use: const error = await assert.isRejected(meeting.someMethod()); assert.instanceOf(error, SpecificErrorType); assert.equal(error.code, expectedCode); assert.equal(error.message, expectedMessage);
- Testing error propagation:
it('propagates errors from dependent services', async () => { const originalError = new Error('Service error'); dependentService.stub.rejects(originalError); const error = await assert.isRejected(meeting.someMethod()); assert.equal(error.cause, originalError); });
- Verifying error cleanup:
it('cleans up resources on error', async () => { await assert.isRejected(meeting.someMethod()); assert.isNull(meeting.someResource); assert.calledOnce(cleanup.stub); });
Line range hint
1-6269: Improve test setup and cleanup to prevent memory leaksSome areas for improvement in test setup and cleanup:
- Ensure timers are always cleaned up:
describe('timer tests', () => { let clock; beforeEach(() => { clock = sinon.useFakeTimers(); }); afterEach(() => { // Always restore timers even if test fails if (clock) { clock.restore(); clock = null; } }); });
- Clean up event listeners:
describe('event tests', () => { const listeners = new Set(); beforeEach(() => { // Track added listeners sinon.stub(emitter, 'on').callsFake((event, listener) => { listeners.add({event, listener}); }); }); afterEach(() => { // Remove all listeners listeners.forEach(({event, listener}) => { emitter.off(event, listener); }); listeners.clear(); }); });
- Simplify common setup:
// Create helper for common setup const setupMeetingTest = () => { const meeting = new Meeting(...); // Common setup code return meeting; }; it('test case', () => { const meeting = setupMeetingTest(); // Test specific setup });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/@webex/plugin-meetings/src/constants.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts(5 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/metrics/constants.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
🔇 Additional comments (7)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (3)
206-227: LGTM! Error handling implementation is solid.
The implementation follows established patterns and includes proper metric tracking before throwing errors.
444-444: LGTM! Proper error handling sequence.
The webinar registration error check is correctly placed within the 403 status code block, maintaining proper error handling hierarchy.
131-135: Verify complete implementation of registration info return.
While the error handling is implemented well, let's verify that the registration information is properly returned in the successful case. The error class includes meetingInfo, but we should ensure this contains all necessary registration details.
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js (1)
893-927: LGTM! Well-structured test suite for webinar registration errors.
The test suite thoroughly covers the webinar registration error scenarios with proper assertions for:
- Error instance type
- Error message format
- Error code propagation
- Meeting info data structure
- Behavioral metrics tracking
packages/@webex/plugin-meetings/src/constants.ts (1)
1272-1272: LGTM! The constant addition aligns with the PR objectives.
The new constant WEBINAR_REGISTRATION is:
- Correctly positioned within the
MEETING_INFO_FAILURE_REASONobject - Follows the existing naming pattern
- Properly implements the required functionality for webinar registration error handling
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
Line range hint 1-2: LGTM!
The subtraction function is correctly implemented.
Line range hint 5-6: LGTM but needs test coverage.
The implementation is correct but should be covered by tests to verify:
- Basic arithmetic operations
- Edge cases (large numbers, negative numbers)
- Order of operations
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts(5 hunks)
🔇 Additional comments (4)
packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (4)
21-21: LGTM: Error codes constant follows existing patterns
The constant WEBINAR_REGISTRATION_ERROR_CODES is well-defined and follows the established pattern of error code definitions in the file.
206-227: LGTM: Error handler implementation is robust
The handleWebinarRegistrationError method is well-implemented with proper null checks, metric tracking, and error throwing logic.
289-289: Fix error handling sequence in createAdhocSpaceMeeting
The webinar registration error might be swallowed by the subsequent MeetingInfoV2AdhocMeetingError throw.
444-444: Verify error handling sequence in fetchMeetingInfo
The error handling sequence needs verification to ensure all error types are properly caught and handled.
| /** | ||
| * Error preventing join because of a webinar registration error | ||
| */ | ||
| export class MeetingInfoV2WebinarRegistrationError extends Error { | ||
| meetingInfo: any; | ||
| sdkMessage: any; | ||
| wbxAppApiCode: any; | ||
| body: any; | ||
| /** | ||
| * | ||
| * @constructor | ||
| * @param {Number} [wbxAppApiErrorCode] | ||
| * @param {Object} [meetingInfo] | ||
| * @param {String} [message] | ||
| */ | ||
| constructor(wbxAppApiErrorCode?: number, meetingInfo?: object, message?: string) { | ||
| super(`${message}, code=${wbxAppApiErrorCode}`); | ||
| this.name = 'MeetingInfoV2AdhocMeetingError'; | ||
| this.sdkMessage = message; | ||
| this.stack = new Error().stack; | ||
| this.wbxAppApiCode = wbxAppApiErrorCode; | ||
| this.meetingInfo = meetingInfo; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix error class name and improve type safety
The error class implementation has a few issues that need attention:
- The error name in the constructor is incorrect (shows "MeetingInfoV2AdhocMeetingError")
- Properties are loosely typed with 'any'
Apply these improvements:
export class MeetingInfoV2WebinarRegistrationError extends Error {
- meetingInfo: any;
- sdkMessage: any;
- wbxAppApiCode: any;
- body: any;
+ meetingInfo: object | undefined;
+ sdkMessage: string | undefined;
+ wbxAppApiCode: number | undefined;
+ body: object | undefined;
constructor(wbxAppApiErrorCode?: number, meetingInfo?: object, message?: string) {
super(`${message}, code=${wbxAppApiErrorCode}`);
- this.name = 'MeetingInfoV2AdhocMeetingError';
+ this.name = 'MeetingInfoV2WebinarRegistrationError';
this.sdkMessage = message;
this.stack = new Error().stack;
this.wbxAppApiCode = wbxAppApiErrorCode;
this.meetingInfo = meetingInfo;
}
}📝 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.
| /** | |
| * Error preventing join because of a webinar registration error | |
| */ | |
| export class MeetingInfoV2WebinarRegistrationError extends Error { | |
| meetingInfo: any; | |
| sdkMessage: any; | |
| wbxAppApiCode: any; | |
| body: any; | |
| /** | |
| * | |
| * @constructor | |
| * @param {Number} [wbxAppApiErrorCode] | |
| * @param {Object} [meetingInfo] | |
| * @param {String} [message] | |
| */ | |
| constructor(wbxAppApiErrorCode?: number, meetingInfo?: object, message?: string) { | |
| super(`${message}, code=${wbxAppApiErrorCode}`); | |
| this.name = 'MeetingInfoV2AdhocMeetingError'; | |
| this.sdkMessage = message; | |
| this.stack = new Error().stack; | |
| this.wbxAppApiCode = wbxAppApiErrorCode; | |
| this.meetingInfo = meetingInfo; | |
| } | |
| } | |
| /** | |
| * Error preventing join because of a webinar registration error | |
| */ | |
| export class MeetingInfoV2WebinarRegistrationError extends Error { | |
| meetingInfo: object | undefined; | |
| sdkMessage: string | undefined; | |
| wbxAppApiCode: number | undefined; | |
| body: object | undefined; | |
| /** | |
| * | |
| * @constructor | |
| * @param {Number} [wbxAppApiErrorCode] | |
| * @param {Object} [meetingInfo] | |
| * @param {String} [message] | |
| */ | |
| constructor(wbxAppApiErrorCode?: number, meetingInfo?: object, message?: string) { | |
| super(`${message}, code=${wbxAppApiErrorCode}`); | |
| this.name = 'MeetingInfoV2WebinarRegistrationError'; | |
| this.sdkMessage = message; | |
| this.stack = new Error().stack; | |
| this.wbxAppApiCode = wbxAppApiErrorCode; | |
| this.meetingInfo = meetingInfo; | |
| } | |
| } |
| */ | ||
| constructor(wbxAppApiErrorCode?: number, meetingInfo?: object, message?: string) { | ||
| super(`${message}, code=${wbxAppApiErrorCode}`); | ||
| this.name = 'MeetingInfoV2AdhocMeetingError'; |
There was a problem hiding this comment.
the name is incorrect
|
|
||
| if (err.meetingInfo) { | ||
| this.meetingInfo = err.meetingInfo; | ||
| } | ||
| throw new PermissionError(); |
There was a problem hiding this comment.
just to double check: we're still throwing the same PermissionError here as for MeetingInfoV2PolicyError - is this correct?
COMPLETES #SPARK-579207
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
Return webinar registration info when call fetchMeetingInfo
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests