just a small hotfix for device updates #3943
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3943 +/- ##
===========================================
- Coverage 11.74% 11.73% -0.02%
===========================================
Files 114 114
Lines 15317 15318 +1
Branches 318 318
===========================================
- Hits 1799 1797 -2
- Misses 13518 13521 +3
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/device-registry/models/Device.js (1)
292-298: Avoid code duplication by extractingsanitizeNamefunctionThe
sanitizeNamefunction is defined twice within theprehook. To improve maintainability and readability, consider defining it once outside the conditional blocks to avoid duplication.Apply this diff to extract the function:
+ // Define sanitizeName function outside of conditional blocks + const sanitizeName = (name) => { + return name + .replace(/[^a-zA-Z0-9]/g, "_") + .slice(0, 41) + .trim() + .toLowerCase(); + }; if (isNew) { - // Sanitize name - const sanitizeName = (name) => { - return name - .replace(/[^a-zA-Z0-9]/g, "_") - .slice(0, 41) - .trim() - .toLowerCase(); - }; if (this.name) { this.name = sanitizeName(this.name); } else if (this.long_name) { this.name = sanitizeName(this.long_name); } // Other code... } // Later in the code if (updateData.name) { - const sanitizeName = (name) => { - return name - .replace(/[^a-zA-Z0-9]/g, "_") - .slice(0, 41) - .trim() - .toLowerCase(); - }; updateData.name = sanitizeName(updateData.name); }Also applies to: 356-361
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 292-293: src/device-registry/models/Device.js#L292-L293
Added lines #L292 - L293 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/device-registry/models/Device.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/device-registry/models/Device.js
[error] 379-379: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 384-384: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 389-389: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
src/device-registry/models/Device.js
[warning] 250-250: src/device-registry/models/Device.js#L250
Added line #L250 was not covered by tests
[warning] 252-253: src/device-registry/models/Device.js#L252-L253
Added lines #L252 - L253 were not covered by tests
[warning] 255-255: src/device-registry/models/Device.js#L255
Added line #L255 was not covered by tests
[warning] 257-258: src/device-registry/models/Device.js#L257-L258
Added lines #L257 - L258 were not covered by tests
[warning] 262-265: src/device-registry/models/Device.js#L262-L265
Added lines #L262 - L265 were not covered by tests
[warning] 274-275: src/device-registry/models/Device.js#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 279-282: src/device-registry/models/Device.js#L279-L282
Added lines #L279 - L282 were not covered by tests
[warning] 292-293: src/device-registry/models/Device.js#L292-L293
Added lines #L292 - L293 were not covered by tests
[warning] 300-303: src/device-registry/models/Device.js#L300-L303
Added lines #L300 - L303 were not covered by tests
[warning] 307-308: src/device-registry/models/Device.js#L307-L308
Added lines #L307 - L308 were not covered by tests
[warning] 312-314: src/device-registry/models/Device.js#L312-L314
Added lines #L312 - L314 were not covered by tests
[warning] 318-320: src/device-registry/models/Device.js#L318-L320
Added lines #L318 - L320 were not covered by tests
[warning] 322-323: src/device-registry/models/Device.js#L322-L323
Added lines #L322 - L323 were not covered by tests
[warning] 325-326: src/device-registry/models/Device.js#L325-L326
Added lines #L325 - L326 were not covered by tests
[warning] 330-331: src/device-registry/models/Device.js#L330-L331
Added lines #L330 - L331 were not covered by tests
[warning] 334-335: src/device-registry/models/Device.js#L334-L335
Added lines #L334 - L335 were not covered by tests
[warning] 345-349: src/device-registry/models/Device.js#L345-L349
Added lines #L345 - L349 were not covered by tests
[warning] 354-356: src/device-registry/models/Device.js#L354-L356
Added lines #L354 - L356 were not covered by tests
[warning] 362-362: src/device-registry/models/Device.js#L362
Added line #L362 was not covered by tests
[warning] 366-367: src/device-registry/models/Device.js#L366-L367
Added lines #L366 - L367 were not covered by tests
[warning] 371-371: src/device-registry/models/Device.js#L371
Added line #L371 was not covered by tests
[warning] 375-375: src/device-registry/models/Device.js#L375
Added line #L375 was not covered by tests
[warning] 377-379: src/device-registry/models/Device.js#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 382-384: src/device-registry/models/Device.js#L382-L384
Added lines #L382 - L384 were not covered by tests
[warning] 387-389: src/device-registry/models/Device.js#L387-L389
Added lines #L387 - L389 were not covered by tests
[warning] 393-394: src/device-registry/models/Device.js#L393-L394
Added lines #L393 - L394 were not covered by tests
[warning] 397-397: src/device-registry/models/Device.js#L397
Added line #L397 was not covered by tests
[warning] 399-399: src/device-registry/models/Device.js#L399
Added line #L399 was not covered by tests
[warning] 596-596: src/device-registry/models/Device.js#L596
Added line #L596 was not covered by tests
| try { | ||
| // Determine if this is a new document or an update | ||
| const isNew = this.isNew; | ||
| const updateData = this.getUpdate ? this.getUpdate() : this; | ||
|
|
There was a problem hiding this comment.
Ensure correct use of this.isNew and this.getUpdate() in pre-hooks
In Mongoose query middleware (e.g., pre-hooks for 'update' operations), this refers to the query object, and properties like isNew are not available. Similarly, in document middleware, methods like getUpdate() are not present. Combining both query and document middleware in a single hook may lead to unexpected behavior. Consider separating the middleware for query and document operations or adjusting the logic to handle each case appropriately.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 250-250: src/device-registry/models/Device.js#L250
Added line #L250 was not covered by tests
[warning] 252-253: src/device-registry/models/Device.js#L252-L253
Added lines #L252 - L253 were not covered by tests
| } | ||
| } | ||
|
|
||
| deviceSchema.pre( | ||
| ["update", "findByIdAndUpdate", "updateMany", "updateOne"], | ||
| function(next) { | ||
| // Enable validation on update | ||
| this.setOptions({ runValidators: true }); | ||
|
|
||
| const updateData = this.getUpdate(); // Get the data being updated | ||
|
|
||
| // Handling the network condition | ||
| if (updateData.network === "airqo") { | ||
| if (updateData.device_number) { | ||
| updateData.serial_number = String(updateData.device_number); | ||
| } else if (updateData.serial_number) { | ||
| updateData.device_number = Number(updateData.serial_number); | ||
| // Sanitize name if modified | ||
| if (updateData.name) { | ||
| const sanitizeName = (name) => { | ||
| return name | ||
| .replace(/[^a-zA-Z0-9]/g, "_") | ||
| .slice(0, 41) | ||
| .trim() | ||
| .toLowerCase(); | ||
| }; | ||
| updateData.name = sanitizeName(updateData.name); | ||
| } | ||
| } | ||
|
|
||
| // Sanitize name if modified | ||
| if (updateData.name) { | ||
| const sanitizeName = (name) => { | ||
| return name | ||
| .replace(/[^a-zA-Z0-9]/g, "_") | ||
| .slice(0, 41) | ||
| .trim() | ||
| .toLowerCase(); | ||
| }; | ||
| updateData.name = sanitizeName(updateData.name); | ||
| } | ||
| // Generate access code if present in update | ||
| if (updateData.access_code) { | ||
| const access_code = accessCodeGenerator.generate({ | ||
| length: 16, | ||
| excludeSimilarCharacters: true, | ||
| }); | ||
| updateData.access_code = access_code.toUpperCase(); | ||
| } | ||
|
|
||
| // Generate access code if present in update | ||
| if (updateData.access_code) { | ||
| const access_code = accessCodeGenerator.generate({ | ||
| length: 16, | ||
| excludeSimilarCharacters: true, | ||
| }); | ||
| updateData.access_code = access_code.toUpperCase(); | ||
| } | ||
| // Handle $addToSet for device_codes, previous_sites, and pictures | ||
| const addToSetUpdates = {}; | ||
|
|
||
| // Handle $addToSet for device_codes, previous_sites, and pictures | ||
| const addToSetUpdates = {}; | ||
| if (updateData.device_codes) { | ||
| addToSetUpdates.device_codes = { $each: updateData.device_codes }; | ||
| delete updateData.device_codes; // Remove from main update object | ||
| } | ||
|
|
||
| if (updateData.device_codes) { | ||
| addToSetUpdates.device_codes = { $each: updateData.device_codes }; | ||
| delete updateData.device_codes; // Remove from main update object | ||
| } | ||
| if (updateData.previous_sites) { | ||
| addToSetUpdates.previous_sites = { $each: updateData.previous_sites }; | ||
| delete updateData.previous_sites; // Remove from main update object | ||
| } | ||
|
|
||
| if (updateData.previous_sites) { | ||
| addToSetUpdates.previous_sites = { $each: updateData.previous_sites }; | ||
| delete updateData.previous_sites; // Remove from main update object | ||
| } | ||
| if (updateData.pictures) { | ||
| addToSetUpdates.pictures = { $each: updateData.pictures }; | ||
| delete updateData.pictures; // Remove from main update object | ||
| } | ||
|
|
||
| if (updateData.pictures) { | ||
| addToSetUpdates.pictures = { $each: updateData.pictures }; | ||
| delete updateData.pictures; // Remove from main update object | ||
| } | ||
| // If there are any $addToSet updates, merge them into the main update object | ||
| if (Object.keys(addToSetUpdates).length > 0) { | ||
| updateData.$addToSet = addToSetUpdates; | ||
| } | ||
|
|
||
| // If there are any $addToSet updates, merge them into the main update object | ||
| if (Object.keys(addToSetUpdates).length > 0) { | ||
| updateData.$addToSet = addToSetUpdates; | ||
| next(); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for new pre hook logic
The new pre hook introduces significant logic for handling document creation and updates. To ensure the code behaves as expected and to prevent future regressions, please add unit tests that cover the new scenarios and code paths.
Would you like assistance in creating test cases for this new functionality?
🧰 Tools
🪛 Biome (1.9.4)
[error] 379-379: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 384-384: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 389-389: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
[warning] 250-250: src/device-registry/models/Device.js#L250
Added line #L250 was not covered by tests
[warning] 252-253: src/device-registry/models/Device.js#L252-L253
Added lines #L252 - L253 were not covered by tests
[warning] 255-255: src/device-registry/models/Device.js#L255
Added line #L255 was not covered by tests
[warning] 257-258: src/device-registry/models/Device.js#L257-L258
Added lines #L257 - L258 were not covered by tests
[warning] 262-265: src/device-registry/models/Device.js#L262-L265
Added lines #L262 - L265 were not covered by tests
[warning] 274-275: src/device-registry/models/Device.js#L274-L275
Added lines #L274 - L275 were not covered by tests
[warning] 279-282: src/device-registry/models/Device.js#L279-L282
Added lines #L279 - L282 were not covered by tests
[warning] 292-293: src/device-registry/models/Device.js#L292-L293
Added lines #L292 - L293 were not covered by tests
[warning] 300-303: src/device-registry/models/Device.js#L300-L303
Added lines #L300 - L303 were not covered by tests
[warning] 307-308: src/device-registry/models/Device.js#L307-L308
Added lines #L307 - L308 were not covered by tests
[warning] 312-314: src/device-registry/models/Device.js#L312-L314
Added lines #L312 - L314 were not covered by tests
[warning] 318-320: src/device-registry/models/Device.js#L318-L320
Added lines #L318 - L320 were not covered by tests
[warning] 322-323: src/device-registry/models/Device.js#L322-L323
Added lines #L322 - L323 were not covered by tests
[warning] 325-326: src/device-registry/models/Device.js#L325-L326
Added lines #L325 - L326 were not covered by tests
[warning] 330-331: src/device-registry/models/Device.js#L330-L331
Added lines #L330 - L331 were not covered by tests
[warning] 334-335: src/device-registry/models/Device.js#L334-L335
Added lines #L334 - L335 were not covered by tests
[warning] 345-349: src/device-registry/models/Device.js#L345-L349
Added lines #L345 - L349 were not covered by tests
[warning] 354-356: src/device-registry/models/Device.js#L354-L356
Added lines #L354 - L356 were not covered by tests
[warning] 362-362: src/device-registry/models/Device.js#L362
Added line #L362 was not covered by tests
[warning] 366-367: src/device-registry/models/Device.js#L366-L367
Added lines #L366 - L367 were not covered by tests
[warning] 371-371: src/device-registry/models/Device.js#L371
Added line #L371 was not covered by tests
[warning] 375-375: src/device-registry/models/Device.js#L375
Added line #L375 was not covered by tests
[warning] 377-379: src/device-registry/models/Device.js#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 382-384: src/device-registry/models/Device.js#L382-L384
Added lines #L382 - L384 were not covered by tests
[warning] 387-389: src/device-registry/models/Device.js#L387-L389
Added lines #L387 - L389 were not covered by tests
[warning] 393-394: src/device-registry/models/Device.js#L393-L394
Added lines #L393 - L394 were not covered by tests
[warning] 397-397: src/device-registry/models/Device.js#L397
Added line #L397 was not covered by tests
Description
just a small hotfix for device updates
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
just a small hotfix for device updates
Summary by CodeRabbit
New Features
Bug Fixes