first version of the averages API #3762
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce two new methods, Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3762 +/- ##
===========================================
- Coverage 11.84% 11.71% -0.14%
===========================================
Files 113 113
Lines 14938 15162 +224
Branches 274 274
===========================================
+ Hits 1770 1776 +6
- Misses 13168 13386 +218
|
|
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: 11
🧹 Outside diff range and nitpick comments (1)
src/device-registry/controllers/create-event.js (1)
1234-1261: Add time range validation and response documentation.The averaging functionality should include:
- Validation for time range parameters to ensure meaningful averages
- JSDoc documentation describing the response format and time window
Add validation and documentation:
+/** + * List average measurements for sites/devices + * @param {Object} req - Express request object + * @param {Object} req.query + * @param {string} [req.query.start_date] - Start date for averaging window (ISO format) + * @param {string} [req.query.end_date] - End date for averaging window (ISO format) + * @returns {Object} Response containing averages and metadata + */ listAverages: async (req, res, next) => { + // Validate time range + const { start_date, end_date } = req.query; + if (start_date && end_date) { + const start = new Date(start_date); + const end = new Date(end_date); + if (isNaN(start.getTime()) || isNaN(end.getTime()) || start > end) { + return res.status(httpStatus.BAD_REQUEST).json({ + success: false, + errors: { message: "Invalid time range provided" }, + message: "Bad Request Error", + }); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/device-registry/controllers/create-event.js (1 hunks)
- src/device-registry/models/Event.js (1 hunks)
- src/device-registry/routes/v2/measurements.js (1 hunks)
- src/device-registry/utils/create-event.js (3 hunks)
🔇 Additional comments (3)
src/device-registry/routes/v2/measurements.js (1)
Line range hint
1-24: Enhance API security with additional headers and request validation.While the current implementation includes basic security measures, consider adding:
- Additional security headers
- Request size limits
- More robust error handling
[security]
Add security middleware at the router level:const helmet = require('helmet'); const express = require('express'); const router = express.Router(); // Add security headers router.use(helmet()); // Limit request size router.use(express.json({ limit: '10kb' })); // Global error handler router.use((err, req, res, next) => { console.error(err.stack); res.status(500).json({ success: false, message: 'Internal Server Error', errors: { message: process.env.NODE_ENV === 'production' ? 'An unexpected error occurred' : err.message } }); });src/device-registry/utils/create-event.js (2)
Line range hint
2131-2157: LGTM: Cache ID generation update is consistentThe addition of the
averagesparameter to the cache ID generation follows the existing pattern and maintains consistency with other parameters.
818-840: Consider adding rate limiting and improving cache securityThe caching mechanism could benefit from additional security measures:
- Consider implementing rate limiting to prevent abuse
- Add cache key versioning to handle breaking changes
- Consider using shorter cache expiration for sensitive data
Let's verify the cache configuration:
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | ||
| success: false, | ||
| errors: { | ||
| message: `Unable to process measurements for the provided measurement IDs`, | ||
| }, | ||
| message: "Internal Server Error", | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with better categorization and logging.
The error handling could be improved with:
- More specific error categories for better client-side handling
- Structured error logging for better debugging
} catch (error) {
+ const errorContext = {
+ method: 'listAverages',
+ params: req.params,
+ query: req.query,
+ error: error.message,
+ stack: error.stack,
+ };
- logger.error(`🐛🐛 Internal Server Error ${error.message}`);
+ logger.error('Internal Server Error in listAverages', errorContext);
next(
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
- { message: error.message }
+ {
+ message: error.message,
+ code: 'AVERAGES_ERROR',
+ context: errorContext,
+ }
)
);
return;
}📝 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.
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | |
| success: false, | |
| errors: { | |
| message: `Unable to process measurements for the provided measurement IDs`, | |
| }, | |
| message: "Internal Server Error", | |
| }); | |
| } | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| return; | |
| } | |
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | |
| success: false, | |
| errors: { | |
| message: `Unable to process measurements for the provided measurement IDs`, | |
| }, | |
| message: "Internal Server Error", | |
| }); | |
| } | |
| } catch (error) { | |
| const errorContext = { | |
| method: 'listAverages', | |
| params: req.params, | |
| query: req.query, | |
| error: error.message, | |
| stack: error.stack, | |
| }; | |
| logger.error('Internal Server Error in listAverages', errorContext); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { | |
| message: error.message, | |
| code: 'AVERAGES_ERROR', | |
| context: errorContext, | |
| } | |
| ) | |
| ); | |
| return; | |
| } |
| 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 }; |
There was a problem hiding this comment.
🛠️ 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.
| 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 }; |
| .withMessage("the site_id should be provided") | ||
| .bail() | ||
| .notEmpty() | ||
| .withMessage("the provided site_id cannot be empty") | ||
| .bail() | ||
| .trim() | ||
| .isMongoId() | ||
| .withMessage("the site_id must be an object ID") | ||
| .bail() | ||
| .customSanitizer((value) => { | ||
| return ObjectId(value); | ||
| }), | ||
| ], | ||
| ]), | ||
| oneOf([ | ||
| [ | ||
| query("startTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("startTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("startTime must be a valid datetime."), | ||
| query("endTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("endTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("endTime must be a valid datetime."), | ||
| query("frequency") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the frequency cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["hourly", "daily", "raw", "minute"]) | ||
| .withMessage( | ||
| "the frequency value is not among the expected ones which include: hourly, daily, minute and raw" | ||
| ), | ||
| query("format") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the format cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["json", "csv"]) | ||
| .withMessage( | ||
| "the format value is not among the expected ones which include: csv and json" | ||
| ), | ||
| query("external") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("external cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the external value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("recent") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("recent cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the recent value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("metadata") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("metadata cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["site", "site_id", "device", "device_id"]) | ||
| .withMessage( | ||
| "valid values include: site, site_id, device and device_id" | ||
| ), | ||
| query("test") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("test cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage("valid values include: YES and NO"), | ||
| ], | ||
| ]), | ||
| eventController.listAverages | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding documentation and rate limiting for the averages endpoint.
While the implementation follows good validation practices, consider:
- Adding JSDoc comments to document the route's purpose, expected response format, and example usage
- Implementing rate limiting for this endpoint as average calculations can be resource-intensive
Add documentation above the route:
/**
* @route GET /sites/:site_id/averages
* @description Get average measurements for a specific site
* @param {string} site_id - MongoDB ObjectId of the site
* @query {string} [tenant] - Optional tenant identifier
* @query {string} [startTime] - ISO8601 start time
* @query {string} [endTime] - ISO8601 end time
* @query {string} [frequency] - Data frequency (hourly|daily|raw|minute)
* @returns {object} Average measurements data
*/Consider implementing rate limiting using Express middleware:
const rateLimit = require('express-rate-limit');
const averagesLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100 // limit each IP to 100 requests per windowMs
});
router.get('/sites/:site_id/averages', averagesLimiter, /* ... */);| query("startTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("startTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("startTime must be a valid datetime."), | ||
| query("endTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("endTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("endTime must be a valid datetime."), | ||
| query("frequency") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the frequency cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["hourly", "daily", "raw", "minute"]) | ||
| .withMessage( | ||
| "the frequency value is not among the expected ones which include: hourly, daily, minute and raw" | ||
| ), | ||
| query("format") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the format cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["json", "csv"]) | ||
| .withMessage( | ||
| "the format value is not among the expected ones which include: csv and json" | ||
| ), | ||
| query("external") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("external cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the external value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("recent") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("recent cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the recent value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("metadata") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("metadata cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["site", "site_id", "device", "device_id"]) | ||
| .withMessage( | ||
| "valid values include: site, site_id, device and device_id" | ||
| ), | ||
| query("test") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("test cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage("valid values include: YES and NO"), | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication by extracting common validation rules.
The validation rules for query parameters (startTime, endTime, frequency, etc.) are duplicated across multiple routes. Consider extracting these into reusable validation chains.
Create a validation utilities module:
// validation/measurement-params.js
const { query } = require('express-validator');
const commonValidators = {
timeRange: [
query('startTime')
.optional()
.notEmpty()
.withMessage('startTime cannot be empty IF provided')
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage('startTime must be a valid datetime.'),
query('endTime')
/* ... similar validation ... */
],
frequency: [
query('frequency')
/* ... existing validation ... */
],
// ... other common validators
};
module.exports = commonValidators;Then use in routes:
const { timeRange, frequency } = require('./validation/measurement-params');
router.get('/sites/:site_id/averages',
oneOf([/* tenant validation */]),
oneOf([/* site_id validation */]),
timeRange,
frequency,
/* other validators */,
eventController.listAverages
);There was a problem hiding this comment.
generate a github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as you requested: Issue #3768.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue for this as you requested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Reduce code duplication in validation rules" \
--body "Code duplication has been observed in the validation rules for query parameters across multiple routes.
This issue has been requested by @Baalmart.
Related PR: [#3762](https://github.com/airqo-platform/AirQo-api/pull/3762)
Comment URL: https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816357999" \
--assignee Baalmart
Length of output: 472
| @@ -2541,6 +2541,132 @@ eventSchema.statics.signal = async function(filter) { | |||
| } | |||
| }; | |||
|
|
|||
| eventSchema.statics.getAirQualityAverages = async function(siteId, next) { | |||
There was a problem hiding this comment.
🛠️ 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.
| eventSchema.statics.getAirQualityAverages = async function(siteId, next) { | |
| eventSchema.statics.getAirQualityAverages = async function(siteId) { |
There was a problem hiding this comment.
generate a Github issue for this
There was a problem hiding this comment.
@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
src/device-registry/models/Event.js
Outdated
| const percentageDifference = | ||
| ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) / | ||
| previousWeek.weeklyAverage) * | ||
| 100; |
There was a problem hiding this comment.
Handle potential division by zero in percentage calculation
If previousWeek.weeklyAverage is zero, dividing by zero will result in Infinity or NaN. To maintain robustness, please add a check to prevent division by zero.
Apply this diff to safeguard the calculation:
const percentageDifference =
- ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) /
- previousWeek.weeklyAverage) *
- 100;
+ previousWeek.weeklyAverage !== 0
+ ? ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) /
+ previousWeek.weeklyAverage) *
+ 100
+ : 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.
| const percentageDifference = | |
| ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) / | |
| previousWeek.weeklyAverage) * | |
| 100; | |
| const percentageDifference = | |
| previousWeek.weeklyAverage !== 0 | |
| ? ((currentWeek.weeklyAverage - previousWeek.weeklyAverage) / | |
| previousWeek.weeklyAverage) * | |
| 100 | |
| : 0; |
| pm2_5: "$values.pm2_5.value", | ||
| dayOfYear: { $dayOfYear: "$values.time" }, | ||
| year: { $year: "$values.time" }, | ||
| week: { $week: "$values.time" }, |
There was a problem hiding this comment.
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.
| week: { $week: "$values.time" }, | |
| week: { $isoWeek: "$values.time" }, |
src/device-registry/models/Event.js
Outdated
| const twoWeeksAgo = new Date(); | ||
| twoWeeksAgo.setDate(today.getDate() - 14); |
There was a problem hiding this comment.
Correct the calculation of twoWeeksAgo
The current method of calculating twoWeeksAgo may produce incorrect dates, especially around month boundaries. It's advisable to base twoWeeksAgo on today to ensure accuracy.
Apply this diff to fix the date calculation:
-const twoWeeksAgo = new Date();
-twoWeeksAgo.setDate(today.getDate() - 14);
+const twoWeeksAgo = new Date(today);
+twoWeeksAgo.setDate(twoWeeksAgo.getDate() - 14);📝 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 twoWeeksAgo = new Date(); | |
| twoWeeksAgo.setDate(today.getDate() - 14); | |
| const twoWeeksAgo = new Date(today); | |
| twoWeeksAgo.setDate(twoWeeksAgo.getDate() - 14); |
| `Internal Server Error --- getAirQualityAverages --- ${error.message}` | ||
| ); | ||
| logObject("error", error); | ||
| next( | ||
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | ||
| message: error.message, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| `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, | |
| }); | |
| } |
There was a problem hiding this comment.
please create a Github issue for this.
There was a problem hiding this comment.
@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
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
src/device-registry/routes/v2/readings.js (2)
286-296: Simplify Validation: Remove Unnecessary 'oneOf' Wrapper for 'tenant'The
oneOffunction is used when there are alternative validation chains. Since there's only one validation chain for thetenantquery parameter, consider removing theoneOfwrapper to simplify the code.Suggested change:
-router.get( "/sites/:site_id/averages", - oneOf([ query("tenant") .optional() .notEmpty() .withMessage("tenant should not be empty if provided") .bail() .trim() .toLowerCase() .isIn(constants.NETWORKS) .withMessage("the tenant value is not among the expected ones"), - ]),
297-314: Simplify Validation: Remove Unnecessary 'oneOf' and Array for 'site_id'The validation for
site_idis wrapped insideoneOfand an array, which is unnecessary since there's only a single validation chain. Simplify the code by removingoneOfand the array.Suggested change:
query("tenant") // Tenant validation logic - ]), - oneOf([ - [ param("site_id") .exists() .withMessage("the site_id should be provided") .bail() .notEmpty() .withMessage("the provided site_id cannot be empty") .bail() .trim() .isMongoId() .withMessage("the site_id must be an object ID") .bail() .customSanitizer((value) => { return ObjectId(value); }), - ], - ]),src/device-registry/models/Reading.js (4)
538-552: Handle division by zero in compliance calculationsIn the
calculateCompliancefunction, if any of the WHO guideline values are zero, dividing by zero will occur. While the current guidelines have non-zero values, adding a check enhances robustness.You might implement the check like this:
pm2_5: { compliant: averages.pm2_5 <= WHO_GUIDELINES.pm2_5, - percentage: (averages.pm2_5 / WHO_GUIDELINES.pm2_5) * 100, + percentage: WHO_GUIDELINES.pm2_5 !== 0 ? (averages.pm2_5 / WHO_GUIDELINES.pm2_5) * 100 : null, },
579-589: Enhance trend calculation logic for accuracyThe current method uses a fixed 5% threshold to determine trends, which might not be suitable for all data sets. Consider making the threshold configurable or employing statistical methods to determine significant changes more accurately.
Exploring statistical approaches could provide more meaningful insights into the trends.
642-652: Simplify the recursive rounding functionThe
roundObjectfunction can be simplified for better readability without affecting its functionality.Here's an alternative implementation:
const roundObject = (obj) => { return Object.fromEntries( Object.entries(obj).map(([key, value]) => [ key, - typeof value === "object" && value !== null ? roundObject(value) : - typeof value === "number" ? Number(value.toFixed(2)) : value, + (typeof value === "number") ? Number(value.toFixed(2)) : + (value && typeof value === "object") ? roundObject(value) : value, ]) ); };
642-652: Avoid mutating objects within recursive functionsTo prevent unintended side effects, ensure that the
roundObjectfunction does not mutate the original objects. This practice enhances the reliability of your code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/device-registry/controllers/create-event.js (2 hunks)
- src/device-registry/models/Reading.js (1 hunks)
- src/device-registry/routes/v2/readings.js (2 hunks)
- src/device-registry/utils/create-event.js (4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/controllers/create-event.js
[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
[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
🔇 Additional comments (6)
src/device-registry/utils/create-event.js (2)
1386-1502: Apply the same refactoring patterns as listAveragesThis method has similar patterns and issues as the
listAveragesmethod.Please apply the same refactoring suggestions provided for the
listAveragesmethod to maintain consistency and reduce code duplication.
Line range hint
2248-2274: LGTM: Cache key generation updated correctlyThe addition of the
averagesparameter to the cache key generation follows the existing pattern and maintains consistency.src/device-registry/routes/v2/readings.js (2)
7-7: Import Addition: Included 'param' from 'express-validator'Adding
paramto the import statement is appropriate for parameter validation in the new route.
284-285: New Route Defined for Average ReadingsThe new GET route
'/sites/:site_id/averages'is correctly defined to fetch average readings by site ID.src/device-registry/models/Reading.js (2)
375-386: Ensure correct handling of date boundaries and time zonesThe current date calculations use local time, which might lead to inconsistencies if the server's time zone differs from the data's time zone. It's important to handle dates in a consistent time zone, preferably UTC, to avoid any discrepancies.
Please verify whether the date boundaries should be adjusted to UTC to align with the data's time zone.
362-693: Overall, the analytics function is well-implementedThe
getAirQualityAnalyticsfunction is well-structured and thoughtfully implements the required analytics. The use of aggregation pipelines and concurrent execution is efficient, and error handling is appropriately managed.
|
|
||
| if (cacheResult.success === true) { | ||
| logText(cacheResult.message); | ||
| return cacheResult.data; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Errors -- ${stringify(error)}`); | ||
| } | ||
|
|
||
| const responseFromListEvents = await EventModel( | ||
| tenant | ||
| ).getAirQualityAverages(site_id, next); | ||
|
|
||
| if ( | ||
| language !== undefined && | ||
| !isEmpty(responseFromListEvents) && | ||
| responseFromListEvents.success === true && | ||
| !isEmpty(responseFromListEvents.data) | ||
| ) { | ||
| const data = responseFromListEvents.data; | ||
| for (const event of data) { | ||
| const translatedHealthTips = await translateUtil.translateTips( | ||
| { healthTips: event.health_tips, targetLanguage: language }, | ||
| next | ||
| ); | ||
| if (translatedHealthTips.success === true) { | ||
| event.health_tips = translatedHealthTips.data; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (responseFromListEvents.success === true) { | ||
| const data = !isEmpty(missingDataMessage) | ||
| ? [] | ||
| : responseFromListEvents.data; | ||
|
|
||
| logText("Setting cache..."); | ||
|
|
||
| try { | ||
| const resultOfCacheOperation = await Promise.race([ | ||
| createEvent.setCache(data, request, next), | ||
| new Promise((resolve) => | ||
| setTimeout(resolve, 60000, { | ||
| success: false, | ||
| message: "Internal Server Error", | ||
| status: httpStatus.INTERNAL_SERVER_ERROR, | ||
| errors: { message: "Cache timeout" }, | ||
| }) | ||
| ), | ||
| ]); | ||
| if (resultOfCacheOperation.success === false) { | ||
| const errors = resultOfCacheOperation.errors | ||
| ? resultOfCacheOperation.errors | ||
| : { message: "Internal Server Error" }; | ||
| logger.error(`🐛🐛 Internal Server Error -- ${stringify(errors)}`); | ||
| // return resultOfCacheOperation; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Errors -- ${stringify(error)}`); | ||
| } | ||
|
|
||
| logText("Cache set."); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: !isEmpty(missingDataMessage) | ||
| ? missingDataMessage | ||
| : isEmpty(data) | ||
| ? "no measurements for this search" | ||
| : responseFromListEvents.message, | ||
| data, | ||
| status: responseFromListEvents.status || "", | ||
| isCache: false, | ||
| }; | ||
| } else { | ||
| logger.error( | ||
| `Unable to retrieve events --- ${stringify( | ||
| responseFromListEvents.errors | ||
| )}` | ||
| ); | ||
|
|
||
| return { | ||
| success: false, | ||
| message: responseFromListEvents.message, | ||
| errors: responseFromListEvents.errors || { message: "" }, | ||
| status: responseFromListEvents.status || "", | ||
| isCache: false, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor opportunity: Extract common patterns and improve error handling
The listAverages method has several areas that could be improved:
- The cache timeout value (60000ms) is hardcoded in multiple places
- The
missingDataMessagevariable is initialized but never populated - The method shares significant code patterns with other methods like
listandview - Missing input validation for
site_id
Consider these improvements:
+ // Define constants at the top of the file
+ const CACHE_TIMEOUT_MS = 60000;
+
+ // Extract common cache handling logic
+ async function handleCache(request, next) {
+ try {
+ const cacheResult = await Promise.race([
+ createEvent.getCache(request, next),
+ new Promise((resolve) =>
+ setTimeout(resolve, CACHE_TIMEOUT_MS, {
+ success: false,
+ message: "Internal Server Error",
+ status: httpStatus.INTERNAL_SERVER_ERROR,
+ errors: { message: "Cache timeout" },
+ })
+ ),
+ ]);
+
+ if (cacheResult.success === true) {
+ return cacheResult.data;
+ }
+ return null;
+ } catch (error) {
+ logger.error(`🐛🐛 Cache Error: ${stringify(error)}`);
+ return null;
+ }
+ }
+ // Add input validation
+ function validateSiteId(site_id) {
+ if (!site_id) {
+ throw new HttpError(
+ "Bad Request",
+ httpStatus.BAD_REQUEST,
+ { message: "site_id is required" }
+ );
+ }
+ }Then update the method:
listAverages: async (request, next) => {
try {
+ const { site_id, tenant, language } = {
+ ...request.query,
+ ...request.params,
+ };
+
+ validateSiteId(site_id);
+
- let missingDataMessage = ""; // Remove unused variable
+ const cacheData = await handleCache(request, next);
+ if (cacheData) {
+ return cacheData;
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Create a Github issue for this
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been created successfully: #3773.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue to track this refactor opportunity.
@Baalmart, thank you for bringing this to my attention. The backticks in the script were causing command substitution errors. I've corrected the script by escaping the backticks so they're treated as literal characters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track the refactor opportunity.
gh issue create \
--title "Refactor `listAverages` method in `create-event.js`" \
--body "The `listAverages` method in \`src/device-registry/utils/create-event.js\` can be refactored to improve error handling and code reuse.
Consider extracting common cache handling logic, defining constants for cache timeouts, and adding input validation for \`site_id\`.
Refer to PR [#3762](https://github.com/airqo-platform/AirQo-api/pull/3762) and [this comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816402973).
Requested by @Baalmart."
Length of output: 857
Script:
#!/bin/bash
# Create a GitHub issue to track the refactor opportunity.
gh issue create \
--title "Refactor \\\`listAverages\\\` method in \\\`create-event.js\\\`" \
--body "The \\\`listAverages\\\` method in \\\`src/device-registry/utils/create-event.js\\\` can be refactored to improve error handling and code reuse.
Consider extracting common cache handling logic, defining constants for cache timeouts, and adding input validation for \\\`site_id\\\`.
Refer to PR [#3762](https://github.com/airqo-platform/AirQo-api/pull/3762) and [this comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816402973).
Requested by @Baalmart."
Length of output: 618
| oneOf([ | ||
| [ | ||
| query("startTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("startTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("startTime must be a valid datetime."), | ||
| query("endTime") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("endTime cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .isISO8601({ strict: true, strictSeparator: true }) | ||
| .withMessage("endTime must be a valid datetime."), | ||
| query("frequency") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the frequency cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["hourly", "daily", "raw", "minute"]) | ||
| .withMessage( | ||
| "the frequency value is not among the expected ones which include: hourly, daily, minute and raw" | ||
| ), | ||
| query("format") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the format cannot be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["json", "csv"]) | ||
| .withMessage( | ||
| "the format value is not among the expected ones which include: csv and json" | ||
| ), | ||
| query("external") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("external cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the external value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("recent") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("recent cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage( | ||
| "the recent value is not among the expected ones which include: no and yes" | ||
| ), | ||
| query("metadata") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("metadata cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["site", "site_id", "device", "device_id"]) | ||
| .withMessage( | ||
| "valid values include: site, site_id, device and device_id" | ||
| ), | ||
| query("test") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("test cannot be empty IF provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["yes", "no"]) | ||
| .withMessage("valid values include: YES and NO"), | ||
| ], | ||
| ]), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Suggestion: Extract Common Validation Logic
The validation rules for query parameters like startTime, endTime, frequency, etc., are similar across multiple routes. Consider refactoring them into reusable middleware functions or validation schemas to reduce code duplication and enhance maintainability.
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been created successfully: airqo-platform/AirQo-api#3770. Please let me know if there's anything else I can assist with.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue to track this refactoring suggestion as per your request.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for refactoring common validation logic
gh issue create \
--title "Refactor Common Validation Logic in Routes" \
--body "The validation rules for query parameters like \`startTime\`, \`endTime\`, \`frequency\`, etc., are similar across multiple routes such as in \`src/device-registry/routes/v2/readings.js\`. Consider refactoring them into reusable middleware functions or validation schemas to reduce code duplication and enhance maintainability.
Referenced in PR [#3762](https://github.com/airqo-platform/AirQo-api/pull/3762) and [this comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816402975).
Requested by @Baalmart." \
--assignee Baalmart
Length of output: 679
| const WHO_GUIDELINES = { | ||
| pm2_5: 5, // μg/m³ | ||
| pm10: 15, // μg/m³ | ||
| no2: 10, // μg/m³ | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize WHO guidelines for maintainability
Hardcoding the WHO air quality guidelines within the function makes updates more cumbersome. To enhance maintainability, consider externalizing these values into a configuration file or constants module.
This approach will make it easier to update the guidelines in the future without modifying the function logic.
There was a problem hiding this comment.
Generate a Github issue for this
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been created: #3772. Thank you for addressing this.
🔗 Analysis chain
@Baalmart, I've created a GitHub issue to track this suggestion. Thank you for addressing this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to externalize WHO Air Quality Guidelines
gh issue create \
--title "Externalize WHO Air Quality Guidelines in getAirQualityAnalytics()" \
--body "In [PR #3762](https://github.com/airqo-platform/AirQo-api/pull/3762), within the \`getAirQualityAnalytics()\` method in \`src/device-registry/models/Reading.js\`, the WHO air quality guidelines are hardcoded. To enhance maintainability, these values should be externalized into a configuration file or constants module.
Original PR comment: [Link to the comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816402978)
Requested by @Baalmart." \
--assignee Baalmart
Length of output: 642
| const lastValue = data[data.length - 1]; | ||
| const previousValue = data[data.length - 2]; | ||
| const percentChange = ((lastValue - previousValue) / previousValue) * 100; |
There was a problem hiding this comment.
Prevent potential division by zero in trend calculation
In the calculateTrend function, if previousValue is zero, dividing by zero will cause an error. To safeguard against this, consider adding a check for zero before performing the division.
You can modify the calculation as follows:
const percentChange =
- ((lastValue - previousValue) / previousValue) * 100;
+ previousValue === 0 ? 0 : ((lastValue - previousValue) / previousValue) * 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 lastValue = data[data.length - 1]; | |
| const previousValue = data[data.length - 2]; | |
| const percentChange = ((lastValue - previousValue) / previousValue) * 100; | |
| const lastValue = data[data.length - 1]; | |
| const previousValue = data[data.length - 2]; | |
| const percentChange = previousValue === 0 ? 0 : ((lastValue - previousValue) / previousValue) * 100; |
| site_id: siteId, | ||
| time: { | ||
| $gte: today, | ||
| $lt: tomorrow, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize query performance with appropriate indexing
The aggregation pipeline matches on site_id and time. Ensuring these fields are properly indexed can significantly improve query performance.
Consider verifying that indexes exist on these fields:
+ReadingsSchema.index({ site_id: 1, time: 1 });Committable suggestion was skipped due to low confidence.
| const weeklyData = processWeeklyData(weeklyComparison); | ||
| const dailyAverages = { | ||
| pm2_5: | ||
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / | ||
| 24, | ||
| pm10: | ||
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm10, 0) / | ||
| 24, | ||
| no2: | ||
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.no2, 0) / 24, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Consider adjusting the calculation of daily averages
Currently, the daily averages are calculated by summing the hourly pollutant values and dividing by 24. If there are missing data points for some hours, this could result in inaccurate averages. It would be more accurate to divide by the actual number of data points available.
You might consider the following adjustment:
const dailyAverages = {
pm2_5:
- Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / 24,
+ Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / Object.keys(hourlyData).length,
pm10:
- Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm10, 0) / 24,
+ Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm10, 0) / Object.keys(hourlyData).length,
no2:
- Object.values(hourlyData).reduce((sum, hour) => sum + hour.no2, 0) / 24,
+ Object.values(hourlyData).reduce((sum, hour) => sum + hour.no2, 0) / Object.keys(hourlyData).length,
};📝 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 weeklyData = processWeeklyData(weeklyComparison); | |
| const dailyAverages = { | |
| pm2_5: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / | |
| 24, | |
| pm10: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm10, 0) / | |
| 24, | |
| no2: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.no2, 0) / 24, | |
| }; | |
| const weeklyData = processWeeklyData(weeklyComparison); | |
| const dailyAverages = { | |
| pm2_5: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / | |
| Object.keys(hourlyData).length, | |
| pm10: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm10, 0) / | |
| Object.keys(hourlyData).length, | |
| no2: | |
| Object.values(hourlyData).reduce((sum, hour) => sum + hour.no2, 0) / | |
| Object.keys(hourlyData).length, | |
| }; |
| listReadingAverages: 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, | ||
| query: { | ||
| ...req.query, | ||
| tenant: isEmpty(req.query.tenant) ? "airqo" : req.query.tenant, | ||
| averages: "readings", | ||
| }, | ||
| }; | ||
|
|
||
| const result = await createEventUtil.listReadingAverages(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| const status = result.status || httpStatus.OK; | ||
| if (result.success === true) { | ||
| res.status(status).json({ | ||
| success: true, | ||
| message: result.message, | ||
| measurements: result.data, | ||
| }); | ||
| } else { | ||
| const errorStatus = result.status || httpStatus.INTERNAL_SERVER_ERROR; | ||
| res.status(errorStatus).json({ | ||
| success: false, | ||
| errors: result.errors || { message: "" }, | ||
| message: result.message, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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
| 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 }; | ||
|
|
||
| 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++; | ||
| } | ||
| } | ||
|
|
||
| if (locationErrors === 0) { | ||
| const result = await createEventUtil.listAverages(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| if (result.success === true) { | ||
| const status = result.status ? result.status : httpStatus.OK; | ||
|
|
||
| res.status(status).json({ | ||
| success: true, | ||
| isCache: result.isCache, | ||
| message: result.message, | ||
| measurements: result.data, | ||
| }); | ||
| } else if (result.success === false) { | ||
| const status = result.status | ||
| ? result.status | ||
| : httpStatus.INTERNAL_SERVER_ERROR; | ||
| const errors = result.errors ? result.errors : { message: "" }; | ||
| res.status(status).json({ | ||
| success: false, | ||
| errors, | ||
| message: result.message, | ||
| }); | ||
| } | ||
| } else { | ||
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | ||
| success: false, | ||
| errors: { | ||
| message: `Unable to process measurements for the provided measurement IDs`, | ||
| }, | ||
| message: "Internal Server Error", | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add unit tests for listAverages to enhance test coverage.
The new method listAverages lacks unit tests, as highlighted by the static analysis hints. Adding unit tests will help verify the functionality and ensure any future changes do not introduce regressions.
🧰 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
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | ||
| success: false, | ||
| errors: { | ||
| message: `Unable to process measurements for the provided measurement IDs`, | ||
| }, | ||
| message: "Internal Server Error", | ||
| }); |
There was a problem hiding this comment.
Use appropriate HTTP status code for client-side errors.
The error response uses httpStatus.INTERNAL_SERVER_ERROR, but the issue arises due to client input (e.g., missing or invalid measurement IDs). Consider using httpStatus.BAD_REQUEST to indicate a client-side error, and update the message accordingly.
Apply this diff to adjust the status code and message:
- res.status(httpStatus.INTERNAL_SERVER_ERROR).json({
+ res.status(httpStatus.BAD_REQUEST).json({
success: false,
errors: {
message: `Unable to process measurements for the provided measurement IDs`,
},
- message: "Internal Server Error",
+ message: "Bad Request",
});📝 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.
| res.status(httpStatus.INTERNAL_SERVER_ERROR).json({ | |
| success: false, | |
| errors: { | |
| message: `Unable to process measurements for the provided measurement IDs`, | |
| }, | |
| message: "Internal Server Error", | |
| }); | |
| res.status(httpStatus.BAD_REQUEST).json({ | |
| success: false, | |
| errors: { | |
| message: `Unable to process measurements for the provided measurement IDs`, | |
| }, | |
| message: "Bad Request", | |
| }); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1314-1314: src/device-registry/controllers/create-event.js#L1314
Added line #L1314 was not covered by tests
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
src/device-registry/models/Reading.js (1)
690-698: Consider adding specific error types.The error handling could be improved by categorizing different types of errors (e.g., database errors, calculation errors) to provide more specific error messages.
} catch (error) { logger.error(`🐛🐛 Internal Server Error -- ${error.message}`); + const errorType = error.name === 'MongoError' ? 'Database Error' : 'Calculation Error'; next( - new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { + new HttpError(errorType, httpStatus.INTERNAL_SERVER_ERROR, { message: error.message, }) );src/device-registry/models/Event.js (3)
2553-2559: Consider adding index hint for query optimizationThe base query matches on
values.site_idandvalues.time. Adding an index hint could improve query performance.Consider creating a compound index:
eventSchema.index({ "values.site_id": 1, "values.time": 1 });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2553-2553: src/device-registry/models/Event.js#L2553
Added line #L2553 was not covered by tests
2616-2623: Enhance error response with more detailsThe insufficient data response could be more informative.
Consider enhancing the error response:
return { success: false, message: "Insufficient data for comparison", + data: { + availableWeeks: result.length, + requiredWeeks: 2, + timeRange: { + start: twoWeeksAgo.toISOString(), + end: now.toISOString() + } + }, status: httpStatus.NOT_FOUND, };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2617-2618: src/device-registry/models/Event.js#L2617-L2618
Added lines #L2617 - L2618 were not covered by tests
2544-2671: Add test coverage for getAirQualityAverages methodThe static analysis indicates that the new method lacks test coverage. This is critical functionality that should be thoroughly tested.
Would you like me to help create comprehensive test cases covering:
- Success scenarios with various data patterns
- Edge cases (empty data, single week data)
- Error handling scenarios
- Date calculation accuracy
- Aggregation result validation
I can generate a complete test suite if you'd like.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2545-2545: src/device-registry/models/Event.js#L2545
Added line #L2545 was not covered by tests
[warning] 2547-2550: src/device-registry/models/Event.js#L2547-L2550
Added lines #L2547 - L2550 were not covered by tests
[warning] 2553-2553: src/device-registry/models/Event.js#L2553
Added line #L2553 was not covered by tests
[warning] 2561-2561: src/device-registry/models/Event.js#L2561
Added line #L2561 was not covered by tests
[warning] 2617-2618: src/device-registry/models/Event.js#L2617-L2618
Added lines #L2617 - L2618 were not covered by testssrc/device-registry/controllers/create-event.js (1)
941-992: Enhance logging for better observability.Consider adding debug logs to track the flow of data through this endpoint, similar to other methods in this controller.
Add logging statements:
listReadingAverages: async (req, res, next) => { try { + logText("processing reading averages request..."); const errors = extractErrorsFromRequest(req); if (errors) { + logger.error(`Bad request errors: ${JSON.stringify(errors)}`); next( new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) ); return; } const request = { ...req, query: { ...req.query, tenant: isEmpty(req.query.tenant) ? "airqo" : req.query.tenant, averages: "readings", }, }; + logObject("request query parameters", request.query); const result = await createEventUtil.listReadingAverages(request, next); + logObject("reading averages result", result);🧰 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/device-registry/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
- src/device-registry/controllers/create-event.js (2 hunks)
- src/device-registry/models/Event.js (1 hunks)
- src/device-registry/models/Reading.js (1 hunks)
- src/device-registry/package.json (1 hunks)
- src/device-registry/routes/v2/measurements.js (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/controllers/create-event.js
[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
[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 testssrc/device-registry/models/Event.js
[warning] 2545-2545: src/device-registry/models/Event.js#L2545
Added line #L2545 was not covered by tests
[warning] 2547-2550: src/device-registry/models/Event.js#L2547-L2550
Added lines #L2547 - L2550 were not covered by tests
[warning] 2553-2553: src/device-registry/models/Event.js#L2553
Added line #L2553 was not covered by tests
[warning] 2561-2561: src/device-registry/models/Event.js#L2561
Added line #L2561 was not covered by tests
[warning] 2617-2618: src/device-registry/models/Event.js#L2617-L2618
Added lines #L2617 - L2618 were not covered by tests
🔇 Additional comments (11)
src/device-registry/package.json (2)
57-57: Good addition of rate limiting middleware!Adding
express-rate-limitis a great security practice to protect your API endpoints from potential DoS attacks and excessive usage. The version7.4.1is recent and includes important security fixes.
57-57: Verify rate limit configuration for averages endpoints.Since these endpoints perform averaging calculations which could be computationally intensive, let's verify if the rate limit of 100 requests per 15-minute window is appropriate for your use case.
Consider factors like:
- Expected number of client applications
- Typical request patterns
- Server resource capacity
✅ Verification successful
Rate limit configuration appears appropriate for the use case
The rate limit of 100 requests per 15-minute window for the averages endpoint is reasonable based on the following findings:
- The rate limit is specifically applied only to the
/sites/:site_id/averagesendpoint, not impacting other measurement endpoints- The implementation in
measurements.jsshows proper configuration withwindowMs: 15 * 60 * 1000andmax: 100- The codebase has multiple other measurement endpoints (
/historical,/recent,/latestetc.) that remain unrestricted, providing alternative access points if needed- The rate limit is per IP address, allowing reasonable usage while preventing abuse
- The averaging endpoint appears to be one of many available endpoints, so restricting it does not severely impact overall API usability
The current limit of ~6.7 requests per minute per IP should be sufficient for most client applications while protecting server resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for rate limiter configuration to verify the limits rg -A 5 "express-rate-limit" rg -A 5 "RateLimit"Length of output: 3079
Script:
#!/bin/bash # Let's check the usage of averagesLimiter and any related endpoints rg -A 10 "averagesLimiter" src/device-registry/routes/v2/measurements.js # Also check for any averaging-related routes or calculations rg -A 5 "average" src/device-registry/routes/v2/measurements.js # Let's see the actual route handlers to understand the computation rg -A 10 "router\.(get|post)" src/device-registry/routes/v2/measurements.jsLength of output: 6938
Script:
#!/bin/bash # Let's examine the implementation of listAverages to understand the computational complexity ast-grep --pattern 'listAverages = async $_' src/device-registry/controllers/event.controller.js # Check if there are any existing performance-related configurations or comments rg -A 5 "performance|intensive|heavy" src/device-registry/ # Look for any documentation about API usage patterns fd README.md --exec cat {} \;Length of output: 79146
src/device-registry/models/Reading.js (4)
362-372: LGTM! Input validation is properly implemented.The function correctly validates the required
siteIdparameter and returns an appropriate error response if missing.
389-393: Previous review comment is still valid.The WHO guidelines should be externalized into a configuration file.
659-689: LGTM! Response structure is well-organized.The response object is properly structured with clear naming and includes all necessary analytics data.
401-424: Consider adding indexes to improve query performance.The aggregation pipelines heavily rely on
site_idandtimefields. While there are existing indexes, verify they are being utilized effectively.Also applies to: 426-447, 449-494
✅ Verification successful
Let me search for index definitions in the codebase since we can't directly query the database. This will help us verify if appropriate indexes are defined in the schema/model files.
Indexes are properly configured for the aggregation pipeline
The codebase shows that appropriate indexes are already defined in the Reading model:
- Compound index on
{device_id: 1, time: 1}- Compound index on
{device: 1, time: 1}- Compound index on
{site_id: 1, time: 1}These indexes, particularly the
{site_id: 1, time: 1}index, will efficiently support the aggregation pipeline's$matchstage that filters onsite_idandtimefields. The index order matches the query pattern, making it optimal for the given query.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if indexes are being used for the aggregation queries echo "Checking index usage for site_id and time fields..." mongo --eval 'db.readings.getIndexes()' | grep -E "site_id|time" # Verify if the indexes are being used in the aggregation pipeline mongo --eval 'db.readings.explain("executionStats").aggregate([ { $match: { site_id: "sample_id", time: { $gte: new Date() } } } ])'Length of output: 462
Script:
#!/bin/bash # Let's check for index definitions in the schema or model files echo "Searching for index definitions in schema/model files..." rg -l "index|createIndex" --type js # Look for specific index definitions for site_id and time echo -e "\nChecking for site_id and time index definitions..." rg "(?i)(?:index|createIndex).*(?:site_id|time)" --type js -A 2 -B 2 # Look for mongoose schema definitions that might contain index configurations echo -e "\nChecking mongoose schema definitions..." ast-grep --pattern 'new Schema({ $$$ })'Length of output: 36222
src/device-registry/models/Event.js (2)
2570-2576: Use ISO week for consistent week numberingThe current week calculation might give inconsistent results across different locales.
A past review comment already suggested using
$isoWeekinstead of$week. This change is still needed.
2660-2670: Refactor error handling to use async/await patternThe error handling should be modernized and consistent.
A past review comment already suggested throwing errors instead of using the
nextcallback. Additionally, consider using try-catch at the controller level.src/device-registry/controllers/create-event.js (1)
1314-1321: LGTM: Improved error response.The error response now correctly uses
httpStatus.BAD_REQUESTinstead ofINTERNAL_SERVER_ERRORfor client-side validation failures, which is more appropriate.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1314-1314: src/device-registry/controllers/create-event.js#L1314
Added line #L1314 was not covered by testssrc/device-registry/routes/v2/measurements.js (2)
19-23: LGTM! Well-configured rate limiting.The rate limiter configuration with 100 requests per 15-minute window provides good protection against potential abuse while allowing reasonable usage of the averages endpoint.
1203-1330: Well-structured implementation of the averages endpoint.The implementation includes:
- Clear documentation
- Comprehensive parameter validation
- Rate limiting protection
- Consistent error handling
Let's verify the controller implementation:
✅ Verification successful
Controller implementation for averages endpoint is properly structured and complete
The implementation follows a clean architecture with proper separation of concerns:
- Controller (
create-event.js) handles request validation and response formatting- Utility layer (
create-event.jsutils) manages business logic and data retrieval- Model layer (
Event.js) implements the coregetAirQualityAveragesfunctionality- Proper error handling and status codes are implemented at each layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of listAverages in the controller # Expect: Method implementation with proper error handling and average calculation logic # Search for the listAverages method implementation ast-grep --pattern 'listAverages = async (req, res, next) => { $$$ }' # Look for average calculation logic rg -l "getAirQualityAverages|calculateAverages" --type jsLength of output: 209
Script:
#!/bin/bash # Let's check the implementation in the Event model and utils rg -A 10 "getAirQualityAverages|calculateAverages" src/device-registry/models/Event.js src/device-registry/utils/create-event.js # Also check for the controller file that might contain listAverages fd -e js controller src/device-registry --exec rg -l "listAverages" # If we find the controller, let's examine its implementation fd -e js controller src/device-registry --exec rg -A 20 "listAverages"Length of output: 2645
Script:
#!/bin/bash # Let's find the event controller specifically fd -t f "event.controller.js|events.controller.js" src/device-registry # And check its content for listAverages fd -t f "event.controller.js|events.controller.js" src/device-registry --exec cat {} # Also check for any test files that might verify this functionality fd -t f -e test.js src/device-registry --exec rg -l "listAverages"Length of output: 610
Script:
#!/bin/bash # Let's correct the fd command syntax and search for controller files fd -t f "event.controller.js|events.controller.js" . # Search for listAverages in all JS files under controllers directory fd -t f -e js . controllers --exec rg -A 10 "listAverages" {} # Look for the route handler implementation rg -A 10 "listAverages" --type jsLength of output: 3421
| // Calculate trends | ||
| const calculateTrend = (data) => { | ||
| if (data.length < 2) return "insufficient_data"; | ||
|
|
||
| const lastValue = data[data.length - 1]; | ||
| const previousValue = data[data.length - 2]; | ||
| const percentChange = | ||
| previousValue === 0 | ||
| ? 0 | ||
| : ((lastValue - previousValue) / previousValue) * 100; | ||
|
|
||
| if (percentChange > 5) return "increasing"; | ||
| if (percentChange < -5) return "decreasing"; | ||
| return "stable"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance trend calculation with configurable thresholds.
The trend calculation uses hardcoded thresholds (5% and -5%). Consider making these configurable to allow for different sensitivity levels.
- if (percentChange > 5) return "increasing";
- if (percentChange < -5) return "decreasing";
+ const TREND_THRESHOLD = config.analytics.trendThreshold || 5;
+ if (percentChange > TREND_THRESHOLD) return "increasing";
+ if (percentChange < -TREND_THRESHOLD) return "decreasing";Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@Baalmart, the GitHub issue has been successfully created: #3774
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to enhance trend calculation thresholds
gh issue create \
--title "Enhance trend calculation with configurable thresholds" \
--body "The trend calculation in the \`ReadingsSchema\` uses hardcoded thresholds (5% and -5%). It would be beneficial to make these thresholds configurable to allow for different sensitivity levels.
References:
- Pull Request: https://github.com/airqo-platform/AirQo-api/pull/3762
- Comment: https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816465489
Requested by @Baalmart." \
--assignee "Baalmart"
Length of output: 553
| // Get current date boundaries | ||
| const today = new Date(); | ||
| today.setHours(0, 0, 0, 0); | ||
| const tomorrow = new Date(today); | ||
| tomorrow.setDate(tomorrow.getDate() + 1); | ||
|
|
||
| // Calculate time boundaries | ||
| const currentWeekStart = new Date(today); | ||
| currentWeekStart.setDate(currentWeekStart.getDate() - 7); | ||
| const lastWeekStart = new Date(currentWeekStart); | ||
| lastWeekStart.setDate(lastWeekStart.getDate() - 7); | ||
| const monthStart = new Date(today); | ||
| monthStart.setDate(1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a date utility library for better maintainability.
The date calculations are correct but could benefit from using a library like date-fns or moment.js to improve readability and reduce potential bugs.
- const today = new Date();
- today.setHours(0, 0, 0, 0);
- const tomorrow = new Date(today);
- tomorrow.setDate(tomorrow.getDate() + 1);
+ const today = startOfDay(new Date());
+ const tomorrow = addDays(today, 1);
+ const currentWeekStart = subDays(today, 7);
+ const lastWeekStart = subDays(currentWeekStart, 7);
+ const monthStart = startOfMonth(today);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as requested: #3775
🔗 Analysis chain
@Baalmart, I'll create a GitHub issue to track this suggestion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Consider using a date utility library for better maintainability" \
--body "As suggested in [PR #3762](https://github.com/airqo-platform/AirQo-api/pull/3762), in the [comment](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816465493), @Baalmart requested to consider using a date utility library like \`date-fns\` or \`moment.js\` to improve readability and reduce potential bugs in date calculations.\n\nRequest from @Baalmart." \
--assignee Baalmart
Length of output: 546
| const roundObject = (obj) => { | ||
| return Object.entries(obj).reduce((acc, [key, value]) => { | ||
| if (typeof value === "object" && value !== null) { | ||
| acc[key] = roundObject(value); | ||
| } else { | ||
| acc[key] = | ||
| typeof value === "number" ? Number(value.toFixed(2)) : value; | ||
| } | ||
| return acc; | ||
| }, {}); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the roundObject function.
The roundObject function is called multiple times with potentially similar objects. Memoization could improve performance for large datasets.
+ const memoizedRoundObject = memoize(roundObject);
const response = {
data: {
- dailyAverage: roundObject(dailyAverages),
- hourlyBreakdown: roundObject(hourlyData),
+ dailyAverage: memoizedRoundObject(dailyAverages),
+ hourlyBreakdown: memoizedRoundObject(hourlyData),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as requested: #3776
🔗 Analysis chain
@Baalmart, I've created a GitHub issue for this as per your request.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for the suggestion to memoize the roundObject function in getAirQualityAnalytics.
gh issue create \
--title "Consider memoizing the roundObject function in getAirQualityAnalytics" \
--body "The function \`roundObject\` in \`src/device-registry/models/Reading.js\` is called multiple times with potentially similar objects. Memoization could improve performance for large datasets.
Reference: [Pull Request #3762](https://github.com/airqo-platform/AirQo-api/pull/3762)
Comment: [CodeRabbit suggestion](https://github.com/airqo-platform/AirQo-api/pull/3762#discussion_r1816465495)
Requester: @Baalmart" \
--assignee 'Baalmart'
Length of output: 589
| const now = new Date(); | ||
| const today = new Date(now.getFullYear(), now.getMonth(), now.getDate()); | ||
| const twoWeeksAgo = new Date(today); | ||
| twoWeeksAgo.setDate(today.getDate() - 14); |
There was a problem hiding this comment.
🛠️ 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.
| 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
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@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
| listReadingAverages: 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, | ||
| query: { | ||
| ...req.query, | ||
| tenant: isEmpty(req.query.tenant) ? "airqo" : req.query.tenant, | ||
| averages: "readings", | ||
| }, | ||
| }; | ||
|
|
||
| const result = await createEventUtil.listReadingAverages(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| const status = result.status || httpStatus.OK; | ||
| if (result.success === true) { | ||
| res.status(status).json({ | ||
| success: true, | ||
| message: result.message, | ||
| measurements: result.data, | ||
| }); | ||
| } else { | ||
| const errorStatus = result.status || httpStatus.INTERNAL_SERVER_ERROR; | ||
| res.status(errorStatus).json({ | ||
| success: false, | ||
| errors: result.errors || { message: "" }, | ||
| message: result.message, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
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
| 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 }; | ||
|
|
||
| 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++; | ||
| } | ||
| } | ||
|
|
||
| if (locationErrors === 0) { | ||
| const result = await createEventUtil.listAverages(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| if (result.success === true) { | ||
| const status = result.status ? result.status : httpStatus.OK; | ||
|
|
||
| res.status(status).json({ | ||
| success: true, | ||
| isCache: result.isCache, | ||
| message: result.message, | ||
| measurements: result.data, | ||
| }); | ||
| } else if (result.success === false) { | ||
| const status = result.status | ||
| ? result.status | ||
| : httpStatus.INTERNAL_SERVER_ERROR; | ||
| const errors = result.errors ? result.errors : { message: "" }; | ||
| res.status(status).json({ | ||
| success: false, | ||
| errors, | ||
| message: result.message, | ||
| }); | ||
| } | ||
| } else { | ||
| res.status(httpStatus.BAD_REQUEST).json({ | ||
| 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( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
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
|
|
||
| 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++; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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
There was a problem hiding this comment.
Create a Github issue for this.
There was a problem hiding this comment.
@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
Description
first version of the averages API
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
{{baseUrl}}/api/v2/devices/measurements/sites/{SITE_ID}/averages{{baseUrl}}/api/v2/devices/readings/sites/{SITE_ID}/averagesAPI Documentation Updated?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
/sites/:site_id/averages.Bug Fixes
Documentation