adding a helper function to update offline Sites #4082
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new helper function Changes
Sequence DiagramsequenceDiagram
participant Job as Store Readings Job
participant DeviceRegistry as Device Registry
Job->>DeviceRegistry: Fetch Measurements
Job->>DeviceRegistry: Parallel Updates
par
Job->>DeviceRegistry: Update Offline Devices
Job->>DeviceRegistry: Update Offline Sites
end
DeviceRegistry-->>Job: Status Updated
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 #4082 +/- ##
===========================================
- Coverage 11.81% 11.81% -0.01%
===========================================
Files 115 115
Lines 15482 15485 +3
Branches 319 319
===========================================
Hits 1829 1829
- Misses 13653 13656 +3
|
![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 (3)
src/device-registry/bin/jobs/v2.1-store-readings-job.js (3)
185-199: Good implementation, consider adding error handling and loggingThe implementation is well-structured and efficient, using Set for unique values and bulk updates. However, consider adding error handling and logging for better observability.
Consider applying this enhancement:
async function updateOfflineSites(data) { + try { const activeSiteIds = new Set(data.map((doc) => doc.site_id).filter(Boolean)); const thresholdTime = moment() .subtract(INACTIVE_THRESHOLD, "milliseconds") .toDate(); - await SiteModel("airqo").updateMany( + const result = await SiteModel("airqo").updateMany( { _id: { $nin: Array.from(activeSiteIds) }, lastActive: { $lt: thresholdTime }, }, { isOnline: false } ); + logger.info(`Updated ${result.modifiedCount} offline sites`); + } catch (error) { + logger.error(`Error updating offline sites: ${error.message}`); + throw error; // Re-throw to be handled by the caller + } }
278-279: LGTM! Efficient parallel execution implementationGood use of Promise.all for concurrent execution of offline updates. The implementation is efficient as both operations are independent.
Consider adding a more descriptive log message after successful completion:
- logText("All data inserted successfully and offline devices updated"); + logText("All data inserted successfully, offline devices and sites updated");
185-199: Consider architectural improvements for better maintainabilityA few suggestions to enhance the implementation:
- Consider moving the
INACTIVE_THRESHOLDto the configuration file for easier maintenance across environments.- Consider implementing metrics collection for offline sites to track trends and patterns.
This would make the system more configurable and provide better insights into site health.
Example configuration structure:
// @config/constants.js module.exports = { // ... existing constants SITE_INACTIVE_THRESHOLD: process.env.SITE_INACTIVE_THRESHOLD || 5 * 60 * 60 * 1000, };
| @@ -282,4 +298,5 @@ module.exports = { | |||
| updateEntityStatus, | |||
| isEntityActive, | |||
| updateOfflineDevices, | |||
| updateOfflineSites, | |||
There was a problem hiding this comment.
💡 Codebase verification
Tests needed for the new updateOfflineSites function
The updateOfflineSites function is not covered by the existing test suite in ut_v2-store-readings-job.js. While the test file has good coverage for other functions including similar functionality in fetchAndStoreDataIntoReadingsModel, it lacks specific tests for this newly exported function.
Please add test cases for:
- Updating sites as offline when they're not in the active set and beyond the threshold
- Handling empty data arrays
- Verifying the correct MongoDB query conditions are used
- Ensuring the SiteModel update operation is called with correct parameters
The test structure should follow the existing patterns in the file, using the same testing utilities (chai, sinon) and setup/teardown approach.
🔗 Analysis chain
Verify test coverage for the new function
The new function is correctly exported for testing purposes. However, we should ensure adequate test coverage exists.
Let's verify the test coverage:
The previous attempt failed because we assumed the test directory location. Let's first understand the project structure and then look for test files that might contain our target function.
The previous search revealed the test directory structure and the function implementation. Let's specifically look for test files in the jobs directory where our function resides.
We found the relevant test file. Let's check if it contains tests for our new function and understand the test structure to verify coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test files and coverage for updateOfflineSites
# Look for test files that might contain tests for this function
rg -l "updateOfflineSites" --type js test/
Length of output: 88
Script:
#!/bin/bash
# Let's first find the test directory structure
fd -t d "test" -d 3
# Then search for the function name in all JavaScript files
rg -l "updateOfflineSites" --type js
Length of output: 181
Script:
#!/bin/bash
# Let's check for tests in the device-registry directory
fd -t d "test" -d 3 "src/device-registry"
# Also search for any test files containing our function name in device-registry
rg -l "updateOfflineSites" "src/device-registry/**/*test*"
rg -l "updateOfflineSites" "src/device-registry/**/*spec*"
# Let's also see the function implementation to understand what kind of tests would be needed
rg -A 10 "function updateOfflineSites" src/device-registry/bin/jobs/v2.1-store-readings-job.js
Length of output: 1105
Script:
#!/bin/bash
# Let's check for test files in the jobs test directory specifically
fd -t f . "src/device-registry/bin/jobs/test"
# Let's also check for any test files related to store-readings-job
rg -l "store-readings" "src/device-registry/bin/jobs/test"
Length of output: 739
Script:
#!/bin/bash
# Let's check if the updateOfflineSites function is tested in the v2 store readings test file
rg -C 5 "updateOfflineSites" "src/device-registry/bin/jobs/test/ut_v2-store-readings-job.js"
# Also get the test file content to understand the test structure
cat "src/device-registry/bin/jobs/test/ut_v2-store-readings-job.js"
Length of output: 8442
There was a problem hiding this comment.
please generate a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been successfully created: #4083. Let me know if there's anything else I can assist with.
🔗 Analysis chain
@Baalmart, I've opened a GitHub issue to add tests for the updateOfflineSites function.
(_/)
(•_•)
/ 💻 Happy to help!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for adding tests for the updateOfflineSites function
gh issue create \
--title "Add tests for the updateOfflineSites function" \
--body "The \`updateOfflineSites\` function introduced in PR https://github.com/airqo-platform/AirQo-api/pull/4082 lacks test coverage.
Specific tests should be added for:
* Updating sites as offline when they're not in the active set and beyond the threshold
* Handling empty data arrays
* Verifying the correct MongoDB query conditions are used
* Ensuring the SiteModel update operation is called with correct parameters
The test structure should follow the existing patterns in the file, using the same testing utilities (chai, sinon) and setup/teardown approach.
Refer to the comment: https://github.com/airqo-platform/AirQo-api/pull/4082#discussion_r1885023407
Requester: @Baalmart" \
--assignee Baalmart
Length of output: 843
|
Device registry changes in this PR available for preview here |
Description
adding a helper function to update offline Sites
Changes Made
Testing
Affected Services
API Documentation Updated?
Additional Notes
adding a helper function to update offline Sites
Summary by CodeRabbit
New Features
Bug Fixes
Documentation