just ensure that activity emails have the AirQo logo #4318
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThe pull request introduces a systematic enhancement to the email functionality in the Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4318 +/- ##
========================================
Coverage 11.24% 11.24%
========================================
Files 156 156
Lines 17919 17919
Branches 388 388
========================================
Hits 2015 2015
Misses 15902 15902
Partials 2 2
|
|
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: 0
🔭 Outside diff range comments (2)
src/auth-service/utils/common/mailer.js (2)
Line range hint
19-63: Improve robustness of attachments configuration.The attachments configuration has several areas for improvement:
- Inconsistent path construction: Some paths use
path.join()while others use string concatenation.- No error handling for missing image files.
- Hardcoded image paths could be moved to configuration.
Consider applying these improvements:
const projectRoot = path.join(__dirname, "..", ".."); // Go two levels up const imagePath = path.join(projectRoot, "config", "images"); +const fs = require('fs'); + +// Move to configuration +const LOGO_CONFIG = { + airqo: { filename: "airqoLogo.png", cid: "AirQoEmailLogo" }, + facebook: { filename: "facebookLogo.png", cid: "FacebookLogo" }, + youtube: { filename: "youtubeLogo.png", cid: "YoutubeLogo" }, + twitter: { filename: "Twitter.png", cid: "Twitter" }, + linkedin: { filename: "linkedInLogo.png", cid: "LinkedInLogo" } +}; + +// Create attachments with error handling +const attachments = Object.entries(LOGO_CONFIG).map(([key, config]) => { + const filepath = path.join(imagePath, config.filename); + // Verify file exists + if (!fs.existsSync(filepath)) { + logger.error(`Missing image file: ${filepath}`); + return null; + } + return { + filename: config.filename, + path: filepath, + cid: config.cid, + contentDisposition: "inline" + }; +}).filter(Boolean);
Line range hint
119-1859: Reduce code duplication in mailer methods.The mailer methods contain significant code duplication, particularly in subscription checking and email sending logic.
Consider extracting common patterns into reusable functions:
+// Extract common subscription check +const checkSubscription = async (email, tenant = "airqo") => { + const checkResult = await SubscriptionModel(tenant).checkNotificationStatus({ + email, + type: "email" + }); + return checkResult; +}; + +// Extract common email sending logic +const sendEmail = async (mailOptions, skipActualSend = false) => { + if (skipActualSend || mailOptions.to === "automated-tests@airqo.net") { + return { + success: true, + message: "email successfully sent", + data: [], + status: httpStatus.OK + }; + } + + const data = await transporter.sendMail(mailOptions); + return handleMailResponse(data); +}; + +// Extract common BCC email collection +const getSubscribedBccEmails = async (emailList, tenant = "airqo") => { + if (!emailList) return ""; + + const bccEmails = emailList.split(",").map(email => email.trim()); + const subscribedEmails = await Promise.all( + bccEmails.map(async (email) => { + const checkResult = await checkSubscription(email, tenant); + return checkResult.success ? email : null; + }) + ); + + return subscribedEmails.filter(Boolean).join(","); +};This refactoring would:
- Reduce code duplication
- Improve maintainability
- Make the code more testable
- Centralize error handling
Example usage in a mailer method:
mailer.candidate = async ( { firstName, lastName, email, tenant = "airqo" } = {}, next ) => { try { - const checkResult = await SubscriptionModel(tenant).checkNotificationStatus({ - email, - type: "email" - }); + const checkResult = await checkSubscription(email, tenant); if (!checkResult.success) { return checkResult; } - let bccEmails = []; - if (constants.REQUEST_ACCESS_EMAILS) { - bccEmails = constants.REQUEST_ACCESS_EMAILS.split(","); - } - // ... more BCC email handling + const bcc = await getSubscribedBccEmails(constants.REQUEST_ACCESS_EMAILS, tenant); const mailOptions = { from: { name: constants.EMAIL_NAME, address: constants.EMAIL }, to: email, subject: "AirQo Analytics JOIN request", html: msgs.joinRequest(firstName, lastName, email), bcc, attachments }; - if (email === "automated-tests@airqo.net") { - return { - success: true, - message: "email successfully sent", - data: [], - status: httpStatus.OK - }; - } - - let response = transporter.sendMail(mailOptions); - let data = await response; - if (isEmpty(data.rejected) && !isEmpty(data.accepted)) { - return { - success: true, - message: "email successfully sent", - data, - status: httpStatus.OK - }; - } else { - next(new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { - message: "email not sent", - emailResults: data - })); - } + return await sendEmail(mailOptions); } catch (error) { logger.error(`🐛🐛 Internal Server Error ${error.message}`); next( new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { message: error.message }) ); } };
🧹 Nitpick comments (1)
src/auth-service/utils/common/mailer.js (1)
Line range hint
65-118: Enhance error handling and input validation in utility functions.The utility functions could benefit from improved error handling and input validation:
Consider these improvements:
const createMailOptions = ({ email, firstName, lastName, activityDetails, deviceDetails, bccEmails, activityType, } = {}) => { + // Validate required inputs + if (!email) { + throw new Error('Email is required'); + } + const subject = activityType === "recall" ? "AirQo Analytics: Device Recall Notification" : "AirQo Analytics: Device Deployment Notification"; return { from: { name: constants.EMAIL_NAME, address: constants.EMAIL, }, to: email, subject, html: msgs.field_activity({ firstName, lastName, email, activityDetails, deviceDetails, activityType, }), bcc: bccEmails, attachments: attachments, }; }; const handleMailResponse = (data) => { + if (!data) { + throw new HttpError( + "Invalid mail response", + httpStatus.INTERNAL_SERVER_ERROR + ); + } + 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 } ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/auth-service/utils/common/mailer.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/auth-service/utils/common/mailer.js (1)
Line range hint
1-1859: Address security considerations in email handling.Several security aspects need attention:
- Email headers could be vulnerable to injection
- Attachment paths could be manipulated
- BCC email addresses are exposed in the code
Consider implementing these security measures:
- Sanitize all email headers and content
- Validate and sanitize file paths
- Move sensitive email addresses to secure configuration
- Implement rate limiting for email sending
- Add audit logging for email operations
- Consider using email templates from a secure source
Run this security check:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
just ensure that activity emails have the AirQo logo. These activity emails are triggered by Kafka consumer immediately device activities are carried out.
Changes Made
Testing
Affected Services
API Documentation Updated?
Summary by CodeRabbit
New Features
Improvements