centralising the AQI categorisation logic #3854
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@Baalmart has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #3854 +/- ##
===========================================
+ Coverage 11.76% 11.78% +0.01%
===========================================
Files 114 114
Lines 15218 15220 +2
Branches 274 274
===========================================
+ Hits 1791 1793 +2
Misses 13427 13427
|
Baalmart
changed the title
|
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: 3
🧹 Outside diff range and nitpick comments (4)
src/device-registry/models/Reading.js (3)
49-77: Consider adding value range validation for AQI categoriesThe AQI range schema is well-structured, but could benefit from additional validation to ensure that:
- Category ranges don't overlap
- Each category's max is greater than its min
- Categories form a continuous range (no gaps)
Consider adding custom validation:
const AqiRangeSchema = new Schema( { good: { - min: { type: Number, required: true }, - max: { type: Number, required: true }, + min: { + type: Number, + required: true, + validate: { + validator: function(v) { return v >= 0; }, + message: 'min value must be non-negative' + } + }, + max: { + type: Number, + required: true, + validate: { + validator: function(v) { + return v > this.min && v === this.parent.moderate.min; + }, + message: 'max must be greater than min and equal to next category min' + } + }, }, // Similar validation for other categories }, { _id: false, validateModifiedOnly: true } ); +AqiRangeSchema.pre('validate', function(next) { + const categories = ['good', 'moderate', 'u4sg', 'unhealthy', 'very_unhealthy', 'hazardous']; + for (let i = 0; i < categories.length - 1; i++) { + const current = this[categories[i]]; + const next = this[categories[i + 1]]; + if (current.max !== next.min) { + return next(new Error(`Gap detected between ${categories[i]} and ${categories[i + 1]}`)); + } + } + next(); +});
Line range hint
449-452: Add guard clause for empty hourly dataThe daily averages calculation could throw a division by zero error if
hourlyDatais empty.Add a guard clause:
- const dailyAverages = { - pm2_5: - Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / - Object.keys(hourlyData).length, + const hourlyDataLength = Object.keys(hourlyData).length; + const dailyAverages = hourlyDataLength === 0 ? { + pm2_5: 0, + pm10: 0, + no2: 0 + } : { + pm2_5: + Object.values(hourlyData).reduce((sum, hour) => sum + hour.pm2_5, 0) / + hourlyDataLength,
Line range hint
449-603: Consider implementing caching for analyticsThe
getAirQualityAnalyticsmethod performs complex aggregations that could benefit from caching, especially for frequently accessed sites.Consider implementing a caching strategy:
- Cache analytics results with a reasonable TTL (e.g., 15 minutes)
- Use site_id as cache key
- Implement cache invalidation when new readings are added
Example implementation using Redis:
const CACHE_TTL = 900; // 15 minutes in seconds async function getAirQualityAnalytics(siteId, next) { const cacheKey = `analytics:${siteId}`; // Try to get from cache first const cachedData = await redisClient.get(cacheKey); if (cachedData) { return JSON.parse(cachedData); } // If not in cache, compute and store const result = // ... existing analytics computation ... await redisClient.setex(cacheKey, CACHE_TTL, JSON.stringify(result)); return result; }src/device-registry/models/Event.js (1)
881-936: Consider encapsulating AQI calculation logic into a reusable functionThe logic for determining
aqi_color,aqi_category, andaqi_color_nameis repeated multiple times in different aggregation pipelines (lines 881-936, 958-1024, 1038-1104, etc.). Encapsulating this logic into a reusable function or pipeline can improve readability and maintainability.Since MongoDB aggregation pipelines don't directly support function calls, consider creating a custom aggregation pipeline stage or using a library function if possible. Alternatively, you could pre-process the data or refactor the logic to reduce repetition.
Also applies to: 958-1024, 1038-1104, 1566-1621, 1635-1691, 1705-1761, 2253-2328, 2341-2416, 2436-2512
🧰 Tools
🪛 Biome
[error] 885-885: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 898-898: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 907-907: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 920-920: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/device-registry/models/Event.js(11 hunks)src/device-registry/models/Reading.js(2 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/models/Event.js
[error] 885-885: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 898-898: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 907-907: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 920-920: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 939-939: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 962-962: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 975-975: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 984-984: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 997-997: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1016-1016: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1042-1042: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1055-1055: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1064-1064: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1077-1077: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1096-1096: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1106-1106: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1570-1570: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1579-1579: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1588-1588: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1597-1597: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1616-1616: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1639-1639: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1648-1648: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1657-1657: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1666-1666: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1685-1685: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1709-1709: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1718-1718: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1727-1727: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1736-1736: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1755-1755: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1763-1763: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2257-2258: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2276-2277: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2285-2286: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2304-2305: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2323-2324: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2329-2330: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2346-2346: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2364-2365: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2373-2373: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2392-2393: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2411-2411: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2441-2441: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2459-2460: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2468-2469: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2488-2488: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2506-2507: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2519-2520: Do not add then to an object.
(lint/suspicious/noThenProperty)
| @@ -59,6 +89,7 @@ const ReadingsSchema = new Schema( | |||
| frequency: String, | |||
| no2: { value: Number }, | |||
| siteDetails: SiteDetailsSchema, | |||
| aqi_ranges: AqiRangeSchema, | |||
There was a problem hiding this comment.
Include aqi_ranges in toJSON method
The new aqi_ranges field is added to the schema but not included in the toJSON method, which could lead to inconsistent API responses.
Update the toJSON method:
toJSON() {
return {
device: this.device,
device_id: this.device_id,
is_reading_primary: this.is_reading_primary,
health_tips: this.health_tips,
site_id: this.site_id,
time: this.time,
pm2_5: this.pm2_5,
pm10: this.pm10,
frequency: this.frequency,
no2: this.no2,
siteDetails: this.siteDetails,
+ aqi_ranges: this.aqi_ranges,
timeDifferenceHours: this.timeDifferenceHours,
aqi_color: this.aqi_color,
aqi_category: this.aqi_category,
aqi_color_name: this.aqi_color_name,
};
},📝 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.
| aqi_ranges: AqiRangeSchema, | |
| toJSON() { | |
| return { | |
| device: this.device, | |
| device_id: this.device_id, | |
| is_reading_primary: this.is_reading_primary, | |
| health_tips: this.health_tips, | |
| site_id: this.site_id, | |
| time: this.time, | |
| pm2_5: this.pm2_5, | |
| pm10: this.pm10, | |
| frequency: this.frequency, | |
| no2: this.no2, | |
| siteDetails: this.siteDetails, | |
| aqi_ranges: this.aqi_ranges, | |
| timeDifferenceHours: this.timeDifferenceHours, | |
| aqi_color: this.aqi_color, | |
| aqi_category: this.aqi_category, | |
| aqi_color_name: this.aqi_color_name, | |
| }; | |
| }, |
src/device-registry/models/Event.js
Outdated
| .addFields({ | ||
| aqi_ranges: { | ||
| good: { min: 0, max: 9.0 }, | ||
| moderate: { min: 9.1, max: 35.4 }, | ||
| u4sg: { min: 35.5, max: 55.4 }, | ||
| unhealthy: { min: 55.5, max: 125.4 }, | ||
| very_unhealthy: { min: 125.5, max: 225.4 }, | ||
| hazardous: { min: 225.5, max: null }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor aqi_ranges into a shared constant to eliminate duplication
The aqi_ranges object is defined multiple times throughout the code (lines 859-868, 1544-1553, 2231-2240). Extracting this object into a shared constant or configuration file would improve maintainability and reduce code duplication.
Consider defining aqi_ranges outside of the functions and referencing it where needed. Here's how you might refactor:
+// Define at the top of the module
+const AQI_RANGES = {
+ good: { min: 0, max: 9.0 },
+ moderate: { min: 9.1, max: 35.4 },
+ u4sg: { min: 35.5, max: 55.4 },
+ unhealthy: { min: 55.5, max: 125.4 },
+ very_unhealthy: { min: 125.5, max: 225.4 },
+ hazardous: { min: 225.5, max: null },
+};Then, in your aggregation pipelines:
- .addFields({
- aqi_ranges: {
- good: { min: 0, max: 9.0 },
- moderate: { min: 9.1, max: 35.4 },
- u4sg: { min: 35.5, max: 55.4 },
- unhealthy: { min: 55.5, max: 125.4 },
- very_unhealthy: { min: 125.5, max: 225.4 },
- hazardous: { min: 225.5, max: null },
- },
- })
+ .addFields({
+ aqi_ranges: AQI_RANGES,
+ })This change would reduce redundancy and make it easier to update the AQI ranges in the future.
Also applies to: 1544-1553, 2231-2240
src/device-registry/models/Event.js
Outdated
| case: { | ||
| $gte: ["$pm2_5.value", "$aqi_ranges.hazardous.max"], | ||
| }, |
There was a problem hiding this comment.
Correct the 'hazardous' AQI category condition in $switch statements
In the calculation of aqi_color, aqi_category, and aqi_color_name, the condition for the 'hazardous' category compares $pm2_5.value with $aqi_ranges.hazardous.max, which is null. This comparison will not function correctly. Instead, you should compare $pm2_5.value with $aqi_ranges.hazardous.min using $gte only.
Apply this diff to fix the condition:
- case: {
- $gte: ["$pm2_5.value", "$aqi_ranges.hazardous.max"],
- },
+ case: {
+ $gte: ["$pm2_5.value", "$aqi_ranges.hazardous.min"],
+ },Ensure this correction is applied consistently across all the $switch statements for the 'hazardous' category.
Also applies to: 1020-1024, 1100-1104, 1619-1621, 1689-1691, 1759-1761, 2326-2328, 2415-2422, 2510-2517
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/device-registry/models/Event.js (1)
Line range hint
868-1114: Consider refactoring duplicated AQI calculation logic into a shared utilityThe AQI calculation logic within your aggregation pipelines is repeated across multiple functions (
fetchData,signalData, andlist). To improve maintainability and reduce potential errors, it would be beneficial to abstract this logic into a shared utility or helper function.Also applies to: 1546-1764, 2226-2513
🧰 Tools
🪛 Biome
[error] 1044-1044: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1057-1057: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1066-1066: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1079-1079: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1098-1098: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1108-1108: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/device-registry/models/Event.js(12 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/models/Event.js
[error] 887-887: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 900-900: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 909-909: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 922-922: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 941-941: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 964-964: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 977-977: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 986-986: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 999-999: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1018-1018: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1044-1044: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1057-1057: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1066-1066: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1079-1079: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1098-1098: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1108-1108: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1565-1565: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1574-1574: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1583-1583: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1592-1592: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1611-1611: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1634-1634: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1643-1643: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1652-1652: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1661-1661: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1680-1680: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1704-1704: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1713-1713: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1722-1722: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1731-1731: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1750-1750: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 1758-1758: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2245-2246: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2264-2265: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2273-2274: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2292-2293: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2311-2312: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2317-2318: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2334-2334: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2352-2353: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2361-2361: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2380-2381: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2399-2399: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2429-2429: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2447-2448: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2456-2457: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2476-2476: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2494-2495: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 2507-2508: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (1)
src/device-registry/models/Event.js (1)
26-33: Great work centralizing the AQI thresholds into AQI_RANGES
Centralizing the AQI thresholds enhances maintainability by reducing duplication and making future updates more straightforward.
Description
centralising the AQI logic
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
centralising the AQI logic
Summary by CodeRabbit
New Features
Bug Fixes