Hotfixes of metadata for Devices and Sites #4104
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the device registry system, focusing on improving data integrity and management. The changes include a new scheduled job for updating duplicate site fields, modifications to device and site models to enhance validation and update processes, and the integration of a new job into the server configuration. The primary goal is to ensure data consistency and provide more robust handling of site and device records. Changes
Sequence DiagramsequenceDiagram
participant Job as Duplicate Site Fields Job
participant SitesModel as Sites Database
participant UpdateProcess as Update Processor
Job->>SitesModel: Fetch Active Sites
SitesModel-->>Job: Return Site Records
Job->>Job: Group Sites by Duplicate Fields
Job->>UpdateProcess: Generate Unique Field Values
UpdateProcess->>SitesModel: Perform Bulk Updates
SitesModel-->>Job: Update Confirmation
Job->>Job: Log Update Report
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 #4104 +/- ##
===========================================
+ Coverage 11.81% 11.85% +0.04%
===========================================
Files 115 116 +1
Lines 15485 15576 +91
Branches 319 319
===========================================
+ Hits 1829 1847 +18
- Misses 13656 13729 +73
|
|
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: 0
🧹 Nitpick comments (5)
src/device-registry/bin/jobs/update-duplicate-site-fields-job.js (2)
91-104: Validate filtering for "active" sites.
The code uses { isOnline: true } to filter active sites. If your definition of “active” changes or includes additional predicates, it may be beneficial to centralize this into a helper method for consistency.
150-155: Cron scheduling feedback.
Running every 4 hours at minute 15 is generally fine. However, if the dataset grows or the job becomes more resource-intensive, you may consider adjusting frequency or implementing more fine-grained scheduling.src/device-registry/models/Device.js (2)
273-278: Use 'undefined' assignment instead of 'delete' for performance.
Static analysis highlights the potential performance overhead of the delete operator. Consider setting the field to undefined instead, which often proves faster and safer in V8.- if (this.category === null) { - delete this.category; - } else if ( + if (this.category === null) { + this.category = undefined; + } else if ( !DEVICE_CONFIG.ALLOWED_CATEGORIES.includes(this.category)🧰 Tools
🪛 Biome (1.9.4)
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
295-297: Refactor removing 'category' field on updates.
Similar to line 278, consider assigning to undefined rather than deleting to align with best practices.- if (updateData.category === null) { - delete updateData.category; - } else if ( + if (updateData.category === null) { + updateData.category = undefined; + } else if ( !DEVICE_CONFIG.ALLOWED_CATEGORIES.includes(updateData.category)🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/device-registry/models/Site.js (1)
396-420: Optional chaining for improved readability.
The repeated checks for existence in lines 398, 403, 404-406, and 407 can be made more concise and safe using optional chaining (e.g., updates?.$set?.groups) if you’re comfortable with that style.- const hasExplicitDataProvider = - updates.data_provider || (updates.$set && updates.$set.data_provider); + const hasExplicitDataProvider = + updates.data_provider || updates?.$set?.data_provider;(and similarly for other lines)
🧰 Tools
🪛 Biome (1.9.4)
[error] 398-398: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 404-406: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 407-407: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/device-registry/bin/jobs/update-duplicate-site-fields-job.js(1 hunks)src/device-registry/bin/server.js(1 hunks)src/device-registry/models/Device.js(2 hunks)src/device-registry/models/Site.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/device-registry/models/Device.js
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/device-registry/models/Site.js
[error] 398-398: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 404-406: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 407-407: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/device-registry/bin/jobs/update-duplicate-site-fields-job.js (2)
58-68: Consider concurrency and validation for net-new fieldValue updates.
While building updates, ensure other concurrent jobs aren’t updating the same documents, which might cause race conditions or partial updates.
17-20: Consider more robust pattern validation.
Currently, the regex only captures "site_123" patterns. If there's a possibility of uppercase, extra dashes, or other numeric prefixes, consider expanding the pattern or adding fallback logic.
Do you need a more flexible or strict pattern? Let’s confirm by searching for additional naming patterns elsewhere in the code.
✅ Verification successful
Let me search for the actual site name patterns in the codebase to understand the naming conventions.
Let me check how the site names are generated to understand the pattern requirements better.
Current site name pattern is sufficient and correct
Based on the codebase analysis, the current regex pattern /site_(\d+)/ is actually appropriate because:
- Site names are systematically generated using a counter starting from "site_0"
- The pattern is consistently used across the codebase with lowercase "site_" prefix followed by numbers
- The generation is controlled through the
UniqueIdentifierCounterModel, ensuring standardized naming
The current implementation correctly handles the only format that can exist in the system, making additional pattern flexibility unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to "extractSiteNumber" usage
rg -A 5 $'extractSiteNumber'
Length of output: 1468
Script:
#!/bin/bash
# Search for site name patterns in the codebase
rg -i "site_\d+" --type js -A 2 -B 2
# Also search for generated_name assignments or declarations
ast-grep --pattern 'generated_name = $_'
Length of output: 2326
Script:
#!/bin/bash
# Check the site name generation logic
rg -A 10 "generateName" src/device-registry/utils/create-site.js
# Also check for any site name validation
rg -i "site.*name.*validation" --type js
Length of output: 1313
src/device-registry/models/Device.js (1)
280-310: Ensure consistent error and field handling.
This logic properly enforces valid categories, default networks, and checks the AirQo constraints. Keep an eye on whether future custom categories or networks might need additional exception handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/device-registry/bin/server.js (1)
33-33: Importing the new job looks good.
This ensures that the duplicate site fields job is available and scheduled. Great job expanding your overall data integrity strategy by integrating the job with your existing environment.
Description
Hotfixes of metadata for Devices and Sites
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit
New Features
categoryfield in device records, ensuring data integrity and validation.data_providerfield in site records, streamlining the update process.Bug Fixes
gridsandgroupsfields during updates.Documentation