feat(internal-plugin-usersub): add usersub plugin to allow the publis… #3936
Conversation
WalkthroughThe changes in this pull request introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebexCore
participant Usersub
User->>WebexCore: Register Usersub Plugin
WebexCore->>Usersub: Initialize with Config
User->>Usersub: Update Answer Calls
Usersub->>WebexCore: Send State Update
WebexCore->>Usersub: Confirm State Update
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: 11
🧹 Outside diff range and nitpick comments (8)
packages/@webex/internal-plugin-usersub/src/index.ts (1)
1-4: Update copyright year rangeThe copyright notice should include the current year since this is a new file created in 2024.
-/*! - * Copyright (c) 2015-2020 Cisco Systems, Inc. See LICENSE file. - */ +/*! + * Copyright (c) 2015-2024 Cisco Systems, Inc. See LICENSE file. + */packages/@webex/internal-plugin-usersub/test/unit/spec/usersub.ts (4)
15-19: Consider improving constants organization and documentation.Consider:
- Adding a comment explaining the significance of the
ttlconstant (120 seconds)- Grouping related constants together (e.g., device-related constants)
const testGuid = 'test-guid'; +// Device-related constants const deviceId = 'dc5c6ab1-1feb-4a6e-be4e-88c48aaee7da'; const testDeviceUrl = `https://wdm-a.wbx2.com/wdm/api/v1/devices/${deviceId}`; + const testAppName = 'wxcc'; +// Time-to-live in seconds for the cross-client-state const ttl = 120;
43-46: Enhance test cases documentation and coverage.Consider making the test cases more descriptive and comprehensive:
const testCases = [ - {enable: true, state: {'answer-calls-on-wxcc': true}}, - {enable: false, state: {'answer-calls-on-wxcc': false}}, + { + name: 'enable answer calls', + enable: true, + state: {'answer-calls-on-wxcc': true}, + expected: 'should enable cross-client state' + }, + { + name: 'disable answer calls', + enable: false, + state: {'answer-calls-on-wxcc': false}, + expected: 'should disable cross-client state' + }, ];
83-106: Improve auto-refresh test readability and maintainability.The test uses magic numbers for timing calculations. Consider extracting these values into named constants for better maintainability.
+// Constants for timing calculations +const REFRESH_INTERVAL_MS = 61 * 1000; // 61 seconds in milliseconds + it('should auto refresh cross-client-state', async () => { // ... setup code ... - const time = 61 * 1000; - clock.tick(time); + clock.tick(REFRESH_INTERVAL_MS); await Promise.resolve(); assert.calledTwice(webex.request); - clock.tick(time); + clock.tick(REFRESH_INTERVAL_MS); await Promise.resolve(); assert.calledThrice(webex.request); });
10-145: Consider restructuring tests for better organization.The test suite has good coverage but could benefit from better organization:
- Consider grouping related tests using nested
describeblocks (e.g., "when enabled", "when disabled", "error scenarios")- Consider extracting common setup code into shared hooks or helper functions
- Consider adding integration tests to verify the interaction with the Mercury messaging service mentioned in the PR objectives
This would improve maintainability and make the test structure more intuitive.
packages/@webex/internal-plugin-usersub/src/usersub.ts (3)
1-3: Update copyright yearThe header comment mentions the year range as 2015-2020. Since this is new code being added in 2024, consider updating the year to reflect the current year.
60-60: Define'answer-calls-on-wxcc'as a constantThe string
'answer-calls-on-wxcc'is used directly in the code. Defining it as a constant can improve maintainability and reduce the risk of typos in future usage.Define the constant at the top of the file:
+ const ANSWER_CALLS_ON_WXCC = 'answer-calls-on-wxcc'; ... state: { - 'answer-calls-on-wxcc': enable, + [ANSWER_CALLS_ON_WXCC]: enable, },
115-119: Clear the refresh timer reference after stopping itAfter calling
clearTimeoutonthis.refreshTimer, it's a good practice to set it toundefinedto prevent potential issues with timer management.Apply this diff to clear the timer reference:
if (this.refreshTimer) { clearTimeout(this.refreshTimer); + this.refreshTimer = undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
- jest.config.js (1 hunks)
- package.json (1 hunks)
- packages/@webex/internal-plugin-usersub/.eslintrc.js (1 hunks)
- packages/@webex/internal-plugin-usersub/README.md (1 hunks)
- packages/@webex/internal-plugin-usersub/babel.config.js (1 hunks)
- packages/@webex/internal-plugin-usersub/jest.config.js (1 hunks)
- packages/@webex/internal-plugin-usersub/package.json (1 hunks)
- packages/@webex/internal-plugin-usersub/process (1 hunks)
- packages/@webex/internal-plugin-usersub/src/config.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/constants.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/index.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/usersub.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/test/unit/spec/usersub.ts (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/@webex/internal-plugin-usersub/.eslintrc.js
- packages/@webex/internal-plugin-usersub/babel.config.js
- packages/@webex/internal-plugin-usersub/jest.config.js
- packages/@webex/internal-plugin-usersub/process
- packages/@webex/internal-plugin-usersub/src/config.ts
🧰 Additional context used
🪛 LanguageTool
packages/@webex/internal-plugin-usersub/README.md
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...) for more details. ## License © 2016-2020 Cisco and/or its affiliates. All Rights...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (11)
packages/@webex/internal-plugin-usersub/src/constants.ts (2)
2-2: LGTM!The service name constant follows the package naming convention and maintains consistency with the microservice architecture.
1-1: Consider increasing the EXPIRATION_OFFSET value.A 1-minute offset for TTL refreshes might lead to frequent refresh operations and unnecessary load on the system. Consider increasing this to a more standard duration (e.g., 5-15 minutes) unless there's a specific requirement for such frequent updates.
packages/@webex/internal-plugin-usersub/src/index.ts (3)
5-8: LGTM!The imports are well-organized and all imports are used within the file.
12-12: Verify Usersub class implementationThe export looks good, but let's verify the implementation of the Usersub class to ensure it includes the required functionality mentioned in the PR objectives (updateAnswerCallsCrossClient API and Mercury messaging integration).
#!/bin/bash # Description: Verify the Usersub class implementation # Test 1: Check for the updateAnswerCallsCrossClient method ast-grep --pattern 'class Usersub { $$$ updateAnswerCallsCrossClient($_) { $$$ } $$$ }' # Test 2: Look for Mercury messaging integration rg -A 5 "Mercury" ./packages/@webex/internal-plugin-usersub/src/usersub.ts
10-10: Verify plugin name uniquenessThe plugin registration looks correct. Let's verify that 'usersub' doesn't conflict with existing plugin names.
✅ Verification successful
Let me gather more information to verify the plugin name uniqueness effectively.
This will help us:
- See all plugin registrations across the codebase
- Find any other occurrences or references to 'usersub' that might indicate existing usage
Plugin name 'usersub' is unique and correctly registered
Based on the search results, the plugin name 'usersub' is unique across the codebase. The registration appears in only one location (packages/@webex/internal-plugin-usersub/src/index.ts) and doesn't conflict with other registered plugins (voicea, metrics, newMetrics, llm, dss). All other occurrences of 'usersub' are within its own package scope, following the expected pattern for internal plugins.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other occurrences of 'usersub' plugin registration # to ensure there are no naming conflicts # Test: Look for other plugin registrations with the same name rg -l "registerInternalPlugin\(['\"]usersub['\"]" --type tsLength of output: 176
Script:
#!/bin/bash # Search for all internal plugin registrations to check for name conflicts rg -A 2 "registerInternalPlugin\(" --type ts # Also search for any other occurrences of 'usersub' string rg -i "usersub" --type tsLength of output: 4327
packages/@webex/internal-plugin-usersub/package.json (2)
15-20: LGTM!The browserify configuration follows the standard Webex SDK package structure.
1-55: Verify package registration in the SDK.Let's ensure this package is properly registered in the SDK's plugin system.
✅ Verification successful
Let me generate additional scripts to verify the package registration more thoroughly.
Based on the shell script results, I can now provide a final response as we have sufficient evidence about the package's registration and integration.
Package is properly registered and integrated in the SDK
The verification shows that:
- The package correctly registers itself as an internal plugin using
registerInternalPlugin('usersub', Usersub, {config})in its index file- The registration mechanism is properly implemented in webex-core with the necessary export of
registerInternalPlugin- The package follows the standard plugin registration pattern used across the SDK
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify package registration and integration # Test 1: Check if the package is registered in the SDK's plugin system echo "Checking plugin registration..." rg -l "registerInternalPlugin.*internal-plugin-usersub" packages/@webex/ # Test 2: Check for proper import paths in other packages echo "Checking import references..." rg "@webex/internal-plugin-usersub" # Test 3: Verify package inclusion in workspace dependencies echo "Checking workspace dependencies..." rg -l "\"@webex/internal-plugin-usersub\".*workspace:\\*" packages/*/package.jsonLength of output: 3303
Script:
#!/bin/bash # Let's check the package's registration and exports # Check the package's index file for plugin registration echo "Checking index file for plugin registration..." cat packages/@webex/internal-plugin-usersub/src/index.js 2>/dev/null || cat packages/@webex/internal-plugin-usersub/src/index.ts 2>/dev/null # Check webex-core for plugin registration patterns echo "Checking webex-core for plugin registration..." rg -A 5 "registerInternalPlugin|registerPlugin" packages/@webex/webex-core/src/ # Check if the package is included in the main Webex bundle echo "Checking main Webex bundle configuration..." cat packages/webex/src/config.js 2>/dev/null || cat packages/webex/src/config.ts 2>/dev/null # Look for any direct dependencies on this package echo "Checking direct dependencies..." rg -l "@webex/internal-plugin-usersub.*workspace:\\*" packages/@webex/*/package.jsonLength of output: 10990
jest.config.js (2)
29-29: LGTM! Package addition follows the established pattern.The new package
@webex/internal-plugin-usersubis correctly integrated into the Jest configuration and will inherit the standard test setup used by other internal plugins.
29-29: Consider defining specific coverage thresholds for the new package.The new package will currently use the global coverage thresholds (85%). If the package contains complex logic or edge cases that might initially have lower coverage, consider adding specific thresholds in the
coverageThresholdsection, similar to other packages with custom thresholds.Also applies to: 126-134
packages/@webex/internal-plugin-usersub/test/unit/spec/usersub.ts (1)
1-9: LGTM! File setup and imports are well-structured.The necessary testing dependencies are properly imported, and the copyright notice is correctly formatted.
package.json (1)
30-30: LGTM! Verify package workspace setup.The
@legacyscript has been correctly updated to include the new@webex/internal-plugin-usersubpackage in alphabetical order.Let's verify the package setup:
✅ Verification successful
Package workspace setup verified successfully
The verification confirms that:
- The package directory exists at the correct location:
packages/@webex/internal-plugin-usersub/- The package.json is properly configured with the correct name and directory path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new package exists and is properly configured in the workspace # Test 1: Check if package directory exists echo "Checking package directory..." fd -t d "internal-plugin-usersub$" packages/@webex # Test 2: Verify package.json exists and has correct name echo "Checking package.json..." fd -t f "package.json$" packages/@webex/internal-plugin-usersub -x cat {} | grep "internal-plugin-usersub"Length of output: 433
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…hing of cross-client-state
mmulcair1981
force-pushed
the
userSubPlugin
branch
from
8712d7a
to
699457b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/internal-plugin-usersub/src/usersub.ts (1)
137-138: Fix JSDoc return type in_extractIdFromUrlThe JSDoc comment specifies that
_extractIdFromUrlreturns{string | null}, but the implementation always returns a string (the device ID or an empty string). Update the JSDoc comment to{string}to match the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
- jest.config.js (1 hunks)
- package.json (1 hunks)
- packages/@webex/internal-plugin-usersub/.eslintrc.js (1 hunks)
- packages/@webex/internal-plugin-usersub/README.md (1 hunks)
- packages/@webex/internal-plugin-usersub/babel.config.js (1 hunks)
- packages/@webex/internal-plugin-usersub/jest.config.js (1 hunks)
- packages/@webex/internal-plugin-usersub/package.json (1 hunks)
- packages/@webex/internal-plugin-usersub/process (1 hunks)
- packages/@webex/internal-plugin-usersub/src/config.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/constants.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/index.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/src/usersub.ts (1 hunks)
- packages/@webex/internal-plugin-usersub/test/unit/spec/usersub.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/internal-plugin-usersub/process
🚧 Files skipped from review as they are similar to previous changes (11)
- jest.config.js
- package.json
- packages/@webex/internal-plugin-usersub/.eslintrc.js
- packages/@webex/internal-plugin-usersub/README.md
- packages/@webex/internal-plugin-usersub/babel.config.js
- packages/@webex/internal-plugin-usersub/jest.config.js
- packages/@webex/internal-plugin-usersub/package.json
- packages/@webex/internal-plugin-usersub/src/config.ts
- packages/@webex/internal-plugin-usersub/src/constants.ts
- packages/@webex/internal-plugin-usersub/src/index.ts
- packages/@webex/internal-plugin-usersub/test/unit/spec/usersub.ts
| deviceId: this._extractIdFromUrl(this.webex.internal.device.url), | ||
| appName, |
There was a problem hiding this comment.
Handle empty deviceId before sending the request
The deviceId extracted by _extractIdFromUrl could be an empty string if the device URL does not match the expected format. This may cause issues in the JSON payload sent to the server. Consider adding a check to handle the case when deviceId is empty or invalid.
Apply this diff to validate deviceId before sending the request:
+ const deviceId = this._extractIdFromUrl(this.webex.internal.device.url);
+ if (!deviceId) {
+ return Promise.reject(new Error('Device ID could not be extracted from device URL.'));
+ }Then, update jsonData to use the deviceId variable:
- deviceId: this._extractIdFromUrl(this.webex.internal.device.url),
+ deviceId: deviceId,📝 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.
| deviceId: this._extractIdFromUrl(this.webex.internal.device.url), | |
| appName, | |
| const deviceId = this._extractIdFromUrl(this.webex.internal.device.url); | |
| if (!deviceId) { | |
| return Promise.reject(new Error('Device ID could not be extracted from device URL.')); | |
| } | |
| deviceId: deviceId, | |
| appName, |
There was a problem hiding this comment.
deviceId can be empty - so not rejecting it in this case
There was a problem hiding this comment.
@mmulcair1981, understood. Thank you for the clarification.
(_/)
(•_•)
(>🌸<)
✏️ Learnings added
Learnt from: mmulcair1981
PR: webex/webex-js-sdk#3936
File: packages/@webex/internal-plugin-usersub/src/usersub.ts:65-66
Timestamp: 2024-10-24T08:50:23.497Z
Learning: In the `updateAnswerCallsCrossClient` method in the `Usersub` class (`packages/@webex/internal-plugin-usersub/src/usersub.ts`), the `deviceId` can be empty, and the request should not be rejected in this case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Shreyas281299
added
the
validated
There was a problem hiding this comment.
- I think we should first have a discussion about whether this should be a new plugin or not?
- We could update the plugin-presence or the internal-plugin-presence to use ApheliaV2.
- Also Usersub is 1 of the pieces from apheliaV2, is there a plan to add functionalities as well like the availability, workstatus and activity.
@mmulcair1981 I think we should have a discussion to plan what this plugin should be, I think usersub could be part of our new presence-plugin.
cc. @sreenara
…hing of cross-client-state
COMPLETES CX-15924
This pull request adds a new internal plugin for the userSub micorservice. And adds an API that allows the consumer to set the cross-client-state composition, details can be found here
The newly added API allows the cross-client-state to be set for the signed in user. This setting is then published in a userState update over mercury.
Adds a new internal plugin for the userSub microservice
Add updateAnswerCallsCrossClient API which will publish the cross-client-composition.
Change Type
The following scenarios were tested
Added unit tests
I also consumed the module in webex contact center and tested that the requests were publishing cross-client-state on the integration environment.
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
@webex/internal-plugin-usersubpackage for managing user subscription services.Documentation
Tests
Usersubplugin, covering various scenarios including parameter validation and error handling.Chores