WEB-95 Fix problems with GLIM application: saved total, "Moratorium", "Overdue Charges"" #2717
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
GLIM Account Creationsrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts |
setData() signature changed to setData(applicationId, client, totalLoan, isFirst, isLast); adds applicationId/groupId, conditional isParentAccount/lastApplication; charges simplified to { chargeId, amount, currency }; removes moratorium/deprecated fields; submit() removed; buildRequestData() now generates applicationId, computes first/last flags and calls setData(...) with reference; notify() signature changed to notify(body, data) and appends serialized data to error messages. |
Loan Charges Management (TS + Template)src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts,src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html |
Template: buttons updated with type="button" and aria-label, currency displayed conditionally; Cancel button rendering guarded by *ngIf="loanId" (merge-conflict resolved to use button type). TS: added editOverdueChargeAmount(charge) opening a FormDialog to edit amount, replaces matching overdue charge in overDueChargesDataSource, reassigns array and marks form non-pristine. |
Loan Terms Accessibilitysrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html |
Added aria-label to many inputs and explicit type="button" on action controls (add/remove/stepper); no logic changes. |
Loan Preview Renderingsrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html |
Replaced charge.chargeTimeType.value usages with optional chaining charge.chargeTimeType?.value and guarded date-related checks. |
Authentication & Coresrc/app/login/login-form/login-form.component.ts,src/app/app.module.ts |
Added MatIconButton and MatCheckboxModule to login component imports; registered I18nService in AppModule providers. |
Sequence Diagram(s)
(omitted — changes do not introduce a multi-component sequential flow requiring a diagram)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- WEB-117 Loan Account Charge(Amount% charges) Doubling on Application Modification #2690 — Similar changes to how charge objects are constructed and normalized in loan request payloads.
- WEB-437-fix(charges): resolve chargeId undefined issue breaking disbursement date #2822 — Overlapping modifications to charges mapping and charge ID handling in CreateGlimAccountComponent.
- WEB-113 GLIM Application Fails to Submit #2662 — Directly related edits to CreateGlimAccountComponent request-building, notify signature, and submit flow.
Suggested reviewers
- IOhacker
- alberto-art3ch
- steinwinde
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main changes: fixing GLIM application issues including saved total calculation, Moratorium field handling, and Overdue Charges functionality. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
![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
Outdated
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.
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.
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Please review my comment
|
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
Outdated
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. |
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
b53e2f6 to
33deaa7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
5-5: *Add trackBy function to ngFor for performance.The
*ngForloop on line 5 lacks a trackBy function, which can harm performance for larger datasets. Per Angular best practices and coding guidelines, trackBy should be used to help Angular identify which items have changed.Verify that a trackBy function is defined in the component (e.g.,
trackByChargeId) and apply it:- <mat-option *ngFor="let charge of chargeData" [value]="charge"> + <mat-option *ngFor="let charge of chargeData; trackBy: trackByChargeId" [value]="charge">If a trackBy function does not exist, create one in the component TypeScript file:
trackByChargeId(index: number, charge: any): any { return charge.id || index; }src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
180-180: Improve type safety for theclientparameter.The
clientparameter usesany, reducing type safety as required by coding guidelines. Consider defining an interface for the client object with properties likeidandprincipal.Based on coding guidelines.
224-224: Consider backend-generated applicationId for production safety.The random 10-digit
applicationIdgeneration may have collision risks in high-volume production environments. Consider whether the backend can generate and return a uniqueapplicationIdinstead, or use a more robust unique identifier generation strategy (e.g., UUID).
249-254: Improve type safety for error handling method.Both parameters use
any, reducing type safety as required by coding guidelines. Consider defining proper interfaces for thebodyparameter (e.g., withdefaultUserMessage: stringanderrors?: Array<{ developerMessage: string }>) and thedataparameter.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(4 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/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
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
- src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-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/app.module.tssrc/app/login/login-form/login-form.component.tssrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/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
🔇 Additional comments (8)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (2)
133-145: Verify editOverdueChargeAmount method exists in component.The template calls
editOverdueChargeAmount(charge)on line 138, which should handle inline editing of overdue charge amounts. Ensure this method is implemented in the component's TypeScript file with proper type safety.Please confirm that the
editOverdueChargeAmountmethod exists in the corresponding component TypeScript file and accepts the expected charge object parameter.
27-27: Excellent accessibility and button semantics improvements.The addition of explicit
type="button"attributes and descriptive aria-labels across all interactive buttons (lines 27, 42–50, 93–94, 104, 158, 162, 166) significantly improves accessibility and follows Angular best practices. The defensive guard*ngIf="charge.currency"on line 27 and line 122 also properly addresses the TypeError issue noted in PR comments regarding undefined currency symbols.Also applies to: 42-50, 93-94, 104-104, 158-158, 162-162, 166-166
src/app/app.module.ts (1)
4-4: LGTM! Fixes the reported NullInjectorError.Adding
I18nServiceto the module providers correctly resolves the NullInjectorError that prevented the GLIM form from opening, as reported in the PR comments.Also applies to: 134-136
src/app/login/login-form/login-form.component.ts (1)
13-13: LGTM! Correct Angular Material v19 module import.The change from
MatCheckboxtoMatCheckboxModuleis correct for Angular Material v19 standalone components and resolves the runtime import error mentioned by the author.Also applies to: 34-34
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (4)
186-202: LGTM! Data payload enhancements align with backend requirements.The additions of
currencyin charges mapping (line 189),applicationId(line 194),dateFormat(line 200),locale(line 201), andgroupId(line 202) correctly implement the backend GLIM requirements and fix the reported TypeError.
204-209: LGTM! Conditional flags correctly implement GLIM parent/child logic.The conditional
isParentAccountandlastApplicationflags correctly implement the backend requirement to mark the first request as parent and the last request to close the GLIM application, as described in the PR comments.
214-215: Verify moratorium deletion aligns with PR objectives.The PR description states "Made Moratorium and Overdue Charges fields configurable and editable in the UI and API payload," but these lines delete
moratoriumPrincipalandmoratoriumInterestfrom the payload entirely. Please confirm whether these fields should be conditionally included based on user input rather than deleted.
227-234: LGTM! Request chaining correctly implements GLIM ordering.The
isFirst/isLastflag calculation andreferencefield chaining (line 231) correctly implement the backend requirement to process GLIM requests sequentially, as described in rhopman's comment.
...s/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
33deaa7 to
1919aa9
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 (1)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (1)
334-357: Apply defensive coding pattern consistently to overdue charges table.The overdue charges table lacks the optional chaining and fallback guards applied to the main charges table:
- Line 341:
charge.chargeCalculationType.valueuses direct access (compare to line 272:charge.chargeCalculationType?.value || '')- Line 351:
charge.chargeTimeType.valueuses direct access (compare to line 286:charge.chargeTimeType?.value || '')- Line 335: spacing inconsistency—
,{{should be, {{(compare to line 265)Apply these fixes to prevent runtime errors if nested properties are undefined:
<ng-container matColumnDef="name"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.name' | translate }}</th> <td mat-cell *matCellDef="let charge"> - {{ charge.name }}<span *ngIf="charge.currency">,{{ charge.currency.displaySymbol }}</span> + {{ charge.name }}<span *ngIf="charge.currency">, {{ charge.currency.displaySymbol }}</span> </td> </ng-container> <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>
♻️ Duplicate comments (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
186-190: Remove currency from the batched payload to prevent Fineract errors.Including the full
currencyobject in each charge means the POST/loanspayload now contains an unsupported field. Fineract’s loan-charge schema only acceptschargeId,amount, and (optionally) date/locale metadata, so the server will respond with “Unsupported parameter: currency” and the GLIM application cannot be submitted.(fineract.apache.org) Please keep the rich charge data for the preview UI, but strip it before stringifying the request body.- charges: this.loansAccount.charges.map((charge: any) => ({ - chargeId: charge.id, - amount: charge.amount, - currency: charge.currency - })), + charges: this.loansAccount.charges.map((charge: any) => ({ + chargeId: charge.id, + amount: charge.amount + })),If the preview still needs
currency.displaySymbol, populate that on a separate view model (e.g., source it fromloansAccountChargesStep) without sending it to the API.
🧹 Nitpick comments (1)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: Consider defining a dedicated interface for overdue charges.The method correctly implements inline editing with defensive checks and proper pristine flag handling. The typing improvements from the previous review are applied.
To further strengthen type safety per the coding guidelines, consider defining a dedicated interface for overdue charges instead of using
[key: string]: any:interface OverdueCharge { amount: number; name: string; // Add other known properties }Then update the signature:
- editOverdueChargeAmount(charge: { amount: number; [key: string]: any }) { + editOverdueChargeAmount(charge: OverdueCharge) {Additionally, consider adding an error handler to the
afterClosed()subscription for more robust observable handling, though the current pattern is consistent with other edit methods in this component.As per 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(4 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- 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-terms-step/loans-account-terms-step.component.ts
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
- src/app/app.module.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/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/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 (2)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (2)
265-266: Good defensive coding: charge.currency guard prevents displaySymbol errors.The conditional wrapping and safe property access pattern fixes the reported TypeError about undefined currency.displaySymbol. This aligns with the PR objective to resolve the "Cannot read properties of undefined" error.
272-272: Consistent optional chaining and fallbacks in main charges table.The main charges table defensively uses optional chaining (
?.value) with empty string fallbacks across all columns, which prevents undefined access errors reported in the PR.Also applies to: 286-286, 294-310
|
@rhopman Thank you for your detailed summary and for reviewing the GLIM application process. -I have generated a unique 10-digit applicationId and use it for all requests in the batch, linking all child loans to the same GLIM. If you have any furthur suggestions please let me know . |
|
@steinwinde Thank you for your feedback. I have not yet discussed these with @bharathcgowda or @IOhacke but I will reach out to them to confirm the necessity and placement of the new input fields I agree with your suggestion that these fields should be moved under the Moratorium section and that their labels should use translation keys. I will also update English texts along with the above changes . |
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
8527999 to
e46e3c0
Compare
|
@JaySoni1 it is not working this is the error, please make sure it is being tested using the latest Apache Fineract: { |
|
Ok @IOhacker I will solve this and will update the PR soon. Thank you for the review |
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
e46e3c0 to
140a28d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/app/login/login-form/login-form.component.ts (1)
10-16: MatCheckbox import fix looks correct; please verify Angular Material v19 recommended import + PR scope.Switching to
MatCheckboxModuleand adding it to the componentimportsis the right direction for enabling<mat-checkbox>usage here. Also, this file change is tangential to GLIM; if possible, consider splitting it into a separate PR/commit to keep review scope tight.Angular Material 19.2.x: Is MatCheckboxModule still the recommended import for standalone components, or should MatCheckbox (standalone) be imported instead?Also applies to: 29-37
src/app/app.module.ts (1)
4-4: Remove unused I18nService import and provider registration.As noted in a previous review, if
I18nServiceusesprovidedIn: 'root', it doesn't require module-level registration. The import and provider entry are redundant.Also applies to: 134-136
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
180-180: Type safety concerns remain unresolved.The
anytypes insetData(lines 180, 184, 189) andnotify(line 249) were flagged in previous reviews but remain unaddressed. Per coding guidelines, Angular code should use strict type safety. The past review suggested defining interfaces forGLIMClient,GLIMRequestData,ErrorBody, andCurrency.As per coding guidelines.
Also applies to: 184-184, 189-189, 249-249
📜 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(18 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 (1)
- src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-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/login/login-form/login-form.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.htmlsrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.tssrc/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.tssrc/app/app.module.ts
🧠 Learnings (1)
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/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.htmlsrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.tssrc/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 (2)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (3)
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/shares/shares-account-stepper/shares-account-charges-step/shares-account-charges-step.component.ts (1)
editCharge(81-105)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
src/app/clients/create-client/create-client.component.ts (1)
client(90-103)src/app/settings/settings.service.ts (1)
dateFormat(111-113)src/app/core/logger/logger.service.ts (1)
error(100-102)
🔇 Additional comments (4)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: LGTM! Overdue charge editing implemented correctly.The
editOverdueChargeAmountmethod correctly mirrors the existing charge editing pattern and addresses previous review concerns about typing, defensive checks, and subscription cleanup.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
2-2: LGTM! Clean migration to property-based DI.The refactor from constructor-based injection to property-based
inject()is correct. The properties are initialized before the constructor runs, so accessingthis.routein the constructor body is safe.Also applies to: 165-170
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (1)
20-20: LGTM! Comprehensive accessibility improvements.The additions of
aria-labelattributes to inputs andtype="button"to buttons significantly improve accessibility and prevent unintended form submissions. The changes are consistent throughout the template.Also applies to: 42-42, 57-57, 67-67, 79-79, 94-94, 116-116, 172-172, 219-224, 341-341, 351-351, 362-362, 367-367, 372-372, 504-510, 521-521, 558-558, 636-636, 666-672, 684-684, 688-688, 692-692
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
27-27: LGTM! Consistent accessibility and UI improvements.The changes add proper button semantics (
type="button"), aria-labels for accessibility, consistent currency formatting, and support for the new overdue charge editing functionality. The implementation is clean and follows established patterns.Also applies to: 42-50, 84-97, 104-107, 121-123, 135-144, 158-169
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
140a28d to
c042ae1
Compare
...s/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
Show resolved
Hide resolved
...s/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html
Outdated
Show resolved
Hide resolved
|
@JaySoni1 prettier message |
|
Ok @IOhacker I will update the PR soon |
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
a1d1ce2 to
ef69560
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
180-216: GLIM payload shaping and moratorium cleanup look right; tighten typing onsetData.The payload composition here looks aligned with the backend expectations and PR goals:
chargesare mapped to{ chargeId, amount, currency }, and the rest ofthis.loansAccountis preserved via spread.- Shared
applicationId,clientId,totalLoan,loanType: 'glim',groupId,dateFormat, andlocaleare all included.isParentAccount(first member) andlastApplication(last member) are added conditionally, matching the described GLIM batching protocol.- Deleting
moratoriumPrincipalandmoratoriumInterestbefore stringifying should address the Fineract “parameter not supported” validation errors.Two follow‑ups:
From a typing perspective,
setDataclearly returns a JSON string, so the signature should reflect that and avoidany:setData( applicationId: number, client: /* typed client */, totalLoan: number, isFirst: boolean, isLast: boolean ): string {Likewise, replace
client: anyanddata: anywith proper interfaces for strict mode. Based on coding guidelines.Please re‑test against the latest Fineract to confirm that removing
moratoriumPrincipalandmoratoriumInterestfully resolves the earlier HTTP 400s.
219-237: VerifyselectedMembersstructure when building batched GLIM requests.
buildRequestDatanow correctly:
- Computes a single
applicationIdreused for all members.- Marks first/last members with
isFirst/isLast.- Chains
/batchesrequests viareferenceto preserve order.However, there’s a potential shape mismatch in how members are sourced:
this.selectedMembers = this.loansActiveClientMembers.selectedClientMembers; // in `loansAccount` getter const memberSelected = this.selectedMembers?.selectedMembers ?? []; // hereIf
selectedClientMembersis already the array of members,memberSelectedwill always be[], and no loan requests will be generated. If it’s an object that contains.selectedMembers, this is fine.Please double‑check the type/shape of
LoansActiveClientMembersComponent.selectedClientMembersand make these two usages consistent (either store the array directly or access its property consistently).Also, consider whether the
Math.random‑based 10‑digitapplicationIdis acceptable long‑term or if a more collision‑resistant strategy (or backend‑assigned ID) is preferred. This is low risk but has been noted before.
249-253: Type and scope thenotifycontract; avoid logging full payloads in production.
notifycurrently usesanyfor bothbodyanddataand logs the entire payload viaJSON.stringify(data):notify(body: any, data: any) { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error: any) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }To align with the strict typing guideline and keep error handling clearer:
- Define a typed error body interface (with
defaultUserMessageand optionalerrors[]).- Use a typed union for
data(e.g., GLIM request type / client type) instead ofany.- Consider trimming or guarding the logged payload (e.g., omit PII, or only log full data in non‑production builds).
Based on coding guidelines.
🧹 Nitpick comments (1)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
93-96: Add missingtype="button"to edit date button for consistency.The edit date button (line 93) lacks
type="button", whereas other edit/delete buttons in this template include it for accessibility and semantic correctness.- <button mat-icon-button color="primary" (click)="editChargeDate(charge)"> + <button mat-icon-button color="primary" (click)="editChargeDate(charge)" type="button" aria-label="Edit charge date"> <fa-icon icon="pen"></fa-icon> </button>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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(6 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(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(13 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/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
- src/app/app.module.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/login/login-form/login-form.component.tssrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
🧠 Learnings (1)
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/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.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(109-111)
🔇 Additional comments (3)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (2)
29-29: Verify currency object availability for overdue charges.Both regular and overdue charge name columns safely guard against undefined currency via
*ngIf="charge.currency". However, the PR comments indicate steinwinde encountered a TypeError (Cannot read properties of undefined (reading 'displaySymbol')) in the "Active Client Members" tab. Confirm that the API response data structure foroverDueChargesDataSourceconsistently includes thecurrencyobject, or ensure your component's data preparation logic handles missing currency objects before binding.Also applies to: 121-121
43-43: Reconcile amount formatting between regular and overdue charges.The regular charges amount (line 43) displays
{{ charge.amount }}without a pipe filter, whereas overdue charges (line 133) use{{ charge.amount | formatNumber }}. Confirm whether this is intentional or if both should apply consistent formatting.Also applies to: 133-133
src/app/login/login-form/login-form.component.ts (1)
13-37: MatCheckboxModule import wiring looks correct for standalone usage.Adding
MatCheckboxModuleand including it in the componentimportsis consistent with the existing standalone Material setup and should resolve the earlier import/runtime issue without side effects.
...s/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
ef69560 to
91ea1ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
180-184: Type safety concerns remain unresolved.The method still uses
anyfor theclientparameter, return type, and the internaldataobject. This was flagged in a previous review and violates the coding guidelines requiring strict type safety for Angular code.
224-224: ApplicationId collision risk previously flagged.The random number generation approach for
applicationIdwas noted in a previous review. Consider using a more robust approach (UUID, timestamp-based) if collision handling isn't implemented on the backend.
249-254: Type safety concerns in notify method remain unresolved.Both
bodyanddataparameters useanytype. This was flagged in a previous review with suggested interfaces (ErrorBody,GLIMRequestData).
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
5-5: Use a trackBy function for the charge list.The
@forloop usestrack chargefor identity tracking. Per Angular best practices, use a dedicated trackBy function to provide stable, unique keys based on charge ID or index for better change detection and performance.trackByChargeId(index: number, charge: any): any { return charge.id || index; }Then update the template:
- @for (charge of chargeData; track charge) { + @for (charge of chargeData; track trackByChargeId($index, charge)) {src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
249-254: Error handling lacks user-facing notification.The
notifymethod only logs errors to the console. Users won't see these errors in the UI. Consider integrating with a toast/snackbar service to display error messages to users when GLIM submission fails.// Consider adding user notification, e.g.: notify(body: any, data: any) { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error: any) => (message += error.developerMessage + ' ')); console.error('GLIM Error:', message, 'Data:', data); // Add user notification: // this.snackBar.open(body.defaultUserMessage, 'Close', { duration: 5000 }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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(6 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(1 hunks)src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html(13 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/app.module.ts
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.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-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/login/login-form/login-form.component.tssrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/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 (2)
src/app/clients/create-client/create-client.component.ts (1)
client(90-103)src/app/settings/settings.service.ts (1)
dateFormat(109-111)
🔇 Additional comments (4)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html (1)
44-50: ✓ Accessibility improvements applied correctly.Edit and delete buttons now include
type="button"and appropriatearia-labelattributes. This ensures proper button semantics and screen reader support.Also applies to: 103-103, 134-142
src/app/login/login-form/login-form.component.ts (1)
13-13: MatCheckboxModule wiring into the standalone imports looks correctImporting
MatCheckboxModuleand adding it to theimportsarray is consistent with Angular Material’s module-based usage and should resolve the checkbox provider/import error you were seeing, without impacting login form behavior.Please double-check that this matches how other Material components are imported in the project (module vs standalone component style) under
@angular/material@20.2.14, and that there are no duplicate/global imports that could be removed later for cleanliness.Also applies to: 34-35
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
226-236: LGTM! Batching logic correctly implements GLIM request chaining.The implementation correctly:
- Sets
isFirstfor the first member to triggerisParentAccount: true- Sets
isLastfor the final member to triggerlastApplication: true- Chains requests via the
referencefield (null for first, previous index for subsequent)This aligns with the backend requirements described in the PR objectives.
214-215: Good fix: Removal of unsupported moratorium parameters.The deletion of
moratoriumPrincipalandmoratoriumInterestaddresses the backend validation errors (HTTP 400) reported by the reviewer indicating these parameters are not supported by Apache Fineract.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
...s/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
91ea1ac to
92fa43d
Compare
|
@IOhacker I have udated this PR please review |
|
@JaySoni1 there is one conflict here |
|
@JaySoni1 could you fix the conflic? |
|
@IOhacker I will fix this will update you soon |
…torium", "Overdue Charges""
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
92fa43d to
0053f7d
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 (2)
src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (2)
318-323: Apply optional chaining to prevent TypeError.Line 321 accesses
charge.chargeTimeType.valuewithout optional chaining, which will throw a TypeError ifchargeTimeTypeis undefined. This inconsistency means the partial fix on lines 339-342 won't prevent runtime errors, as the table cell will fail first.🔎 Proposed fix
<ng-container matColumnDef="chargeTimeType"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Collected On' | translate }}</th> <td mat-cell *matCellDef="let charge"> - {{ charge.chargeTimeType.value }} + {{ charge.chargeTimeType?.value || ('labels.inputs.Unassigned' | translate) }} </td> </ng-container>
324-350: Incomplete fix: apply optional chaining consistently across all conditions.The optional chaining added on lines 339-342 is a step in the right direction, but lines 327 and 332 still access
charge.chargeTimeType.valuewithout protection. Since these conditions are evaluated first, a TypeError will occur before reaching the "safe" code on lines 339-342.This directly relates to the runtime error reported by steinwinde in the PR comments and will continue to block GLIM application submission when charge objects lack a
chargeTimeTypeproperty.🔎 Proposed fix
<ng-container matColumnDef="date"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Date' | translate }}</th> <td mat-cell *matCellDef="let charge"> - @if (charge.chargeTimeType.value === 'Specified due date' || charge.chargeTimeType.value === 'Weekly Fee') { + @if (charge.chargeTimeType?.value === 'Specified due date' || charge.chargeTimeType?.value === 'Weekly Fee') { <span> {{ (charge.dueDate | dateFormat) || 'Unassigned' }} </span> } - @if (charge.chargeTimeType.value === 'Monthly Fee' || charge.chargeTimeType.value === 'Annual Fee') { + @if (charge.chargeTimeType?.value === 'Monthly Fee' || charge.chargeTimeType?.value === 'Annual Fee') { <span> {{ (charge.feeOnMonthDay | dateFormat) || 'Unassigned' }} </span> } @if ( !( charge.chargeTimeType?.value === 'Monthly Fee' || charge.chargeTimeType?.value === 'Annual Fee' || charge.chargeTimeType?.value === 'Specified due date' || charge.chargeTimeType?.value === 'Weekly Fee' ) ) { <span> {{ 'labels.inputs.N/A' | translate }} </span> } </td> </ng-container>
♻️ Duplicate comments (4)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (4)
195-195: HardcodedamortizationType: 1overwrites user selection.This was flagged in a previous review. The
amortizationTypeshould be derived fromthis.loansAccount.amortizationType(which comes from the terms form) rather than hardcoded. If a default of 1 is required specifically for GLIM, add a comment explaining this constraint.
180-180: Type safety: method usesanytypes.This was flagged in previous reviews. The method signature uses
anyforclientparameter and return type, violating coding guidelines. Consider defining interfaces (GLIMClient,GLIMRequestData) when time permits.
224-224: Collision risk with randomapplicationIdgeneration.This was flagged in a previous review. Using
Math.random()has a theoretical collision risk. Consider usingcrypto.randomUUID()for stronger guarantees, or document that the backend handles collisions.
249-254: Type safety:notifymethod usesanytypes.This was flagged in a previous review. Both parameters use
anytypes. Consider defining interfaces for the error body and data parameters when addressing type safety across this component.
🧹 Nitpick comments (3)
src/app/login/login-form/login-form.component.ts (1)
12-12: Remove unusedMatButtonimport.
MatButtonis imported but never added to the component's imports array (line 31 only includesMatIconButton). Remove the unused import to keep the code clean.🔎 Proposed fix
-import { MatIconButton, MatButton } from '@angular/material/button'; +import { MatIconButton } from '@angular/material/button';src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
51-51: UnusedI18nServiceinjection.The
i18nServiceis injected but not used in this component. If this is scaffolding for future translation work (as mentioned in PR objectives), consider adding a TODO comment. Otherwise, remove it to avoid dead code.src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (1)
385-396: Consider adding explicit button types for clarity.While Material button directives handle the behavior correctly, adding explicit
type="button"attributes improves code clarity and prevents potential issues if the buttons are used within form contexts.🔎 Proposed enhancement
<div class="layout-row responsive-column align-center gap-2px margin-t"> - <button mat-raised-button matStepperPrevious> + <button type="button" mat-raised-button matStepperPrevious> <fa-icon icon="arrow-left" class="m-r-10"></fa-icon> {{ 'labels.buttons.Previous' | translate }} </button> - <button mat-raised-button [routerLink]="['../..']"> + <button type="button" mat-raised-button [routerLink]="['../..']"> {{ 'labels.buttons.Cancel' | translate }} </button> - <button mat-raised-button color="primary" (click)="submitEvent.emit()"> + <button type="button" mat-raised-button color="primary" (click)="submitEvent.emit()"> {{ 'labels.buttons.Submit' | translate }} </button> </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/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-charges-step/loans-account-charges-step.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/login/login-form/login-form.component.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- 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
- src/app/app.module.ts
- 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/login/login-form/login-form.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
🧠 Learnings (1)
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to 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(109-111)
🔇 Additional comments (4)
src/app/login/login-form/login-form.component.ts (1)
12-13: Remove unusedMatCheckboxModuleimport;MatIconButtonis necessary.The template uses
mat-icon-button(line 25-31 for the password visibility toggle), soMatIconButtonis correctly imported. However, the template does not contain anymat-checkboxelements. RemoveMatCheckboxModulefrom lines 12 and 31-32 as it is unused.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
214-215: LGTM on moratorium field cleanup.Removing
moratoriumPrincipalandmoratoriumInterestaddresses the backend validation errors (HTTP 400) reported by reviewers. These parameters are not supported by the Fineract API for GLIM loans.
226-236: LGTM on batch request construction.The batching logic correctly:
- Sets
isFirstandisLastflags to controlisParentAccountandlastApplication.- Chains requests via the
referencefield (pointing to the previous index).- Uses a shared
applicationIdacross all child loan requests.This aligns with the GLIM creation requirements described in PR comments.
240-247: LGTM ontotalLoanAmountcalculation.The null-safe access pattern with
??and straightforward iteration is clear and correct.
5 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
Bug Fixes
Accessibility
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.