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: should change to: - 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: