enhancing the emails for field activities (deploy and recall) with more specific content #3554
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant updates across several files related to Kafka topics and email notification functionalities. Two new Kafka topics, 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 #3554 +/- ##
===========================================
- Coverage 29.89% 29.69% -0.21%
===========================================
Files 184 185 +1
Lines 24693 24994 +301
Branches 3257 3310 +53
===========================================
+ Hits 7382 7421 +39
- Misses 17189 17451 +262
Partials 122 122
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Baalmart
changed the title
|
Auth-service 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: 12
🧹 Outside diff range and nitpick comments (4)
src/auth-service/config/global/envs.js (1)
Line range hint
1-70: Overall assessment of environment variable additionsThe changes to this file are minimal yet impactful. The addition of
DEPLOY_TOPICandRECALL_TOPICenvironment variables is in line with the PR objectives to introduce new Kafka topics for deployment and recallment processes.A few recommendations:
- Ensure that these new environment variables are documented in your project's README or environment setup guide.
- Consider grouping related environment variables together. For instance, you might want to place these new Kafka-related variables near other Kafka configurations if they exist.
- It might be beneficial to add comments explaining the purpose of these new topics, especially if their usage is not immediately obvious to other developers.
As the number of environment variables grows, consider implementing a more structured configuration management system. This could involve categorizing variables or using a configuration library to manage different environments more effectively.
src/auth-service/bin/jobs/kafka-consumer.js (3)
231-257: Ensure all asynchronous operations are properly handledIn the try-catch block, if an error occurs during the asynchronous operations, it is caught and logged. However, consider adding more context to the error messages to aid in debugging.
Enhance the error logging by including stack traces:
} catch (error) { - logger.error(`🐛 Internal Server Error -- ${error.message}`); + logger.error(`🐛 Internal Server Error in emailsForDeployedDevices -- ${error.stack}`); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 231-232: src/auth-service/bin/jobs/kafka-consumer.js#L231-L232
Added lines #L231 - L232 were not covered by tests
[warning] 238-239: src/auth-service/bin/jobs/kafka-consumer.js#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 242-242: src/auth-service/bin/jobs/kafka-consumer.js#L242
Added line #L242 was not covered by tests
[warning] 253-253: src/auth-service/bin/jobs/kafka-consumer.js#L253
Added line #L253 was not covered by tests
[warning] 256-256: src/auth-service/bin/jobs/kafka-consumer.js#L256
Added line #L256 was not covered by tests
277-303: Enhance error logging for better debuggingWhen catching errors, consider including more detailed information in the logs to facilitate troubleshooting.
Include the error stack in the log message:
} catch (error) { - logger.error(`🐛 Internal Server Error -- ${error.message}`); + logger.error(`🐛 Internal Server Error in emailsForRecalledDevices -- ${error.stack}`); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 277-278: src/auth-service/bin/jobs/kafka-consumer.js#L277-L278
Added lines #L277 - L278 were not covered by tests
[warning] 284-285: src/auth-service/bin/jobs/kafka-consumer.js#L284-L285
Added lines #L284 - L285 were not covered by tests
[warning] 288-288: src/auth-service/bin/jobs/kafka-consumer.js#L288
Added line #L288 was not covered by tests
[warning] 299-299: src/auth-service/bin/jobs/kafka-consumer.js#L299
Added line #L299 was not covered by tests
[warning] 302-302: src/auth-service/bin/jobs/kafka-consumer.js#L302
Added line #L302 was not covered by tests
14-14: Import order and grouping for readabilityFor better code organization, consider grouping all external package imports together and separating them from local imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- k8s/kafka/topics/kafka-topics.yaml (3 hunks)
- src/auth-service/.env.example (1 hunks)
- src/auth-service/bin/jobs/kafka-consumer.js (3 hunks)
- src/auth-service/config/global/envs.js (1 hunks)
- src/auth-service/utils/email.msgs.js (1 hunks)
- src/auth-service/utils/mailer.js (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/auth-service/.env.example
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/auth-service/bin/jobs/kafka-consumer.js
[warning] 40-40: src/auth-service/bin/jobs/kafka-consumer.js#L40
Added line #L40 was not covered by tests
[warning] 42-42: src/auth-service/bin/jobs/kafka-consumer.js#L42
Added line #L42 was not covered by tests
[warning] 59-59: src/auth-service/bin/jobs/kafka-consumer.js#L59
Added line #L59 was not covered by tests
[warning] 223-223: src/auth-service/bin/jobs/kafka-consumer.js#L223
Added line #L223 was not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
[warning] 231-232: src/auth-service/bin/jobs/kafka-consumer.js#L231-L232
Added lines #L231 - L232 were not covered by tests
[warning] 238-239: src/auth-service/bin/jobs/kafka-consumer.js#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 242-242: src/auth-service/bin/jobs/kafka-consumer.js#L242
Added line #L242 was not covered by tests
[warning] 253-253: src/auth-service/bin/jobs/kafka-consumer.js#L253
Added line #L253 was not covered by tests
[warning] 256-256: src/auth-service/bin/jobs/kafka-consumer.js#L256
Added line #L256 was not covered by tests
[warning] 263-264: src/auth-service/bin/jobs/kafka-consumer.js#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 266-267: src/auth-service/bin/jobs/kafka-consumer.js#L266-L267
Added lines #L266 - L267 were not covered by tests
[warning] 270-270: src/auth-service/bin/jobs/kafka-consumer.js#L270
Added line #L270 was not covered by tests
[warning] 273-274: src/auth-service/bin/jobs/kafka-consumer.js#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 277-278: src/auth-service/bin/jobs/kafka-consumer.js#L277-L278
Added lines #L277 - L278 were not covered by tests
[warning] 284-285: src/auth-service/bin/jobs/kafka-consumer.js#L284-L285
Added lines #L284 - L285 were not covered by tests
[warning] 288-288: src/auth-service/bin/jobs/kafka-consumer.js#L288
Added line #L288 was not covered by tests
[warning] 299-299: src/auth-service/bin/jobs/kafka-consumer.js#L299
Added line #L299 was not covered by tests
[warning] 302-302: src/auth-service/bin/jobs/kafka-consumer.js#L302
Added line #L302 was not covered by testssrc/auth-service/utils/email.msgs.js
[warning] 326-326: src/auth-service/utils/email.msgs.js#L326
Added line #L326 was not covered by tests
[warning] 335-336: src/auth-service/utils/email.msgs.js#L335-L336
Added lines #L335 - L336 were not covered by tests
[warning] 340-341: src/auth-service/utils/email.msgs.js#L340-L341
Added lines #L340 - L341 were not covered by tests
[warning] 349-349: src/auth-service/utils/email.msgs.js#L349
Added line #L349 was not covered by tests
[warning] 368-369: src/auth-service/utils/email.msgs.js#L368-L369
Added lines #L368 - L369 were not covered by testssrc/auth-service/utils/mailer.js
[warning] 56-56: src/auth-service/utils/mailer.js#L56
Added line #L56 was not covered by tests
[warning] 58-59: src/auth-service/utils/mailer.js#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 65-65: src/auth-service/utils/mailer.js#L65
Added line #L65 was not covered by tests
[warning] 82-82: src/auth-service/utils/mailer.js#L82
Added line #L82 was not covered by tests
[warning] 102-102: src/auth-service/utils/mailer.js#L102
Added line #L102 was not covered by tests
[warning] 104-104: src/auth-service/utils/mailer.js#L104
Added line #L104 was not covered by tests
🔇 Additional comments (7)
k8s/kafka/topics/kafka-topics.yaml (3)
Line range hint
1-264: Existing topics remain consistent and well-configured.I've reviewed the existing Kafka topics, and I'm pleased to report that they maintain their previous configurations. The minor formatting change (additional whitespace) in the
new-hourly-measurements-topicdoesn't affect its functionality. This consistency is commendable and aligns with best practices in configuration management.
Line range hint
1-292: Summary: Kafka topics updated to support new operational processes.This update to the Kafka topics configuration is well-structured and aligns perfectly with the PR objectives. Here's a summary of the changes:
- Existing topics have been maintained, ensuring stability in the current system.
- Two new topics,
recall-topicanddeploy-topic, have been added to support the new deployment and recallment processes.- The new topics follow the configuration pattern of existing operational topics, maintaining consistency across the file.
These changes should effectively support the modification of email content based on field activity, as outlined in the PR objectives. The consistent approach to topic configuration is commendable and will aid in future maintenance and scalability.
Great job on implementing these changes while maintaining the integrity of the existing configuration!
266-292: New topics added for recall and deployment processes.The addition of
recall-topicanddeploy-topicaligns perfectly with the PR objectives. Their configurations (2 partitions, 2 replicas) are consistent with other operational topics in the file, which is excellent for maintaining uniformity.One point to consider:
- The retention period is set to 5 hours (18,000,000 ms). While this seems reasonable for operational topics, it would be prudent to verify if this duration adequately covers the expected lifecycle of recall and deployment processes.
Could you please confirm that the 5-hour retention period is sufficient for these operational topics? If you need to adjust this, here's a script to help you find similar configurations in the file:
✅ Verification successful
Retention period for new Kafka topics is consistent with existing configurations.
The
retention.msvalue of 18,000,000 ms (5 hours) for bothrecall-topicanddeploy-topicaligns with the retention settings of other Kafka topics in the repository. This consistency ensures uniform behavior across your messaging infrastructure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find topics with 18000000 ms retention time rg --type yaml 'retention.ms: 18000000' k8s/kafka/topics/kafka-topics.yamlLength of output: 184
src/auth-service/bin/jobs/kafka-consumer.js (2)
260-268: Good error handling inemailsForRecalledDevicesExcellent job implementing a try-catch block around
JSON.parse(messageData)to handle invalid JSON formats. This ensures the application remains robust against malformed inputs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 263-264: src/auth-service/bin/jobs/kafka-consumer.js#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 266-267: src/auth-service/bin/jobs/kafka-consumer.js#L266-L267
Added lines #L266 - L267 were not covered by tests
323-324: Verify topic subscriptions in Kafka consumerThe topics
"deploy-topic"and"recall-topic"have been added to the consumer. Ensure that these topics are correctly configured and exist in your Kafka setup.Please confirm that these topics are available and that the necessary permissions are in place for the consumer group.
If you'd like, you can run the following script to list available topics:
Replace
your_kafka_broker:9092with your Kafka broker's address.src/auth-service/utils/mailer.js (2)
68-99: Validate Input Parameters increateMailOptionsThe
createMailOptionsfunction destructures its input parameters from an object, defaulting to an empty object. If required fields likeactivityTypeare missing, it could lead to runtime errors or unintended behaviors. To ensure reliability, consider adding input validation or default values for essential parameters.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: src/auth-service/utils/mailer.js#L82
Added line #L82 was not covered by tests
1958-1959: Ensure Proper Utilization of New Parameters insiteActivityThe
siteActivitymethod now includesactivityDetailsanddeviceDetailsas additional parameters. Please verify that these parameters are correctly utilized within themsgs.site_activityfunction and that they align with the expected input of that function to avoid any discrepancies in the email content.
| DEPLOY_TOPIC: process.env.DEPLOY_TOPIC, | ||
| RECALL_TOPIC: process.env.RECALL_TOPIC, |
There was a problem hiding this comment.
💡 Codebase verification
DEPLOY_TOPIC and RECALL_TOPIC are declared but not used elsewhere.
It appears that the newly added environment variables DEPLOY_TOPIC and RECALL_TOPIC are declared in envs.js but are not referenced in the codebase. Please verify whether these variables are intended for future use or perform an oversight. If they are necessary, ensure they are properly integrated into the relevant parts of the system. Otherwise, consider removing them to maintain code cleanliness.
🔗 Analysis chain
New Kafka topics added for deployment and recallment processes.
The addition of DEPLOY_TOPIC and RECALL_TOPIC environment variables aligns well with the PR objectives. These new Kafka topics will facilitate the deployment and recallment processes, potentially enhancing the system's ability to manage field activities.
A few points to consider:
- The naming convention is consistent with existing variables, which is excellent.
- Ensure that these environment variables are properly set in all relevant environments (development, staging, production).
- Update any deployment scripts or documentation to include these new variables.
To ensure these new topics are being utilized correctly, let's verify their usage in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new Kafka topics in the codebase
# Search for DEPLOY_TOPIC usage
echo "Searching for DEPLOY_TOPIC usage:"
rg --type js "DEPLOY_TOPIC" -C 3
echo "\nSearching for RECALL_TOPIC usage:"
rg --type js "RECALL_TOPIC" -C 3
Length of output: 1535
| const extractDeviceDetails = (updatedDevice) => { | ||
| const { | ||
| _id, | ||
| status, | ||
| category, | ||
| isActive, | ||
| long_name, | ||
| device_number, | ||
| name, | ||
| deployment_date, | ||
| latitude, | ||
| longitude, | ||
| mountType, | ||
| powerType, | ||
| } = updatedDevice; | ||
|
|
||
| return { | ||
| status, | ||
| category, | ||
| _id, | ||
| isActive, | ||
| long_name, | ||
| device_number, | ||
| name, | ||
| deployment_date, | ||
| latitude, | ||
| longitude, | ||
| mountType, | ||
| powerType, | ||
| }; | ||
| }; | ||
|
|
||
| const extractActivityDetails = (createdActivity) => { | ||
| return { | ||
| ...createdActivity, // Spread operator to include all properties directly | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Unit tests are missing for new utility functions
The utility functions extractDeviceDetails and extractActivityDetails are new additions but are not covered by unit tests.
Writing unit tests for these functions will improve code reliability and prevent future regressions. Would you like assistance in creating these tests or opening a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 40-40: src/auth-service/bin/jobs/kafka-consumer.js#L40
Added line #L40 was not covered by tests
[warning] 42-42: src/auth-service/bin/jobs/kafka-consumer.js#L42
Added line #L42 was not covered by tests
[warning] 59-59: src/auth-service/bin/jobs/kafka-consumer.js#L59
Added line #L59 was not covered by tests
There was a problem hiding this comment.
create a Github issue for this, please.
There was a problem hiding this comment.
@Baalmart, I've created a GitHub issue to track adding unit tests for extractDeviceDetails and extractActivityDetails. You can find it here: #3556
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track adding unit tests for the utility functions.
gh issue create \
--title "Add unit tests for `extractDeviceDetails` and `extractActivityDetails`" \
--body "The utility functions \`extractDeviceDetails\` and \`extractActivityDetails\` added in PR [#3554](https://github.com/airqo-platform/AirQo-api/pull/3554) lack unit tests. Writing unit tests for these functions will improve code reliability and prevent future regressions.
See the discussion at [this comment](https://github.com/airqo-platform/AirQo-api/pull/3554#discussion_r1787818825).
Requested by @Baalmart." \
--assignee Baalmart
Length of output: 774
| const { createdActivity, updatedDevice, user_id } = JSON.parse(messageData); | ||
|
|
||
| // Validate input data | ||
| if (!createdActivity || !updatedDevice || !user_id) { | ||
| logger.error("Invalid input data: Missing required fields."); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const user = await UserModel("airqo") | ||
| .findOne({ _id: ObjectId(user_id) }, "email _id firstName lastName") | ||
| .lean(); | ||
|
|
||
| // Check if user exists | ||
| if (!user) { | ||
| logger.error(`User not found for user_id: ${user_id}`); | ||
| return; | ||
| } | ||
|
|
||
| const emailResponse = await mailer.fieldActivity({ | ||
| email: user.email, | ||
| firstName: user.firstName, | ||
| lastName: user.lastName, | ||
| deviceDetails: extractDeviceDetails(updatedDevice), | ||
| activityDetails: extractActivityDetails(createdActivity), | ||
| activityType: "deploy", | ||
| }); | ||
|
|
||
| // Handle email response | ||
| if (emailResponse && emailResponse.success === false) { | ||
| logger.error(`🐛 Internal Server Error -- ${stringify(emailResponse)}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛 Internal Server Error -- ${error.message}`); | ||
| } | ||
| }; | ||
|
|
||
| const emailsForRecalledDevices = async (messageData) => { | ||
| // Parse the message and validate input data | ||
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(messageData); | ||
| } catch (error) { | ||
| logger.error("Invalid JSON format in messageData."); | ||
| return; | ||
| } | ||
|
|
||
| const { createdActivity, updatedDevice, user_id } = parsedData; | ||
|
|
||
| if (!createdActivity || !updatedDevice || !user_id) { | ||
| logger.error("Invalid input data: Missing required fields."); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const user = await UserModel("airqo") | ||
| .findOne({ _id: ObjectId(user_id) }, "email _id firstName lastName") | ||
| .lean(); | ||
|
|
||
| // Check if user exists | ||
| if (!user) { | ||
| logger.error(`User not found for user_id: ${user_id}`); | ||
| return; | ||
| } | ||
|
|
||
| const emailResponse = await mailer.fieldActivity({ | ||
| email: user.email, | ||
| firstName: user.firstName, | ||
| lastName: user.lastName, | ||
| deviceDetails: extractDeviceDetails(updatedDevice), | ||
| activityDetails: extractActivityDetails(createdActivity), | ||
| activityType: "recall", | ||
| }); | ||
|
|
||
| // Handle email response | ||
| if (emailResponse && emailResponse.success === false) { | ||
| logger.error(`🐛 Internal Server Error -- ${stringify(emailResponse)}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛 Internal Server Error -- ${error.message}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Increase test coverage for new email handler functions
The functions emailsForDeployedDevices and emailsForRecalledDevices are significant additions that are not currently covered by tests, as indicated by the static analysis tool.
Adding unit tests for these functions will ensure they behave as expected and handle edge cases appropriately. Can I assist in writing these tests or creating a GitHub issue to address this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: src/auth-service/bin/jobs/kafka-consumer.js#L223
Added line #L223 was not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
[warning] 231-232: src/auth-service/bin/jobs/kafka-consumer.js#L231-L232
Added lines #L231 - L232 were not covered by tests
[warning] 238-239: src/auth-service/bin/jobs/kafka-consumer.js#L238-L239
Added lines #L238 - L239 were not covered by tests
[warning] 242-242: src/auth-service/bin/jobs/kafka-consumer.js#L242
Added line #L242 was not covered by tests
[warning] 253-253: src/auth-service/bin/jobs/kafka-consumer.js#L253
Added line #L253 was not covered by tests
[warning] 256-256: src/auth-service/bin/jobs/kafka-consumer.js#L256
Added line #L256 was not covered by tests
[warning] 263-264: src/auth-service/bin/jobs/kafka-consumer.js#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 266-267: src/auth-service/bin/jobs/kafka-consumer.js#L266-L267
Added lines #L266 - L267 were not covered by tests
[warning] 270-270: src/auth-service/bin/jobs/kafka-consumer.js#L270
Added line #L270 was not covered by tests
[warning] 273-274: src/auth-service/bin/jobs/kafka-consumer.js#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 277-278: src/auth-service/bin/jobs/kafka-consumer.js#L277-L278
Added lines #L277 - L278 were not covered by tests
[warning] 284-285: src/auth-service/bin/jobs/kafka-consumer.js#L284-L285
Added lines #L284 - L285 were not covered by tests
[warning] 288-288: src/auth-service/bin/jobs/kafka-consumer.js#L288
Added line #L288 was not covered by tests
[warning] 299-299: src/auth-service/bin/jobs/kafka-consumer.js#L299
Added line #L299 was not covered by tests
[warning] 302-302: src/auth-service/bin/jobs/kafka-consumer.js#L302
Added line #L302 was not covered by tests
There was a problem hiding this comment.
Create a Github issue for this please
There was a problem hiding this comment.
@Baalmart, I've created the GitHub issue as requested. You can track it here: #3555.
🔗 Analysis chain
@Baalmart, I've prepared the GitHub issue as per your request.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to address the lack of tests for new email handler functions.
gh issue create \
--title "Add unit tests for new email handler functions in kafka-consumer.js" \
--body "The functions \`emailsForDeployedDevices\` and \`emailsForRecalledDevices\` in \`src/auth-service/bin/jobs/kafka-consumer.js\` are not currently covered by tests. Adding unit tests will ensure they behave as expected and handle edge cases appropriately.\n\nPR URL: https://github.com/airqo-platform/AirQo-api/pull/3554\nComment URL: https://github.com/airqo-platform/AirQo-api/pull/3554#discussion_r1787818828\n\nRequested by @Baalmart." \
--assignee Baalmart
Length of output: 611
| const { createdActivity, updatedDevice, user_id } = parsedData; | ||
|
|
||
| if (!createdActivity || !updatedDevice || !user_id) { | ||
| logger.error("Invalid input data: Missing required fields."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate user_id before converting to ObjectId
Similar to emailsForDeployedDevices, ensure that user_id is a valid ObjectId to prevent potential errors.
Add a validation step:
if (!createdActivity || !updatedDevice || !user_id) {
logger.error("Invalid input data: Missing required fields.");
return;
}
+ if (!ObjectId.isValid(user_id)) {
+ logger.error(`Invalid user_id format: ${user_id}`);
+ 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.
| const { createdActivity, updatedDevice, user_id } = parsedData; | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } | |
| const { createdActivity, updatedDevice, user_id } = parsedData; | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } | |
| if (!ObjectId.isValid(user_id)) { | |
| logger.error(`Invalid user_id format: ${user_id}`); | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 270-270: src/auth-service/bin/jobs/kafka-consumer.js#L270
Added line #L270 was not covered by tests
[warning] 273-274: src/auth-service/bin/jobs/kafka-consumer.js#L273-L274
Added lines #L273 - L274 were not covered by tests
There was a problem hiding this comment.
All cool, this is taken care of by the producing functionality, so the consumer does not have to worry about this input validation
There was a problem hiding this comment.
@Baalmart, understood. Thank you for the clarification that input validation for user_id is handled by the producing functionality, so the consumer does not need to perform this validation.
(_/)
(•ᗜ•)?
/ づ
✏️ Learnings added
Learnt from: Baalmart
PR: airqo-platform/AirQo-api#3554
File: src/auth-service/bin/jobs/kafka-consumer.js:270-275
Timestamp: 2024-10-04T14:41:56.941Z
Learning: In `src/auth-service/bin/jobs/kafka-consumer.js`, the `emailsForRecalledDevices` function relies on the producing functionality to validate `user_id`, so additional validation in the consumer is not necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const { createdActivity, updatedDevice, user_id } = JSON.parse(messageData); | ||
|
|
||
| // Validate input data | ||
| if (!createdActivity || !updatedDevice || !user_id) { | ||
| logger.error("Invalid input data: Missing required fields."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate user_id before converting to ObjectId
When fetching the user, user_id is directly converted using ObjectId(user_id). If user_id is not a valid MongoDB ObjectId, this will throw an error.
Add a validation step to ensure user_id is a valid ObjectId:
// Validate input data
if (!createdActivity || !updatedDevice || !user_id) {
logger.error("Invalid input data: Missing required fields.");
return;
}
+ if (!ObjectId.isValid(user_id)) {
+ logger.error(`Invalid user_id format: ${user_id}`);
+ 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.
| const { createdActivity, updatedDevice, user_id } = JSON.parse(messageData); | |
| // Validate input data | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } | |
| const { createdActivity, updatedDevice, user_id } = JSON.parse(messageData); | |
| // Validate input data | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } | |
| if (!ObjectId.isValid(user_id)) { | |
| logger.error(`Invalid user_id format: ${user_id}`); | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: src/auth-service/bin/jobs/kafka-consumer.js#L223
Added line #L223 was not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
Handle JSON parsing errors in emailsForDeployedDevices
Currently, the function emailsForDeployedDevices does not handle exceptions that might occur during JSON.parse(messageData). If messageData is not valid JSON, this could cause the application to crash or behave unpredictably.
Consider wrapping the parsing logic in a try-catch block to gracefully handle invalid JSON inputs:
+ const emailsForDeployedDevices = async (messageData) => {
+ let parsedData;
+ try {
+ parsedData = JSON.parse(messageData);
+ } catch (error) {
+ logger.error("Invalid JSON format in messageData.");
+ return;
+ }
+ const { createdActivity, updatedDevice, user_id } = parsedData;
- const emailsForDeployedDevices = async (messageData) => {
- const { createdActivity, updatedDevice, user_id } = JSON.parse(messageData);📝 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 { createdActivity, updatedDevice, user_id } = JSON.parse(messageData); | |
| // Validate input data | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } | |
| const emailsForDeployedDevices = async (messageData) => { | |
| let parsedData; | |
| try { | |
| parsedData = JSON.parse(messageData); | |
| } catch (error) { | |
| logger.error("Invalid JSON format in messageData."); | |
| return; | |
| } | |
| const { createdActivity, updatedDevice, user_id } = parsedData; | |
| // Validate input data | |
| if (!createdActivity || !updatedDevice || !user_id) { | |
| logger.error("Invalid input data: Missing required fields."); | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: src/auth-service/bin/jobs/kafka-consumer.js#L223
Added line #L223 was not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
| field_activity: ({ | ||
| firstName = "", | ||
| lastName = "", | ||
| activityDetails = {}, | ||
| deviceDetails = {}, | ||
| email = "", | ||
| activityType = "recall", // New parameter to determine activity type | ||
| }) => { | ||
| // Create a list of activity details | ||
| let activityDetailsList = Object.entries(activityDetails) | ||
| .map(([key, value]) => `<li>${key}: "${value}"</li>`) | ||
| .join("\n"); | ||
|
|
||
| // Create a list of device details | ||
| let deviceDetailsList = Object.entries(deviceDetails) | ||
| .map(([key, value]) => `<li>${key}: "${value}"</li>`) | ||
| .join("\n"); | ||
|
|
||
| const actionMessage = | ||
| activityType === "recall" | ||
| ? "A device has been recalled in your AirQo system." | ||
| : "A device has been deployed in your AirQo system."; | ||
|
|
||
| const content = ` | ||
| <tr> | ||
| <td style="color: #344054; font-size: 16px; font-family: Inter; font-weight: 400; line-height: 24px; word-wrap: break-word;"> | ||
| ${actionMessage} | ||
| <br /> | ||
| <strong>Here are the details:</strong> | ||
| <br /> | ||
| <strong>Activity Details:</strong> | ||
| <ol>${activityDetailsList}</ol> | ||
| <strong>Device Details:</strong> | ||
| <ol>${deviceDetailsList}</ol> | ||
| <br /> | ||
| If you have any questions or concerns regarding this action, please contact your organization's administrator. | ||
| <br /> | ||
| Access AirQo Analytics here: ${constants.LOGIN_PAGE} | ||
| <br /><br /> | ||
| </td> | ||
| </tr>`; | ||
|
|
||
| const name = `${firstName} ${lastName}`; | ||
| return constants.EMAIL_BODY({ email, content, name }); | ||
| }, |
There was a problem hiding this comment.
Add unit tests for the field_activity method
The new field_activity method is currently not covered by unit tests, as indicated by the code coverage report. Adding unit tests will help ensure the method works as expected and will detect any future regressions.
Would you like assistance in creating unit tests for this method, or should we open a new GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 326-326: src/auth-service/utils/email.msgs.js#L326
Added line #L326 was not covered by tests
[warning] 335-336: src/auth-service/utils/email.msgs.js#L335-L336
Added lines #L335 - L336 were not covered by tests
[warning] 340-341: src/auth-service/utils/email.msgs.js#L340-L341
Added lines #L340 - L341 were not covered by tests
[warning] 349-349: src/auth-service/utils/email.msgs.js#L349
Added line #L349 was not covered by tests
[warning] 368-369: src/auth-service/utils/email.msgs.js#L368-L369
Added lines #L368 - L369 were not covered by tests
There was a problem hiding this comment.
please generate a new Github issue based on this suggestion
There was a problem hiding this comment.
@Baalmart, I've created a new GitHub issue to track adding unit tests for the field_activity method in email.msgs.js. You can find it here: Issue #3557. Let me know if there's anything else I can assist with!
🔗 Analysis chain
@Baalmart, I've generated a shell script to open a new GitHub issue to track adding unit tests for the field_activity method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Add unit tests for the \`field_activity\` method in \`email.msgs.js\`" \
--body "The new \`field_activity\` method added in [PR #3554](https://github.com/airqo-platform/AirQo-api/pull/3554) lacks unit tests. Adding unit tests will help ensure the method works as expected and detect any future regressions.\n\nComment: https://github.com/airqo-platform/AirQo-api/pull/3554#discussion_r1787818843\n\nRequested by @Baalmart." \
--assignee Baalmart
Length of output: 518
src/auth-service/utils/mailer.js
Outdated
| for (const bccEmail of bccEmails.map((email) => email.trim())) { | ||
| const checkResult = await SubscriptionModel( | ||
| "airqo" | ||
| ).checkNotificationStatus({ email: bccEmail, type: "email" }); | ||
| if (checkResult.success) subscribedEmails.push(bccEmail); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Asynchronous Operations with Concurrent Processing
The subscription status checks within getSubscribedBccEmails are executed sequentially, which may lead to increased latency, especially when handling multiple emails. To improve performance, consider executing these checks concurrently using Promise.all.
Proposed enhancement:
- for (const bccEmail of bccEmails.map((email) => email.trim())) {
- const checkResult = await SubscriptionModel("airqo").checkNotificationStatus({ email: bccEmail, type: "email" });
- if (checkResult.success) subscribedEmails.push(bccEmail);
- }
+ const checkPromises = bccEmails.map(async (bccEmail) => {
+ try {
+ const checkResult = await SubscriptionModel("airqo").checkNotificationStatus({ email: bccEmail.trim(), type: "email" });
+ return checkResult.success ? bccEmail.trim() : null;
+ } catch (error) {
+ logger.error(`Error checking notification status for ${bccEmail}: ${error.message}`);
+ return null;
+ }
+ });
+ const successfulEmails = (await Promise.all(checkPromises)).filter(email => email !== null);
+ subscribedEmails = successfulEmails;📝 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.
| for (const bccEmail of bccEmails.map((email) => email.trim())) { | |
| const checkResult = await SubscriptionModel( | |
| "airqo" | |
| ).checkNotificationStatus({ email: bccEmail, type: "email" }); | |
| if (checkResult.success) subscribedEmails.push(bccEmail); | |
| } | |
| const checkPromises = bccEmails.map(async (bccEmail) => { | |
| try { | |
| const checkResult = await SubscriptionModel("airqo").checkNotificationStatus({ email: bccEmail.trim(), type: "email" }); | |
| return checkResult.success ? bccEmail.trim() : null; | |
| } catch (error) { | |
| logger.error(`Error checking notification status for ${bccEmail}: ${error.message}`); | |
| return null; | |
| } | |
| }); | |
| const successfulEmails = (await Promise.all(checkPromises)).filter(email => email !== null); | |
| subscribedEmails = successfulEmails; |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-59: src/auth-service/utils/mailer.js#L58-L59
Added lines #L58 - L59 were not covered by tests
| const handleMailResponse = (data) => { | ||
| if (isEmpty(data.rejected) && !isEmpty(data.accepted)) { | ||
| return { success: true, message: "Email successfully sent", data }; | ||
| } else { | ||
| throw new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: "Email not sent", emailResults: data } | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align Error Handling in handleMailResponse with Application Standards
In handleMailResponse, errors are thrown directly using throw new HttpError, whereas other parts of the mailer object use the next callback to handle errors. To maintain consistency and facilitate centralized error handling, consider modifying the function to accept next as a parameter and use it to pass errors forward.
Suggested modification:
- const handleMailResponse = (data) => {
+ const handleMailResponse = (data, next) => {
if (isEmpty(data.rejected) && !isEmpty(data.accepted)) {
return { success: true, message: "Email successfully sent", data };
} else {
- throw new HttpError(
+ next(new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: "Email not sent", emailResults: data }
- );
+ ));
+ 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.
| const handleMailResponse = (data) => { | |
| if (isEmpty(data.rejected) && !isEmpty(data.accepted)) { | |
| return { success: true, message: "Email successfully sent", data }; | |
| } else { | |
| throw new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: "Email not sent", emailResults: data } | |
| ); | |
| } | |
| }; | |
| const handleMailResponse = (data, next) => { | |
| if (isEmpty(data.rejected) && !isEmpty(data.accepted)) { | |
| return { success: true, message: "Email successfully sent", data }; | |
| } else { | |
| next(new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: "Email not sent", emailResults: data } | |
| )); | |
| return; | |
| } | |
| }; |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 102-102: src/auth-service/utils/mailer.js#L102
Added line #L102 was not covered by tests
[warning] 104-104: src/auth-service/utils/mailer.js#L104
Added line #L104 was not covered by tests
| fieldActivity: async ({ | ||
| email = "", | ||
| firstName = "", | ||
| lastName = "", | ||
| activityDetails = {}, | ||
| deviceDetails = {}, | ||
| activityType = "recall", // New parameter to determine activity type | ||
| }) => { | ||
| try { | ||
| const checkResult = await SubscriptionModel( | ||
| "airqo" | ||
| ).checkNotificationStatus({ email, type: "email" }); | ||
| if (!checkResult.success) return checkResult; | ||
|
|
||
| const bccEmails = await getSubscribedBccEmails(); | ||
| const mailOptions = createMailOptions({ | ||
| email, | ||
| firstName, | ||
| lastName, | ||
| activityDetails, | ||
| deviceDetails, | ||
| bccEmails, | ||
| activityType, | ||
| }); | ||
|
|
||
| let response = await transporter.sendMail(mailOptions); | ||
| return handleMailResponse(response); | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| throw new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Maintain Consistent Error Handling in fieldActivity Method
The fieldActivity method throws errors using throw new HttpError, differing from other methods in the mailer object that use the next callback for error propagation. For consistency and better error management across the application, consider adding next as a parameter and using it to handle errors, aligning with the established pattern.
Proposed change:
- fieldActivity: async ({
email = "",
firstName = "",
lastName = "",
activityDetails = {},
deviceDetails = {},
activityType = "recall",
- }) => {
+ }, next) => {
try {
// Existing implementation
const response = await transporter.sendMail(mailOptions);
- return handleMailResponse(response);
+ return handleMailResponse(response, next);
} catch (error) {
logger.error(`🐛🐛 Internal Server Error ${error.message}`);
- throw new HttpError(
+ next(new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
- );
+ ));
+ return;
}
},Committable suggestion was skipped due to low confidence.
src/auth-service/utils/mailer.js
Outdated
| const getSubscribedBccEmails = async () => { | ||
| let bccEmails = constants.HARDWARE_AND_DS_EMAILS | ||
| ? constants.HARDWARE_AND_DS_EMAILS.split(",") | ||
| : []; | ||
| let subscribedEmails = []; | ||
|
|
||
| for (const bccEmail of bccEmails.map((email) => email.trim())) { | ||
| const checkResult = await SubscriptionModel( | ||
| "airqo" | ||
| ).checkNotificationStatus({ email: bccEmail, type: "email" }); | ||
| if (checkResult.success) subscribedEmails.push(bccEmail); | ||
| } | ||
|
|
||
| return subscribedEmails.join(","); | ||
| }; |
There was a problem hiding this comment.
Consider Adding Error Handling in getSubscribedBccEmails
Currently, the getSubscribedBccEmails function does not handle potential errors that may occur during the asynchronous operation of SubscriptionModel().checkNotificationStatus. If an error is thrown, it could lead to unhandled promise rejections and may affect the stability of the application. To enhance robustness, consider wrapping the await call within a try-catch block to manage any exceptions gracefully.
Suggested code modification:
const getSubscribedBccEmails = async () => {
let bccEmails = constants.HARDWARE_AND_DS_EMAILS
? constants.HARDWARE_AND_DS_EMAILS.split(",")
: [];
let subscribedEmails = [];
- for (const bccEmail of bccEmails.map((email) => email.trim())) {
- const checkResult = await SubscriptionModel("airqo").checkNotificationStatus({ email: bccEmail, type: "email" });
- if (checkResult.success) subscribedEmails.push(bccEmail);
- }
+ try {
+ for (const bccEmail of bccEmails.map((email) => email.trim())) {
+ const checkResult = await SubscriptionModel("airqo").checkNotificationStatus({ email: bccEmail, type: "email" });
+ if (checkResult.success) subscribedEmails.push(bccEmail);
+ }
+ } catch (error) {
+ logger.error(`Error checking notification status for BCC emails: ${error.message}`);
+ }
return subscribedEmails.join(",");
};Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-56: src/auth-service/utils/mailer.js#L56
Added line #L56 was not covered by tests
[warning] 58-59: src/auth-service/utils/mailer.js#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 65-65: src/auth-service/utils/mailer.js#L65
Added line #L65 was not covered by tests
| @@ -262,4 +262,31 @@ spec: | |||
| replicas: 2 | |||
| config: | |||
| retention.ms: 18000000 | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
introducing new Kafka topics for sending emails based on field activities
| @@ -0,0 +1,123 @@ | |||
| AIRQO_WEBSITE=https://airqo.net/ | |||
| AIRQO_YOUTUBE= | |||
| DB_NAME_STAGING= | |||
There was a problem hiding this comment.
Introduced the template .env file for open source contributions and new codebase setup
| @@ -196,7 +320,8 @@ const kafkaConsumer = async () => { | |||
| const topicOperations = { | |||
| // ["new-mobile-app-user-topic"]: operationForNewMobileAppUser, | |||
| ["ip-address"]: operationForBlacklistedIPs, | |||
| // topic2: operationFunction2, | |||
| ["deploy-topic"]: emailsForDeployedDevices, | |||
There was a problem hiding this comment.
Added functions to send emails when device is deployed or recalled
| @@ -1980,6 +2043,43 @@ const mailer = { | |||
| return; | |||
| } | |||
| }, | |||
|
|
|||
| fieldActivity: async ({ | |||
There was a problem hiding this comment.
new function to send field activity emails based on Kafka topics.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/auth-service/bin/jobs/kafka-consumer.js (3)
26-62: Well-structured utility functions, with a minor suggestionThe
extractDeviceDetailsfunction is well-implemented, providing a clear structure for device information. However, forextractActivityDetails, consider being more selective about which properties to include, similar toextractDeviceDetails. This approach can help prevent accidentally exposing sensitive information and make the function more intentional.const extractActivityDetails = (createdActivity) => { const { _id, type, date, // Add other relevant properties } = createdActivity; return { _id, type, date, // Return other relevant properties }; };This change would make
extractActivityDetailsmore consistent withextractDeviceDetailsand potentially more secure.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 40-40: src/auth-service/bin/jobs/kafka-consumer.js#L40
Added line #L40 was not covered by tests
[warning] 42-42: src/auth-service/bin/jobs/kafka-consumer.js#L42
Added line #L42 was not covered by tests
[warning] 59-59: src/auth-service/bin/jobs/kafka-consumer.js#L59
Added line #L59 was not covered by tests
222-270: Well-implemented email handling for deployed devicesThe
emailsForDeployedDevicesfunction is thoughtfully structured with proper error handling and input validation. A few suggestions for improvement:
- Consider using a more descriptive error message for JSON parsing errors, including the actual error details.
- The error logging in the catch block could include more context, such as the function name.
- There seems to be potential for code sharing with
emailsForRecalledDevices. Consider extracting common logic into a shared helper function to reduce duplication.Example for point 1:
} catch (error) { logger.error(`Invalid JSON format in messageData: ${error.message}`); return; }These minor adjustments could enhance the maintainability and debuggability of the code.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 224-225: src/auth-service/bin/jobs/kafka-consumer.js#L224-L225
Added lines #L224 - L225 were not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
[warning] 230-230: src/auth-service/bin/jobs/kafka-consumer.js#L230
Added line #L230 was not covered by tests
[warning] 234-235: src/auth-service/bin/jobs/kafka-consumer.js#L234-L235
Added lines #L234 - L235 were not covered by tests
[warning] 239-240: src/auth-service/bin/jobs/kafka-consumer.js#L239-L240
Added lines #L239 - L240 were not covered by tests
[warning] 243-244: src/auth-service/bin/jobs/kafka-consumer.js#L243-L244
Added lines #L243 - L244 were not covered by tests
[warning] 250-251: src/auth-service/bin/jobs/kafka-consumer.js#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 254-254: src/auth-service/bin/jobs/kafka-consumer.js#L254
Added line #L254 was not covered by tests
[warning] 265-265: src/auth-service/bin/jobs/kafka-consumer.js#L265
Added line #L265 was not covered by tests
[warning] 268-268: src/auth-service/bin/jobs/kafka-consumer.js#L268
Added line #L268 was not covered by tests
272-321: Consider refactoring to reduce duplicationThe
emailsForRecalledDevicesfunction is well-implemented but nearly identical toemailsForDeployedDevices. To improve maintainability and reduce the risk of inconsistencies, consider refactoring these functions into a single, more generic function:const handleFieldActivityEmail = async (messageData, activityType) => { // Common parsing and validation logic // ... try { // User fetching logic // ... const emailResponse = await mailer.fieldActivity({ // ... common properties ... activityType, }); // Handle email response // ... } catch (error) { // Error handling // ... } }; const emailsForDeployedDevices = (messageData) => handleFieldActivityEmail(messageData, 'deploy'); const emailsForRecalledDevices = (messageData) => handleFieldActivityEmail(messageData, 'recall');This refactoring would significantly reduce code duplication while maintaining the distinct functionality for deployed and recalled devices.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 275-276: src/auth-service/bin/jobs/kafka-consumer.js#L275-L276
Added lines #L275 - L276 were not covered by tests
[warning] 278-279: src/auth-service/bin/jobs/kafka-consumer.js#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 282-282: src/auth-service/bin/jobs/kafka-consumer.js#L282
Added line #L282 was not covered by tests
[warning] 285-286: src/auth-service/bin/jobs/kafka-consumer.js#L285-L286
Added lines #L285 - L286 were not covered by tests
[warning] 290-291: src/auth-service/bin/jobs/kafka-consumer.js#L290-L291
Added lines #L290 - L291 were not covered by tests
[warning] 294-295: src/auth-service/bin/jobs/kafka-consumer.js#L294-L295
Added lines #L294 - L295 were not covered by tests
[warning] 301-302: src/auth-service/bin/jobs/kafka-consumer.js#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-305: src/auth-service/bin/jobs/kafka-consumer.js#L305
Added line #L305 was not covered by tests
[warning] 316-316: src/auth-service/bin/jobs/kafka-consumer.js#L316
Added line #L316 was not covered by tests
[warning] 319-319: src/auth-service/bin/jobs/kafka-consumer.js#L319
Added line #L319 was not covered by testssrc/auth-service/utils/mailer.js (2)
78-109: Enhance validation increateMailOptionsConsider adding validation to
createMailOptionsto ensure that all required parameters are provided and correctly formatted. This can help prevent potential runtime errors when constructing email options.
Missing Updated Parameters in
siteActivityCallsThe current calls to
siteActivityin bothmiddleware/passport.jsfiles do not include the newly requiredactivityDetailsanddeviceDetailsparameters. Please update these calls to pass the additional parameters to ensure proper functionality.
- Files to update:
src/blog-content-manager/middleware/passport.jssrc/auth-service/middleware/passport.js🔗 Analysis chain
Line range hint
1968-2016: Verify usage of updatedsiteActivitymethodThe
siteActivitymethod now includesactivityDetailsanddeviceDetailsas parameters. Please ensure that all calls tositeActivitywithin the codebase have been updated to pass these new parameters.Run the following script to identify calls to
siteActivity:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to the 'siteActivity' method in JavaScript files # Search for calls to siteActivity and display 3 lines before and after for context rg --type js -A 3 -B 3 $'mailer\\.siteActivity\\('Length of output: 1180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/auth-service/bin/jobs/kafka-consumer.js (3 hunks)
- src/auth-service/utils/mailer.js (4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/auth-service/bin/jobs/kafka-consumer.js (1)
Learnt from: Baalmart PR: airqo-platform/AirQo-api#3554 File: src/auth-service/bin/jobs/kafka-consumer.js:270-275 Timestamp: 2024-10-04T14:41:57.423Z Learning: In `src/auth-service/bin/jobs/kafka-consumer.js`, the `emailsForRecalledDevices` function relies on the producing functionality to validate `user_id`, so additional validation in the consumer is not necessary.
🪛 GitHub Check: codecov/patch
src/auth-service/bin/jobs/kafka-consumer.js
[warning] 40-40: src/auth-service/bin/jobs/kafka-consumer.js#L40
Added line #L40 was not covered by tests
[warning] 42-42: src/auth-service/bin/jobs/kafka-consumer.js#L42
Added line #L42 was not covered by tests
[warning] 59-59: src/auth-service/bin/jobs/kafka-consumer.js#L59
Added line #L59 was not covered by tests
[warning] 224-225: src/auth-service/bin/jobs/kafka-consumer.js#L224-L225
Added lines #L224 - L225 were not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
[warning] 230-230: src/auth-service/bin/jobs/kafka-consumer.js#L230
Added line #L230 was not covered by tests
[warning] 234-235: src/auth-service/bin/jobs/kafka-consumer.js#L234-L235
Added lines #L234 - L235 were not covered by tests
[warning] 239-240: src/auth-service/bin/jobs/kafka-consumer.js#L239-L240
Added lines #L239 - L240 were not covered by tests
[warning] 243-244: src/auth-service/bin/jobs/kafka-consumer.js#L243-L244
Added lines #L243 - L244 were not covered by tests
[warning] 250-251: src/auth-service/bin/jobs/kafka-consumer.js#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 254-254: src/auth-service/bin/jobs/kafka-consumer.js#L254
Added line #L254 was not covered by tests
[warning] 265-265: src/auth-service/bin/jobs/kafka-consumer.js#L265
Added line #L265 was not covered by tests
[warning] 268-268: src/auth-service/bin/jobs/kafka-consumer.js#L268
Added line #L268 was not covered by tests
[warning] 275-276: src/auth-service/bin/jobs/kafka-consumer.js#L275-L276
Added lines #L275 - L276 were not covered by tests
[warning] 278-279: src/auth-service/bin/jobs/kafka-consumer.js#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 282-282: src/auth-service/bin/jobs/kafka-consumer.js#L282
Added line #L282 was not covered by tests
[warning] 285-286: src/auth-service/bin/jobs/kafka-consumer.js#L285-L286
Added lines #L285 - L286 were not covered by tests
[warning] 290-291: src/auth-service/bin/jobs/kafka-consumer.js#L290-L291
Added lines #L290 - L291 were not covered by tests
[warning] 294-295: src/auth-service/bin/jobs/kafka-consumer.js#L294-L295
Added lines #L294 - L295 were not covered by tests
[warning] 301-302: src/auth-service/bin/jobs/kafka-consumer.js#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-305: src/auth-service/bin/jobs/kafka-consumer.js#L305
Added line #L305 was not covered by tests
[warning] 316-316: src/auth-service/bin/jobs/kafka-consumer.js#L316
Added line #L316 was not covered by tests
[warning] 319-319: src/auth-service/bin/jobs/kafka-consumer.js#L319
Added line #L319 was not covered by testssrc/auth-service/utils/mailer.js
[warning] 56-56: src/auth-service/utils/mailer.js#L56
Added line #L56 was not covered by tests
[warning] 58-60: src/auth-service/utils/mailer.js#L58-L60
Added lines #L58 - L60 were not covered by tests
🔇 Additional comments (3)
src/auth-service/bin/jobs/kafka-consumer.js (3)
14-20: Excellent additions to support new functionalities!The new imports and global variable declarations are well-chosen to support the upcoming changes in the file. The
UserModelwill be crucial for user-related database operations, whilemongooseandObjectIdwill assist in working with MongoDB. TheisEmptyutility function can help with input validation.
340-341: Excellent integration of new topicsThe addition of 'deploy-topic' and 'recall-topic' to the
topicOperationsmapping is well-implemented and consistent with the existing pattern. This change seamlessly integrates the new email handling functions into the Kafka consumer logic.
Line range hint
26-321: Enhance reliability with comprehensive unit testsThe static analysis tool indicates that the newly added functions lack test coverage. To ensure the reliability and correctness of these critical email handling functions, it's highly recommended to add comprehensive unit tests. Consider covering the following scenarios:
- Successful email sending for both deployed and recalled devices
- Handling of invalid JSON input
- Behavior when required fields are missing
- Cases where the user is not found in the database
- Error handling when the email sending fails
Adding these tests will not only improve code coverage but also help catch potential issues early and facilitate future maintenance.
Would you like assistance in creating a template for these unit tests?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 224-225: src/auth-service/bin/jobs/kafka-consumer.js#L224-L225
Added lines #L224 - L225 were not covered by tests
[warning] 227-228: src/auth-service/bin/jobs/kafka-consumer.js#L227-L228
Added lines #L227 - L228 were not covered by tests
[warning] 230-230: src/auth-service/bin/jobs/kafka-consumer.js#L230
Added line #L230 was not covered by tests
[warning] 234-235: src/auth-service/bin/jobs/kafka-consumer.js#L234-L235
Added lines #L234 - L235 were not covered by tests
[warning] 239-240: src/auth-service/bin/jobs/kafka-consumer.js#L239-L240
Added lines #L239 - L240 were not covered by tests
[warning] 243-244: src/auth-service/bin/jobs/kafka-consumer.js#L243-L244
Added lines #L243 - L244 were not covered by tests
[warning] 250-251: src/auth-service/bin/jobs/kafka-consumer.js#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 254-254: src/auth-service/bin/jobs/kafka-consumer.js#L254
Added line #L254 was not covered by tests
[warning] 265-265: src/auth-service/bin/jobs/kafka-consumer.js#L265
Added line #L265 was not covered by tests
[warning] 268-268: src/auth-service/bin/jobs/kafka-consumer.js#L268
Added line #L268 was not covered by tests
[warning] 275-276: src/auth-service/bin/jobs/kafka-consumer.js#L275-L276
Added lines #L275 - L276 were not covered by tests
[warning] 278-279: src/auth-service/bin/jobs/kafka-consumer.js#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 282-282: src/auth-service/bin/jobs/kafka-consumer.js#L282
Added line #L282 was not covered by tests
[warning] 285-286: src/auth-service/bin/jobs/kafka-consumer.js#L285-L286
Added lines #L285 - L286 were not covered by tests
[warning] 290-291: src/auth-service/bin/jobs/kafka-consumer.js#L290-L291
Added lines #L290 - L291 were not covered by tests
[warning] 294-295: src/auth-service/bin/jobs/kafka-consumer.js#L294-L295
Added lines #L294 - L295 were not covered by tests
[warning] 301-302: src/auth-service/bin/jobs/kafka-consumer.js#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-305: src/auth-service/bin/jobs/kafka-consumer.js#L305
Added line #L305 was not covered by tests
[warning] 316-316: src/auth-service/bin/jobs/kafka-consumer.js#L316
Added line #L316 was not covered by tests
[warning] 319-319: src/auth-service/bin/jobs/kafka-consumer.js#L319
Added line #L319 was not covered by tests
| const getSubscribedBccEmails = async () => { | ||
| let bccEmails = constants.HARDWARE_AND_DS_EMAILS | ||
| ? constants.HARDWARE_AND_DS_EMAILS.split(",") | ||
| : []; | ||
| let subscribedEmails = []; | ||
|
|
||
| const checkPromises = bccEmails.map(async (bccEmail) => { | ||
| try { | ||
| const checkResult = await SubscriptionModel( | ||
| "airqo" | ||
| ).checkNotificationStatus({ email: bccEmail.trim(), type: "email" }); | ||
| return checkResult.success ? bccEmail.trim() : null; | ||
| } catch (error) { | ||
| logger.error( | ||
| `Error checking notification status for ${bccEmail}: ${error.message}` | ||
| ); | ||
| return null; | ||
| } | ||
| }); | ||
| const successfulEmails = (await Promise.all(checkPromises)).filter( | ||
| (email) => email !== null | ||
| ); | ||
| subscribedEmails = successfulEmails; | ||
| return subscribedEmails.join(","); | ||
| }; |
There was a problem hiding this comment.
Add unit tests for getSubscribedBccEmails
The new function getSubscribedBccEmails is not covered by tests, as indicated by the static analysis hints. To ensure its reliability and prevent future regressions, please add unit tests to cover various scenarios, including successful subscription checks and error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-56: src/auth-service/utils/mailer.js#L56
Added line #L56 was not covered by tests
[warning] 58-60: src/auth-service/utils/mailer.js#L58-L60
Added lines #L58 - L60 were not covered by tests
Description
Changing the email content based on field activity.
Changes Made
Testing
Affected Services
API Documentation Updated?
Additional Notes
Changing the email content based on field activity
Summary by CodeRabbit
Release Notes
New Features
recall-topicanddeploy-topic.Bug Fixes
Documentation
These enhancements aim to improve the application's functionality and user experience by facilitating better communication and management of device-related activities.