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

first version of the averages API #3762

Merged
merged 9 commits into from
Oct 25, 2024
136 changes: 136 additions & 0 deletions src/device-registry/controllers/create-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,58 @@
return;
}
},
listReadingAverages: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
if (errors) {
next(

Check warning on line 945 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L942-L945

Added lines #L942 - L945 were not covered by tests
new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)
);
return;

Check warning on line 948 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L948

Added line #L948 was not covered by tests
}

const request = {

Check warning on line 951 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L951

Added line #L951 was not covered by tests
...req,
query: {
...req.query,
tenant: isEmpty(req.query.tenant) ? "airqo" : req.query.tenant,
averages: "readings",
},
};

const result = await createEventUtil.listReadingAverages(request, next);

Check warning on line 960 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L960

Added line #L960 was not covered by tests

if (isEmpty(result) || res.headersSent) {
return;

Check warning on line 963 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L962-L963

Added lines #L962 - L963 were not covered by tests
}

const status = result.status || httpStatus.OK;
if (result.success === true) {
res.status(status).json({

Check warning on line 968 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L966-L968

Added lines #L966 - L968 were not covered by tests
success: true,
message: result.message,
measurements: result.data,
});
} else {
const errorStatus = result.status || httpStatus.INTERNAL_SERVER_ERROR;
res.status(errorStatus).json({

Check warning on line 975 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L974-L975

Added lines #L974 - L975 were not covered by tests
success: false,
errors: result.errors || { message: "" },
message: result.message,
});
}
} catch (error) {
logger.error(`🐛🐛 Internal Server Error ${error.message}`);
next(

Check warning on line 983 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L982-L983

Added lines #L982 - L983 were not covered by tests
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
)
);
return;

Check warning on line 990 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L990

Added line #L990 was not covered by tests
}
Comment on lines +941 to +991
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 unit tests for listReadingAverages to improve code coverage.

The new method listReadingAverages is not currently covered by unit tests, as indicated by the static analysis tools. Implementing unit tests will help ensure the method functions correctly and maintain code reliability.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 942-945: src/device-registry/controllers/create-event.js#L942-L945
Added lines #L942 - L945 were not covered by tests


[warning] 948-948: src/device-registry/controllers/create-event.js#L948
Added line #L948 was not covered by tests


[warning] 951-951: src/device-registry/controllers/create-event.js#L951
Added line #L951 was not covered by tests


[warning] 960-960: src/device-registry/controllers/create-event.js#L960
Added line #L960 was not covered by tests


[warning] 962-963: src/device-registry/controllers/create-event.js#L962-L963
Added lines #L962 - L963 were not covered by tests


[warning] 966-968: src/device-registry/controllers/create-event.js#L966-L968
Added lines #L966 - L968 were not covered by tests


[warning] 974-975: src/device-registry/controllers/create-event.js#L974-L975
Added lines #L974 - L975 were not covered by tests


[warning] 982-983: src/device-registry/controllers/create-event.js#L982-L983
Added lines #L982 - L983 were not covered by tests


[warning] 990-990: src/device-registry/controllers/create-event.js#L990
Added line #L990 was not covered by tests

},
Comment on lines +941 to +992
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 unit tests for the new listReadingAverages endpoint.

The new endpoint lacks test coverage, which is crucial for ensuring reliability and preventing regressions. Consider adding tests that cover:

  • Success scenarios with different query parameters
  • Error handling for invalid inputs
  • Edge cases with empty/null responses

Would you like me to help create a test suite for this endpoint?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 942-945: src/device-registry/controllers/create-event.js#L942-L945
Added lines #L942 - L945 were not covered by tests


[warning] 948-948: src/device-registry/controllers/create-event.js#L948
Added line #L948 was not covered by tests


[warning] 951-951: src/device-registry/controllers/create-event.js#L951
Added line #L951 was not covered by tests


[warning] 960-960: src/device-registry/controllers/create-event.js#L960
Added line #L960 was not covered by tests


[warning] 962-963: src/device-registry/controllers/create-event.js#L962-L963
Added lines #L962 - L963 were not covered by tests


[warning] 966-968: src/device-registry/controllers/create-event.js#L966-L968
Added lines #L966 - L968 were not covered by tests


[warning] 974-975: src/device-registry/controllers/create-event.js#L974-L975
Added lines #L974 - L975 were not covered by tests


[warning] 982-983: src/device-registry/controllers/create-event.js#L982-L983
Added lines #L982 - L983 were not covered by tests


[warning] 990-990: src/device-registry/controllers/create-event.js#L990
Added line #L990 was not covered by tests

signalsForMap: async (req, res, next) => {
try {
logText("the signals for the AirQo Map...");
Expand Down Expand Up @@ -1195,6 +1247,90 @@
return;
}
},
listAverages: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
if (errors) {
next(

Check warning on line 1254 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1251-L1254

Added lines #L1251 - L1254 were not covered by tests
new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)
);
return;

Check warning on line 1257 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1257

Added line #L1257 was not covered by tests
}

const request = req;
const defaultTenant = constants.DEFAULT_TENANT || "airqo";
request.query.tenant = isEmpty(req.query.tenant)

Check warning on line 1262 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1260-L1262

Added lines #L1260 - L1262 were not covered by tests
? defaultTenant
: req.query.tenant;

request.query.recent = "no";
request.query.metadata = "site_id";
request.query.averages = "events";
request.query.brief = "yes";
const { cohort_id, grid_id } = { ...req.query, ...req.params };

Check warning on line 1270 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1266-L1270

Added lines #L1266 - L1270 were not covered by tests
Comment on lines +1250 to +1270
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 centralizing the default tenant configuration.

The default tenant value "airqo" is duplicated across multiple controller methods. Consider moving this to a single configuration constant to improve maintainability.

-const defaultTenant = constants.DEFAULT_TENANT || "airqo";
+const { DEFAULT_TENANT: defaultTenant = "airqo" } = constants;
📝 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
listAverages: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
if (errors) {
next(
new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)
);
return;
}
const request = req;
const defaultTenant = constants.DEFAULT_TENANT || "airqo";
request.query.tenant = isEmpty(req.query.tenant)
? defaultTenant
: req.query.tenant;
request.query.recent = "no";
request.query.metadata = "site_id";
request.query.averages = "events";
request.query.brief = "yes";
const { cohort_id, grid_id } = { ...req.query, ...req.params };
listAverages: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
if (errors) {
next(
new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)
);
return;
}
const request = req;
const { DEFAULT_TENANT: defaultTenant = "airqo" } = constants;
request.query.tenant = isEmpty(req.query.tenant)
? defaultTenant
: req.query.tenant;
request.query.recent = "no";
request.query.metadata = "site_id";
request.query.averages = "events";
request.query.brief = "yes";
const { cohort_id, grid_id } = { ...req.query, ...req.params };


let locationErrors = 0;

Check warning on line 1272 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1272

Added line #L1272 was not covered by tests

if (cohort_id) {
await processCohortIds(cohort_id, request);
if (isEmpty(request.query.device_id)) {
locationErrors++;

Check warning on line 1277 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1274-L1277

Added lines #L1274 - L1277 were not covered by tests
}
} else if (grid_id) {
await processGridIds(grid_id, request);
if (isEmpty(request.query.site_id)) {
locationErrors++;

Check warning on line 1282 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1279-L1282

Added lines #L1279 - L1282 were not covered by tests
}
}
Comment on lines +1271 to +1284
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring location processing logic.

The location processing logic (handling cohort_id and grid_id) is duplicated across multiple methods in this controller. Consider extracting it into a shared utility function.

Create a shared utility function:

+const processLocationIds = async (request, { cohort_id, grid_id }) => {
+  let locationErrors = 0;
+
+  if (cohort_id) {
+    await processCohortIds(cohort_id, request);
+    if (isEmpty(request.query.device_id)) {
+      locationErrors++;
+    }
+  } else if (grid_id) {
+    await processGridIds(grid_id, request);
+    if (isEmpty(request.query.site_id)) {
+      locationErrors++;
+    }
+  }
+
+  return locationErrors;
+};

 listAverages: async (req, res, next) => {
   try {
     // ... existing setup code ...
     
-    let locationErrors = 0;
-    if (cohort_id) {
-      await processCohortIds(cohort_id, request);
-      if (isEmpty(request.query.device_id)) {
-        locationErrors++;
-      }
-    } else if (grid_id) {
-      await processGridIds(grid_id, request);
-      if (isEmpty(request.query.site_id)) {
-        locationErrors++;
-      }
-    }
+    const locationErrors = await processLocationIds(request, { cohort_id, grid_id });

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1272-1272: src/device-registry/controllers/create-event.js#L1272
Added line #L1272 was not covered by tests


[warning] 1274-1277: src/device-registry/controllers/create-event.js#L1274-L1277
Added lines #L1274 - L1277 were not covered by tests


[warning] 1279-1282: src/device-registry/controllers/create-event.js#L1279-L1282
Added lines #L1279 - L1282 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create a 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 refactoring suggestion: Issue #3779.

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a new GitHub issue for refactoring the location processing logic
gh issue create \
  --title "Refactor location processing logic in create-event.js" \
  --body "As discussed in [PR #3762](https://github.com/airqo-platform/AirQo-api/pull/3762), the location processing logic for handling \`cohort_id\` and \`grid_id\` is duplicated across multiple methods in \`src/device-registry/controllers/create-event.js\`. Extracting this logic into a shared utility function would improve code maintainability and reduce redundancy.

This issue was requested by @Baalmart in [this comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816465509)." \
  --assignee "Baalmart"

Length of output: 657


if (locationErrors === 0) {
const result = await createEventUtil.listAverages(request, next);

Check warning on line 1287 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1286-L1287

Added lines #L1286 - L1287 were not covered by tests

if (isEmpty(result) || res.headersSent) {
return;

Check warning on line 1290 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1289-L1290

Added lines #L1289 - L1290 were not covered by tests
}

if (result.success === true) {
const status = result.status ? result.status : httpStatus.OK;

Check warning on line 1294 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1293-L1294

Added lines #L1293 - L1294 were not covered by tests

res.status(status).json({

Check warning on line 1296 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1296

Added line #L1296 was not covered by tests
success: true,
isCache: result.isCache,
message: result.message,
measurements: result.data,
});
} else if (result.success === false) {
const status = result.status

Check warning on line 1303 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1302-L1303

Added lines #L1302 - L1303 were not covered by tests
? result.status
: httpStatus.INTERNAL_SERVER_ERROR;
const errors = result.errors ? result.errors : { message: "" };
res.status(status).json({

Check warning on line 1307 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1306-L1307

Added lines #L1306 - L1307 were not covered by tests
success: false,
errors,
message: result.message,
});
}
} else {
res.status(httpStatus.BAD_REQUEST).json({

Check warning on line 1314 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1314

Added line #L1314 was not covered by tests
success: false,
errors: {
message: `Unable to process measurements for the provided measurement IDs`,
},
message: "Bad Request",
});
}
} catch (error) {
logger.error(`🐛🐛 Internal Server Error ${error.message}`);
next(

Check warning on line 1324 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1323-L1324

Added lines #L1323 - L1324 were not covered by tests
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
)
);
return;

Check warning on line 1331 in src/device-registry/controllers/create-event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-event.js#L1331

Added line #L1331 was not covered by tests
}
},
Comment on lines +1250 to +1333
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 unit tests for the new listAverages endpoint.

The new endpoint requires test coverage to ensure proper functionality. Consider adding tests that cover:

  • Success scenarios for both cohort and grid processing
  • Error handling for invalid cohort/grid IDs
  • Edge cases with empty/null responses

Would you like me to help create a test suite for this endpoint?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 1251-1254: src/device-registry/controllers/create-event.js#L1251-L1254
Added lines #L1251 - L1254 were not covered by tests


[warning] 1257-1257: src/device-registry/controllers/create-event.js#L1257
Added line #L1257 was not covered by tests


[warning] 1260-1262: src/device-registry/controllers/create-event.js#L1260-L1262
Added lines #L1260 - L1262 were not covered by tests


[warning] 1266-1270: src/device-registry/controllers/create-event.js#L1266-L1270
Added lines #L1266 - L1270 were not covered by tests


[warning] 1272-1272: src/device-registry/controllers/create-event.js#L1272
Added line #L1272 was not covered by tests


[warning] 1274-1277: src/device-registry/controllers/create-event.js#L1274-L1277
Added lines #L1274 - L1277 were not covered by tests


[warning] 1279-1282: src/device-registry/controllers/create-event.js#L1279-L1282
Added lines #L1279 - L1282 were not covered by tests


[warning] 1286-1287: src/device-registry/controllers/create-event.js#L1286-L1287
Added lines #L1286 - L1287 were not covered by tests


[warning] 1289-1290: src/device-registry/controllers/create-event.js#L1289-L1290
Added lines #L1289 - L1290 were not covered by tests


[warning] 1293-1294: src/device-registry/controllers/create-event.js#L1293-L1294
Added lines #L1293 - L1294 were not covered by tests


[warning] 1296-1296: src/device-registry/controllers/create-event.js#L1296
Added line #L1296 was not covered by tests


[warning] 1302-1303: src/device-registry/controllers/create-event.js#L1302-L1303
Added lines #L1302 - L1303 were not covered by tests


[warning] 1306-1307: src/device-registry/controllers/create-event.js#L1306-L1307
Added lines #L1306 - L1307 were not covered by tests


[warning] 1314-1314: src/device-registry/controllers/create-event.js#L1314
Added line #L1314 was not covered by tests


[warning] 1323-1324: src/device-registry/controllers/create-event.js#L1323-L1324
Added lines #L1323 - L1324 were not covered by tests


[warning] 1331-1331: src/device-registry/controllers/create-event.js#L1331
Added line #L1331 was not covered by tests

listHistorical: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
Expand Down
129 changes: 129 additions & 0 deletions src/device-registry/models/Event.js
Original file line number Diff line number Diff line change
Expand Up @@ -2541,6 +2541,135 @@
}
};

eventSchema.statics.getAirQualityAverages = async function(siteId, next) {
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove the next parameter from the static method

Including next in the model's static method getAirQualityAverages is unconventional for Mongoose models. It's better to handle errors by throwing them, allowing the caller to manage error propagation.

Apply this diff to refactor the method signature:

-eventSchema.statics.getAirQualityAverages = async function(siteId, next) {
+eventSchema.statics.getAirQualityAverages = async function(siteId) {
📝 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
eventSchema.statics.getAirQualityAverages = async function(siteId, next) {
eventSchema.statics.getAirQualityAverages = async function(siteId) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generate a 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, thank you for creating the GitHub issue. This will help address the refactoring suggestion.

(^.^)/

🔗 Analysis chain

@Baalmart, I've prepared a GitHub issue for this.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
gh issue create \
  --title "Refactor getAirQualityAverages method to remove unnecessary 'next' parameter" \
  --body "There's an issue with the static method \`getAirQualityAverages\` in \`eventSchema\`. Including the \`next\` parameter in this method is unconventional for Mongoose models. It's recommended to handle errors by throwing them, allowing the caller to manage error propagation.

Refer to this PR: https://github.com/airqo-platform/AirQo-api/pull/3762

Refer to the comment: https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816358012

Requested by: @Baalmart" \
  --assignee Baalmart

Length of output: 667

try {

Check warning on line 2545 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2545

Added line #L2545 was not covered by tests
// Get current date and date 2 weeks ago
const now = new Date();
const today = new Date(now.getFullYear(), now.getMonth(), now.getDate());
const twoWeeksAgo = new Date(today);
twoWeeksAgo.setDate(today.getDate() - 14);

Check warning on line 2550 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2547-L2550

Added lines #L2547 - L2550 were not covered by tests
Comment on lines +2547 to +2550
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve date calculation logic for better accuracy

The current date calculation logic could be improved for better accuracy and clarity.

Apply this diff to improve the date calculations:

-const now = new Date();
-const today = new Date(now.getFullYear(), now.getMonth(), now.getDate());
-const twoWeeksAgo = new Date(today);
-twoWeeksAgo.setDate(today.getDate() - 14);
+const now = new Date();
+const today = new Date(now.setHours(0, 0, 0, 0));
+const twoWeeksAgo = new Date(today.getTime() - (14 * 24 * 60 * 60 * 1000));

This change:

  • Ensures consistent midnight timestamp for today
  • Uses millisecond arithmetic for more precise date calculation
  • Avoids potential issues with month boundaries
📝 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 now = new Date();
const today = new Date(now.getFullYear(), now.getMonth(), now.getDate());
const twoWeeksAgo = new Date(today);
twoWeeksAgo.setDate(today.getDate() - 14);
const now = new Date();
const today = new Date(now.setHours(0, 0, 0, 0));
const twoWeeksAgo = new Date(today.getTime() - (14 * 24 * 60 * 60 * 1000));
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 2547-2550: src/device-registry/models/Event.js#L2547-L2550
Added lines #L2547 - L2550 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create a 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 as requested: #3778.

🔗 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 "Improve date calculation logic for better accuracy" \
  --body "As discussed in [PR #3762](https://github.com/airqo-platform/AirQo-api/pull/3762), it's suggested to improve the date calculation logic for better accuracy.\n\nRequester: @Baalmart\n\nRelated comment: [Link to the comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816465498)" \
  --assignee Baalmart

Length of output: 467


// Base query to match site and time range
const baseMatch = {

Check warning on line 2553 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2553

Added line #L2553 was not covered by tests
"values.site_id": mongoose.Types.ObjectId(siteId),
"values.time": {
$gte: twoWeeksAgo,
$lte: now,
},
};

const result = await this.aggregate([

Check warning on line 2561 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2561

Added line #L2561 was not covered by tests
// Unwind the values array to work with individual readings
{ $unwind: "$values" },

// Match site and time range
{ $match: baseMatch },

// Project only needed fields and add date fields for grouping
{
$project: {
time: "$values.time",
pm2_5: "$values.pm2_5.value",
dayOfYear: { $dayOfYear: "$values.time" },
year: { $year: "$values.time" },
week: { $week: "$values.time" },
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

Use $isoWeek instead of $week for ISO-compliant week numbers

The $week operator may yield inconsistent results depending on the locale and MongoDB version. Using $isoWeek ensures that week numbers conform to the ISO 8601 standard.

Apply this diff to update the operator:

-week: { $week: "$values.time" },
+week: { $isoWeek: "$values.time" },
📝 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
week: { $week: "$values.time" },
week: { $isoWeek: "$values.time" },

},
},

// Group by day to get daily averages
{
$group: {
_id: {
year: "$year",
dayOfYear: "$dayOfYear",
},
dailyAverage: { $avg: "$pm2_5" },
date: { $first: "$time" },
week: { $first: "$week" },
},
},

// Sort by date
{ $sort: { date: -1 } },

// Group again to calculate weekly averages
{
$group: {
_id: "$week",
weeklyAverage: { $avg: "$dailyAverage" },
days: {
$push: {
date: "$date",
average: "$dailyAverage",
},
},
},
},

// Sort by week
{ $sort: { _id: -1 } },

// Limit to get only last 2 weeks
{ $limit: 2 },
]).allowDiskUse(true);

// If we don't have enough data
if (result.length < 2) {
return {

Check warning on line 2618 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2617-L2618

Added lines #L2617 - L2618 were not covered by tests
success: false,
message: "Insufficient data for comparison",
status: httpStatus.NOT_FOUND,
};
}

// Get current week and previous week data
const currentWeek = result[0];
const previousWeek = result[1];

Check warning on line 2627 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2626-L2627

Added lines #L2626 - L2627 were not covered by tests

// Calculate percentage difference

const percentageDifference =
previousWeek.weeklyAverage !== 0

Check warning on line 2632 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2632

Added line #L2632 was not covered by tests
? ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) /
previousWeek.weeklyAverage) *
100
: 0;

// Get today's date string in YYYY-MM-DD format
const todayStr = today.toISOString().split("T")[0];

Check warning on line 2639 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2639

Added line #L2639 was not covered by tests

// Find today's average from the current week's days
const todayAverage =
currentWeek.days.find(
(day) => day.date.toISOString().split("T")[0] === todayStr

Check warning on line 2644 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2643-L2644

Added lines #L2643 - L2644 were not covered by tests
)?.average || null;

return {

Check warning on line 2647 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2647

Added line #L2647 was not covered by tests
success: true,
data: {
dailyAverage: todayAverage ? parseFloat(todayAverage.toFixed(2)) : null,
percentageDifference: parseFloat(percentageDifference.toFixed(2)),
weeklyAverages: {
currentWeek: parseFloat(currentWeek.weeklyAverage.toFixed(2)),
previousWeek: parseFloat(previousWeek.weeklyAverage.toFixed(2)),
},
},
message: "Successfully retrieved air quality averages",
status: httpStatus.OK,
};
} catch (error) {
logger.error(

Check warning on line 2661 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2661

Added line #L2661 was not covered by tests
`Internal Server Error --- getAirQualityAverages --- ${error.message}`
);
logObject("error", error);
next(

Check warning on line 2665 in src/device-registry/models/Event.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Event.js#L2664-L2665

Added lines #L2664 - L2665 were not covered by tests
new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, {
message: error.message,
})
);
}
Comment on lines +2662 to +2670
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor error handling to throw errors instead of calling next

Within model methods, throwing errors is preferred over calling next. This change promotes better error management and aligns with standard practices.

Apply this diff to update the error handling:

     logger.error(
       `Internal Server Error --- getAirQualityAverages --- ${error.message}`
     );
     logObject("error", error);
-    next(
-      new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, {
+    throw new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, {
         message: 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
`Internal Server Error --- getAirQualityAverages --- ${error.message}`
);
logObject("error", error);
next(
new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, {
message: error.message,
})
);
}
`Internal Server Error --- getAirQualityAverages --- ${error.message}`
);
logObject("error", error);
throw new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, {
message: 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 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, the GitHub issue has been successfully created: #3769.

🔗 Analysis chain

@Baalmart, I'll create a GitHub issue for this as requested.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Creating a new GitHub issue for refactoring error handling in getAirQualityAverages method
gh issue create \
  --title "Refactor error handling in getAirQualityAverages method" \
  --body "Within the \`getAirQualityAverages\` method in \`src/device-registry/models/Event.js\`, refactor the error handling to throw errors instead of calling \`next\`. This issue was raised during code review at [this comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816358022) in [PR #3762](https://github.com/airqo-platform/AirQo-api/pull/3762). Requested by @Baalmart." \
  --assignee Baalmart

Length of output: 554

};

const eventsModel = (tenant) => {
try {
const events = mongoose.model("events");
Expand Down
Loading
Loading