WEB-95 Fix problems with GLIM application: saved total, "Moratorium", "Overdue Charges"" #2717
Conversation
|
Warning Rate limit exceeded@JaySoni1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
Note
|
| Cohort / File(s) | Summary |
|---|---|
GLIM Account Creation Refactor src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts |
Added GLIM interfaces; changed setData(applicationId, client, totalLoan, isFirst, isLast): string; expanded request payload (applicationId, currency on charges, dateFormat, locale, groupId, conditional isParentAccount, lastApplication); buildRequestData() now generates random applicationId and passes isFirst/isLast per member; removed submit(); notify(body, data) signature updated and error messages now append data payload. |
Charges Step — Template Accessibility & UI src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html |
Conditional currency span for name columns; added type="button" and aria-label to edit/delete/stepper/cancel buttons; added inline edit control for overdue charge amount. |
Charges Step — Overdue Charge Edit Logic src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts |
Added editOverdueChargeAmount(charge: { amount: number; [k: string]: any }) to open edit dialog, update overDueChargesDataSource (replace item + reassign array) and mark form non-pristine; dialog subscriptions use afterClosed().pipe(take(1)) and safe response access. |
Loan Terms Step — Template Accessibility src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html |
Added ARIA attributes and type="button" to many inputs, date controls and action buttons (add/remove disbursement, collateral, previous/next/cancel) to improve accessibility semantics. |
Loan Terms Step — Form Controls & API src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts |
Added moratoriumPrincipal and moratoriumInterest form controls (default 0) and included them in all patchValue flows; added explicit return types for allowAddDisbursementDetails(): boolean, setCustomValidators(): void, setLoanTermListener(): void; preserved conditional maxOutstandingLoanBalance handling. |
Preview Step — Template Safeguards src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html |
Adjusted charge and overdue-charge displays to use conditional currency spans and optional chaining with fallbacks for chargeCalculationType, chargeTimeType, type, and collected on fields. |
Login Form — Standalone Import src/app/login/login-form/login-form.component.ts |
Replaced MatCheckbox import with MatCheckboxModule and added it to the component's standalone imports. |
I18n Service Injection src/app/core/i18n/i18n.service.ts, src/app/web-app.component.ts |
@Injectable() → @Injectable({ providedIn: 'root' }) for I18nService; removed manual instantiation and inject i18nService via WebAppComponent constructor. |
Sequence Diagram(s)
sequenceDiagram
participant UI as CreateGlimAccountComponent
participant Builder as RequestBuilder
participant Notify as NotifyHandler
rect rgb(235,245,255)
Note over UI,Builder: New build-only GLIM flow (submit removed)
UI->>Builder: buildRequestData()
Builder->>Builder: generate random applicationId
loop per member
Builder->>UI: setData(applicationId, client, totalLoan, isFirst, isLast)
end
Builder-->>Notify: notify(body, data)
Note right of Notify: Errors now include data payload\ninstead of context object
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
- Review CreateGlimAccountComponent usages (new setData signature, removed submit, notify signature) and any cross-file callers.
- Verify random applicationId generation and any tests or integrations that expect deterministic IDs.
- Validate editOverdueChargeAmount change-detection (array reassignment) and correct dialog response handling.
- Ensure moratorium controls are consistently patched and validators preserved across lifecycle hooks.
Possibly related PRs
- WEB-373 Javascript error message opening the first step of Create Loans Account #2744 — modifies loans-account-terms-step form initialization/patching and may overlap on moratorium/maxOutstandingLoanBalance changes.
- WEB-113 GLIM Application Fails to Submit #2662 — touches CreateGlimAccountComponent and GLIM submit/notify behavior; likely to overlap on notify signature and submit flow.
Suggested reviewers
- steinwinde
- alberto-art3ch
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 PR title accurately summarizes the main changes: fixing GLIM application issues related to saved total calculation, Moratorium fields, and Overdue Charges handling. |
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
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 . |
|
@IOhacker I will resolve the conflict and will update the PR soon |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
351-357: Consider removing redundant form recreation after patching values.The
createloansAccountTermsForm()call at line 357 recreates the form immediately after patching values (lines 322-355), which would discard those patched values. Since the form is already created in the constructor (line 172) and properly patched inngOnChanges(), this recreation appears redundant.Consider removing line 357 or moving the patchValue calls to after the form recreation to avoid the wasted work:
}); } - this.createloansAccountTermsForm(); this.setAdvancedPaymentStrategyControls();Note: This is a pre-existing pattern, not introduced by your changes to add moratorium fields.
526-528: LGTM! Moratorium form controls properly defined.The moratorium fields are correctly added to the form with sensible default values. They're consistently initialized in both
ngOnChanges()andngOnInit()and will be included in the form output viagetRawValue().Optionally, consider adding validators to ensure these "months" values are non-negative integers:
-moratoriumPrincipal: [0], -moratoriumInterest: [0] +moratoriumPrincipal: [0, [Validators.min(0)]], +moratoriumInterest: [0, [Validators.min(0)]]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts(3 hunks)
🧰 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.ts
⏰ 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 (1)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
222-226: LGTM! Moratorium fields correctly initialized from data.The moratorium fields are properly initialized from
loansAccountTermsDatausing nullish coalescing to default to 0 when the values are absent. This ensures existing loan data is preserved when editing and prevents the data loss issue flagged in previous reviews.
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
d2d59a4 to
4803c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/core/http/error-handler.interceptor.ts (1)
80-83: Add the missingerror.resource.not.foundtranslation
this.translate.instant('error.resource.not.found')looks up a key that doesn’t exist in our locale files, so the alert’s heading will render the raw key instead of a human-readable string. Please add theerror.resource.not.foundentry (and its equivalents for other locales) or fall back to an existing translated label before merging.src/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.ts (1)
78-84: Running additional verification to check howaccountTypeIdis used after extraction:Pre-populate the type field from the accountType query parameter.
When users navigate from the view component to create a subledger account, the
accountTypequery parameter is passed but never applied to the form. Currently, theaccountTypeIdis extracted at line 79 but unused, leaving the type field empty instead of pre-populating it.Add to
setGLAccountForm()before thevalueChangessubscription:if (this.accountTypeId) { this.glAccountForm.patchValue({ type: this.accountTypeId }); }src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
322-357: Fix the order of form creation and patching inngOnInit.Lines 322–355 patch the form with values from
loansAccountTermsData, but Line 357 then callscreateloansAccountTermsForm(), which recreates the form with default values. This wipes out all the patched values, preventing proper initialization of the form when editing an existing loan.Reorder the operations:
if (this.loansAccountTermsData) { if (this.loansAccountTermsData.loanProductId) { let formattedDate = null; if (this.loansAccountTermsData.expectedFirstRepaymentOnDate) { const repaymentDate = new Date(this.loansAccountTermsData.expectedFirstRepaymentOnDate); formattedDate = this.formatDateToDDMMYYYY(repaymentDate); } + this.createloansAccountTermsForm(); this.loansAccountTermsForm.patchValue({ repaymentsStartingFromDate: this.loansAccountTermsData.expectedFirstRepaymentOnDate && formattedDate }); } this.loansAccountTermsForm.patchValue({ principalAmount: this.loansAccountTermsData.principal, loanTermFrequency: this.loansAccountTermsData.termFrequency, loanTermFrequencyType: this.loansAccountTermsData.termPeriodFrequencyType.id, numberOfRepayments: this.loansAccountTermsData.numberOfRepayments, repaymentEvery: this.loansAccountTermsData.repaymentEvery, repaymentFrequencyType: this.loansAccountTermsData.repaymentFrequencyType.id, amortizationType: this.loansAccountTermsData.amortizationType.id, isEqualAmortization: this.loansAccountTermsData.isEqualAmortization, interestType: this.loansAccountTermsData.interestType.id, isFloatingInterestRate: this.loansAccountTermsData.isLoanProductLinkedToFloatingRate ? false : null, interestCalculationPeriodType: this.loansAccountTermsData.interestCalculationPeriodType.id, allowPartialPeriodInterestCalculation: this.loansAccountTermsData.allowPartialPeriodInterestCalculation, inArrearsTolerance: this.loansAccountTermsData.inArrearsTolerance, graceOnPrincipalPayment: this.loansAccountTermsData.graceOnPrincipalPayment, graceOnInterestPayment: this.loansAccountTermsData.graceOnInterestPayment, graceOnArrearsAgeing: this.loansAccountTermsData.graceOnArrearsAgeing, graceOnInterestCharged: this.loansAccountTermsData.graceOnInterestCharged, fixedEmiAmount: this.loansAccountTermsData.fixedEmiAmount, maxOutstandingLoanBalance: this.loansAccountTermsData.maxOutstandingLoanBalance, transactionProcessingStrategyCode: this.loansAccountTermsData.transactionProcessingStrategyCode, interestRateDifferential: this.loansAccountTermsData.interestRateDifferential, multiDisburseLoan: this.loansAccountTermsData.multiDisburseLoan, interestRateFrequencyType: this.loansAccountTermsData.interestRateFrequencyType.id, balloonRepaymentAmount: this.loansAccountTermsData.balloonRepaymentAmount, interestRecognitionOnDisbursementDate: this.loansAccountTermsData.interestRecognitionOnDisbursementDate || false, moratoriumPrincipal: this.loansAccountTermsData.moratoriumPrincipal ?? 0, moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 }); + } else { + this.createloansAccountTermsForm(); } - this.createloansAccountTermsForm(); this.setAdvancedPaymentStrategyControls();src/app/settings/settings.service.ts (1)
120-128: Provide a concrete fallback object for language parsingPassing
undefinedintosafeParseObjectviolates itsT extends objectconstraint, so this won’t compile. Even if it did, the getter could now returnundefined, andthis.language.codedirectly below would throw at runtime. Please supply an actual fallback object that matches the expected shape (e.g., derive it fromenvironment.defaultLanguage) and reuse it whenmifosXLanguageis missing or malformed.- if (!localStorage.getItem('mifosXLanguage')) { - this.setDefaultLanguage(); - } - return safeParseObject<{ name: string; code: string } | undefined>( - localStorage.getItem('mifosXLanguage'), - undefined - ); + const defaultLocale = environment.defaultLanguage ?? 'en-US'; + const defaultLanguage = { + name: defaultLocale, + code: defaultLocale.substring(0, 2) + }; + const rawLanguage = localStorage.getItem('mifosXLanguage'); + if (!rawLanguage) { + this.setDefaultLanguage(); + return defaultLanguage; + } + return safeParseObject<{ name: string; code: string }>(rawLanguage, defaultLanguage);
🧹 Nitpick comments (20)
src/app/clients/edit-client/edit-client.component.html (1)
31-38: Remove redundant hardcoded placeholder/title attributes.All modified input fields have duplicate placeholder and title attributes: hardcoded strings (e.g.,
placeholder="Account No",title="Account No") followed by Angular property bindings ([placeholder]="'labels.inputs...' | translate",[title]="..."). The static attributes are overridden during change detection and should be removed for clarity.Example: Lines 31–38 for accountNo. The bindings on lines 36–37 will override the hardcoded strings on lines 34–35.
Apply this cleanup pattern across all modified inputs (12 instances):
- <input - matInput - formControlName="accountNo" - placeholder="Account No" - title="Account No" - [placeholder]="'labels.inputs.Account No' | translate" - [title]="'labels.inputs.Account No' | translate" - /> + <input + matInput + formControlName="accountNo" + [placeholder]="'labels.inputs.Account No' | translate" + [title]="'labels.inputs.Account No' | translate" + />Repeat for all 12+ affected inputs (fullname, firstname, middlename, lastname, dateOfBirth, incorpValidityTillDate, incorpNumber, remarks, mobileNo, emailAddress, submittedOnDate, activationDate).
Also applies to: 43-50, 60-68, 77-85, 94-101, 106-114, 130-143, 204-209, 221-228, 233-242, 250-258, 263-270, 303-308, 325-329
src/app/organization/offices/view-office/general-tab/general-tab.component.html (1)
7-7: Addaria-hidden="true"to decorative horizontal dividers.The
<hr>elements used as section dividers are purely decorative and should be hidden from screen readers to improve accessibility.Apply this diff to mark dividers as decorative:
- <hr class="section-divider" /> + <hr class="section-divider" aria-hidden="true" />Also applies to: 13-13, 19-19
src/app/organization/entity-data-table-checks/entity-data-table-checks.component.ts (1)
127-139: Map-based lookup is a solid improvement; consider adding type safety.The refactored Map-based approach significantly improves performance (O(n+m) vs O(n*m) nested loops) and readability.
However, the use of
anytypes for theentityanditemparameters (lines 129, 133) bypasses strict type safety. Consider defining explicit interfaces:interface EntityMapping { code: string; value: string; } interface DataItem { entity: string; [key: string]: any; }Then update the method:
setEntity() { const entityMap = new Map<string, string>(); - this.entityValues.forEach((entity: any) => { + this.entityValues.forEach((entity: EntityMapping) => { entityMap.set(entity.code, entity.value); }); - this.dataSource.data.forEach((item: any) => { + this.dataSource.data.forEach((item: DataItem) => { const entityValue = entityMap.get(item.entity); if (entityValue) { item.entity = entityValue; } }); }As per coding guidelines
src/app/system/manage-data-tables/column-dialog/column-dialog.component.html (1)
21-21: *Consider addingtrackByfunctions to ngFor directives for consistency with Angular best practices.While
columnTypeDatais a small static array (8 items) anddata.columnCodesis provided once via dialog input without updates during the dialog lifecycle, Angular coding guidelines recommend implementingtrackByon all *ngFor loops. The performance impact here is minimal, but adding trackBy functions maintains consistency across the codebase.For line 21:
trackByValue = (index: number, item: any) => item.value;
For line 45:trackByCode = (index: number, item: any) => item.code;Also applies to: 45-45
src/app/products/charges/create-charge/create-charge.component.ts (1)
78-84: Add null check for parent object and improve type safety.While the individual array defaults are good,
data.chargesTemplate.incomeOrLiabilityAccountOptionsitself could be undefined or null, causing a runtime error before the|| []guards take effect. Additionally, the coding guidelines require strict type safety for Angular code, butincomeOptions,liabilityOptions, andincomeAndLiabilityAccountDatalack type annotations.Consider applying these improvements:
- const incomeOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.incomeAccountOptions || []; - const liabilityOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.liabilityAccountOptions || []; + const accountOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions || {}; + const incomeOptions: any[] = accountOptions.incomeAccountOptions || []; + const liabilityOptions: any[] = accountOptions.liabilityAccountOptions || []; if (liabilityOptions.length > 0) { this.incomeAndLiabilityAccountData = incomeOptions.concat(liabilityOptions); } else { this.incomeAndLiabilityAccountData = incomeOptions; }Note: The conditional concatenation at line 80 is safe but technically unnecessary, since concatenating an empty array has no effect. The current approach may be clearer for readers, so retaining it is reasonable.
src/app/home/dashboard/dashboard.component.ts (1)
85-106: Consider extracting shared logic into a utility function.The
getFrequentActivitiesmethod is duplicated in bothdashboard.component.tsandsidenav.component.ts(lines 133-154) with only the result count differing (8 vs 3). Extracting this into a shared utility function would reduce duplication and improve maintainability.Example refactor:
Create a shared utility file:
// src/app/shared/utils/activity-tracker.utils.ts export function getFrequentActivities(userActivity: string[], topN: number): string[] { const frequencyCounts: any = {}; let index = userActivity?.length; while (index) { const activity = userActivity[--index]; frequencyCounts[activity] = (frequencyCounts[activity] || 0) + 1; } const frequencyCountsArray = Object.entries(frequencyCounts); return frequencyCountsArray .sort((a: any, b: any) => b[1] - a[1]) .map((entry: any[]) => entry[0]) .filter( (activity: string) => !['/', '/login', '/home', '/dashboard'].includes(activity) ) .slice(0, topN); }Then simplify both components:
- getFrequentActivities() { - const frequencyCounts: any = {}; - let index = this.userActivity?.length; - while (index) { - const activity = this.userActivity[--index]; - frequencyCounts[activity] = (frequencyCounts[activity] || 0) + 1; - } - const frequencyCountsArray = Object.entries(frequencyCounts); - const topEigthFrequentActivities = frequencyCountsArray - .sort((a: any, b: any) => b[1] - a[1]) - .map((entry: any[]) => entry[0]) - .filter( - (activity: string) => ![ - '/', - '/login', - '/home', - '/dashboard' - ].includes(activity) - ) - .slice(0, 8); - return topEigthFrequentActivities; + getFrequentActivities() { + return getFrequentActivities(this.userActivity, 8); }src/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/reschedule-loan.component.ts (1)
99-100: LGTM! Route data subscription updated correctly.The subscription now correctly reads
rescheduleLoansData, matching the corrected resolver key in the routing configuration.Optional: Consider stronger typing. The route data uses
anytype. For better type safety, consider defining an interface for the resolved data:interface RescheduleLoansRouteData { rescheduleLoansData: any[]; // or more specific loan type } this.route.data.subscribe((data: RescheduleLoansRouteData) => { this.loans = data.rescheduleLoansData; // ... });As per coding guidelines.
src/app/home/timeout-dialog/idle-timeout.service.ts (2)
3-4: Remove unused RxJS operators.The operators
switchMap,takeUntil,tap, andmapare imported but not used anywhere in the file.Apply this diff to clean up the imports:
-import { switchMap, takeUntil, tap, map } from 'rxjs/operators';
27-33: Consider storing and cleaning up the constructor subscription.The subscription to
resetTimer$is created in the constructor but never stored or explicitly cleaned up. Since this is a root-level service, the subscription will persist for the app's lifetime, which is functionally acceptable. However, for best practices and testability, consider storing the subscription and cleaning it up in anngOnDestroyhook.Example:
private resetTimerSubscription?: Subscription; constructor() { this.$onSessionTimeout = this.timeout$.asObservable(); this.resetTimerSubscription = this.resetTimer$.subscribe(() => { // ... existing logic }); } ngOnDestroy() { this.resetTimerSubscription?.unsubscribe(); this.stop(); }src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts (2)
158-168: Consider guarding against empty relationship column names.When
getRelationshipColumnNamereturns an empty string (for unknown table names), the conditionitem.columnName === relationshipColumnNameon line 167 could theoretically match columns with empty names. While unlikely in practice, adding a guard would make the logic more robust.// Get the relationship column name based on application table const relationshipColumnName = this.getRelationshipColumnName(this.dataTableData.applicationTableName); this.dataTableData.columnHeaderData.forEach((item: any) => { // Mark system columns (id, created_at, updated_at) and relationship column as system item.system = [ 'id', 'created_at', 'updated_at' - ].includes(item.columnName) || item.columnName === relationshipColumnName; + ].includes(item.columnName) || (relationshipColumnName && item.columnName === relationshipColumnName); });
174-195: LGTM! Consider documenting the empty string return behavior.The mapping logic is clear and correct. For maintainability, consider adding a comment or logging a warning when an unknown table name is encountered, as the empty string return is silent.
getRelationshipColumnName(appTableName: string): string { // Map application table names to their relationship column names const tableToColumnMap: { [key: string]: string } = { m_client: 'client_id', m_group: 'group_id', m_center: 'center_id', m_office: 'office_id', m_loan: 'loan_id', m_savings_account: 'savings_account_id', m_savings_account_transaction: 'savings_transaction_id', m_product_loan: 'product_loan_id', m_savings_product: 'savings_product_id', m_share_product: 'share_product_id' }; - return tableToColumnMap[appTableName] || ''; + // Return empty string for unknown tables (relationship column won't be marked as system) + return tableToColumnMap[appTableName] || ''; }src/app/templates/create-edit-template/create-edit-template.component.ts (2)
268-270: Consider aligning signature with Angular's TrackByFunction pattern.While functional, Angular's
TrackByFunction<T>expects both index and item parameters. Consider updating to match the pattern for consistency:trackByMapperIndex(index: number, mapper: any): number { return index; }Note: Using index as the identifier means items will be re-rendered if reordered or inserted in the middle. This is acceptable for append-only scenarios but consider using a unique property if available.
288-290: Consider improving type safety for entity and type parameters.Both
trackByEntityIdandtrackByTypeIduseanyfor their object parameters. Consider defining interfaces or using more specific types to leverage TypeScript's type checking:interface EntityType { id: number; name: string; // other properties... } interface TemplateType { id: number; name: string; // other properties... } trackByEntityId(index: number, entity: EntityType): number { return entity.id; } trackByTypeId(index: number, type: TemplateType): number { return type.id; }This would provide better type safety and IDE support throughout the component.
As per coding guidelines.
Also applies to: 298-300
src/app/accounting/chart-of-accounts/edit-gl-account/edit-gl-account.component.html (1)
13-16: Consider applying the same error display improvements from the create form.The create GL account form (create-gl-account.component.html) gates error display behind both validation state AND user interaction (
touched || dirty), but this edit form shows errors immediately. For consistency and better UX, consider applying the same pattern here.Example for the Account Type field:
-<mat-error *ngIf="glAccountForm.controls.type.hasError('required')"> +<mat-error *ngIf="glAccountForm.controls.type.hasError('required') && (glAccountForm.controls.type.touched || glAccountForm.controls.type.dirty)"> {{ 'labels.inputs.Account Type' | translate }} {{ 'labels.commons.is' | translate }} <strong>{{ 'labels.commons.required' | translate }}</strong> </mat-error>Apply the same pattern to the other three required fields (name, usage, glCode).
Also applies to: 22-25, 35-38, 44-47
src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.ts (1)
109-118: Improve type safety for theofficerIdparameter.The
officerIdparameter is typed asany, which bypasses TypeScript's type checking. Consider using a more specific type such asstring | number | null | undefinedto improve type safety.Apply this diff:
- getFromOfficers(officerId: any) { + getFromOfficers(officerId: string | number | null | undefined) { this.toLoanOfficers = this.fromLoanOfficers?.filter((officer: any) => officer.id !== officerId) || []; if (officerId && this.officeTemplate && this.officeTemplate.id) { this.organizationSevice.getOfficerTemplate(officerId, this.officeTemplate.id).subscribe((response: any) => {src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.html (1)
9-10: *Consider adding trackBy functions to ngFor loops.Multiple
*ngForloops in this template lacktrackByfunctions, which can impact rendering performance when the underlying collections change. Consider adding trackBy functions to improve change detection efficiency.As per coding guidelines.
Example for the offices loop:
// In the component trackByOfficeId(index: number, office: any): any { return office.id; }- <mat-option *ngFor="let office of offices" [value]="office.id"> + <mat-option *ngFor="let office of offices; trackBy: trackByOfficeId" [value]="office.id">Apply similar trackBy functions for other loops (fromLoanOfficers, toLoanOfficers, clients, loans, groups).
Also applies to: 40-42, 55-57, 73-79, 81-87, 99-105, 107-112
src/app/organization/fund-mapping/fund-mapping.component.html (1)
21-23: *Consider adding trackBy functions to ngFor loops.The
*ngForloops for loan products and offices lacktrackByfunctions, which can impact rendering performance when the collections change.As per coding guidelines.
Example implementation:
// In the component trackByProductId(index: number, product: any): any { return product.id; } trackByOfficeId(index: number, office: any): any { return office.id; }Then update the template:
- <mat-option *ngFor="let product of advanceSearchTemplate.loanProducts" [value]="product.id"> + <mat-option *ngFor="let product of advanceSearchTemplate.loanProducts; trackBy: trackByProductId" [value]="product.id">- <mat-option *ngFor="let office of advanceSearchTemplate.offices" [value]="office.id"> + <mat-option *ngFor="let office of advanceSearchTemplate.offices; trackBy: trackByOfficeId" [value]="office.id">Also applies to: 30-32
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
196-226: Extract duplicated form patching logic into a helper method.The form patching code in
ngOnChanges()(lines 196–226) andngOnInit()(lines 326–355) is nearly identical, violating the DRY principle. This duplication makes maintenance error-prone and has already led to inconsistencies (e.g.,interestRatePerPeriodcontrol removal logic exists only inngOnChanges).Extract the common patching logic:
private patchLoansAccountTermsForm(): void { this.loansAccountTermsForm.patchValue({ principalAmount: this.loansAccountTermsData.principal, loanTermFrequency: this.loansAccountTermsData.termFrequency, loanTermFrequencyType: this.loansAccountTermsData.termPeriodFrequencyType.id, numberOfRepayments: this.loansAccountTermsData.numberOfRepayments, repaymentEvery: this.loansAccountTermsData.repaymentEvery, repaymentFrequencyType: this.loansAccountTermsData.repaymentFrequencyType.id, amortizationType: this.loansAccountTermsData.amortizationType.id, isEqualAmortization: this.loansAccountTermsData.isEqualAmortization, interestType: this.loansAccountTermsData.interestType.id, isFloatingInterestRate: this.loansAccountTermsData.isLoanProductLinkedToFloatingRate ? false : null, interestCalculationPeriodType: this.loansAccountTermsData.interestCalculationPeriodType.id, allowPartialPeriodInterestCalculation: this.loansAccountTermsData.allowPartialPeriodInterestCalculation, inArrearsTolerance: this.loansAccountTermsData.inArrearsTolerance, graceOnPrincipalPayment: this.loansAccountTermsData.graceOnPrincipalPayment, graceOnInterestPayment: this.loansAccountTermsData.graceOnInterestPayment, graceOnArrearsAgeing: this.loansAccountTermsData.graceOnArrearsAgeing, graceOnInterestCharged: this.loansAccountTermsData.graceOnInterestCharged, fixedEmiAmount: this.loansAccountTermsData.fixedEmiAmount, maxOutstandingLoanBalance: this.loansAccountTermsData.maxOutstandingLoanBalance, transactionProcessingStrategyCode: this.loansAccountTermsData.transactionProcessingStrategyCode, interestRateDifferential: this.loansAccountTermsData.interestRateDifferential, multiDisburseLoan: this.loansAccountTermsData.multiDisburseLoan, interestRateFrequencyType: this.loansAccountTermsData.interestRateFrequencyType.id, balloonRepaymentAmount: this.loansAccountTermsData.balloonRepaymentAmount, interestRecognitionOnDisbursementDate: this.loansAccountTermsData.interestRecognitionOnDisbursementDate || false, moratoriumPrincipal: this.loansAccountTermsData.moratoriumPrincipal ?? 0, moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 }); }Then call
this.patchLoansAccountTermsForm()from both lifecycle hooks.Also applies to: 326-355
293-304: Extract duplicatedmaxOutstandingLoanBalancecontrol manipulation.The logic to remove and conditionally re-add the
maxOutstandingLoanBalancecontrol (lines 293–304 inngOnChangesand lines 362–374 inngOnInit) is duplicated. This violates DRY and makes the code harder to maintain.Extract this into a helper method:
private updateMaxOutstandingLoanBalanceControl(): void { this.loansAccountTermsForm.removeControl('maxOutstandingLoanBalance'); const value = this.loansAccountTermsData?.maxOutstandingLoanBalance ?? null; if (this.allowAddDisbursementDetails()) { this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', new UntypedFormControl(value, Validators.required) ); } else { this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', new UntypedFormControl(value) ); } }Then call
this.updateMaxOutstandingLoanBalanceControl()from bothngOnChanges()andngOnInit().Also applies to: 362-374
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
249-254: Improve type safety in notify method.While the array mutation issue has been resolved, both parameters still use
anytypes, reducing type safety as required by coding guidelines. Consider defining proper interfaces for the error body structure.interface ErrorBody { defaultUserMessage: string; errors?: Array<{ developerMessage: string }>; } notify(body: ErrorBody, data: any) { 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 (88)
Dockerfile(1 hunks)src/app/accounting/accounting-rules/edit-rule/edit-rule.component.html(1 hunks)src/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.html(4 hunks)src/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.ts(1 hunks)src/app/accounting/chart-of-accounts/edit-gl-account/edit-gl-account.component.html(1 hunks)src/app/app.module.ts(2 hunks)src/app/clients/client-stepper/client-general-step/client-general-step.component.ts(1 hunks)src/app/clients/edit-client/edit-client.component.html(10 hunks)src/app/clients/edit-client/edit-client.component.scss(1 hunks)src/app/core/authentication/authentication.service.ts(5 hunks)src/app/core/http/error-handler.interceptor.ts(3 hunks)src/app/core/shell/sidenav/sidenav.component.ts(1 hunks)src/app/core/utils/json.ts(1 hunks)src/app/home/dashboard/dashboard.component.ts(1 hunks)src/app/home/timeout-dialog/idle-timeout.service.ts(2 hunks)src/app/loans/common-resolvers/loan-action-button.resolver.ts(1 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(8 hunks)src/app/loans/loans-view/general-tab/general-tab.component.html(1 hunks)src/app/loans/loans-view/general-tab/general-tab.component.ts(1 hunks)src/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.html(2 hunks)src/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.ts(4 hunks)src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts(2 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)src/app/organization/bulk-import/view-bulk-import/view-bulk-import.component.html(1 hunks)src/app/organization/bulk-import/view-bulk-import/view-bulk-import.component.scss(2 hunks)src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.html(1 hunks)src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.scss(1 hunks)src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.ts(1 hunks)src/app/organization/entity-data-table-checks/entity-data-table-checks.component.ts(1 hunks)src/app/organization/fund-mapping/fund-mapping.component.html(10 hunks)src/app/organization/holidays/edit-holiday/edit-holiday.component.ts(1 hunks)src/app/organization/loan-provisioning-criteria/create-loan-provisioning-criteria/create-loan-provisioning-criteria.component.scss(1 hunks)src/app/organization/loan-provisioning-criteria/create-loan-provisioning-criteria/create-loan-provisioning-criteria.component.ts(2 hunks)src/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.html(1 hunks)src/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.scss(1 hunks)src/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.ts(3 hunks)src/app/organization/offices/view-office/general-tab/general-tab.component.html(2 hunks)src/app/organization/offices/view-office/general-tab/general-tab.component.scss(2 hunks)src/app/organization/offices/view-office/view-office.component.html(2 hunks)src/app/organization/offices/view-office/view-office.component.scss(1 hunks)src/app/organization/payment-types/payment-types.component.scss(1 hunks)src/app/pipes/find.pipe.ts(1 hunks)src/app/pipes/url-to-string.pipe.spec.ts(1 hunks)src/app/products/charges/create-charge/create-charge.component.ts(1 hunks)src/app/settings/settings.service.ts(7 hunks)src/app/shared/error-dialog/error-dialog.component.html(1 hunks)src/app/shared/error-dialog/error-dialog.component.ts(3 hunks)src/app/shared/theme-picker/theme-storage.service.ts(2 hunks)src/app/shared/theme-toggle/theme-toggle.component.ts(1 hunks)src/app/system/audit-trails/audit-trails.component.ts(4 hunks)src/app/system/audit-trails/view-audit/view-audit.component.ts(1 hunks)src/app/system/codes/view-code/view-code.component.html(5 hunks)src/app/system/configurations/business-date-tab/business-date-tab.component.ts(1 hunks)src/app/system/manage-data-tables/column-dialog/column-dialog.component.html(2 hunks)src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts(2 hunks)src/app/system/manage-jobs/scheduler-jobs/view-history-scheduler-job/view-history-scheduler-job.component.ts(1 hunks)src/app/tasks/checker-inbox-and-tasks-tabs/client-approval/client-approval.component.ts(1 hunks)src/app/tasks/checker-inbox-and-tasks-tabs/loan-approval/loan-approval.component.ts(1 hunks)src/app/tasks/checker-inbox-and-tasks-tabs/loan-disbursal/loan-disbursal.component.ts(1 hunks)src/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/reschedule-loan.component.ts(1 hunks)src/app/tasks/tasks-routing.module.ts(1 hunks)src/app/tasks/view-checker-inbox/view-checker-inbox.component.ts(2 hunks)src/app/templates/create-edit-template/create-edit-template.component.html(3 hunks)src/app/templates/create-edit-template/create-edit-template.component.scss(1 hunks)src/app/templates/create-edit-template/create-edit-template.component.ts(1 hunks)src/app/users/create-user/create-user.component.html(1 hunks)src/app/web-app.component.ts(7 hunks)src/app/zitadel/auth.service.ts(2 hunks)src/app/zitadel/callback/callback.component.ts(2 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)src/index.html(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/app/system/configurations/business-date-tab/business-date-tab.component.ts
- src/assets/translations/fr-FR.json
- src/assets/translations/de-DE.json
- src/app/templates/create-edit-template/create-edit-template.component.scss
🚧 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/app.module.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.ts
🧰 Additional context used
📓 Path-based instructions (2)
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-view/general-tab/general-tab.component.tssrc/app/system/manage-data-tables/column-dialog/column-dialog.component.htmlsrc/app/tasks/view-checker-inbox/view-checker-inbox.component.tssrc/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/organization/bulk-import/view-bulk-import/view-bulk-import.component.scsssrc/app/organization/bulk-import/view-bulk-import/view-bulk-import.component.htmlsrc/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.scsssrc/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.htmlsrc/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.htmlsrc/app/clients/client-stepper/client-general-step/client-general-step.component.tssrc/app/users/create-user/create-user.component.htmlsrc/app/core/shell/sidenav/sidenav.component.tssrc/app/organization/entity-data-table-checks/entity-data-table-checks.component.tssrc/app/organization/loan-provisioning-criteria/create-loan-provisioning-criteria/create-loan-provisioning-criteria.component.tssrc/app/organization/fund-mapping/fund-mapping.component.htmlsrc/app/shared/error-dialog/error-dialog.component.htmlsrc/app/tasks/checker-inbox-and-tasks-tabs/client-approval/client-approval.component.tssrc/app/system/audit-trails/view-audit/view-audit.component.tssrc/app/organization/offices/view-office/view-office.component.scsssrc/app/tasks/checker-inbox-and-tasks-tabs/loan-approval/loan-approval.component.tssrc/app/organization/offices/view-office/general-tab/general-tab.component.htmlsrc/app/shared/theme-picker/theme-storage.service.tssrc/app/pipes/url-to-string.pipe.spec.tssrc/app/core/http/error-handler.interceptor.tssrc/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.tssrc/app/accounting/accounting-rules/edit-rule/edit-rule.component.htmlsrc/app/core/authentication/authentication.service.tssrc/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.tssrc/app/templates/create-edit-template/create-edit-template.component.tssrc/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.tssrc/app/organization/offices/view-office/view-office.component.htmlsrc/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.scsssrc/app/products/charges/create-charge/create-charge.component.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/system/manage-jobs/scheduler-jobs/view-history-scheduler-job/view-history-scheduler-job.component.tssrc/app/system/codes/view-code/view-code.component.htmlsrc/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.tssrc/app/web-app.component.tssrc/app/zitadel/auth.service.tssrc/app/zitadel/callback/callback.component.tssrc/app/organization/loan-provisioning-criteria/create-loan-provisioning-criteria/create-loan-provisioning-criteria.component.scsssrc/app/pipes/find.pipe.tssrc/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/reschedule-loan.component.tssrc/app/templates/create-edit-template/create-edit-template.component.htmlsrc/app/organization/payment-types/payment-types.component.scsssrc/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.htmlsrc/app/core/utils/json.tssrc/app/clients/edit-client/edit-client.component.scsssrc/app/clients/edit-client/edit-client.component.htmlsrc/app/system/manage-data-tables/edit-data-table/edit-data-table.component.tssrc/app/accounting/chart-of-accounts/edit-gl-account/edit-gl-account.component.htmlsrc/app/home/dashboard/dashboard.component.tssrc/app/tasks/tasks-routing.module.tssrc/app/home/timeout-dialog/idle-timeout.service.tssrc/app/tasks/checker-inbox-and-tasks-tabs/loan-disbursal/loan-disbursal.component.tssrc/app/organization/offices/view-office/general-tab/general-tab.component.scsssrc/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.tssrc/app/system/audit-trails/audit-trails.component.tssrc/app/shared/error-dialog/error-dialog.component.tssrc/app/loans/loans-view/general-tab/general-tab.component.htmlsrc/app/loans/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.htmlsrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.htmlsrc/app/settings/settings.service.tssrc/app/organization/holidays/edit-holiday/edit-holiday.component.tssrc/app/shared/theme-toggle/theme-toggle.component.ts
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: For tests: encourage clear Arrange-Act-Assert structure,
stable selectors, and minimal brittle timing dependencies.
Files:
src/app/pipes/url-to-string.pipe.spec.ts
🧬 Code graph analysis (12)
src/app/tasks/view-checker-inbox/view-checker-inbox.component.ts (1)
src/app/core/utils/json.ts (1)
safeParseObject(10-18)
src/app/clients/client-stepper/client-general-step/client-general-step.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(112-115)
src/app/shared/theme-picker/theme-storage.service.ts (2)
src/app/core/utils/json.ts (1)
safeParseObject(10-18)src/app/shared/theme-picker/theme.model.ts (1)
Theme(4-10)
src/app/core/http/error-handler.interceptor.ts (3)
src/app/core/http/http.service.ts (1)
request(104-110)src/environments/environment.prod.ts (1)
environment(7-74)src/environments/environment.ts (1)
environment(12-78)
src/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.ts (2)
src/app/shared/models/general.model.ts (1)
CodeValue(65-72)src/app/shared/models/option-data.model.ts (1)
OptionData(1-5)
src/app/web-app.component.ts (1)
src/app/core/utils/json.ts (1)
safeParseArray(20-28)
src/app/zitadel/auth.service.ts (1)
src/app/core/authentication/o-auth2-token.model.ts (1)
OAuth2Token(4-10)
src/app/home/timeout-dialog/idle-timeout.service.ts (2)
src/environments/environment.prod.ts (1)
environment(7-74)src/environments/environment.ts (1)
environment(12-78)
src/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.ts (1)
src/app/products/loan-products/models/loan-product.model.ts (1)
LoanProduct(17-162)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(112-115)
src/app/settings/settings.service.ts (1)
src/app/core/utils/json.ts (3)
safeParse(1-8)safeParseObject(10-18)safeParseArray(20-28)
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(112-115)
🪛 ast-grep (0.39.9)
src/app/zitadel/auth.service.ts
[warning] 144-144: Sensitive information detected in localStorage. Storing sensitive data like emails, usernames, or personal information in localStorage exposes it to malicious scripts and XSS attacks. Use secure storage alternatives or avoid storing sensitive data client-side.
Context: localStorage.setItem('refresh_token', tokens.refresh_token)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://cwe.mitre.org/data/definitions/312.html
- https://owasp.org/www-community/vulnerabilities/HTML5_Local_Storage_Security
(local-storage-sensitive-information)
⏰ 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
src/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.ts
Show resolved
Hide resolved
...anage-jobs/scheduler-jobs/view-history-scheduler-job/view-history-scheduler-job.component.ts
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
4803c76 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-352: Add optional chaining to overdue charges table for consistency and safety.While the currency display was updated with a conditional (line 335), the property access on lines 341 and 351 still uses direct property access without optional chaining. This is inconsistent with the main charges table (lines 272, 286) and could cause the same "Cannot read properties of undefined" runtime errors that were fixed above.
Apply this diff to add defensive guards:
<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="amount"> <th mat-header-cell *matHeaderCellDef>{{ 'labels.inputs.Amount' | translate }}</th> <td mat-cell *matCellDef="let charge">{{ charge.amount | formatNumber }}</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>
🧹 Nitpick comments (3)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
180-217: Improve type safety for theclientparameter.The
clientparameter uses theanytype, which reduces type safety and can lead to runtime errors if required properties are missing. Define an interface that specifies the expected properties (id,principal) to catch errors at compile time.Consider adding an interface and updating the signature:
interface GlimClientMember { id: number; principal: number; } setData(applicationId: number, client: GlimClientMember, totalLoan: number, isFirst: boolean, isLast: boolean): any { // ... rest of implementation }Based on coding guidelines.
220-238: Consider potential applicationId collisions withMath.random().The applicationId is generated using
Math.random()(line 224), which is not cryptographically secure and could theoretically produce collisions in high-volume scenarios. While the 10-digit range makes collisions unlikely for single-session GLIM batches, consider using a more robust approach if this ID must be globally unique or if multiple concurrent GLIM applications are common.Options to consider:
- Use
crypto.getRandomValues()for better randomness- Combine timestamp with a random component:
Date.now() * 10000 + Math.floor(Math.random() * 10000)- Let the backend generate the applicationId if this is a critical identifier
// Option 2 example: const applicationId = Date.now() * 10000 + Math.floor(Math.random() * 10000);
249-254: Complete type safety improvements for thenotifymethod.The array mutation issue has been addressed by using
forEachinstead ofpop(). However, the type safety concern from the previous review remains: both parameters still useanytypes. Define proper interfaces to improve type safety per coding guidelines.Consider defining interfaces for the parameters:
interface ErrorResponse { defaultUserMessage: string; errors?: Array<{ 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); }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 (4)
- src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
- src/app/app.module.ts
- 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.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/glim-account/create-glim-account/create-glim-account.component.tssrc/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
🧬 Code graph analysis (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(112-115)
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)
⏰ 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 (5)
src/app/login/login-form/login-form.component.ts (2)
13-13: LGTM! Correct module import for standalone component.The change from
MatCheckboxtoMatCheckboxModuleis the proper pattern for Angular standalone components. This ensures the checkbox functionality works correctly with Angular Material.
34-34: LGTM! Module properly registered.The
MatCheckboxModuleis correctly added to the component's standalone imports array, enabling the "remember me" checkbox functionality. This fix resolves the Angular Material import error mentioned in the PR discussion.src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: LGTM! Previous review feedback has been addressed.The method properly handles optional response data and moves the
pristineflag inside the conditional. The typing forresponseparameter uses optional chaining (response?.data) to safely handle dialog cancellation. The implementation is consistent with other edit methods in the component.src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html (2)
265-265: Good defensive programming.The conditional currency display prevents the TypeError reported in the PR comments when
charge.currencyis undefined.
272-272: Proper use of optional chaining and fallbacks.These changes add defensive guards against undefined properties, preventing runtime errors when charge data is incomplete. The N/A fallback on line 313 improves the user experience when date types don't match known patterns.
Also applies to: 286-286, 294-314
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
53e14d9 to
1919aa9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
526-528: Consider using null as default for optional moratorium fields.The moratorium fields default to
0, which doesn't distinguish between "not set" and "explicitly set to 0 periods". While0is semantically reasonable for moratorium (meaning no moratorium), usingnullas the default would make the distinction clearer and align with optional field patterns.If you want to distinguish "not set" from "set to 0", consider this change:
interestRecognitionOnDisbursementDate: [false], -moratoriumPrincipal: [0], -moratoriumInterest: [0] +moratoriumPrincipal: [null], +moratoriumInterest: [null]And update the patchValue calls to use
?? nullinstead of?? 0on lines 224-225 and 353-354. However, if the backend API expects0for "no moratorium," the current implementation is acceptable.
81-86: Type safety: Replace 'any' types with proper interfaces.The component uses
anytypes extensively for inputs and internal properties (loansProductOptions,loansAccountProductTemplate,loansAccountTemplate,loansAccountTermsData). This bypasses TypeScript's type checking and makes the code more error-prone.As per coding guidelines requiring strict type safety for Angular code, consider defining proper interfaces for these data structures. For example:
interface LoansAccountProductTemplate { currency: Currency; principal: number; termFrequency: number; moratoriumPrincipal?: number; moratoriumInterest?: number; // ... other properties } @Input() loansAccountProductTemplate: LoansAccountProductTemplate;This would provide compile-time type checking and better IDE support.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (3)
180-217: Improve type safety throughoutsetDatamethod.The method uses
anyfor multiple parameters and variables, violating the strict type safety guideline for Angular code:
client: anyparameter (line 180)data: anyvariable (line 184)charge: anyin the map callback (line 186)Define proper interfaces for these types to improve maintainability and catch type errors at compile time.
Based on coding guidelines.
Consider creating interfaces like:
interface GlimClient { id: string | number; principal: number; } interface LoanCharge { id: string | number; amount: number; currency: any; // or a proper Currency interface }Then update the signature:
-setData(applicationId: number, client: any, totalLoan: number, isFirst: boolean, isLast: boolean): any { +setData(applicationId: number, client: GlimClient, totalLoan: number, isFirst: boolean, isLast: boolean): string {
224-224: Consider the risk of ID collisions with Math.random().The
applicationIdis generated usingMath.random()to create a 10-digit number. While the probability of collision is low for typical usage, it's theoretically possible, especially as the application scales. Since this ID links batched child loans together, a collision could cause data integrity issues.For a production system handling high volumes, consider using a more robust approach such as:
- A server-generated UUID or sequential ID
crypto.randomUUID()(if the environment supports it)- A combination of timestamp + random component
Example using timestamp + random:
const timestamp = Date.now().toString(); const random = Math.floor(Math.random() * 10000).toString().padStart(4, '0'); const applicationId = parseInt(timestamp + random);
249-254: Address type safety innotifymethod.While the array mutation issue from the past review was addressed (using
forEachinstead ofpop), the type safety concern remains. Both parameters still useanytypes, and theerrorin the forEach callback is also untyped.Based on coding guidelines.
Define proper interfaces for the error response structure:
interface ErrorDetail { developerMessage: string; } interface ErrorResponse { defaultUserMessage: string; errors?: ErrorDetail[]; } notify(body: ErrorResponse, data: any) { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error: ErrorDetail) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }
📜 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 (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-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-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/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)
⏰ 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/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
222-226: LGTM! Moratorium fields properly initialized.The moratorium fields are now correctly populated from
loansAccountTermsDatausing nullish coalescing, which addresses the data loss concern raised in previous reviews. The use of?? 0ensures that undefined or null values default to 0, which is appropriate for moratorium period fields.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
227-234: LGTM! Request chaining and batching implementation is correct.The implementation correctly:
- Generates a shared
applicationIdfor all batched child loans- Sets
isParentAccount: truefor the first request via theisFirstflag- Sets
lastApplication: truefor the final request via theisLastflag- Chains requests by setting each request's
referenceto the previous index (null for the first request)This aligns with the PR objectives and the backend guidance provided by the reviewer rhopman.
214-215: Clarify whether deletion of moratorium/other fields is intentional GLIM API limitation or incomplete implementation.The evidence confirms a contradiction:
moratoriumPrincipal,moratoriumInterest,principalAmount,isFloatingInterestRate,allowPartialPeriodInterestCalculation, andmultiDisburseLoanare all form controls in the loans-account-terms-step component and can be edited by users. However, lines 214-215 delete these fields before sending to the backend.The PR objective states these fields should be "configurable and editable in the UI and API payload," but the GLIM implementation removes them from the API payload entirely. Either:
- GLIM backend doesn't support these fields (needs documentation), or
- The PR scope excludes GLIM (update PR objectives), or
- The deletion is incomplete implementation
Add a comment explaining why GLIM API excludes these fields, or verify that the PR objectives align with the current implementation.
...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
20c0eaa to
42a5d9a
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)
249-253: Good fix on non‑mutating error handling; remaining gap is strong typingNice change to iterate over
body.errorsinstead of popping — this keeps the error payload intact while still logging all messages.One remaining nit from the earlier review: both
bodyanddataare still typed asany. Defining a minimal interface for the error shape (e.g.{ defaultUserMessage: string; errors?: ReadonlyArray<{ developerMessage: string }>; }) and a concrete type fordata(the batch payload or request body) would bring this method in line with the “strict type safety” guideline and make future refactors safer.
🧹 Nitpick comments (3)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: Overdue charge edit flow is correct; consider shared helper & i18n keysThe implementation looks sound: it defensively handles the dialog result, updates the correct entry in
overDueChargesDataSource, and only flipspristinewhen a change is applied. Two follow‑ups to consider:
- The logic closely mirrors
editChargeAmount; extracting a small shared helper (e.g.,openAmountEditDialogor a generic “update charge in dataSource” function) would reduce duplication and centralize the typing of the dialog response/charge shape.- The hard‑coded strings (
'Edit Overdue Charge Amount','Amount','Confirm') should eventually be switched to translation keys to match the rest of the app’s i18n practices and the stated PR plan.As per coding guidelines.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
180-217: ClarifysetDatareturn type and make charges mapping a bit saferThe overall GLIM payload shaping here (shared
applicationId,groupId,isParentAccount/lastApplication, field cleanup) looks consistent with the described backend contract.Two small improvements:
- Return type
setDataalways returnsJSON.stringify(data), so its signature can be made precise:- setData(applicationId: number, client: any, totalLoan: number, isFirst: boolean, isLast: boolean): any { + setData(applicationId: number, client: any, totalLoan: number, isFirst: boolean, isLast: boolean): string {
- Defensive charges mapping
To avoid a possible runtime error ifthis.loansAccount.chargesis evernull/undefined, you can guard it without changing semantics when it’s an array:- charges: this.loansAccount.charges.map((charge: any) => ({ + charges: (this.loansAccount.charges ?? []).map((charge: any) => ({ chargeId: charge.id, amount: charge.amount, currency: charge.currency })),
219-238: Batch request construction looks correct; consider tightening types and decoupling fromselectedMembersstateThe batching logic here (single 10‑digit
applicationId, one POST per member, first request as parent, last withlastApplication, andreferencechaining toindex - 1) matches the intended GLIM behavior.Two optional cleanups:
Avoid
any[]for the return type
Defining a small interface (e.g.BatchRequest) and returningBatchRequest[]instead ofany[]would improve type safety forrequestId,reference,method, andbody.Reduce reliance on
this.selectedMembersside‑effects
memberSelectedcurrently depends onthis.selectedMembersbeing populated elsewhere (via theloansAccountgetter). If feasible, consider deriving it directly fromthis.loansActiveClientMembers(e.g., itsselectedClientMembers) sobuildRequestDatais self‑contained and doesn’t depend on another getter having been invoked first.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app/app.module.ts(1 hunks)src/app/core/i18n/i18n.service.ts(1 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(5 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)src/app/web-app.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.html
- 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.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/login/login-form/login-form.component.tssrc/app/web-app.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.tssrc/app/core/i18n/i18n.service.tssrc/app/app.module.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html
🧬 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 (5)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (2)
1-25: ✅ Moratorium fields are now properly inside the form.The previous critical issue flagged in commit 686ba98 has been resolved—
moratoriumPrincipalandmoratoriumInterestform controls are now correctly positioned within the<form [formGroup]="loansAccountTermsForm">tag (lines 2–25). Both fields follow the established accessibility and localization patterns witharia-labelattributes and translation keys. The structure is correct.
44-44: Comprehensive accessibility and semantic enhancements across the template.ARIA labels have been systematically added to input fields, buttons have explicit
type="button"attributes, and accessibility attributes are consistently applied throughout the form. This improves keyboard navigation, screen reader support, and semantic HTML structure. The additions align with Angular and WCAG accessibility best practices.Also applies to: 66-66, 81-81, 91-91, 103-103, 118-118, 140-140, 243-248, 365-365, 375-375, 386-386, 391-391, 396-396, 545-545, 582-582, 660-660, 690-695, 708-708, 712-712, 716-716
src/app/login/login-form/login-form.component.ts (1)
13-13: LGTM! Correct Material module import pattern.The change from
MatCheckboxtoMatCheckboxModulefollows Angular Material best practices for standalone components. This properly resolves the Angular Material import error mentioned in the PR discussion.Also applies to: 34-34
src/app/core/i18n/i18n.service.ts (1)
8-8: Excellent! Modern Angular DI pattern applied.Using
providedIn: 'root'is the recommended approach for services. This enables tree-shaking, simplifies dependency management, and properly resolves theNullInjectorErrorreported in the PR discussion.src/app/web-app.component.ts (1)
124-125: Proper dependency injection applied.This correctly replaces the manual service instantiation with constructor-based DI, eliminating the anti-pattern that likely caused the original
NullInjectorError. The change aligns with Angular best practices and works seamlessly with theprovidedIn: 'root'configuration inI18nService.
...loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-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
42a5d9a to
937f4e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/web-app.component.ts (1)
184-189: Consider improving type safety of the translation result.The observable pattern is clean and correct. However,
titleTranslatedis typed asanywhen it should bestringfor better type safety.Apply this diff to improve type safety:
this.i18nService .translate(title) .pipe(take(1)) - .subscribe((titleTranslated: any) => { + .subscribe((titleTranslated: string) => { this.titleService.setTitle(titleTranslated); });src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
220-238: Add explicit return type to buildRequestData.The method lacks an explicit return type annotation, reducing type safety. Based on the structure, the return type should be an array of batch request objects.
Apply this diff to add explicit typing:
-buildRequestData(): any[] { +buildRequestData(): Array<{ requestId: string; reference: string | null; method: string; relativeUrl: string; body: string }> {Consider defining a dedicated interface for better maintainability:
interface BatchRequest { requestId: string; reference: string | null; method: string; relativeUrl: string; body: string; }Then use:
buildRequestData(): BatchRequest[] {Based on coding guidelines.
249-254: Complete type safety improvements for notify method.While the method correctly uses
forEachinstead ofpop()(as addressed in previous reviews), both parameters still useanytypes, which violates coding guidelines for strict type safety.Define interfaces for the parameters:
interface ErrorResponse { defaultUserMessage: string; errors?: Array<{ developerMessage: string }>; }Then update the signature:
-notify(body: any, data: any) { +notify(body: ErrorResponse, data: any) {For the
dataparameter, consider using a more specific type based on the actual data being passed (likely the GLIM request data).Based on coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app/core/i18n/i18n.service.ts(1 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(5 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)src/app/web-app.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/app/core/i18n/i18n.service.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-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/login/login-form/login-form.component.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-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/web-app.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)
⏰ 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/web-app.component.ts (1)
124-125: LGTM! Proper migration to dependency injection.The change from manual instantiation to constructor-based dependency injection is correct and follows Angular best practices. This properly addresses the NullInjectorError mentioned in the PR objectives by leveraging the updated I18nService configuration with
providedIn: 'root'.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
214-215: Verify whether moratorium field deletion is intentional for GLIM accounts or inconsistent with PR objectives.The review comment is well-founded. While moratorium fields are editable in the UI and included in the terms form (confirmed in
loans-account-terms-step.component.tslines 224-225, 350-351), thesetData()method in the GLIM account creation component unconditionally deletes both fields from the API payload (lines 214-215).Key findings:
- Moratorium fields are part of the editable form across all loan workflows
- The deletion is grouped with other GLIM-specific unsupported fields, suggesting intentional exclusion
- However, no code comments or documentation explain why GLIM accounts should not include moratorium fields
- If the PR objective to make moratorium fields "configurable and editable in the API payload" applies to GLIM accounts, this deletion contradicts that goal
The concern is valid but unresolvable without confirmation of:
- PR scope: Does the moratorium feature apply to GLIM accounts or only standard loans?
- API constraints: Does the GLIM backend API reject or ignore these fields?
- Intended behavior: Should moratorium values be conditionally included only when non-zero/non-null, or permanently excluded for GLIM?
src/app/loans/glim-account/create-glim-account/create-glim-account.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
937f4e5 to
efc7a7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: Add explicit return type and refine charge parameter type.The method lacks an explicit return type annotation, and the charge parameter still uses
anyin its index signature, reducing type safety.Apply this diff to improve type safety:
- editOverdueChargeAmount(charge: { amount: number; [key: string]: any }) { + editOverdueChargeAmount(charge: { amount: number; [key: string]: unknown }): void { const formfields: FormfieldBase[] = [ new InputBase({ controlName: 'amount', label: 'Amount', value: charge.amount, type: 'number', required: false }) ]; const data = { title: 'Edit Overdue Charge Amount', 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 }; this.overDueChargesDataSource.splice(this.overDueChargesDataSource.indexOf(charge), 1, newCharge); this.overDueChargesDataSource = this.overDueChargesDataSource.concat([]); this.pristine = false; } }); }Ideally, define a proper interface for charges (e.g.,
OverdueCharge) instead of inline types to improve maintainability and reusability across the component.As per coding guidelines.
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (2)
355-355: Remove or uncomment the setCustomValidators call.The commented-out call to
setCustomValidators()creates ambiguity about whether this validation logic is intentionally disabled or forgotten. If the validators are no longer needed, remove the commented code. If they are needed, uncomment the call and ensure proper functionality.this.setAdvancedPaymentStrategyControls(); - // this.setCustomValidators(); this.setLoanTermListener();
358-370: Remove redundant control removal.Line 360 redundantly removes
maxOutstandingLoanBalancewhich was already removed on line 358. This adds unnecessary code and reduces clarity.this.loansAccountTermsForm.removeControl('maxOutstandingLoanBalance'); if (this.allowAddDisbursementDetails()) { - this.loansAccountTermsForm.removeControl('maxOutstandingLoanBalance'); this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', new UntypedFormControl(this.loansAccountTermsData?.maxOutstandingLoanBalance ?? null, Validators.required) ); } else { this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', new UntypedFormControl(this.loansAccountTermsData?.maxOutstandingLoanBalance ?? null) ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app/core/i18n/i18n.service.ts(1 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(5 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)src/app/web-app.component.ts(1 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.html
- src/app/core/i18n/i18n.service.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/web-app.component.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.tssrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts
🧬 Code graph analysis (2)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)
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)
🔇 Additional comments (9)
src/app/login/login-form/login-form.component.ts (1)
13-13: LGTM! Correct standalone component pattern.The change from importing
MatCheckboxtoMatCheckboxModuleand adding it to the component's imports array follows the correct pattern for Angular standalone components. While this appears unrelated to GLIM functionality, it's a necessary fix for Angular Material to work properly.Also applies to: 34-34
src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts (1)
329-354: Good improvements to response handling and pristine flag placement.The method correctly implements optional response handling with optional chaining and places the
pristine = falseassignment inside the conditional, addressing previous review feedback effectively. The dialog-based edit flow for overdue charges mirrors the existing pattern and integrates well with the component.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (4)
222-226: LGTM! Moratorium fields properly initialized.The moratorium fields are now correctly populated from
loansAccountTermsDatausing nullish coalescing to default to 0, preventing data loss when editing existing loans with moratorium values.
350-351: LGTM! Consistent moratorium field initialization.The moratorium fields are consistently initialized in
ngOnInitusing the same pattern asngOnChanges, ensuring proper data population across all initialization paths.
374-374: LGTM! Explicit return types improve type safety.Adding explicit return type annotations (
booleanforallowAddDisbursementDetailsandvoidfor the setter methods) enhances type safety and makes the method contracts clearer, aligning with TypeScript best practices.Also applies to: 386-386, 405-405
523-525: LGTM! Moratorium form controls properly defined.The new
moratoriumPrincipalandmoratoriumInterestform controls are correctly added to the form definition with appropriate default values, supporting the configurable Moratorium feature for GLIM applications.src/app/web-app.component.ts (1)
124-125: LGTM! Proper dependency injection pattern.The refactoring to inject
I18nServicevia constructor follows Angular best practices and improves testability. The service is now provided in root, making DI the correct approach.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
236-237: Verify deletion of moratorium fields aligns with PR objectives.The PR objectives state "Made Moratorium and Overdue Charges fields configurable and editable in the UI and API payload," but these lines delete
moratoriumPrincipalandmoratoriumInterestfrom the payload. If users can now configure these fields in the UI, deleting them here would lose that input.Please verify:
- Should these fields be included in the payload when set by the user?
- Or are these deletions intentional because GLIM doesn't support moratorium fields?
If moratorium fields should be sent when configured, consider conditionally including them:
-delete data.moratoriumPrincipal; -delete data.moratoriumInterest; +if (!data.moratoriumPrincipal) delete data.moratoriumPrincipal; +if (!data.moratoriumInterest) delete data.moratoriumInterest;
242-266: LGTM! Batching implementation follows PR requirements.The implementation correctly:
- Generates a unique 10-digit
applicationIdshared across all child loans- Sets
isFirstandisLastflags to mark parent account creation and GLIM closure- Chains requests via the
referencefield to preserve processing order- Follows the guidance provided by rhopman in PR comments
This matches the batching requirements described in the PR objectives.
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts
Show resolved
Hide resolved
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
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
efc7a7d to
bd94b43
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-terms-step/loans-account-terms-step.component.ts (1)
196-226: Extract duplicate patchValue logic into a private method.Both
ngOnChanges(lines 196–226) andngOnInit(lines 323–352) contain nearly identical form-patching logic with 25+ overlapping fields. This violates the DRY principle and increases maintenance burden.Refactor by extracting the common logic:
+ private patchLoansAccountTermsForm(): void { + let formattedDate = null; + if (this.loansAccountTermsData.expectedFirstRepaymentOnDate) { + const repaymentDate = new Date(this.loansAccountTermsData.expectedFirstRepaymentOnDate); + formattedDate = this.formatDateToDDMMYYYY(repaymentDate); + } + + this.loansAccountTermsForm.patchValue({ + repaymentsStartingFromDate: this.loansAccountTermsData.expectedFirstRepaymentOnDate && formattedDate, + principalAmount: this.loansAccountTermsData.principal, + loanTermFrequency: this.loansAccountTermsData.termFrequency, + loanTermFrequencyType: this.loansAccountTermsData.termPeriodFrequencyType.id, + numberOfRepayments: this.loansAccountTermsData.numberOfRepayments, + repaymentEvery: this.loansAccountTermsData.repaymentEvery, + repaymentFrequencyType: this.loansAccountTermsData.repaymentFrequencyType.id, + amortizationType: this.loansAccountTermsData.amortizationType.id, + isEqualAmortization: this.loansAccountTermsData.isEqualAmortization, + interestType: this.loansAccountTermsData.interestType.id, + isFloatingInterestRate: this.loansAccountTermsData.isLoanProductLinkedToFloatingRate ? false : null, + interestCalculationPeriodType: this.loansAccountTermsData.interestCalculationPeriodType.id, + allowPartialPeriodInterestCalculation: this.loansAccountTermsData.allowPartialPeriodInterestCalculation, + inArrearsTolerance: this.loansAccountTermsData.inArrearsTolerance, + graceOnPrincipalPayment: this.loansAccountTermsData.graceOnPrincipalPayment, + graceOnInterestPayment: this.loansAccountTermsData.graceOnInterestPayment, + graceOnArrearsAgeing: this.loansAccountTermsData.graceOnArrearsAgeing, + graceOnInterestCharged: this.loansAccountTermsData.graceOnInterestCharged, + fixedEmiAmount: this.loansAccountTermsData.fixedEmiAmount, + maxOutstandingLoanBalance: this.loansAccountTermsData.maxOutstandingLoanBalance, + transactionProcessingStrategyCode: this.loansAccountTermsData.transactionProcessingStrategyCode, + interestRateDifferential: this.loansAccountTermsData.interestRateDifferential, + multiDisburseLoan: this.loansAccountTermsData.multiDisburseLoan, + interestRateFrequencyType: this.loansAccountTermsData.interestRateFrequencyType.id, + balloonRepaymentAmount: this.loansAccountTermsData.balloonRepaymentAmount, + interestRecognitionOnDisbursementDate: + this.loansAccountTermsData.interestRecognitionOnDisbursementDate || false, + moratoriumPrincipal: this.loansAccountTermsData.moratoriumPrincipal ?? 0, + moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 + }); + }Then replace both patchValue blocks:
ngOnChanges() { if (this.loansAccountProductTemplate) { // ... existing setup code ... - this.loansAccountTermsForm.patchValue({ - principalAmount: this.loansAccountTermsData.principal, - // ... 25+ lines ... - moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 - }); + this.patchLoansAccountTermsForm();ngOnInit() { // ... existing setup code ... if (this.loansAccountTermsData) { if (this.loansAccountTermsData.loanProductId) { - let formattedDate = null; - // ... date formatting ... - this.loansAccountTermsForm.patchValue({ - repaymentsStartingFromDate: ..., - // ... 25+ lines ... - moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 - }); + this.patchLoansAccountTermsForm(); }Also applies to: 323-352
♻️ Duplicate comments (3)
src/app/login/login-form/login-form.component.ts (1)
13-13: Technically correct but out of scope (already discussed).The change from
MatCheckboxtoMatCheckboxModuleis the correct approach for standalone components in Angular—you must import modules rather than individual components when using Angular Material. However, as previously noted by alberto-art3ch, this login form change is unrelated to the GLIM loan application fixes that are the focus of this PR. While the author has explained that Angular Material import errors necessitated this fix, consider whether such unrelated changes should be split into a separate PR to keep changes focused and easier to review.Also applies to: 34-34
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
6-7: Use the imported Currency type instead ofany.The
currencyproperty in theGLIMRequestDatainterface is still typed asany, which violates strict type safety guidelines. TheCurrencyinterface is already imported on line 25 fromsrc/app/shared/models/general.model.ts.Apply this diff:
interface GLIMRequestData { - charges: Array<{ chargeId: number; amount: number; currency: any }>; + charges: Array<{ chargeId: number; amount: number; currency: Currency }>; clientId: number;Based on coding guidelines.
278-283: Address type safety in notify method parameters.The
notifymethod still usesanytypes for both parameters, which violates strict type safety guidelines. This issue was flagged in previous reviews but remains unresolved.Define and use proper interfaces:
+interface ErrorBody { + defaultUserMessage: string; + errors?: Array<{ developerMessage: string }>; +} + - notify(body: any, data: any) { + notify(body: ErrorBody, data: GLIMRequestData | GLIMClient): void { let message = body.defaultUserMessage + ' '; body.errors?.forEach((error: any) => (message += error.developerMessage + ' ')); message += 'Data: ' + JSON.stringify(data); console.error(message); }Based on coding guidelines.
🧹 Nitpick comments (2)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
358-370: Remove redundantremoveControlcall.Line 360 removes
maxOutstandingLoanBalanceagain after it was already removed on line 358. This is unnecessary.this.loansAccountTermsForm.removeControl('maxOutstandingLoanBalance'); if (this.allowAddDisbursementDetails()) { - this.loansAccountTermsForm.removeControl('maxOutstandingLoanBalance'); this.loansAccountTermsForm.addControl( 'maxOutstandingLoanBalance', new UntypedFormControl(this.loansAccountTermsData?.maxOutstandingLoanBalance ?? null, Validators.required) );src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
253-253: Consider using a more robust approach for applicationId generation.The current implementation uses
Math.random()to generate a 10-digit applicationId. While collision probability is low, consider using a timestamp-based or UUID approach for better uniqueness guarantees and debuggability.For example:
// Option 1: Timestamp-based (easier to trace in logs) const applicationId = Date.now(); // Option 2: If uniqueness within same millisecond is a concern const applicationId = Date.now() * 1000 + Math.floor(Math.random() * 1000);This makes it easier to correlate application submissions with timestamps in server logs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/app/core/i18n/i18n.service.ts(1 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(4 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(5 hunks)src/app/login/login-form/login-form.component.ts(2 hunks)src/app/web-app.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/core/i18n/i18n.service.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-preview-step/loans-account-preview-step.component.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
- src/app/web-app.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/login/login-form/login-form.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 (1)
src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
src/app/settings/settings.service.ts (1)
dateFormat(111-113)src/app/shared/models/general.model.ts (1)
Currency(3-11)
🔇 Additional comments (6)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (5)
222-226: LGTM: Moratorium fields properly initialized.The moratorium fields are correctly populated from
loansAccountTermsDatawith appropriate fallbacks using nullish coalescing, addressing the previous review feedback.
354-356: Verify the commented-outsetCustomValidators()call.Line 355 has
setCustomValidators()commented out without explanation. This method (lines 386–402) sets required validators forrepaymentFrequencyNthDayTypeandrepaymentFrequencyDayOfWeekTypewhenrepaymentFrequencyType === 2. Commenting it out may prevent proper validation for weekly repayment schedules.Confirm whether:
- This was intentionally disabled to fix a specific issue
- The validation logic is no longer needed
- This should be uncommented
If the validation is still needed, uncomment it:
this.setAdvancedPaymentStrategyControls(); - // this.setCustomValidators(); + this.setCustomValidators(); this.setLoanTermListener();
374-376: Good: Explicit return type improves type safety.Adding the explicit
: booleanreturn type annotation improves code clarity and type safety, aligning with Angular best practices.
386-386: Good: Explicit void return types improve clarity.Adding explicit
: voidreturn type annotations tosetCustomValidators()andsetLoanTermListener()improves code documentation and type safety.Also applies to: 405-405
523-525: LGTM: Moratorium form controls properly defined.The new form controls for
moratoriumPrincipalandmoratoriumInterestare correctly initialized with default values that match the patchValue fallbacks, ensuring consistent behavior.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (1)
237-238: Verify scope: Do PR objectives apply to GLIM accounts or only regular loans?The unconditional deletion of moratorium fields in the
setDatamethod (lines 237-238) is intentional and follows a deliberate filtering pattern. Lines 233-236 also unconditionally delete other GLIM-incompatible fields (principalAmount, allowPartialPeriodInterestCalculation, multiDisburseLoan, isFloatingInterestRate), suggesting this is intentional API payload filtering for the GLIM product model.However, verification shows that:
- The GLIM creation flow DOES reuse
LoansAccountTermsStepComponent, so moratorium fields ARE available in the UI form- Users can enter moratorium values, but they are stripped from the API request
- The original PR objectives state "Made Moratorium and Overdue Charges fields configurable and editable in the UI and API payload"
The concern is: Does this PR scope include GLIM accounts, or only regular loan accounts? If the PR intends to make moratorium fields configurable across all loan types (including GLIM), then this deletion contradicts that objective. If GLIM intentionally excludes moratorium support, then the deletion is correct.
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
bd94b43 to
4a90f4e
Compare
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 Improvements
Other Changes
✏️ Tip: You can customize this high-level summary in your review settings.