WEB-95 Fix problems with GLIM application: saved total, "Moratorium", "Overdue Charges"" #2717
Conversation
WalkthroughAdds parent-entry support to GLIM request construction with conditional payload fields and shifted requestIds; removes the component submit method; changes notify signature/behavior; adds moratorium form controls; introduces overdue-charge edit handler; applies ARIA/type template fixes and replaces a MatCheckbox import with MatCheckboxModule. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CreateGlimAccountComponent
participant Builder as setData(...)
participant Server as Backend
rect rgba(135,206,250,0.12)
note right of UI: buildRequestData()
UI->>Builder: setData(parentClient, totalLoan, isParent=true)\n(requestId: "0", includes totalLoan & isParentAccount)
Builder-->>UI: parentRequest
loop for each member i
UI->>Builder: setData(memberClient_i, undefined, isParent=false)\n(requestId: (i+1))
Builder-->>UI: memberRequest_i
end
UI->>UI: assemble requests array (parent + members)\nensure groupId, dateFormat, locale present
end
UI->>Server: POST assembled GLIM payload
Server-->>UI: response
note right of UI: notify(body, data) builds message + JSON.stringify(data) and logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
306-331: Refactor to eliminate code duplication.The
editOverdueChargeAmountmethod is nearly identical toeditChargeAmount(lines 189-214), differing only in the dialog title and target data source. Consider refactoring to a shared private method:private editChargeAmountCommon( charge: any, dialogTitle: string, dataSource: any[] ): void { const formfields: FormfieldBase[] = [ new InputBase({ controlName: 'amount', label: 'Amount', value: charge.amount, type: 'number', required: false }) ]; const data = { title: dialogTitle, layout: { addButtonText: 'Confirm' }, formfields: formfields }; const editNoteDialogRef = this.dialog.open(FormDialogComponent, { data }); editNoteDialogRef.afterClosed().subscribe((response: any) => { if (response.data) { const newCharge = { ...charge, amount: response.data.value.amount }; dataSource.splice(dataSource.indexOf(charge), 1, newCharge); // Trigger change detection by creating new array reference if (dataSource === this.chargesDataSource) { this.chargesDataSource = this.chargesDataSource.concat([]); } else if (dataSource === this.overDueChargesDataSource) { this.overDueChargesDataSource = this.overDueChargesDataSource.concat([]); } this.pristine = false; } }); } editChargeAmount(charge: any) { this.editChargeAmountCommon(charge, 'Edit Charge Amount', this.chargesDataSource); } editOverdueChargeAmount(charge: any) { this.editChargeAmountCommon(charge, 'Edit Overdue Charge Amount', this.overDueChargesDataSource); }Note: The
pristineflag should only be set tofalsewhen the user confirms the change (inside the subscribe callback), not when they cancel. Consider moving line 330 inside theif (response.data)block. However, since this mirrors the existing behavior ineditChargeAmount(line 213), addressing this in both methods would be ideal but can be deferred to maintain consistency with the current implementation.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
178-209: Clarify thetotalLoanparameter usage.The
totalLoanparameter is only used whenisParentis true (lines 200), but it's passed in all calls tosetData(lines 221, 231). This can be confusing for maintainers.Consider making the parameter truly optional or documenting why it's passed for member accounts even though it's unused in those cases.
Apply this diff to make the parameter optional and document its usage:
- setData(client: any, totalLoan: number, isParent?: boolean): any { + setData(client: any, totalLoan?: number, isParent?: boolean): any { const locale = this.settingsService.language.code; const dateFormat = this.settingsService.dateFormat; // const monthDayFormat = 'dd MMMM';Or explicitly pass
undefinedfor member accounts to make the intent clear:- body: this.setData(memberSelected[index], totalLoan, false) + body: this.setData(memberSelected[index], undefined, false)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
697-701: Remove redundant field assignments.The
moratoriumPrincipalandmoratoriumInterestfields are already included in the object returned bygetRawValue(), so explicitly adding them is redundant.Apply this diff to simplify:
get loansAccountTerms() { - return { - ...this.loansAccountTermsForm.getRawValue(), - moratoriumPrincipal: this.loansAccountTermsForm.get('moratoriumPrincipal')?.value, - moratoriumInterest: this.loansAccountTermsForm.get('moratoriumInterest')?.value - }; + return this.loansAccountTermsForm.getRawValue(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(2 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(5 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(2 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (2)
src/app/shared/form-dialog/formfield/model/formfield-base.ts (1)
FormfieldBase(3-38)src/app/shared/form-dialog/formfield/model/input-base.ts (1)
InputBase(17-25)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🔇 Additional comments (8)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (2)
42-50: Excellent accessibility improvements!The addition of
type="button"attributes prevents unintended form submissions, and thearia-labelattributes enhance screen reader accessibility. These changes align well with WCAG guidelines.Also applies to: 84-97, 104-104, 156-166
131-142: New overdue charge edit functionality looks good.The new edit button for overdue charge amounts follows the same pattern as the existing charge edit buttons and includes proper accessibility attributes.
src/app/login/login-form/login-form.component.ts (1)
13-13: Remove review comment — incorrect technical guidance about Angular Material importsThe web search confirms that for standalone components in Angular Material 19, the correct pattern is to import
MatCheckboxModule(not the individualMatCheckboxcomponent) in the imports array. The original review's claim that this represents a "backwards" move away from modern best practices is incorrect. The PR change aligns with Angular Material's documented standalone component pattern.Likely an incorrect or invalid review comment.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
195-197: LGTM! Essential fields now included in all payloads.The addition of
dateFormat,locale, andgroupIdto all loan payloads ensures consistency and proper data formatting across parent and child requests.
199-202: LGTM! Parent account distinction implemented correctly.The conditional logic properly adds
totalLoanandisParentAccountonly for parent account requests, enabling the API to distinguish between parent and child loan accounts.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
520-522: LGTM! Moratorium form controls added correctly.The new
moratoriumPrincipalandmoratoriumInterestform controls are properly initialized with default values of 0, which aligns with the PR objective of making Moratorium fields configurable.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (2)
10-10: Excellent accessibility improvements with ARIA labels.The addition of
aria-labelattributes to form inputs and buttons throughout the template significantly improves screen reader support and WCAG compliance.Also applies to: 21-21, 44-44, 66-66, 81-81, 91-91, 103-103, 118-118, 140-140, 196-196, 243-247, 365-365, 375-375, 386-386, 391-391, 396-396, 545-545, 582-582, 694-694, 708-708, 712-712, 716-716
542-542: Good practice: Explicit button types prevent unintended form submission.Adding
type="button"to action buttons ensures they don't trigger form submission, which is the correct approach for buttons that perform navigation or other non-submit actions.Also applies to: 579-579, 660-660, 690-695, 708-708, 712-712, 716-716
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
...loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
218-227: Remove redundantisParentAccountproperty from the client object.On line 223, the client object includes
isParentAccount: true, but this property is not used withinsetData. The method derivesdata.isParentAccountfrom theisParentparameter (line 203). The property in the client object is redundant.Apply this diff to remove the redundant property:
body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true )
🧹 Nitpick comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
180-180: Consider strengthening type safety.The method signature uses
anyfor the client parameter and return type. Defining proper interfaces would improve type safety and code maintainability.Consider applying this pattern:
interface GLIMAccountClient { id: number; principal: number; } setData(client: GLIMAccountClient, totalLoan: number, isParent?: boolean): string { // ... method body }
233-233: ThetotalLoanparameter is unused for child accounts.When
isParentisfalse, thetotalLoanparameter passed tosetDatais not used (see lines 201-204). While not causing incorrect behavior, passing unused parameters can be confusing. Consider whether the method signature could be improved to make this relationship clearer, or document why it's passed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(2 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/login/login-form/login-form.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧬 Code graph analysis (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
70f0400 to
776d91a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
218-227: Remove redundantisParentAccountproperty from the client object.On line 223, the client object includes
isParentAccount: true, but this property is not used withinsetData. Thedata.isParentAccountis set based on theisParentparameter (line 203). The property in the client object is redundant.Apply this diff to remove the redundant property:
body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true )
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
697-701: Remove redundant explicit field assignment in getter.The explicit inclusion of
moratoriumPrincipalandmoratoriumInterestis redundant. Since these fields are already defined as form controls (lines 521-522),getRawValue()will automatically include them in the returned object.Apply this diff to simplify the getter:
get loansAccountTerms() { - return { - ...this.loansAccountTermsForm.getRawValue(), - moratoriumPrincipal: this.loansAccountTermsForm.get('moratoriumPrincipal')?.value, - moratoriumInterest: this.loansAccountTermsForm.get('moratoriumInterest')?.value - }; + return this.loansAccountTermsForm.getRawValue(); }src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-211: Improve type safety for thesetDatamethod.The method signature uses
anytypes for both theclientparameter and return type, which reduces type safety. Consider defining a proper interface for the client parameter and declaring the return type asstring(since it returnsJSON.stringify(data)).Additionally, lines 197-199 could use object property shorthand notation for cleaner code.
Apply this diff to improve type safety:
-setData(client: any, totalLoan: number, isParent?: boolean): any { +setData(client: { id: number; principal: number }, totalLoan: number, isParent?: boolean): string { const locale = this.settingsService.language.code; const dateFormat = this.settingsService.dateFormat;For cleaner code, consider using property shorthand:
- dateFormat: dateFormat, - locale: locale, + dateFormat, + locale,Based on coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(5 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(2 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧬 Code graph analysis (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🔇 Additional comments (4)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (2)
42-104: LGTM! Excellent accessibility improvements.The addition of explicit
type="button"attributes andaria-labeldescriptors on all action buttons follows accessibility best practices and prevents unintended form submissions.
131-142: editOverdueChargeAmount handler is properly implemented.Verification confirms the method exists at line 329 and includes complete implementation: it opens a dialog for editing the charge amount, handles the user response, updates the overDueChargesDataSource correctly, and sets the pristine flag to false. The button template structure with accessibility attributes aligns with the implementation.
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
520-522: Template bindings for new moratorium fields are correctly in place.The verification confirms that
formControlNamebindings exist in the template file at lines 7 and 18, corresponding to themoratoriumPrincipalandmoratoriumInterestform controls added at lines 520-522. Angular'sgetRawValue()returns all control values, regardless of disabled status, so these form controls will be properly included in form value operations. The code changes align with the PR objective.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
228-236: Request ID logic looks correct.The implementation properly assigns sequential request IDs starting from '1' for child requests (parent is '0'). Each child correctly passes
isParent: falsetosetData.Note: The
totalLoanparameter passed tosetDataon line 233 is not used for child requests (only used whenisParentis true). While this keeps the method signature consistent, consider documenting this behavior if it's intentional.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
776d91a to
686ba98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
248-253: Good fix for array mutation, but type safety still needs improvement.The method now correctly uses
forEachinstead ofpop(), avoiding mutation of the errors array. However, both parameters still useanytypes, reducing type safety.Consider defining proper interfaces for the parameters:
interface ErrorResponse { defaultUserMessage: string; errors?: Array<{ developerMessage: string }>; } notify(body: ErrorResponse, data: unknown): void { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }Based on coding guidelines.
218-227: Remove redundantisParentAccountproperty from the client object.The client object on line 223 includes
isParentAccount: true, but this property is not used bysetData. Instead,data.isParentAccountis set based on theisParentparameter (line 203). The property in the client object is redundant.Apply this diff to remove the redundant property:
requestData.push({ requestId: '0', method: 'POST', relativeUrl: 'loans', body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true ) });
🧹 Nitpick comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-211: Improve type safety by defining interfaces for client and return type.The method uses
anytypes for theclientparameter anddatavariable, reducing type safety. Consider defining proper interfaces to ensure type correctness and improve maintainability.For example, define interfaces like:
interface GLIMClient { id: string | number; principal: number; } interface GLIMRequestData { clientId: string | number; principal: number; loanType: string; charges: Array<{ chargeId: string | number; amount: number }>; dateFormat: string; locale: string; groupId: string | number; totalLoan?: number; isParentAccount?: boolean; // ... other properties }Then update the method signature:
-setData(client: any, totalLoan: number, isParent?: boolean): any { +setData(client: GLIMClient, totalLoan: number, isParent?: boolean): string {Based on coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(5 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(2 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.tssrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧬 Code graph analysis (2)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (2)
src/app/shared/form-dialog/formfield/model/formfield-base.ts (1)
FormfieldBase(3-38)src/app/shared/form-dialog/formfield/model/input-base.ts (1)
InputBase(17-25)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🔇 Additional comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
228-236: LGTM!The child request construction correctly iterates through members, assigns sequential requestIds starting at 1, and passes
falsefor theisParentparameter to ensure child-specific payload structure.src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
97-102: The review comment is incorrect.The Edit button for overdue charges is already present in the template, embedded within the "amount" column definition with the pen icon and
editOverdueChargeAmount(charge)handler. TheoverdueChargesDisplayedColumnsarray correctly lists all displayed columns:['name', 'type', 'amount', 'collectedon']. No separate 'action' column is needed.Likely an incorrect or invalid review comment.
...ans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
686ba98 to
87b9c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
248-253: Good fix for array mutation, but type safety still needs improvement.The mutation issue has been resolved by using
forEachinstead ofpop()- well done! However, both parameters still useanytypes, which reduces type safety as noted in previous reviews.Based on coding guidelines.
Consider defining proper interfaces for type safety:
interface ErrorResponse { defaultUserMessage: string; errors?: Array<{ developerMessage: string }>; } notify(body: ErrorResponse, data: any) { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }
218-227: Remove redundantisParentAccountproperty from the client object.The client object on line 223 includes
isParentAccount: true, but this property is not used withinsetData. The method derivesisParentAccountfrom theisParentparameter (line 225 passestrue). The property in the client object is redundant.Apply this diff to remove the redundant property:
requestData.push({ requestId: '0', method: 'POST', relativeUrl: 'loans', body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true ) });
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (1)
528-534: Minor inconsistency: Missing aria-label on Maximum Outstanding Balance field.This field uses a
titleattribute but lacks anaria-labelfor consistency with other form controls throughout the template (e.g., lines 44, 66, 81, etc.). Consider adding anaria-label="Maximum allowed outstanding balance"attribute for better screen reader support.<mat-form-field class="flex-48"> <mat-label>{{ 'labels.inputs.Maximum allowed outstanding balance' | translate }}</mat-label> <input matInput type="number" formControlName="maxOutstandingLoanBalance" placeholder="Enter maximum allowed outstanding balance" title="Maximum allowed outstanding balance" + aria-label="Maximum allowed outstanding balance" />src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-180: Improve type safety for method parameters and local variables.The
clientparameter anddatavariable useanytypes, which reduces type safety. Consider defining proper interfaces for the client object and the loan data structure to improve type safety and catch potential errors at compile time.Based on coding guidelines.
For example, define interfaces like:
interface GLIMClient { id: number; principal: number; } interface GLIMLoanData { clientId: number; loanType: string; principal: number; totalLoan?: number; isParentAccount?: boolean; groupId: number; dateFormat: string; locale: string; // ... other properties }Then update the signature:
-setData(client: any, totalLoan: number, isParent?: boolean): any { +setData(client: GLIMClient, totalLoan: number, isParent?: boolean): string { const locale = this.settingsService.language.code; const dateFormat = this.settingsService.dateFormat; - const data: any = { + const data: GLIMLoanData = {Also applies to: 184-184
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(5 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(2 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
🧬 Code graph analysis (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
🔇 Additional comments (9)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (2)
1-25: ✅ Moratorium fields correctly placed inside form tag.The moratorium input fields are now properly nested inside the
<form [formGroup]="loansAccountTermsForm">element at lines 2-25, resolving the critical issue flagged in the previous review. Form control bindings (moratoriumPrincipalandmoratoriumInterest) are correct, and the inputs are properly typed and labeled.
2-25: ✅ Comprehensive accessibility improvements with aria-labels and explicit button types.The changes add extensive ARIA accessibility attributes (
aria-label) to form inputs and set explicittype="button"on all interactive buttons (e.g., Previous, Next, Cancel, Add/Delete actions). This prevents unintended form submissions and improves screen reader clarity across the form. The improvements are applied consistently throughout the template.Also applies to: 44-44, 66-66, 81-81, 91-91, 103-103, 118-118, 140-140, 196-196, 243-248, 365-365, 375-375, 386-386, 391-391, 396-396, 545-545, 582-582, 660-660, 690-698, 708-708, 712-712, 716-716
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (5)
42-50: Edit charge button properly attributes and accessible.type="button" prevents accidental form submission, and aria-label improves accessibility for screen readers. Aligns with template improvements.
84-98: Edit charge date button properly attributes and accessible.type="button" and aria-label correctly applied following the same pattern as the edit amount button.
104-107: Delete button properly secured with type and aria-label.type="button" prevents accidental form submission, and aria-label clarifies action for accessibility.
156-156: Stepper and navigation buttons properly typed to prevent form submission.type="button" correctly added to Previous (line 156), Next (line 160), and Cancel (line 164) buttons to ensure they do not trigger form submission when not intended.
Also applies to: 160-160, 164-164
131-142: Verification complete: Both required dependencies are properly implemented.The
editOverdueChargeAmount(charge)method exists at line 329 in the component, and theformatNumberpipe is properly imported fromsrc/app/pipes/format-number.pipe.ts(line 37). The component is a standalone Angular component with both dependencies correctly included in its imports array. The code changes are valid and follow expected patterns.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
201-204: LGTM! Conditional parent-specific fields are correctly implemented.The logic correctly adds
totalLoanandisParentAccountonly when theisParentflag is true, ensuring proper parent/child payload structure for the GLIM account creation.
228-235: LGTM! Child request construction is correct.The loop correctly generates child requests with sequential request IDs and passes
falsefor theisParentparameter, ensuring each member gets their own loan record with the correct payload structure.
|
Hi @JaySoni1 , I'm very surprised that you were even able to test your changes - have you tested them? I downloaded your PR and tried to test it. However, when I chose "Applications" - "GLIM Application" from the hamburger menu, I got a Javascript error in the console:
Only after making changes in i18n.service.ts, I was able to open the form for GLIM application. Have you not met this problem? When I finally get to the "Active Client Members" tab/screen, I'm getting a series of Javascript errors
In the "loansAccount.charges" array, the charge seems to lack a currency, which causes the error, because the "displaySymbol" is retrieved from the currency. I'm not 100% sure this problem is related to your changes, but it looks to me as if it does. Because I'm not seeing this Javascript error when testing based on the code in the dev branch (but I see other Javascript errors). From what I can tell, it is the above Javascript error that causes the submit/next button to be disabled. I can't save a GLIM application, based on your PR. |
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
87b9c7f to
fcc3ffd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (2)
341-341: Missing null safety guard on overdue charges "type" column.Line 341 directly accesses
charge.chargeCalculationType.valuewithout optional chaining. This mirrors the exact issue reported in the PR comments—when chargeCalculationType is undefined, this will throw "Cannot read properties of undefined".This is inconsistent with the pattern applied at line 272 for regular charges.
Apply this diff to add optional chaining:
- <td mat-cell *matCellDef="let charge">{{ charge.chargeCalculationType.value }}</td> + <td mat-cell *matCellDef="let charge">{{ charge.chargeCalculationType?.value || '' }}</td>
351-351: Missing null safety guard on overdue charges "collected on" column.Line 351 directly accesses
charge.chargeTimeType.valuewithout optional chaining. This mirrors the exact issue reported in the PR comments—when chargeTimeType is undefined, this will throw "Cannot read properties of undefined".This is inconsistent with the pattern applied at line 286 for regular charges.
Apply this diff to add optional chaining:
- <td mat-cell *matCellDef="let charge">{{ charge.chargeTimeType.value }}</td> + <td mat-cell *matCellDef="let charge">{{ charge.chargeTimeType?.value || '' }}</td>
♻️ Duplicate comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
219-228: Remove redundantisParentAccountproperty from the client object.Line 224 includes
isParentAccount: truein the client object, but this property is redundant. ThesetDatamethod derivesisParentAccountfrom theisParentparameter (line 226 passestrue), so the property in the object literal is unused.Apply this diff to remove the redundant property:
requestData.push({ requestId: '0', method: 'POST', relativeUrl: 'loans', body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true ) });
249-254: Good improvement usingforEach, but type safety still needs attention.The change from
pop()toforEacheliminates array mutation, which is an improvement. However, both parameters still useanytypes, which reduces type safety as noted in previous reviews.Based on coding guidelines.
Consider defining interfaces for the parameters:
interface ErrorResponse { defaultUserMessage: string; errors?: ReadonlyArray<{ developerMessage: string }>; } notify(body: ErrorResponse, data: any): void { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }
🧹 Nitpick comments (5)
src/app/app.module.ts (1)
134-136: Provider registration fixes the DI error, but consider modern pattern.The addition of
I18nServiceto the providers array correctly resolves the NullInjectorError reported in the PR comments.However, modern Angular best practice is to use
providedIn: 'root'in the service's@Injectabledecorator rather than adding it to module providers:@Injectable({ providedIn: 'root' }) export class I18nService { // ... }This approach offers better tree-shaking and eliminates the need for explicit module provider registration.
If the service file can be modified to use
providedIn: 'root', consider removing this provider entry. Otherwise, the current solution is acceptable.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
521-522: Moratorium controls added successfully.The new
moratoriumPrincipalandmoratoriumInterestform controls align with the PR objective of making Moratorium fields configurable. The default value of 0 is appropriate.Consider adding validation constraints to ensure data integrity:
- moratoriumPrincipal: [0], - moratoriumInterest: [0] + moratoriumPrincipal: [0, [Validators.min(0)]], + moratoriumInterest: [0, [Validators.min(0)]]
697-701: Simplify getter by removing redundant field assignments.Since
moratoriumPrincipalandmoratoriumInterestare defined as form controls (lines 521-522),getRawValue()already includes them. The explicit re-assignment is unnecessary.Apply this diff to simplify:
get loansAccountTerms() { - return { - ...this.loansAccountTermsForm.getRawValue(), - moratoriumPrincipal: this.loansAccountTermsForm.get('moratoriumPrincipal')?.value, - moratoriumInterest: this.loansAccountTermsForm.get('moratoriumInterest')?.value - }; + return this.loansAccountTermsForm.getRawValue(); }src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: Consider extracting common dialog logic to reduce duplication.This method is nearly identical to
editChargeAmount(lines 212-237), differing only in the title and target data source. Consider refactoring to a helper method that accepts the data source and configuration as parameters to reduce code duplication and improve maintainability.For example:
private editAmount( charge: any, dataSource: any[], title: string, onUpdate: (dataSource: any[]) => void ) { const formfields: FormfieldBase[] = [ new InputBase({ controlName: 'amount', label: 'Amount', value: charge.amount, type: 'number', required: false }) ]; const data = { title: title, layout: { addButtonText: 'Confirm' }, formfields: formfields }; const editNoteDialogRef = this.dialog.open(FormDialogComponent, { data }); editNoteDialogRef.afterClosed().subscribe((response?: { data?: { value: { amount: number } } }) => { if (response?.data) { const newCharge = { ...charge, amount: response.data.value.amount }; dataSource.splice(dataSource.indexOf(charge), 1, newCharge); onUpdate(dataSource.concat([])); this.pristine = false; } }); }Then call it as:
editOverdueChargeAmount(charge: any) { this.editAmount( charge, this.overDueChargesDataSource, 'Edit Overdue Charge Amount', (updated) => this.overDueChargesDataSource = updated ); }src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-180: Improve type safety for method parameters and return type.The method uses
anyforclientand the return type, reducing type safety as required by coding guidelines.Based on coding guidelines.
Consider defining interfaces for the parameters:
interface GLIMClient { id: number; principal: number; isParentAccount?: boolean; } setData(client: GLIMClient, totalLoan: number, isParent?: boolean): string { // ... method body }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/app.module.ts(2 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(7 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html(3 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(3 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/login/login-form/login-form.component.tssrc/app/app.module.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.tssrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧬 Code graph analysis (2)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (2)
src/app/shared/form-dialog/formfield/model/formfield-base.ts (1)
FormfieldBase(3-38)src/app/shared/form-dialog/formfield/model/input-base.ts (1)
InputBase(17-25)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (7)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (2)
265-265: Defensive null checks applied correctly for currency access.The conditional rendering
*ngIf="charge.currency"guards against undefined currency and prevents the displaySymbol TypeError. This aligns with the fix for the reported issue.Also applies to: 335-335
272-272: Optional chaining and fallbacks used defensively.The use of optional chaining (
?.value || '') and enhanced date column conditions provides better null safety. These changes prevent potential runtime errors.Also applies to: 286-286, 294-310
src/app/login/login-form/login-form.component.ts (1)
13-13: LGTM: Correct Material module import for standalone component.The change from
MatCheckboxtoMatCheckboxModuleis correct for Angular standalone components. This follows the proper pattern where standalone components should import modules rather than individual components.While this change is unrelated to the GLIM loan application fixes in the PR objectives, you've explained it resolved an Angular Material import error. For future reference, consider separating unrelated fixes into separate PRs for clearer change tracking.
Also applies to: 34-34
src/app/app.module.ts (1)
4-4: LGTM: I18nService import added correctly.This import addresses the NullInjectorError mentioned in the PR review comments by making I18nService available for dependency injection.
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
361-361: Good defensive coding with safe fallback.The nullish coalescing operator
?? 0combined with safe navigation?.properly handles cases whereloansAccountTermsDataormaxOutstandingLoanBalancemight be undefined, preventing potential runtime errors.Also applies to: 366-366
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
229-236: LGTM! Request ID shifting is correct.The logic correctly shifts child request IDs to start from '1' (after the parent '0'), and properly passes
falsefor theisParentparameter to distinguish child loans from the parent entry.
209-210: Remove unconditional deletion of moratorium fields, or clarify API constraints.The code collects user-editable moratorium field values in the form (with UI controls), explicitly extracts them, spreads them into the payload, then unconditionally deletes them before sending to the API. This prevents these fields from reaching the API regardless of user input, which contradicts the stated objective of making them "configurable and editable in the UI and API payload."
The deletion was explicitly added in this commit and appears intentional (replacing an old TODO comment about these fields being "unsupported for GLIM"). However, the approach needs clarification:
- If GLIM API does not support moratorium fields, remove the form controls and form fields entirely instead of collecting and discarding them.
- If GLIM API should support these fields when provided, remove the delete statements (lines 209–210).
- If these fields should only be sent conditionally (e.g., only when non-zero), replace the unconditional deletion with conditional logic.
delete data.principalAmount; delete data.allowPartialPeriodInterestCalculation; delete data.multiDisburseLoan; delete data.isFloatingInterestRate; -delete data.moratoriumPrincipal; -delete data.moratoriumInterest; return JSON.stringify(data);Alternatively, if these fields are intentionally unsupported in GLIM, remove the form controls from the loans-account-terms-step template and form group to avoid collecting user input that will be discarded.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
...ans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
fcc3ffd to
75213a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
220-229: Remove redundantisParentAccountproperty from the client object.The
isParentAccount: trueproperty in the client object on line 225 is redundant. ThesetDatamethod derivesisParentAccountfrom the third parameterisParent(line 227), making the property in the client object unnecessary.Apply this diff to remove the redundant property:
body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true )
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
361-361: Apply the same nullish coalescing pattern to ngOnChanges for consistency.The nullish coalescing operator improves safety here. However, lines 294 and 299 in
ngOnChanges()set the same control without this pattern, which could lead to inconsistent behavior depending on which lifecycle method executes.Apply this pattern to lines 294 and 299 as well:
this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', - new UntypedFormControl(this.loansAccountTermsData.maxOutstandingLoanBalance, Validators.required) + new UntypedFormControl(this.loansAccountTermsData?.maxOutstandingLoanBalance ?? 0, Validators.required) ); } else { this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', - new UntypedFormControl(this.loansAccountTermsData.maxOutstandingLoanBalance) + new UntypedFormControl(this.loansAccountTermsData?.maxOutstandingLoanBalance ?? 0) );Also applies to: 366-366
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
250-255: Good fix for mutation, but type safety remains a concern.The change from
pop()toforEach()eliminates the array mutation issue. However, both parameters still useanytypes, reducing type safety as required by the coding guidelines for Angular code.Consider defining proper interfaces for the parameters:
interface ErrorBody { defaultUserMessage: string; errors?: ReadonlyArray<{ developerMessage: string }>; } notify(body: ErrorBody, data: unknown): void { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error) => message += error.developerMessage + ' '); message += 'Data: ' + JSON.stringify(data); console.error(message); }Based on coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/app.module.ts(2 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(7 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html(3 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(3 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/app.module.ts
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
🧬 Code graph analysis (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (3)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
186-190: Critical fix: Currency field added to charges mapping.This addition directly resolves the TypeError reported by the reviewer: "Cannot read properties of undefined (reading 'displaySymbol')". The currency field is required by the preview component to properly render charge information. This fix unblocks the preview rendering and enables the submit button.
202-205: LGTM! Proper conditional parent payload structure.The conditional logic correctly adds
totalLoanandisParentAccountonly to parent requests, ensuring proper parent/child payload differentiation as per the PR objectives.
210-211: Verify whether moratorium fields should be supported in GLIM loans.The code unconditionally deletes
moratoriumPrincipalandmoratoriumInterestas part of GLIM-specific payload processing. This appears intentional—the deletion is grouped with other GLIM-incompatible fields and there's no conditional logic based on form values. However, the PR objectives are ambiguous about whether moratorium field configurability applies to GLIM loans specifically or only standard loans.Please confirm from the PR description/objectives:
- Should GLIM loans support moratorium fields in the UI and API?
- If no, the deletion is correct and this review comment is invalid.
- If yes, the deletion should be removed or made conditional.
...p/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
Show resolved
Hide resolved
...p/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
75213a2 to
b53e2f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (1)
341-341: CRITICAL: Missing defensive guards in Overdue Charges — direct access to nested properties will fail if undefined.Lines 341 and 351 access
charge.chargeCalculationType.valueandcharge.chargeTimeType.valuewithout optional chaining. This mirrors the exact error the PR aims to fix (undefined property access on chargeCalculationType/chargeTimeType). The main Charges section correctly uses optional chaining (lines 272, 286), but the Overdue Charges section does not.Per the AI summary, these columns should have been updated, yet they lack the defensive syntax applied elsewhere. This inconsistency will cause the same TypeError if overdue charges data is missing these nested objects.
Apply this diff to match the pattern used in the main Charges section:
<ng-container matColumnDef="type"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Type' | translate }}</th> - <td mat-cell *matCellDef="let charge">{{ charge.chargeCalculationType.value }}</td> + <td mat-cell *matCellDef="let charge">{{ charge.chargeCalculationType?.value || '' }}</td> </ng-container> <ng-container matColumnDef="collectedon"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Collected On' | translate }}</th> - <td mat-cell *matCellDef="let charge">{{ charge.chargeTimeType.value }}</td> + <td mat-cell *matCellDef="let charge">{{ charge.chargeTimeType?.value || '' }}</td> </ng-container>Also applies to: 351-351
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
322-357: Critical: Form is recreated after patching values, discarding all changes.Lines 322-355 call
patchValueto populate the form, but line 357 immediately recreates the form by callingcreateloansAccountTermsForm(), which discards all the patched values. The form is already created in the constructor (line 172), so this second creation is unnecessary and destructive.This means all the moratorium fields and other patched values are lost, which could explain data integrity issues and contribute to the runtime errors reported by the reviewer.
Apply this diff to remove the redundant form recreation:
interestRecognitionOnDisbursementDate: this.loansAccountTermsData.interestRecognitionOnDisbursementDate || false, moratoriumPrincipal: this.loansAccountTermsData.moratoriumPrincipal ?? 0, moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 }); } - this.createloansAccountTermsForm(); this.setAdvancedPaymentStrategyControls();
179-179: Major: Unsafe currency assignment causes TypeError reported by reviewer.The reviewer reports a TypeError: "Cannot read properties of undefined (reading 'displaySymbol')" that blocks GLIM application submission. This line assigns
loansAccountProductTemplate.currencytothis.currencywithout checking for null/undefined. When the currency is missing from the product template or charges data, downstream template code that accessescurrency.displaySymbolfails.Apply this diff to safely initialize the currency:
if (this.loansAccountProductTemplate) { - this.currency = this.loansAccountProductTemplate.currency; + this.currency = this.loansAccountProductTemplate.currency ?? this.loansAccountProductTemplate.currency;Wait, that's not helpful. Better approach:
if (this.loansAccountProductTemplate) { - this.currency = this.loansAccountProductTemplate.currency; + if (this.loansAccountProductTemplate.currency) { + this.currency = this.loansAccountProductTemplate.currency; + }Additionally, update the type declaration at line 153 to reflect that currency can be undefined:
- currency: Currency; + currency: Currency | undefined;And ensure all template references to
currencyuse optional chaining (e.g.,currency?.displaySymbol).
♻️ Duplicate comments (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
250-255: Improve type safety for method parameters.While the mutation issue has been addressed by using
forEach()instead ofpop(), the type safety concern from the previous review remains. Both parameters still useanytypes, which violates the coding guidelines requirement for strict type safety in Angular code.Consider defining proper interfaces for the parameters:
interface ErrorBody { defaultUserMessage: string; errors?: ReadonlyArray<{ developerMessage: string }>; } notify(body: ErrorBody, data: unknown): void { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }Based on coding guidelines.
220-229: Remove redundantisParentAccountproperty from the client object.The client object on line 225 includes
isParentAccount: true, but this property is not used bysetData. Instead,data.isParentAccountis set based on theisParentparameter (line 204). The property in the client object is redundant and should be removed.Apply this diff to remove the redundant property:
requestData.push({ requestId: '0', method: 'POST', relativeUrl: 'loans', body: this.setData( - { id: this.loansAccountTemplate.group.id, principal: totalLoan, isParentAccount: true }, + { id: this.loansAccountTemplate.group.id, principal: totalLoan }, totalLoan, true ) });
🧹 Nitpick comments (3)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
121-124: Minor: Inconsistent spacing after comma in currency display.Line 122 differs from line 27 in spacing: line 27 uses
", "after the charge name, while line 122 uses","without a space. For consistency, add a space after the comma on line 122.- {{ charge.name }}<span *ngIf="charge.currency">,{{ charge.currency.displaySymbol }}</span> + {{ charge.name }}<span *ngIf="charge.currency">, {{ charge.currency.displaySymbol }}</span>src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-213: Improve type safety and parameter design.The method has type safety concerns per coding guidelines:
- Type safety: The
clientparameter usesanytype, reducing type safety. Consider defining a proper interface.- Parameter design: The
totalLoanparameter is required but only used whenisParentis true (line 203). Consider making it optional to reflect its conditional usage.Consider defining an interface and updating the signature:
interface GLIMClient { id: string | number; principal: number; } setData(client: GLIMClient, totalLoan?: number, isParent?: boolean): string { // ... existing code ... if (isParent && totalLoan !== undefined) { data.totalLoan = totalLoan; data.isParentAccount = true; } // ... rest of method }Based on coding guidelines.
src/app/app.module.ts (1)
4-4: I need to verify the full context of AppModule to understand the complete picture:Approved: Fix correctly resolves the NullInjectorError for I18nService.
The change successfully fixes the critical runtime error reported by adding I18nService to the providers array, making it available for dependency injection throughout the application. No duplicate providers exist—CoreModule does not provide this service.
However, consider updating the service definition for improved maintainability. Modern Angular best practice uses @Injectable({providedIn: 'root'}) which enables tree-shaking for unused services, while module providers cause services to be included in the app even if unused.
Optional recommendation: Update
src/app/core/i18n/i18n.service.tsfrom@Injectable()to@Injectable({providedIn: 'root'}), then remove I18nService from AppModule providers. This provides the same functionality with better bundle optimization and aligns with Angular 6+ standards.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/app.module.ts(2 hunks)src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts(3 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html(7 hunks)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts(1 hunks)src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html(3 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(19 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(4 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/login/login-form/login-form.component.ts
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/app.module.tssrc/app/loans/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html
🧬 Code graph analysis (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (9)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (2)
131-145: Verify the new editOverdueChargeAmount method is implemented in the component TypeScript.Line 138 calls
editOverdueChargeAmount(charge), which is new functionality referenced in the PR. Ensure this method exists in the component's TypeScript file and properly handles the dialog/update flow for overdue charge amounts. Additionally, note the inconsistency: line 134 formats the overdue charge amount using theformatNumberfilter, while regular charges on line 41 do not apply this filter. Verify this difference is intentional.
27-27: Conditional currency rendering protects against undefined, but verify data source consistency.Both lines 27 and 122 defensively guard access to
charge.currency.displaySymbolwith*ngIf="charge.currency". However, the PR objectives report a TypeError inloans-account-preview-step.component.htmlthat references the same displaySymbol access on charges lacking a currency. Ensure that:
- The charge data source consistently initializes the
currencyfield (or leaves it undefined/null, not as a partially-defined object).- If some charges genuinely lack currency information, the template guards are sufficient; if not, investigate why the data source is populating charges inconsistently.
Also applies to: 122-122
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (3)
265-267: ✓ Good: Defensive guard for currency property.The
*ngIf="charge.currency"guard prevents the TypeError mentioned in the PR objectives. This pattern correctly protects against undefined currency.
272-272: ✓ Good: Consistent optional chaining for charge properties.Lines 272, 286, and the conditional logic in 294–309 properly use optional chaining (
?.value) with fallback values (|| '') to guard against undefined nested properties in the main Charges section.Also applies to: 286-286, 294-309
334-336: ✓ Good: Currency guard applied consistently across sections.The Overdue Charges section's name column now also guards against undefined currency using
*ngIf="charge.currency", matching the pattern in the main Charges section.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (4)
222-226: Moratorium fields correctly initialized in ngOnChanges.The moratorium fields are properly populated from
loansAccountTermsDatausing nullish coalescing to provide safe defaults. This addresses the previous review comment about preventing data loss when editing existing loans with moratorium values.Note: This approval assumes the critical issue with form recreation at line 357 (flagged separately) is fixed; otherwise, these patched values will be discarded.
351-355: Moratorium fields correctly initialized in ngOnInit (but see critical issue).The initialization pattern is correct and matches the implementation in
ngOnChanges. However, these values will be discarded by thecreateloansAccountTermsForm()call at line 357 (flagged separately as a critical issue).
367-367: Good defensive programming for maxOutstandingLoanBalance.The use of optional chaining and nullish coalescing provides safe initialization that prevents runtime errors when
loansAccountTermsDataor its properties are undefined.Also applies to: 372-372
526-528: Form controls properly defined for new fields.The moratorium and interest recognition controls are correctly added to the form definition with appropriate default values. The defaults (0 for moratorium fields, false for interest recognition) align with the initialization logic in
ngOnChangesandngOnInit.
|
Hi @steinwinde, Thank you for your feedback. I have now addressed the issues you mentioned. Yes, I did test the GLIM application flow earlier and did not encounter the erros at that time. However, after your review, I re-tested the code and was able to find the errors . I have now resolved the errors , please review . |
There was a problem hiding this comment.
@JaySoni1 , have you discussed this with @bharathcgowda or @IOhacker or someone competent? E.g. this adds two new, optional input fields on top of the Terms page - a position where we had the required "Principal" field before. Why do you think these two new input fields make sense? If you change significant parts of the application, please point out in the Jira ticket why. From my point of view these fields should rather appear under the Moratorium section (and their labels need translations!), but I don't even see why we need these fields.
In addition to that, please don't add English texts literally into the markup, e.g. aria-label="Edit charge amount".
|
@JaySoni1 I did some digging into the GLIM application process on the backend. My understanding is as follows:
All of this is reflected in the following code change in setData(applicationId: number, client: any, totalLoan: number, isFirst: boolean, isLast: boolean): any {
const locale = this.settingsService.language.code;
const dateFormat = this.settingsService.dateFormat;
// const monthDayFormat = 'dd MMMM';
const data = {
...this.loansAccount,
charges: this.loansAccount.charges.map((charge: any) => ({
chargeId: charge.id,
amount: charge.amount
})),
clientId: client.id,
totalLoan: totalLoan,
loanType: 'glim',
applicationId: applicationId,
lastApplication: isLast,
amortizationType: 1,
principal: client.principal,
syncDisbursementWithMeeting: false,
expectedDisbursementDate: this.dateUtils.formatDate(this.loansAccount.expectedDisbursementDate, dateFormat),
submittedOnDate: this.dateUtils.formatDate(this.loansAccount.submittedOnDate, dateFormat),
dateFormat,
// monthDayFormat,
locale
};
data.groupId = this.loansAccountTemplate.group.id;
if (isFirst) {
// Note: Only add isParentAccount = true for the first request.
// Setting isParentAccount = false for subsequent requests is not handled correctly by Fineract.
data.isParentAccount = true;
}
delete data.principalAmount;
// TODO: 2025-03-17: Apparently (?!) unsupported for GLIM
delete data.allowPartialPeriodInterestCalculation;
delete data.multiDisburseLoan;
delete data.isFloatingInterestRate;
return JSON.stringify(data);
}
/** Request Body Data */
buildRequestData(): any[] {
const requestData = [];
const memberSelected = this.selectedMembers?.selectedMembers ?? [];
const totalLoan = this.totalLoanAmount();
// Create a random 10-digit applicationId for the GLIM
const applicationId = Math.floor(1000000000 + Math.random() * 9000000000);
for (let index = 0; index < memberSelected.length; index++) {
const isFirst = index === 0;
const isLast = index === memberSelected.length - 1;
requestData.push({
requestId: index,
reference: index === 0 ? null : index - 1, // Make sure requests are executed in order
method: 'POST',
relativeUrl: 'loans',
body: this.setData(applicationId, memberSelected[index], totalLoan, isFirst, isLast)
});
}
return requestData;
}This only changes about a dozen lines from the Please let me know if you have a different understanding of the process or API. |
4 participants
Changes Made :-
-Fixed GLIM loan application total calculation and ensured correct parent/child payload structure.
-Made Moratorium and Overdue Charges fields configurable and editable in the UI and API payload.
WEB :- 95
Summary by CodeRabbit
New Features
Accessibility
Usability & Fixes