General: 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. - Error Handling: We suggest to centralize error handling using following pattern: 1. create a util function that accepts an error object and returns a message string function handleErr(error): string { // extract error code // if error code has message attached return error message, // else map error code with a message return 'error message' } 2. use the handleErr function inside the error block of any observable subscriptions 3. display the message to the user either inline or with the msgSvc like this.invoiceSvc.getSettingByClientId().subscribe({error : (err) => this.handleErr.msgSvc(addFailedMsg(err))}) ---------------------------- customer-settings.component.ts invoice-settings-guard.service.ts settings.component.ts customer-settings-resolver.service.ts setting-resolver.service.ts - The spacing format have been modified in serveral contents from the original without any behaviour changes in the following files: job-edit.component.html job-edit.component.ts job-list.component.ts app.menu.components.ts - Example: have been changed to + Do not modify (if did please 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. - Checking for not null of a service is not necessary since the base component should already injected it. Example: this.authSvc != null && this.authSvc.hasRole([RoleIds.APP, RoleIds.APP_ADM]); - The the business logic to calculate the values for (subTotal, discounted, totalExcludingTax, taxed, total) are duplicated in invoice-detail.component.ts invoice-edit.component.ts invoices-list.component.ts - These calculations are critical to the application and should be placed in a single util function, and tested for various of edge cases. + The generic format of the function is something like: calcInvoice(subTotal, discount, taxRate) { // do calculation return { subTotal, discounted, totalExcludingTax, taxed, total }; } - The calculation for the sum of an array is redundant in many components. job-list.component.ts Example: this.invoice.clients .map(i => this.billToListPriceObject(i).totalExcludingTax) .reduce( (accumulator, currentValue) => accumulator + currentValue, 0, ); this.invoice.clients .map(i => +i.split) .reduce( (accumulator, currentValue) => accumulator + currentValue, 0, ); Do: - Replace all these redundant sums with a common util function that return a default value in case the array is not defined. The structure of the util function can be something like: const sum = (arr: Array[], field: string, func?: Function) => arr?.map((item) => func ? +(func(item)[field]) : +item[field]).reduce((acc,val) => acc + val, 0) || 0; totalExcludingTax = sum(this.invoice.clients, 'totalExcludingTax', this.billToListPriceObject); totalSplit = sum(this.invoice.clients, 'split'); - Make sure the min and max values of form fields are bounded according to context Example: - Taxes and Discount percentages should be bounded min = 0, max = 100 - Various http calls to backend api that being done directly within these components: invoice-detail.component.ts invoice-edit.component.ts invoices-list.component.ts - Some of these functions have usability issues that may effect the user like downloadCSV, saveWithLogPayment, saveWithLogPayment. These calls don't have proper error handlings incase if the http calls return error conditions. Do: - Separate these calls into ngrx effects and dispatch them with proper error handlings or add error handling for each subscribe block. invoice-edit.component.html - Don't linebreak on translated text as it will add additional line when the text is translated to another language. Example: Amount is required should change to: Amount is required - Try to separate translated text from special characters like (:,%,!). Example: should change to: - Create css class for the rules "background-color: transparent; color: #212121;" "text-align: center; font-weight: 600; border-bottom: 1px solid #bdbdbd;" "gap: 4px; justify-content: center" "justify-content: end" "border-bottom: 1px solid #bdbdbd;" "display: flex; justify-content: space-between" Do: - Use css overriding to reuse rules - The logic isNew || isDraftEdit is repeated in many places in the template and the ts files Do: - Create a predicate function for the logic isNew || isDraftEdit invoice-edit.component.ts - getSettingByClientId http call on line 369-396, why is there such complex error handling mechanism, and it is being called again on line 773 and the error handling here is different. Do: - If getSettingByClientId information is reused, use a resolver to fetch the data before the component is loaded. Otherwise, if this information is critical to the component, show a descriptive errror message to the user explaining why. As of right now, it seems that it just fails silently and resets the form to some values. - There are no error handlings of the observable subscriptions in the function saveNewTerm and removeTermOpt, loadJobs, fetchLogPayment, saveInvoice. Do: - Generally any function that make http request should have error handling condition with a message telling the source of the error. - In saveWithLogPayment, removeBillToClient, addClientToInvoice, saveNewTerm, addClientDlg make sure that this.selectedClients, and this.selectedJobs, this.paymentTermOpts this.orgSearchClientList are not null before calling map and [...] syntax Do: - Use optional chaining https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining or if statement to check for null arrays first - saveWithLogPayment, saveNewTerm, saveLog the asynchronous call sequence implementation are incorrect without proper error handlings. Do: - Use pipe to chain the http calls. For example, this.invoiceSvc.saveInvoice().pipe( switchMap(() => { return this.invoiceSvc.createListLogPayment(payload); }) ).subscribe({error: (err) => handleErr(err)}); // Generic handleErr() mentioned above in General. if these same sequence of calls are repeated, create an ngrx action and effect for the sequence and dispatch it with a single command. see https://rxjs.dev/api/index/function/pipe on how to chain the async calls and catch the errors in one place. invoices-list.component.ts - On line 147-158 ngAfterViewInit, check dueDate.value and dueDate.value for null before calling map function. What happen when the user clear the calendar? is there a function that clears 'inv-ops' from sessionStorage? invoice-detail.component.html - See invoice-edit.component comments for translated text line breaks and special characters. - Create css class for rules "width: 200px; text-align: right; font-weight: 400;" "border-bottom: 1px solid #bdbdbd;" "font-weight: 600;" "min-width: 150px; padding-left: 0;" "padding-left: 0;" - Why is there an interpolation of a digit ({{1}}) value between lines 365 and 369 in invoice-detail.component.html - Separate translated text from nested elements with interpolation: Example:
Tax ({{printDetail.client.taxRate}}%)
change to
Tax ({{printDetail.client.taxRate}}%)
- Create a function to compute the value as these are redundant. printDetail.client.amountDue ? printDetail.client.amountDue : printDetail.total invoice-detail.component.ts - In the saveLog function of the observable subscriptions between lines 316-327, do these async api calls need to be sequential? - what if fetchInvoiceDetail, fetchLogPayment failed? the message snackbar will still show "Create log payment succeeded" as it will be confusing to the user. Do: - See https://rxjs.dev/api/index/function/pipe on how chain the async calls and catch the errors in one place. settings.component.html - See invoice-edit.component.html comments for translated text line breaks and special characters. - See invoice-edit.component.html comments for separating translated text from nested elements. - Interpolate the allowedFormats array items for the allowed formats text on line (134-138) as these can change later on. - Max value for Tax and Discount % should be 100. settings.component.ts - Move allowedFormats, maxLogoSize constant into a shared variable in the module. - Create a requiredFieldTrimmed function in invoice service to share the logic in both setting.component and customer-setting.component. Example: requiredFieldTrimmed(setting: CustomerInvoiceSetting) { return setting?.companyName?.length == setting?.companyName?.trim().length && setting?.address?.length == setting?.address?.trim().length } - There are no error handling in the functions saveNewTerm,removeTermOpt see comments for invoice-edit.component.ts above for more details on how to handle http call errors. customer-settings.component.html - See invoice-edit.component comments for translated text line breaks and special characters. - See settings.component comments for allowedFormats, maxLogoSize, requiredFieldTrimmed. - Max value for Tax and Discount % should be 100. costing-item.component.html - Wrap translatable texts in p-header element with ng-container. job-list.component.ts - Create an enum constant for statuses ('all', 'new', 'ready', 'download', 'spray', 'invoiced'). - Wrap lines 148-168 into a function and do a return after each this.statusFilter assignment avoiding future bugs where more than one of the conditions can be true. Example: if (!listFilter.filters.status?.value && listFilter.filters.invoices?.value == 1) { this.statusFilter = 'invoiced'; } change to if (!listFilter.filters.status?.value && listFilter.filters.invoices?.value == 1) { return this.statusFilter = 'invoiced'; } - Is there a default value for this.statusFilter if (listFilter && listFilter.filters) is false? - Should this.statusFilter = 'all' be in the the outter if/else as the default value instead? job-edit.component.ts - Why is there async await in createInvoice function? invoice-list-canactive.guard.ts - What is the purpose of this guard? its only function is to fetch the invoice list. The list then get fetched again in invoices-list.component.ts ngOnInit. invoice-edit-resolver.service.ts and invoice-detail-resolver.service.ts - These two resolvers get the exact same data from the backend. Try to combine them into one resolver. invoice-create-canactive.guard.ts and customer-setting-list-canactive.guard.ts - These two guards seem to have the same function. Try to combine them into one. enums folder contents - Combine these ts files into a single shared file in the module as these are just constant values. - Freeze the invoiceStatus object. invoice.service.ts - getPrintDetail need a return type from the backend. - Check for null array of jobs parameter in flattenJobCostingItem(jobs). client.service.ts - getClientWithInvoiceSetting needs a return type (important: API contract) from the backend.