Skip to content

Commit f3cd28f

Browse files
yash-pouranikyash-pouranik
authored
Merge pull request #180 from tirth1356/fix/mail-template-issues
fixes #125, address remaining review issues from PR #124
2 parents 213f952 + 3b78063 commit f3cd28f

6 files changed

Lines changed: 197 additions & 24 deletions

File tree

‎apps/dashboard-api/src/controllers/project.controller.js‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,7 +1489,7 @@ const toSlug = (value) => {
14891489
};
14901490

14911491

1492-
module.exports.listMailTemplates = async (req, res) => {
1492+
module.exports.listMailTemplates = async (req, res, next) => {
14931493
try {
14941494
const { projectId } = req.params;
14951495

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

1611-
module.exports.listGlobalMailTemplates = async (req, res) => {
1612+
module.exports.listGlobalMailTemplates = async (req, res, next) => {
16121613
try {
16131614
const { projectId } = req.params;
16141615

@@ -1639,11 +1640,12 @@ module.exports.listGlobalMailTemplates = async (req, res) => {
16391640
message: "Global mail templates fetched.",
16401641
});
16411642
} catch (err) {
1642-
return res.status(500).json({ success: false, data: {}, message: err.message });
1643+
if (err instanceof AppError) return next(err);
1644+
return next(new AppError(500, "Internal server error"));
16431645
}
16441646
};
16451647

1646-
module.exports.getMailTemplate = async (req, res) => {
1648+
module.exports.getMailTemplate = async (req, res, next) => {
16471649
try {
16481650
const { projectId, templateId } = req.params;
16491651
if (!mongoose.isValidObjectId(templateId)) {
@@ -1702,11 +1704,12 @@ module.exports.getMailTemplate = async (req, res) => {
17021704
message: "Mail template fetched.",
17031705
});
17041706
} catch (err) {
1705-
return res.status(500).json({ success: false, data: {}, message: "Failed to fetch template." });
1707+
if (err instanceof AppError) return next(err);
1708+
return next(new AppError(500, "Internal server error"));
17061709
}
17071710
};
17081711

1709-
module.exports.createMailTemplate = async (req, res) => {
1712+
module.exports.createMailTemplate = async (req, res, next) => {
17101713
try {
17111714
const { projectId } = req.params;
17121715

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

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

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

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

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

18811884
return res.json({ success: true, data: {}, message: "Mail template deleted." });
18821885
} catch (err) {
1883-
return res.status(500).json({ success: false, data: {}, message: "Failed to delete template." });
1886+
if (err instanceof AppError) return next(err);
1887+
return next(new AppError(500, "Internal server error"));
18841888
}
18851889
};
18861890

‎apps/dashboard-api/src/middlewares/authMiddleware.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,9 @@ module.exports = function (req, res, next) {
3131
// Proceed to the next middleware or route handler
3232
next();
3333
} catch (err) {
34-
console.log("err---------------2")
35-
console.error(err);
34+
if (process.env.NODE_ENV !== 'test') {
35+
console.error(err);
36+
}
3637

3738
res.status(400).json({ error: 'Invalid Token' });
3839
}

‎apps/public-api/src/__tests__/mail.controller.test.js‎

Lines changed: 150 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ jest.mock('@urbackend/common', () => {
7070
};
7171
});
7272

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

@@ -129,6 +129,18 @@ describe('mail.controller', () => {
129129
success: true,
130130
data: expect.objectContaining({ provider: 'byok', monthlyUsage: 1 }),
131131
}));
132+
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
133+
projectId: 'proj_1',
134+
usingByok: true,
135+
payload: expect.objectContaining({
136+
to: 'user@example.com',
137+
subject: 'Hello',
138+
text: 'This is a message.'
139+
})
140+
}), expect.objectContaining({
141+
attempts: 3,
142+
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
143+
}));
132144
});
133145

134146
test('falls back to default key when BYOK missing', async () => {
@@ -145,6 +157,18 @@ describe('mail.controller', () => {
145157
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
146158
data: expect.objectContaining({ provider: 'default', monthlyUsage: 2 }),
147159
}));
160+
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
161+
projectId: 'proj_1',
162+
usingByok: false,
163+
payload: expect.objectContaining({
164+
to: 'user@example.com',
165+
subject: 'Hello',
166+
text: 'This is a message.'
167+
})
168+
}), expect.objectContaining({
169+
attempts: 3,
170+
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
171+
}));
148172
});
149173

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

235+
test('renders and sends a project-scoped mail template from DB', async () => {
236+
const req = makeReq();
237+
req.body = {
238+
to: 'user@example.com',
239+
templateName: 'welcome',
240+
variables: { name: 'Yash' },
241+
};
242+
const res = makeRes();
243+
244+
mockProjectConfig({
245+
_id: 'proj_1',
246+
resendApiKey: null,
247+
});
248+
249+
MailTemplate.findOne.mockReturnValueOnce({
250+
lean: jest.fn().mockResolvedValue({
251+
_id: 'tpl_db_1',
252+
name: 'welcome',
253+
subject: 'Hello {{name}}',
254+
text: 'Welcome to project!',
255+
html: '<p>Welcome to project!</p>',
256+
projectId: 'proj_1'
257+
})
258+
});
259+
260+
decrypt.mockReturnValue(null);
261+
redis.eval.mockResolvedValue(1);
262+
263+
await mailController.sendMail(req, res);
264+
265+
// Assert project-scope query was made first
266+
expect(MailTemplate.findOne).toHaveBeenCalledWith(expect.objectContaining({
267+
projectId: 'proj_1',
268+
}));
269+
270+
expect(res.status).toHaveBeenCalledWith(200);
271+
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
272+
success: true,
273+
data: expect.objectContaining({
274+
templateUsed: expect.objectContaining({ name: 'welcome', id: 'tpl_db_1', scope: 'project' }),
275+
}),
276+
}));
277+
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
278+
projectId: 'proj_1',
279+
usingByok: false,
280+
payload: expect.objectContaining({
281+
to: 'user@example.com',
282+
subject: 'Hello Yash',
283+
text: 'Welcome to project!',
284+
html: '<p>Welcome to project!</p>'
285+
})
286+
}), expect.objectContaining({
287+
attempts: 3,
288+
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
289+
}));
290+
});
291+
292+
test('renders and sends a global mail template from DB when no project template exists', async () => {
293+
const req = makeReq();
294+
req.body = {
295+
to: 'user@example.com',
296+
templateName: 'welcome',
297+
variables: { name: 'Yash' },
298+
};
299+
const res = makeRes();
300+
301+
mockProjectConfig({
302+
_id: 'proj_1',
303+
resendApiKey: null,
304+
});
305+
306+
// First call: project scope (returns null)
307+
MailTemplate.findOne.mockReturnValueOnce({
308+
lean: jest.fn().mockResolvedValue(null)
309+
});
310+
311+
// Second call: global scope
312+
MailTemplate.findOne.mockReturnValueOnce({
313+
lean: jest.fn().mockResolvedValue({
314+
_id: 'tpl_global_1',
315+
name: 'welcome',
316+
subject: 'Global Hello {{name}}',
317+
text: 'Global welcome!',
318+
html: '<p>Global welcome!</p>',
319+
projectId: null,
320+
isSystem: true
321+
})
322+
});
323+
324+
decrypt.mockReturnValue(null);
325+
redis.eval.mockResolvedValue(1);
326+
327+
await mailController.sendMail(req, res);
328+
329+
// Assert project-scope was queried first, then global fallback
330+
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(1, expect.objectContaining({
331+
projectId: 'proj_1',
332+
}));
333+
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(2, expect.objectContaining({
334+
projectId: null,
335+
isSystem: true,
336+
}));
337+
338+
expect(res.status).toHaveBeenCalledWith(200);
339+
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({
340+
success: true,
341+
data: expect.objectContaining({
342+
templateUsed: expect.objectContaining({ name: 'welcome', id: 'tpl_global_1', scope: 'global' }),
343+
}),
344+
}));
345+
expect(publicEmailQueue.add).toHaveBeenCalledWith("send-public-email", expect.objectContaining({
346+
projectId: 'proj_1',
347+
usingByok: false,
348+
payload: expect.objectContaining({
349+
to: 'user@example.com',
350+
subject: 'Global Hello Yash',
351+
text: 'Global welcome!',
352+
html: '<p>Global welcome!</p>'
353+
})
354+
}), expect.objectContaining({
355+
attempts: 3,
356+
backoff: expect.objectContaining({ type: 'exponential', delay: 5000 })
357+
}));
358+
});
359+
211360
test('refunds quota on terminal async worker failure', async () => {
212361
let failedHandler;
213362
jest.resetModules();

‎apps/public-api/src/controllers/data.controller.js‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,9 @@ module.exports.insertData = async (req, res) => {
8484
if (isDebug) console.log(`[DEBUG] insert data took ${(performance.now() - start).toFixed(2)}ms`);
8585
res.status(201).json(result);
8686
} catch (err) {
87-
console.error(err);
87+
if (process.env.NODE_ENV !== 'test') {
88+
console.error(err);
89+
}
8890

8991
if (isDuplicateKeyError(err)) {
9092
return res.status(409).json({
@@ -173,7 +175,9 @@ module.exports.insertData = async (req, res) => {
173175
message: "Bulk insert successful",
174176
});
175177
} catch (err) {
176-
console.error(err);
178+
if (process.env.NODE_ENV !== 'test') {
179+
console.error(err);
180+
}
177181

178182
if (isDuplicateKeyError(err)) {
179183
return next(
@@ -285,7 +289,9 @@ module.exports.getAllData = async (req, res) => {
285289
message: "Data fetched successfully",
286290
});
287291
} catch (err) {
288-
console.error(err);
292+
if (process.env.NODE_ENV !== 'test') {
293+
console.error(err);
294+
}
289295

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

359365
res.json(doc);
360366
} catch (err) {
361-
console.error(err);
367+
if (process.env.NODE_ENV !== 'test') {
368+
console.error(err);
369+
}
362370
res.status(500).json({ error: err.message });
363371
}
364372
};
@@ -418,7 +426,9 @@ module.exports.aggregateData = async (req, res) => {
418426
message: "Aggregation executed successfully.",
419427
});
420428
} catch (err) {
421-
console.error(err);
429+
if (process.env.NODE_ENV !== 'test') {
430+
console.error(err);
431+
}
422432

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

490500
res.json({ message: "Updated", data: result });
491501
} catch (err) {
492-
console.error(err);
502+
if (process.env.NODE_ENV !== 'test') {
503+
console.error(err);
504+
}
493505

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

558570
res.json({ message: "Document deleted", id });
559571
} catch (err) {
560-
console.error(err);
572+
if (process.env.NODE_ENV !== 'test') {
573+
console.error(err);
574+
}
561575
res.status(500).json({ error: err.message });
562576
}
563577
};

‎packages/common/src/queues/publicEmailQueue.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,9 @@ const initPublicEmailWorker = () => {
103103
});
104104

105105
worker.on('failed', async (job, err) => {
106-
console.error(`[Queue] Job ${job?.id} (public email) failed:`, err);
106+
if (process.env.NODE_ENV !== 'test') {
107+
console.error(`[Queue] Job ${job?.id} (public email) failed:`, err);
108+
}
107109
if (job && job.data && job.data.consumedQuotaKey) {
108110
const maxAttempts = job.opts?.attempts || 1;
109111
if (job.attemptsMade >= maxAttempts) {

‎packages/common/src/utils/input.validation.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,10 @@ module.exports.updateWebhookSchema = z.object({
553553

554554
module.exports.sendMailSchema = z
555555
.object({
556-
to: z.string().email("Invalid recipient email format"),
556+
to: z.union([
557+
z.string().email("Invalid recipient email format"),
558+
z.array(z.string().email("Invalid recipient email format")).nonempty("Recipient list cannot be empty")
559+
]),
557560

558561
// Direct-send fields (backward compatible)
559562
subject: z.preprocess(

0 commit comments

Comments
 (0)