allow sites and devices to belong to more than one Group #4038
Conversation
|
Warning Rate limit exceeded@Baalmart has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve renaming the Changes
Poem
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 #4038 +/- ##
===========================================
- Coverage 11.73% 11.73% -0.01%
===========================================
Files 114 114
Lines 15331 15349 +18
Branches 319 319
===========================================
+ Hits 1799 1801 +2
- Misses 13532 13548 +16
|
|
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: 1
🧹 Outside diff range and nitpick comments (9)
src/device-registry/models/Airqloud.js (1)
116-117: LGTM! Consider adding validation for groupsThe groups field implementation is consistent with other models. Consider adding validation to ensure group values are unique within the array.
groups: { type: [String], trim: true, + validate: { + validator: function(v) { + return new Set(v).size === v.length; + }, + message: 'Duplicate group values are not allowed' + } },src/device-registry/models/Location.js (1)
87-90: Consider adding validation for the groups arrayWhile the schema change supports multiple groups, consider adding validation to ensure data integrity:
- Minimum/maximum array length constraints
- String format validation for group identifiers
- Default empty array value
groups: { type: [String], trim: true, + default: [], + validate: { + validator: function(v) { + return v.every(group => /^[a-zA-Z0-9_-]+$/.test(group)); + }, + message: props => 'Group identifiers must contain only alphanumeric characters, underscores, and hyphens' + } },src/device-registry/models/Cohort.js (1)
253-253: LGTM! Consider indexing for performanceThe groups field is correctly mapped in the device response. Given that this field will be frequently queried, consider adding an index for it.
cohortSchema.index({ geoHash: 1 }); +cohortSchema.index({ groups: 1 });src/device-registry/models/Photo.js (1)
Line range hint
86-90: Fix duplicate image_url field in toJSONWhile the groups field is correctly added, there's an unrelated issue: image_url appears twice in the output (lines 86 and 87).
return { image_url: this.image_url, metadata: this.metadata, id: this._id, tags: this.tags, name: this.name, network: this.network, groups: this.groups, - image_url: this.image_url, device_id: this.device_id,src/device-registry/models/Device.js (1)
77-79: Consider removing trim from groups array fieldThe
trimoption on the array field may not work as expected since it applies to the array itself rather than the individual string elements.groups: { type: [String], - trim: true, },Consider validating and trimming individual strings during input processing if needed.
src/device-registry/validators/device.validators.js (2)
129-137: Consider adding validation for individual group stringsWhile the array validation is good, consider adding checks for individual string elements to ensure they meet your requirements (e.g., non-empty strings, format, length).
body("groups") .optional() .custom((value) => { - return Array.isArray(value); + return Array.isArray(value) && value.every(item => + typeof item === 'string' && item.trim().length > 0 + ); }) - .withMessage("the groups should be an array") + .withMessage("groups should be an array of non-empty strings") .bail() .notEmpty() .withMessage("the groups should not be empty"),
396-404: Maintain consistency with create validationWhen implementing the suggested improvements for create validation, ensure the same validation logic is applied here for consistency.
src/device-registry/models/Site.js (2)
78-80: Remove trim from groups array field for consistencySimilar to the Device model, the
trimoption on the array field should be removed as it won't work as expected on the array type.groups: { type: [String], - trim: true, },
367-379: Consider moving checkDuplicates to a shared utilityThe
checkDuplicatesfunction is identical in both Device and Site models. Consider moving it to a shared utility file to maintain DRY principles.Consider creating a new utility file (e.g.,
src/device-registry/utils/array-validators.js) and moving the function there:// src/device-registry/utils/array-validators.js const { HttpError } = require("@utils/errors"); const httpStatus = require("http-status"); exports.checkDuplicates = (arr, fieldName) => { const duplicateValues = arr.filter( (value, index, self) => self.indexOf(value) !== index ); if (duplicateValues.length > 0) { return new HttpError( `Duplicate values found in ${fieldName} array.`, httpStatus.BAD_REQUEST ); } return null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
src/device-registry/config/global/db-projections.js(6 hunks)src/device-registry/models/Activity.js(2 hunks)src/device-registry/models/Airqloud.js(2 hunks)src/device-registry/models/Cohort.js(7 hunks)src/device-registry/models/Device.js(5 hunks)src/device-registry/models/Grid.js(3 hunks)src/device-registry/models/Location.js(5 hunks)src/device-registry/models/Photo.js(3 hunks)src/device-registry/models/Site.js(5 hunks)src/device-registry/routes/v2/sites.js(2 hunks)src/device-registry/utils/generate-filter.js(9 hunks)src/device-registry/validators/device.validators.js(2 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- src/device-registry/routes/v2/sites.js
- src/device-registry/config/global/db-projections.js
- src/device-registry/utils/generate-filter.js
🔇 Additional comments (16)
src/device-registry/models/Activity.js (2)
73-73: LGTM! Consistent toJSON update
The toJSON method correctly reflects the schema change from 'group' to 'groups'.
43-44: LGTM! Schema change for multiple group support
The change from single group to groups array is well-implemented with appropriate type definition and trim support.
Let's verify the schema migration impact:
✅ Verification successful
The previous search patterns might have been too strict. Let's broaden the search to catch any variations in the schema definition and also look for related migrations to understand the schema change better.
Schema change for multiple groups is consistently implemented across models
The verification shows that the groups field with type [String] and trim: true is consistently implemented across all relevant models:
- Activity.js
- Airqloud.js
- Cohort.js
- Device.js
- Grid.js
- Location.js
- Photo.js
- Site.js
No instances of the old single group field were found, indicating a complete migration to the new array-based schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining single 'group' field references
rg -l 'group: \{.*type: String' src/device-registry/models/
# Check for consistent array type usage
rg -l 'groups: \{.*type: \[String\]' src/device-registry/models/
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search for group/groups fields in the schema
rg -A 2 -B 2 'group[s]?:\s*\{' src/device-registry/models/
# Also search for any group-related migrations
fd migration src/device-registry/ --exec rg -l 'group'
Length of output: 4655
src/device-registry/models/Grid.js (2)
46-47: LGTM! Consistent schema implementation
The groups field implementation maintains consistency with other models in the system.
124-124: LGTM! Well-structured toJSON update
The toJSON method properly includes the groups field in both destructuring and return statement.
Also applies to: 142-142
src/device-registry/models/Airqloud.js (1)
155-155: LGTM! Consistent toJSON implementation
The groups field is properly included in the toJSON output, maintaining consistent field ordering with other models.
src/device-registry/models/Location.js (1)
131-131: LGTM!
The toJSON method correctly includes the new groups field in the serialized output.
src/device-registry/models/Cohort.js (3)
20-22: Apply consistent validation across models
Similar to the Location model, consider adding validation for the groups array here.
84-84: LGTM!
The toJSON method correctly handles the new groups field in both destructuring and return value.
Also applies to: 95-95
205-205: LGTM!
The aggregation pipeline correctly projects the groups field.
src/device-registry/models/Photo.js (2)
21-24: Apply consistent validation across models
Similar to the Location and Cohort models, consider adding validation for the groups array here.
161-161: LGTM!
The groups field is correctly included in the aggregation projection.
src/device-registry/models/Device.js (3)
240-252: Well-implemented duplicate check utility!
The checkDuplicates function is clean, reusable, and handles error cases appropriately.
343-352: Good validation implementation in pre-save middleware!
The duplicate checks for both cohorts and groups are properly implemented with clear error handling.
386-399: Excellent handling of array updates!
The use of $addToSet for the groups field ensures no duplicates during updates while maintaining consistency with other array fields.
src/device-registry/models/Site.js (2)
427-429: Good consistency in array field handling!
The addition of groups to arrayFieldsToAddToSet maintains consistency with the Device model and ensures proper handling of array updates.
466-469: Well-implemented groups validation in pre-save middleware!
The duplicate checking for groups is properly implemented with clear error handling, maintaining consistency with the rest of the codebase.
| @@ -200,6 +206,7 @@ locationSchema.statics = { | |||
| admin_level: 1, | |||
| description: 1, | |||
| network: 1, | |||
| group: 1, | |||
There was a problem hiding this comment.
Critical: Fix inconsistent field name in projectSummary
The projection still uses the old field name 'group' instead of 'groups', which could cause data inconsistency in summary responses.
- group: 1,
+ groups: 1,📝 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.
| group: 1, | |
| groups: 1, |
|
Device registry changes in this PR available for preview here |
1 similar comment
|
Device registry changes in this PR available for preview here |
Description
allow sites and devices to belong to more than one Group
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
Request Body:
API Documentation Updated?
Additional Notes
allow sites and devices to belong to more than one Group
Summary by CodeRabbit
New Features
groupsparameter for creating and updating sites.groupsfield in device creation and update.Bug Fixes
groupsinstead ofgroup.Documentation
groupsfield across multiple models.Chores