just adding the query parameters for site and device online status #3489
Conversation
WalkthroughWalkthroughThe changes in this pull request enhance the device and site listing functionalities by introducing new query parameters for filtering. Specifically, the Changes
Possibly related PRs
Suggested reviewers
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 #3489 +/- ##
===========================================
- Coverage 30.31% 30.26% -0.06%
===========================================
Files 185 185
Lines 24708 24750 +42
Branches 3228 3240 +12
===========================================
Hits 7491 7491
- Misses 17102 17136 +34
- Partials 115 123 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
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: 15
Outside diff range and nitpick comments (1)
src/device-registry/routes/v2/sites.js (1)
142-152: Ensure Consistent Validation MessagesThe validation messages for
online_statususe slightly different phrasing. For consistency and clarity, consider standardizing the messages across all parameters. For example, ensure that messages consistently state whether the field "cannot be empty if provided" or "should not be empty if provided."Also applies to: 224-234
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/device-registry/routes/v2/devices.js (1 hunks)
- src/device-registry/routes/v2/sites.js (2 hunks)
- src/device-registry/utils/generate-filter.js (4 hunks)
Additional context used
GitHub Check: codecov/patch
src/device-registry/routes/v2/sites.js
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-136: src/device-registry/routes/v2/sites.js#L135-L136
Added lines #L135 - L136 were not covered by tests
[warning] 206-207: src/device-registry/routes/v2/sites.js#L206-L207
Added lines #L206 - L207 were not covered by tests
[warning] 216-217: src/device-registry/routes/v2/sites.js#L216-L217
Added lines #L216 - L217 were not covered by testssrc/device-registry/utils/generate-filter.js
[warning] 933-935: src/device-registry/utils/generate-filter.js#L933-L935
Added lines #L933 - L935 were not covered by tests
[warning] 939-941: src/device-registry/utils/generate-filter.js#L939-L941
Added lines #L939 - L941 were not covered by tests
[warning] 945-947: src/device-registry/utils/generate-filter.js#L945-L947
Added lines #L945 - L947 were not covered by tests
[warning] 952-952: src/device-registry/utils/generate-filter.js#L952
Added line #L952 was not covered by tests
[warning] 954-954: src/device-registry/utils/generate-filter.js#L954
Added line #L954 was not covered by tests
[warning] 1116-1118: src/device-registry/utils/generate-filter.js#L1116-L1118
Added lines #L1116 - L1118 were not covered by tests
[warning] 1122-1124: src/device-registry/utils/generate-filter.js#L1122-L1124
Added lines #L1122 - L1124 were not covered by tests
[warning] 1128-1130: src/device-registry/utils/generate-filter.js#L1128-L1130
Added lines #L1128 - L1130 were not covered by tests
[warning] 1135-1135: src/device-registry/utils/generate-filter.js#L1135
Added line #L1135 was not covered by tests
[warning] 1137-1137: src/device-registry/utils/generate-filter.js#L1137
Added line #L1137 was not covered by tests
Additional comments not posted (2)
src/device-registry/routes/v2/devices.js (1)
404-441: LGTM! Approve changes with a note on documentationThe addition of these new query parameters is a well-implemented enhancement to the device listing endpoint. The changes are consistent with the existing code style and maintain backwards compatibility.
As a final note, ensure that these new filtering options are properly documented in the API documentation. This will help API consumers understand and utilize these new capabilities effectively.
src/device-registry/routes/v2/sites.js (1)
125-127: Verify Handling of ConvertedObjectIdTypes in Application LogicAfter sanitizing
idandsite_idtoObjectId, ensure that the rest of the application logic correctly handles these fields asObjectIdtypes rather than strings. This will prevent potential type-related issues when interacting with the database or other parts of the system.Also applies to: 135-137, 206-208, 216-218
Tools
GitHub Check: codecov/patch
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
| query("online_status") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the online_status should not be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["online", "offline"]) | ||
| .withMessage( | ||
| "the online_status value is not among the expected ones which include: online, offline" | ||
| ), | ||
| query("last_active_before") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_before date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active_before date must be a valid datetime."), | ||
| query("last_active_after") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_after date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active_after date must be a valid datetime."), | ||
| query("last_active") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active date must be a valid datetime."), |
There was a problem hiding this comment.
Excellent addition of new query parameters for device filtering!
The new query parameters online_status, last_active_before, last_active_after, and last_active provide valuable filtering options for the device listing endpoint. These additions will greatly enhance the API's flexibility and usefulness. A few observations:
- The
online_statusparameter is well-validated, ensuring it's either "online" or "offline". - The date-related parameters (
last_active_before,last_active_after, andlast_active) are properly validated as ISO8601 dates. - All new parameters are optional, which is a good practice for backwards compatibility.
One suggestion to consider:
Consider adding a custom error message for the date format validation. This would provide more specific feedback to API users. For example:
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty IF provided")
.bail()
.trim()
.toDate()
.isISO8601({ strict: true, strictSeparator: true })
- .withMessage("last_active_before date must be a valid datetime."),
+ .withMessage("last_active_before must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."),Apply this change to last_active_after and last_active as well for consistency.
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.
| query("online_status") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("the online_status should not be empty if provided") | |
| .bail() | |
| .trim() | |
| .toLowerCase() | |
| .isIn(["online", "offline"]) | |
| .withMessage( | |
| "the online_status value is not among the expected ones which include: online, offline" | |
| ), | |
| query("last_active_before") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active_before date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active_before date must be a valid datetime."), | |
| query("last_active_after") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active_after date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active_after date must be a valid datetime."), | |
| query("last_active") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active date must be a valid datetime."), | |
| query("online_status") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("the online_status should not be empty if provided") | |
| .bail() | |
| .trim() | |
| .toLowerCase() | |
| .isIn(["online", "offline"]) | |
| .withMessage( | |
| "the online_status value is not among the expected ones which include: online, offline" | |
| ), | |
| query("last_active_before") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active_before date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active_before must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."), | |
| query("last_active_after") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active_after date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active_after must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."), | |
| query("last_active") | |
| .optional() | |
| .notEmpty() | |
| .withMessage("last_active date cannot be empty IF provided") | |
| .bail() | |
| .trim() | |
| .toDate() | |
| .isISO8601({ strict: true, strictSeparator: true }) | |
| .withMessage("last_active must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."), |
Tip
Codebase Verification
Implementation Missing in Controller and Service Layers
The new query parameters (online_status, last_active_before, last_active_after, last_active) have been added to the route validations but are not handled in the controller and service layers. This discrepancy can lead to the API not processing the filters correctly.
-
Controller Layer:
- No implementations found to handle the new query parameters in
src/device-registry/controllers/.
- No implementations found to handle the new query parameters in
-
Service Layer:
- The
src/device-registry/services/directory is missing or does not contain relevant implementations for the new filters.
- The
Analysis chain
Verify implementation of new filters in controller and service layers
The addition of these new query parameters is a great improvement to the API's capabilities. To ensure full functionality, we should verify that the corresponding changes have been made in the controller and service layers.
Let's check the implementation of these new filters:
This script will help us confirm that the new filtering capabilities are properly implemented throughout the codebase.
Further Verification Needed for New Filter Implementations
The initial checks did not locate the implementation of the new query parameters in the expected directories. To ensure that the new filters are correctly integrated, let's perform a broader search across the entire codebase.
This script will help us identify where the new query parameters are implemented and ensure that all relevant parts of the codebase have been appropriately updated.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the implementation of new filters in the controller and service layers
# Test 1: Check for the use of new query parameters in the controller
echo "Checking controller implementation:"
rg --type js 'online_status|last_active_before|last_active_after|last_active' src/device-registry/controllers
# Test 2: Check for the implementation of filtering logic in the service layer
echo "Checking service layer implementation:"
rg --type js 'online_status|last_active_before|last_active_after|last_active' src/device-registry/services
# Test 3: Check if the generateFilter utility has been updated
echo "Checking generateFilter utility:"
rg --type js 'online_status|last_active_before|last_active_after|last_active' src/utils/generateFilter.js
Length of output: 708
Script:
#!/bin/bash
# Comprehensive search for new query parameters across the entire codebase
echo "🔍 Searching for new query parameters in all JavaScript files:"
rg --type js 'online_status|last_active_before|last_active_after|last_active' .
echo "🔍 Searching for updates in utility functions related to filtering:"
rg --type js 'generateFilter'
Length of output: 180838
| .customSanitizer((value) => { | ||
| return ObjectId(value); | ||
| }), |
There was a problem hiding this comment.
Add Unit Tests for Custom Sanitizers Converting to ObjectId
The custom sanitizers for id and site_id convert input values to ObjectId. Currently, these lines are not covered by tests. To ensure the sanitization logic works as intended and to prevent potential issues, please consider adding unit tests for these sanitizers.
Also applies to: 135-137, 206-208, 216-218
Tools
GitHub Check: codecov/patch
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
There was a problem hiding this comment.
Please create a new Github issue for this suggestion
There was a problem hiding this comment.
@Baalmart, I've successfully created the GitHub issue for this suggestion. You can find it here: #3490
Analysis chain
@Baalmart, sure, I'll open a new GitHub issue for this suggestion.
((\
( ^.^)
o_(")(")
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for adding unit tests for custom sanitizers
gh issue create \
--title "Add Unit Tests for Custom Sanitizers in 'sites.js'" \
--body "The custom sanitizers for \`id\` and \`site_id\` in \`sites.js\` convert input values to \`ObjectId\`. These lines are not currently covered by tests. To ensure the sanitization logic works as intended and to prevent potential issues, please consider adding unit tests for these sanitizers.
PR: https://github.com/airqo-platform/AirQo-api/pull/3489
Comment: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771370131
Requested by: @Baalmart" \
--assignee "Baalmart"
Length of output: 622
| query("last_active_before") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_before date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active_before date must be a valid datetime."), |
There was a problem hiding this comment.
Validate Date Format Before Converting With toDate() for last_active_before
In the validation chain for last_active_before, .toDate() is called before verifying if the input is a valid ISO8601 date using .isISO8601(). Converting before validation might lead to unexpected behavior if an invalid date is provided, as .toDate() could produce an Invalid Date object without raising an error.
Please rearrange the validation steps to check the date format before conversion:
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty if provided")
.bail()
.trim()
+ .isISO8601({ strict: true, strictSeparator: true })
+ .withMessage("last_active_before date must be a valid ISO8601 datetime.")
+ .bail()
.toDate()Also applies to: 235-243
| query("last_active_after") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_after date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active_after date must be a valid datetime."), |
There was a problem hiding this comment.
Validate Date Format Before Converting With toDate() for last_active_after
Similarly, for last_active_after, it's advisable to validate the date format before attempting to convert it:
query("last_active_after")
.optional()
.notEmpty()
.withMessage("last_active_after date cannot be empty if provided")
.bail()
.trim()
+ .isISO8601({ strict: true, strictSeparator: true })
+ .withMessage("last_active_after date must be a valid ISO8601 datetime.")
+ .bail()
.toDate()Also applies to: 244-252
| query("last_active") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toDate() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("last_active date must be a valid datetime."), |
There was a problem hiding this comment.
Validate Date Format Before Converting With toDate() for last_active
For the last_active parameter, ensure that you validate the date format before converting it to a Date object:
query("last_active")
.optional()
.notEmpty()
.withMessage("last_active date cannot be empty if provided")
.bail()
.trim()
+ .isISO8601({ strict: true, strictSeparator: true })
+ .withMessage("last_active date must be a valid ISO8601 datetime.")
+ .bail()
.toDate()Also applies to: 253-261
| if (last_active_before) { | ||
| filter.last_active_before = {}; | ||
| const start = new Date(last_active_before); | ||
| filter["lastActive"]["$lte"] = start; | ||
| } |
There was a problem hiding this comment.
Potential Typo: Incorrect Initialization in last_active_before Filter
Similarly, in line 1128, filter.last_active_before is initialized as an empty object, but in line 1130, you're assigning a condition to filter["lastActive"]["$lte"]. Consider initializing filter.lastActive instead, or remove the unnecessary initialization to correct this potential issue.
Suggested fix:
if (last_active_before) {
- filter.last_active_before = {};
+ filter.lastActive = filter.lastActive || {};
const start = new Date(last_active_before);
filter["lastActive"]["$lte"] = start;
}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.
| if (last_active_before) { | |
| filter.last_active_before = {}; | |
| const start = new Date(last_active_before); | |
| filter["lastActive"]["$lte"] = start; | |
| } | |
| if (last_active_before) { | |
| filter.lastActive = filter.lastActive || {}; | |
| const start = new Date(last_active_before); | |
| filter["lastActive"]["$lte"] = start; | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 1128-1130: src/device-registry/utils/generate-filter.js#L1128-L1130
Added lines #L1128 - L1130 were not covered by tests
| if (last_active_before) { | ||
| filter.last_active_before = {}; | ||
| const start = new Date(last_active_before); | ||
| filter["lastActive"]["$lte"] = start; | ||
| } |
There was a problem hiding this comment.
Potential Typo: Incorrect Initialization in last_active_before Filter
Similarly, in line 945, filter.last_active_before is initialized as an empty object, but in line 947, you're assigning a condition to filter["lastActive"]["$lte"]. The initialization of filter.last_active_before appears unnecessary or incorrect. Consider initializing filter.lastActive instead, or remove the unnecessary initialization.
Suggested fix:
if (last_active_before) {
- filter.last_active_before = {};
+ filter.lastActive = filter.lastActive || {};
const start = new Date(last_active_before);
filter["lastActive"]["$lte"] = start;
}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.
| if (last_active_before) { | |
| filter.last_active_before = {}; | |
| const start = new Date(last_active_before); | |
| filter["lastActive"]["$lte"] = start; | |
| } | |
| if (last_active_before) { | |
| filter.lastActive = filter.lastActive || {}; | |
| const start = new Date(last_active_before); | |
| filter["lastActive"]["$lte"] = start; | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 945-947: src/device-registry/utils/generate-filter.js#L945-L947
Added lines #L945 - L947 were not covered by tests
| if (last_active_after) { | ||
| filter.last_active_after = {}; | ||
| const start = new Date(last_active_after); | ||
| filter["lastActive"]["$gte"] = start; | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential Typo: Incorrect Initialization in last_active_after Filter
In line 1122, filter.last_active_after is initialized as an empty object, but in line 1124, you assign a condition to filter["lastActive"]["$gte"]. This initialization seems unnecessary or may be a typo. Consider initializing filter.lastActive instead, or remove the unnecessary initialization.
Suggested fix:
if (last_active_after) {
- filter.last_active_after = {};
+ filter.lastActive = filter.lastActive || {};
const start = new Date(last_active_after);
filter["lastActive"]["$gte"] = start;
}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.
| if (last_active_after) { | |
| filter.last_active_after = {}; | |
| const start = new Date(last_active_after); | |
| filter["lastActive"]["$gte"] = start; | |
| } | |
| if (last_active_after) { | |
| filter.lastActive = filter.lastActive || {}; | |
| const start = new Date(last_active_after); | |
| filter["lastActive"]["$gte"] = start; | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 1122-1124: src/device-registry/utils/generate-filter.js#L1122-L1124
Added lines #L1122 - L1124 were not covered by tests
| if (online_status) { | ||
| if (online_status.toLowerCase() === "online") { | ||
| filter["isOnline"] = true; | ||
| } else if (online_status.toLowerCase() === "offline") { | ||
| filter["isOnline"] = false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle Unexpected Values for online_status
In the sites filter, when online_status is neither "online" nor "offline", the filter remains unchanged. To enhance user experience and prevent potential confusion, consider handling unexpected values by adding an else clause that returns an informative error message.
Suggested addition:
if (online_status) {
if (online_status.toLowerCase() === "online") {
filter["isOnline"] = true;
} else if (online_status.toLowerCase() === "offline") {
filter["isOnline"] = false;
+ } else {
+ // Handle unexpected value
+ next(new HttpError("Invalid online_status value", httpStatus.BAD_REQUEST));
}
}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 1135-1135: src/device-registry/utils/generate-filter.js#L1135
Added line #L1135 was not covered by tests
[warning] 1137-1137: src/device-registry/utils/generate-filter.js#L1137
Added line #L1137 was not covered by tests
| if (last_active_after) { | ||
| filter.last_active_after = {}; | ||
| const start = new Date(last_active_after); | ||
| filter["lastActive"]["$gte"] = start; | ||
| } |
There was a problem hiding this comment.
Potential Typo: Incorrect Initialization in last_active_after Filter
In line 939, filter.last_active_after is initialized as an empty object, but in line 941, you're assigning a condition to filter["lastActive"]["$gte"]. The initialization of filter.last_active_after seems unnecessary and may be a typo. Consider initializing filter.lastActive instead, or remove the unnecessary initialization.
Suggested fix:
if (last_active_after) {
- filter.last_active_after = {};
+ filter.lastActive = filter.lastActive || {};
const start = new Date(last_active_after);
filter["lastActive"]["$gte"] = start;
}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.
| if (last_active_after) { | |
| filter.last_active_after = {}; | |
| const start = new Date(last_active_after); | |
| filter["lastActive"]["$gte"] = start; | |
| } | |
| if (last_active_after) { | |
| filter.lastActive = filter.lastActive || {}; | |
| const start = new Date(last_active_after); | |
| filter["lastActive"]["$gte"] = start; | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 939-941: src/device-registry/utils/generate-filter.js#L939-L941
Added lines #L939 - L941 were not covered by tests
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
src/device-registry/routes/v2/sites.js (1)
Line range hint
116-280: Summary of changes and recommendationsThe introduction of new query parameters for filtering sites based on online status and last active time significantly enhances the API's capabilities. The implementation is thorough and well-structured, providing users with more granular control over site data retrieval.
However, there are several areas for improvement:
Code Duplication: The identical validation logic in the "/" and "/summary" routes should be refactored into a shared middleware function.
Test Coverage: Custom sanitizers and new query parameter validations need additional test coverage to ensure reliability.
Error Handling: Consider implementing a custom error handler for more user-friendly error messages.
Code Organization: Date-related parameter validations could be extracted into a reusable function.
By addressing these points, you can improve the maintainability, reliability, and user-friendliness of the API. The new functionality is a valuable addition, and with these refinements, it will be even more robust and easier to maintain in the future.
To further improve the architecture, consider:
Implementing a validation middleware factory that can generate validation chains based on a configuration object. This would make it easier to add or modify validations in the future.
Using a schema validation library like Joi or Yup for more declarative and potentially more performant validation logic.
Implementing API versioning to allow for future changes without breaking existing clients.
These architectural improvements will make the API more scalable and easier to evolve over time.
Tools
GitHub Check: codecov/patch
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-136: src/device-registry/routes/v2/sites.js#L135-L136
Added lines #L135 - L136 were not covered by testssrc/device-registry/utils/generate-filter.js (2)
906-909: Excellent addition of new query parameters!The new parameters for filtering devices based on their online status and last active time are a valuable enhancement to the API. This implementation aligns well with the PR objectives and maintains consistency with the existing code style.
To further improve code readability, consider grouping related parameters together. For example:
const { // ... other parameters ... online_status, last_active, last_active_before, last_active_after, // ... remaining parameters ... } = { ...req.query, ...req.params };This grouping makes it easier to identify and manage related parameters.
Line range hint
1068-1140: Address similarities betweendevicesandsitesmethodsThe changes made to the
sitesmethod are very similar to those in thedevicesmethod. The same recommendations apply:
- Consider combining the
last_activeandlast_active_afterconditions to reduce code duplication.- Enhance the robustness of the
online_statusfilter by handling unexpected values.- Add comprehensive test coverage for all new filters.
To maintain consistency across the codebase, please apply the same improvements suggested for the
devicesmethod to thesitesmethod as well.If you need any clarification or assistance in implementing these changes consistently across both methods, please don't hesitate to ask.
Tools
GitHub Check: codecov/patch
[warning] 1116-1118: src/device-registry/utils/generate-filter.js#L1116-L1118
Added lines #L1116 - L1118 were not covered by tests
[warning] 1122-1124: src/device-registry/utils/generate-filter.js#L1122-L1124
Added lines #L1122 - L1124 were not covered by tests
[warning] 1128-1130: src/device-registry/utils/generate-filter.js#L1128-L1130
Added lines #L1128 - L1130 were not covered by tests
[warning] 1135-1135: src/device-registry/utils/generate-filter.js#L1135
Added line #L1135 was not covered by tests
[warning] 1137-1137: src/device-registry/utils/generate-filter.js#L1137
Added line #L1137 was not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/device-registry/routes/v2/devices.js (2 hunks)
- src/device-registry/routes/v2/sites.js (2 hunks)
- src/device-registry/utils/generate-filter.js (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/device-registry/routes/v2/devices.js
Additional context used
GitHub Check: codecov/patch
src/device-registry/routes/v2/sites.js
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-136: src/device-registry/routes/v2/sites.js#L135-L136
Added lines #L135 - L136 were not covered by tests
[warning] 215-216: src/device-registry/routes/v2/sites.js#L215-L216
Added lines #L215 - L216 were not covered by tests
[warning] 225-226: src/device-registry/routes/v2/sites.js#L225-L226
Added lines #L225 - L226 were not covered by testssrc/device-registry/utils/generate-filter.js
[warning] 933-935: src/device-registry/utils/generate-filter.js#L933-L935
Added lines #L933 - L935 were not covered by tests
[warning] 939-941: src/device-registry/utils/generate-filter.js#L939-L941
Added lines #L939 - L941 were not covered by tests
[warning] 945-947: src/device-registry/utils/generate-filter.js#L945-L947
Added lines #L945 - L947 were not covered by tests
[warning] 952-952: src/device-registry/utils/generate-filter.js#L952
Added line #L952 was not covered by tests
[warning] 954-954: src/device-registry/utils/generate-filter.js#L954
Added line #L954 was not covered by tests
[warning] 1116-1118: src/device-registry/utils/generate-filter.js#L1116-L1118
Added lines #L1116 - L1118 were not covered by tests
[warning] 1122-1124: src/device-registry/utils/generate-filter.js#L1122-L1124
Added lines #L1122 - L1124 were not covered by tests
[warning] 1128-1130: src/device-registry/utils/generate-filter.js#L1128-L1130
Added lines #L1128 - L1130 were not covered by tests
[warning] 1135-1135: src/device-registry/utils/generate-filter.js#L1135
Added line #L1135 was not covered by tests
[warning] 1137-1137: src/device-registry/utils/generate-filter.js#L1137
Added line #L1137 was not covered by tests
Additional comments not posted (1)
src/device-registry/routes/v2/sites.js (1)
206-280: Eliminate code duplication in route validationThe validation logic for the "/summary" route (lines 206-280) is identical to the "/" route (lines 116-190). This duplication violates the DRY (Don't Repeat Yourself) principle and can lead to maintenance issues and inconsistencies if one route is updated but not the other.
To address this, I recommend extracting the shared validation logic into a separate middleware function:
const validateSiteQueryParams = [ oneOf([ [ query("id") .optional() .notEmpty() .trim() .isMongoId() .withMessage("id must be an object ID") .bail() .customSanitizer((value) => { return ObjectId(value); }), // ... (include all other query parameter validations) ], ]), ]; // Usage in routes router.get("/", validateSiteQueryParams, siteController.list); router.get("/summary", validateSiteQueryParams, siteController.listSummary);This refactoring will significantly reduce code duplication, improve maintainability, and ensure consistency between the two routes. Any future changes to the validation logic will only need to be made in one place.
To ensure this refactoring doesn't introduce any regressions, we should verify the current test coverage for these routes:
#!/bin/bash # Check test coverage for the "/" and "/summary" routes test_file=$(find test -name "*sites.test.js" -o -name "*sites.spec.js") if [ -n "$test_file" ]; then echo "Test file found: $test_file" echo "Checking for route tests:" grep -n "router.get('/'," "$test_file" && grep -n "router.get('/summary'," "$test_file" || echo "No specific route tests found." else echo "No test file found for sites.js" fiThis script will help identify if there are existing tests for these routes and where to add or update tests after the refactoring.
Tools
GitHub Check: codecov/patch
[warning] 215-216: src/device-registry/routes/v2/sites.js#L215-L216
Added lines #L215 - L216 were not covered by tests
[warning] 225-226: src/device-registry/routes/v2/sites.js#L225-L226
Added lines #L225 - L226 were not covered by tests
| oneOf([ | ||
| [ | ||
| query("id") | ||
| .optional() | ||
| .notEmpty() | ||
| .trim() | ||
| .isMongoId() | ||
| .withMessage("id must be an object ID") | ||
| .bail() | ||
| .customSanitizer((value) => { | ||
| return ObjectId(value); | ||
| }), | ||
| query("site_id") | ||
| .optional() | ||
| .notEmpty() | ||
| .trim() | ||
| .isMongoId() | ||
| .withMessage("site_id must be an object ID") | ||
| .bail() | ||
| .customSanitizer((value) => { | ||
| return ObjectId(value); | ||
| }), | ||
| query("name") | ||
| .optional() | ||
| .notEmpty() | ||
| .trim(), | ||
| query("online_status") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the online_status should not be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["online", "offline"]) | ||
| .withMessage( | ||
| "the online_status value is not among the expected ones which include: online, offline" | ||
| ), | ||
| query("last_active_before") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_before date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage( | ||
| "last_active_before date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)." | ||
| ) | ||
| .bail() | ||
| .toDate(), | ||
| query("last_active_after") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active_after date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage( | ||
| "last_active_after date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)." | ||
| ) | ||
| .bail() | ||
| .toDate(), | ||
| query("last_active") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("last_active date cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage( | ||
| "last_active date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)." | ||
| ) | ||
| .bail() | ||
| .toDate(), | ||
| ], | ||
| ]), |
There was a problem hiding this comment.
Tip
Codebase Verification
Missing Test Coverage for Custom Sanitizers
No test files were found for src/device-registry/routes/v2/sites.js, indicating that the customSanitizer functions for id and site_id are not currently covered by unit tests. It's crucial to add appropriate tests to ensure these sanitizers behave as expected and maintain the integrity of the API.
Analysis chain
Comprehensive query parameter validation implemented
The addition of new query parameters (id, site_id, name, online_status, last_active_before, last_active_after, last_active) with their respective validation logic enhances the API's capability to filter sites based on various criteria. The validation is thorough and well-structured.
However, there are a few points to consider:
-
Error Handling: Consider implementing a custom error handler to provide more user-friendly error messages. This could improve the API's usability.
-
Code Organization: The validation logic for date-related parameters (last_active_before, last_active_after, last_active) is quite similar. You might want to consider extracting this into a reusable function to reduce code duplication.
-
Test Coverage: The static analysis hints indicate that the custom sanitizers for 'id' and 'site_id' (lines 125-126 and 135-136) are not covered by tests. It's important to add unit tests for these to ensure they behave as expected.
To improve code organization, consider extracting the date validation logic into a reusable function:
function validateDateParameter(field) {
return query(field)
.optional()
.notEmpty()
.withMessage(`${field} date cannot be empty IF provided`)
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
`${field} date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ).`
)
.bail()
.toDate();
}
// Usage
validateDateParameter('last_active_before'),
validateDateParameter('last_active_after'),
validateDateParameter('last_active'),To address the test coverage issue, we can use the following script to locate the relevant test file:
This script will help identify if there are any existing tests for the custom sanitizers and where to add them if they're missing.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the test file for src/device-registry/routes/v2/sites.js
test_file=$(find test -name "*sites.test.js" -o -name "*sites.spec.js")
if [ -n "$test_file" ]; then
echo "Test file found: $test_file"
echo "Checking for sanitizer tests:"
grep -n "customSanitizer" "$test_file" || echo "No sanitizer tests found."
else
echo "No test file found for sites.js"
fi
Length of output: 203
Script:
#!/bin/bash
# Search for test files matching *sites.test.js or *sites.spec.js across the entire repository
test_files=$(find . -type f \( -name '*sites.test.js' -o -name '*sites.spec.js' \))
if [ -n "$test_files" ]; then
echo "Test files found:"
echo "$test_files"
echo "Checking for sanitizer tests:"
grep -n "customSanitizer" $test_files || echo "No sanitizer tests found."
else
echo "No test files found for sites.js"
fi
Length of output: 179
Tools
GitHub Check: codecov/patch
[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-136: src/device-registry/routes/v2/sites.js#L135-L136
Added lines #L135 - L136 were not covered by tests
There was a problem hiding this comment.
Please create a new Github issue for this suggestion.
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been created to track this suggestion: Issue #3494.
Analysis chain
@Baalmart, I'll create a new GitHub issue to track this suggestion.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add unit tests for custom sanitizers in src/device-registry/routes/v2/sites.js" \
--body $'The custom sanitizers for `id` and `site_id` in `src/device-registry/routes/v2/sites.js` are not currently covered by unit tests. It\'s crucial to add appropriate tests to ensure these sanitizers behave as expected and maintain the integrity of the API.\n\nOriginally suggested in [PR #3489](https://github.com/airqo-platform/AirQo-api/pull/3489) by @Baalmart.\n\nSee the [comment](https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708562) for more details.' \
--assignee Baalmart
Length of output: 667
| if (last_active) { | ||
| filter.lastActive = {}; | ||
| const start = new Date(last_active); | ||
| filter["lastActive"]["$gte"] = start; | ||
| } | ||
|
|
||
| if (last_active_after) { | ||
| filter.lastActive = {}; | ||
| const start = new Date(last_active_after); | ||
| filter["lastActive"]["$gte"] = start; | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for last_active and last_active_after filters
The implementation of last_active and last_active_after is correct, but they currently set the same filter condition. To improve clarity and maintainability, consider combining these conditions:
if (last_active || last_active_after) {
filter.lastActive = {};
const start = new Date(last_active || last_active_after);
filter["lastActive"]["$gte"] = start;
}This refactoring reduces code duplication and makes the intent clearer.
Additionally, it's important to note that this new code is not covered by tests. To ensure the reliability of these new filters, please add unit tests that cover various scenarios for these parameters.
Would you like assistance in generating unit tests for these new filters?
Tools
GitHub Check: codecov/patch
[warning] 933-935: src/device-registry/utils/generate-filter.js#L933-L935
Added lines #L933 - L935 were not covered by tests
[warning] 939-941: src/device-registry/utils/generate-filter.js#L939-L941
Added lines #L939 - L941 were not covered by tests
There was a problem hiding this comment.
Please create a new Github issue for this suggestion.
There was a problem hiding this comment.
@Baalmart, I've created a GitHub issue for this suggestion as requested: #3492.
Analysis chain
@Baalmart, I'll create a GitHub issue for this suggestion.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Open a new GitHub issue for the suggestion
gh issue create --title "Refactor filters for 'last_active' and 'last_active_after' in generateFilter utility" --body "The implementation of \`last_active\` and \`last_active_after\` in the \`generateFilter\` utility sets the same filter condition. To improve clarity and maintainability, consider combining these conditions to reduce code duplication and enhance readability.
**Pull Request**: https://github.com/airqo-platform/AirQo-api/pull/3489
**Comment**: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708574
**Requested by**: @Baalmart" --assignee Baalmart
Length of output: 654
| if (last_active_before) { | ||
| filter.lastActive = {}; | ||
| const start = new Date(last_active_before); | ||
| filter["lastActive"]["$lte"] = start; | ||
| } |
There was a problem hiding this comment.
Correct implementation of last_active_before filter
The implementation of the last_active_before filter is correct. It properly uses the $lte operator to find devices active before the specified date.
However, as with the previous filters, this new code is not covered by tests. To ensure the reliability of this filter, please add unit tests that cover various scenarios for the last_active_before parameter.
Consider adding tests that check:
- Devices active before a specific date are included
- Devices active after the specified date are excluded
- Edge cases, such as devices active exactly on the specified date
If you need assistance in writing these tests, please let me know, and I'd be happy to help.
Tools
GitHub Check: codecov/patch
[warning] 945-947: src/device-registry/utils/generate-filter.js#L945-L947
Added lines #L945 - L947 were not covered by tests
There was a problem hiding this comment.
Please create a new Github issue for this suggestion.
There was a problem hiding this comment.
@Baalmart, I've successfully updated the GitHub issue with the correct title, including the last_active_before filter. You can view the updated issue here: #3491. Let me know if there's anything else I can assist with.
(_/)
(^.^)
(")_(")
Analysis chain
@Baalmart, I'll create a GitHub issue for this suggestion.
@Baalmart, it appears there was an error in the script due to unescaped backticks, which caused the issue title to be incorrect. I've corrected the script by escaping the backticks to prevent shell interpretation:
This script will update the existing issue with the correct title. You can check the updated issue here: #3491.
Let me know if there's anything else I can assist with.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for adding unit tests for the `last_active_before` filter
gh issue create --title "Add unit tests for `last_active_before` filter in generate-filter.js" \
--body "In PR #3489 (https://github.com/airqo-platform/AirQo-api/pull/3489), new code was added implementing the \`last_active_before\` filter in \`src/device-registry/utils/generate-filter.js\`. This new code is not currently covered by unit tests. To ensure its reliability, unit tests should be added covering various scenarios for the \`last_active_before\` parameter.
Original comment: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708575
Requester: @Baalmart" \
--assignee Baalmart
Length of output: 746
Script:
#!/bin/bash
# Update the title of the existing issue to include `last_active_before`
gh issue edit 3491 --title "Add unit tests for \\\`last_active_before\\\` filter in generate-filter.js"
Length of output: 157
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if (online_status) { | ||
| if (online_status.toLowerCase() === "online") { | ||
| filter["isOnline"] = true; | ||
| } else if (online_status.toLowerCase() === "offline") { | ||
| filter["isOnline"] = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance robustness of online_status filter
The implementation of the online_status filter is fundamentally correct, but it could be made more robust. Consider the following improvements:
- Handle unexpected values:
if (online_status) {
const status = online_status.toLowerCase();
if (status === "online") {
filter["isOnline"] = true;
} else if (status === "offline") {
filter["isOnline"] = false;
} else {
// Handle unexpected value
console.warn(`Unexpected online_status value: ${online_status}`);
// Optionally, you could throw an error or set a default value
}
}- Add test coverage for this new filter. Include tests for:
- Setting filter to true for "online" status
- Setting filter to false for "offline" status
- Handling of unexpected values
- Case insensitivity of the input
These enhancements will improve the reliability and maintainability of the online_status filter.
Would you like assistance in implementing these improvements or writing the corresponding unit tests?
Tools
GitHub Check: codecov/patch
[warning] 952-952: src/device-registry/utils/generate-filter.js#L952
Added line #L952 was not covered by tests
[warning] 954-954: src/device-registry/utils/generate-filter.js#L954
Added line #L954 was not covered by tests
There was a problem hiding this comment.
Please create a new Github issue for this suggestion.
There was a problem hiding this comment.
@Baalmart, I've created a new GitHub issue for this suggestion: #3493.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for the suggested improvements
gh issue create \
--title "Enhance robustness of 'online_status' filter" \
--body "There is a suggestion to enhance the robustness of the \`online_status\` filter by handling unexpected values and adding test coverage.
[PR URL](https://github.com/airqo-platform/AirQo-api/pull/3489)
[Comment URL](https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708577)
Requested by @Baalmart" \
--assignee Baalmart
Length of output: 478
Description
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
Just adding new query parameters to support the query for sites or devices based on the online status of the devices. the new query parameters include:
Summary by CodeRabbit
New Features
Bug Fixes
Chores