ability to query devices using status values #3467
Conversation
WalkthroughWalkthroughThe changes introduce a new constant, 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 #3467 +/- ##
===========================================
- Coverage 30.21% 30.20% -0.01%
===========================================
Files 184 184
Lines 24554 24565 +11
Branches 3219 3222 +3
===========================================
+ Hits 7418 7419 +1
- Misses 17022 17031 +9
- Partials 114 115 +1
|
|
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/utils/generate-filter.js (1)
1015-1016: Optimize string replacement for status normalizationThe replacement of underscores and dashes with spaces might not cover all scenarios. Consider using a more comprehensive normalization method if necessary.
Tools
GitHub Check: codecov/patch
[warning] 1015-1016: src/device-registry/utils/generate-filter.js#L1015-L1016
Added lines #L1015 - L1016 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/device-registry/utils/generate-filter.js (2 hunks)
Additional context used
GitHub Check: codecov/patch
src/device-registry/utils/generate-filter.js
[warning] 1007-1008: src/device-registry/utils/generate-filter.js#L1007-L1008
Added lines #L1007 - L1008 were not covered by tests
[warning] 1010-1010: src/device-registry/utils/generate-filter.js#L1010
Added line #L1010 was not covered by tests
[warning] 1015-1016: src/device-registry/utils/generate-filter.js#L1015-L1016
Added lines #L1015 - L1016 were not covered by tests
[warning] 1018-1018: src/device-registry/utils/generate-filter.js#L1018
Added line #L1018 was not covered by tests
[warning] 1021-1021: src/device-registry/utils/generate-filter.js#L1021
Added line #L1021 was not covered by tests
Additional comments not posted (2)
src/device-registry/utils/generate-filter.js (2)
905-905: Introduction of 'status' parameter to device filterThe addition of the
statusparameter enhances the querying capabilities of the devices. Please ensure that this parameter is properly validated and sanitized to handle unexpected inputs gracefully.
1005-1005: Verify the regular expression for splitting status stringsThe regular expression used to split the
statusstring may not handle all edge cases, such as statuses with embedded commas or unusual formatting. Please verify that the regex correctly handles all expected input formats.
| const validStatuses = [ | ||
| "recalled", | ||
| "ready", | ||
| "deployed", | ||
| "undeployed", | ||
| "decommissioned", | ||
| "assembly", | ||
| "testing", | ||
| "not deployed", | ||
| ]; |
There was a problem hiding this comment.
Externalize 'validStatuses' for better maintainability
Currently, the validStatuses array is hardcoded within the function. To improve maintainability and allow for easier updates or reuse, consider moving validStatuses to a constants file or configuration module.
| if (status) { | ||
| // Split the status string by commas, but not within quotes | ||
| const statusArray = status.match(/(".*?"|[^",\s]+)(?=\s*,|\s*$)/g) || []; | ||
|
|
||
| const validStatusArray = statusArray | ||
| .map((s) => { | ||
| // Remove quotes and trim whitespace | ||
| s = s | ||
| .replace(/^"|"$/g, "") | ||
| .trim() | ||
| .toLowerCase(); | ||
| // Replace underscores or dashes with spaces | ||
| s = s.replace(/[_-]/g, " "); | ||
| return s; | ||
| }) | ||
| .filter((s) => validStatuses.includes(s)); | ||
|
|
||
| if (validStatusArray.length > 0) { | ||
| filter.status = { $in: validStatusArray }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add unit tests for 'status' parsing logic
The new logic for parsing the status parameter is crucial for filtering devices accurately. However, static analysis indicates that these lines are not covered by tests. Adding unit tests will ensure the reliability of this functionality and help catch potential issues early.
Would you like assistance in writing these unit tests or should I open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 1007-1008: src/device-registry/utils/generate-filter.js#L1007-L1008
Added lines #L1007 - L1008 were not covered by tests
[warning] 1010-1010: src/device-registry/utils/generate-filter.js#L1010
Added line #L1010 was not covered by tests
[warning] 1015-1016: src/device-registry/utils/generate-filter.js#L1015-L1016
Added lines #L1015 - L1016 were not covered by tests
[warning] 1018-1018: src/device-registry/utils/generate-filter.js#L1018
Added line #L1018 was not covered by tests
[warning] 1021-1021: src/device-registry/utils/generate-filter.js#L1021
Added line #L1021 was not covered by tests
There was a problem hiding this comment.
Please open a GitHub issue to track this task
There was a problem hiding this comment.
@Baalmart, I've successfully created the GitHub issue: #3468
(\_/)
(•ᴗ•)
/🍃🌼\
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add unit tests for 'status' parsing logic in generate-filter.js" \
--body "We need to add unit tests for the new 'status' parsing logic introduced in PR #3467.
PR URL: https://github.com/airqo-platform/AirQo-api/pull/3467
Comment URL: https://github.com/airqo-platform/AirQo-api/pull/3467#discussion_r1764048444
Requested by @Baalmart." \
--assignee "Baalmart"
Length of output: 451
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/device-registry/config/global/static-lists.js (1)
11-20: Consider aligning the definition of 'VALID_DEVICE_STATUSES' with 'AQI_CATEGORIES' for consistency.To maintain a consistent coding style within
staticLists, you might defineVALID_DEVICE_STATUSESusing a string and the.split(',')method, similar toAQI_CATEGORIES.Apply this diff to align the style:
- VALID_DEVICE_STATUSES: [ - "recalled", - "ready", - "deployed", - "undeployed", - "decommissioned", - "assembly", - "testing", - "not deployed", - ], + VALID_DEVICE_STATUSES: "recalled,ready,deployed,undeployed,decommissioned,assembly,testing,not deployed".split(","),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/device-registry/config/global/static-lists.js (1 hunks)
- src/device-registry/utils/generate-filter.js (2 hunks)
Additional context used
GitHub Check: codecov/patch
src/device-registry/utils/generate-filter.js
[warning] 998-999: src/device-registry/utils/generate-filter.js#L998-L999
Added lines #L998 - L999 were not covered by tests
[warning] 1001-1001: src/device-registry/utils/generate-filter.js#L1001
Added line #L1001 was not covered by tests
[warning] 1006-1007: src/device-registry/utils/generate-filter.js#L1006-L1007
Added lines #L1006 - L1007 were not covered by tests
[warning] 1009-1009: src/device-registry/utils/generate-filter.js#L1009
Added line #L1009 was not covered by tests
[warning] 1012-1012: src/device-registry/utils/generate-filter.js#L1012
Added line #L1012 was not covered by tests
Additional comments not posted (3)
src/device-registry/config/global/static-lists.js (1)
11-20: Addition of 'VALID_DEVICE_STATUSES' is well-structured and enhances maintainability.Defining
VALID_DEVICE_STATUSESwithinstaticListsprovides a centralized list of valid statuses, improving consistency across the codebase.src/device-registry/utils/generate-filter.js (2)
905-905: Inclusion of 'status' parameter enhances device filtering capabilities.Adding the
statusparameter allows users to query devices based on their operational status, improving the flexibility of the API.
992-1014: 'Status' filtering logic is effectively implemented.The parsing, normalization, and validation of the
statusparameter are handled appropriately. Utilizingconstants.VALID_DEVICE_STATUSESensures only valid statuses are considered in queries.Tools
GitHub Check: codecov/patch
[warning] 998-999: src/device-registry/utils/generate-filter.js#L998-L999
Added lines #L998 - L999 were not covered by tests
[warning] 1001-1001: src/device-registry/utils/generate-filter.js#L1001
Added line #L1001 was not covered by tests
[warning] 1006-1007: src/device-registry/utils/generate-filter.js#L1006-L1007
Added lines #L1006 - L1007 were not covered by tests
[warning] 1009-1009: src/device-registry/utils/generate-filter.js#L1009
Added line #L1009 was not covered by tests
[warning] 1012-1012: src/device-registry/utils/generate-filter.js#L1012
Added line #L1012 was not covered by tests
Description
ability to query devices using status values
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit