feat(internal-device-plugin): added deleteDevices #3998
Conversation
WalkthroughThe changes introduce a new method Changes
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 (1.23.1)
warning eslint@8.57.1: 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: 3
🧹 Outside diff range and nitpick comments (2)
packages/@webex/internal-plugin-device/src/device.js (2)
Line range hint
510-585: Remove unnecessary block statementsThere are redundant block statements in the code between lines 510-585:
- Unnecessary blocks: Standalone block statements without declarations clutter the code.
Consider removing these blocks to improve readability:
- { // Method implementation - }This enhances code clarity without altering functionality.
🧰 Tools
🪛 Biome
[error] 508-509: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 510-510: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 510-510: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
489-498: Assignment within an expression can be confusingUsing assignments within expressions might lead to unintended side effects:
- Confusing assignments: Assignments in expressions can reduce code readability.
Refactor the code to eliminate the assignment within the expression:
- if (!deviceRegistrationOptions.deleteFlag) { + const {deleteFlag = false} = deviceRegistrationOptions; + if (!deleteFlag) {This makes the code clearer and more maintainable.
🧰 Tools
🪛 Biome
[error] 489-489: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 490-497: Illegal return statement outside of a function
(parse)
[error] 498-498: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 489-489: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 489-498: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-device/src/device.js(3 hunks)packages/@webex/internal-plugin-device/test/unit/spec/device.js(2 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/internal-plugin-device/src/device.js
[error] 449-481: Illegal return statement outside of a function
(parse)
[error] 482-482: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 487-488: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 489-489: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 490-497: Illegal return statement outside of a function
(parse)
[error] 498-498: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 489-489: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 447-482: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 489-498: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 510-510: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 510-510: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 510-585: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (8)
packages/@webex/internal-plugin-device/src/device.js (4)
5-5: Importing 'orderBy' from 'lodash'
The orderBy function is correctly imported from 'lodash' and used later in the code.
487-498:
Incorrect use of decorators and syntax errors in 'register' method
The register method has syntax errors due to incorrect use of decorators and misplaced block statements:
- Decorators are not valid here: Decorators should be applied directly to methods within classes.
- Illegal return statement outside of a function: Return statements are incorrectly placed.
- Assignment within an expression: Assignments should not be used within expressions.
Please adjust the method as follows to fix the syntax errors:
+ /**
+ * Registers the device and deletes old devices upon failure
+ *
+ * @param {Object} deviceRegistrationOptions - Options for device registration
+ * @returns {Promise<void>}
+ */
+ @oneFlight
+ @waitForValue('@')
+ register(deviceRegistrationOptions = {}) {
+ return this._registerInternal(deviceRegistrationOptions).catch((error) => {
+ if (!deviceRegistrationOptions.deleteFlag) {
+ return this.deleteDevices().then(() =>
+ this._registerInternal({...deviceRegistrationOptions, deleteFlag: true})
+ );
+ }
+ throw error;
+ });
+ },This correction:
- Places decorators directly above the method definition.
- Fixes the placement of return statements.
- Avoids assignment within expressions by using object spread
{...deviceRegistrationOptions, deleteFlag: true}.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 487-488: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 489-489: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 490-497: Illegal return statement outside of a function
(parse)
[error] 498-498: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 489-489: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 489-498: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
510-510:
Syntax error and decorator misuse in '_registerInternal' method
There's a syntax error due to the incorrect placement of decorators on the _registerInternal method:
- Decorators are not valid here: Decorators must be applied directly to class methods.
Please apply the decorators correctly as shown below:
+ /**
+ * Internal method for device registration
+ *
+ * @param {Object} deviceRegistrationOptions - Options for device registration
+ * @returns {Promise<void>}
+ */
+ @oneFlight
+ @waitForValue('@')
+ _registerInternal(deviceRegistrationOptions = {}) {
this.logger.info('device: registering');
// Rest of the method...
},This ensures that:
- Decorators are correctly applied.
- Syntax errors are resolved.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 510-510: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 510-510: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 510-585: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
447-482: Static analysis reports multiple syntax issues
Static analysis tools have identified several syntax errors in this code segment:
- Illegal return statements
- Invalid decorator usage
- Unnecessary block statements
Please run the following command to identify and fix syntax errors:
This will help in pinpointing the exact locations of syntax issues for correction.
🧰 Tools
🪛 Biome
[error] 449-481: Illegal return statement outside of a function
(parse)
[error] 482-482: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 447-482: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/internal-plugin-device/test/unit/spec/device.js (4)
360-394: New Test Cases for deleteDevices() Functionality
The test cases added for deleteDevices() effectively cover the scenarios where devices should be deleted based on the specified criteria.
427-449: Test Case for No Deletion When Only Two Devices Exist
The test correctly verifies that no devices are deleted when there are only two devices, aligning with the expected behavior.
480-495: Test for Invoking deleteDevices() When Registration Fails and deleteFlag is False
The test appropriately verifies that deleteDevices() is called upon registration failure when deleteFlag is false, and that register is retried as expected.
497-512: Test for Not Invoking deleteDevices() When deleteFlag is True
The test correctly ensures that deleteDevices() is not called when registration fails and deleteFlag is true, adhering to the intended logic.
| this.request({ | ||
| uri: url, | ||
| method: 'DELETE', | ||
| }).then(() => { | ||
| this.clear(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential issue with 'this.clear()' inside deletion loop
Calling this.clear() inside the loop may lead to unintended side effects:
- Clearing the device multiple times:
this.clear()is called for each device deletion. - Asynchronous issues: The loop does not wait for deletion requests to complete before clearing.
Move this.clear() outside the loop and ensure all deletions are complete before clearing:
// Send DELETE requests for each URL
- urlsToDelete.forEach((url) => {
- this.request({
- uri: url,
- method: 'DELETE',
- }).then(() => {
- this.clear();
- });
- });
+ await Promise.all(
+ urlsToDelete.map((url) =>
+ this.request({
+ uri: url,
+ method: 'DELETE',
+ })
+ )
+ );
+ // Clear after all deletions are complete
+ this.clear();This adjustment:
- Uses
Promise.allwithawaitto handle asynchronous deletion requests. - Ensures
this.clear()is called once after all deletions.
Committable suggestion skipped: line range outside the PR's diff.
| deleteDevices() { | ||
| // Fetch devices with a GET request | ||
| return this.request({ | ||
| method: 'GET', | ||
| service: 'wdm', | ||
| resource: 'devices', | ||
| }) | ||
| .then((response) => { | ||
| const {devices} = response.body; | ||
|
|
||
| // Filter devices of type 'WEB' | ||
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | ||
|
|
||
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | ||
|
|
||
| // If there are more than two devices, delete the last third | ||
| if (sortedDevices.length > 2) { | ||
| const totalItems = sortedDevices.length; | ||
| const countToDelete = Math.ceil(totalItems / 3); | ||
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | ||
|
|
||
| // Send DELETE requests for each URL | ||
| urlsToDelete.forEach((url) => { | ||
| this.request({ | ||
| uri: url, | ||
| method: 'DELETE', | ||
| }).then(() => { | ||
| this.clear(); | ||
| }); | ||
| }); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| this.logger.error('Failed to retrieve devices:', error); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Critical syntax errors in 'deleteDevices' method
There are several syntax errors in the deleteDevices method that prevent the code from compiling:
- Illegal return statement outside of a function: The
returnstatements may be misplaced. - Decorators are not valid here: Decorators like
@oneFlightand@waitForValue('@')are incorrectly applied. - Unnecessary block statements: There are block statements that serve no purpose.
- Promise handling issues: Improper use of
.then()and.catch()chaining.
To fix these issues, consider refactoring the method as follows:
+ /**
+ * Fetches the web devices and deletes the third of them which are not recent devices in use
+ * @returns {Promise<void>}
+ */
+ async deleteDevices() {
+ try {
+ // Fetch devices with a GET request
+ const response = await this.request({
+ method: 'GET',
+ service: 'wdm',
+ resource: 'devices',
+ });
+
+ const {devices} = response.body;
+
+ // Filter devices of type 'WEB'
+ const webDevices = devices.filter((item) => item.deviceType === 'WEB');
+
+ const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]);
+
+ // If there are more than two devices, delete the oldest third
+ if (sortedDevices.length > 2) {
+ const totalItems = sortedDevices.length;
+ const countToDelete = Math.ceil(totalItems / 3);
+ const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url);
+
+ // Send DELETE requests for each URL
+ await Promise.all(
+ urlsToDelete.map((url) =>
+ this.request({
+ uri: url,
+ method: 'DELETE',
+ })
+ )
+ );
+ // Clear after deletions are complete
+ this.clear();
+ }
+ } catch (error) {
+ this.logger.error('Failed to retrieve devices:', error);
+ }
+ },This refactoring:
- Uses
async/awaitfor better readability and error handling. - Ensures that
this.clear()is called only once after all deletions are complete. - Fixes syntax errors and removes unnecessary block statements.
📝 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.
| deleteDevices() { | |
| // Fetch devices with a GET request | |
| return this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }) | |
| .then((response) => { | |
| const {devices} = response.body; | |
| // Filter devices of type 'WEB' | |
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the last third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Send DELETE requests for each URL | |
| urlsToDelete.forEach((url) => { | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }).then(() => { | |
| this.clear(); | |
| }); | |
| }); | |
| } | |
| }) | |
| .catch((error) => { | |
| this.logger.error('Failed to retrieve devices:', error); | |
| }); | |
| }, | |
| /** | |
| * Fetches the web devices and deletes the third of them which are not recent devices in use | |
| * @returns {Promise<void>} | |
| */ | |
| async deleteDevices() { | |
| try { | |
| // Fetch devices with a GET request | |
| const response = await this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }); | |
| const {devices} = response.body; | |
| // Filter devices of type 'WEB' | |
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the oldest third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Send DELETE requests for each URL | |
| await Promise.all( | |
| urlsToDelete.map((url) => | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }) | |
| ) | |
| ); | |
| // Clear after deletions are complete | |
| this.clear(); | |
| } | |
| } catch (error) { | |
| this.logger.error('Failed to retrieve devices:', error); | |
| } | |
| }, |
🧰 Tools
🪛 Biome
[error] 449-481: Illegal return statement outside of a function
(parse)
[error] 482-482: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 447-482: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
| it('should delete correct number of devices', async () => { | ||
| const response = { | ||
| body: { | ||
| devices: [ | ||
| {url: 'url3', modificationTime: '2023-10-03T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url4', modificationTime: '2023-10-04T10:00:00Z', deviceType: 'notweb'}, | ||
| {url: 'url1', modificationTime: '2023-10-01T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url2', modificationTime: '2023-10-02T10:00:00Z', deviceType: 'WEB'}, | ||
| ] | ||
| } | ||
| }; | ||
| const requestStub = sinon.stub(device, 'request'); | ||
| requestStub.withArgs(sinon.match({method: 'GET'})).resolves(response); | ||
| requestStub.withArgs(sinon.match({method: 'DELETE'})).resolves(); | ||
|
|
||
| const clearSpy = sinon.spy(device, 'clear'); | ||
| await device.deleteDevices(); | ||
|
|
||
| const expectedDeletions = ['url1']; | ||
|
|
||
| expectedDeletions.forEach(url => { | ||
| assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'}))); | ||
| }); | ||
|
|
||
| const notDeletedUrls = ['url2', 'url3', 'url4']; | ||
| notDeletedUrls.forEach(url => { | ||
| assert(requestStub.neverCalledWith(sinon.match({uri: url, method: 'DELETE'}))); | ||
| }); | ||
|
|
||
| assert.equal(clearSpy.callCount, expectedDeletions.length); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate Test Case with Identical Description and Implementation
The test case starting at line 395 appears to be a duplicate of the test case starting at line 361. Both tests have the same description 'should delete correct number of devices' and contain identical code. This duplication may lead to confusion and unnecessary maintenance.
Consider removing the duplicate test or modifying it to cover a different scenario with a unique description.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| return this._registerInternal(deviceRegistrationOptions).catch((error) => { | ||
| if (!deviceRegistrationOptions.deleteFlag) { | ||
| return this.deleteDevices().then(() => { | ||
| return this._registerInternal({deleteFlag: true}); |
There was a problem hiding this comment.
issue: we don't need the delete flag anymore
| return this._registerInternal(deviceRegistrationOptions).catch((error) => { | ||
| if (!deviceRegistrationOptions.deleteFlag) { | ||
| return this.deleteDevices().then(() => { | ||
| return this._registerInternal({deleteFlag: true}); |
There was a problem hiding this comment.
issue: we should be passing in the original options
| @waitForValue('@') | ||
| register(deviceRegistrationOptions = {}) { | ||
| return this._registerInternal(deviceRegistrationOptions).catch((error) => { | ||
| if (!deviceRegistrationOptions.deleteFlag) { |
There was a problem hiding this comment.
issue: instead of checking the delete flag, we should be checking for the error message of excessive device registrations
packages/@webex/internal-plugin-device/test/unit/spec/device.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-device/test/unit/spec/device.js (2)
361-392: Enhance test coverage with additional test cases.While the test verifies basic functionality, consider adding the following test cases for better coverage:
- Empty device list
- List with only non-WEB devices
- Single device
- Edge case where number of devices to delete is not a whole number (e.g., 5 devices where 1.67 should be deleted)
it('should delete correct number of devices', async () => { + // Given 7 WEB devices, we expect to delete the oldest 2 devices (rounded down from 2.33) const response = { body: { devices: [
445-460: Verify parameter consistency in retry attempt.The test verifies that registration is retried after device deletion, but doesn't confirm that the same parameters are passed to both attempts. Consider enhancing the test to verify parameter consistency.
it('calls delete devices when errors with User has excessive device registrations', async () => { setup(); const deleteDeviceSpy = sinon.stub(device, 'deleteDevices').callsFake(() => Promise.resolve()); const registerStub = sinon.stub(device, '_registerInternal'); + const testParams = {includeDetails: 'test-param'}; registerStub.onFirstCall().rejects({body: {message: 'User has excessive device registrations'}}); registerStub.onSecondCall().callsFake(() => Promise.resolve({exampleKey: 'example response value',})); - const result = await device.register(); + const result = await device.register(testParams); assert.calledOnce(deleteDeviceSpy); assert.equal(registerStub.callCount, 2); + assert.calledWith(registerStub.firstCall, testParams); + assert.calledWith(registerStub.secondCall, testParams); assert.deepEqual(result, {exampleKey: 'example response value'}); }); +it('propagates error when both registration attempts fail', async () => { + setup(); + const deleteDeviceSpy = sinon.stub(device, 'deleteDevices').callsFake(() => Promise.resolve()); + const registerStub = sinon.stub(device, '_registerInternal'); + + const error = {body: {message: 'User has excessive device registrations'}}; + registerStub.rejects(error); + + await assert.isRejected(device.register(), error); + assert.calledOnce(deleteDeviceSpy); + assert.calledTwice(registerStub); +});packages/@webex/internal-plugin-device/src/device.js (1)
482-496: Enhance error handling and logging in register methodThe retry logic is good, but could be improved with better error handling and logging.
Consider these improvements:
+ const EXCESSIVE_DEVICE_ERROR = 'User has excessive device registrations'; register(deviceRegistrationOptions = {}) { return this._registerInternal(deviceRegistrationOptions).catch((error) => { - if (error?.body?.message === 'User has excessive device registrations') { + if (error?.body?.message === EXCESSIVE_DEVICE_ERROR) { + this.logger.info('device: excessive registrations detected, attempting cleanup'); return this.deleteDevices().then(() => { + this.logger.info('device: retrying registration after cleanup'); return this._registerInternal(deviceRegistrationOptions); }); } + this.logger.error('device: registration failed', error); throw error; }); },🧰 Tools
🪛 Biome
[error] 485-486: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 487-487: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 488-495: Illegal return statement outside of a function
(parse)
[error] 496-496: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 487-496: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-device/src/device.js(3 hunks)packages/@webex/internal-plugin-device/test/unit/spec/device.js(2 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/internal-plugin-device/src/device.js
[error] 449-479: Illegal return statement outside of a function
(parse)
[error] 480-480: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 485-486: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 487-487: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 488-495: Illegal return statement outside of a function
(parse)
[error] 496-496: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 487-487: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 447-480: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 487-496: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 508-508: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 508-583: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (2)
packages/@webex/internal-plugin-device/test/unit/spec/device.js (1)
462-477: LGTM! Good negative test case.
The test properly verifies that device deletion is not triggered for other types of errors.
packages/@webex/internal-plugin-device/src/device.js (1)
508-508: LGTM: Good method renaming
The renaming of register to _registerInternal follows best practices for internal methods while maintaining the same functionality.
🧰 Tools
🪛 Biome
[error] 508-508: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 508-508: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 508-583: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
| deleteDevices() { | ||
| // Fetch devices with a GET request | ||
| return this.request({ | ||
| method: 'GET', | ||
| service: 'wdm', | ||
| resource: 'devices', | ||
| }) | ||
| .then((response) => { | ||
| const {devices} = response.body; | ||
|
|
||
| // Filter devices of type 'WEB' | ||
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | ||
|
|
||
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | ||
|
|
||
| // If there are more than two devices, delete the last third | ||
| if (sortedDevices.length > 2) { | ||
| const totalItems = sortedDevices.length; | ||
| const countToDelete = Math.ceil(totalItems / 3); | ||
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | ||
|
|
||
| // Send DELETE requests for each URL | ||
| urlsToDelete.forEach((url) => { | ||
| this.request({ | ||
| uri: url, | ||
| method: 'DELETE', | ||
| }); | ||
| }); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| this.logger.error('Failed to retrieve devices:', error); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Critical: Fix async operations and error handling in deleteDevices
The current implementation has several critical issues:
- DELETE requests are not awaited, potentially leading to race conditions
this.clear()is called multiple times unnecessarily- Errors are swallowed in the catch block, making debugging difficult
Apply this fix to properly handle async operations and error handling:
deleteDevices() {
// Fetch devices with a GET request
return this.request({
method: 'GET',
service: 'wdm',
resource: 'devices',
})
.then((response) => {
const {devices} = response.body;
// Filter devices of type 'WEB'
const webDevices = devices.filter((item) => item.deviceType === 'WEB');
const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]);
// If there are more than two devices, delete the last third
if (sortedDevices.length > 2) {
const totalItems = sortedDevices.length;
const countToDelete = Math.ceil(totalItems / 3);
const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url);
- // Send DELETE requests for each URL
- urlsToDelete.forEach((url) => {
- this.request({
- uri: url,
- method: 'DELETE',
- });
- });
+ // Wait for all DELETE requests to complete
+ return Promise.all(
+ urlsToDelete.map((url) =>
+ this.request({
+ uri: url,
+ method: 'DELETE',
+ })
+ )
+ ).then(() => {
+ // Clear only once after all deletions are complete
+ this.clear();
+ });
}
+ return Promise.resolve();
})
.catch((error) => {
- this.logger.error('Failed to retrieve devices:', error);
+ this.logger.error('Failed to retrieve or delete devices:', error);
+ return Promise.reject(error);
});
},📝 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.
| deleteDevices() { | |
| // Fetch devices with a GET request | |
| return this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }) | |
| .then((response) => { | |
| const {devices} = response.body; | |
| // Filter devices of type 'WEB' | |
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the last third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Send DELETE requests for each URL | |
| urlsToDelete.forEach((url) => { | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }); | |
| }); | |
| } | |
| }) | |
| .catch((error) => { | |
| this.logger.error('Failed to retrieve devices:', error); | |
| }); | |
| }, | |
| deleteDevices() { | |
| // Fetch devices with a GET request | |
| return this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }) | |
| .then((response) => { | |
| const {devices} = response.body; | |
| // Filter devices of type 'WEB' | |
| const webDevices = devices.filter((item) => item.deviceType === 'WEB'); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the last third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Wait for all DELETE requests to complete | |
| return Promise.all( | |
| urlsToDelete.map((url) => | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }) | |
| ) | |
| ).then(() => { | |
| // Clear only once after all deletions are complete | |
| this.clear(); | |
| }); | |
| } | |
| return Promise.resolve(); | |
| }) | |
| .catch((error) => { | |
| this.logger.error('Failed to retrieve or delete devices:', error); | |
| return Promise.reject(error); | |
| }); | |
| }, |
🧰 Tools
🪛 Biome
[error] 449-479: Illegal return statement outside of a function
(parse)
[error] 480-480: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 447-480: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/@webex/internal-plugin-device/src/device.js (2)
484-498: Enhance error logging for registration failuresThe error handling logic is good, but would benefit from additional logging to aid debugging.
register(deviceRegistrationOptions = {}) { return this._registerInternal(deviceRegistrationOptions).catch((error) => { + this.logger.warn('Device registration failed:', error); if (error?.body?.message === 'User has excessive device registrations') { + this.logger.info('Attempting to clean up old devices and retry registration'); return this.deleteDevices().then(() => { return this._registerInternal(deviceRegistrationOptions); }); } + this.logger.error('Registration failed with unhandled error'); throw error; }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 487-488: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 489-489: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 490-497: Illegal return statement outside of a function
(parse)
[error] 489-489: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 489-498: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
500-505: Add null checks to _getBody methodThe method should handle cases where config or its properties might be undefined.
_getBody() { return { - ...(this.config.defaults.body ? this.config.defaults.body : {}), - ...(this.config.body ? this.config.body : {}), + ...(this.config?.defaults?.body || {}), + ...(this.config?.body || {}), }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 501-504: Illegal return statement outside of a function
(parse)
[error] 505-505: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 500-505: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/internal-plugin-device/test/unit/spec/device.js (3)
432-452: Add more edge cases to test suiteThe test suite covers the basic scenarios but misses important edge cases.
Consider adding these test cases:
it('handles empty device list', async () => { setup('WEB'); const response = { body: { devices: [] } }; // ... implementation }); it('handles all devices of different type', async () => { setup('WEB'); const response = { body: { devices: [ {url: 'url1', modificationTime: '2023-10-01T10:00:00Z', deviceType: 'MOBILE'}, {url: 'url2', modificationTime: '2023-10-02T10:00:00Z', deviceType: 'MOBILE'} ] } }; // ... implementation }); it('handles API errors gracefully', async () => { setup('WEB'); const requestStub = sinon.stub(device, 'request'); requestStub.withArgs(sinon.match({method: 'GET'})).rejects(new Error('API Error')); // ... implementation });
484-499: Enhance error handling test coverageThe test verifies basic error handling but could be more comprehensive.
Consider adding these test cases:
it('retries only once after deleting devices', async () => { setup(); const deleteDeviceSpy = sinon.stub(device, 'deleteDevices').resolves(); const registerStub = sinon.stub(device, '_registerInternal'); registerStub.onFirstCall().rejects({body: {message: 'User has excessive device registrations'}}); registerStub.onSecondCall().rejects({body: {message: 'User has excessive device registrations'}}); await assert.isRejected(device.register(), /User has excessive device registrations/); assert.calledOnce(deleteDeviceSpy); assert.calledTwice(registerStub); }); it('verifies the exact error message for device deletion', async () => { setup(); const deleteDeviceSpy = sinon.stub(device, 'deleteDevices').resolves(); const registerStub = sinon.stub(device, '_registerInternal'); registerStub.rejects({body: {message: 'Similar but different error'}}); await assert.isRejected(device.register()); assert.notCalled(deleteDeviceSpy); });
501-516: Improve test readability and error handlingThe test could be more explicit about its expectations and error handling.
- try { - await device.register({deleteFlag: true}); - } catch (error) { - assert.notCalled(deleteDeviceSpy); - - assert.equal(registerStub.callCount, 1); - - assert.match(error.message, /some error/, 'Expected error message not matched'); - } + await assert.isRejected( + device.register({deleteFlag: true}), + /some error/, + 'Should reject with the original error' + ); + + assert.notCalled(deleteDeviceSpy, 'Should not delete devices for unrelated errors'); + assert.calledOnce(registerStub, 'Should not retry registration');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-device/src/device.js(4 hunks)packages/@webex/internal-plugin-device/test/unit/spec/device.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/internal-plugin-device/src/device.js
[error] 449-481: Illegal return statement outside of a function
(parse)
[error] 482-482: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 487-488: Decorators are not valid here.
Decorators are only valid on class declarations, class expressions, and class methods.
(parse)
[error] 489-489: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 490-497: Illegal return statement outside of a function
(parse)
[error] 498-500: Expected a statement but instead found ',
_getBody()'.
Expected a statement here.
(parse)
[error] 501-504: Illegal return statement outside of a function
(parse)
[error] 505-505: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 489-489: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 447-482: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 489-498: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 500-505: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 518-518: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 518-518: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 518-590: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (1)
packages/@webex/internal-plugin-device/src/device.js (1)
Line range hint 518-590: Verify impact on existing device registrations
The refactoring of the registration flow could impact existing device registrations. Please ensure this is tested thoroughly.
✅ Verification successful
No impact on existing device registrations - changes are well tested
Based on the test file analysis, the changes in the registration flow are thoroughly tested, particularly:
- Existing device registration handling is verified through tests that check the
registeredstate - Error handling for excessive device registrations is tested
- Device refresh and re-registration scenarios are covered
- Registration success processing and feature updates are verified
The refactoring maintains backward compatibility and proper error handling, with no adverse effects on existing device registrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all device registration related tests
rg -l "register.*device" --type test
# Search for existing device registration error handling
rg "excessive.*device.*registration" --type test
Length of output: 147
Script:
#!/bin/bash
# Search for device registration tests
rg -l "register.*device" "test|spec"
# Search for device registration implementation and its usages
ast-grep --pattern 'function $_registerInternal'
rg "_registerInternal" -A 2
# Search for device registration tests in test files
fd -e test.js -e spec.js -x rg -l "register.*device" {}
Length of output: 1797
Script:
#!/bin/bash
# Check the test file content for device registration tests
cat packages/@webex/internal-plugin-device/test/unit/spec/device.js
# Look for any other test files related to device registration
fd -e test.js -e spec.js -x rg "describe.*device.*registration|test.*device.*registration" {}
Length of output: 28176
| /** | ||
| * Fetches the web devices and deletes the third of them which are not recent devices in use | ||
| * @returns {Promise<void, Error>} | ||
| */ | ||
| deleteDevices() { | ||
| // Fetch devices with a GET request | ||
| return this.request({ | ||
| method: 'GET', | ||
| service: 'wdm', | ||
| resource: 'devices', | ||
| }) | ||
| .then((response) => { | ||
| const {devices} = response.body; | ||
|
|
||
| const {deviceType} = this._getBody(); | ||
|
|
||
| // Filter devices of type deviceType | ||
| const webDevices = devices.filter((item) => item.deviceType === deviceType); | ||
|
|
||
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | ||
|
|
||
| // If there are more than two devices, delete the last third | ||
| if (sortedDevices.length > 2) { | ||
| const totalItems = sortedDevices.length; | ||
| const countToDelete = Math.ceil(totalItems / 3); | ||
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | ||
|
|
||
| // Send DELETE requests for each URL | ||
| urlsToDelete.forEach((url) => { | ||
| this.request({ | ||
| uri: url, | ||
| method: 'DELETE', | ||
| }); | ||
| }); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| this.logger.error('Failed to retrieve devices:', error); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Critical: Fix async operations and error handling in deleteDevices
The current implementation has several critical issues:
- Race condition: DELETE requests are not awaited, potentially leading to inconsistent state
- Error handling: Errors are swallowed in catch block
- Memory management: Device state not cleared after deletions
Apply this fix to properly handle async operations and error handling:
deleteDevices() {
// Fetch devices with a GET request
return this.request({
method: 'GET',
service: 'wdm',
resource: 'devices',
})
.then((response) => {
const {devices} = response.body;
const {deviceType} = this._getBody();
// Filter devices of type deviceType
const webDevices = devices.filter((item) => item.deviceType === deviceType);
const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]);
// If there are more than two devices, delete the oldest third
if (sortedDevices.length > 2) {
const totalItems = sortedDevices.length;
const countToDelete = Math.ceil(totalItems / 3);
const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url);
- // Send DELETE requests for each URL
- urlsToDelete.forEach((url) => {
- this.request({
- uri: url,
- method: 'DELETE',
- });
- });
+ // Wait for all DELETE requests to complete
+ return Promise.all(
+ urlsToDelete.map((url) =>
+ this.request({
+ uri: url,
+ method: 'DELETE',
+ })
+ )
+ ).then(() => {
+ // Clear device state after all deletions complete
+ this.clear();
+ });
}
+ return Promise.resolve();
})
.catch((error) => {
- this.logger.error('Failed to retrieve devices:', error);
+ this.logger.error('Failed to retrieve or delete devices:', error);
+ return Promise.reject(error);
});
}📝 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.
| /** | |
| * Fetches the web devices and deletes the third of them which are not recent devices in use | |
| * @returns {Promise<void, Error>} | |
| */ | |
| deleteDevices() { | |
| // Fetch devices with a GET request | |
| return this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }) | |
| .then((response) => { | |
| const {devices} = response.body; | |
| const {deviceType} = this._getBody(); | |
| // Filter devices of type deviceType | |
| const webDevices = devices.filter((item) => item.deviceType === deviceType); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the last third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Send DELETE requests for each URL | |
| urlsToDelete.forEach((url) => { | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }); | |
| }); | |
| } | |
| }) | |
| .catch((error) => { | |
| this.logger.error('Failed to retrieve devices:', error); | |
| }); | |
| }, | |
| /** | |
| * Fetches the web devices and deletes the third of them which are not recent devices in use | |
| * @returns {Promise<void, Error>} | |
| */ | |
| deleteDevices() { | |
| // Fetch devices with a GET request | |
| return this.request({ | |
| method: 'GET', | |
| service: 'wdm', | |
| resource: 'devices', | |
| }) | |
| .then((response) => { | |
| const {devices} = response.body; | |
| const {deviceType} = this._getBody(); | |
| // Filter devices of type deviceType | |
| const webDevices = devices.filter((item) => item.deviceType === deviceType); | |
| const sortedDevices = orderBy(webDevices, [(item) => new Date(item.modificationTime)]); | |
| // If there are more than two devices, delete the oldest third | |
| if (sortedDevices.length > 2) { | |
| const totalItems = sortedDevices.length; | |
| const countToDelete = Math.ceil(totalItems / 3); | |
| const urlsToDelete = sortedDevices.slice(0, countToDelete).map((item) => item.url); | |
| // Wait for all DELETE requests to complete | |
| return Promise.all( | |
| urlsToDelete.map((url) => | |
| this.request({ | |
| uri: url, | |
| method: 'DELETE', | |
| }) | |
| ) | |
| ).then(() => { | |
| // Clear device state after all deletions complete | |
| this.clear(); | |
| }); | |
| } | |
| return Promise.resolve(); | |
| }) | |
| .catch((error) => { | |
| this.logger.error('Failed to retrieve or delete devices:', error); | |
| return Promise.reject(error); | |
| }); | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 449-481: Illegal return statement outside of a function
(parse)
[error] 482-482: Expected a statement but instead found ','.
Expected a statement here.
(parse)
[error] 447-482: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
| it('should delete correct number of devices', async () => { | ||
| setup('WEB'); | ||
| const response = { | ||
| body: { | ||
| devices: [ | ||
| {url: 'url3', modificationTime: '2023-10-03T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url4', modificationTime: '2023-10-04T10:00:00Z', deviceType: 'notweb'}, | ||
| {url: 'url1', modificationTime: '2023-10-01T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url2', modificationTime: '2023-10-02T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url5', modificationTime: '2023-10-00T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url6', modificationTime: '2023-09-50T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url7', modificationTime: '2023-09-30T10:00:00Z', deviceType: 'WEB'}, | ||
| {url: 'url8', modificationTime: '2023-08-30T10:00:00Z', deviceType: 'WEB'}, | ||
| ] | ||
| } | ||
| }; | ||
| const requestStub = sinon.stub(device, 'request'); | ||
| requestStub.withArgs(sinon.match({method: 'GET'})).resolves(response); | ||
| requestStub.withArgs(sinon.match({method: 'DELETE'})).resolves(); | ||
|
|
||
| await device.deleteDevices(); | ||
|
|
||
| const expectedDeletions = ['url8', 'url7', 'url1']; | ||
|
|
||
| expectedDeletions.forEach(url => { | ||
| assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'}))); | ||
| }); | ||
|
|
||
| const notDeletedUrls = ['url2', 'url3', 'url5', 'url6', 'url4']; | ||
| notDeletedUrls.forEach(url => { | ||
| assert(requestStub.neverCalledWith(sinon.match({uri: url, method: 'DELETE'}))); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate test cases using parameterized tests
The test cases for 'WEB' and 'WEBCLIENT' device types are nearly identical in structure and assertions. Consider using parameterized tests to reduce duplication and improve maintainability.
- it('should delete correct number of devices', async () => {
- setup('WEB');
- const response = {
- body: {
- devices: [
- {url: 'url3', modificationTime: '2023-10-03T10:00:00Z', deviceType: 'WEB'},
- // ... more devices
- ]
- }
- };
- // ... test implementation
- });
-
- it('should delete correct number of devices when deviceType in config WEBCLIENT', async () => {
- setup('WEBCLIENT');
- const response = {
- body: {
- devices: [
- {url: 'url3', modificationTime: '2023-10-03T10:00:00Z', deviceType: 'WEBCLIENT'},
- // ... more devices
- ]
- }
- };
- // ... same test implementation
- });
+ ['WEB', 'WEBCLIENT'].forEach(deviceType => {
+ it(`should delete correct number of devices for ${deviceType}`, async () => {
+ setup(deviceType);
+ const response = {
+ body: {
+ devices: [
+ {url: 'url3', modificationTime: '2023-10-03T10:00:00Z', deviceType},
+ // ... more devices with dynamic deviceType
+ ]
+ }
+ };
+ // ... test implementation
+ });
+ });Also applies to: 398-430
| const expectedDeletions = ['url8', 'url7', 'url1']; | ||
|
|
||
| expectedDeletions.forEach(url => { | ||
| assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'}))); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for multiple device deletions
The test data includes 8 devices but only verifies deletion of a single device. Expand the test to verify that multiple devices (oldest third) are actually deleted.
- const expectedDeletions = ['url8', 'url7', 'url1'];
-
- expectedDeletions.forEach(url => {
- assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'})));
- });
+ const expectedDeletions = ['url8', 'url7', 'url6']; // Oldest third (3 devices)
+
+ // Verify all expected deletions occurred
+ assert.equal(requestStub.callCount, expectedDeletions.length, 'Should delete exactly 3 devices');
+
+ expectedDeletions.forEach(url => {
+ assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'})));
+ });📝 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.
| const expectedDeletions = ['url8', 'url7', 'url1']; | |
| expectedDeletions.forEach(url => { | |
| assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'}))); | |
| }); | |
| const expectedDeletions = ['url8', 'url7', 'url6']; // Oldest third (3 devices) | |
| // Verify all expected deletions occurred | |
| assert.equal(requestStub.callCount, expectedDeletions.length, 'Should delete exactly 3 devices'); | |
| expectedDeletions.forEach(url => { | |
| assert(requestStub.calledWith(sinon.match({uri: url, method: 'DELETE'}))); | |
| }); |
COMPLETES
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-584363
Added a method deleteDevices which is called when register fails.
This is done to tackle failures where stale device registrations don't get deleted. This is possible when user just closes the browser instead of leaving a meeting.
Added logic where register is tried once more after failure.
Summary by CodeRabbit
New Features
Bug Fixes
Tests