Skip to content

Fix email template loading and conditional TLS configuration #3066

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Fix email template loading and conditional TLS configuration #3066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ee0ad25
Fixing email validation
sameh0 Nov 17, 2025
0b3e023
Addressing CR
sameh0 Nov 17, 2025
621d3c7
Better handling for empty notifications
sameh0 Nov 17, 2025
ed5758f
Addressing CR comments
sameh0 Nov 19, 2025
c01bccb
Updating UI email code preview to reflect reality
sameh0 Nov 19, 2025
fe27ce2
Reverting broken code
sameh0 Nov 19, 2025
515d6cf
CR Comments
sameh0 Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions client/src/Pages/v1/Settings/SettingsEmail.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,24 @@ const SettingsEmail = ({
},
name: systemEmailConnectionHost || "localhost",
pool: systemEmailPool,
tls: {
rejectUnauthorized: systemEmailRejectUnauthorized,
ignoreTLS: systemEmailIgnoreTLS,
requireTLS: systemEmailRequireTLS,
servername: systemEmailTLSServername,
},
...(systemEmailSecure && {
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameh0 --- thanks for the update! I had a question and a suggestion regarding the new logic that builds the transport config.

The current version uses multiple nested spread operators, and while it works, it becomes quite hard to read and maintain — especially for someone new who is trying to contribute. Nested conditional spreads are clever, but they make the intention of the code harder to understand at a glance.

Another point of confusion: systemEmailRequireTLS and systemEmailIgnoreTLS were previously under the tls object, but in the PR they are now moved to the top-level. Is there a specific reason for that? Just trying to understand the intention — since mixing some TLS fields at the root and others inside tls can make the structure inconsistent.

...(systemEmailIgnoreTLS && { ignoreTLS: systemEmailIgnoreTLS }),
...(systemEmailRequireTLS && { requireTLS: systemEmailRequireTLS }),
}),
...(systemEmailSecure &&
(systemEmailRejectUnauthorized !== undefined ||
(systemEmailTLSServername &&
systemEmailTLSServername !== "")) && {
tls: {
...(systemEmailRejectUnauthorized !== undefined && {
rejectUnauthorized: systemEmailRejectUnauthorized,
}),
...(systemEmailTLSServername &&
systemEmailTLSServername !== "" && {
servername: systemEmailTLSServername,
}),
},
}),
},
null,
2
Expand Down
191 changes: 175 additions & 16 deletions server/src/service/v1/infrastructure/emailService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,67 @@ class EmailService {
*/
this.loadTemplate = (templateName) => {
try {
const templatePath = this.path.join(__dirname, `../../../templates/${templateName}.mjml`);
const templateContent = this.fs.readFileSync(templatePath, "utf8");
return this.compile(templateContent);
// Try multiple possible paths for template files
// to support both development and production environments
const possiblePaths = [
Copy link

@llamapreview llamapreview bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

The template path resolution now tries 6 different locations, which increases complexity and makes the code harder to maintain. The paths mix development, production, and build artifacts locations, creating potential confusion about which path should be used in different environments. This approach may mask configuration issues rather than solving them properly.

Code Suggestion:

const templateBase = process.env.EMAIL_TEMPLATE_PATH || this.path.join(process.cwd(), 'templates');
const templatePath = this.path.join(templateBase, `${templateName}.mjml`);

Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameh0 thanks for the contributions, I have some certain points here.

  1. Very long list of paths = harder to maintain
  2. Performance overhead for checking 6 paths per template

Why do we need all these fallback paths?
Can we standardize template paths instead of searching everywhere?

// Production/Docker path - templates in dist/templates (from dist/src/service/v1/infrastructure)
this.path.join(__dirname, `../../../templates/${templateName}.mjml`),
// Alternative production path - templates in dist/templates (from dist/service/v1/infrastructure)
this.path.join(__dirname, `../../templates/${templateName}.mjml`),
// If running from dist, templates might be in parent src directory
this.path.join(__dirname, `../../../../src/templates/${templateName}.mjml`),
// Development path - from the project root
this.path.join(process.cwd(), `templates/${templateName}.mjml`),
this.path.join(process.cwd(), `src/templates/${templateName}.mjml`),
this.path.join(process.cwd(), `dist/templates/${templateName}.mjml`),
];

let templatePath;
let templateContent;

// Try each path until we find one that works
for (const tryPath of possiblePaths) {
try {
if (this.fs.existsSync(tryPath)) {
templatePath = tryPath;
templateContent = this.fs.readFileSync(templatePath, "utf8");
break;
}
} catch (e) {
// Continue to next path
}
}

if (!templateContent) {
throw new Error(`Template file not found in any of: ${possiblePaths.map((p) => p.replace(__dirname, ".")).join(", ")}`);
}

this.logger.debug({
message: `Loading template: ${templateName}`,
service: SERVICE_NAME,
method: "loadTemplate",
templatePath: templatePath,
});

const compiled = this.compile(templateContent);

this.logger.debug({
message: `Template loaded successfully: ${templateName}`,
service: SERVICE_NAME,
method: "loadTemplate",
});
return compiled;
} catch (error) {
this.logger.error({
message: error.message,
message: `Failed to load template '${templateName}': ${error.message}`,
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good. Better clarity.

service: SERVICE_NAME,
method: "loadTemplate",
templateName: templateName,
error: error.message,
stack: error.stack,
});
// Return a no-op function that returns empty string to prevent runtime errors
return () => "";
Comment on lines +108 to +109
Copy link

@llamapreview llamapreview bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 | Confidence: High

When template loading fails, the code returns a function that produces empty content. This creates a silent failure mode where emails are sent with empty content instead of proper error handling. The related context shows that both notificationService.js and settingsController.js rely on sendEmail returning a messageId for success, but they won't receive clear errors about template failures, making debugging difficult.

Code Suggestion:

// Re-throw the error to let callers handle template failures properly
throw new Error(`Failed to load template '${templateName}': ${error.message}`);

Evidence: path:server/src/service/v1/infrastructure/notificationService.js, path:server/src/controllers/v1/settingsController.js

}
};

Expand Down Expand Up @@ -83,20 +134,90 @@ class EmailService {

buildEmail = async (template, context) => {
try {
if (!this.templateLookup[template]) {
this.logger.error({
message: `Template '${template}' not found in templateLookup`,
service: SERVICE_NAME,
method: "buildEmail",
availableTemplates: Object.keys(this.templateLookup),
});
throw new Error(`Template '${template}' not found`);
}
if (typeof this.templateLookup[template] !== "function") {
this.logger.error({
message: `Template '${template}' is not a function. Type: ${typeof this.templateLookup[template]}`,
service: SERVICE_NAME,
method: "buildEmail",
templateValue: this.templateLookup[template],
});
throw new Error(`Template '${template}' is not a function`);
}
const mjml = this.templateLookup[template](context);

// Check if MJML is empty (template failed to load)
if (!mjml || mjml.trim() === "") {
const msg = `Template '${template}' returned empty MJML content. Template may have failed to load.`;
this.logger.error({
message: msg,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
});
throw new Error(msg);
}

const html = await this.mjml2html(mjml);

// Check if HTML is empty
if (!html || !html.html) {
const msg = `MJML conversion failed for template '${template}'. No HTML output.`;
this.logger.error({
message: msg,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
mjmlLength: mjml.length,
});
throw new Error(msg);
}

return html.html;
} catch (error) {
this.logger.error({
message: error.message,
message: `Failed to build email for template '${template}': ${error.message}`,
service: SERVICE_NAME,
method: "buildEmail",
template: template,
error: error.message,
stack: error.stack,
});
throw error;
}
};

sendEmail = async (to, subject, html, transportConfig) => {
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameh0 , honestly the sendEmail is becoming bigger and bigger now, harder to maintain then.

Possible to have a clean code, like:

`sendEmail = async (to, subject, html, transportConfig) => {
// 1. Validate incoming email data
html = this.validateEmailParams(to, subject, html);
if (html === false) return false;

// 2. Load config
const config = await this.getTransportConfig(transportConfig);

// 3. Validate from address
const from = this.validateFromAddress(config.systemEmailAddress, config.systemEmailUser);
if (!from) return false;

// 4. Build TLS options
const tlsRelated = this.buildTLSConfig({
	secure: config.systemEmailSecure,
	systemEmailIgnoreTLS: config.systemEmailIgnoreTLS,
	systemEmailRequireTLS: config.systemEmailRequireTLS,
	systemEmailRejectUnauthorized: config.systemEmailRejectUnauthorized,
	systemEmailTLSServername: config.systemEmailTLSServername,
});

// 5. Build full nodemailer config
const emailConfig = this.buildTransport({ ...config, ...tlsRelated });

// 6. Send
return await this.sendWithTransporter(from, to, subject, html, emailConfig);

};
`

This is just a snapshot which explains what I am thinking, clearly breaking the responsibilities and maintaining it will become much easier.

// Validate required fields
if (!to || !subject) {
this.logger.error({
message: "Invalid email parameters: missing 'to' or 'subject'",
service: SERVICE_NAME,
method: "sendEmail",
});
return false;
}

// Validate HTML content
Copy link

@llamapreview llamapreview bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: Medium

The new validation rejects empty HTML content, but the related context shows notificationService.js calls sendEmail without checking the HTML content first. This could break notification flows where template rendering fails silently. While preventing empty emails is good, the change introduces a hard failure where previously notifications might have continued with degraded functionality.

Code Suggestion:

if (!html || html.trim() === "") {
    this.logger.warn({
        message: "Email HTML content is empty, using fallback text",
        service: SERVICE_NAME,
        method: "sendEmail",
    });
    html = "<p>Email content unavailable</p>";
}

if (!html || html.trim() === "") {
this.logger.warn({
message: "Email HTML content is empty, using fallback text",
service: SERVICE_NAME,
method: "sendEmail",
to: to,
subject: subject,
});
html = "<p>Email content unavailable</p>";
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we sending the email if the content is not available??

}

let config;
if (typeof transportConfig !== "undefined") {
config = transportConfig;
Expand All @@ -107,17 +228,28 @@ class EmailService {
systemEmailHost,
systemEmailPort,
systemEmailSecure,
systemEmailPool,
systemEmailUser,
systemEmailAddress,
systemEmailPassword,
systemEmailConnectionHost,
systemEmailTLSServername,
systemEmailRejectUnauthorized,
systemEmailIgnoreTLS,
systemEmailRequireTLS,
systemEmailRejectUnauthorized,
systemEmailTLSServername,
} = config;

// Validate from address
const fromAddress = systemEmailAddress || systemEmailUser;
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameh0 any reason why we are considering the user and email address is same here??

if (!fromAddress || !fromAddress.includes("@")) {
this.logger.error({
message: "Invalid from email address",
service: SERVICE_NAME,
method: "sendEmail",
});
return false;
}

// Build base email config
const emailConfig = {
host: systemEmailHost,
port: Number(systemEmailPort),
Expand All @@ -128,14 +260,40 @@ class EmailService {
},
name: systemEmailConnectionHost || "localhost",
connectionTimeout: 5000,
pool: systemEmailPool,
Copy link
Contributor

@Owaiseimdad Owaiseimdad Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify the reasoning behind that?
pool controls whether Nodemailer keeps SMTP connections open and reuses them, which can significantly improve performance for multiple emails or batch sending.

tls: {
rejectUnauthorized: systemEmailRejectUnauthorized,
ignoreTLS: systemEmailIgnoreTLS,
requireTLS: systemEmailRequireTLS,
servername: systemEmailTLSServername,
},
};

// Conditionally add TLS settings only if secure is enabled
if (systemEmailSecure) {
// Add top-level TLS options
if (systemEmailIgnoreTLS !== undefined) {
emailConfig.ignoreTLS = systemEmailIgnoreTLS;
}
if (systemEmailRequireTLS !== undefined) {
emailConfig.requireTLS = systemEmailRequireTLS;
}

const tlsSettings = {};

// Only add TLS settings that are explicitly configured
// (rejectUnauthorized and servername go INSIDE the tls object)
if (systemEmailRejectUnauthorized !== undefined) {
tlsSettings.rejectUnauthorized = systemEmailRejectUnauthorized;
}
if (systemEmailTLSServername !== undefined && systemEmailTLSServername !== null && systemEmailTLSServername !== "") {
tlsSettings.servername = systemEmailTLSServername;
}

// Only add tls property if we have TLS settings
if (Object.keys(tlsSettings).length > 0) {
emailConfig.tls = tlsSettings;
this.logger.debug({
message: `TLS settings applied to email config`,
service: SERVICE_NAME,
method: "sendEmail",
tlsSettings: Object.keys(tlsSettings),
});
}
}
this.transporter = this.nodemailer.createTransport(emailConfig);

try {
Expand All @@ -152,7 +310,7 @@ class EmailService {
try {
const info = await this.transporter.sendMail({
to: to,
from: systemEmailAddress,
from: fromAddress,
subject: subject,
html: html,
});
Expand All @@ -164,6 +322,7 @@ class EmailService {
method: "sendEmail",
stack: error.stack,
});
return false;
}
};
}
Expand Down