Bug #4062
openCode review on Ravi shankar task for implement invoice generation after accept quote in the same page not in other new page - 23/09/2025
0%
Description
1. Single Responsibility Principle
This controller is handling too many responsibilities:
Input validation
Loading DB models
Fetching & updating quotation
Handling action history
Creating invoice
Sending emails
Generating JWT
Building API response
Each of these should be broken into separate service functions.
2. Move Validation Logic to Middleware / Utility
Current code validates status, signature, etc. inline.
Better: have a validation middleware so your controller stays clean.
validateAcceptQuotation(req, res, next) {
// Validate status and signature rules here
}
3.Error Handling
Many try/catch blocks inline → clutter.
Use a global error handler middleware (like we discussed earlier with CustomError).
Instead of returning inline 400/500, just throw new CustomError(...) and let middleware handle the response.
4.Split Business Logic Into Services
->Create QuotationService for:
findByToken(token)
updateQuotationStatus(quotation, status, signature, haveAquestion)
addActionHistory(quotation, changes, performedBy)
->Create InvoiceService for:
createInvoiceFromQuotation(quotation)
sendInvoiceEmail(invoice, client)
->Create EmailService for:
sendQuotationStatusEmail(quotation, status, invoice)
In controller we can use above like this
const quotation = await QuotationService.findByToken(token);
QuotationService.updateStatus(quotation, status, signature, haveAquestion);
if (status === "accepted") {
const invoice = await InvoiceService.createInvoiceFromQuotation(quotation);
await InvoiceService.sendInvoiceEmail(invoice, quotation.client);
}
await QuotationService.save(quotation);
await EmailService.sendQuotationStatusEmail(quotation, status, invoice);
5. Avoid Deep Nesting
Right now you have deeply nested if/else + try/catch.
Early return on errors (✅ you already do this a bit).
But some blocks like invoice creation could be moved into a helper.
6. Template/Email Duplication
Email template rendering code (ejs.renderFile(...), attachments, options) is repeated.
Extract into EmailService utility:EmailService.sendTemplate(to, subject, templateName, data, attachments);
7. Magic Strings → Constants
"accepted", "inProgress", "completed", "drafted" should be constants or enums.
Prevents typos and makes it reusable.
8. Token/Invoice Handling
JWT generation inline makes the controller noisy.
Move to AuthService.generateQuotationToken(payload).
9. Response Building
Right now, building res.status(200).json({...}) is verbose.
Extract into a helper function Response.success(res, data, message).
Benefits to apply above cases in your current code style
Controller is now ~50 lines, not 300.
Easier to read & test.
Business logic reusable in other places (e.g., CRON jobs, API v2).
Errors are centralized.
Email sending is abstracted.
Subtasks
Related issues
No data to display