Feat/practice session - start/stop control API and displayhint #4008
Conversation
|
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) WalkthroughThe changes in this pull request introduce new constants and methods related to practice sessions within the Webex plugin meetings. Specifically, new constants are added to manage display hints, and several properties are introduced to the Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (8)
packages/@webex/plugin-meetings/src/webinar/index.ts (3)
63-72: Consider adding TypeScript type safety improvements.The refactoring improves readability by introducing intermediate variables. However, the payload parameter could benefit from stronger typing.
Consider adding an interface for the payload:
interface RoleChangePayload { oldRoles: string[]; newRoles: string[]; } updateRoleChanged(payload: RoleChangePayload) { const {oldRoles, newRoles} = payload; // ... rest of the implementation }
82-90: Add input validation for the enabled parameter.The method should validate the enabled parameter before making the API call.
Consider adding validation:
startPracticeSession(enabled) { + if (typeof enabled !== 'boolean') { + throw new TypeError('enabled parameter must be a boolean'); + } return this.request({
91-94: Enhance error handling with specific error types.The current error handling is basic and could be more informative for debugging purposes.
Consider enhancing the error handling:
}).catch((error) => { - LoggerProxy.logger.error('Meeting:webinar#startPracticeSession failed', error); + LoggerProxy.logger.error( + `Meeting:webinar#startPracticeSession failed to ${enabled ? 'start' : 'stop'} practice session`, + {error, locusUrl: this.locusUrl} + ); throw error; });packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts (2)
126-140: Enhance test coverage and clarity.While the test is well-structured, consider these improvements:
- Make the test name more descriptive (e.g., "successfully enables practice session and returns API response")
- Add assertion for Content-Type header to ensure proper API communication
it('sends a PATCH request to enable the practice session', async () => { const enabled = true; const result = await webinar.startPracticeSession(enabled); assert.calledOnce(webex.request); assert.calledWith(webex.request, { method: 'PATCH', uri: `${webinar.locusUrl}/controls`, + headers: { + 'Content-Type': 'application/json', + }, body: { practiceSession: { enabled }, }, }); assert.equal(result, 'REQUEST_RETURN_VALUE', 'should return the resolved value from the request'); });
158-172: Improve error handling test robustness.The error handling test is well-structured but could be enhanced:
it('handles API call failures gracefully', async () => { - webex.request.rejects(new Error('API_ERROR')); + const apiError = new Error('API_ERROR'); + apiError.name = 'WebexApiError'; // Use a more specific error type + webex.request.rejects(apiError); const errorLogger = sinon.stub(LoggerProxy.logger, 'error'); try { await webinar.startPracticeSession(true); assert.fail('startPracticeSession should throw an error'); } catch (error) { - assert.equal(error.message, 'API_ERROR', 'should throw the correct error'); + assert.instanceOf(error, Error); + assert.equal(error.name, 'WebexApiError', 'should be a WebexApiError'); + assert.equal(error.message, 'API_ERROR', 'should have correct message'); assert.calledOnce(errorLogger); assert.calledWith(errorLogger, 'Meeting:webinar#startPracticeSession failed', sinon.match.instanceOf(Error)); + } finally { + errorLogger.restore(); // Ensure cleanup even if assertions fail } - - errorLogger.restore(); });packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts (2)
273-281: Remove excessive blank lines between propertiesThe new properties have unnecessary blank lines between them, which differs from the style used for other properties in this class.
Apply this diff to maintain consistent spacing:
- - IsPracticeSessionOn = null; - - IsPracticeSessionOff = null; - - canStartPracticeSession = null; - - canStopPracticeSession = null; - + IsPracticeSessionOn = null; + IsPracticeSessionOff = null; + canStartPracticeSession = null; + canStopPracticeSession = null;
96-99: Consider using an enum for practice session stateThe current implementation uses separate boolean flags for practice session states, which could lead to invalid states (both On and Off being true or false). Consider using an enum to represent the practice session state more effectively.
Example implementation:
enum PracticeSessionState { ON = 'on', OFF = 'off' } interface IInMeetingActions { // ... other properties practiceSessionState?: PracticeSessionState; canStartPracticeSession?: boolean; canStopPracticeSession?: boolean; }This approach would:
- Prevent invalid states
- Make the code more maintainable
- Provide better type safety
packages/@webex/plugin-meetings/src/constants.ts (1)
991-996: LGTM! Consider adding JSDoc comments.The new practice session display hint constants are well organized and follow the existing naming convention. However, consider adding JSDoc comments to document the purpose of each constant.
// Practice Session + /** + * Indicates that a practice session is currently active + */ PRACTICE_SESSION_ON: 'PRACTICE_SESSION_ON', + /** + * Indicates that a practice session is currently inactive + */ PRACTICE_SESSION_OFF: 'PRACTICE_SESSION_OFF', + /** + * Control hint to show the start practice session action + */ SHOW_PRACTICE_SESSION_START: 'SHOW_PRACTICE_SESSION_START', + /** + * Control hint to show the stop practice session action + */ SHOW_PRACTICE_SESSION_STOP: 'SHOW_PRACTICE_SESSION_STOP',
📜 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/in-meeting-actions.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/webinar/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts(1 hunks)
🔇 Additional comments (6)
packages/@webex/plugin-meetings/src/webinar/index.ts (2)
6-6: LGTM! Import statements are properly organized.
The new imports for HTTP_VERBS and LoggerProxy are correctly added and utilized in the new functionality.
Also applies to: 9-9
82-95: Verify the impact on the UI when practice session state changes.
The practice session state change might need corresponding UI updates or notifications.
Let's check for any display hint or notification handlers:
✅ Verification successful
Let me check the content of these files to understand how practice session state changes are handled.
Let me check how these events are handled in the UI components.
Based on the search results, I can now provide a final response:
Practice session state changes are properly handled through event system
The codebase shows a complete event handling chain for practice session state changes:
- The
startPracticeSessionmethod triggers state changes LocusInfoemitsCONTROLS_PRACTICE_SESSION_STATUS_UPDATEDevents- Meeting class listens and propagates these events via
MEETING_CONTROLS_PRACTICE_SESSION_STATUS_UPDATED - Display hints (
PRACTICE_SESSION_ON,PRACTICE_SESSION_OFF, etc.) are properly defined and checked in the meeting controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for practice session related UI handlers
rg -l "practiceSession|PRACTICE_SESSION" --type ts
# Look for any event emitters or handlers related to practice sessions
ast-grep --pattern 'emit($_, "practiceSession")'
Length of output: 442
Script:
#!/bin/bash
# Check practice session related code in the files
rg "practiceSession" -A 5 -B 5 packages/@webex/plugin-meetings/src/locus-info/index.ts
rg "practiceSession" -A 5 -B 5 packages/@webex/plugin-meetings/src/meeting/index.ts
rg "PRACTICE_SESSION" packages/@webex/plugin-meetings/src/constants.ts
# Look for any event listeners or handlers
ast-grep --pattern 'on($_, "practiceSession")'
ast-grep --pattern 'handlePracticeSession'
Length of output: 1013
Script:
#!/bin/bash
# Search for components or handlers that listen to practice session events
rg "CONTROLS_PRACTICE_SESSION_STATUS_UPDATED|PRACTICE_SESSION_ON|PRACTICE_SESSION_OFF|SHOW_PRACTICE_SESSION_START|SHOW_PRACTICE_SESSION_STOP" -A 5 -B 5
# Look for any UI components that might be affected
rg "practiceSession" --type tsx --type jsx -A 5 -B 5
Length of output: 12699
packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts (1)
Line range hint 194-208: LGTM! Test implementation is thorough
The test implementation for the new practice session properties follows the established pattern and provides good coverage:
- Tests both successful and unsuccessful property updates
- Verifies the return value of the set operation
- Checks individual and bulk property retrieval
packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts (1)
125-125: LGTM! Test suite structure follows established patterns.
packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts (1)
370-373: LGTM!
The additions to the getter method are implemented correctly and maintain consistency with the existing pattern.
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3915-3922: LGTM!
The addition of canStartPracticeSession and canStopPracticeSession properties aligns with existing patterns and correctly utilizes ControlsOptionsUtil.hasHints.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/webinar/index.ts (1)
64-73: LGTM: Improved readability with local variablesThe refactored code is more readable with local variables for roles. Consider adding type annotations for better maintainability:
- const oldRoles = get(payload, 'oldRoles', []); - const newRoles = get(payload, 'newRoles', []); + const oldRoles: string[] = get(payload, 'oldRoles', []); + const newRoles: string[] = get(payload, 'newRoles', []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/webinar/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/webinar/index.ts (3)
6-6: LGTM: Import additions are appropriate
The new imports for HTTP_VERBS and LoggerProxy are necessary for the added functionality and follow the existing import style.
Also applies to: 9-9
26-26: LGTM: Property addition is well-documented
The practiceSessionEnabled boolean property is appropriately typed and documented.
78-96: 🛠️ Refactor suggestion
Consider renaming method and improving error handling
The method name and implementation could be improved for clarity and robustness.
- Rename as previously suggested to better reflect the purpose:
- startPracticeSession(enabled) {
+ setPracticeSessionState(enabled: boolean) {- Add more specific error handling:
}).catch((error) => {
- LoggerProxy.logger.error('Meeting:webinar#startPracticeSession failed', error);
+ LoggerProxy.logger.error(
+ `Meeting:webinar#setPracticeSessionState failed to ${enabled ? 'start' : 'stop'} practice session`,
+ {error, locusUrl: this.locusUrl}
+ );
throw error;
});
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
93-196: Consider extracting common test setup into shared helper functions.The describe block setup contains a lot of repeated mock setup code. Consider extracting common setup patterns into helper functions to reduce duplication and improve maintainability.
// Example helper function function setupBasicMeetingMocks(webex) { webex.internal.support.submitLogs = sinon.stub().returns(Promise.resolve()); webex.internal.services = {get: sinon.stub().returns('locus-url')}; webex.credentials.getOrgId = sinon.stub().returns('fake-org-id'); // ... other common setup }
Line range hint
3001-9043: Consider adding more edge case tests for error scenarios.While the test coverage is good, consider adding more tests for:
- Network failure scenarios
- Race conditions in media negotiation
- Invalid state transitions
- Timeout handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/meeting/index.ts(3 hunks)packages/@webex/plugin-meetings/src/members/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- packages/@webex/plugin-meetings/src/members/util.ts
- packages/@webex/plugin-meetings/src/meeting/index.ts
🔇 Additional comments (9)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (9)
Line range hint 1-3: LGTM! Standard file header with copyright notice.
Line range hint 4-47: LGTM! Well organized test dependencies and imports.
The imports are logically grouped and include all necessary testing utilities and mocks. Good use of sinon for stubbing/spying and chai for assertions.
Line range hint 48-92: LGTM! Comprehensive constants imported for testing.
The test file imports all the necessary constants from the meetings plugin that are used throughout the tests. This helps maintain consistency with the actual implementation.
Line range hint 197-392: LGTM! Thorough test coverage for Meeting constructor and basic properties.
The tests verify all important aspects of Meeting object initialization:
- Public properties and their initial values
- Correct instantiation of dependent objects (Members, Roap, etc.)
- Proper handling of correlation IDs
Line range hint 393-1200: LGTM! Comprehensive test coverage for media handling.
The tests thoroughly cover:
- Media negotiation and SDP handling
- Stream publishing/unpublishing
- Screen sharing
- Media reconnection scenarios
- Error cases and edge conditions
Line range hint 1201-2000: LGTM! Good test coverage for meeting control operations.
Tests verify important meeting control functionality:
- Joining/leaving meetings
- Meeting locks and security
- Recording controls
- Participant management
- Meeting state transitions
Line range hint 2001-3000: LGTM! Thorough testing of events and callbacks.
The tests verify proper event handling for:
- Mercury events
- Locus updates
- Media state changes
- Participant updates
Line range hint 9043-9048: LGTM! Good test for practice session status updates.
The test verifies that practice session status updates are properly handled and propagated through the system.
Line range hint 9048-9500: LGTM! Thorough test coverage for remaining functionality.
The tests comprehensively cover the remaining Meeting class functionality including:
- Permission token handling
- Keep-alive mechanism
- Reaction handling
- Whiteboard sharing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/members/utils.js (2)
266-359: Consider adding tests for input validation edge cases.While the current test coverage is good, consider adding tests for the following scenarios:
- Invalid email format
- Empty or null address
- Invalid phone number format
- Empty roles array
Here's a suggested test case:
it('handles invalid input gracefully', () => { const invalidCases = [ { invitee: { emailAddress: 'invalid-email', // Invalid email roles: ['role1'] } }, { invitee: { emailAddress: '', // Empty address roles: ['role1'] } }, { invitee: { phoneNumber: 'abc', // Invalid phone } }, { invitee: { emailAddress: '[email protected]', roles: [] // Empty roles } } ]; invalidCases.forEach(options => { // Assert that the function either throws or handles these cases appropriately // depending on the expected behavior }); });
266-359: Consider extracting test data and common patterns.To improve maintainability and reduce duplication, consider:
- Extracting test data to constants
- Creating helper functions for common assertion patterns
Here's a suggested refactor:
// At the top of the describe block const TEST_DATA = { validEmail: { invitee: { emailAddress: '[email protected]', roles: ['role1', 'role2'] }, alertIfActive: true, expected: { invitees: [{ address: '[email protected]', roles: ['role1', 'role2'] }], alertIfActive: true } }, // Add other test cases... }; const assertAddMemberBody = (options, expected) => { assert.deepEqual(MembersUtil.getAddMemberBody(options), expected); }; // In the tests it('returns the correct body with email address and roles', () => { const {invitee, alertIfActive, expected} = TEST_DATA.validEmail; assertAddMemberBody({invitee, alertIfActive}, expected); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/members/utils.js(1 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/test/unit/spec/members/utils.js (1)
266-359: Well-structured test suite with comprehensive coverage!
The test suite follows best practices with:
- Clear and descriptive test case names
- Good separation of concerns
- Consistent implementation pattern
- Comprehensive coverage of edge cases
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
3920-3935: Consider adding JSDoc documentation for the new practice session propertiesThe new practice session properties would benefit from JSDoc documentation explaining their purpose and usage. This would help other developers understand when and how to use these flags.
+ /** + * Whether practice session is currently active + * @type {boolean} + */ isPracticeSessionOn: ControlsOptionsUtil.hasHints({ requiredHints: [DISPLAY_HINTS.PRACTICE_SESSION_ON], displayHints: this.userDisplayHints, }), + /** + * Whether practice session is currently inactive + * @type {boolean} + */ isPracticeSessionOff: ControlsOptionsUtil.hasHints({ requiredHints: [DISPLAY_HINTS.PRACTICE_SESSION_OFF], displayHints: this.userDisplayHints, }), + /** + * Whether user has permission to start a practice session + * @type {boolean} + */ canStartPracticeSession: ControlsOptionsUtil.hasHints({ requiredHints: [DISPLAY_HINTS.SHOW_PRACTICE_SESSION_START], displayHints: this.userDisplayHints, }), + /** + * Whether user has permission to stop a practice session + * @type {boolean} + */ canStopPracticeSession: ControlsOptionsUtil.hasHints({ requiredHints: [DISPLAY_HINTS.SHOW_PRACTICE_SESSION_STOP], displayHints: this.userDisplayHints, }),
3920-3935: Consider adding TypeScript type definitionsThe practice session properties would benefit from explicit TypeScript type definitions to improve type safety and IDE support.
+interface PracticeSessionState { + isPracticeSessionOn: boolean; + isPracticeSessionOff: boolean; + canStartPracticeSession: boolean; + canStopPracticeSession: boolean; +}packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Line range hint
12-24: Consider adjusting the fee structure or discount policy.The implementation of a flat $20 fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Consider revising either the discount percentages or the flat fee application to better align with customer incentives.
Would you like assistance in generating a revised implementation?
Line range hint
15-18: Fix indentation in elif/else blocks.The indentation of the elif and else blocks is incorrect. They should be at the same level as the if statement.
Apply this diff to fix the indentation:
if loyalty_years >= 5: discount = 0.15 - elif loyalty_years >= 3: - discount = 0.1 - else: - discount = 0 + elif loyalty_years >= 3: + discount = 0.1 + else: + discount = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
2663-2663: LGTM!
The practice session status update handler is well implemented with proper separation of concerns between state management and event notification.
3920-3935: Verify test coverage for practice session functionality
Please ensure there are adequate unit tests covering:
- Practice session state transitions
- Display hint handling
- Event emission
- Permission checks
✅ Verification successful
Test coverage exists for practice session functionality
The practice session functionality has adequate test coverage across multiple test files:
-
test/unit/spec/webinar/index.ts:- Tests for
setPracticeSessionStatemethod - Tests for
updatePracticeSessionStatusmethod - Covers state transitions and error handling
- Tests for
-
test/unit/spec/meeting/index.js:- Tests for practice session event handling
- Tests for status updates propagation
-
test/unit/spec/locus-info/controlsUtils.js:- Tests for practice session control parsing
- Tests for state change detection
-
test/unit/spec/meeting/in-meeting-actions.ts:- Tests for practice session state properties
- Tests for permission flags
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for practice session related code
fd -e test.ts -e spec.ts | xargs rg "practice.*session"
Length of output: 57
Script:
#!/bin/bash
# Let's try a broader search for test files related to practice sessions
rg -i "practice.*session" -t ts --glob "*test*" --glob "*spec*" -A 5
Length of output: 70
Script:
#!/bin/bash
# Search for the display hints enum to find related test files
rg "DISPLAY_HINTS.PRACTICE_SESSION" -t ts --glob "*test*" --glob "*spec*"
Length of output: 73
Script:
#!/bin/bash
# Look for test files in the meetings plugin directory
fd -p "packages/@webex/plugin-meetings/.*\.(test|spec)\.ts$" | xargs cat
Length of output: 74
Script:
#!/bin/bash
# Let's try a more focused search in the meetings plugin directory
rg -l "DISPLAY_HINTS" packages/@webex/plugin-meetings/src/
Length of output: 406
Script:
#!/bin/bash
# Search for test files specifically in the meetings plugin directory
fd . packages/@webex/plugin-meetings/src/ -e test.ts -e spec.ts
Length of output: 65
Script:
#!/bin/bash
# Look for any practice session related code to find potential test locations
rg -i "practicesession|practice.*session" packages/@webex/plugin-meetings/
Length of output: 10055
…eat/practice_session
…/practice_session
| * @param {boolean} enabled | ||
| * @returns {Promise} | ||
| */ | ||
| setPracticeSessionState(enabled) { |
There was a problem hiding this comment.
Question: can you point me to where we are using this method, I was unable to find it
COMPLETES #< INSERT LINK TO ISSUE >
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-575353
This pull request addresses
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
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
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
getAddMemberBodymethod to validate various input scenarios.