reconfiguring the store readings operation #3473
Conversation
|
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 8 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. WalkthroughWalkthroughThe pull request introduces a new scheduled job in the Changes
Possibly related PRs
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 #3473 +/- ##
===========================================
- Coverage 30.20% 30.17% -0.03%
===========================================
Files 184 184
Lines 24565 24596 +31
Branches 3222 3227 +5
===========================================
+ Hits 7419 7421 +2
- Misses 17031 17060 +29
Partials 115 115
|
|
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 (6)
src/device-registry/bin/new-store-readings-job.js (6)
18-18: Consider Removing Debug Logging of EntityAt line 18, you're logging the entire
entityobject usinglogObject("entity", entity);. While helpful during development, logging full entities can lead to performance overhead and potential exposure of sensitive data in logs. It would be prudent to remove this log statement or ensure that sensitive information is omitted.Apply this diff to remove the debug log:
- logObject("entity", entity);Tools
GitHub Check: codecov/patch
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
140-142: Use Logger Instead of Console for WarningsAt lines 140-142,
console.warnis used to log duplicate key errors. For consistency and better log management, consider using the logger's warning level instead.Apply this diff to use
logger.warn:- console.warn( + logger.warn( `Duplicate key error for document: ${jsonify(doc)}` );Tools
GitHub Check: codecov/patch
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
16-28: Add Unit Tests forisEntityActiveFunctionThe
isEntityActivefunction is essential for determining entity activity status, but it appears to lack test coverage. Adding unit tests will help ensure its reliability and correctness, especially with various edge cases.Would you like assistance in creating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
[warning] 21-21: src/device-registry/bin/new-store-readings-job.js#L21
Added line #L21 was not covered by tests
[warning] 24-25: src/device-registry/bin/new-store-readings-job.js#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 27-27: src/device-registry/bin/new-store-readings-job.js#L27
Added line #L27 was not covered by tests
30-45: Add Unit Tests forupdateEntityLastActiveFunctionSimilarly,
updateEntityLastActiveupdates thelastActivetimestamps of entities. Comprehensive tests can help guarantee that it behaves as expected under different scenarios, including error conditions.Let me know if you'd like help in writing unit tests for this function.
Tools
GitHub Check: codecov/patch
[warning] 30-32: src/device-registry/bin/new-store-readings-job.js#L30-L32
Added lines #L30 - L32 were not covered by tests
[warning] 36-36: src/device-registry/bin/new-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/new-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-43: src/device-registry/bin/new-store-readings-job.js#L42-L43
Added lines #L42 - L43 were not covered by tests
47-176: Add Unit Tests forfetchAndStoreDataIntoReadingsModelFunctionGiven the complexity of
fetchAndStoreDataIntoReadingsModel, thorough testing is crucial to validate its functionality and to catch potential issues early. Unit tests can simulate various responses and error conditions to ensure robustness.I'm available to assist in developing unit tests for this function if needed.
Tools
Biome
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
[warning] 48-49: src/device-registry/bin/new-store-readings-job.js#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 58-58: src/device-registry/bin/new-store-readings-job.js#L58
Added line #L58 was not covered by tests
[warning] 61-63: src/device-registry/bin/new-store-readings-job.js#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 65-66: src/device-registry/bin/new-store-readings-job.js#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 71-71: src/device-registry/bin/new-store-readings-job.js#L71
Added line #L71 was not covered by tests
[warning] 76-76: src/device-registry/bin/new-store-readings-job.js#L76
Added line #L76 was not covered by tests
[warning] 85-86: src/device-registry/bin/new-store-readings-job.js#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/device-registry/bin/new-store-readings-job.js#L88
Added line #L88 was not covered by tests
[warning] 90-92: src/device-registry/bin/new-store-readings-job.js#L90-L92
Added lines #L90 - L92 were not covered by tests
[warning] 95-98: src/device-registry/bin/new-store-readings-job.js#L95-L98
Added lines #L95 - L98 were not covered by tests
[warning] 102-107: src/device-registry/bin/new-store-readings-job.js#L102-L107
Added lines #L102 - L107 were not covered by tests
[warning] 109-109: src/device-registry/bin/new-store-readings-job.js#L109
Added line #L109 was not covered by tests
[warning] 116-116: src/device-registry/bin/new-store-readings-job.js#L116
Added line #L116 was not covered by tests
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
[warning] 130-130: src/device-registry/bin/new-store-readings-job.js#L130
Added line #L130 was not covered by tests
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
[warning] 155-156: src/device-registry/bin/new-store-readings-job.js#L155-L156
Added lines #L155 - L156 were not covered by tests
[warning] 158-158: src/device-registry/bin/new-store-readings-job.js#L158
Added line #L158 was not covered by tests
[warning] 163-163: src/device-registry/bin/new-store-readings-job.js#L163
Added line #L163 was not covered by tests
[warning] 168-169: src/device-registry/bin/new-store-readings-job.js#L168-L169
Added lines #L168 - L169 were not covered by tests
70-77: Enhance Error Logging for Unexpected Response StructureAt lines 70-77, if the response from
EventModel.fetch()is unexpected, an error is logged. To aid in troubleshooting, consider including more context, such as request parameters or additional response details.This can help identify issues with the data source or request parameters during debugging.
Tools
GitHub Check: codecov/patch
[warning] 71-71: src/device-registry/bin/new-store-readings-job.js#L71
Added line #L71 was not covered by tests
[warning] 76-76: src/device-registry/bin/new-store-readings-job.js#L76
Added line #L76 was not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/device-registry/bin/new-store-readings-job.js (1 hunks)
- src/device-registry/bin/server.js (1 hunks)
Additional context used
Biome
src/device-registry/bin/new-store-readings-job.js
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
src/device-registry/bin/new-store-readings-job.js
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
[warning] 21-21: src/device-registry/bin/new-store-readings-job.js#L21
Added line #L21 was not covered by tests
[warning] 24-25: src/device-registry/bin/new-store-readings-job.js#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 27-27: src/device-registry/bin/new-store-readings-job.js#L27
Added line #L27 was not covered by tests
[warning] 30-32: src/device-registry/bin/new-store-readings-job.js#L30-L32
Added lines #L30 - L32 were not covered by tests
[warning] 36-36: src/device-registry/bin/new-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/new-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-43: src/device-registry/bin/new-store-readings-job.js#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 48-49: src/device-registry/bin/new-store-readings-job.js#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 58-58: src/device-registry/bin/new-store-readings-job.js#L58
Added line #L58 was not covered by tests
[warning] 61-63: src/device-registry/bin/new-store-readings-job.js#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 65-66: src/device-registry/bin/new-store-readings-job.js#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 71-71: src/device-registry/bin/new-store-readings-job.js#L71
Added line #L71 was not covered by tests
[warning] 76-76: src/device-registry/bin/new-store-readings-job.js#L76
Added line #L76 was not covered by tests
[warning] 85-86: src/device-registry/bin/new-store-readings-job.js#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/device-registry/bin/new-store-readings-job.js#L88
Added line #L88 was not covered by tests
[warning] 90-92: src/device-registry/bin/new-store-readings-job.js#L90-L92
Added lines #L90 - L92 were not covered by tests
[warning] 95-98: src/device-registry/bin/new-store-readings-job.js#L95-L98
Added lines #L95 - L98 were not covered by tests
[warning] 102-107: src/device-registry/bin/new-store-readings-job.js#L102-L107
Added lines #L102 - L107 were not covered by tests
[warning] 109-109: src/device-registry/bin/new-store-readings-job.js#L109
Added line #L109 was not covered by tests
[warning] 116-116: src/device-registry/bin/new-store-readings-job.js#L116
Added line #L116 was not covered by tests
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
[warning] 130-130: src/device-registry/bin/new-store-readings-job.js#L130
Added line #L130 was not covered by tests
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
[warning] 155-156: src/device-registry/bin/new-store-readings-job.js#L155-L156
Added lines #L155 - L156 were not covered by tests
[warning] 158-158: src/device-registry/bin/new-store-readings-job.js#L158
Added line #L158 was not covered by tests
[warning] 163-163: src/device-registry/bin/new-store-readings-job.js#L163
Added line #L163 was not covered by tests
[warning] 168-169: src/device-registry/bin/new-store-readings-job.js#L168-L169
Added lines #L168 - L169 were not covered by tests
Additional comments not posted (3)
src/device-registry/bin/server.js (1)
27-27: Verify Removal ofstore-readings-joband Update ImportAt line 27, you have added
require("@bin/new-store-readings-job");. Please ensure that all references to@bin/store-readings-jobhave been appropriately updated or removed throughout the codebase to prevent any unintended side effects.Run the following script to check for any remaining references to
@bin/store-readings-job:src/device-registry/bin/new-store-readings-job.js (2)
178-182: Confirm Cron Schedule TimingThe cron schedule at line 178 is set to
"30 * * * *", which runs the job at the 30th minute of every hour. Please verify that this timing aligns with the operational requirements for fetching and storing readings.If a different schedule is intended, adjust the cron expression accordingly.
20-22: Handle Missingentity.lastActiveGracefullyIn the
isEntityActivefunction, whenentity.lastActiveis missing, you're returningfalse. Ensure that this is the desired behavior and that upstream code appropriately handles entities without alastActivetimestamp.If entities should always have a
lastActivevalue, consider logging a warning or handling this case differently.Tools
GitHub Check: codecov/patch
[warning] 21-21: src/device-registry/bin/new-store-readings-job.js#L21
Added line #L21 was not covered by tests
| await Promise.all( | ||
| batch.map(async (doc) => { | ||
| await asyncRetry( | ||
| async (bail) => { | ||
| try { | ||
| // Update Site lastActive | ||
| await updateEntityLastActive( | ||
| SiteModel("airqo"), | ||
| { _id: doc.site_id }, | ||
| doc.time | ||
| ); | ||
|
|
||
| // Update Device lastActive | ||
| await updateEntityLastActive( | ||
| DeviceModel("airqo"), | ||
| { _id: doc.device_id }, | ||
| doc.time | ||
| ); | ||
|
|
||
| // Update Reading | ||
| const filter = { site_id: doc.site_id, time: doc.time }; | ||
| const updateDoc = { ...doc }; | ||
| delete updateDoc._id; | ||
| await ReadingModel("airqo").updateOne(filter, updateDoc, { | ||
| upsert: true, | ||
| }); | ||
| } catch (error) { | ||
| logObject("the error inside processing of batches", error); | ||
| if (error.name === "MongoError" && error.code !== 11000) { | ||
| logger.error( | ||
| `🐛🐛 MongoError -- fetchAndStoreDataIntoReadingsModel -- ${jsonify( | ||
| error | ||
| )}` | ||
| ); | ||
| throw error; // Retry the operation | ||
| } else if (error.code === 11000) { | ||
| // Ignore duplicate key errors | ||
| console.warn( | ||
| `Duplicate key error for document: ${jsonify(doc)}` | ||
| ); | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| retries: 3, // Number of retry attempts | ||
| minTimeout: 1000, // Initial delay between retries (in milliseconds) | ||
| factor: 2, // Exponential factor for increasing delay between retries | ||
| } | ||
| ); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Optimize Batch Processing with Bulk Operations
Processing each document individually within the batch may lead to performance issues due to multiple database calls. Consider utilizing MongoDB's bulk operations (bulkWrite) to perform inserts and updates more efficiently.
This change can significantly improve performance by reducing the number of round trips to the database.
Tools
Biome
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
[warning] 109-109: src/device-registry/bin/new-store-readings-job.js#L109
Added line #L109 was not covered by tests
[warning] 116-116: src/device-registry/bin/new-store-readings-job.js#L116
Added line #L116 was not covered by tests
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
[warning] 130-130: src/device-registry/bin/new-store-readings-job.js#L130
Added line #L130 was not covered by tests
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
| delete updateDoc._id; | ||
| await ReadingModel("airqo").updateOne(filter, updateDoc, { |
There was a problem hiding this comment.
Avoid Using delete Operator for Better Performance
Using the delete operator on objects can negatively affect performance due to engine optimizations. At lines 125-126, you delete the _id property from updateDoc. Consider alternative approaches to exclude the _id field without using delete.
Option 1: Set _id to undefined:
- delete updateDoc._id;
+ updateDoc._id = undefined;Option 2: Use object destructuring to omit _id:
- const updateDoc = { ...doc };
- delete updateDoc._id;
+ const { _id, ...updateDoc } = doc;Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
| if (error.name === "MongoError" && error.code !== 11000) { | ||
| logger.error( | ||
| `🐛🐛 MongoError -- fetchAndStoreDataIntoReadingsModel -- ${jsonify( | ||
| error | ||
| )}` | ||
| ); | ||
| throw error; // Retry the operation | ||
| } else if (error.code === 11000) { | ||
| // Ignore duplicate key errors | ||
| console.warn( | ||
| `Duplicate key error for document: ${jsonify(doc)}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refine Error Handling in Batch Processing
In the error handling block starting at line 131, only MongoError exceptions with codes other than 11000 are retried. Other types of errors might also occur during database operations. Ensure that all relevant errors are properly caught and handled, and consider logging them for easier debugging.
Consider updating the error handling to cover additional exceptions:
} catch (error) {
logObject("Error during batch processing", error);
logger.error(`Error during batch processing: ${jsonify(error)}`);
- if (error.name === "MongoError" && error.code !== 11000) {
+ if ((error.name === "MongoError" && error.code !== 11000) || /* other conditions */) {
throw error; // Retry the operation
} else if (error.code === 11000) {
// Handle duplicate key errors
logger.warn(`Duplicate key error for document: ${jsonify(doc)}`);
} else {
// Handle other errors
throw error; // Retry the operation
}
}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 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: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/device-registry/bin/new-store-readings-job.js (1 hunks)
Additional context used
Biome
src/device-registry/bin/new-store-readings-job.js
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
src/device-registry/bin/new-store-readings-job.js
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
[warning] 21-21: src/device-registry/bin/new-store-readings-job.js#L21
Added line #L21 was not covered by tests
[warning] 24-25: src/device-registry/bin/new-store-readings-job.js#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 27-27: src/device-registry/bin/new-store-readings-job.js#L27
Added line #L27 was not covered by tests
[warning] 30-32: src/device-registry/bin/new-store-readings-job.js#L30-L32
Added lines #L30 - L32 were not covered by tests
[warning] 36-36: src/device-registry/bin/new-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/new-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-43: src/device-registry/bin/new-store-readings-job.js#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 48-49: src/device-registry/bin/new-store-readings-job.js#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 58-58: src/device-registry/bin/new-store-readings-job.js#L58
Added line #L58 was not covered by tests
[warning] 61-63: src/device-registry/bin/new-store-readings-job.js#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 65-66: src/device-registry/bin/new-store-readings-job.js#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 71-71: src/device-registry/bin/new-store-readings-job.js#L71
Added line #L71 was not covered by tests
[warning] 76-76: src/device-registry/bin/new-store-readings-job.js#L76
Added line #L76 was not covered by tests
[warning] 85-86: src/device-registry/bin/new-store-readings-job.js#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/device-registry/bin/new-store-readings-job.js#L88
Added line #L88 was not covered by tests
[warning] 90-92: src/device-registry/bin/new-store-readings-job.js#L90-L92
Added lines #L90 - L92 were not covered by tests
[warning] 95-98: src/device-registry/bin/new-store-readings-job.js#L95-L98
Added lines #L95 - L98 were not covered by tests
[warning] 102-107: src/device-registry/bin/new-store-readings-job.js#L102-L107
Added lines #L102 - L107 were not covered by tests
[warning] 109-109: src/device-registry/bin/new-store-readings-job.js#L109
Added line #L109 was not covered by tests
[warning] 116-116: src/device-registry/bin/new-store-readings-job.js#L116
Added line #L116 was not covered by tests
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
[warning] 130-130: src/device-registry/bin/new-store-readings-job.js#L130
Added line #L130 was not covered by tests
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
[warning] 155-156: src/device-registry/bin/new-store-readings-job.js#L155-L156
Added lines #L155 - L156 were not covered by tests
[warning] 158-158: src/device-registry/bin/new-store-readings-job.js#L158
Added line #L158 was not covered by tests
[warning] 163-163: src/device-registry/bin/new-store-readings-job.js#L163
Added line #L163 was not covered by tests
[warning] 168-169: src/device-registry/bin/new-store-readings-job.js#L168-L169
Added lines #L168 - L169 were not covered by tests
|
|
||
| function isEntityActive(entity) { | ||
| const inactiveThreshold = 30 * 60 * 1000; // 30 minutes in milliseconds | ||
| logObject("entity", entity); |
There was a problem hiding this comment.
Consider removing verbose logging of entities
Logging the entire entity object at line 18 may expose sensitive information and clutter the logs. Consider removing this log statement or adjusting the logging level.
Tools
GitHub Check: codecov/patch
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
| function isEntityActive(entity) { | ||
| const inactiveThreshold = 30 * 60 * 1000; // 30 minutes in milliseconds | ||
| logObject("entity", entity); | ||
|
|
||
| if (!entity || !entity.lastActive) { | ||
| return false; | ||
| } | ||
|
|
||
| const now = new Date(); | ||
| const lastActiveDate = new Date(entity.lastActive); | ||
|
|
||
| return now.getTime() - lastActiveDate.getTime() < inactiveThreshold; | ||
| } |
There was a problem hiding this comment.
Add unit tests for isEntityActive function
The isEntityActive function determines if an entity is active based on its lastActive timestamp. To ensure its correctness, especially with time calculations, consider adding unit tests.
Tools
GitHub Check: codecov/patch
[warning] 16-18: src/device-registry/bin/new-store-readings-job.js#L16-L18
Added lines #L16 - L18 were not covered by tests
[warning] 21-21: src/device-registry/bin/new-store-readings-job.js#L21
Added line #L21 was not covered by tests
[warning] 24-25: src/device-registry/bin/new-store-readings-job.js#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 27-27: src/device-registry/bin/new-store-readings-job.js#L27
Added line #L27 was not covered by tests
There was a problem hiding this comment.
create for me a Github issue for this please
There was a problem hiding this comment.
@Baalmart, glad to see the GitHub issue was created successfully: #3477. Let me know if there's anything else I can assist you with!
(>‿◠)✌
Analysis chain
@Baalmart, I noticed that the GitHub issue was created, but due to a syntax error in the script, the function name isEntityActive was omitted from the issue title. The backticks in the double-quoted strings caused the shell to interpret isEntityActive as a command.
The created issue is here: #3476
Here's a corrected script that uses single quotes to prevent command substitution:
Let me know if you'd like me to run this corrected script to create a new issue with the proper title.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add unit tests for `isEntityActive` function" \
--body "The \`isEntityActive\` function determines if an entity is active based on its \`lastActive\` timestamp. To ensure its correctness, especially with time calculations, we should add unit tests.
Related PR: https://github.com/airqo-platform/AirQo-api/pull/3473
Comment: https://github.com/airqo-platform/AirQo-api/pull/3473#discussion_r1765798562
Requested by @Baalmart." \
--assignee Baalmart
Length of output: 608
Script:
#!/bin/bash
gh issue create \
--title 'Add unit tests for `isEntityActive` function' \
--body 'The `isEntityActive` function determines if an entity is active based on its `lastActive` timestamp. To ensure its correctness, especially with time calculations, we should add unit tests.
Related PR: https://github.com/airqo-platform/AirQo-api/pull/3473
Comment: https://github.com/airqo-platform/AirQo-api/pull/3473#discussion_r1765798562
Requested by @Baalmart.' \
--assignee Baalmart
Length of output: 524
| async function updateEntityLastActive(Model, filter, time) { | ||
| try { | ||
| const entity = await Model.findOne(filter); | ||
| if (entity) { | ||
| if (isEntityActive(entity)) { | ||
| // Entity is still active, no need to update | ||
| return; | ||
| } | ||
|
|
||
| await Model.updateOne(filter, { lastActive: time }); | ||
| } | ||
| } catch (error) { | ||
| logObject("Error updating entity's lastActive", error); | ||
| logger.error(`Error updating entity's lastActive: ${jsonify(error)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add unit tests for updateEntityLastActive function
The updateEntityLastActive function updates an entity's lastActive field and includes error handling. Adding unit tests will help verify its behavior under various conditions, including error scenarios.
Tools
GitHub Check: codecov/patch
[warning] 30-32: src/device-registry/bin/new-store-readings-job.js#L30-L32
Added lines #L30 - L32 were not covered by tests
[warning] 36-36: src/device-registry/bin/new-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/new-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-43: src/device-registry/bin/new-store-readings-job.js#L42-L43
Added lines #L42 - L43 were not covered by tests
There was a problem hiding this comment.
create for me a Github issue for this please
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as requested: Add unit tests for updateEntityLastActive function. Please let me know if there's anything else I can assist you with.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add unit tests for `updateEntityLastActive` function" \
--body "The function \`updateEntityLastActive\` updates an entity's \`lastActive\` field and includes error handling. Adding unit tests will help verify its behavior under various conditions, including error scenarios.
Reference PR: https://github.com/airqo-platform/AirQo-api/pull/3473
Comment: https://github.com/airqo-platform/AirQo-api/pull/3473#discussion_r1765798563
Requester: @Baalmart" \
--assignee Baalmart
Length of output: 644
| } | ||
|
|
||
| if (viewEventsResponse.success === true) { | ||
| if ( | ||
| !viewEventsResponse.data || | ||
| !Array.isArray(viewEventsResponse.data) || | ||
| viewEventsResponse.data.length === 0 | ||
| ) { | ||
| logText("No data found in the response"); | ||
| return; | ||
| } | ||
| const data = viewEventsResponse.data[0].data; | ||
| if (!data || data.length === 0) { | ||
| logText("No Events found to insert into Readings"); | ||
| logger.error(`🐛🐛 Didn't find any Events to insert into Readings`); | ||
| return; | ||
| } | ||
|
|
||
| const batchSize = 50; | ||
| const batches = []; | ||
| for (let i = 0; i < data.length; i += batchSize) { | ||
| batches.push(data.slice(i, i + batchSize)); | ||
| } | ||
|
|
||
| // Insert each batch in the 'readings' collection with retry logic | ||
| for (const batch of batches) { | ||
| await Promise.all( | ||
| batch.map(async (doc) => { | ||
| await asyncRetry( | ||
| async (bail) => { | ||
| try { | ||
| // Update Site lastActive | ||
| await updateEntityLastActive( | ||
| SiteModel("airqo"), | ||
| { _id: doc.site_id }, | ||
| doc.time | ||
| ); | ||
|
|
||
| // Update Device lastActive | ||
| await updateEntityLastActive( | ||
| DeviceModel("airqo"), | ||
| { _id: doc.device_id }, | ||
| doc.time | ||
| ); | ||
|
|
||
| // Update Reading | ||
| const filter = { site_id: doc.site_id, time: doc.time }; | ||
| const updateDoc = { ...doc }; | ||
| delete updateDoc._id; | ||
| await ReadingModel("airqo").updateOne(filter, updateDoc, { | ||
| upsert: true, | ||
| }); | ||
| } catch (error) { | ||
| logObject("the error inside processing of batches", error); | ||
| if (error.name === "MongoError" && error.code !== 11000) { | ||
| logger.error( | ||
| `🐛🐛 MongoError -- fetchAndStoreDataIntoReadingsModel -- ${jsonify( | ||
| error | ||
| )}` | ||
| ); | ||
| throw error; // Retry the operation | ||
| } else if (error.code === 11000) { | ||
| // Ignore duplicate key errors | ||
| console.warn( | ||
| `Duplicate key error for document: ${jsonify(doc)}` | ||
| ); | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| retries: 3, // Number of retry attempts | ||
| minTimeout: 1000, // Initial delay between retries (in milliseconds) | ||
| factor: 2, // Exponential factor for increasing delay between retries | ||
| } | ||
| ); | ||
| }) | ||
| ); | ||
| } | ||
| logText(`All data inserted successfully`); | ||
| return; | ||
| } else { | ||
| logObject( | ||
| `🐛🐛 Unable to retrieve Events to insert into Readings`, | ||
| viewEventsResponse | ||
| ); | ||
|
|
||
| logger.error( | ||
| `🐛🐛 Unable to retrieve Events to insert into Readings -- ${jsonify( | ||
| viewEventsResponse | ||
| )}` | ||
| ); | ||
| logText(`🐛🐛 Unable to retrieve Events to insert into Readings`); | ||
| return; | ||
| } | ||
| } catch (error) { | ||
| logObject("error", error); | ||
| logger.error(`🐛🐛 Internal Server Error ${jsonify(error)}`); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add tests for fetchAndStoreDataIntoReadingsModel function
The fetchAndStoreDataIntoReadingsModel function is crucial for data ingestion but lacks test coverage. Adding integration tests will help ensure its reliability and catch potential issues early.
Tools
Biome
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: codecov/patch
[warning] 48-49: src/device-registry/bin/new-store-readings-job.js#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 58-58: src/device-registry/bin/new-store-readings-job.js#L58
Added line #L58 was not covered by tests
[warning] 61-63: src/device-registry/bin/new-store-readings-job.js#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 65-66: src/device-registry/bin/new-store-readings-job.js#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 71-71: src/device-registry/bin/new-store-readings-job.js#L71
Added line #L71 was not covered by tests
[warning] 76-76: src/device-registry/bin/new-store-readings-job.js#L76
Added line #L76 was not covered by tests
[warning] 85-86: src/device-registry/bin/new-store-readings-job.js#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 88-88: src/device-registry/bin/new-store-readings-job.js#L88
Added line #L88 was not covered by tests
[warning] 90-92: src/device-registry/bin/new-store-readings-job.js#L90-L92
Added lines #L90 - L92 were not covered by tests
[warning] 95-98: src/device-registry/bin/new-store-readings-job.js#L95-L98
Added lines #L95 - L98 were not covered by tests
[warning] 102-107: src/device-registry/bin/new-store-readings-job.js#L102-L107
Added lines #L102 - L107 were not covered by tests
[warning] 109-109: src/device-registry/bin/new-store-readings-job.js#L109
Added line #L109 was not covered by tests
[warning] 116-116: src/device-registry/bin/new-store-readings-job.js#L116
Added line #L116 was not covered by tests
[warning] 123-126: src/device-registry/bin/new-store-readings-job.js#L123-L126
Added lines #L123 - L126 were not covered by tests
[warning] 130-130: src/device-registry/bin/new-store-readings-job.js#L130
Added line #L130 was not covered by tests
[warning] 132-132: src/device-registry/bin/new-store-readings-job.js#L132
Added line #L132 was not covered by tests
[warning] 137-137: src/device-registry/bin/new-store-readings-job.js#L137
Added line #L137 was not covered by tests
[warning] 140-140: src/device-registry/bin/new-store-readings-job.js#L140
Added line #L140 was not covered by tests
[warning] 155-156: src/device-registry/bin/new-store-readings-job.js#L155-L156
Added lines #L155 - L156 were not covered by tests
[warning] 158-158: src/device-registry/bin/new-store-readings-job.js#L158
Added line #L158 was not covered by tests
[warning] 163-163: src/device-registry/bin/new-store-readings-job.js#L163
Added line #L163 was not covered by tests
[warning] 168-169: src/device-registry/bin/new-store-readings-job.js#L168-L169
Added lines #L168 - L169 were not covered by tests
There was a problem hiding this comment.
create for me a Github issue for this please
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as requested: #3475
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add tests for fetchAndStoreDataIntoReadingsModel function" \
--body "The function \`fetchAndStoreDataIntoReadingsModel\` is crucial for data ingestion but lacks test coverage. Adding integration tests will help ensure its reliability and catch potential issues early.\n\nPR: https://github.com/airqo-platform/AirQo-api/pull/3473\nComment: https://github.com/airqo-platform/AirQo-api/pull/3473#discussion_r1765798564\n\nRequested by @Baalmart" \
--assignee Baalmart
Length of output: 541
|
Device registry changes in this PR available for preview here |
Description
reconfiguring the store readings operation
Changes Made
Testing
Affected Services
API Documentation Updated?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor