feat: add Tax CRUD API with default tax handling#1433
Conversation
| if (isDefault) { | ||
| await Tax.updateMany({ isDefault: true }, { isDefault: false }); | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion: taxName and taxValue are required fields in the database, but there is currently no validation to ensure they are provided. We should add proper validation and return an appropriate error if these fields are missing.
| // Controller to get tax by ID | ||
| exports.getTaxById = async (req, res) => { | ||
| try { | ||
| const tax = await Tax.findById(req.params.id); |
There was a problem hiding this comment.
suggestion: req.params.id is being used directly in the database query without any validation. We should validate the ID (e.g., check if it is a valid MongoDB ObjectId) before querying the database to prevent potential errors or unexpected behavior.
| // Here our API Routes | ||
|
|
||
| // api routes for tax management | ||
| app.use('/api/tax', taxRoutes); |
There was a problem hiding this comment.
question: The tax routes /api/tax are not protected by any authentication middleware, while other routes are using adminAuth.isValidAuthToken. Is this intentional, or should we also secure these routes to prevent unauthorized access?
2 participants
Please provide a brief description of the changes or additions made in this pull request.
Related Issues
If this pull request is related to any issue(s), please list them here.
Steps to Test
Provide steps on how to test the changes introduced in this pull request.
Screenshots (if applicable)
If your changes include visual updates, it would be helpful to provide screenshots of the before and after.
Checklist