-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
just adding more logging for field activity emailing functions #3560
Conversation
📝 WalkthroughWalkthroughThe changes to 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 #3560 +/- ##
========================================
Coverage 29.68% 29.68%
========================================
Files 185 185
Lines 24997 24997
Branches 3312 3312
========================================
Hits 7421 7421
Misses 17454 17454
Partials 122 122
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/auth-service/bin/jobs/kafka-consumer.js (2)
230-239
: Excellent enhancement of input validation and error logging!I appreciate the improvements made to the
emailsForDeployedDevices
function. The addition of input parsing, validation of required fields, and enhanced error logging significantly improves the robustness of this function.A minor suggestion to further improve the code:
Consider extracting the common input validation logic into a separate function, as it's likely to be reused in other similar functions (e.g.,
emailsForRecalledDevices
). This would promote code reusability and maintainability.Here's a potential implementation:
function validateInputData(parsedData) { const { createdActivity, updatedDevice, user_id } = parsedData; if (!createdActivity || !updatedDevice || !user_id) { throw new Error(`Invalid input data: Missing required fields -- parsedData: ${stringify(parsedData)}`); } if (!ObjectId.isValid(user_id)) { throw new Error(`Invalid user_id format: ${user_id}`); } } // Usage in emailsForDeployedDevices and other similar functions try { parsedData = JSON.parse(messageData); validateInputData(parsedData); // Rest of the function logic } catch (error) { logger.error(`Error in emailsForDeployedDevices: ${error.message}`); return; }This refactoring would make the code more DRY and easier to maintain. What do you think about this approach?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-235: src/auth-service/bin/jobs/kafka-consumer.js#L235
Added line #L235 was not covered by tests
Line range hint
285-290
: Consistent improvements, but let's reduce duplicationI'm pleased to see that the improvements made to
emailsForRecalledDevices
are consistent with those inemailsForDeployedDevices
. This consistency is crucial for maintaining code quality across the codebase.However, I've noticed that these changes have introduced some code duplication between these two functions. To adhere to the DRY (Don't Repeat Yourself) principle, we should consider further refactoring.
Here's a suggestion to reduce duplication:
- Extract the common logic for parsing, validating, and sending emails into a separate function.
- Use this function in both
emailsForDeployedDevices
andemailsForRecalledDevices
.Here's a potential implementation:
async function processFieldActivityEmail(messageData, activityType) { let parsedData; try { parsedData = JSON.parse(messageData); validateInputData(parsedData); // Using the function suggested in the previous comment const { createdActivity, updatedDevice, user_id } = parsedData; const user = await UserModel("airqo") .findOne({ _id: ObjectId(user_id) }, "email _id firstName lastName") .lean(); 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: activityType, }); if (emailResponse && emailResponse.success === false) { logger.error(`🐛 Internal Server Error -- ${stringify(emailResponse)}`); } } catch (error) { logger.error(`🐛 Error in processFieldActivityEmail: ${error.message}`); } } // Usage const emailsForDeployedDevices = async (messageData) => { await processFieldActivityEmail(messageData, "deploy"); }; const emailsForRecalledDevices = async (messageData) => { await processFieldActivityEmail(messageData, "recall"); };This refactoring would significantly reduce code duplication and make future maintenance easier. What are your thoughts on this approach?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-235: src/auth-service/bin/jobs/kafka-consumer.js#L235
Added line #L235 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/auth-service/bin/jobs/kafka-consumer.js (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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.
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] 235-235: src/auth-service/bin/jobs/kafka-consumer.js#L235
Added line #L235 was not covered by tests
🔇 Additional comments (2)
src/auth-service/bin/jobs/kafka-consumer.js (2)
Line range hint
351-351
: Inquiry about commented-out Kafka topicI noticed that the "new-mobile-app-user-topic" is still commented out in the
topicOperations
object. Is this intentional, or is it planned for future implementation?If it's intended for future use, I suggest adding a TODO comment to make it clear for other developers. Here's an example:
const topicOperations = { // TODO: Implement new mobile app user handling // ["new-mobile-app-user-topic"]: operationForNewMobileAppUser, ["ip-address"]: operationForBlacklistedIPs, ["deploy-topic"]: emailsForDeployedDevices, ["recall-topic"]: emailsForRecalledDevices, };This would help maintain clarity about the status of this feature. What are your thoughts on this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-235: src/auth-service/bin/jobs/kafka-consumer.js#L235
Added line #L235 was not covered by tests
235-235
: Enhance test coverage for new error loggingThe static analysis tool has flagged that line 235 is not covered by tests. This line is part of the new error logging logic we've added.
To ensure the robustness of our error handling, I recommend adding a test case that covers this scenario. Here's a suggestion for a test case:
it('should log an error when required fields are missing', async () => { const invalidMessageData = JSON.stringify({ // Missing createdActivity and updatedDevice user_id: new ObjectId().toString() }); const loggerSpy = jest.spyOn(logger, 'error'); await emailsForDeployedDevices(invalidMessageData); expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('Invalid input data: Missing required fields')); });This test would verify that our new error logging behaves correctly when given invalid input. What do you think about adding this test case?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-235: src/auth-service/bin/jobs/kafka-consumer.js#L235
Added line #L235 was not covered by tests
Description
just adding more logging for field activity emailing functions
Changes Made
Testing
Affected Services
API Documentation Updated?
Summary by CodeRabbit
Bug Fixes
Chores