227 lines
13 KiB
Plaintext
227 lines
13 KiB
Plaintext
General:
|
|
What's good:
|
|
+ User roles and permissions validation implemented in routes.
|
|
+ Endpoint params are granularly validated using 'joi' schemas.
|
|
+ Used helpers function for reusability and avoiding code duplication.
|
|
|
|
What's Todo for production:
|
|
- DRY (DO NOT REPEAT YOURSELF) principle to be applied.
|
|
- To avoid potential crashing, it should do guard check (i.e.: !utils.isEmptyArray(array)) before accessing/calling functions of any array, object, string etc. especially on function's input arguments.
|
|
- Do not modify/restore to the orginal format if there are no behaviour changes, and keep the same formattings as it's easier for us to review the codes.
|
|
- All ending code statements to be ended with comma (;).
|
|
- 2 spaces (tab) each level indents to applied to all code files. When modifying existing files. The previous indentations should be kept intacted.
|
|
- When done editing, a good practice to always performe:
|
|
+ Pressing a combination of Ctrl+Shift+I to format the whole codes within the file to a common indentation. If using VSCode, these settings is already available under .vscode\settings.json file of each project folder.
|
|
+ Clean up dead codes or redandant codes such as empty/unsed functions, imports, constants, etc.
|
|
+ Add proper comments where there are complicated logic/algorithm.
|
|
|
|
- Keep simple codes (such as properties of objects) inline when possible for better readability.
|
|
For example:
|
|
const job = job.findOne({
|
|
_id: jobId,
|
|
});
|
|
|
|
Changed to: const job = job.findOne({ _id: jobId }); is much cleaner, shorter and easier to read.
|
|
|
|
------------------
|
|
Routes:
|
|
- Most of Joi fluent objects or child objects/properties are similar in structure. Can they be resused to avoid duplicated codes?
|
|
- const editRoles, viewRoles should not be duplicated in all Job Invoicing routes.
|
|
|
|
invoice_settings.js:
|
|
- Why paymentTerm in createInvoiceSettingSchema is required but optional in updateInvoiceSettingSchema?
|
|
- .route('/byClient/:clientId') and .route('/:invoiceSettingId') are duplicated (repeated) with the exact similar handlings. How about using route regex be used or simply params 1 route? E.g.: .route('/:invoiceSettingId/byClient/:clientId')
|
|
|
|
costing_items.js:
|
|
- Why all field validation for updateCostingItemSchema are optional compared but required in createCostingItemSchema?
|
|
|
|
invoices.js:
|
|
- Most of properties and nested ones in .items({..}) in updateInvoiceSchema and in createInvoiceSchema are the same with only .code property different.
|
|
- Why openDate, dueDate, jobs, clients are optional in updateInvoiceSchema but required in createInvoiceSchema?
|
|
- Why companyName, address, logo, currency in createInvoiceSchema but not updateInvoiceSchema? Is currency optional in updateInvoiceSchema?
|
|
- Why all costing item fields are optional in updateInvoiceSchema.jobs.items.costings? Are invalid costing items allowed?
|
|
- What is clients.items.code for?
|
|
- Is invoiceIds (array) expected in getExportMultipleInformationSchema intead of invoiceId?
|
|
- route('/:id'). Validation schema is missing. To add validation for expected params.
|
|
|
|
log_payment.js:
|
|
- Check and revise comments to refer to logpayment instead of '*invoice'
|
|
- Why updateLogPaymentSchema is and empty Joi schema object (Joi.object({})) with no validation?
|
|
- /createMultipleLogPayments => to renamed to /createLogPayments. Notice that it already ending with plural (s).
|
|
- Input fields Naming should be consistent to avoid confusion. All object Ids to be [name]Id. E.g.: invoice => invoiceId, client => clientId. Please remember to update controller's handlers after these changes.
|
|
|
|
|
|
------------------
|
|
Controllers:
|
|
- Errors are contrialized handled in /middlewares/error_handle.js, thus all try catch at the top level in each controllers's API handlers are redundant. Thus, they should be removed. Specific try/catch based on known logic can also be used then they might need to be rethrown.
|
|
Example:
|
|
async apiHandler(req, res, next) {
|
|
try {
|
|
....
|
|
} catch (error) {
|
|
AppParamError.throw(error);
|
|
}
|
|
}
|
|
- Delete API handlers should not throw an error if the document(s) can not be found.
|
|
- Naming. Controllers's API handler functions should be named ending with verb (_get, _post, _put) except delete.
|
|
|
|
invoice_settings.js:
|
|
- Did the FrontEnd handle Errors.INVOICE_SETTING_NOT_FOUND when calling RUD Endpoints ?
|
|
|
|
costing_items.js:
|
|
- In updateCostingItemSchema, why .name and .price are optional? When changing, please remember DRY.
|
|
- Did the FrontEnd handle Errors.COSTING_ITEM_NOT_FOUND when calling RUD Endpoints ?
|
|
|
|
invoices.js:
|
|
- To revise:
|
|
+ Handler functions's naming convention (.._suffix exept delete). E.g.: getInvoices => getInvoices_get
|
|
+ Possible inline codes
|
|
|
|
- getInvoices():
|
|
+ To guard the the case if puid is null (or not an valid objectid) to avoid unnecessary whole collection scanning.
|
|
+ To remove duplication codes for getting invoices and adding them to an array, line 34-76. Isn't there is only one field differed in the filter object?
|
|
|
|
- getJobById():
|
|
+ if (!job) AppParamError.throw(Errors.TO_NOT_FOUND); => to correct the error code.
|
|
|
|
- generateRandomInvoiceCode():
|
|
+ How about simplifing invoiceUnique by using utils.padZero(num, size) instead?
|
|
|
|
- createInvoice():
|
|
+ What if invoiceBody.clients, invoiceBody.jobs, jobIds, listJobs are null or undefined (later)? => the whole server app would crash.
|
|
+ To guard for the case if puid is null (or not an valid ObjectId). If puid is not valid, it must return [] or throw an error to avoid potential return all invoices within the db.
|
|
|
|
- getInvoiceById():
|
|
+ To guard for the case if puid, invoiceId are null (or not an valid objectid).
|
|
+ To avoid potential crashing, it should do guard check before accessing any array (e.g.: invoice.clients.filter()).
|
|
+ The same duplication pattern (only diff. in query filter object) repeated when querying invoices, then checkInvoiceStatus() like mentioned in getInvoices(). To be revised avoiding code duplication.
|
|
|
|
- checkInvoiceStatus():
|
|
+ What if invoice param is null or app? => undefined the whole server will crash.
|
|
|
|
- getPrintInformation():
|
|
+ Line 223, invoice.clients.find(..) and line 240 invoiceObject.jobs(). To avoid potential crashing, it should do guard check before accessing any array.
|
|
|
|
- getUnitName(unit):
|
|
+ Line 260, unit.toString().toLowerCase(). It should do safe guard check before calling unit's string chain functions to avoid potential crash.
|
|
|
|
- calInvoiceExport():
|
|
+ As mentioned in getInvoiceById(). To guard for the case if puid is null (or not an valid ObjectId). If puid is not valid, it must return [] or throw an error to avoid potential return all invoices within the db.
|
|
+ To revise and make sure always doing safe guard check before accessing an array (.clients, .jobs, .items,..)
|
|
+ It returns an complicated data but the structure was not documented. To add documentation mentioning about the params and return data structure.
|
|
|
|
- geInvoiceCsv():
|
|
+ 1st line, calInvoiceExport(req, invoiceIds)).flat().flat().flat(). Doing shortcuts to access array functions like this is not a good practice because of potential crash.
|
|
+ Line 372, delimiter: ';'. Isn't it to be ',' for CSV output?
|
|
|
|
- splitAddress(inputAddress):
|
|
+ inputAddress.trim(). What if inputAddress is null or undefined?
|
|
|
|
- geInvoiceIIF():
|
|
+ 1st line, const invoices = await calInvoiceExport(req, invoiceIds); Is it expected to return an invoice list?. There is only 1 iteration for clients [line 419.. for (const clients of invoices)]
|
|
|
|
- getExportInformation(), getExportMultipleInformation():
|
|
+ DRY. They looks almost 90+% duplicated. What is the real different in these two functions?
|
|
+ Refactoring to only one function exportInvoices(req ({ invoiceIds:[], invoiceType }), res) should be enough to handle export 1 or multiple invoices.
|
|
|
|
- updateInvoiceById():
|
|
+ Firstly, must do safe guard checks on input params (invoiceId (potential null or undefined), data (potential null of clients and jobs and others), puid) before using.
|
|
+ When req.userInfo is null or undefined => It should not proceed but throw an AppError error.
|
|
+ Less param objects => inline 1 line for readability.
|
|
+ Invoice.update() is deprecated. To use other update[..] function instead.
|
|
+ If arrays like .clients is optional, .clients.map will potentially crash.
|
|
+ For effiency and data integrity, all updates logic (if status == Void) to be best made on a deep clone of the req.body then finally calling updateOne to the db at one time.
|
|
|
|
- deleteInvoiceById(), deleteInvoiceByIdUsecase():
|
|
+ ..Invoice.findByIdAndDelete({ _id: invoiceId, byPuid: puid }). Model.findByIdAndDelete(id, ..) expect an single value id param not an condition object. To use Model.findOneAndDelete({_id, byPuid},..) instead.
|
|
+ Thowing Errors.INVOICE_NOT_FOUND is not necessary.
|
|
+ deleteInvoiceByIdUsecase() => rename to deleteInvoice(id, puid)
|
|
+ These two function do the same thing with duplicated content. deleteInvoiceById() should call deleteInvoice(id, puid).
|
|
|
|
- deleteMultipleInvoices():
|
|
+ To rename to deleteInvoices(..); calling deleteInvoice(id, puid).
|
|
|
|
- checkInvoiceStatus(invoice):
|
|
+ First must do safe guard check on invoice before accessing its properties or the app will potentially crash if invoice is null or indefined.
|
|
|
|
|
|
log_payment.js:
|
|
- Why are these API handler functions blank? getLogPaymentById(req, res), updateLogPayment(req, res), deleteLogPayment(req, res)
|
|
- createMultipleLogPayments() => to rename to createLogPayments as mentioned in the route.
|
|
|
|
- getLogPayments():
|
|
+ The same duplication pattern (only diff. in query filter object) repeated when querying for payment logs. To remove duplication codes by using query condition conditionally.
|
|
|
|
- createLogPayment():
|
|
+ Firstly, must do safe guard check on invoice.clients before calling invoice.clients.find() to avoid potential crash.
|
|
+ Why saving the invoice without any changes? Line 74, invoice.save()
|
|
|
|
- createMultipleLogPayments():
|
|
+ createMultipleLogPayments => renamed to createLogPayments as mentioned in route.
|
|
+ Why also saving invoice here? Line 122, await invoice.save()
|
|
+ Why throw Errors.TO_NOT_FOUND error (line 104)?
|
|
|
|
- createLogPayment() and createMultipleLogPayments():
|
|
+ Both have the same duplicated logic to log an payment log. To refactor to one function called LogPayment(paymentLog). Then reuse them in these two handler functions.
|
|
|
|
job.js:
|
|
- handleCostingItems().... Why not use the costing items as being seen on the UI when the user creating/updating a job? Please remove this logic.
|
|
|
|
- createJob_post():
|
|
+ In the UI, it does not clone the job's costing items. Why calling handleCostingItems() here?
|
|
+ As Why does it still query the invoices collection to get invoice's related info? This would create an future severve performance issue when there are lots of invoices. How's about adding an invoiceStatus field within the job model and updating the corresponding status when needed? Remember to update the status again when a job was removed from and invoice or when the invoice is deleted.
|
|
|
|
- updateJob_put():
|
|
+ Why querying and updating invoices, jobs's costing items when updating a job? It is unnecessary and will create a problem of user data integrity and performance hit.
|
|
|
|
- getJobs_get():
|
|
+ When added the invoiceStatus to the job, the join/lookup to invoices collection can be removed here. Then, It would help to improve the performance by avoid scanning invoices collection.
|
|
|
|
|
|
------------------
|
|
Models:
|
|
- To simplify properties with inline codes for cleaner and readability.
|
|
|
|
costing_item.js:
|
|
- userParentId field to be renamed to byPuid for consistency.
|
|
|
|
invoice_settings.js:
|
|
- Please add comments for .userId and userParenetId. Is .userId for storing a client userid?
|
|
|
|
invoice.js:
|
|
- PaymentTerm should not be confused with real life invoice payment term (days to due(be paid) after open). Thus rename to something like createOp to represent how the invoice was created with one of the three options. Likewise, the PaymentTerm constant to be rename as InvCreateOption
|
|
- overdueDateAllowance. Should be renamed to paymentTerm.
|
|
|
|
location.js:
|
|
+ What is require('./invoice_settings'); for?
|
|
|
|
job.js:
|
|
+ add invoiceStatus field to manage its invoicing status as mentioned in the job.js controller.
|
|
+ Please restore the original spacing and indents
|
|
|
|
------------------
|
|
Helpers:
|
|
- We use global dotenv to avoid duplication in each project and for cleaner deployment. Thus, no dotenv in local dependencies. Reference: Preload in https://www.npmjs.com/package/dotenv
|
|
|
|
validate.js:
|
|
- To be relocated under /middlewares/ folder.
|
|
- HTTP 400 status code and the validation error message. How/did the FrontEnd handle these Joi validation errors?. We will need to merge this into the application current architech by returning an AppInputError with 409 code and may be along with the additional message field.
|
|
|
|
multer.js:
|
|
+ folderInvoiceSettingUpload => hidden path constant. It is to use an ENV param such as INV_CFG_UPLOAD_DIR. Please be noted that all ENV params need to be wrapped in the env.js helper module.
|
|
+ To be relocated under /middlewares/ folder.
|
|
|
|
- invoiceSettingsUpload():
|
|
+ It should allow image file extentions in uppercase as well.
|
|
+ fileSize should use an ENV like INV_CFG_MAX_UPLOAD_SIZE_MB = 5 (default).
|
|
|
|
env.js:
|
|
+ We are using global dotenv. Please remove dotenv related usages.
|
|
|
|
utils.js
|
|
+ Why using Bignumber types?
|
|
+ Why using dayjs? momentjs has been used and should be used in the project.
|
|
|
|
------------------
|
|
|