WEB-95 Fix problems with GLIM application: saved total, "Moratorium", "Overdue Charges"" #2717
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
GLIM Account Component Refactoring src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts |
Changed setData() to setData(applicationId, client, totalLoan, isFirst, isLast) and simplified charges mapping to {chargeId, amount, currency}; conditionally adds isParentAccount/lastApplication; generates applicationId and isFirst/isLast in buildRequestData(); removed submit(); updated notify(body, data) signature and error payload handling; removed several deprecated/moratorium fields from constructed payload. |
Loan Charges Step Accessibility & Feature 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 |
Templates: added type="button" and aria-label attributes for charge controls and conditional currency symbol rendering. Component: added editOverdueChargeAmount(charge) to open a FormDialog, update overdue charge amount in the data source, and mark form non-pristine. |
Loan Terms Step Accessibility & Form Fields 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 |
Templates: added ARIA labels and changed actionable controls to type="button". Component: added moratoriumPrincipal and moratoriumInterest form controls (default 0) and included them in patch/populate logic. |
Loan Preview Step Template Improvements src/app/loans/loans-account-stepper/loans-account-preview-step/loans-account-preview-step.component.html |
Adjusted name rendering to conditionally show currency symbol in a separate span; guarded several property accesses with optional chaining and fallback defaults for charge/time/type fields. |
Login Form Material Import src/app/login/login-form/login-form.component.ts |
Replaced direct MatCheckbox import with MatCheckboxModule and added it to the component's standalone imports. |
AppModule Service Provider src/app/app.module.ts |
Added I18nService to AppModule providers. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Focus review on:
create-glim-account.component.ts: verify newsetData()payload shape matches backend contract, correct conditional inclusion/removal of moratorium/deprecated fields, andbuildRequestData()applicationId/isFirst/isLast generation and reference chaining.notify()signature change: ensure callers and error handling align with newdataparameter and serialized payload usage.editOverdueChargeAmount()flow and data-source replacement to confirm change-detection and form-pristine semantics.- Form additions (
moratoriumPrincipal,moratoriumInterest) propagation and any serialization/validation impacts.
Possibly related PRs
- WEB-437-fix(charges): resolve chargeId undefined issue breaking disbursement date #2822 — overlaps changes to charges mapping in CreateGlimAccountComponent (chargeId/charge handling).
- WEB-113 GLIM Application Fails to Submit #2662 — modifies the same CreateGlimAccountComponent methods/signatures (buildRequestData, submit/notify/setData).
- WEB-373 Javascript error message opening the first step of Create Loans Account #2744 — touches loans-account-terms-step form initialization and defaults related to moratorium/term fields.
Suggested reviewers
- steinwinde
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly addresses the main objectives: fixing GLIM application issues with saved total, Moratorium, and Overdue Charges, which align with the substantial code changes across multiple components. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent 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/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.html
- src/app/loans/loans-account-stepper/loans-account-charges-step/loans-account-charges-step.component.ts
- src/app/login/login-form/login-form.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/app.module.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 (2)
src/app/clients/create-client/create-client.component.ts (1)
client(90-103)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)
526-528: LGTM!The moratorium form controls are properly defined with sensible default values of
0, and the correspondingpatchValuecalls inngOnChanges(lines 224-225) andngOnInit(lines 353-354) correctly populate them fromloansAccountTermsDatausing nullish coalescing.src/app/loans/glim-account/create-glim-account/create-glim-account.component.ts (2)
184-216: LGTM - Request data construction is correct.The implementation correctly:
- Preserves
currencyin the charges mapping (lines 186-190), fixing the reported TypeError fordisplaySymbol- Conditionally sets
isParentAccount: truefor the first member andlastApplication: truefor the last member- Removes unsupported GLIM fields including moratorium fields
220-238: Batching logic correctly implemented.The batch request construction properly:
- Generates a shared
applicationIdfor all child loans- Sets
referenceto chain requests (null for first, previous index for subsequent)- Determines
isFirst/isLastflags to mark parent account creation and GLIM closureOne minor note:
Math.random()is not cryptographically secure, but for a batch application ID this is acceptable since it only needs to be unique within the request context, not unpredictable.
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 . |
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
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
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
Outdated
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
2 times, most recently
from
4a90f4e to
d2365ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/app/system/codes/view-code/view-code.component.html (1)
30-30: *Add trackBy function to ngFor loop for performance optimization.FormArray iterations should implement a trackBy function to help Angular identify which items have changed, preventing unnecessary re-renders of form controls. This is especially important when dealing with dynamic form arrays.
Add a trackBy method to the component's TypeScript file:
trackByIndex(index: number): number { return index; }Then update the template loop:
- <div formArrayName="codeValues" *ngFor="let codeValue of codeValues.controls; let i = index"> + <div formArrayName="codeValues" *ngFor="let codeValue of codeValues.controls; let i = index; trackBy: trackByIndex">As per coding guidelines, Angular templates should use trackBy on *ngFor loops for performance.
src/app/clients/client-stepper/client-general-step/client-general-step.component.ts (1)
237-247: Addinstanceof Datecheck for consistency and safety.Unlike the other date fields (
submittedOnDate,activationDate,dateOfBirthat lines 227-235),incorpValidityTillDateis formatted without verifying it's aDateinstance. This inconsistency could cause runtime issues if the field is already a string or another type.Apply this diff to align with the established pattern:
- if (generalDetails.clientNonPersonDetails && generalDetails.clientNonPersonDetails.incorpValidityTillDate) { + if (generalDetails.clientNonPersonDetails && generalDetails.clientNonPersonDetails.incorpValidityTillDate instanceof Date) { generalDetails.clientNonPersonDetails = { ...generalDetails.clientNonPersonDetails, incorpValidityTillDate: this.dateUtils.formatDate( generalDetails.clientNonPersonDetails.incorpValidityTillDate, dateFormat ), dateFormat, locale }; }src/app/web-app.component.ts (3)
147-151: Missing subscription cleanup forthemingService.theme.This subscription is not terminated with
takeUntil(this.destroy$), which could cause a memory leak if the component is destroyed. Since you've implemented thedestroy$pattern for other subscriptions, apply it consistently here.ngOnInit() { - this.themingService.theme.subscribe((value: string) => { + this.themingService.theme.pipe(takeUntil(this.destroy$)).subscribe((value: string) => { this.cssClass = value; });
208-215: Missing subscription cleanup foralertService.alertEvent.This subscription persists for the component's lifetime without cleanup, inconsistent with the
takeUntil(this.destroy$)pattern used elsewhere.// Setup alerts - this.alertService.alertEvent.subscribe((alertEvent: Alert) => { + this.alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe((alertEvent: Alert) => { this.snackBar.open(`${alertEvent.message}`, 'Close', {
247-260: Missing subscription cleanup foridle.$onSessionTimeout.This subscription should also use
takeUntil(this.destroy$)for consistent cleanup when the component is destroyed.- this.idle.$onSessionTimeout.subscribe(() => { + this.idle.$onSessionTimeout.pipe(takeUntil(this.destroy$)).subscribe(() => { this.alertService.alert({src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts (2)
280-312: Guard against missingeditCacheentry and remove empty block.Line 284 accesses
editCache[period.period]without verifying the key exists, which could throw if the cache wasn't populated for this period. Additionally, lines 309-310 contain an emptyifblock that appears to be incomplete.editInstallment(period: RepaymentSchedulePeriod): void { if (!period.period) { return; } + if (!this.editCache[period.period]) { + return; + } this.editCache[period.period].edit = true; // ... addDialogRef.afterClosed().subscribe((response: { data?: { value?: Record<string, unknown> } }) => { - if (response.data) { - } + // TODO: Handle dialog result when editing is implemented }); }
336-346: Guard against undefinedperiodwhen populating edit cache.Line 340 uses
item.periodas the cache key, butperiodis optional inRepaymentSchedulePeriod. Ifperiodis undefined, this creates an entry with key"undefined".updateEditCache(): void { if (this.repaymentScheduleDetails?.periods) { this.listOfData = this.repaymentScheduleDetails.periods; this.totalRepaymentExpected = 0; this.listOfData.forEach((item) => { + if (item.period == null) { + return; + } this.editCache[item.period] = { edit: false, data: { ...item } }; this.totalRepaymentExpected = this.totalRepaymentExpected + item.totalDueForPeriod; }); } }src/app/account-transfers/make-account-transfers/make-account-transfers.component.ts (2)
108-131: Wrap switch case declarations in blocks to prevent scope leakage.The
constdeclarations inside the switch case can be erroneously accessed from other cases. Wrap them in a block as flagged by static analysis.case 'fromsavings': - case 'interbank': + case 'interbank': { this.accountTypeId = '2'; this.id = this.route.snapshot.queryParams['savingsId']; this.interbank = this.route.snapshot.queryParams['interbank'] === 'true'; const navigationBalance = this.router.getCurrentNavigation()?.extras?.state?.balance; const templateBalance = this.accountTransferTemplateData?.fromAccount?.availableBalance ?? this.accountTransferTemplateData?.fromAccount?.summary?.accountBalance ?? this.accountTransferTemplateData?.fromAccount?.balance ?? 0; this.balance = typeof navigationBalance === 'number' ? navigationBalance : templateBalance; break; + }
316-327: Clean up subscription to prevent memory leak.The
valueChangessubscription is not cleaned up when the component is destroyed. This can cause memory leaks if the component is recreated multiple times.Consider using
takeUntilDestroyed()from Angular:+import { DestroyRef, inject } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; export class MakeAccountTransfersComponent implements OnInit, AfterViewInit { + private destroyRef = inject(DestroyRef); // ... ngAfterViewInit() { if (!this.interbank && this.makeAccountTransferForm) { - this.makeAccountTransferForm.controls.toClientId.valueChanges.subscribe((value: any) => { + this.makeAccountTransferForm.controls.toClientId.valueChanges + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((value: any) => {src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
196-208: Based on my analysis of the provided code snippet and Angular best practices, I can now provide the rewritten review comment.Key findings:
The code pattern is confirmed: Line 207 shows
isFloatingInterestRatederived solely fromisLoanProductLinkedToFloatingRate, ignoring any existing account value.Angular best practices validate the concern:
patchValueonly updates specified fields, but the current logic unconditionally overwrites this field without checking for an existing account-levelisFloatingInterestRatevalue—a real risk for edit flows.The suggested fix is sound: Using nullish coalescing to prefer the existing loan value aligns with standard Angular patterns for preserving form state during updates.
Limitation: I cannot access the full component file to verify all three line ranges (196-208, 326-337, 501-501) or confirm the exact sequence of
ngOnInit/ngOnChangescalls. However, the pattern shown in the provided snippet is clear and the concern is valid.
Preserve existing
isFloatingInterestRatestate instead of deriving only from product flagIn
patchValue, theisFloatingInterestRateis unconditionally set based onisLoanProductLinkedToFloatingRate, ignoring any existing loan-level value. On edit flows this silently overwrites a previously stored floating vs. fixed choice.Prefer the loan's own value when present, falling back to the product flag only for new loans:
- isFloatingInterestRate: this.loansAccountTermsData.isLoanProductLinkedToFloatingRate ? false : null, + isFloatingInterestRate: + this.loansAccountTermsData.isFloatingInterestRate ?? + (this.loansAccountTermsData.isLoanProductLinkedToFloatingRate ? false : null),Apply the same pattern consistently across all initialization paths (ngOnInit, ngOnChanges, and any other locations where this field is set) so form state is reliably preserved during updates.
Also applies to: 326-337, 501-501
♻️ Duplicate comments (5)
src/app/shared/error-dialog/error-dialog.component.ts (1)
60-62: Fix return type mismatch:sanitize()returnsstring | null, notSafeHtml.
DomSanitizer.sanitize()returnsstring | null, but the getter is declared as returningSafeHtml. This causes a TypeScript compilation error.- get sanitizedData(): SafeHtml { - return this.sanitizer.sanitize(SecurityContext.HTML, this.displayData) || ''; + get sanitizedData(): string { + return this.sanitizer.sanitize(SecurityContext.HTML, this.displayData) ?? ''; }Also update the import to remove unused
SafeHtml:-import { DomSanitizer, SafeHtml } from '@angular/platform-browser'; +import { DomSanitizer } from '@angular/platform-browser';src/app/system/manage-jobs/scheduler-jobs/view-history-scheduler-job/view-history-scheduler-job.component.ts (1)
116-119: The NaN comparison issue from the previous review remains unresolved.The code still parses strings without validating they contain numeric data. When
parseIntencounters a non-numeric string, it returnsNaN, andNaN === NaNevaluates tofalsein JavaScript, causing unexpected filter behavior.Apply the previously suggested fix to validate numeric content before parsing:
-if (filter.value !== '' && val !== '') { - matchFilter.push(parseInt(val.toString(), 10) === parseInt(filter.value, 10)); +if (filter.value !== '' && val !== '') { + const numVal = parseInt(val.toString(), 10); + const numFilter = parseInt(filter.value, 10); + if (!isNaN(numVal) && !isNaN(numFilter)) { + matchFilter.push(numVal === numFilter); + } else { + matchFilter.push(val.toString().toLowerCase().includes(filter.value.toLowerCase())); + } } else if (filter.value === '' || val === '') { matchFilter.push(val.toString().toLowerCase().includes(filter.value.toLowerCase())); }src/app/zitadel/auth.service.ts (1)
78-87: Fix ZITADEL token lifecycle: stale access_token after refresh and insecure refresh_token storageTwo coupled issues around the new token handling:
Stale access_token after refresh (functional bug)
getAccessToken()reads fromsessionStorage['mifosXZitadelTokenDetails'], which is only written inexchangeCodeForTokens.refreshToken()updateslocalStorage(access_token,expires_in, etc.) but never updates the sessionStorage token details.- After the first refresh,
getAccessToken()continues returning the original (now expired) access token, so calls likeuserdetails(),getUsers(),createRole(), etc., will eventually start failing once the initial token expires.Consider keeping a single canonical source of truth. For example, in the refresh path also rebuild and persist the
OAuth2Tokenin sessionStorage:.then((tokens) => {
- if (!tokens || !tokens.access_token || !tokens.expires_in) {
- if (!tokens || !tokens.access_token || !tokens.expires_in) {
throw new Error("The server's response does not contain the expected fields.");
}
- localStorage.setItem('access_token', tokens.access_token);
- localStorage.setItem('access_token', tokens.access_token);
// ... existing localStorage updates ...- const token: OAuth2Token = {
access_token: tokens.access_token,token_type: tokens.token_type,refresh_token: tokens.refresh_token,expires_in: tokens.expires_in,scope: 'Bearer'- };
- sessionStorage.setItem('mifosXZitadelTokenDetails', JSON.stringify(token));
or, alternatively, have `getAccessToken()` prefer the latest `localStorage` value instead of the stale session copy. 2. **refresh_token still stored in localStorage (security, previously flagged)** - `exchangeCodeForTokens` and `refreshToken()` both persist `refresh_token` to `localStorage`, which is directly readable by any injected script (XSS). As earlier tools/comments noted, a long‑lived refresh token in localStorage is a high‑value credential (CWE‑312). - Long‑term, this should be moved to a backend‑managed, Secure, HttpOnly, SameSite cookie (or equivalent server‑side storage), with the SPA only holding short‑lived access data. The refresh flow would then rely on that cookie instead of `localStorage.getItem('refresh_token')`. At minimum, please (a) fix the stale access_token vs. `getAccessToken()` inconsistency, and (b) plan a follow‑up to remove `refresh_token` from localStorage in favor of a cookie‑based or other server‑side approach. Also applies to: 108-153, 319-368 </blockquote></details> <details> <summary>src/app/loans/loans-view/loan-account-actions/loan-reamortize/loan-reamortize.component.ts (1)</summary><blockquote> `44-46`: **Type mismatch between form initialization and template binding still exists.** This issue was previously flagged: the form control is initialized with the full `OptionData` object, but if the template binds `[value]` to `option.id`, Angular Material's `mat-select` won't match the initial value. The fix from the previous review should be applied: ```diff reAmortizationInterestHandling: [ - this.reAmortizationInterestHandlingOptions[0] || null + this.reAmortizationInterestHandlingOptions[0]?.id ?? null ],src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
322-375: Form recreation inngOnInitwipes all patched values (including moratorium fields)
ngOnInit()patches a full set of values intoloansAccountTermsForm(principal, schedule, moratorium fields, etc.), then immediately callsthis.createloansAccountTermsForm();which recreates the form and discards everything just patched. The component then configures strategy controls andmaxOutstandingLoanBalanceon a brand-new, empty form.This breaks both create and edit flows and specifically undoes the newly added moratorium/interestRecognition values.
You should remove the redundant recreation and rely on the form built in the constructor:
if (this.loansAccountTermsData) { // ...patchValue calls including moratorium* this.loansAccountTermsForm.patchValue({ // ... moratoriumInterest: this.loansAccountTermsData.moratoriumInterest ?? 0 }); } - this.createloansAccountTermsForm(); this.setAdvancedPaymentStrategyControls();This also aligns with the earlier review feedback that flagged this as destructive.
🧹 Nitpick comments (55)
src/app/organization/bulk-import/view-bulk-import/view-bulk-import.component.scss (1)
23-23: Consider responsive min-height for improved mobile UX.The fixed
min-height: 400pxonmat-cardmay create excessive vertical space on mobile devices, especially when the parent container (gap-2percent) switches toflex-direction: columnbelow 768px. This could result in poor visual hierarchy and unnecessary scrolling on smaller viewports.Consider using a responsive approach:
- Apply
min-height: 400pxonly above the 768px breakpoint, or- Use a percentage-based or viewport-relative height that adapts to screen size.
Apply this diff to make the min-height responsive:
mat-card { flex: 1; padding: 24px; display: flex; flex-direction: column; border-radius: 20px; - min-height: 400px; + min-height: auto; + + @media (width > 768px) { + min-height: 400px; + }src/app/organization/offices/view-office/view-office.component.html (1)
15-27: *AddtrackByfunction to improve ngFor performance.The dynamic datatable tabs use *ngFor without a trackBy function. Angular will destroy and recreate DOM nodes unnecessarily on array mutations, degrading performance and causing visual jank.
Add a trackBy function to the component and bind it to the *ngFor. Assuming
officeDatatablehas a stable identifier likeregisteredTableName:<span *ngFor="let officeDatatable of officeDatatables"> + *ngFor="let officeDatatable of officeDatatables; trackBy: trackByDatatableName"Then in the component class, define:
trackByDatatableName(index: number, datatable: any): string { return datatable.registeredTableName; }As per coding guidelines, trackBy is essential for Angular performance on *ngFor loops.
src/app/tasks/checker-inbox-and-tasks-tabs/reschedule-loan/reschedule-loan.component.ts (1)
99-103: Route data key now correctly matches resolverUsing
data.rescheduleLoansDatahere aligns the component with the updated route resolve key and fixes the previous typo-driven mismatch; the initialization ofloans,dataSource, andselectionremains correct.You might later consider tightening the type of
rescheduleLoansDataandloansinstead ofany, but that’s optional and not required for this fix.src/app/system/manage-data-tables/column-dialog/column-dialog.component.html (2)
21-21: AddtrackByfunctions to both*ngForloops for performance optimization.Per Angular best practices and the project's coding guidelines, both loops should use
trackByto prevent unnecessary DOM re-rendering and improve performance, especially if the data list changes:
- Line 21:
columnTypeDataloop- Line 45:
columnCodesloopTo implement, add trackBy methods in the component class:
trackByColumnTypeValue(_index: number, item: any): any { return item.value; } trackByColumnCodeName(_index: number, item: any): string { return item.name; }Then update the template:
- <mat-option *ngFor="let columnType of columnTypeData" [value]="columnType.value"> + <mat-option *ngFor="let columnType of columnTypeData; trackBy: trackByColumnTypeValue" [value]="columnType.value"> - <mat-option *ngFor="let columnCode of data.columnCodes" [value]="columnCode.name"> + <mat-option *ngFor="let columnCode of data.columnCodes; trackBy: trackByColumnCodeName" [value]="columnCode.name">Also applies to: 45-45
31-40: Consider adding validation error display for Column Length field.The Column Length field is marked as
requiredbut lacks an error display block. For consistency with Column Name (lines 12–15) and Column Type (lines 25–28), consider adding:<mat-form-field *ngIf="columnForm.value.type === 'String'"> <mat-label>{{ 'labels.inputs.Column Length' | translate }}</mat-label> <input matInput required type="number" formControlName="length" placeholder="{{ 'labels.inputs.Column Length' | translate }}" /> + <mat-error *ngIf="columnForm.controls.length.hasError('required')"> + {{ 'labels.inputs.Column Length' | translate }} {{ 'labels.commons.is' | translate }} + <strong>{{ 'labels.commons.required' | translate }}</strong> + </mat-error> </mat-form-field>This ensures users receive clear feedback if they leave the field empty.
src/app/pipes/url-to-string.pipe.spec.ts (2)
23-33: Align test description with actual capitalization behaviorThe
"should capitalize first letter of each word"description suggests word-level capitalization, but the expectations (e.g.,'Users management'for/users%20management) reflect only the first character of the segment being capitalized, not each word. Consider either:
- Renaming the test to describe “segment capitalization”, or
- Adjusting pipe behavior and expectations if true word-level capitalization is desired.
This is purely about clarity of intent in tests, not a functional bug.
63-69: Revisit expectations for trailing separators and empty segmentsThe cases:
'/users/' -> 'Users | ''/users//accounts' -> 'Users | | Accounts'lock in behavior that surfaces empty breadcrumb segments and trailing
" | ". That may not be ideal in the UI and could make it harder to later change the pipe to skip empty segments or trim trailing separators.If the intended UX is to hide empty segments and avoid trailing delimiters, consider updating both the pipe and these expectations to something like:
- expect(pipe.transform('/users/')).toBe('Users | '); + expect(pipe.transform('/users/')).toBe('Users'); - expect(pipe.transform('/users//accounts')).toBe('Users | | Accounts'); + expect(pipe.transform('/users//accounts')).toBe('Users | Accounts');Otherwise, a brief comment explaining why this behavior is intentional would help future maintainers.
src/app/navigation/navigation.component.scss (1)
37-44: Simplify redundant media query rules.Lines 37–39 and 41–43 both set
grid-template-columns: 50% 50%. Since the first rule applies at ≥768px, it will already take effect at 1200px unless explicitly overridden. Remove the duplicate at line 41–43 or consolidate if there are other intended differences.@media (width >= 768px) { grid-template-columns: 50% 50%; } - @media (width >= 1200px) { - grid-template-columns: 50% 50%; + @media (width >= 1200px) { gap: 1rem; }src/app/navigation/office-navigation/office-navigation.component.scss (1)
40-40: Use modernoverflow-wrapinstead of deprecatedword-wrap.Line 40 uses
word-wrap: break-word;, which is deprecated. Replace with the modernoverflow-wrap: break-word;property for better alignment with current CSS standards..flex-50 { padding: 0.625rem 0; display: flex; align-items: center; font-size: 0.875rem; - word-wrap: break-word; + overflow-wrap: break-word; line-height: 1.6;src/app/system/manage-jobs/scheduler-jobs/view-history-scheduler-job/view-history-scheduler-job.component.ts (1)
54-54: Consider adding type definitions for improved type safety.The component uses
anytype in multiple places (jobHistoryData, filter predicate parameters), which reduces type safety and IDE support. As per coding guidelines, strict type safety is recommended for Angular code.Consider defining interfaces for the data structures:
interface SchedulerJobHistoryItem { version: number; run_start_time: string; run_end_time: string; status: string; run_type: string; jobRunErrorLog?: string; [key: string]: any; } interface FilterCriteria { id: string; value: string; }Then update the types:
-jobHistoryData: any; +jobHistoryData: { pageItems: SchedulerJobHistoryItem[] }; -this.dataSource.filterPredicate = (data: any, filtersJson: string) => { - const matchFilter: any[] = []; +this.dataSource.filterPredicate = (data: SchedulerJobHistoryItem, filtersJson: string) => { + const matchFilter: boolean[] = []; const filters = JSON.parse(filtersJson); - filters.forEach((filter: any) => { + filters.forEach((filter: FilterCriteria) => {Also applies to: 111-114
src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts (3)
155-171: System-column detection logic looks correct; consider a small defensive guard.Marking
id/created_at/updated_atplus the relationship column assystemusingrelationshipColumnNameis sound and will correctly protect those fields in the UI.If there is any chance
this.dataTableData.applicationTableNamecan be missing or malformed, you might optionally add a short guard before callinggetRelationshipColumnName(or allow the helper to acceptstring | undefined) to keep the typing fully honest, but this is non-blocking.
175-195: NewgetRelationshipColumnNamehelper is clear; mapping could be centralized for maintainability.The helper cleanly encapsulates the table→relationship-column mapping and makes the constructor logic much easier to read; returning
''as a safe default ensures no column is incorrectly marked as system when there’s no mapping.If this mapping is or becomes needed elsewhere, consider extracting it to a shared constant (e.g., in a small
datatable-relationshipsconfig module) so additions/new tables stay in sync across the app.
197-198: Conditional removal of theidcolumn is safer; you can further decouple from column ordering.The updated comment and check so that
shift()only runs when the first column is actually'id'avoids the previous “always drop first column” behavior and prevents accidentally hiding a non-idfield — good change.To fully match the comment “Remove the 'id' column if it exists”, you might optionally search for any column with
columnName === 'id'and splice that entry, instead of assuming it must be at index 0. That would make the logic robust against any future backend changes in column ordering while still keeping the relationship column visible.Also applies to: 221-227
src/app/core/shell/sidenav/sidenav.component.ts (1)
60-60: Consider tightening type declaration to match defensive null-check.The optional chaining on line 135 suggests
userActivitycan be null (which is correct sinceJSON.parse(localStorage.getItem(...))returns null for missing items), but the type declaration on line 60 isstring[]rather thanstring[] | null.As per coding guidelines, strict type safety for Angular code.
Apply this diff to align the type with runtime behavior:
- userActivity: string[]; + userActivity: string[] | null;Also applies to: 135-135
src/app/products/charges/create-charge/create-charge.component.ts (1)
78-84: Defensive handling of account options looks good.The fallback to empty arrays and conditional concatenation correctly handles scenarios where liability options may be absent or empty. The logic ensures
incomeAndLiabilityAccountDatais always defined and contains the appropriate combined options.Optional simplification:
Since
incomeOptions.concat([])produces the same result asincomeOptions, the conditional check could be simplified to always concatenate:- const incomeOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.incomeAccountOptions || []; - const liabilityOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.liabilityAccountOptions || []; - if (liabilityOptions.length > 0) { - this.incomeAndLiabilityAccountData = incomeOptions.concat(liabilityOptions); - } else { - this.incomeAndLiabilityAccountData = incomeOptions; - } + const incomeOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.incomeAccountOptions || []; + const liabilityOptions = data.chargesTemplate.incomeOrLiabilityAccountOptions.liabilityAccountOptions || []; + this.incomeAndLiabilityAccountData = incomeOptions.concat(liabilityOptions);However, the current approach is clear and avoids an unnecessary array operation when liability options are empty, so it's acceptable as-is.
src/app/organization/holidays/create-holiday/create-holiday.component.ts (1)
335-342: Date formatting is correct; consider guarding for non‑Date values for robustnessUsing
formatDateAsStringhere aligns create‑holiday with the expected backend dateFormat/locale contract. To make this more robust (and consistent withEditHolidayComponent), you could only format when the value is aDateinstance and otherwise leave existing strings untouched:- const prevFromDate: Date = this.holidayForm.value.fromDate; - const prevToDate: Date = this.holidayForm.value.toDate; - holidayFormData.fromDate = this.dateUtils.formatDateAsString(prevFromDate, dateFormat); - holidayFormData.toDate = this.dateUtils.formatDateAsString(prevToDate, dateFormat); + const prevFromDate = this.holidayForm.value.fromDate; + const prevToDate = this.holidayForm.value.toDate; + if (prevFromDate instanceof Date) { + holidayFormData.fromDate = this.dateUtils.formatDateAsString(prevFromDate, dateFormat); + } + if (prevToDate instanceof Date) { + holidayFormData.toDate = this.dateUtils.formatDateAsString(prevToDate, dateFormat); + } if (this.holidayForm.contains('repaymentsRescheduledTo')) { - const prevRepaymentsRescheduledTo: Date = this.holidayForm.value.repaymentsRescheduledTo; - holidayFormData.repaymentsRescheduledTo = this.dateUtils.formatDateAsString( - prevRepaymentsRescheduledTo, - dateFormat - ); + const prevRepaymentsRescheduledTo = this.holidayForm.value.repaymentsRescheduledTo; + if (prevRepaymentsRescheduledTo instanceof Date) { + holidayFormData.repaymentsRescheduledTo = this.dateUtils.formatDateAsString( + prevRepaymentsRescheduledTo, + dateFormat + ); + } }This avoids accidental double‑formatting if the form is ever populated with pre‑stringified dates.
src/app/organization/holidays/edit-holiday/edit-holiday.component.ts (1)
137-151: Robust date serialization for edit flow looks goodConditionally formatting
fromDate,toDate, andrepaymentsRescheduledToonly when they areDateinstances is a solid approach and prevents double‑formatting of already‑string values, while keeping behavior scoped to non‑active holidays andreSchedulingType === 2. If you find yourself touching these flows again, you might consider a small shared helper for this pattern to keep create/edit holiday logic in sync, but it’s not necessary for this PR.src/app/system/audit-trails/view-audit/view-audit.component.ts (1)
83-101: Robust JSON parsing forauditTrailCommandsDatais a solid improvementThe added null checks, guarded
JSON.parse, and shape validation (non‑array object only) eliminate crashes on invalid or missingcommandAsJsonand safely fall back to an empty list. This is a good defensive change. If this getter ever ends up used directly in the template, consider caching the parsed result in a field duringngOnInitto avoid repeated parsing, but that’s optional.src/app/organization/loan-provisioning-criteria/view-loan-provisioning-criteria/view-loan-provisioning-criteria.component.ts (1)
118-125: Consider enhancing error handling for better user experience.The error handling logs to console but provides no user feedback on deletion failure. Consider displaying a snackbar or alert to inform users of the error.
Example enhancement:
this.organizationService.deleteProvisioningCriteria(this.provisioningData.criteriaId).subscribe( () => { this.router.navigate(['/organization/provisioning-criteria']); }, (error) => { - console.error('Failed to delete provisioning criteria:', error); + console.error('Failed to delete provisioning criteria:', error); + // Add user notification, e.g., using MatSnackBar: + // this.snackBar.open('Failed to delete provisioning criteria', 'Close', { duration: 3000 }); } );src/app/organization/loan-provisioning-criteria/create-loan-provisioning-criteria/create-loan-provisioning-criteria.component.ts (1)
240-247: Consider adding type annotation for the products variable.While the local variable extraction improves readability, adding an explicit type would enhance type safety and make the code more maintainable.
Apply this diff:
- const products = this.provisioningCriteriaForm.get('loanProducts').value; + const products: LoanProduct[] = this.provisioningCriteriaForm.get('loanProducts').value; const loanProvisioningCriteria = { ...this.provisioningCriteriaForm.value, - loanProducts: products.map((product: any) => ({ + loanProducts: products.map((product) => ({ id: product.id, name: product.name, includeInBorrowerCycle: product.includeInBorrowerCycle })),Note: You would need to import
LoanProductfrom the appropriate model file.src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts (4)
18-19: Replaceanytypes with proper interfaces for type safety.The mock router and test data are typed as
any, which bypasses TypeScript's type checking. This can hide potential type mismatches between test data and actual component expectations.Consider defining explicit interfaces:
+interface TaxComponentData { + id: number; + name: string; + percentage?: number; + startDate?: number[] | string; + creditAccount?: { + id: number; + name: string; + glCode: string; + }; + creditAccountId?: number; + creditAccountName?: string; + creditAccountType?: { + id: number; + code: string; + value: string; + }; +} + let component: ViewTaxComponentComponent; let fixture: ComponentFixture<ViewTaxComponentComponent>; -let mockRouter: jest.Mocked<Router>; -let routeDataSubject: BehaviorSubject<any>; +let mockRouter: jest.Mocked<Pick<Router, 'navigate'>>; +let routeDataSubject: BehaviorSubject<{ taxComponent: TaxComponentData }>; -const mockTaxComponentData: any = { +const mockTaxComponentData: TaxComponentData = { id: 1, name: 'VAT',Also applies to: 21-42
113-117: Verify behavior after change detection.This test accesses component properties without calling
fixture.detectChanges(), which means it's testing the initial state before Angular's lifecycle hooks run. While this may work if the component assigns data synchronously in the constructor, it's more reliable to test after the component has fully initialized.Consider adding
fixture.detectChanges()for consistency:it('should subscribe to route data on initialization', () => { + fixture.detectChanges(); expect(component.taxComponentData).toBeDefined(); expect(component.taxComponentData.id).toBe(1); expect(component.taxComponentData.name).toBe('VAT'); });
238-349: Verify graceful handling of edge cases, not just data assignment.These edge case tests verify that data is correctly assigned to
component.taxComponentData, but they don't verify that the component handles these cases gracefully (e.g., no template errors, proper rendering, appropriate fallbacks).Add
fixture.detectChanges()and verify no errors are thrown during rendering:it('should handle tax component with null credit account', () => { const taxWithNullAccount: any = { id: 1, name: 'Tax Without Account', percentage: 10, creditAccount: null }; routeDataSubject.next({ taxComponent: taxWithNullAccount }); expect(component.taxComponentData.creditAccount).toBeNull(); + expect(() => { + fixture.detectChanges(); + }).not.toThrow(); });Apply similar changes to other edge case tests (empty object, undefined properties, negative ID, etc.) to verify the component renders without errors.
528-542: Test name doesn't match what it actually verifies.This test is named "should not create memory leaks" but it only verifies that the component can handle 100 sequential updates and ends with the correct final state. It doesn't actually detect memory leaks (which would require measuring heap size, verifying cleanup, or checking for retained references).
Either rename the test to match what it does, or implement actual memory leak detection:
Option 1: Rename to match actual behavior
-it('should not create memory leaks with multiple updates', () => { +it('should handle many sequential data updates correctly', () => {Option 2: Add actual memory verification
If you want to test memory leaks, verify subscriptions don't accumulate:it('should not create memory leaks with multiple updates', () => { fixture.detectChanges(); // Track subscription count if component exposes subscriptions const getActiveSubscriptions = () => { // Implementation depends on component structure }; for (let i = 0; i < 100; i++) { routeDataSubject.next({ taxComponent: { id: i, name: `Tax ${i}` } }); } // Verify only one subscription remains active expect(getActiveSubscriptions()).toBe(1); });package.json (1)
57-57: Chart.js version is current and recommendation is valid but optional.The web search confirms that v4.5.1 (released Oct 13, 2025) is indeed the latest stable Chart.js v4 version. The review comment is factually accurate regarding the version being current.
The recommendation to use exact version pinning is a reasonable best practice for major libraries to prevent unexpected minor/patch updates that could introduce subtle breaking changes. However, since the current
^4.5.1range only allows minor and patch updates within v4 (and breaking changes typically require a major version bump to v5), this is more of an optional best practice rather than a critical issue. The current version specification is acceptable from a stability standpoint.src/app/core/http/error-handler.interceptor.ts (1)
15-15: Align translation usage across all alert messages for consistencyInjecting
TranslateServiceand usingthis.translate.instant('error.resource.not.found')for the 404 alert type is fine, but it now makes that case the only one localized while othertype/messagestrings remain hard-coded English. Consider adopting a consistent strategy (e.g., all alerttype/messagevalues come from translation keys, or translation happens inside the alert component/service) so the UX isn’t partially localized.Please verify that:
- The
error.resource.not.foundkey exists in all supported locales.TranslateServiceis available in the same injector hierarchy as this interceptor (noNullInjectorErrorat runtime).Also applies to: 31-31, 80-83
src/app/organization/entity-data-table-checks/entity-data-table-checks.component.ts (1)
128-138: Map-based entity lookup is correct and more efficientThe new
setEntity()implementation usingMap<string, string>is functionally correct, avoids nested iteration, and respects the existing call order (runs aftersetEntityDataTableChecks, sodataSourceis initialized). No correctness issues.As an optional improvement, you could:
- Hoist
entityMapto a readonly class property (or a plain object literal) to avoid rebuilding it on everysetEntity()call.- Replace some
anyuses with a typed interface for the table rows to strengthen type safety.src/app/loans/loans.service.ts (1)
244-260: Consider improving type safety and parameter building.The method correctly implements the re-age preview endpoint and filters out empty values before building the request. However, a few refinements could improve maintainability:
Type safety: Replace
Observable<any>with a specific return type (e.g.,Observable<RepaymentSchedule>or create an interface for the preview response).Parameter building: The current approach with
.toString()works for primitives but could be more explicit:getReAgePreview(loanId: string, data: any): Observable<RepaymentSchedulePreview> { let httpParams = new HttpParams(); Object.entries(data).forEach(([key, value]) => { if (value != null && value !== '') { httpParams = httpParams.set(key, String(value)); } }); return this.http.get<RepaymentSchedulePreview>(`/loans/${loanId}/transactions/reage-preview`, { params: httpParams }); }The current implementation is functional but these changes would align with Angular best practices for type safety.
src/app/zitadel/callback/callback.component.ts (1)
2-3: Consider falling back to thecodequery param ifauth_codeis missingThe new sessionStorage‑based flow with immediate cleanup and navigation on error looks good. To make the callback more robust (e.g., if the index.html bootstrap or storage write fails), consider also checking
this.route.snapshot.queryParamMap.get('code')as a fallback whensessionStorage.getItem('auth_code')is null, so valid OIDC callbacks don’t end up as no‑ops.Also applies to: 10-34
src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts (1)
4-8: Re‑aging data preparation and preview flow are cleanly implementedCentralizing date/interest‑handling normalization in
prepareReagingData()and reusing it for bothpreview()andsubmit()keeps the API payloads consistent; the MatDialog‑based preview with currency fallback from response or route data is also sensibly guarded when no currency is available. Consider, as a later enhancement, surfacing API errors to the user instead of only logging them.Also applies to: 11-12, 37-47, 58-62, 93-140, 142-155
src/app/loans/loans-account-stepper/loans-account-schedule-step/loans-account-schedule-step.component.ts (1)
59-61: Consider adding error handling for the subscription.If
calculateLoanSchedulefails, the UI may remain in an indeterminate state withrepaymentScheduleDetailsstillnull. Adding error handling would improve user experience.- this.loansService.calculateLoanSchedule(payload).subscribe((response: RepaymentSchedule) => { - this.repaymentScheduleDetails = response; - }); + this.loansService.calculateLoanSchedule(payload).subscribe({ + next: (response: RepaymentSchedule) => { + this.repaymentScheduleDetails = response; + }, + error: (err) => { + console.error('Failed to calculate loan schedule', err); + // Optionally show user-facing error message + } + });src/app/home/timeout-dialog/idle-timeout.service.ts (2)
3-4: Unused RxJS operator imports.
switchMap,takeUntil,tap, andmapare imported but not used in the current implementation. Consider removing unused imports to keep the code clean.-import { switchMap, takeUntil, tap, map } from 'rxjs/operators';
36-54: Consider throttling user activity events.High-frequency events like
mousemovewill callreset()very frequently, which recreates the timer subscription each time. For better performance, consider throttling the user actions:+import { throttleTime } from 'rxjs/operators'; + // Subscribe to user actions only when active const events = [ 'mousemove', 'keydown', 'wheel', 'mousedown', 'scroll' ]; const userActions$ = merge(...events.map((e) => fromEvent(document, e))); -this.userActionsSubscription = userActions$.subscribe(() => { +this.userActionsSubscription = userActions$.pipe(throttleTime(1000)).subscribe(() => { this.reset(); });src/app/loans/loans-view/loan-account-actions/edit-repayment-schedule/edit-repayment-schedule.component.ts (2)
178-182: MovewasValidatedassignment outside the loop.Setting
wasValidated = trueinside theforEachloop is redundant since it's set to the same value on each iteration. Move it after the loop for clarity.response.periods.forEach((period: EditablePeriod) => { period.changed = true; this.repaymentScheduleDetails!.periods.push(period); - this.wasValidated = true; }); + this.wasValidated = true;
207-210: Consider usingObject.values()for cleaner iteration.
Object.values()is more concise and directly expresses intent.- const modifiedinstallments: ScheduleChangeRecord[] = []; - Object.keys(this.repaymentScheduleChanges).forEach((key: string) => { - modifiedinstallments.push(this.repaymentScheduleChanges[key]); - }); + const modifiedinstallments: ScheduleChangeRecord[] = Object.values(this.repaymentScheduleChanges);src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts (1)
233-248: Side effect inisCurrentmethod reassignsbusinessDate.Line 237 reassigns
this.businessDateinside what appears to be a read-only check method. This side effect may be intentional to refresh the business date, but it reduces method predictability. Consider initializingbusinessDateat call sites or using a local variable.isCurrent(installment: RepaymentSchedulePeriod): string { if (!installment.fromDate) { return ''; } else { - this.businessDate = this.settingsService.businessDate; + const businessDate = this.settingsService.businessDate; const fromDate = this.dateUtils.parseDate(installment.fromDate); const dueDate = this.dateUtils.parseDate(installment.dueDate); - if (fromDate <= this.businessDate && this.businessDate < dueDate) { + if (fromDate <= businessDate && businessDate < dueDate) { return 'current'; } - if (this.businessDate > dueDate) { + if (businessDate > dueDate) { return 'overdued'; } } return ''; }src/app/pipes/find.pipe.ts (1)
20-40: Consider stricter type annotations.The caching logic is sound and provides O(1) lookups. However, the
options: anyparameter could be more specific asoptions: any[]since the code expects an array (line 32 check).Apply this diff to improve type safety:
- transform(value: any, options: any, key: string, property: string): string { + transform(value: any, options: any[], key: string, property: string): string {Also consider returning early if options is not an array to make the behavior more explicit:
let valueMap = keyMap.get(key); if (!valueMap) { // Build the lookup map: O(n) - but only once per options array + key combination valueMap = new Map<any, any>(); if (Array.isArray(options)) { for (const option of options) { if (option && option[key] !== undefined && option[key] !== null) { valueMap.set(option[key], option); } } + } else { + // Non-array options won't be processed + console.warn('FindPipe: options must be an array'); } keyMap.set(key, valueMap); }src/app/shared/theme-toggle/theme-toggle.component.ts (1)
26-32: Boolean coercion is fine; ngOnChanges likely unnecessary without @input().Using
!!this.settingsService.themeDarkEnabledcorrectly normalizes the value to a boolean given the updatedSettingsServiceAPI. However,ThemeToggleComponentcurrently has no@Input()bindings, songOnChangeswill not fire and just duplicates the initialization logic fromngOnInit.You can simplify by dropping
OnChanges/ngOnChangesentirely, or introduce an@Input()if you intend to react to external changes. This is optional but reduces noise and confusion around lifecycle hooks.src/app/settings/settings.service.ts (1)
244-252: Correct use of dateFormat getter; consider a safe default.Switching from
this.dateFormat()tothis.dateFormatis consistent with the getter-based API and avoids calling a non-function, so this change is correct.Separately,
dateFormatcurrently deserializes fromlocalStorageviaJSON.parse(localStorage.getItem('mifosXDateFormat')). If that key is ever missing or invalid JSON, this will throw and break the alert construction here. Consider returning a safe default (e.g.SettingsService.businessDateFormat) when the stored value is absent or malformed.src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.ts (1)
113-122: Defensive officer template handling is soundFiltering
toLoanOfficersfromfromLoanOfficerswith optional chaining and only callinggetOfficerTemplatewhen bothofficerIdandofficeTemplate.idexist avoids the undefined access/runtime error paths described in the PR. ClearingofficerTemplatein theelsebranch is also a safe fallback when selection is reset.You might optionally also reset the
toLoanOfficerIdform control whenofficerIdchanges or becomes falsy to avoid a stale selection that no longer exists intoLoanOfficers.src/app/organization/fund-mapping/fund-mapping.component.ts (1)
241-254: Consider validating before collapsing and formatting
submit()now guards onthis.fundMappingForm.invalid, which is good. However, the guard currently runs afterthis.isCollapsed = trueand after some date handling. For clearer behavior and to avoid collapsing the parameters view on an invalid submit (even if the button is normally disabled), consider:
- Checking
this.fundMappingForm.invalidand marking controls as touched first.- Only then setting
this.isCollapsed = trueand formatting dates.This keeps the side effects strictly behind a valid form.
src/app/clients/edit-client/edit-client.component.html (1)
342-347: Align submit handling with other reactive formsThe form currently relies on
(click)="submit()"on the primary button, with the form itself not listening to(ngSubmit). While this works, other components in the codebase tend to use(ngSubmit)="submit()"on the<form>and keep the button as a plaintype="submit"without a click handler.For consistency and clearer semantics you could:
<form [formGroup]="editClientForm" (ngSubmit)="submit()"> ... <button type="submit" mat-raised-button color="primary" [disabled]="!editClientForm.valid"> {{ 'labels.buttons.Submit' | translate }} </button> </form>This centralizes submission logic and ensures the same behavior for keyboard and pointer activation.
src/app/organization/bulk-loan-reassignmnet/bulk-loan-reassignmnet.component.html (1)
86-96: Loan checkbox change handlers match the TS API; consider tightening removal logicWiring both client and group loan checkboxes to
(change)="getLoans($event, loans.id)"is consistent with the existinggetLoans(event, loanId)method and enables collecting selected loan IDs from both sections.In
getLoans(TS), unselecting currently does:const index = this.loans.indexOf(loanId, 0); this.loans.splice(index, 1);If for any reason
loanIdis not found,indexOfreturns-1and the last element inthis.loanswill be removed instead.To make this robust, you could guard the index:
const index = this.loans.indexOf(loanId); if (index > -1) { this.loans.splice(index, 1); }or use a filter-based approach.
Also applies to: 112-117
src/app/accounting/chart-of-accounts/create-gl-account/create-gl-account.component.ts (1)
163-166: Consider marking form controls as touched before returning.The early return prevents submission when the form is invalid, but the user won't see validation error messages unless controls are marked as touched. This could leave users confused about why submission doesn't work.
submit() { if (this.glAccountForm.invalid) { + this.glAccountForm.markAllAsTouched(); return; }src/app/account-transfers/make-account-transfers/make-account-transfers.component.scss (1)
206-268: Redundant dark theme selectors with overlapping specificity.The dark theme styles are defined three times with increasing specificity:
.dark-theme(line 208),:host-context(.dark-theme)(line 252), andbody.dark-theme(line 271). This causes maintenance overhead and potential conflicts. Consider consolidating to a single approach.src/app/account-transfers/make-account-transfers/make-account-transfers.component.html (2)
117-119: AddtrackByfunction to*ngForloops.Per Angular best practices,
*ngForshould include atrackByfunction to optimize change detection. This applies to all*ngForloops in this template (lines 117, 143, 151, 164).Example for the office select:
-<mat-option *ngFor="let toOfficeType of toOfficeTypeData" [value]="toOfficeType.id"> +<mat-option *ngFor="let toOfficeType of toOfficeTypeData; trackBy: trackById" [value]="toOfficeType.id">Add to the component class:
trackById(index: number, item: any): number { return item.id; }
319-339: Consolidate duplicate submit buttons.The two submit buttons are identical except for the
*ngIfcondition. They can be merged into a single button.- <button - *ngIf="interbank && interbankTransferForm" - mat-raised-button - color="primary" - [disabled]="!makeAccountTransferForm.valid" - (click)="submit()" - class="primary-button" - > - {{ 'labels.buttons.Submit' | translate }} - </button> - - <button - *ngIf="!interbank" - mat-raised-button - color="primary" - [disabled]="!makeAccountTransferForm.valid" - (click)="submit()" - class="primary-button" - > - {{ 'labels.buttons.Submit' | translate }} - </button> + <button + *ngIf="(interbank && interbankTransferForm) || !interbank" + mat-raised-button + color="primary" + [disabled]="!makeAccountTransferForm.valid" + (click)="submit()" + class="primary-button" + > + {{ 'labels.buttons.Submit' | translate }} + </button>src/app/account-transfers/make-account-transfers/make-account-transfers.component.ts (3)
106-107: Remove duplicate comment.Lines 106-107 have the same comment duplicated.
- /** Sets the value from the URL */ /** Sets the value from the URL */ setParams() {
145-148: Use English for comments and translation keys for user-facing text.The comment on line 146 is in Spanish. For consistency, use English comments. Also, the default transfer description on line 262 (
'Transferencia interbancaria') should use a translation key instead of hardcoded Spanish text./** - * Crea un formulario vacío para interbank mientras se carga + * Creates an empty form for interbank while loading */ createEmptyInterbankForm() {
410-421: Use observer object syntax for subscribe.The separate callback parameters for
subscribe()are deprecated in newer RxJS. Use the observer object syntax for consistency and future compatibility.this.accountTransfersService.sendInterbankTransfer(JSON.stringify(payload)).subscribe( - (trnsfr) => { + { + next: (trnsfr) => { if (trnsfr.systemMessage) { this.isLoading = false; this.router.navigate(['../../transactions'], { relativeTo: this.route }); } }, - (error) => { + error: (error) => { console.error('Interbank transfer error:', error); this.isLoading = false; } + } );src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
222-225: Moratorium andmaxOutstandingLoanBalancehandling looks consistent; confirm 0 vs “unset” semanticsThe new moratorium fields and
interestRecognitionOnDisbursementDateare now:
- Included in both
ngOnChangesandngOnInitpatch flows.- Backed by form controls with sensible defaults (
falseand0).- Returned via
getRawValue()so they’ll reach the payload.maxOutstandingLoanBalancenow uses?.maxOutstandingLoanBalance ?? nullin both branches, avoidingundefinedand keeping the required/optional behavior consistent.This wiring looks coherent. The main remaining behavioral question is whether the backend distinguishes between “field absent” and explicit
0for moratorium values; if it does, you may want to default tonullinstead of0for truly unset values and only coerce to0when the user actually interacts with the field.If the API treats
0and “not provided” equivalently, the current implementation is fine and improves robustness around missing data.Also applies to: 293-303, 351-354, 362-373, 526-528
src/app/organization/offices/view-office/general-tab/general-tab.component.html (2)
22-27: Simplify External Id conditional rendering to reduce duplication.The External Id field repeats the div structure and label logic in two *ngIf branches. This can be consolidated into a single conditional that renders the value or fallback text.
Consider refactoring to:
<div class="flex-45 mat-body-strong left">{{ 'labels.inputs.External Id' | translate }}</div> - <div class="flex-50 right" *ngIf="officeData.externalId"> - <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> - </div> - <div class="flex-50 right" *ngIf="!officeData.externalId"> - {{ 'labels.inputs.Unassigned' | translate }} - </div> + <div class="flex-50 right"> + <mifosx-external-identifier + *ngIf="officeData.externalId" + externalId="{{ officeData.externalId }}"> + </mifosx-external-identifier> + <span *ngIf="!officeData.externalId"> + {{ 'labels.inputs.Unassigned' | translate }} + </span> + </div>
11-11: Translate hardcoded "Unassigned" text for consistency.Line 11 has hardcoded 'Unassigned' while line 26 uses the translation key 'labels.inputs.Unassigned'. For consistency, line 11 should also use the translation pipe.
Apply this diff:
- {{ officeData.openingDate ? (officeData.openingDate | dateFormat) : 'Unassigned' }} + {{ officeData.openingDate ? (officeData.openingDate | dateFormat) : ('labels.inputs.Unassigned' | translate) }}Also applies to: 26-26
src/app/organization/offices/view-office/general-tab/general-tab.component.scss (2)
72-77: Consolidate redundant.compact-viewselectors.The
.compact-viewclass is defined standalone (lines 72–77) and again as.tab-container.compact-view(lines 101–103) with identical max-width and width values. The second selector overrides or duplicates the first. Clarify the intent: is the combined selector needed, or can the standalone class suffice?If the combined selector is unnecessary, remove lines 101–103:
-.tab-container.compact-view { - max-width: 450px; - width: 90%; -}If the combined selector is intentional (e.g., to apply
.compact-viewstyles only within.tab-container), clarify the distinction or merge the intent.Also applies to: 101-103
1-10: Review responsive behavior of tab-container width constraints.Lines 2–5 set padding, margin, max-width, and width on
.tab-container. On smaller screens (< 600px), thewidth: 90%constraint combined withmargin: 1% automay cause layout shift or overflow. Verify that this scales correctly on mobile devices.Consider verifying responsive behavior with media queries or reviewing mobile testing results:
#!/bin/bash # Search for media queries in related SCSS files rg '@media' src/app/organization/offices/
...loans-view/loan-account-actions/edit-repayment-schedule/edit-repayment-schedule.component.ts
Show resolved
Hide resolved
src/app/loans/loans-view/repayment-schedule-tab/repayment-schedule-tab.component.ts
Show resolved
Hide resolved
src/app/products/manage-tax-components/view-tax-component/view-tax-component.component.spec.ts
Show resolved
Hide resolved
src/app/templates/create-edit-template/create-edit-template.component.ts
Show resolved
Hide resolved
src/app/templates/create-edit-template/create-edit-template.component.ts
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
d2365ad to
8527999
Compare
JaySoni1
force-pushed
the
WEB-95-fix-problems-with-glim-application-saved-total-moratorium-overdue-charges
branch
from
8527999 to
e46e3c0
Compare
|
@JaySoni1 it is not working this is the error, please make sure it is being tested using the latest Apache Fineract: { |
|
Ok @IOhacker I will solve this and will update the PR soon. Thank you for the review |
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
Accessibility
Chores
✏️ Tip: You can customize this high-level summary in your review settings.