Add Three New Jobs and Their Respective Unit Tests #3740
Conversation
📝 WalkthroughWalkthroughThis pull request introduces three new job scripts in the device registry: 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 #3740 +/- ##
===========================================
+ Coverage 11.69% 11.84% +0.15%
===========================================
Files 110 113 +3
Lines 14848 14938 +90
Branches 274 274
===========================================
+ Hits 1737 1770 +33
- Misses 13111 13168 +57
|
|
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: 26
🧹 Outside diff range and nitpick comments (7)
src/device-registry/bin/jobs/check-unassigned-devices-job.js (2)
1-10: Consider adding JSDoc module documentation.Adding a module-level JSDoc comment would help other developers understand the purpose and behavior of this job script.
+/** + * @module check-unassigned-devices-job + * @description Cron job that monitors and reports unassigned devices in the system. + * Runs every 2 hours and logs warnings when devices are found without category assignments. + */ const constants = require("@config/constants");
11-65: Add JSDoc for the main function.Document the function's purpose, behavior, and return type for better code maintainability.
+/** + * @async + * @function checkUnassignedDevices + * @description Checks for devices without category assignments and logs warnings if found + * @returns {Promise<void>} + */ const checkUnassignedDevices = async () => {src/device-registry/bin/jobs/check-unassigned-sites-job.js (1)
11-11: Add JSDoc documentation for better code maintainabilityAdding comprehensive documentation will help other developers understand the function's purpose, parameters, and return values.
+/** + * Checks for sites that are not assigned to any grid and logs warnings if the percentage + * exceeds the defined threshold. + * @async + * @throws {Error} If there's an error during the database operations + * @returns {Promise<void>} + */ const checkUnassignedSites = async () => {src/device-registry/bin/jobs/test/ut_check-unassigned-sites-job.js (1)
1-92: Well-structured test suite aligned with monitoring objectives.The test suite effectively validates the unassigned sites monitoring functionality, which is one of the three new monitoring capabilities mentioned in the PR objectives. The test structure provides good coverage of success and error scenarios, supporting the PR's goal of enhancing system reliability.
Consider adding these test cases to improve coverage further:
- Test with different site counts to verify pagination handling
- Test with malformed site data to verify robustness
- Test the cron schedule configuration (mentioned in PR objectives as "every 30 minutes")
Would you like assistance in implementing these additional test cases?
🧰 Tools
🪛 Biome
[error] 15-15: Can't assign SitesModelMock because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 16-16: Can't assign logTextSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 17-17: Can't assign logObjectSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 18-18: Can't assign loggerErrorSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
src/device-registry/bin/jobs/test/ut_check-unassigned-devices.js (1)
38-70: Add test coverage for percentage calculation logic.The tests verify logging behavior but don't validate the actual calculation of the unassigned devices percentage. Consider adding a test case that specifically verifies this business logic.
Add a new test case:
it("should correctly calculate the percentage of unassigned devices", async () => { DeviceModelMock.expects("countDocuments").resolves(100); DeviceModelMock.expects("aggregate").resolves([ { _id: "device1" }, { _id: "device2" }, { _id: "device3" }, ]); await checkUnassignedDevices(); // Assuming 3 unassigned devices out of 100 total = 3% expect(consoleLogSpy).to.have.been.calledWith( sinon.match(/3% of devices are unassigned/i) ); });src/device-registry/bin/jobs/check-active-statuses.js (1)
11-110: Consider adding metrics collection for monitoring job performance.While the job implements good error handling and logging, it would be valuable to track metrics about its execution for monitoring purposes.
Consider:
- Track job execution time
- Record success/failure rates
- Collect metrics about the number of issues found
- Export these metrics to your monitoring system (e.g., Prometheus)
Would you like me to provide an example implementation using your preferred metrics collection system?
src/device-registry/bin/server.js (1)
29-31: Consider documenting job schedules.While the code organization is clean, it would be helpful to add a comment block above these imports documenting the purpose and schedule of each job for better maintainability.
Consider adding a comment block like this:
+// Monitoring Jobs (runs every 2 hours): +// - check-unassigned-devices-job: Monitors devices without assignments +// - check-active-statuses: Verifies device status consistency +// - check-unassigned-sites-job: Tracks sites without assignments require("@bin/jobs/check-unassigned-devices-job"); require("@bin/jobs/check-active-statuses"); require("@bin/jobs/check-unassigned-sites-job");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/device-registry/bin/jobs/check-active-statuses.js (1 hunks)
- src/device-registry/bin/jobs/check-unassigned-devices-job.js (1 hunks)
- src/device-registry/bin/jobs/check-unassigned-sites-job.js (1 hunks)
- src/device-registry/bin/jobs/test/ut_check-active-statuses.js (1 hunks)
- src/device-registry/bin/jobs/test/ut_check-unassigned-devices.js (1 hunks)
- src/device-registry/bin/jobs/test/ut_check-unassigned-sites-job.js (1 hunks)
- src/device-registry/bin/server.js (1 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/bin/jobs/test/ut_check-active-statuses.js
[error] 15-15: Can't assign DeviceModelMock because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 16-16: Can't assign logTextSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 17-17: Can't assign logObjectSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 18-18: Can't assign loggerErrorSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
src/device-registry/bin/jobs/test/ut_check-unassigned-sites-job.js
[error] 15-15: Can't assign SitesModelMock because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 16-16: Can't assign logTextSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 17-17: Can't assign logObjectSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 18-18: Can't assign loggerErrorSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (3)
src/device-registry/bin/jobs/test/ut_check-unassigned-sites-job.js (1)
14-23: LGTM! Well-structured test lifecycle hooks.The test setup and teardown are properly implemented:
beforeEachinitializes fresh spies and mocks for each testafterEachproperly restores all Sinon stubs, mocks, and spies🧰 Tools
🪛 Biome
[error] 15-15: Can't assign SitesModelMock because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 16-16: Can't assign logTextSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 17-17: Can't assign logObjectSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
[error] 18-18: Can't assign loggerErrorSpy because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
src/device-registry/bin/jobs/check-active-statuses.js (1)
8-8: Consider if zero threshold is too strict for production use.Setting
ACTIVE_STATUS_THRESHOLDto 0 means the system will alert on any single device with incorrect status. Consider if this might generate too many alerts in a production environment where temporary inconsistencies might be normal.Would you like me to help implement a configuration-based threshold that can be adjusted per environment?
src/device-registry/bin/server.js (1)
29-31: Verify job initialization and scheduling.The new job imports are well-placed alongside existing job imports. However, let's ensure these jobs are properly initialized when imported.
Let's verify the job initialization in the imported files:
#!/bin/bash # Description: Check if the new jobs are properly initialized with cron schedules # Test 1: Verify cron scheduling in the job files echo "Checking cron scheduling in job files..." rg -A 3 "cron.schedule" bin/jobs/check-unassigned-devices-job.js bin/jobs/check-active-statuses.js bin/jobs/check-unassigned-sites-job.js # Test 2: Verify error handling in job files echo "Checking error handling in job files..." rg "catch" bin/jobs/check-unassigned-devices-job.js bin/jobs/check-active-statuses.js bin/jobs/check-unassigned-sites-job.js # Test 3: Verify logging implementation echo "Checking logging implementation..." rg "log[4]?js|logger\." bin/jobs/check-unassigned-devices-job.js bin/jobs/check-active-statuses.js bin/jobs/check-unassigned-sites-job.js
| $match: { | ||
| isActive: false, | ||
| category: { $exists: false } || { $eq: "" }, | ||
| }, |
There was a problem hiding this comment.
Fix logical error in category match condition.
The current condition category: { $exists: false } || { $eq: "" } is not valid MongoDB syntax and won't work as intended.
- category: { $exists: false } || { $eq: "" },
+ $or: [
+ { category: { $exists: false } },
+ { category: "" }
+ ]📝 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.
| $match: { | |
| isActive: false, | |
| category: { $exists: false } || { $eq: "" }, | |
| }, | |
| $match: { | |
| isActive: false, | |
| $or: [ | |
| { category: { $exists: false } }, | |
| { category: "" } | |
| ] | |
| }, |
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created a GitHub issue to track this logical error: Issue #3747.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for the logical error in category match condition
PR_URL="https://github.com/airqo-platform/AirQo-api/pull/3740"
COMMENT_URL="https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344795"
gh issue create \
--title "Fix logical error in category match condition in check-unassigned-devices-job.js" \
--body "A potential logical error was identified in the category match condition in \`check-unassigned-devices-job.js\`. See [PR #3740]($PR_URL) and the [review comment]($COMMENT_URL).\n\nReported by @Baalmart." \
--assignee Baalmart
Length of output: 658
| logText( | ||
| `🤦♀️🫣 ${percentage.toFixed( | ||
| 2 | ||
| )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join( | ||
| ", " | ||
| )})` | ||
| ); | ||
| logger.info( | ||
| `🤦♀️🫣 ${percentage.toFixed( | ||
| 2 | ||
| )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join( | ||
| ", " | ||
| )})` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate logging.
The same message is being logged twice using different logging utilities. Consider consolidating the logs.
- logText(
- `🤦♀️🫣 ${percentage.toFixed(
- 2
- )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join(
- ", "
- )})`
- );
logger.info(
`🤦♀️🫣 ${percentage.toFixed(
2
)}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join(
", "
)})`
);📝 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.
| logText( | |
| `🤦♀️🫣 ${percentage.toFixed( | |
| 2 | |
| )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join( | |
| ", " | |
| )})` | |
| ); | |
| logger.info( | |
| `🤦♀️🫣 ${percentage.toFixed( | |
| 2 | |
| )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join( | |
| ", " | |
| )})` | |
| ); | |
| logger.info( | |
| `🤦♀️🫣 ${percentage.toFixed( | |
| 2 | |
| )}% of deployed devices are not assigned to any category (${uniqueDeviceNames.join( | |
| ", " | |
| )})` | |
| ); |
| if (unassignedDevicesCount === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const percentage = (unassignedDevicesCount / totalCount) * 100; |
There was a problem hiding this comment.
Add guard against division by zero.
The percentage calculation could throw an error if totalCount is 0.
if (unassignedDevicesCount === 0) {
return;
}
+ if (totalCount === 0) {
+ logger.warn("No devices found in the system");
+ return;
+ }
+
const percentage = (unassignedDevicesCount / totalCount) * 100;📝 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 (unassignedDevicesCount === 0) { | |
| return; | |
| } | |
| const percentage = (unassignedDevicesCount / totalCount) * 100; | |
| if (unassignedDevicesCount === 0) { | |
| return; | |
| } | |
| if (totalCount === 0) { | |
| logger.warn("No devices found in the system"); | |
| return; | |
| } | |
| const percentage = (unassignedDevicesCount / totalCount) * 100; |
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created a GitHub issue for this: Add guard against division by zero in checkUnassignedDevices function.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue for this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Open a new GitHub issue to address the division by zero check in checkUnassignedDevices function
gh issue create \
--title "Add guard against division by zero in checkUnassignedDevices function" \
--body $'In `src/device-registry/bin/jobs/check-unassigned-devices-job.js`, the calculation of `percentage` could result in a division by zero error when `totalCount` is zero. A guard should be added to handle this case.\n\nPR: https://github.com/airqo-platform/AirQo-api/pull/3740\nComment: https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344806\n\nRequested by @Baalmart.' \
--assignee Baalmart
Length of output: 564
| ); | ||
| const DeviceModel = require("@models/Device"); | ||
| const cron = require("node-cron"); | ||
| const UNASSIGNED_THRESHOLD = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving UNASSIGNED_THRESHOLD to configuration.
The threshold value could be environment-specific. Consider moving it to the configuration system for better maintainability.
-const UNASSIGNED_THRESHOLD = 0;
+const UNASSIGNED_THRESHOLD = constants.DEVICE_UNASSIGNED_THRESHOLD || 0;Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue #3741 to track this.
🔗 Analysis chain
@Baalmart, I'll create a GitHub issue for this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create --title "Move UNASSIGNED_THRESHOLD to configuration" \
--body "The threshold value could be environment-specific. Consider moving it to the configuration system for better maintainability.
**Reference:**
- PR URL: https://github.com/airqo-platform/AirQo-api/pull/3740
- Comment URL: https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344809
- Requested by: Baalmart" \
--assignee Baalmart
Length of output: 478
| logText("Unassigned devices job is now running....."); | ||
| const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour | ||
| cron.schedule(schedule, checkUnassignedDevices, { | ||
| scheduled: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider immediate execution and schedule documentation.
The job only runs at scheduled times. Consider adding immediate execution for faster feedback during system startup.
logText("Unassigned devices job is now running.....");
-const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
+// Run at minute 30 of every 2nd hour (12:30, 2:30, 4:30, etc.)
+const schedule = "30 */2 * * *";
cron.schedule(schedule, checkUnassignedDevices, {
scheduled: true,
});
+
+// Execute immediately on startup
+checkUnassignedDevices().catch(error =>
+ logger.error(`Initial execution failed: ${error.message}`)
+);📝 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.
| logText("Unassigned devices job is now running....."); | |
| const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour | |
| cron.schedule(schedule, checkUnassignedDevices, { | |
| scheduled: true, | |
| }); | |
| logText("Unassigned devices job is now running....."); | |
| // Run at minute 30 of every 2nd hour (12:30, 2:30, 4:30, etc.) | |
| const schedule = "30 */2 * * *"; | |
| cron.schedule(schedule, checkUnassignedDevices, { | |
| scheduled: true, | |
| }); | |
| // Execute immediately on startup | |
| checkUnassignedDevices().catch(error => | |
| logger.error(`Initial execution failed: ${error.message}`) | |
| ); |
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created a new GitHub issue for this as requested: #3743
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as requested by the user
gh issue create --title "Consider immediate execution and schedule documentation for unassigned devices job" --body "As discussed in [PR #3740](https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344814), we should consider adding immediate execution and schedule documentation for the unassigned devices job.
Requested by @Baalmart.
Reference PR: https://github.com/airqo-platform/AirQo-api/pull/3740
Reference Comment: https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344814" --assignee Baalmart
Length of output: 597
| const activeMissingStatusResult = await DeviceModel("airqo").aggregate([ | ||
| { | ||
| $match: { | ||
| isActive: true, | ||
| status: { $exists: false } || { $eq: null } || { $eq: "" }, | ||
| }, |
There was a problem hiding this comment.
Apply the same fix to the aggregation query.
The same logical issue exists in the aggregation pipeline.
Here's the correction:
$match: {
isActive: true,
- status: { $exists: false } || { $eq: null } || { $eq: "" },
+ $or: [
+ { status: { $exists: false } },
+ { status: null },
+ { status: "" }
+ ]
},📝 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.
| const activeMissingStatusResult = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true, | |
| status: { $exists: false } || { $eq: null } || { $eq: "" }, | |
| }, | |
| const activeMissingStatusResult = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true, | |
| $or: [ | |
| { status: { $exists: false } }, | |
| { status: null }, | |
| { status: "" } | |
| ], | |
| }, |
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue for this logical issue. You can find it here: #3746.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue for this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue for the logical issue in the aggregation query.
gh issue create \
--title "Logical issue in aggregation query in check-active-statuses.js" \
--body "A logical issue exists in the aggregation pipeline in 'check-active-statuses.js' that needs to be addressed.
PR: https://github.com/airqo-platform/AirQo-api/pull/3740
Comment: https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344875
Reported by @Baalmart." \
--assignee Baalmart
Length of output: 459
| cron.schedule(schedule, checkActiveStatuses, { | ||
| scheduled: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cron job configuration with timezone and retry options.
While the basic scheduling works, consider adding timezone configuration and retry mechanisms for improved reliability.
Here's an enhanced configuration:
cron.schedule(schedule, checkActiveStatuses, {
scheduled: true,
+ timezone: "UTC",
+ retry: {
+ retries: 3,
+ onRetry: (error, attempt) => {
+ logger.warn(`Retry attempt ${attempt} due to error: ${error.message}`);
+ }
+ }
});Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
please create a new Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created a new GitHub issue as requested: #3744.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as requested by @Baalmart.
gh issue create \
--title "Enhance cron job configuration with timezone and retry options" \
--body "## Enhancement Suggestion
Consider adding timezone configuration and retry mechanisms to the cron job in \`src/device-registry/bin/jobs/check-active-statuses.js\` for improved reliability.
**Original Discussion:** https://github.com/airqo-platform/AirQo-api/pull/3740#discussion_r1813344880
**Requested by:** @Baalmart
"
Length of output: 477
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | ||
| isActive: true, | ||
| status: { $exists: false } || { $eq: null } || { $eq: "" }, | ||
| }); |
There was a problem hiding this comment.
Fix invalid MongoDB query syntax for status check.
The current condition status: { $exists: false } || { $eq: null } || { $eq: "" } won't work as expected. In MongoDB, logical OR operations should use the $or operator.
Here's the corrected query:
const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({
isActive: true,
- status: { $exists: false } || { $eq: null } || { $eq: "" },
+ $or: [
+ { status: { $exists: false } },
+ { status: null },
+ { status: "" }
+ ]
});📝 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.
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | |
| isActive: true, | |
| status: { $exists: false } || { $eq: null } || { $eq: "" }, | |
| }); | |
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | |
| isActive: true, | |
| $or: [ | |
| { status: { $exists: false } }, | |
| { status: null }, | |
| { status: "" } | |
| ] | |
| }); |
| const percentageActiveIncorrectStatus = | ||
| (activeIncorrectStatusCount / | ||
| (await DeviceModel("airqo").countDocuments({ isActive: true }))) * | ||
| 100; | ||
| const percentageActiveMissingStatus = | ||
| (activeMissingStatusCount / | ||
| (await DeviceModel("airqo").countDocuments({ isActive: true }))) * | ||
| 100; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries for percentage calculations.
The code makes separate database calls for the denominator in percentage calculations, which could be optimized.
Consider calculating the total count once:
+ const totalActiveDevices = await DeviceModel("airqo").countDocuments({ isActive: true });
const percentageActiveIncorrectStatus =
- (activeIncorrectStatusCount /
- (await DeviceModel("airqo").countDocuments({ isActive: true }))) *
- 100;
+ (activeIncorrectStatusCount / totalActiveDevices) * 100;
const percentageActiveMissingStatus =
- (activeMissingStatusCount /
- (await DeviceModel("airqo").countDocuments({ isActive: true }))) *
- 100;
+ (activeMissingStatusCount / totalActiveDevices) * 100;📝 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.
| const percentageActiveIncorrectStatus = | |
| (activeIncorrectStatusCount / | |
| (await DeviceModel("airqo").countDocuments({ isActive: true }))) * | |
| 100; | |
| const percentageActiveMissingStatus = | |
| (activeMissingStatusCount / | |
| (await DeviceModel("airqo").countDocuments({ isActive: true }))) * | |
| 100; | |
| const totalActiveDevices = await DeviceModel("airqo").countDocuments({ isActive: true }); | |
| const percentageActiveIncorrectStatus = | |
| (activeIncorrectStatusCount / totalActiveDevices) * 100; | |
| const percentageActiveMissingStatus = | |
| (activeMissingStatusCount / totalActiveDevices) * 100; |
| require("@bin/jobs/check-unassigned-devices-job"); | ||
| require("@bin/jobs/check-active-statuses"); | ||
| require("@bin/jobs/check-unassigned-sites-job"); |
There was a problem hiding this comment.
💡 Codebase verification
The AI summary is correct - jobs are scheduled to run every 2 hours
The code clearly shows that all three jobs are configured to run every 2 hours at minute 30 ("30 */2 * * *"), not every 30 minutes as mentioned in the PR objectives. The cron expressions and their inline comments in all three files confirm this:
check-unassigned-devices-job.js: "30 */2 * * *" // At minute 30 of every 2nd hourcheck-active-statuses.js: "30 */2 * * *" // At minute 30 of every 2nd hourcheck-unassigned-sites-job.js: "30 */2 * * *" // At minute 30 of every 2nd hour
The PR objectives should be updated to match the actual implementation, or the scheduling should be modified to run every 30 minutes if that's the intended frequency.
🔗 Analysis chain
Verify consistency with PR objectives.
The PR objectives mention these jobs should run every 30 minutes, but the AI summary indicates every 2 hours. This discrepancy should be clarified.
Let's check the actual scheduling:
The previous attempt failed because we were looking in the wrong directory. The imports in server.js use "@bin" which is likely an alias, so let's try to locate these files first and then examine their scheduling logic.
Now that we've located the correct files, let's examine their scheduling configuration to verify the timing discrepancy mentioned in the review comment.
We need to find the actual schedule definition to verify the timing. Let's check both direct schedule definitions and any potential configuration imports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Extract cron expressions to verify scheduling frequency
echo "Checking cron scheduling expressions..."
rg "cron\.schedule.*['\"].*['\"]" bin/jobs/check-unassigned-devices-job.js bin/jobs/check-active-statuses.js bin/jobs/check-unassigned-sites-job.js
Length of output: 1363
Script:
#!/bin/bash
# Let's first find the correct paths for these job files using fd
echo "Finding job files in the repository..."
fd -t f "check-(unassigned|active)" -E "node_modules"
# Once we find the files, let's check their content for scheduling
echo "Checking content of found files for scheduling patterns..."
fd -t f "check-(unassigned|active)" -E "node_modules" -x rg -l "cron\.schedule|setInterval|setTimeout"
# Let's also try to find these files under src directory specifically
echo "Checking under src directory..."
fd -t f "check-(unassigned|active)" -E "node_modules" . src/
Length of output: 1800
Script:
#!/bin/bash
# Now that we found the correct paths, let's examine the scheduling logic in these files
echo "Checking scheduling in check-unassigned-devices-job.js..."
rg -A 3 "cron\.schedule|setInterval|setTimeout" "src/device-registry/bin/jobs/check-unassigned-devices-job.js"
echo "Checking scheduling in check-active-statuses.js..."
rg -A 3 "cron\.schedule|setInterval|setTimeout" "src/device-registry/bin/jobs/check-active-statuses.js"
echo "Checking scheduling in check-unassigned-sites-job.js..."
rg -A 3 "cron\.schedule|setInterval|setTimeout" "src/device-registry/bin/jobs/check-unassigned-sites-job.js"
Length of output: 894
Script:
#!/bin/bash
# Let's check the schedule variable definition in these files
echo "Checking schedule definition in check-unassigned-devices-job.js..."
rg "schedule.*=|const.*schedule" "src/device-registry/bin/jobs/check-unassigned-devices-job.js"
echo "Checking schedule definition in check-active-statuses.js..."
rg "schedule.*=|const.*schedule" "src/device-registry/bin/jobs/check-active-statuses.js"
echo "Checking schedule definition in check-unassigned-sites-job.js..."
rg "schedule.*=|const.*schedule" "src/device-registry/bin/jobs/check-unassigned-sites-job.js"
# Also check for any environment variables or config imports that might define the schedule
echo "Checking for config or env imports..."
rg "require.*config|process\.env|import.*config" "src/device-registry/bin/jobs/check-unassigned-devices-job.js" "src/device-registry/bin/jobs/check-active-statuses.js" "src/device-registry/bin/jobs/check-unassigned-sites-job.js"
Length of output: 1518
Description
This pull request introduces three new jobs and their associated unit test files to enhance monitoring and reporting capabilities within the system. The jobs focus on checking unassigned devices, unassigned sites, and active statuses. Each job includes error handling and logging mechanisms.
Key features:
These additions will significantly enhance the system's ability to monitor and report on the status of deployed devices and sites, improving overall operational efficiency and reliability.
Changes Made
/bin/jobs/check-unassigned-sites-job.js/bin/jobs/check-unassigned-devices-job.js/bin/jobs/check-active-statuses-job.jscheckUnassignedSites: Checks for unassigned sitescheckUnassignedDevices: Checks for unassigned devicescheckActiveStatuses: Checks for incorrect and missing statuses of active devicesut_check-unassigned-sites.jsut_check-unassigned-devices.jsut_check-active-statuses.jsTesting
Affected Services
API Documentation Updated?
Additional Notes
These changes introduce robust monitoring capabilities, improve error detection, and enhance the overall system's ability to track and report on device and site statuses.
Summary by CodeRabbit
New Features
Bug Fixes
Tests