Update Offline Status for Untracked Devices #3620
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the functionality of the Changes
Possibly related PRs
Suggested reviewers
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 #3620 +/- ##
===========================================
- Coverage 27.09% 27.04% -0.06%
===========================================
Files 146 146
Lines 21336 21339 +3
Branches 273 273
===========================================
- Hits 5782 5772 -10
- Misses 15554 15567 +13
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // Update devices that are not in the activeDeviceIds set to offline | ||
| await DeviceModel("airqo").updateMany( | ||
| { _id: { $nin: Array.from(activeDeviceIds) } }, | ||
| { isOnline: false } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Issue: Devices May Be Incorrectly Marked as Offline
The current logic updates isOnline to false for all devices not present in activeDeviceIds. This may inadvertently mark devices as offline even if they are still active but didn't report in the current batch due to intermittent connectivity or data fetch issues.
Consider modifying the logic to mark devices as offline only if they haven't sent data within the defined INACTIVE_THRESHOLD. This ensures that only truly inactive devices are marked offline.
You might adjust the update query as follows:
- await DeviceModel("airqo").updateMany(
- { _id: { $nin: Array.from(activeDeviceIds) } },
- { isOnline: false }
- );
+ const thresholdTime = moment().subtract(INACTIVE_THRESHOLD, 'milliseconds').toDate();
+ await DeviceModel("airqo").updateMany(
+ {
+ _id: { $nin: Array.from(activeDeviceIds) },
+ lastActive: { $lt: thresholdTime }
+ },
+ { isOnline: false }
+ );This change adds a condition to only update devices whose lastActive timestamp is older than the INACTIVE_THRESHOLD, preventing premature offline status updates.
Committable suggestion was skipped due to low confidence.
… in fetched measurements
|
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 (3)
src/device-registry/bin/jobs/test/ut_v2-store-readings-job.js (3)
137-151: Excellent enhancement to test coverage!The addition of stubs for the Device Model significantly improves the test's ability to verify offline device updates. This is a valuable improvement to the test suite.
As a minor suggestion, consider extracting the device ID "456" into a constant at the beginning of the test case. This would make the test more maintainable if you need to change the ID in the future.
Here's a small refactor suggestion:
const ACTIVE_DEVICE_ID = "456"; const mockData = [ { data: [{ site_id: "123", device_id: ACTIVE_DEVICE_ID, time: new Date() }] }, ]; // ... rest of the test case ... expect(deviceUpdateManyStub).to.have.been.calledWith( { _id: { $nin: [ACTIVE_DEVICE_ID] } }, { isOnline: false } );This change would make the test more readable and easier to maintain.
158-161: Well-crafted expectation for offline device updates!This expectation effectively verifies that devices not present in the fetched data are marked as offline. It's a crucial check that ensures the core functionality is working as intended.
To enhance clarity, you might consider adding a brief comment explaining the logic behind the
$ninoperator. This would make the test more self-documenting for future readers.Here's a suggestion to improve the comment:
expect(deviceUpdateManyStub).to.have.been.calledWith( { _id: { $nin: ["456"] } }, // Update all devices except the one in fetched data { isOnline: false } );This small change provides more context about the purpose of the
$ninoperator in this test.
193-223: Excellent addition of a focused test case!This new test case specifically targeting the offline marking behavior is a valuable addition to the test suite. It ensures that this critical functionality is explicitly verified, which is crucial for maintaining system reliability.
To maintain consistency with the earlier suggestion and improve maintainability, consider extracting the device ID into a constant here as well.
Here's a suggestion to improve consistency and maintainability:
const ACTIVE_DEVICE_ID = "456"; const mockData = [ { data: [{ site_id: "123", device_id: ACTIVE_DEVICE_ID, time: new Date() }] }, ]; // ... rest of the test case ... expect(deviceUpdateManyStub).to.have.been.calledWith( { _id: { $nin: [ACTIVE_DEVICE_ID] } }, { isOnline: false } );This change aligns with the earlier suggestion and makes the test more robust against future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/device-registry/bin/jobs/test/ut_v2-store-readings-job.js (2 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: Baalmart PR: airqo-platform/AirQo-api#3611 File: src/device-registry/bin/server.js:27-27 Timestamp: 2024-10-10T06:33:42.428Z Learning: The `v2-store-readings-job.js` file exists in the repository at `src/device-registry/bin/jobs/v2-store-readings-job.js`.
🔇 Additional comments (1)
src/device-registry/bin/jobs/test/ut_v2-store-readings-job.js (1)
Line range hint
137-223: Commendable enhancement of test coverage!The changes made to this test file significantly improve the verification of offline device update functionality. The addition of new test cases and expectations demonstrates a thorough approach to ensuring the reliability of this critical feature.
Key strengths of these changes include:
- Comprehensive coverage of offline device marking scenarios.
- Consistent use of sinon stubs for dependency isolation.
- Clear and focused test cases that target specific behaviors.
The minor suggestions provided earlier for extracting constants and improving comments would further enhance the maintainability and readability of these tests.
Overall, these changes represent a valuable contribution to the test suite and will help ensure the robustness of the
new-store-readings-jobmodule.
|
Device registry changes in this PR available for preview here |
Description
Enhance Device Status Management: Update Offline Status for Untracked Devices
Changes Made
Affected Services
API Documentation Updated?
Additional Notes
Enhance Device Status Management: Update Offline Status for Untracked Devices
Summary by CodeRabbit
New Features
Bug Fixes
Tests