247 lines
13 KiB
Plaintext
247 lines
13 KiB
Plaintext
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:
|
|
<input type="text" id="orderNum" name="orderNum" pInputText [(ngModel)]="selectedItem.orderNumber" maxlength="20" style="font-weight: bold;">
|
|
have been changed to
|
|
<input type="text" id="orderNum" name="orderNum" pInputText [(ngModel)]="selectedItem.orderNumber"
|
|
maxlength="20" style="font-weight: bold;">
|
|
|
|
+ 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:
|
|
<span i18n="@@amountReqVal" *ngIf="logPaymentForm.amount == 0 && (amount.dirty || amount.touched)"
|
|
class="ui-message ui-messages-error ui-corner-all">
|
|
Amount is required
|
|
</span>
|
|
should change to:
|
|
<span i18n="@@amountReqVal" *ngIf="logPaymentForm.amount == 0 && (amount.dirty || amount.touched)" class="ui-message ui-messages-error ui-corner-all">Amount is required</span>
|
|
|
|
- Try to separate translated text from special characters like (:,%,!).
|
|
Example:
|
|
<label i18n="@@taxRate">Tax rate (%)</label>
|
|
should change to:
|
|
<label><ng-container i18n="@@taxRate">Tax rate</ng-container> (%)</label>
|
|
|
|
- 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:
|
|
<div i18n="@@tax" style="width: 150px; text-align: right; flex: 1;">Tax
|
|
<span>({{printDetail.client.taxRate}}%)</span>
|
|
</div>
|
|
change to
|
|
<div style="width: 150px; text-align: right; flex: 1;">
|
|
<ng-container i18n="@@tax">Tax</ng-container>
|
|
<span>({{printDetail.client.taxRate}}%)</span>
|
|
</div>
|
|
|
|
- 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.
|
|
|