Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Three New Jobs and Their Respective Unit Tests #3740

Merged
merged 4 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions src/device-registry/bin/jobs/check-active-statuses.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
const constants = require("@config/constants");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- /bin/jobs/check-active-statuses-job`
);
const DeviceModel = require("@models/Device");
const cron = require("node-cron");
const ACTIVE_STATUS_THRESHOLD = 0;
const { logText, logObject } = require("@utils/log");

const checkActiveStatuses = async () => {
try {
// Check for Deployed devices with incorrect statuses
const activeIncorrectStatusCount = await DeviceModel(
"airqo"
).countDocuments({
isActive: true,
status: { $ne: "deployed" },
});

const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({
isActive: true,
status: { $exists: false } || { $eq: null } || { $eq: "" },
});
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 activeIncorrectStatusResult = await DeviceModel("airqo").aggregate([
{
$match: {
isActive: true,
status: { $ne: "deployed" },
},
},
{
$group: {
_id: "$name",
},
},
]);

const activeMissingStatusResult = await DeviceModel("airqo").aggregate([
{
$match: {
isActive: true,
status: { $exists: false } || { $eq: null } || { $eq: "" },
},
Comment on lines +40 to +45
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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: "" }
],
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

},
{
$group: {
_id: "$name",
},
},
]);

const activeIncorrectStatusUniqueNames = activeIncorrectStatusResult.map(
(doc) => doc._id
);
const activeMissingStatusUniqueNames = activeMissingStatusResult.map(
(doc) => doc._id
);

logObject("activeIncorrectStatusCount", activeIncorrectStatusCount);
logObject("activeMissingStatusCount", activeMissingStatusCount);

const percentageActiveIncorrectStatus =
(activeIncorrectStatusCount /
(await DeviceModel("airqo").countDocuments({ isActive: true }))) *
100;
const percentageActiveMissingStatus =
(activeMissingStatusCount /
(await DeviceModel("airqo").countDocuments({ isActive: true }))) *
100;
Comment on lines +64 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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;


logObject(
"percentageActiveIncorrectStatus",
percentageActiveIncorrectStatus
);
logObject("percentageActiveMissingStatus", percentageActiveMissingStatus);

if (
percentageActiveIncorrectStatus > ACTIVE_STATUS_THRESHOLD ||
percentageActiveMissingStatus > ACTIVE_STATUS_THRESHOLD
) {
logText(
`⁉️ Deployed devices with incorrect statuses (${activeIncorrectStatusUniqueNames.join(
", "
)}) - ${percentageActiveIncorrectStatus.toFixed(2)}%`
);
logger.info(
`⁉️ Deployed devices with incorrect statuses (${activeIncorrectStatusUniqueNames.join(
", "
)}) - ${percentageActiveIncorrectStatus.toFixed(2)}%`
);

logText(
`⁉️ Deployed devices missing status (${activeMissingStatusUniqueNames.join(
", "
)}) - ${percentageActiveMissingStatus.toFixed(2)}%`
);
logger.info(
`⁉️ Deployed devices missing status (${activeMissingStatusUniqueNames.join(
", "
)}) - ${percentageActiveMissingStatus.toFixed(2)}%`
);
}
} catch (error) {
logText(`🐛🐛 Error checking active statuses: ${error.message}`);
logger.error(`🐛🐛 Error checking active statuses: ${error.message}`);
logger.error(`🐛🐛 Stack trace: ${error.stack}`);
}
};

logText("Active statuses job is now running.....");
const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
cron.schedule(schedule, checkActiveStatuses, {
scheduled: true,
});
Comment on lines +114 to +116
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

71 changes: 71 additions & 0 deletions src/device-registry/bin/jobs/check-unassigned-devices-job.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
const constants = require("@config/constants");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- /bin/jobs/check-unassigned-devices-job`
);
const DeviceModel = require("@models/Device");
const cron = require("node-cron");
const UNASSIGNED_THRESHOLD = 0;
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

const { logText, logObject } = require("@utils/log");

const checkUnassignedDevices = async () => {
try {
const totalCount = await DeviceModel("airqo").countDocuments({
isActive: false,
});

const result = await DeviceModel("airqo").aggregate([
{
$match: {
isActive: false,
category: { $exists: false } || { $eq: "" },
},
Comment on lines +19 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
$match: {
isActive: false,
category: { $exists: false } || { $eq: "" },
},
$match: {
isActive: false,
$or: [
{ category: { $exists: false } },
{ category: "" }
]
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

},
{
$group: {
_id: "$name",
},
},
]);

const unassignedDevicesCount = result.length;
const uniqueDeviceNames = result.map((doc) => doc._id);
logObject("unassignedDevicesCount", unassignedDevicesCount);
logObject("totalCount ", totalCount);

if (unassignedDevicesCount === 0) {
return;
}

const percentage = (unassignedDevicesCount / totalCount) * 100;
Comment on lines +36 to +40
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


logObject("percentage", percentage);

if (percentage > UNASSIGNED_THRESHOLD) {
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(
", "
)})`
);
Comment on lines +45 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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(
", "
)})`
);

}
} catch (error) {
logText(`🐛🐛 Error checking unassigned devices: ${error.message}`);
logger.error(`🐛🐛 Error checking unassigned devices: ${error.message}`);
logger.error(`🐛🐛 Stack trace: ${error.stack}`);
}
};

logText("Unassigned devices job is now running.....");
const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
cron.schedule(schedule, checkUnassignedDevices, {
scheduled: true,
});
Comment on lines +67 to +71
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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}`)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please create a new Github issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

73 changes: 73 additions & 0 deletions src/device-registry/bin/jobs/check-unassigned-sites-job.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
const constants = require("@config/constants");
const log4js = require("log4js");
const logger = log4js.getLogger(
`${constants.ENVIRONMENT} -- /bin/jobs/check-unassigned-sites-job`
);
const SitesModel = require("@models/Site");
const cron = require("node-cron");
const UNASSIGNED_THRESHOLD = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making UNASSIGNED_THRESHOLD configurable

Setting UNASSIGNED_THRESHOLD to 0 means any single unassigned site will trigger warnings. Consider making this configurable through environment variables or configuration files to allow for operational flexibility.

-const UNASSIGNED_THRESHOLD = 0;
+const UNASSIGNED_THRESHOLD = process.env.UNASSIGNED_SITES_THRESHOLD || 0;
📝 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.

Suggested change
const UNASSIGNED_THRESHOLD = 0;
const UNASSIGNED_THRESHOLD = process.env.UNASSIGNED_SITES_THRESHOLD || 0;

const { logText, logObject } = require("@utils/log");

const checkUnassignedSites = async () => {
try {
// Count total number of active sites
const totalCount = await SitesModel("airqo").countDocuments({
isOnline: true,
});
Comment on lines +14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the code analysis, I can now provide a definitive response about the indexing concern.

The review comment about indexing can be refined

The Site model already has several indexes defined:

  • lat_long field has a unique index
  • generated_name field has a unique index

However, I notice that isOnline field, which is frequently used in queries, does not have an index. The grids field is an array that references the "grid" collection and is already optimized through MongoDB's default indexing of _id fields.

Recommendations:

  • Add an index for the isOnline field since it's frequently used in filtering operations
  • The existing indexes and MongoDB's default indexing adequately cover the grids array references

Example index to add:

siteSchema.index({ isOnline: 1 });
🔗 Analysis chain

Optimize database queries with proper indexing

The queries filter on isOnline and check the grids array size frequently. Consider adding appropriate indexes to improve query performance.

Also applies to: 19-25


Let me gather more information about the database schema and queries to make a well-informed verification.

This will help us understand:

  1. The schema structure and any existing indexes
  2. Usage patterns of the isOnline field
  3. Model definitions that might contain index declarations

Let's analyze the results to verify if the performance concern is valid and if indexes are indeed missing.


Let me search for any existing indexes on the Site model and specifically look for indexes on the isOnline and grids fields.

This will help us verify if there are already indexes defined for these frequently queried fields or if we need to recommend adding them.


Let me try one more search pattern to look specifically for index definitions in the Site model and related files.

This will help us:

  1. Find any index declarations specifically in Site-related files
  2. See the complete Site model implementation to understand the schema structure and any existing optimizations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if indexes exist for the queried fields
ast-grep --pattern 'schema.index({ $$$: $_$$ })'

Length of output: 50


Script:

#!/bin/bash
# Search for schema definition and indexes
rg -A 5 "new Schema\({" 

# Search for queries using isOnline field
rg "isOnline.*true" -A 3

# Look for any mongoose model definitions
rg "mongoose.model|Schema\(" -A 5

Length of output: 149305


Script:

#!/bin/bash
# Search for Site schema definition and any index declarations
rg -A 10 "const.*[Ss]ite[Ss]chema.*=" 

# Search for any explicit index calls on the Site model
rg "schema.*index|createIndex" 

Length of output: 3121


Script:

#!/bin/bash
# Search for index-related code in Site model and its usage
rg -g "**/*[Ss]ite*.js" "index\(|createIndex|ensureIndex" -A 3

# Look for the complete Site model implementation
rg -g "**/Site.js" "." -A 50

Length of output: 52337


// Find sites with empty or non-existent grids array
const result = await SitesModel("airqo").aggregate([
{
$match: {
isOnline: true,
grids: { $size: 0 },
},
},
{
$group: {
_id: "$generated_name",
},
},
]);

const unassignedSiteCount = result.length;
const uniqueSiteNames = result.map((site) => site._id);
logObject("unassignedSiteCount", unassignedSiteCount);
logObject("totalCount", totalCount);

if (unassignedSiteCount === 0) {
return;
}

const percentage = (unassignedSiteCount / totalCount) * 100;

Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add guard clause for division by zero

The percentage calculation could throw an error if totalCount is 0. Add a guard clause to handle this case.

+    if (totalCount === 0) {
+      logText("No active sites found in the system");
+      return;
+    }
     const percentage = (unassignedSiteCount / totalCount) * 100;

Committable suggestion was skipped due to low confidence.

logObject("percentage", percentage);

if (percentage > UNASSIGNED_THRESHOLD) {
logText(
`⚠️🙉 ${percentage.toFixed(
2
)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
", "
)})`
);
logger.info(
`⚠️🙉 ${percentage.toFixed(
2
)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
", "
)})`
);
}
Comment on lines +47 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reduce code duplication in logging

The same message is being logged twice using different logging methods. Consider consolidating the logging logic.

-      logText(
-        `⚠️🙉 ${percentage.toFixed(
-          2
-        )}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
-          ", "
-        )})`
-      );
+      const warningMessage = `⚠️🙉 ${percentage.toFixed(2)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(", ")})`;
+      logText(warningMessage);
       logger.info(
-        `⚠️🙉 ${percentage.toFixed(
-          2
-        )}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
-          ", "
-        )})`
+        warningMessage
       );
📝 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.

Suggested change
logText(
`⚠️🙉 ${percentage.toFixed(
2
)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
", "
)})`
);
logger.info(
`⚠️🙉 ${percentage.toFixed(
2
)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(
", "
)})`
);
}
const warningMessage = `⚠️🙉 ${percentage.toFixed(2)}% of active sites are not assigned to any grid (${uniqueSiteNames.join(", ")})`;
logText(warningMessage);
logger.info(
warningMessage
);
}

} catch (error) {
logText(`🐛🐛 Error checking unassigned sites: ${error.message}`);
logger.error(`🐛🐛 Error checking unassigned sites: ${error.message}`);
logger.error(`🐛🐛 Stack trace: ${error.stack}`);
}
};

logText("Unassigned sites job is now running.....");
const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
cron.schedule(schedule, checkUnassignedSites, {
scheduled: true,
});
Comment on lines +70 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance cron job configuration and error handling

The cron schedule could be more readable, and error handling should be added for the cron job setup.

-const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
+const CRON_SCHEDULE = process.env.UNASSIGNED_SITES_CHECK_SCHEDULE || "30 */2 * * *";
+const SCHEDULE_DESCRIPTION = "At minute 30 of every 2nd hour";
+
+// Validate cron expression
+if (!cron.validate(CRON_SCHEDULE)) {
+  logger.error(`Invalid cron schedule: ${CRON_SCHEDULE}`);
+  process.exit(1);
+}
+
 cron.schedule(schedule, checkUnassignedSites, {
   scheduled: true,
+  timezone: "UTC"  // Explicitly set timezone
 });
📝 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.

Suggested change
const schedule = "30 */2 * * *"; // At minute 30 of every 2nd hour
cron.schedule(schedule, checkUnassignedSites, {
scheduled: true,
});
const CRON_SCHEDULE = process.env.UNASSIGNED_SITES_CHECK_SCHEDULE || "30 */2 * * *";
const SCHEDULE_DESCRIPTION = "At minute 30 of every 2nd hour";
// Validate cron expression
if (!cron.validate(CRON_SCHEDULE)) {
logger.error(`Invalid cron schedule: ${CRON_SCHEDULE}`);
process.exit(1);
}
cron.schedule(schedule, checkUnassignedSites, {
scheduled: true,
timezone: "UTC" // Explicitly set timezone
});

97 changes: 97 additions & 0 deletions src/device-registry/bin/jobs/test/ut_check-active-statuses.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
require("module-alias/register");
const sinon = require("sinon");
const chai = require("chai");
const expect = chai.expect;
const sinonChai = require("sinon-chai");

const checkActiveStatuses = require("@bin/jobs/check-active-statuses");

Comment on lines +1 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add missing imports and complete chai setup.

There are several issues in the import section:

  1. logger is used but not imported
  2. DeviceModel is used but not imported
  3. sinonChai is required but not configured with chai

Apply this diff to fix the imports:

 require("module-alias/register");
 const sinon = require("sinon");
 const chai = require("chai");
 const expect = chai.expect;
 const sinonChai = require("sinon-chai");
+const { logger } = require("@utils/logger");
+const DeviceModel = require("@models/Device");
+
+chai.use(sinonChai);
📝 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.

Suggested change
require("module-alias/register");
const sinon = require("sinon");
const chai = require("chai");
const expect = chai.expect;
const sinonChai = require("sinon-chai");
const checkActiveStatuses = require("@bin/jobs/check-active-statuses");
require("module-alias/register");
const sinon = require("sinon");
const chai = require("chai");
const expect = chai.expect;
const sinonChai = require("sinon-chai");
const { logger } = require("@utils/logger");
const DeviceModel = require("@models/Device");
chai.use(sinonChai);
const checkActiveStatuses = require("@bin/jobs/check-active-statuses");

const DeviceModelMock = sinon.mock(DeviceModel);
const logTextSpy = sinon.spy(console.log);
const logObjectSpy = sinon.spy(console.log);
const loggerErrorSpy = sinon.spy(logger.error);

beforeEach(() => {
DeviceModelMock = sinon.mock(DeviceModel);
logTextSpy = sinon.spy(console.log);
logObjectSpy = sinon.spy(console.log);
loggerErrorSpy = sinon.spy(logger.error);
});

afterEach(() => {
sinon.restore();
});
Comment on lines +9 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix constant reassignments and optimize spy setup.

There are several issues in the test setup:

  1. Constants are being reassigned in beforeEach which will cause runtime errors
  2. Console.log spies are duplicated
  3. Logger spy setup could be more consistent

Apply this diff to fix the setup:

-const DeviceModelMock = sinon.mock(DeviceModel);
-const logTextSpy = sinon.spy(console.log);
-const logObjectSpy = sinon.spy(console.log);
-const loggerErrorSpy = sinon.spy(logger.error);
+let deviceModelMock;
+let consoleLogSpy;
+let loggerInfoSpy;
+let loggerErrorSpy;

 beforeEach(() => {
-  DeviceModelMock = sinon.mock(DeviceModel);
-  logTextSpy = sinon.spy(console.log);
-  logObjectSpy = sinon.spy(console.log);
-  loggerErrorSpy = sinon.spy(logger.error);
+  deviceModelMock = sinon.mock(DeviceModel);
+  consoleLogSpy = sinon.spy(console, 'log');
+  loggerInfoSpy = sinon.spy(logger, 'info');
+  loggerErrorSpy = sinon.spy(logger, 'error');
 });
📝 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.

Suggested change
const DeviceModelMock = sinon.mock(DeviceModel);
const logTextSpy = sinon.spy(console.log);
const logObjectSpy = sinon.spy(console.log);
const loggerErrorSpy = sinon.spy(logger.error);
beforeEach(() => {
DeviceModelMock = sinon.mock(DeviceModel);
logTextSpy = sinon.spy(console.log);
logObjectSpy = sinon.spy(console.log);
loggerErrorSpy = sinon.spy(logger.error);
});
afterEach(() => {
sinon.restore();
});
let deviceModelMock;
let consoleLogSpy;
let loggerInfoSpy;
let loggerErrorSpy;
beforeEach(() => {
deviceModelMock = sinon.mock(DeviceModel);
consoleLogSpy = sinon.spy(console, 'log');
loggerInfoSpy = sinon.spy(logger, 'info');
loggerErrorSpy = sinon.spy(logger, 'error');
});
afterEach(() => {
sinon.restore();
});
🧰 Tools
🪛 Biome

[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)


describe("checkActiveStatuses", () => {
describe("when devices have incorrect statuses", () => {
it("should log deployed devices with incorrect statuses", async () => {
DeviceModelMock.expects("countDocuments")
.twice()
.resolves(5);

const result = await checkActiveStatuses();

expect(logTextSpy).to.have.been.calledWith(sinon.match.string);
expect(logObjectSpy).to.have.been.calledWith(sinon.match.object);
expect(logger.info).to.have.been.calledWith(sinon.match.string);
expect(logTextSpy).to.have.been.calledWith(sinon.match.string);
expect(logger.info).to.have.been.calledWith(sinon.match.string);

sinon.assert.notCalled(loggerErrorSpy);
});
});

describe("when devices have missing status fields", () => {
it("should log deployed devices missing status", async () => {
DeviceModelMock.expects("countDocuments")
.twice()
.resolves(3);

const result = await checkActiveStatuses();

expect(logTextSpy).to.have.been.calledWith(sinon.match.string);
expect(logObjectSpy).to.have.been.calledWith(sinon.match.object);
expect(logger.info).to.have.been.calledWith(sinon.match.string);
expect(logTextSpy).to.have.been.calledWith(sinon.match.string);
expect(logger.info).to.have.been.calledWith(sinon.match.string);

sinon.assert.notCalled(loggerErrorSpy);
});
});

describe("when both conditions are met", () => {
it("should log both deployed devices with incorrect statuses and missing status fields", async () => {
DeviceModelMock.expects("countDocuments")
.thrice()
.resolves([5, 3]);

const result = await checkActiveStatuses();

expect(logTextSpy).to.have.been.calledThrice();
expect(logObjectSpy).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledWith(sinon.match.string);
expect(logger.info).to.have.been.calledWith(sinon.match.string);

sinon.assert.notCalled(loggerErrorSpy);
});
});
Comment on lines +62 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inconsistent mock resolution and verify expectations.

In the "both conditions" test:

  1. The mock resolution pattern is inconsistent with other tests
  2. Mock expectations are not verified

Apply this diff to fix the issues:

   describe("when both conditions are met", () => {
     it("should log both deployed devices with incorrect statuses and missing status fields", async () => {
-      DeviceModelMock.expects("countDocuments")
+      deviceModelMock.expects("countDocuments")
         .thrice()
-        .resolves([5, 3]);
+        .resolves(5);

       const result = await checkActiveStatuses();

       expect(logTextSpy).to.have.been.calledThrice();
       expect(logObjectSpy).to.have.been.calledTwice();
       expect(logger.info).to.have.been.calledTwice();
       expect(logger.info).to.have.been.calledWith(sinon.match.string);
       expect(logger.info).to.have.been.calledWith(sinon.match.string);

       sinon.assert.notCalled(loggerErrorSpy);
+      deviceModelMock.verify();
     });
   });
📝 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.

Suggested change
describe("when both conditions are met", () => {
it("should log both deployed devices with incorrect statuses and missing status fields", async () => {
DeviceModelMock.expects("countDocuments")
.thrice()
.resolves([5, 3]);
const result = await checkActiveStatuses();
expect(logTextSpy).to.have.been.calledThrice();
expect(logObjectSpy).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledWith(sinon.match.string);
expect(logger.info).to.have.been.calledWith(sinon.match.string);
sinon.assert.notCalled(loggerErrorSpy);
});
});
describe("when both conditions are met", () => {
it("should log both deployed devices with incorrect statuses and missing status fields", async () => {
deviceModelMock.expects("countDocuments")
.thrice()
.resolves(5);
const result = await checkActiveStatuses();
expect(logTextSpy).to.have.been.calledThrice();
expect(logObjectSpy).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledTwice();
expect(logger.info).to.have.been.calledWith(sinon.match.string);
expect(logger.info).to.have.been.calledWith(sinon.match.string);
sinon.assert.notCalled(loggerErrorSpy);
deviceModelMock.verify();
});
});


describe("when error occurs", () => {
it("should log the error message", async () => {
const error = new Error("Test error");

DeviceModelMock.expects("countDocuments").throws(error);

await checkActiveStatuses();

expect(logTextSpy).to.have.been.calledWith(sinon.match.string);
expect(logger.error).to.have.been.calledWith(sinon.match.string);
expect(logger.error).to.have.been.calledWith(sinon.match.string);
expect(logger.error).to.have.been.calledWith(sinon.match.string);

sinon.assert.notCalled(logObjectSpy);
sinon.assert.notCalled(logger.info);
});
});
});
Comment on lines +25 to +97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor test assertions for better maintainability.

The test scenarios have redundant assertions that could be simplified. Consider creating helper functions for common assertion patterns.

Here's a suggested helper function:

function verifySuccessfulExecution(consoleLogCalls, loggerInfoCalls) {
  expect(consoleLogSpy).to.have.callCount(consoleLogCalls);
  expect(logger.info).to.have.callCount(loggerInfoCalls);
  sinon.assert.notCalled(loggerErrorSpy);
  deviceModelMock.verify();
}

This can be used in your tests like:

it("should log deployed devices with incorrect statuses", async () => {
  deviceModelMock.expects("countDocuments")
    .twice()
    .resolves(5);

  await checkActiveStatuses();
  
  verifySuccessfulExecution(2, 2);
});

Loading
Loading