Skip to content

fixes #125, address remaining review issues from PR #124 #180

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

Merged
yash-pouranik merged 2 commits into from
May 19, 2026
Merged

fixes #125, address remaining review issues from PR #124 #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd0277c
Fix mail template sanitization, test coverage, validation type mismat…
tirth1356 May 17, 2026
3b78063
fix: address coderabbitai review issues from PR #124
tirth1356 May 19, 2026
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
28 changes: 16 additions & 12 deletions apps/dashboard-api/src/controllers/project.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,7 +1489,7 @@ const toSlug = (value) => {
};


module.exports.listMailTemplates = async (req, res) => {
module.exports.listMailTemplates = async (req, res, next) => {
try {
const { projectId } = req.params;

Expand Down Expand Up @@ -1604,11 +1604,12 @@ module.exports.listMailTemplates = async (req, res) => {
message: "Mail templates fetched.",
});
} catch (err) {
return res.status(500).json({ success: false, data: {}, message: "Failed to fetch mail templates." });
if (err instanceof AppError) return next(err);
return next(new AppError(500, "Internal server error"));
}
};

module.exports.listGlobalMailTemplates = async (req, res) => {
module.exports.listGlobalMailTemplates = async (req, res, next) => {
try {
const { projectId } = req.params;

Expand Down Expand Up @@ -1639,11 +1640,12 @@ module.exports.listGlobalMailTemplates = async (req, res) => {
message: "Global mail templates fetched.",
});
} catch (err) {
return res.status(500).json({ success: false, data: {}, message: err.message });
if (err instanceof AppError) return next(err);
return next(new AppError(500, "Internal server error"));
}
};

module.exports.getMailTemplate = async (req, res) => {
module.exports.getMailTemplate = async (req, res, next) => {
try {
const { projectId, templateId } = req.params;
if (!mongoose.isValidObjectId(templateId)) {
Expand Down Expand Up @@ -1702,11 +1704,12 @@ module.exports.getMailTemplate = async (req, res) => {
message: "Mail template fetched.",
});
} catch (err) {
return res.status(500).json({ success: false, data: {}, message: "Failed to fetch template." });
if (err instanceof AppError) return next(err);
return next(new AppError(500, "Internal server error"));
}
};

module.exports.createMailTemplate = async (req, res) => {
module.exports.createMailTemplate = async (req, res, next) => {
try {
const { projectId } = req.params;

Expand Down Expand Up @@ -1767,11 +1770,11 @@ module.exports.createMailTemplate = async (req, res) => {
return res.status(409).json({ success: false, data: {}, message: "Template name/key already exists." });
}

return res.status(500).json({ success: false, data: {}, message: "Failed to create template." });
return next(new AppError(500, "Internal server error"));
}
};

module.exports.updateMailTemplate = async (req, res) => {
module.exports.updateMailTemplate = async (req, res, next) => {
try {
const { projectId, templateId } = req.params;
if (!mongoose.isValidObjectId(templateId)) {
Expand Down Expand Up @@ -1853,11 +1856,11 @@ module.exports.updateMailTemplate = async (req, res) => {
return res.status(409).json({ success: false, data: {}, message: "Template name/key already exists." });
}

return res.status(500).json({ success: false, data: {}, message: "Failed to update template." });
return next(new AppError(500, "Internal server error"));
}
};

module.exports.deleteMailTemplate = async (req, res) => {
module.exports.deleteMailTemplate = async (req, res, next) => {
try {
const { projectId, templateId } = req.params;
if (!mongoose.isValidObjectId(templateId)) {
Expand All @@ -1880,7 +1883,8 @@ module.exports.deleteMailTemplate = async (req, res) => {

return res.json({ success: true, data: {}, message: "Mail template deleted." });
} catch (err) {
return res.status(500).json({ success: false, data: {}, message: "Failed to delete template." });
if (err instanceof AppError) return next(err);
return next(new AppError(500, "Internal server error"));
}
};

Expand Down
5 changes: 3 additions & 2 deletions apps/dashboard-api/src/middlewares/authMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ module.exports = function (req, res, next) {
// Proceed to the next middleware or route handler
next();
} catch (err) {
console.log("err---------------2")
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

res.status(400).json({ error: 'Invalid Token' });
}
Expand Down
151 changes: 150 additions & 1 deletion apps/public-api/src/__tests__/mail.controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ jest.mock('@urbackend/common', () => {
};
});

const { Project, decrypt, redis, publicEmailQueue, MailLog } = require('@urbackend/common');
const { Project, decrypt, redis, publicEmailQueue, MailTemplate, MailLog } = require('@urbackend/common');
const mailController = require('../controllers/mail.controller');
const originalResendApiKey2 = process.env.RESEND_API_KEY_2;

Expand Down Expand Up @@ -129,6 +129,18 @@ describe('mail.controller', () => {
success: true,
data: expect.objectContaining({ provider: 'byok', monthlyUsage: 1 }),
}));
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
projectId: 'proj_1',
usingByok: true,
payload: expect.objectContaining({
to: 'user@example.com',
subject: 'Hello',
text: 'This is a message.'
})
}), expect.objectContaining({
attempts: 3,
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
}));
});

test('falls back to default key when BYOK missing', async () => {
Expand All @@ -145,6 +157,18 @@ describe('mail.controller', () => {
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
data: expect.objectContaining({ provider: 'default', monthlyUsage: 2 }),
}));
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
projectId: 'proj_1',
usingByok: false,
payload: expect.objectContaining({
to: 'user@example.com',
subject: 'Hello',
text: 'This is a message.'
})
}), expect.objectContaining({
attempts: 3,
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
}));
});

test('enforces monthly limit', async () => {
Expand Down Expand Up @@ -208,6 +232,131 @@ describe('mail.controller', () => {
}), expect.objectContaining({ attempts: expect.any(Number) }));
});

test('renders and sends a project-scoped mail template from DB', async () => {
const req = makeReq();
req.body = {
to: 'user@example.com',
templateName: 'welcome',
variables: { name: 'Yash' },
};
const res = makeRes();

mockProjectConfig({
_id: 'proj_1',
resendApiKey: null,
});

MailTemplate.findOne.mockReturnValueOnce({
lean: jest.fn().mockResolvedValue({
_id: 'tpl_db_1',
name: 'welcome',
subject: 'Hello {{name}}',
text: 'Welcome to project!',
html: '<p>Welcome to project!</p>',
projectId: 'proj_1'
})
});

decrypt.mockReturnValue(null);
redis.eval.mockResolvedValue(1);

await mailController.sendMail(req, res);

// Assert project-scope query was made first
expect(MailTemplate.findOne).toHaveBeenCalledWith(expect.objectContaining({
projectId: 'proj_1',
}));

expect(res.status).toHaveBeenCalledWith(200);
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
success: true,
data: expect.objectContaining({
templateUsed: expect.objectContaining({ name: 'welcome', id: 'tpl_db_1', scope: 'project' }),
}),
}));
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
projectId: 'proj_1',
usingByok: false,
payload: expect.objectContaining({
to: 'user@example.com',
subject: 'Hello Yash',
text: 'Welcome to project!',
html: '<p>Welcome to project!</p>'
})
}), expect.objectContaining({
attempts: 3,
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
}));
});

test('renders and sends a global mail template from DB when no project template exists', async () => {
const req = makeReq();
req.body = {
to: 'user@example.com',
templateName: 'welcome',
variables: { name: 'Yash' },
};
const res = makeRes();

mockProjectConfig({
_id: 'proj_1',
resendApiKey: null,
});

// First call: project scope (returns null)
MailTemplate.findOne.mockReturnValueOnce({
lean: jest.fn().mockResolvedValue(null)
});

// Second call: global scope
MailTemplate.findOne.mockReturnValueOnce({
lean: jest.fn().mockResolvedValue({
_id: 'tpl_global_1',
name: 'welcome',
subject: 'Global Hello {{name}}',
text: 'Global welcome!',
html: '<p>Global welcome!</p>',
projectId: null,
isSystem: true
})
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

decrypt.mockReturnValue(null);
redis.eval.mockResolvedValue(1);

await mailController.sendMail(req, res);

// Assert project-scope was queried first, then global fallback
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(1, expect.objectContaining({
projectId: 'proj_1',
}));
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(2, expect.objectContaining({
projectId: null,
isSystem: true,
}));

expect(res.status).toHaveBeenCalledWith(200);
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
success: true,
data: expect.objectContaining({
templateUsed: expect.objectContaining({ name: 'welcome', id: 'tpl_global_1', scope: 'global' }),
}),
}));
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
projectId: 'proj_1',
usingByok: false,
payload: expect.objectContaining({
to: 'user@example.com',
subject: 'Global Hello Yash',
text: 'Global welcome!',
html: '<p>Global welcome!</p>'
})
}), expect.objectContaining({
attempts: 3,
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
}));
});

test('refunds quota on terminal async worker failure', async () => {
let failedHandler;
jest.resetModules();
Expand Down
28 changes: 21 additions & 7 deletions apps/public-api/src/controllers/data.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ module.exports.insertData = async (req, res) => {
if (isDebug) console.log(`[DEBUG] insert data took ${(performance.now() - start).toFixed(2)}ms`);
res.status(201).json(result);
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

if (isDuplicateKeyError(err)) {
return res.status(409).json({
Expand Down Expand Up @@ -173,7 +175,9 @@ module.exports.insertData = async (req, res) => {
message: "Bulk insert successful",
});
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

if (isDuplicateKeyError(err)) {
return next(
Expand Down Expand Up @@ -285,7 +289,9 @@ module.exports.getAllData = async (req, res) => {
message: "Data fetched successfully",
});
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

if (err && (err.statusCode === 400 || err.name === 'QueryFilterError')) {
return res.status(400).json({
Expand Down Expand Up @@ -358,7 +364,9 @@ module.exports.getSingleDoc = async (req, res) => {

res.json(doc);
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}
res.status(500).json({ error: err.message });
}
};
Expand Down Expand Up @@ -418,7 +426,9 @@ module.exports.aggregateData = async (req, res) => {
message: "Aggregation executed successfully.",
});
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

if (err instanceof z.ZodError) {
return res.status(400).json({
Expand Down Expand Up @@ -489,7 +499,9 @@ module.exports.updateSingleData = async (req, res) => {

res.json({ message: "Updated", data: result });
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}

if (isDuplicateKeyError(err)) {
return res.status(409).json({
Expand Down Expand Up @@ -557,7 +569,9 @@ module.exports.deleteSingleDoc = async (req, res) => {

res.json({ message: "Document deleted", id });
} catch (err) {
console.error(err);
if (process.env.NODE_ENV !== 'test') {
console.error(err);
}
res.status(500).json({ error: err.message });
}
};
4 changes: 3 additions & 1 deletion packages/common/src/queues/publicEmailQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ const initPublicEmailWorker = () => {
});

worker.on('failed', async (job, err) => {
console.error(`[Queue] Job ${job?.id} (public email) failed:`, err);
if (process.env.NODE_ENV !== 'test') {
console.error(`[Queue] Job ${job?.id} (public email) failed:`, err);
}
if (job && job.data && job.data.consumedQuotaKey) {
const maxAttempts = job.opts?.attempts || 1;
if (job.attemptsMade >= maxAttempts) {
Expand Down
5 changes: 4 additions & 1 deletion packages/common/src/utils/input.validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,10 @@ module.exports.updateWebhookSchema = z.object({

module.exports.sendMailSchema = z
.object({
to: z.string().email("Invalid recipient email format"),
to: z.union([
z.string().email("Invalid recipient email format"),
z.array(z.string().email("Invalid recipient email format")).nonempty("Recipient list cannot be empty")
]),

// Direct-send fields (backward compatible)
subject: z.preprocess(
Expand Down
Loading