fix(plugin-meetings): init locusInfo before join.response event #3914
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
|
||
| const parseLocusJoinSpy = sinon.stub(MeetingUtil, 'parseLocusJoin'); | ||
| it('#Should call `meetingRequest.joinMeeting', async () => { |
There was a problem hiding this comment.
Only this test was changed so locusInfo is tested.
Because locusInfo is now required for joinMeeting to work with tests, meeting object was moved one level (to beforeEach) to remove unnecessary duplication
There was a problem hiding this comment.
Just got a quick question about the tests here, why are we having a # here?
There was a problem hiding this comment.
looks like the other existing tests nearby also already have # at the start of the title, they're all unnecessary. From what I've seen, the SDK tests usually have a convention to add a # at the beginning of method name when creating a describe for tests for such method, so in this case we should have describe('#joinMeeting'), () => { in line 380 and not have any # in the it() calls
| // @ts-ignore | ||
| const parsed = MeetingUtil.parseLocusJoin(res); | ||
|
|
||
| meeting.locusInfo.initialSetup(parsed.locus); |
There was a problem hiding this comment.
I'm not sure if we need to do a second initial setup if it's already been done. Inside Locus info, there are a couple of interesting functions that sound like something that could be useful here, like updateLocusInfo and onDeltaLocus. Have you tried using them?
There was a problem hiding this comment.
initialSetup is called when meeting is created in meetings.create function. Here meeting is already created and locusInfo is still not initialized (initialSetup was not called yet) so I would say it's good that we are calling this.
From what I can see, updateLocusInfo and onDeltaLocus are used more like internal updates and initialSetup as external update.
But still I would leave the decision to someone from SDK team if we should call here initialSetup or updateLocusInfo
There was a problem hiding this comment.
As we discussed, I want to double-check if these two logics are not repeating each other. It seems that we are making two calls to initialSetup for locusInfo in the join meeting flow. If this is the case, it would be better to consolidate it into one.
| trackingid: 'trackingId', | ||
| }, | ||
| } | ||
| let meeting |
There was a problem hiding this comment.
I am not sure if syntaxys here is right. Should be ";" at the end like in other test cases?
I feel that some js files in SDK have an issue with checking this, but still, I believe we need to try to align this
|
|
||
| const parseLocusJoinSpy = sinon.stub(MeetingUtil, 'parseLocusJoin'); | ||
| it('#Should call `meetingRequest.joinMeeting', async () => { |
There was a problem hiding this comment.
Just got a quick question about the tests here, why are we having a # here?
| @@ -313,34 +315,25 @@ const MeetingUtil = { | |||
| } | |||
|
|
|||
| // normal join meeting, scenario A, D | |||
| return MeetingUtil.joinMeeting(meeting, options) | |||
| .then((response) => { | |||
There was a problem hiding this comment.
Removed then block
SomeBody16
force-pushed
the
SPARK-570035-parse_locus_before_join
branch
from
8d1dbf5
to
1450437
Compare
| sinon.stub(MeetingUtil, 'joinMeeting').callsFake(async (meeting) => { | ||
| const result = { | ||
| id: 'fake locus from mocked join request', | ||
| locusUrl: 'fake locus url', | ||
| mediaId: 'fake media id', | ||
| }; | ||
| meeting.setLocus(result) | ||
| return result; |
There was a problem hiding this comment.
Because we are removing then block after joinMeeting we need to call setLocus manually in tests
| locusUrl: 'fake locus url', | ||
| mediaId: 'fake media id', | ||
| }; | ||
| meeting.setLocus(result) |
There was a problem hiding this comment.
this is getting a bit messy... maybe we could instead of stubbing MeetingUtil.joinMeeting() just stub meeting.meetingRequest.joinMeeting()?
SomeBody16
force-pushed
the
SPARK-570035-parse_locus_before_join
branch
from
368512b
to
a312e2a
Compare
WalkthroughThe changes in this pull request focus on enhancing the functionality and error handling of the meetings plugin. Key modifications include updates to the Changes
Poem
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: 4
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3797-3797: Consider stubbingmeeting.meetingRequest.joinMeeting()insteadmarcin-bazyl's suggestion to stub
meeting.meetingRequest.joinMeeting()instead ofMeetingUtil.joinMeeting()seems reasonable to simplify the test setup. This will avoid the need to manually callmeeting.setLocus().sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ id: 'fake locus from mocked join request', locusUrl: 'fake locus url', mediaId: 'fake media id' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/@webex/plugin-meetings/src/meeting/util.ts (3 hunks)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1 hunks)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (4 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
[error] 495-495: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 506-506: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
packages/@webex/plugin-meetings/src/meeting/util.ts (3)
153-155: Properly initializinglocusInfobefore event submissionInitializing
locusInfowithmeeting.setLocus(parsed);before emitting theclient.locus.join.responseevent ensures thatlocusInfois available when the event is handled. This change effectively addresses the issue mentioned in the PR objectives.
166-166: IncludingmediaConnectionsin event metricsAdding
mediaConnections: parsed.mediaConnectionsto the event options provides additional context in theclient.locus.join.responseevent, which can be useful for diagnostics and analytics.
170-170: Returning parsed locus data fromjoinMeetingReturning
parsedfrom thejoinMeetingmethod allows the caller to access the processed locus data directly, improving the method's usability and aligning with standard practices.packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
392-392:⚠️ Potential issueUse camelCase for property names
The property
trackingidshould be renamed totrackingIdto adhere to camelCase naming conventions used throughout the codebase.Apply this diff to fix the issue:
- trackingid: 'trackingId', + trackingId: 'trackingId',Likely invalid or redundant comment.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3791-3798: ****
The comment from SomeBody16 is still valid and applicable to the current code. No need to duplicate the comment.
| return MeetingUtil.joinMeeting(meeting, options).catch((err) => { | ||
| // joining a claimed PMR that is not my own, scenario B | ||
| if (MeetingUtil.isPinOrGuest(err)) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Avoid suppressing TypeScript errors with @ts-ignore
The use of // @ts-ignore on line 321 suppresses TypeScript errors, which may mask underlying type issues. It's generally better to resolve the type error directly.
If the webex.internal.newMetrics.submitClientEvent method has incorrect or missing type definitions, consider updating the type definitions to reflect the actual usage instead of ignoring the error.
| return MeetingUtil.joinMeeting(meeting, options).catch((err) => { | ||
| // joining a claimed PMR that is not my own, scenario B | ||
| if (MeetingUtil.isPinOrGuest(err)) { | ||
| // @ts-ignore | ||
| webex.internal.newMetrics.submitClientEvent({ | ||
| name: 'client.pin.prompt', | ||
| options: { | ||
| meetingId: meeting.id, | ||
| }, | ||
| }); | ||
|
|
||
| // request host pin or non host for unclaimed PMR, start of Scenario C | ||
| // see https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature | ||
| return Promise.reject(new IntentToJoinError('Error Joining Meeting', err)); | ||
| } | ||
| LoggerProxy.logger.error( | ||
| 'Meeting:util#joinMeetingOptions --> Error joining the call, ', | ||
| err | ||
| ); | ||
| // request host pin or non host for unclaimed PMR, start of Scenario C | ||
| // see https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature | ||
| return Promise.reject(new IntentToJoinError('Error Joining Meeting', err)); | ||
| } | ||
| LoggerProxy.logger.error('Meeting:util#joinMeetingOptions --> Error joining the call, ', err); | ||
|
|
||
| return Promise.reject(new JoinMeetingError(options, 'Error Joining Meeting', err)); | ||
| }); | ||
| return Promise.reject(new JoinMeetingError(options, 'Error Joining Meeting', err)); | ||
| }); |
There was a problem hiding this comment.
Avoid including internal URLs in comments
The comment on line 330 includes an internal URL (https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature). Including internal or private URLs in code comments may expose confidential information and can be unhelpful for external developers who may not have access to these resources. Consider removing the URL or replacing it with a reference that does not rely on internal resources.
Apply this diff to update the comment:
- // see https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature
+ // Refer to Locus Lobby and IVR Feature documentation for more details📝 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.
| return MeetingUtil.joinMeeting(meeting, options).catch((err) => { | |
| // joining a claimed PMR that is not my own, scenario B | |
| if (MeetingUtil.isPinOrGuest(err)) { | |
| // @ts-ignore | |
| webex.internal.newMetrics.submitClientEvent({ | |
| name: 'client.pin.prompt', | |
| options: { | |
| meetingId: meeting.id, | |
| }, | |
| }); | |
| // request host pin or non host for unclaimed PMR, start of Scenario C | |
| // see https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature | |
| return Promise.reject(new IntentToJoinError('Error Joining Meeting', err)); | |
| } | |
| LoggerProxy.logger.error( | |
| 'Meeting:util#joinMeetingOptions --> Error joining the call, ', | |
| err | |
| ); | |
| // request host pin or non host for unclaimed PMR, start of Scenario C | |
| // see https://sqbu-github.cisco.com/WebExSquared/locus/wiki/Locus-Lobby-and--IVR-Feature | |
| return Promise.reject(new IntentToJoinError('Error Joining Meeting', err)); | |
| } | |
| LoggerProxy.logger.error('Meeting:util#joinMeetingOptions --> Error joining the call, ', err); | |
| return Promise.reject(new JoinMeetingError(options, 'Error Joining Meeting', err)); | |
| }); | |
| return Promise.reject(new JoinMeetingError(options, 'Error Joining Meeting', err)); | |
| }); | |
| return MeetingUtil.joinMeeting(meeting, options).catch((err) => { | |
| // joining a claimed PMR that is not my own, scenario B | |
| if (MeetingUtil.isPinOrGuest(err)) { | |
| // @ts-ignore | |
| webex.internal.newMetrics.submitClientEvent({ | |
| name: 'client.pin.prompt', | |
| options: { | |
| meetingId: meeting.id, | |
| }, | |
| }); | |
| // request host pin or non host for unclaimed PMR, start of Scenario C | |
| // Refer to Locus Lobby and IVR Feature documentation for more details | |
| return Promise.reject(new IntentToJoinError('Error Joining Meeting', err)); | |
| } | |
| LoggerProxy.logger.error('Meeting:util#joinMeetingOptions --> Error joining the call, ', err); | |
| return Promise.reject(new JoinMeetingError(options, 'Error Joining Meeting', err)); | |
| }); |
| }; | ||
|
|
||
| const parseLocusJoinSpy = sinon.stub(MeetingUtil, 'parseLocusJoin'); | ||
| delete meeting.meetingJoinUrl; |
There was a problem hiding this comment.
Avoid using the delete operator for better performance
As above, replace the delete operation with an assignment to undefined for the meetingJoinUrl property to enhance performance.
Apply this diff to fix the issue:
-delete meeting.meetingJoinUrl;
+meeting.meetingJoinUrl = undefined;📝 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.
| delete meeting.meetingJoinUrl; | |
| meeting.meetingJoinUrl = undefined; |
🧰 Tools
🪛 Biome
[error] 506-506: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| }; | ||
|
|
||
| const parseLocusJoinSpy = sinon.stub(MeetingUtil, 'parseLocusJoin'); | ||
| delete meeting.meetingJoinUrl; |
There was a problem hiding this comment.
Avoid using the delete operator for better performance
Using the delete operator can negatively impact performance as it forces the JavaScript engine to reoptimize the object's structure. Instead, set the property to undefined to maintain optimal performance.
Apply this diff to fix the issue:
-delete meeting.meetingJoinUrl;
+meeting.meetingJoinUrl = undefined;📝 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.
| delete meeting.meetingJoinUrl; | |
| meeting.meetingJoinUrl = undefined; |
🧰 Tools
🪛 Biome
[error] 495-495: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (2)
Line range hint
424-443: LGTM! Consider minor consistency improvement.The new assertions for
meeting.setLocusand the updatedclient.locus.join.responseevent payload correctly reflect the changes in thejoinMeetingfunction's response structure. These modifications align well with the PR objectives.For consistency, consider using an object spread operator for the
setLocusParameterassertion:assert.deepEqual(setLocusParameter, {...MeetingUtil.parseLocusJoin(joinMeetingResponse)})This change would make it clear that we're comparing objects and allow for potential future additions to the parsed locus without breaking the test.
484-491: LGTM! Consider adding a negative test case.The addition of the
isMultistreamproperty and the corresponding assertion forpreferTranscoding: falsewhen multistream is enabled is a good enhancement to the test suite. This change aligns with the improvements in media management mentioned in the AI-generated summary.To improve the robustness of the test suite, consider adding a negative test case where
isMultistreamis false, ensuring thatpreferTranscodingis true in that scenario. This would provide complete coverage for both multistream and non-multistream cases.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3796-3800: Nitpick: Use a more descriptive name for the stub response.Consider renaming the stub response from
{headers: {trackingid: 'fake tracking id'}}to something more meaningful like{headers: {trackingid: 'mocked_tracking_id'}}. This improves code readability by clearly indicating it's a mocked value.- sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ - headers: { - trackingid: 'fake tracking id', - } - }) + sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ + headers: { + trackingid: 'mocked_tracking_id', + } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/@webex/plugin-meetings/src/meeting/util.ts (3 hunks)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1 hunks)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/meeting/util.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
381-408: LGTM! Changes align with PR objectives.The updated
joinMeetingResponsestructure andmeetingobject initialization in thebeforeEachblock are consistent with the PR objectives. These changes reflect the new response format for thejoinMeetingfunction, including themediaConnectionsarray andlocusobject with URL and self ID.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
1-2: LGTM!The copyright header and import statements look good.
3791-3795: Verify the impact of stubbing parseLocusJoin() on other tests.Stubbing
MeetingUtil.parseLocusJoin()to return a fixed object could potentially impact the behavior of other tests that rely on the real implementation. Please ensure this stub does not cause unintended side effects.Run the following script to check if
parseLocusJoin()is used in other test files:If found, carefully review those tests to confirm they still work as expected with the stubbed version.
✅ Verification successful
Stub of
parseLocusJoin()does not impact other tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -g '*.js' -g '!index.js' 'MeetingUtil.parseLocusJoin'Length of output: 213
COMPLETES SPARK-570035
This pull request addresses
Missing locusInfo in
event.identifiersin firstclient.locus.join.responseeventby making the following changes
Initial setup of locusInfo before first
client.locus.join.responseeventChange Type
The following scenarios were tested
client.locus.join.responsenow has propertyidentifiers.locusUrl(with id) andidentifiers.locusIdfilledI 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
Refactor
joinMeetingandjoinMeetingOptionsmethods for better readability and efficiency.Tests