fix(meetings): Meeting joined time header #4124
Conversation
📝 WalkthroughWalkthroughThis update introduces a new property, 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. 🔧 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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ 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
|
chrisadubois
added
the
validated
arun3528
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
183-183: Implementation looks good, consider adding fallback handling.The code correctly retrieves the meeting join time from the response headers using safe optional chaining. This aligns well with the PR objectives to record when a meeting is joined.
While the comment mentions falling back to the system clock if the header doesn't exist, the fallback implementation isn't visible in this line. Consider either:
- Adding explicit fallback here:
-meeting.isoLocalClientMeetingJoinTime = res?.headers?.date; // read from header if exist, else fall back to system clock : https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657 +meeting.isoLocalClientMeetingJoinTime = res?.headers?.date || new Date().toISOString(); // read from header if exist, else fall back to system clock : https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657
- Or confirming the fallback is handled in the setter method for
isoLocalClientMeetingJoinTimein the Meeting class.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(1 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (7)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
209-209: New test variable for meeting join time tracking.Adding the
isoLocalClientMeetingJoinTimevariable to capture the join time aligns well with the PR objective of reading joined time from headers.packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
739-742: Addition of meeting joined time to client event object looks good.This change correctly implements the feature for recording the time a meeting is joined by adding the
meetingJoinedTimeproperty to the client event object when available. The implementation properly checks if the value exists before assigning it.packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
464-505: Good test coverage for the meeting join time header feature.This test case properly verifies that the
isoLocalClientMeetingJoinTimeproperty is correctly set from the date header in the response when joining a meeting. The test adequately sets up the necessary mocks and assertions to validate this functionality.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
1637-1650: Well-implemented setter method for meeting join time tracking.The new setter method for
isoLocalClientMeetingJoinTimecorrectly implements the logic to record meeting join time. It ensures the value is set only once (the first time it's called) and handles both cases appropriately - using the provided timestamp if one exists, or generating a new ISO timestamp if none is provided.This implementation aligns perfectly with the PR objective to add functionality for recording meeting join time, reading from the header if it exists or using the system clock as a fallback.
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
61-65: Good addition of test fixture for meeting join time.The new
fakeMeeting4test fixture correctly extends the base meeting object while adding the newisoLocalClientMeetingJoinTimeproperty, which aligns with the PR's purpose of tracking meeting join times.
71-71: LGTM: Properly registered the new test fixture.The new test fixture is correctly added to the
fakeMeetingsobject, making it accessible for tests.
1071-1204: Well-structured test case for meeting join time functionality.This test case thoroughly validates that the
submitClientEventmethod correctly incorporates theisoLocalClientMeetingJoinTimeproperty from the meeting object into the event payload asmeetingJoinedTime. The test covers all necessary assertions:
- Verification that the correct identifiers are fetched from the meeting
- Confirmation that the
meetingJoinedTimeproperty is set correctly in the diagnostic event- Validation of the full call chain with appropriate spy checks
The test case aligns perfectly with the PR objective of adding functionality to record meeting join times.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Outdated
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
Waiting for response from CA and Locus on Date header |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
7521-7528:⚠️ Potential issueTests are using direct property assignment instead of setter method.
While the test suite is named
#setIsoLocalClientMeetingJoinTime, the tests are directly assigning to the property rather than calling a setter method. This doesn't properly test the setter functionality mentioned in the PR objectives.Update the test to use the setter method instead of direct property assignment:
- meeting.isoLocalClientMeetingJoinTime = 'test'; + meeting.setIsoLocalClientMeetingJoinTime('test'); - meeting.isoLocalClientMeetingJoinTime = 'test2'; + meeting.setIsoLocalClientMeetingJoinTime('test2');
7530-7537:⚠️ Potential issueTest doesn't properly verify the fallback behavior.
This test doesn't explicitly verify that when no value is provided, the system falls back to using the system clock as mentioned in the PR objectives. Additionally, the test shows that the property can still be changed after setting it to
undefinedwhich seems inconsistent with the "once and only once" assertion in the test name.Improve the test to clearly verify the fallback behavior:
- meeting.isoLocalClientMeetingJoinTime = undefined; + meeting.setIsoLocalClientMeetingJoinTime(undefined); const time = meeting.isoLocalClientMeetingJoinTime; + // Verify time is in ISO format indicating system clock was used + assert.match(time, /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/); assert.equal(meeting.isoLocalClientMeetingJoinTime, time); - meeting.isoLocalClientMeetingJoinTime = 'test2'; - assert.equal(meeting.isoLocalClientMeetingJoinTime, 'test2'); + // Verify value cannot be changed after it's set + meeting.setIsoLocalClientMeetingJoinTime('test2'); + assert.equal(meeting.isoLocalClientMeetingJoinTime, time, 'Value should not change after initial setting');
7521-7538: 🛠️ Refactor suggestionAdd dedicated test for header fallback scenario.
The current tests don't explicitly verify the PR's main objective: reading the joined time from the header if it exists, with a fallback to the system clock.
Add a dedicated test case that verifies both scenarios:
it('should use header value when available and fallback to system clock when not', () => { // Mock a header value const headerTime = '2023-01-01T12:00:00.000Z'; // Test with header value meeting.setIsoLocalClientMeetingJoinTime(headerTime); assert.equal(meeting.isoLocalClientMeetingJoinTime, headerTime); // Reset for fallback test meeting = new Meeting({}); // Test fallback to system clock const beforeSet = new Date().toISOString(); meeting.setIsoLocalClientMeetingJoinTime(undefined); const afterSet = new Date().toISOString(); // Verify time is between before and after (proving it used system clock) const actualTime = meeting.isoLocalClientMeetingJoinTime; assert.isTrue( beforeSet <= actualTime && actualTime <= afterSet, `Expected ${actualTime} to be between ${beforeSet} and ${afterSet}` ); });
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
211-211: Missing initialization for test variable.The
isoLocalClientMeetingJoinTimevariable is declared but doesn't appear to be initialized before it's used in the tests. This could lead to unpredictable test behavior.You should initialize this variable in the
beforeEachblock to ensure consistent test behavior.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
1642-1653: Implementation of the meeting joined time header looks good.The newly added setter method for
isoLocalClientMeetingJoinTimeappropriately handles both cases where a time value is provided or not. When no time is provided, it correctly uses the current date in ISO format as a fallback.However, I noticed that there was a previous concern about the behavior when a user rejoins a meeting: the current implementation will overwrite the join time on rejoin, as the method doesn't check if a value is already set. This may be intended behavior, but it's worth confirming that we want to track the most recent join time rather than preserving the first join time.
Consider adding a check to prevent overwriting the join time value if it's already set:
set isoLocalClientMeetingJoinTime(time: string | undefined) { + // Only set this value if it hasn't been set before + if (this.#isoLocalClientMeetingJoinTime === undefined) { if (!time) { this.#isoLocalClientMeetingJoinTime = new Date().toISOString(); } else { this.#isoLocalClientMeetingJoinTime = time; } + } }This would ensure we always keep the first join time regardless of rejoins.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (3)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
63-67: Implementation of meeting join time in test fixtures.The addition of
fakeMeeting4with theisoLocalClientMeetingJoinTimeproperty provides a proper test fixture for the new join time functionality.
73-73: Good test setup: fakeMeeting4 added to fakeMeetings map.Correctly adds the new test fixture to the map for use in tests.
1079-1215: Well-implemented test case for meeting join time functionality.This test thoroughly validates that when a meeting has an
isoLocalClientMeetingJoinTimeproperty, it's correctly passed through to the client event payload asmeetingJoinedTime. The test properly:
- Sets up the necessary spies
- Executes the
submitClientEventmethod withfakeMeeting4- Verifies that all relevant methods receive the
meetingJoinedTimeparameter- Confirms the value matches
fakeMeeting4.isoLocalClientMeetingJoinTimeThis effectively tests the core functionality required for SPARK-555657, ensuring that meeting join time is properly captured in the metrics system.
This reverts commit aa64df3.
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657
This pull request addresses
add meeting joined time, read from header if exists, and set to system clock if not. Apply to CA metrics thereafter
by making the following changes
Change Type
The following scenarios were tested
manual,
unit
I certified that
Make sure to have followed the contributing guidelines before submitting.