WEB-657: Working Capital loan update period payment rates#3524
WEB-657: Working Capital loan update period payment rates#3524alberto-art3ch wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Routing & Module src/app/loans/loans-routing.module.ts |
Adds child route :loanId/payment-rates resolving loanPaymentRatesData via LoanPeriodPaymentRatesResolver, renders LoanPeriodPaymentRatesComponent, and registers resolver in providers. |
Loans View Template src/app/loans/loans-view/loans-view.component.html |
Fixes routerLinkActive reference for Transactions and adds a conditional "Period Payment Rates" tab linking to ./payment-rates with productType query param. |
Period Payment Rates component src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts, .html, .scss |
New standalone component displaying a Material table of period rate changes, Add flow (form dialog), Undo flow (undo dialog), CSS for reversed rows, and data refresh logic via LoansService. |
Resolver src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts |
New resolver extracts loanId from route/parent and returns getWorkingCapitalPeriodPaymentRates(loanId) observable for route resolve. |
Service Layer src/app/loans/loans.service.ts |
Added methods: getWorkingCapitalPeriodPaymentRates(loanId), addWorkingCapitalPeriodPaymentRate(loanId,payload), undoWorkingCapitalPeriodPaymentRate(loanId,payload) mapping to new REST endpoints. |
Data Model src/app/loans/models/working-capital-loan-account.model.ts |
Added exported interface PeriodPaymentRateChange (id, loanId, effectiveDate:number[], previousRate, newRate, reversed, createdDate:number[]). |
Shared Undo Dialog src/app/shared/undo-dialog/undo-dialog.component.ts, .html, .scss |
New standalone UndoDialogComponent with reactive note control; template returns { confirm: true, data } on confirm. SCSS includes license header. |
Translations src/assets/translations/* |
Added "Period Payment Rates" (inputs & text) and "Undo Payment Rate" (menus) keys across multiple locale JSON files (cs‑CS, de‑DE, en‑US, es‑CL, es‑MX, fr‑FR, it‑IT, ko‑KO, lt‑LT, lv‑LV, ne‑NE, pt‑PT, sw‑SW). |
Sequence Diagrams
sequenceDiagram
autonumber
participant User
participant Component as LoanPeriodPaymentRatesComponent
participant Dialog as FormDialogComponent
participant Service as LoansService
participant API as Backend
User->>Component: Click "Add Payment Rate"
Component->>Dialog: Open form dialog
Dialog->>User: Display form
User->>Dialog: Submit form (rate, note)
Dialog-->>Component: Return form data
Component->>Service: addWorkingCapitalPeriodPaymentRate(loanId, payload)
Service->>API: PUT /loans/{loanId}/period-rates
API-->>Service: Success
Service-->>Component: Observable resolved
Component->>Service: getWorkingCapitalPeriodPaymentRates(loanId)
Service->>API: GET /loans/{loanId}/period-rates
API-->>Service: Payment rates data
Service-->>Component: Updated list
Component->>Component: Update loanPaymentRatesData
sequenceDiagram
autonumber
participant User
participant Component as LoanPeriodPaymentRatesComponent
participant Dialog as UndoDialogComponent
participant Service as LoansService
participant API as Backend
User->>Component: Click "Undo Changes"
Component->>Dialog: Open undo dialog
Dialog->>User: Display confirmation & note
User->>Dialog: Confirm with note
Dialog-->>Component: { confirm: true, data }
Component->>Service: undoWorkingCapitalPeriodPaymentRate(loanId, payload)
Service->>API: POST /loans/{loanId}/period-rates/undo
API-->>Service: Success
Service-->>Component: Observable resolved
Component->>Service: getWorkingCapitalPeriodPaymentRates(loanId)
Service->>API: GET /loans/{loanId}/period-rates
API-->>Service: Updated data
Service-->>Component: Refreshed list
Component->>Component: Update loanPaymentRatesData
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
- WEB-634 The dashboard menu should display the full credit traceability. #3078: Overlaps changes to loan-view routing and adding loan child routes/tabs.
Suggested reviewers
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main feature being added: period payment rates functionality for Working Capital loans, which aligns with the PR's primary objective. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/app/loans/loans.service.ts (1)
850-865: Consider narrowingloanIdtostring | numberfor type safety.The
loanIdparameter should be typed more specifically thananysince it's used only in URL construction. Regarding the broader typing of request/response payloads, note that this codebase usesObservable<any>as a project-wide pattern; full API response typing is tracked as a separate enhancement rather than addressed piecemeal across individual PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans.service.ts` around lines 850 - 865, The three methods getWorkingCapitalPeriodPaymentRates, addWorkingCapitalPeriodPaymentRate, and undoWorkingCapitalPeriodPaymentRate use loanId: any — change those signatures to use loanId: string | number for stronger typing (keep payload: any as-is per project convention) and update any related internal references to treat loanId as string | number so URL construction remains correct; no other behavior changes required.src/assets/translations/pt-PT.json (1)
3116-3116: Remove unused translation key "Undo Payment Rate" or verify it has a documented purpose.The codebase does not reference
labels.menus.Undo Payment Rateoutside of translation files. The undo button is rendered using separate translation keys (labels.buttons.Undocombined withlabels.inputs.Period Payment Rate), making this key appear unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/translations/pt-PT.json` at line 3116, Remove the unused translation key "Undo Payment Rate" from the translations file or, if it is intended to be used, add a documented usage and update code to reference it; specifically search for and reconcile the translation key "Undo Payment Rate" with existing keys used for the undo UI flow (labels.buttons.Undo and labels.inputs.Period Payment Rate) and either delete the "Undo Payment Rate" entry or replace UI references to use this single key consistently and document its purpose.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss (1)
12-12: Use 8px-grid spacing instead of percentage padding.
padding-bottom: 2%makes vertical spacing viewport-dependent. Prefer a fixed 8px-grid multiple for predictable layout rhythm.Suggested change
.container { - padding-bottom: 2%; + padding-bottom: 16px; max-width: 100%;As per coding guidelines: “
src/**/*.{scss,html}: Stick to the 8px grid system for visual design and spacing”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss` at line 12, Replace the viewport-dependent rule "padding-bottom: 2%" with an 8px-grid based value; locate the selector in loan-period-payment-rates.component.scss that sets "padding-bottom: 2%" and change it to a fixed multiple of 8px (for example "padding-bottom: 16px" for 2 × 8px) or to the project's spacing token/variable if one exists (e.g., $spacing-2) to align with the 8px grid guideline.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html (1)
12-12: Use an 8px-grid spacing utility here.
gap-20pxbreaks the project’s spacing grid; use a 16px or 24px gap utility/SCSS variable instead. As per coding guidelines,src/**/*.{scss,html}should “Stick to the 8px grid system for visual design and spacing.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html` at line 12, The div uses a non-compliant spacing class "gap-20px" in the class list "layout-row transaction-buttons align-end gap-20px"; replace that token with an 8px-grid-compliant utility (e.g., gap-16px or gap-24px) or a project SCSS spacing variable (e.g., $spacing-16) and ensure the corresponding gap utility exists in the global utilities/SCSS so the HTML uses a permitted spacing class instead of "gap-20px".src/app/shared/undo-dialog/undo-dialog.component.ts (1)
32-49: Type the dialog data and form contract.This shared dialog currently accepts untyped
MAT_DIALOG_DATAand usesUntypedFormGroup; define explicit interfaces so callers and templates get compile-time checks. As per coding guidelines,src/app/**/*.tsshould “Use TypeScript for all application code with strict typing conventions.”Suggested typing direction
-import { UntypedFormBuilder, UntypedFormGroup, Validators } from '@angular/forms'; +import { FormBuilder, FormGroup, FormControl, Validators } from '@angular/forms'; + +interface UndoDialogData { + undoContext: string; +} + +interface UndoActionForm { + note: FormControl<string>; +} @@ - data = inject(MAT_DIALOG_DATA); - private formBuilder = inject(UntypedFormBuilder); + data = inject<UndoDialogData>(MAT_DIALOG_DATA); + private formBuilder = inject(FormBuilder); - undoActionForm: UntypedFormGroup | null = null; + undoActionForm: FormGroup<UndoActionForm> | null = null; @@ this.undoActionForm = this.formBuilder.group({ - note: [ - '', - Validators.required - ] + note: this.formBuilder.nonNullable.control('', Validators.required) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/undo-dialog/undo-dialog.component.ts` around lines 32 - 49, Add explicit TypeScript types for the dialog input and form model: define an interface (e.g., UndoDialogData) for the MAT_DIALOG_DATA payload and an interface (e.g., UndoActionFormValue) for the form value (note: string). Replace UntypedFormBuilder/UntypedFormGroup with typed FormBuilder and FormGroup<UndoActionFormValue>, change the data inject to inject<UndoDialogData>(MAT_DIALOG_DATA), type dialogRef as MatDialogRef<UndoDialogComponent, UndoActionFormValue> if it returns a value, and update undoActionForm’s type and createUndoActionForm to build a typed group in the createUndoActionForm function so templates and callers get compile-time checks (refer to UndoDialogComponent, dialogRef, data, undoActionForm, createUndoActionForm, MAT_DIALOG_DATA, UntypedFormBuilder).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`:
- Around line 21-30: The Undo button is shown whenever
loanPaymentRatesData.length > 0 even if all rows are already reversed; add a
getter hasUndoablePaymentRates in loan-period-payment-rates.component.ts that
returns whether any item in loanPaymentRatesData is not reversed (use
Array.prototype.some on loanPaymentRatesData), then change the template
condition to use hasUndoablePaymentRates instead of loanPaymentRatesData.length
> 0 so the Undo button only appears when undoable payment rates exist; ensure
undoPaymentRate remains unchanged and wired to the button.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 108-110: Remove the debug console output in the dialog close
handler: inside dialogRef.afterClosed().subscribe(...) (in
loan-period-payment-rates.component.ts) delete the console.log(response) line
and ensure any needed logic uses response?.data directly (or sanitizes fields)
instead of printing the full response to the browser console to avoid leaking
user-entered notes or sensitive context.
- Around line 83-104: The dialog uses the plural translation key for the
field/title; change labelHeadingText and any uses in data.title/layout to the
singular translation key so the Add dialog reads "Period Payment Rate" (not
"Period Payment Rates"). Update the assignment of labelHeadingText (currently
used by the InputBase with controlName 'periodPaymentRate' and by data.title and
layout.addButtonText) to call translateService.instant with the singular key and
ensure the input label and dialog title use that updated labelHeadingText.
In `@src/app/loans/models/working-capital-loan-account.model.ts`:
- Around line 12-16: The date fields in the WorkingCapitalLoanAccount model are
typed as Date but the API returns Fineract local-date arrays; update
effectiveDate and createdDate to type number[] to match the codebase pattern
(see other models like LoanCharge, LoanTransactionTemplate, DelinquentData),
ensuring any consumers of WorkingCapitalLoanAccount (e.g., templates using |
dateFormat) continue to work at runtime.
In `@src/app/shared/undo-dialog/undo-dialog.component.html`:
- Around line 10-27: Remove the unused template export `#formRef`="ngForm" and
replace the outer <div> wrapper with a <mat-dialog-content> element to follow
Angular Material dialog structure; keep the reactive form binding
[formGroup]="undoActionForm" and existing controls (formControlName="note",
mat-dialog-actions buttons) intact so the component uses undoActionForm
exclusively without the unused ngForm export.
In `@src/assets/translations/en-US.json`:
- Line 3159: Update the translation entry for "Undo Payment Rate" to match the
UI label by changing the key and its value from "Undo Payment Rate" to "Undo
Period Payment Rate" (i.e., replace the "Undo Payment Rate" JSON key/value pair
with "Undo Period Payment Rate": "Undo Period Payment Rate") so the button text
matches the tab/page label; ensure the exact phrase "Undo Period Payment Rate"
is used for both key and value.
In `@src/assets/translations/es-CL.json`:
- Line 3118: The Spanish translation key "Undo Payment Rate" should be changed
to match the "Period Payment Rate" terminology used elsewhere; update the value
for the key "Undo Payment Rate" in src/assets/translations/es-CL.json to
"Deshacer tasa de pago del período" so it remains consistent with the Period
Payment Rate table/tab labels.
In `@src/assets/translations/it-IT.json`:
- Line 3115: The Italian translation for the "Undo Payment Rate" label loses the
period context; update the value for the "Undo Payment Rate" key in
src/assets/translations/it-IT.json to explicitly include "period" (for example:
"Annulla il tasso di pagamento del periodo" or "Annulla il tasso di pagamento
periodico") so the label clearly refers to Period Payment Rates.
In `@src/assets/translations/ko-KO.json`:
- Line 3116: The translation for the key "Undo Payment Rate" uses inconsistent
terminology ("지불 비율 취소"); update the value to match the project's existing term
"기간별 지급률" (e.g., change to "기간별 지급률 취소") so it aligns with other labels; locate
the "Undo Payment Rate" entry in src/assets/translations/ko-KO.json and replace
its value accordingly.
---
Nitpick comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`:
- Line 12: The div uses a non-compliant spacing class "gap-20px" in the class
list "layout-row transaction-buttons align-end gap-20px"; replace that token
with an 8px-grid-compliant utility (e.g., gap-16px or gap-24px) or a project
SCSS spacing variable (e.g., $spacing-16) and ensure the corresponding gap
utility exists in the global utilities/SCSS so the HTML uses a permitted spacing
class instead of "gap-20px".
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss`:
- Line 12: Replace the viewport-dependent rule "padding-bottom: 2%" with an
8px-grid based value; locate the selector in
loan-period-payment-rates.component.scss that sets "padding-bottom: 2%" and
change it to a fixed multiple of 8px (for example "padding-bottom: 16px" for 2 ×
8px) or to the project's spacing token/variable if one exists (e.g., $spacing-2)
to align with the 8px grid guideline.
In `@src/app/loans/loans.service.ts`:
- Around line 850-865: The three methods getWorkingCapitalPeriodPaymentRates,
addWorkingCapitalPeriodPaymentRate, and undoWorkingCapitalPeriodPaymentRate use
loanId: any — change those signatures to use loanId: string | number for
stronger typing (keep payload: any as-is per project convention) and update any
related internal references to treat loanId as string | number so URL
construction remains correct; no other behavior changes required.
In `@src/app/shared/undo-dialog/undo-dialog.component.ts`:
- Around line 32-49: Add explicit TypeScript types for the dialog input and form
model: define an interface (e.g., UndoDialogData) for the MAT_DIALOG_DATA
payload and an interface (e.g., UndoActionFormValue) for the form value (note:
string). Replace UntypedFormBuilder/UntypedFormGroup with typed FormBuilder and
FormGroup<UndoActionFormValue>, change the data inject to
inject<UndoDialogData>(MAT_DIALOG_DATA), type dialogRef as
MatDialogRef<UndoDialogComponent, UndoActionFormValue> if it returns a value,
and update undoActionForm’s type and createUndoActionForm to build a typed group
in the createUndoActionForm function so templates and callers get compile-time
checks (refer to UndoDialogComponent, dialogRef, data, undoActionForm,
createUndoActionForm, MAT_DIALOG_DATA, UntypedFormBuilder).
In `@src/assets/translations/pt-PT.json`:
- Line 3116: Remove the unused translation key "Undo Payment Rate" from the
translations file or, if it is intended to be used, add a documented usage and
update code to reference it; specifically search for and reconcile the
translation key "Undo Payment Rate" with existing keys used for the undo UI flow
(labels.buttons.Undo and labels.inputs.Period Payment Rate) and either delete
the "Undo Payment Rate" entry or replace UI references to use this single key
consistently and document its purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 447f83cc-c267-4fd8-9df3-b6d70fb200b3
📒 Files selected for processing (24)
src/app/loans/loans-routing.module.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.htmlsrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scsssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.tssrc/app/loans/loans.service.tssrc/app/loans/models/working-capital-loan-account.model.tssrc/app/shared/undo-dialog/undo-dialog.component.htmlsrc/app/shared/undo-dialog/undo-dialog.component.scsssrc/app/shared/undo-dialog/undo-dialog.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
| effectiveDate: Date; | ||
| previousRate: number; | ||
| newRate: number; | ||
| reversed: boolean; | ||
| createdDate: Date; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how similar API date fields are modeled and consumed in the app.
# Expectation: Use the same date wire type used by comparable loan models/components,
# or add explicit mapping from the endpoint response to JavaScript Date objects.
rg -n --type=ts -C3 '\b(effectiveDate|createdDate)\b\s*:\s*(Date|number\[\]|\[number,\s*number,\s*number\]|string)' src/app || true
rg -n -C2 '\|\s*dateFormat' src/app/loans src/app/shared || trueRepository: openMF/web-app
Length of output: 40066
🏁 Script executed:
# Find the service that provides getWorkingCapitalPeriodPaymentRates
rg -n "getWorkingCapitalPeriodPaymentRates" src/app --type=tsRepository: openMF/web-app
Length of output: 744
🏁 Script executed:
# Look for the working-capital service file
fd -e ts "working.*capital.*service" src/appRepository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check for any response mappers or interceptors
rg -n "PeriodPaymentRateChange" src/app --type=tsRepository: openMF/web-app
Length of output: 734
🏁 Script executed:
# View the service implementation
sed -n '845,860p' src/app/loans/loans.service.tsRepository: openMF/web-app
Length of output: 469
🏁 Script executed:
# Check the resolver implementation
cat src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tsRepository: openMF/web-app
Length of output: 1025
🏁 Script executed:
# Check for pipe definition of dateFormat
fd -e ts "dateFormat" src/app --type=pipeRepository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Look for dateFormat pipe definition
rg -n "dateFormat" src/app -A5 -B2 --type=ts | grep -A5 -B2 "@Pipe\|transform"Repository: openMF/web-app
Length of output: 3815
🏁 Script executed:
# Look at other loan models to see how date fields are typed
rg -n "export interface" src/app/loans/models/ -A10 --type=ts | head -80Repository: openMF/web-app
Length of output: 5549
🏁 Script executed:
# Check how other services handle date responses
rg -n "http.get" src/app/loans/loans.service.ts -A2 | grep -A2 "date\|Date" | head -40Repository: openMF/web-app
Length of output: 40
Use number[] for date fields to match API response format and codebase pattern.
The interface declares effectiveDate: Date and createdDate: Date, but the service returns raw HTTP response data where these fields are number[] (Fineract's local-date format: [year, month, day]). This matches how other loan models in the same codebase type date fields—see LoanCharge, LoanTransactionTemplate, and DelinquentData which all use number[] for date fields from the API. The template uses | dateFormat pipe (which accepts any) so it works at runtime, but the type annotation is misleading. Align with the established pattern:
export interface PeriodPaymentRateChange {
id: number;
loanId: number;
- effectiveDate: Date;
+ effectiveDate: number[];
previousRate: number;
newRate: number;
reversed: boolean;
- createdDate: Date;
+ createdDate: number[];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| effectiveDate: Date; | |
| previousRate: number; | |
| newRate: number; | |
| reversed: boolean; | |
| createdDate: Date; | |
| export interface PeriodPaymentRateChange { | |
| id: number; | |
| loanId: number; | |
| effectiveDate: number[]; | |
| previousRate: number; | |
| newRate: number; | |
| reversed: boolean; | |
| createdDate: number[]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/models/working-capital-loan-account.model.ts` around lines 12 -
16, The date fields in the WorkingCapitalLoanAccount model are typed as Date but
the API returns Fineract local-date arrays; update effectiveDate and createdDate
to type number[] to match the codebase pattern (see other models like
LoanCharge, LoanTransactionTemplate, DelinquentData), ensuring any consumers of
WorkingCapitalLoanAccount (e.g., templates using | dateFormat) continue to work
at runtime.
| <div> | ||
| <form #formRef="ngForm" [formGroup]="undoActionForm"> | ||
| <mat-form-field class="flex-100"> | ||
| <textarea formControlName="note" matInput placeholder="{{ 'labels.inputs.Notes' | translate }}"></textarea> | ||
| </mat-form-field> | ||
| <mat-dialog-actions align="end"> | ||
| <button mat-raised-button mat-dialog-close>{{ 'labels.buttons.Cancel' | translate }}</button> | ||
| <button | ||
| mat-raised-button | ||
| color="primary" | ||
| [disabled]="undoActionForm.pristine" | ||
| [mat-dialog-close]="{ confirm: true, data: undoActionForm.value }" | ||
| > | ||
| {{ 'labels.buttons.Undo' | translate }} | ||
| </button> | ||
| </mat-dialog-actions> | ||
| </form> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expected after the fix: no matches.
rg -nP '<form(?=[^>]*\[formGroup\])(?=[^>]*#\w+="ngForm")' --type htmlRepository: openMF/web-app
Length of output: 490
🏁 Script executed:
# Check if formRef is used in the component TypeScript files for all three instances
for file in \
"src/app/shared/undo-dialog/undo-dialog.component.ts" \
"src/app/organization/currencies/manage-currencies/manage-currencies.component.ts" \
"src/app/clients/clients-view/custom-dialogs/edit-notes-dialog/edit-notes-dialog.component.ts"
do
echo "=== Checking: $file ==="
if [ -f "$file" ]; then
rg -n "formRef" "$file" || echo "No matches for formRef"
else
echo "File not found"
fi
echo ""
doneRepository: openMF/web-app
Length of output: 523
🏁 Script executed:
# Also check the HTML templates more thoroughly to see if formRef is referenced anywhere
for file in \
"src/app/shared/undo-dialog/undo-dialog.component.html" \
"src/app/organization/currencies/manage-currencies/manage-currencies.component.html" \
"src/app/clients/clients-view/custom-dialogs/edit-notes-dialog/edit-notes-dialog.component.html"
do
echo "=== HTML file: $file ==="
if [ -f "$file" ]; then
rg -n "formRef" "$file" || echo "No references to formRef in template"
else
echo "File not found"
fi
echo ""
doneRepository: openMF/web-app
Length of output: 550
🏁 Script executed:
# Read the full HTML templates to see the complete structure and usage
echo "=== undo-dialog.component.html ===" && cat -n src/app/shared/undo-dialog/undo-dialog.component.html || echo "File not found"Repository: openMF/web-app
Length of output: 1292
Remove the unused ngForm export and use mat-dialog-content for proper Material Dialog structure.
The #formRef="ngForm" export is unused in this component (not referenced in the .ts file) and unnecessary with reactive form setup. Replace the wrapper <div> with <mat-dialog-content> to follow Angular Material Dialog best practices.
Proposed fix
-<div>
- <form `#formRef`="ngForm" [formGroup]="undoActionForm">
+<form [formGroup]="undoActionForm">
+ <mat-dialog-content>
<mat-form-field class="flex-100">
<textarea formControlName="note" matInput placeholder="{{ 'labels.inputs.Notes' | translate }}"></textarea>
</mat-form-field>
+ </mat-dialog-content>
<mat-dialog-actions align="end">
<button mat-raised-button mat-dialog-close>{{ 'labels.buttons.Cancel' | translate }}</button>
<button
mat-raised-button
color="primary"
[disabled]="undoActionForm.pristine"
[mat-dialog-close]="{ confirm: true, data: undoActionForm.value }"
>
{{ 'labels.buttons.Undo' | translate }}
</button>
</mat-dialog-actions>
- </form>
-</div>
+</form>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/shared/undo-dialog/undo-dialog.component.html` around lines 10 - 27,
Remove the unused template export `#formRef`="ngForm" and replace the outer <div>
wrapper with a <mat-dialog-content> element to follow Angular Material dialog
structure; keep the reactive form binding [formGroup]="undoActionForm" and
existing controls (formControlName="note", mat-dialog-actions buttons) intact so
the component uses undoActionForm exclusively without the unused ngForm export.
| "Undo Disbursal": "Undo Disbursal", | ||
| "Undo Charge-Off": "Undo Charge-Off", | ||
| "Undo Last Disbursal": "Undo Last Disbursal", | ||
| "Undo Payment Rate": "Undo Payment Rate", |
There was a problem hiding this comment.
Match the undo label to the feature name.
The PR screenshot calls this action “Undo Period Payment Rate”; the current translation drops “Period”, making the button less consistent with the tab/page label.
✏️ Proposed text fix
- "Undo Payment Rate": "Undo Payment Rate",
+ "Undo Payment Rate": "Undo Period Payment Rate",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Undo Payment Rate": "Undo Payment Rate", | |
| "Undo Payment Rate": "Undo Period Payment Rate", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/en-US.json` at line 3159, Update the translation
entry for "Undo Payment Rate" to match the UI label by changing the key and its
value from "Undo Payment Rate" to "Undo Period Payment Rate" (i.e., replace the
"Undo Payment Rate" JSON key/value pair with "Undo Period Payment Rate": "Undo
Period Payment Rate") so the button text matches the tab/page label; ensure the
exact phrase "Undo Period Payment Rate" is used for both key and value.
| "Undo Disbursal": "Deshacer curso", | ||
| "Undo Charge-Off": "Deshacer Cancelación", | ||
| "Undo Last Disbursal": "Deshacer el último curso", | ||
| "Undo Payment Rate": "Deshacer tasa de pago", |
There was a problem hiding this comment.
Keep the undo action aligned with the Period Payment Rate terminology.
This action appears to target the new “Period Payment Rate” entity, but the Spanish label drops “del período,” unlike the new table/tab labels. Consider keeping the qualifier for consistency.
🌐 Proposed wording tweak
- "Undo Payment Rate": "Deshacer tasa de pago",
+ "Undo Payment Rate": "Deshacer tasa de pago del período",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Undo Payment Rate": "Deshacer tasa de pago", | |
| "Undo Payment Rate": "Deshacer tasa de pago del período", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/es-CL.json` at line 3118, The Spanish translation key
"Undo Payment Rate" should be changed to match the "Period Payment Rate"
terminology used elsewhere; update the value for the key "Undo Payment Rate" in
src/assets/translations/es-CL.json to "Deshacer tasa de pago del período" so it
remains consistent with the Period Payment Rate table/tab labels.
| "Undo Disbursal": "Annullare l'esborso", | ||
| "Undo Charge-Off": "Annullare l'addebito", | ||
| "Undo Last Disbursal": "Annulla l'ultimo esborso", | ||
| "Undo Payment Rate": "Annulla il tasso di pagamento", |
There was a problem hiding this comment.
Keep the period context in the undo action label.
The new screen is specifically for Period Payment Rates; this translation drops “period” and can read like a generic payment-rate undo action.
🌐 Proposed translation adjustment
- "Undo Payment Rate": "Annulla il tasso di pagamento",
+ "Undo Payment Rate": "Annulla il tasso di pagamento del periodo",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Undo Payment Rate": "Annulla il tasso di pagamento", | |
| "Undo Payment Rate": "Annulla il tasso di pagamento del periodo", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/it-IT.json` at line 3115, The Italian translation for
the "Undo Payment Rate" label loses the period context; update the value for the
"Undo Payment Rate" key in src/assets/translations/it-IT.json to explicitly
include "period" (for example: "Annulla il tasso di pagamento del periodo" or
"Annulla il tasso di pagamento periodico") so the label clearly refers to Period
Payment Rates.
| "Undo Disbursal": "지급 취소", | ||
| "Undo Charge-Off": "상각 취소", | ||
| "Undo Last Disbursal": "마지막 지불 취소", | ||
| "Undo Payment Rate": "지불 비율 취소", |
There was a problem hiding this comment.
Keep the Korean term for “Payment Rate” consistent.
The related labels use 기간별 지급률, but this action uses 지불 비율, which may read as a different concept. Consider aligning the undo label with the same terminology.
🌐 Proposed translation consistency fix
- "Undo Payment Rate": "지불 비율 취소",
+ "Undo Payment Rate": "기간별 지급률 취소",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/translations/ko-KO.json` at line 3116, The translation for the key
"Undo Payment Rate" uses inconsistent terminology ("지불 비율 취소"); update the value
to match the project's existing term "기간별 지급률" (e.g., change to "기간별 지급률 취소") so
it aligns with other labels; locate the "Undo Payment Rate" entry in
src/assets/translations/ko-KO.json and replace its value accordingly.
1ba133e to
0a1cfee
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (1)
108-121: Tighten the new response typings when touching this flow.The table data is already modeled as
PeriodPaymentRateChange[], but the dialog/API callbacks fall back toany. Consider adding small close-result interfaces and typing the refreshed rate-change response in the service/component as part of this new feature.Based on learnings, TypeScript API responses should use specific interfaces/types instead of
any, while broader response-layer typing can be handled as a separate enhancement.Also applies to: 135-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 108 - 121, The callbacks use broad any types; tighten them by introducing small result interfaces (e.g. CloseDialogResult { data?: { value: { periodPaymentRate: number; note?: string } } } and API response types like AddRateResponse and PeriodPaymentRateChange[]) and use them in dialogRef.afterClosed().subscribe, the addWorkingCapitalPeriodPaymentRate() response, and getWorkingCapitalPeriodPaymentRates() subscription; update the component's loanPaymentRatesData to be typed as PeriodPaymentRateChange[] and change the local destructure to use the typed CloseDialogResult to eliminate any usage of any in this flow (affecting dialogRef.afterClosed, addWorkingCapitalPeriodPaymentRate, getWorkingCapitalPeriodPaymentRates, and loanPaymentRatesData).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`:
- Around line 51-59: Remove the hard-coded space before the percent sign in the
template so rates render as "1.00%" not "1.00 %"; update the two bindings that
render percent values (the cell showing item.previousRate and the cell showing
item.newRate in the loan-period-payment-rates template) to place the "%"
immediately after the formatted value, and audit other occurrences in this
component/template (e.g., bindings using formatNumber and
periodPaymentRateStyle) to ensure no trailing space is present; if
locale-specific spacing is required later, implement it in the formatter
(formatNumber) rather than in the template.
- Line 12: The div using class "gap-20px" violates the 8px grid — replace that
utility with an existing 8px-grid token (e.g., use a 16px or 24px gap utility
such as "gap-16px" or "gap-24px") or remove the inline gap class and define the
spacing in the component SCSS by styling the .transaction-buttons selector using
the project's spacing variables (the theme/main spacing tokens) so the
layout-row .transaction-buttons uses an 8px-grid-compliant gap.
- Around line 19-28: The template composes two translation keys which can break
word order in other locales; replace the concatenated labels in the buttons
inside loan-period-payment-rates.component.html with single, full translation
keys (e.g., use a dedicated key for the add button and one for the undo button)
instead of "{{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period
Payment Rate' | translate }}", update the button that calls undoPaymentRate()
and the one controlled by hasUndoablePaymentRates to use the new full keys
(unique identifiers like labels.buttons.AddPeriodPaymentRate and
labels.buttons.UndoPeriodPaymentRate), and then run npm run translations:extract
to add the new i18n entries.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 102-104: The dialog title currently composes two translated
fragments (translateService.instant('labels.buttons.Add') + ' ' +
labelHeadingText) which breaks localization; change to a single translation key
with interpolation (e.g., use translateService.instant('dialogs.addItem', {
item: labelHeadingText })) and update the layout.addButtonText similarly to use
a full key, referencing translateService.instant('dialogs.addButton') or a
parameterized key so translators see the whole phrase; ensure the translation
keys are added to i18n files and run npm run translations:extract.
---
Nitpick comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 108-121: The callbacks use broad any types; tighten them by
introducing small result interfaces (e.g. CloseDialogResult { data?: { value: {
periodPaymentRate: number; note?: string } } } and API response types like
AddRateResponse and PeriodPaymentRateChange[]) and use them in
dialogRef.afterClosed().subscribe, the addWorkingCapitalPeriodPaymentRate()
response, and getWorkingCapitalPeriodPaymentRates() subscription; update the
component's loanPaymentRatesData to be typed as PeriodPaymentRateChange[] and
change the local destructure to use the typed CloseDialogResult to eliminate any
usage of any in this flow (affecting dialogRef.afterClosed,
addWorkingCapitalPeriodPaymentRate, getWorkingCapitalPeriodPaymentRates, and
loanPaymentRatesData).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e059f403-2152-4366-a3ab-99e288d8a488
📒 Files selected for processing (24)
src/app/loans/loans-routing.module.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.htmlsrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scsssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.tssrc/app/loans/loans.service.tssrc/app/loans/models/working-capital-loan-account.model.tssrc/app/shared/undo-dialog/undo-dialog.component.htmlsrc/app/shared/undo-dialog/undo-dialog.component.scsssrc/app/shared/undo-dialog/undo-dialog.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (9)
- src/app/shared/undo-dialog/undo-dialog.component.scss
- src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss
- src/assets/translations/en-US.json
- src/assets/translations/es-CL.json
- src/assets/translations/sw-SW.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lv-LV.json
- src/assets/translations/fr-FR.json
- src/assets/translations/ne-NE.json
🚧 Files skipped from review as they are similar to previous changes (13)
- src/assets/translations/ko-KO.json
- src/app/loans/loans-view/loans-view.component.html
- src/app/shared/undo-dialog/undo-dialog.component.html
- src/app/loans/loans.service.ts
- src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts
- src/assets/translations/pt-PT.json
- src/app/loans/loans-routing.module.ts
- src/app/shared/undo-dialog/undo-dialog.component.ts
- src/app/loans/models/working-capital-loan-account.model.ts
- src/assets/translations/it-IT.json
- src/assets/translations/de-DE.json
- src/assets/translations/lt-LT.json
- src/assets/translations/es-MX.json
| <div class="container"> | ||
| <h3>{{ 'labels.inputs.Period Payment Rates' | translate }}</h3> | ||
|
|
||
| <div class="layout-row transaction-buttons align-end gap-20px"> |
There was a problem hiding this comment.
Use an 8px-grid spacing token here.
gap-20px falls outside the project’s 8px spacing scale. Prefer an existing 16px/24px utility or move this spacing into SCSS using the project spacing variables.
As per coding guidelines, src/**/*.{scss,html} should “Stick to the 8px grid system for visual design and spacing” and “Leverage SCSS variables defined in src/main.scss and src/theme/mifosx-theme.scss rather than generating custom classes and explicit pixel values”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`
at line 12, The div using class "gap-20px" violates the 8px grid — replace that
utility with an existing 8px-grid token (e.g., use a 16px or 24px gap utility
such as "gap-16px" or "gap-24px") or remove the inline gap class and define the
spacing in the component SCSS by styling the .transaction-buttons selector using
the project's spacing variables (the theme/main spacing tokens) so the
layout-row .transaction-buttons uses an 8px-grid-compliant gap.
| {{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }} | ||
| </button> | ||
| @if (hasUndoablePaymentRates) { | ||
| <button | ||
| mat-raised-button | ||
| color="primary" | ||
| (click)="undoPaymentRate()" | ||
| *mifosxHasPermission="'UNDORATECHANGE_WORKINGCAPITALLOAN'" | ||
| > | ||
| {{ 'labels.buttons.Undo' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }} |
There was a problem hiding this comment.
Use full translation keys for the action labels.
Composing Add/Undo with Period Payment Rate can produce incorrect word order in non-English locales. Prefer dedicated keys for the full button labels.
🌐 Suggested wording fix
- {{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }}
+ {{ 'labels.buttons.Add Period Payment Rate' | translate }}
...
- {{ 'labels.buttons.Undo' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }}
+ {{ 'labels.buttons.Undo Period Payment Rate' | translate }}As per coding guidelines, use “proper i18n variables from @ngx-translate/core for all user-facing strings” and run npm run translations:extract whenever new strings are added.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }} | |
| </button> | |
| @if (hasUndoablePaymentRates) { | |
| <button | |
| mat-raised-button | |
| color="primary" | |
| (click)="undoPaymentRate()" | |
| *mifosxHasPermission="'UNDORATECHANGE_WORKINGCAPITALLOAN'" | |
| > | |
| {{ 'labels.buttons.Undo' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }} | |
| {{ 'labels.buttons.Add Period Payment Rate' | translate }} | |
| </button> | |
| `@if` (hasUndoablePaymentRates) { | |
| <button | |
| mat-raised-button | |
| color="primary" | |
| (click)="undoPaymentRate()" | |
| *mifosxHasPermission="'UNDORATECHANGE_WORKINGCAPITALLOAN'" | |
| > | |
| {{ 'labels.buttons.Undo Period Payment Rate' | translate }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`
around lines 19 - 28, The template composes two translation keys which can break
word order in other locales; replace the concatenated labels in the buttons
inside loan-period-payment-rates.component.html with single, full translation
keys (e.g., use a dedicated key for the add button and one for the undo button)
instead of "{{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period
Payment Rate' | translate }}", update the button that calls undoPaymentRate()
and the one controlled by hasUndoablePaymentRates to use the new full keys
(unique identifiers like labels.buttons.AddPeriodPaymentRate and
labels.buttons.UndoPeriodPaymentRate), and then run npm run translations:extract
to add the new i18n entries.
| {{ item.previousRate | formatNumber }} % | ||
| </td> | ||
| </ng-container> | ||
| <ng-container matColumnDef="newRate"> | ||
| <th mat-header-cell *matHeaderCellDef> | ||
| {{ 'labels.catalogs.New' | translate }} {{ 'labels.inputs.Rate' | translate }} | ||
| </th> | ||
| <td mat-cell *matCellDef="let item" [ngClass]="periodPaymentRateStyle(item.reversed)"> | ||
| {{ item.newRate | formatNumber }} % |
There was a problem hiding this comment.
Remove the hard-coded space before %.
The current template renders values like 1.00 %, while the PR screenshots show 1.00%. If locale-specific percent spacing is needed later, that should come from a formatter rather than a literal template space.
🧹 Suggested formatting fix
- {{ item.previousRate | formatNumber }} %
+ {{ item.previousRate | formatNumber }}%
...
- {{ item.newRate | formatNumber }} %
+ {{ item.newRate | formatNumber }}%📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ item.previousRate | formatNumber }} % | |
| </td> | |
| </ng-container> | |
| <ng-container matColumnDef="newRate"> | |
| <th mat-header-cell *matHeaderCellDef> | |
| {{ 'labels.catalogs.New' | translate }} {{ 'labels.inputs.Rate' | translate }} | |
| </th> | |
| <td mat-cell *matCellDef="let item" [ngClass]="periodPaymentRateStyle(item.reversed)"> | |
| {{ item.newRate | formatNumber }} % | |
| {{ item.previousRate | formatNumber }}% | |
| </td> | |
| </ng-container> | |
| <ng-container matColumnDef="newRate"> | |
| <th mat-header-cell *matHeaderCellDef> | |
| {{ 'labels.catalogs.New' | translate }} {{ 'labels.inputs.Rate' | translate }} | |
| </th> | |
| <td mat-cell *matCellDef="let item" [ngClass]="periodPaymentRateStyle(item.reversed)"> | |
| {{ item.newRate | formatNumber }}% |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`
around lines 51 - 59, Remove the hard-coded space before the percent sign in the
template so rates render as "1.00%" not "1.00 %"; update the two bindings that
render percent values (the cell showing item.previousRate and the cell showing
item.newRate in the loan-period-payment-rates template) to place the "%"
immediately after the formatted value, and audit other occurrences in this
component/template (e.g., bindings using formatNumber and
periodPaymentRateStyle) to ensure no trailing space is present; if
locale-specific spacing is required later, implement it in the formatter
(formatNumber) rather than in the template.
| const data = { | ||
| title: this.translateService.instant('labels.buttons.Add') + ' ' + labelHeadingText, | ||
| layout: { addButtonText: this.translateService.instant('labels.buttons.Add') }, |
There was a problem hiding this comment.
Use a full translation key for the dialog title too.
This has the same localization issue as the template buttons: translated fragments may not compose correctly across locales.
🌐 Suggested wording fix
- title: this.translateService.instant('labels.buttons.Add') + ' ' + labelHeadingText,
+ title: this.translateService.instant('labels.buttons.Add Period Payment Rate'),As per coding guidelines, use “proper i18n variables from @ngx-translate/core for all user-facing strings” and run npm run translations:extract whenever new strings are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`
around lines 102 - 104, The dialog title currently composes two translated
fragments (translateService.instant('labels.buttons.Add') + ' ' +
labelHeadingText) which breaks localization; change to a single translation key
with interpolation (e.g., use translateService.instant('dialogs.addItem', {
item: labelHeadingText })) and update the layout.addButtonText similarly to use
a full key, referencing translateService.instant('dialogs.addButton') or a
parameterized key so translators see the whole phrase; ensure the translation
keys are added to i18n files and run npm run translations:extract.
0a1cfee to
3064132
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts (1)
26-28: Use a concrete response type instead ofObservable<any>.Line 28 should return a specific model type (from the working-capital loan model) to keep strict typing and avoid
anypropagation.Based on learnings: In TypeScript files, avoid
Observable<any>for API responses and use specific interfaces/types instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts` around lines 26 - 28, The resolve(route: ActivatedRouteSnapshot) method currently returns Observable<any>; change it to a concrete type from the working-capital loan model (e.g., Observable<LoanPeriodPaymentRatesResponse> or the exact interface exported for period/payment rates) by importing that specific interface from the working-capital loan model module and updating the resolve signature in LoanPeriodPaymentRatesResolver (and any intermediate variables/pipe/map return types) so all returned values conform to that interface; ensure any RxJS operators (map/switchMap) are typed accordingly and update imports to remove the use of any.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (2)
108-147: Avoid nested.subscribe()— flatten withswitchMap.Both
addPaymentRate()andundoPaymentRate()chain three nested subscriptions (afterClosed → add/undo → getWorkingCapital…). This is the classic nested-subscribe anti-pattern: errors aren't propagated, the inner observables aren't cancelled if the user navigates away, and the code is harder to read. UseswitchMap/filterand a single subscribe (and ideally tie the subscription lifetime to the component, e.g. withtakeUntilDestroyed()).♻️ Proposed refactor (undo example)
- dialogRef.afterClosed().subscribe((response: any) => { - if (response?.confirm) { - const payload = { - note: response.data.note || '' - }; - this.loanService.undoWorkingCapitalPeriodPaymentRate(this.loanId, payload).subscribe((response: any) => { - this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId).subscribe((data: any) => { - this.loanPaymentRatesData = data; - }); - }); - } - }); + dialogRef + .afterClosed() + .pipe( + filter((response) => !!response?.confirm), + switchMap((response) => + this.loanService.undoWorkingCapitalPeriodPaymentRate(this.loanId, { note: response.data.note || '' }) + ), + switchMap(() => this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId)) + ) + .subscribe((data: PeriodPaymentRateChange[]) => { + this.loanPaymentRatesData = data; + });Apply the same shape to
addPaymentRate(). While here, please also replace theanytypes on the inner callbacks with the actual response/payload types so the strict-typing guideline is honored.As per coding guidelines, “Use RxJS Observables and route resolvers to fetch and pass data” and Angular code should use “clean observable patterns.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 108 - 147, Both addPaymentRate() and undoPaymentRate() use nested .subscribe() chains (dialogRef.afterClosed → loanService.add/undo → loanService.getWorkingCapitalPeriodPaymentRates) which should be flattened; refactor each method to pipe the afterClosed() observable through filter(response => response?.data || response?.confirm), then use switchMap to call loanService.addWorkingCapitalPeriodPaymentRate(...) or loanService.undoWorkingCapitalPeriodPaymentRate(...), followed by another switchMap to loanService.getWorkingCapitalPeriodPaymentRates(this.loanId) and a single subscribe to set this.loanPaymentRatesData; also bind the stream lifetime to the component (e.g., takeUntilDestroyed() or an ngUnsubscribe subject) and replace the inner any types with the proper request/response interfaces referenced by the loanService methods (e.g., payload types and returned data types) to satisfy strict typing.
149-155: Clarify semantics ofhasUndoablePaymentRatesand simplifyperiodPaymentRateStylehelper.
hasUndoablePaymentRatesreturnstruewhenever any entry hasreversed === false, but sinceundoWorkingCapitalPeriodPaymentRateundoes only the latest change (no entry ID parameter), the condition and the button action are semantically mismatched. If the most recent entry is already reversed while older ones are not, the Undo button will display but may not function as intended. Consider renaming to clarify the actual check (e.g.,hasActivePaymentRate) or adjust to scope the check to the latest entry only:get hasUndoablePaymentRates(): boolean { return this.loanPaymentRatesData.length > 0 && !this.loanPaymentRatesData[this.loanPaymentRatesData.length - 1].reversed; }Also remove the
periodPaymentRateStylehelper and bind the class directly in the template: replace all[ngClass]="periodPaymentRateStyle(item.reversed)"with[class.reversed]="item.reversed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 149 - 155, The current hasUndoablePaymentRates getter checks any non-reversed entry which mismatches the single-item undo behavior; change it to only consider the latest entry (e.g., in get hasUndoablePaymentRates(): return this.loanPaymentRatesData.length > 0 && !this.loanPaymentRatesData[this.loanPaymentRatesData.length - 1].reversed) or rename it to hasActivePaymentRate if you keep the broader semantics; also delete the periodPaymentRateStyle(reversed: boolean) helper and update template bindings to use class binding directly ([class.reversed]="item.reversed") instead of [ngClass]="periodPaymentRateStyle(item.reversed)" so styling reflects the boolean without the extra method.src/app/shared/undo-dialog/undo-dialog.component.ts (1)
33-48: ImplementOnInitand initialize the form at declaration.Two small cleanups:
- The class declares an
ngOnInit()lifecycle hook but does notimplements OnInit. Add the interface for type-safety / consistency with the rest of the codebase.undoActionFormis typed asUntypedFormGroup | nullsolely so it can be assigned inngOnInit. SinceformBuilderis available at construction time, initialize the form at the field declaration and drop the null union (andngOnInit/createUndoActionFormaltogether). The template ([formGroup]="undoActionForm") then never sees anullvalue.♻️ Proposed simplification
-import { Component, inject } from '@angular/core'; +import { Component, inject, OnInit } from '@angular/core'; ... -export class UndoDialogComponent { +export class UndoDialogComponent { dialogRef = inject<MatDialogRef<UndoDialogComponent>>(MatDialogRef); data = inject(MAT_DIALOG_DATA); private formBuilder = inject(UntypedFormBuilder); - undoActionForm: UntypedFormGroup | null = null; - - ngOnInit() { - this.createUndoActionForm(); - } - - createUndoActionForm() { - this.undoActionForm = this.formBuilder.group({ - note: [''] - }); - } + undoActionForm: UntypedFormGroup = this.formBuilder.group({ + note: [''] + }); }(If you prefer to keep the lazy init, at minimum add
implements OnInitand importOnInit.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/undo-dialog/undo-dialog.component.ts` around lines 33 - 48, The component declares ngOnInit but doesn't implement OnInit and unnecessarily defers form creation; change the class signature to implement OnInit and simplify by initializing undoActionForm at declaration using the injected formBuilder (remove createUndoActionForm and ngOnInit), so undoActionForm is a non-null UntypedFormGroup; reference the UndoDialogComponent class, the undoActionForm field, the formBuilder injection, and remove the createUndoActionForm/ngOnInit methods (or at minimum add implements OnInit if you keep lazy init).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Around line 29-30: The resolver currently accesses route.parent.paramMap
without guarding and passes a possibly null loanId to
loansService.getWorkingCapitalPeriodPaymentRates; update the code in
loan-period-payment-rates.resolver (where loanId is retrieved) to use optional
chaining (route.parent?.paramMap.get('loanId')) and then validate that loanId is
non-null/undefined before calling
this.loansService.getWorkingCapitalPeriodPaymentRates(loanId) — if loanId is
missing, handle it deterministically (e.g., return EMPTY/throw an
Error/redirect) so the service is never called with a null id.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 62-75: The loanId field is declared as number | null but ngOnInit
assigns route.parent.snapshot.params['loanId'] (a string) and can throw if
parent is null; change ngOnInit to safely access the parent using optional
chaining (this.route.parent?.snapshot.params) and convert the param to a number
before assigning to loanId (e.g., parseInt/Number and handle NaN by setting
loanId = null or throwing a clear error), then ensure calls to
loanService.addWorkingCapitalPeriodPaymentRate,
undoWorkingCapitalPeriodPaymentRate, and getWorkingCapitalPeriodPaymentRates use
the numeric loanId or bail out early if loanId is null.
---
Nitpick comments:
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Around line 26-28: The resolve(route: ActivatedRouteSnapshot) method currently
returns Observable<any>; change it to a concrete type from the working-capital
loan model (e.g., Observable<LoanPeriodPaymentRatesResponse> or the exact
interface exported for period/payment rates) by importing that specific
interface from the working-capital loan model module and updating the resolve
signature in LoanPeriodPaymentRatesResolver (and any intermediate
variables/pipe/map return types) so all returned values conform to that
interface; ensure any RxJS operators (map/switchMap) are typed accordingly and
update imports to remove the use of any.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 108-147: Both addPaymentRate() and undoPaymentRate() use nested
.subscribe() chains (dialogRef.afterClosed → loanService.add/undo →
loanService.getWorkingCapitalPeriodPaymentRates) which should be flattened;
refactor each method to pipe the afterClosed() observable through
filter(response => response?.data || response?.confirm), then use switchMap to
call loanService.addWorkingCapitalPeriodPaymentRate(...) or
loanService.undoWorkingCapitalPeriodPaymentRate(...), followed by another
switchMap to loanService.getWorkingCapitalPeriodPaymentRates(this.loanId) and a
single subscribe to set this.loanPaymentRatesData; also bind the stream lifetime
to the component (e.g., takeUntilDestroyed() or an ngUnsubscribe subject) and
replace the inner any types with the proper request/response interfaces
referenced by the loanService methods (e.g., payload types and returned data
types) to satisfy strict typing.
- Around line 149-155: The current hasUndoablePaymentRates getter checks any
non-reversed entry which mismatches the single-item undo behavior; change it to
only consider the latest entry (e.g., in get hasUndoablePaymentRates(): return
this.loanPaymentRatesData.length > 0 &&
!this.loanPaymentRatesData[this.loanPaymentRatesData.length - 1].reversed) or
rename it to hasActivePaymentRate if you keep the broader semantics; also delete
the periodPaymentRateStyle(reversed: boolean) helper and update template
bindings to use class binding directly ([class.reversed]="item.reversed")
instead of [ngClass]="periodPaymentRateStyle(item.reversed)" so styling reflects
the boolean without the extra method.
In `@src/app/shared/undo-dialog/undo-dialog.component.ts`:
- Around line 33-48: The component declares ngOnInit but doesn't implement
OnInit and unnecessarily defers form creation; change the class signature to
implement OnInit and simplify by initializing undoActionForm at declaration
using the injected formBuilder (remove createUndoActionForm and ngOnInit), so
undoActionForm is a non-null UntypedFormGroup; reference the UndoDialogComponent
class, the undoActionForm field, the formBuilder injection, and remove the
createUndoActionForm/ngOnInit methods (or at minimum add implements OnInit if
you keep lazy init).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 379d5fe1-083f-449b-8a5c-dd3078523a4a
📒 Files selected for processing (24)
src/app/loans/loans-routing.module.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.htmlsrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scsssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.tssrc/app/loans/loans.service.tssrc/app/loans/models/working-capital-loan-account.model.tssrc/app/shared/undo-dialog/undo-dialog.component.htmlsrc/app/shared/undo-dialog/undo-dialog.component.scsssrc/app/shared/undo-dialog/undo-dialog.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (12)
- src/app/shared/undo-dialog/undo-dialog.component.scss
- src/app/loans/models/working-capital-loan-account.model.ts
- src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html
- src/assets/translations/cs-CS.json
- src/assets/translations/fr-FR.json
- src/assets/translations/es-CL.json
- src/assets/translations/es-MX.json
- src/assets/translations/de-DE.json
- src/assets/translations/en-US.json
- src/assets/translations/lv-LV.json
- src/assets/translations/ko-KO.json
- src/assets/translations/it-IT.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss
- src/app/loans/loans-routing.module.ts
- src/app/loans/loans.service.ts
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
| const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); | ||
| return this.loansService.getWorkingCapitalPeriodPaymentRates(loanId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify resolver route shape and service method signature.
rg -n --type ts -C3 "LoanPeriodPaymentRatesResolver|payment-rates|:loanId" src/app/loans/loans-routing.module.ts
rg -n --type ts -C3 "getWorkingCapitalPeriodPaymentRates\\s*\\(" src/app/loans/loans.service.tsRepository: openMF/web-app
Length of output: 3052
🏁 Script executed:
# Read the resolver file to see the actual implementation
cat -n src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tsRepository: openMF/web-app
Length of output: 1250
🏁 Script executed:
# Check the parent route structure more carefully to understand the nesting
rg -n --type ts -B10 -A10 "path: 'payment-rates'" src/app/loans/loans-routing.module.ts | head -40Repository: openMF/web-app
Length of output: 907
🏁 Script executed:
# Look at other resolvers in the same directory to see how they handle parent routes
find src/app/loans -name "*.resolver.ts" -type f -exec wc -l {} + | head -10Repository: openMF/web-app
Length of output: 822
🏁 Script executed:
# Check if other resolvers use parent.paramMap pattern
rg -n "route\.parent" src/app/loans --type tsRepository: openMF/web-app
Length of output: 8987
Guard route.parent with optional chaining and validate loanId before API call.
Line 29 accesses route.parent.paramMap without null checks, and line 30 calls the service with a potentially null loanId. While other resolvers in the codebase use the same pattern, loan-originators.resolver.ts demonstrates the safer approach with optional chaining. Add null validation to prevent runtime errors.
Recommended fix
resolve(route: ActivatedRouteSnapshot): Observable<any> {
- const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId');
+ const loanId = route.paramMap.get('loanId') ?? route.parent?.paramMap.get('loanId');
+ if (!loanId) {
+ throw new Error('Missing route param: loanId');
+ }
return this.loansService.getWorkingCapitalPeriodPaymentRates(loanId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`
around lines 29 - 30, The resolver currently accesses route.parent.paramMap
without guarding and passes a possibly null loanId to
loansService.getWorkingCapitalPeriodPaymentRates; update the code in
loan-period-payment-rates.resolver (where loanId is retrieved) to use optional
chaining (route.parent?.paramMap.get('loanId')) and then validate that loanId is
non-null/undefined before calling
this.loansService.getWorkingCapitalPeriodPaymentRates(loanId) — if loanId is
missing, handle it deterministically (e.g., return EMPTY/throw an
Error/redirect) so the service is never called with a null id.
| loanId: number | null = null; | ||
|
|
||
| loanPaymentRatesColumns: string[] = [ | ||
| 'id', | ||
| 'effectiveDate', | ||
| 'previousRate', | ||
| 'newRate', | ||
| 'createdDate' | ||
| ]; | ||
|
|
||
| constructor() {} | ||
|
|
||
| ngOnInit(): void { | ||
| this.loanId = this.route.parent.snapshot.params['loanId']; |
There was a problem hiding this comment.
loanId is assigned a string despite being typed number | null.
ActivatedRoute.snapshot.params['loanId'] returns a string, but the field is declared as number | null. The string is then forwarded to loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload) and undoWorkingCapitalPeriodPaymentRate(this.loanId, payload) (lines 117, 140), as well as getWorkingCapitalPeriodPaymentRates (lines 118, 141). Depending on how those service methods serialize the URL/path, this can lead to incorrect typing at minimum and, if any downstream code does numeric comparisons or arithmetic on loanId, subtle bugs.
Also, this.route.parent can be null in principle — if this component is ever mounted as a top-level route the access will throw.
🛡️ Proposed fix
ngOnInit(): void {
- this.loanId = this.route.parent.snapshot.params['loanId'];
+ this.loanId = Number(this.route.parent?.snapshot.params['loanId']);
this.route.data.subscribe((data: { loanPaymentRatesData: PeriodPaymentRateChange[] }) => {
this.loanPaymentRatesData = data.loanPaymentRatesData;
});
}As per coding guidelines, use TypeScript with strict typing conventions (src/app/**/*.ts).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loanId: number | null = null; | |
| loanPaymentRatesColumns: string[] = [ | |
| 'id', | |
| 'effectiveDate', | |
| 'previousRate', | |
| 'newRate', | |
| 'createdDate' | |
| ]; | |
| constructor() {} | |
| ngOnInit(): void { | |
| this.loanId = this.route.parent.snapshot.params['loanId']; | |
| loanId: number | null = null; | |
| loanPaymentRatesColumns: string[] = [ | |
| 'id', | |
| 'effectiveDate', | |
| 'previousRate', | |
| 'newRate', | |
| 'createdDate' | |
| ]; | |
| constructor() {} | |
| ngOnInit(): void { | |
| this.loanId = Number(this.route.parent?.snapshot.params['loanId']); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`
around lines 62 - 75, The loanId field is declared as number | null but ngOnInit
assigns route.parent.snapshot.params['loanId'] (a string) and can throw if
parent is null; change ngOnInit to safely access the parent using optional
chaining (this.route.parent?.snapshot.params) and convert the param to a number
before assigning to loanId (e.g., parseInt/Number and handle NaN by setting
loanId = null or throwing a clear error), then ensure calls to
loanService.addWorkingCapitalPeriodPaymentRate,
undoWorkingCapitalPeriodPaymentRate, and getWorkingCapitalPeriodPaymentRates use
the numeric loanId or bail out early if loanId is null.
3064132 to
26d01ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts (1)
28-30:⚠️ Potential issue | 🟠 MajorGuard missing
loanIdbefore invoking the service.Line 29/Line 30 can still pass a nullable
loanIdintogetWorkingCapitalPeriodPaymentRates(...), androute.parentis accessed without null-safe chaining.Proposed fix
-import { Observable } from 'rxjs'; +import { Observable, throwError } from 'rxjs'; ... - resolve(route: ActivatedRouteSnapshot): Observable<any> { - const loanId = route.paramMap.get('loanId') || route.parent.paramMap.get('loanId'); - return this.loansService.getWorkingCapitalPeriodPaymentRates(loanId); + resolve(route: ActivatedRouteSnapshot): Observable<any> { + const loanId = route.paramMap.get('loanId') ?? route.parent?.paramMap.get('loanId'); + if (!loanId) { + return throwError(() => new Error('Missing route param: loanId')); + } + return this.loansService.getWorkingCapitalPeriodPaymentRates(loanId); }#!/bin/bash # Verify current resolver null-safety and loanId flow to service rg -n -C3 "resolve\\(route: ActivatedRouteSnapshot\\)|paramMap.get\\('loanId'\\)|getWorkingCapitalPeriodPaymentRates\\(" src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts rg -n -C3 "getWorkingCapitalPeriodPaymentRates\\s*\\(" src/app/loans/loans.service.ts rg -n -C4 "path: ':loanId'|path: 'payment-rates'" src/app/loans/loans-routing.module.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts` around lines 28 - 30, The resolver's resolve method may pass a null/undefined loanId into loansService.getWorkingCapitalPeriodPaymentRates and accesses route.parent without a null check; update resolve( route: ActivatedRouteSnapshot ) to safely obtain loanId (use route.paramMap.get('loanId') ?? route.parent?.paramMap.get('loanId')), validate it and handle the missing case (e.g. throw an Error, return EMPTY/throwError, or navigate to an error route) before calling getWorkingCapitalPeriodPaymentRates(loanId) so the service is never called with a null loanId.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html (3)
41-41:⚠️ Potential issue | 🟡 MinorRemove the hard-coded space before
%.Line 41 and Line 49 render rates like
1.00 %instead of1.00%.🧹 Proposed fix
- {{ item.previousRate | formatNumber }} % + {{ item.previousRate | formatNumber }}% ... - {{ item.newRate | formatNumber }} % + {{ item.newRate | formatNumber }}%Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html` at line 41, The template currently prints a space before the percent sign causing "1.00 %"; update the loan-period-payment-rates.component.html expressions that render rates (e.g., the bindings that use item.previousRate and item.rate with the formatNumber pipe) to remove the hard-coded space so the percent sign is adjacent to the formatted number (e.g., change "{{ item.previousRate | formatNumber }} %" to "{{ item.previousRate | formatNumber }}%"; apply the same change to the binding that displays item.rate).
12-12:⚠️ Potential issue | 🟡 MinorUse an 8px-grid spacing token instead of
gap-20px.Line 12 uses a non-grid spacing value.
🎯 Proposed fix
- <div class="layout-row transaction-buttons align-end gap-20px"> + <div class="layout-row transaction-buttons align-end gap-16px">As per coding guidelines,
src/**/*.{scss,html}should “Stick to the 8px grid system for visual design and spacing.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html` at line 12, Replace the hardcoded "gap-20px" on the div with the project's 8px-grid spacing token; locate the element with classes "layout-row transaction-buttons align-end gap-20px" in loan-period-payment-rates.component.html and change "gap-20px" to the appropriate token that follows the 8px grid (e.g., the 16px or 24px spacing token used across the project), using the established token name/pattern your codebase uses (CSS variable or utility class) so spacing conforms to the 8px system.
12-21:⚠️ Potential issue | 🟠 MajorUndo action is missing from the toolbar.
The screen currently exposes only Add, so users cannot perform the undo flow from this page when payment-rate changes are reversible.
🛠️ Proposed fix
<div class="layout-row transaction-buttons align-end gap-20px"> <button mat-raised-button color="primary" (click)="addPaymentRate()" *mifosxHasPermission="'UPDATERATE_WORKINGCAPITALLOAN'" > {{ 'labels.buttons.Add' | translate }} {{ 'labels.inputs.Period Payment Rate' | translate }} </button> + <ng-container *ngIf="hasUndoablePaymentRates"> + <button + mat-raised-button + color="primary" + (click)="undoPaymentRate()" + *mifosxHasPermission="'UPDATERATE_WORKINGCAPITALLOAN'" + > + {{ 'labels.menus.Undo Payment Rate' | translate }} + </button> + </ng-container> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html` around lines 12 - 21, Add an "Undo" action to the toolbar so users can revert reversible payment-rate changes: in the template where the Add button calls addPaymentRate(), add a second button alongside it that calls a new undo handler (e.g., undoPaymentRate()) on the component and uses the same permission gating as appropriate; if LoanPeriodPaymentRatesComponent does not yet have undoPaymentRate(), implement it to perform the existing revert flow (or call the service method that performs the undo) and wire it to the template, using the translated label 'labels.buttons.Undo' (and reuse 'labels.inputs.Period Payment Rate' if desired) so the toolbar shows both Add and Undo.src/assets/translations/it-IT.json (1)
3165-3165:⚠️ Potential issue | 🟡 MinorKeep the period context in the undo label.
Line 3165 is still generic (
Annulla il tasso di pagamento) and can read as a non-period action. Make it explicit to the period-payment-rates context.🌐 Proposed fix
- "Undo Payment Rate": "Annulla il tasso di pagamento", + "Undo Payment Rate": "Annulla il tasso di pagamento del periodo",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/translations/it-IT.json` at line 3165, The translation for the key "Undo Payment Rate" is too generic; update the Italian value for "Undo Payment Rate" to explicitly reference the period context (e.g., include "periodo" or "del periodo") so it reads as an action on a period's payment rate—replace "Annulla il tasso di pagamento" with a phrase like "Annulla il tasso di pagamento del periodo" (or similar) in src/assets/translations/it-IT.json for the "Undo Payment Rate" entry.src/assets/translations/es-CL.json (1)
3167-3167:⚠️ Potential issue | 🟡 MinorKeep the undo label aligned with “del período” terminology.
Line 3167 still omits “del período”, which makes it inconsistent with the rest of the new payment-rate labels.
🌐 Suggested fix
- "Undo Payment Rate": "Deshacer tasa de pago", + "Undo Payment Rate": "Deshacer tasa de pago del período",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets/translations/es-CL.json` at line 3167, The Spanish translation for the key "Undo Payment Rate" is missing the "del período" suffix; update the value for the "Undo Payment Rate" entry from "Deshacer tasa de pago" to "Deshacer tasa de pago del período" so it matches the rest of the payment-rate labels and preserves consistency with the "del período" terminology.src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (2)
101-103:⚠️ Potential issue | 🟡 MinorUse a single i18n key for the full dialog title.
Line 102 concatenates translated fragments (
Add+ label). This is localization-fragile for languages with different word order/inflection.Proposed fix
const data = { - title: this.translateService.instant('labels.buttons.Add') + ' ' + labelHeadingText, + title: this.translateService.instant('labels.buttons.Add Period Payment Rate'), layout: { addButtonText: this.translateService.instant('labels.buttons.Add') }, formfields: formfields };As per coding guidelines, use proper i18n variables from
@ngx-translate/corefor all user-facing strings.#!/bin/bash set -euo pipefail # Verify usage in component rg -n -C2 "labels\.buttons\.Add|Add Period Payment Rate|title:" \ src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts # Verify key presence in translation JSON files rg -n "Add Period Payment Rate|labels\.buttons\.Add Period Payment Rate" -g "**/*.json" src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 101 - 103, Replace the concatenation of translated fragments in LoanPeriodPaymentRatesComponent: instead of building title as this.translateService.instant('labels.buttons.Add') + ' ' + labelHeadingText, introduce and use a single i18n key (e.g. 'loans.loanPeriodPaymentRate.dialogTitle') and call this.translateService.instant('loans.loanPeriodPaymentRate.dialogTitle') for the title; do the same for layout.addButtonText (use a single key like 'loans.loanPeriodPaymentRate.addButtonText' or embed both strings into one dialog key if they must match), and add the corresponding entries to your translation JSON files so all languages get a full-sentence/inflected translation rather than concatenated fragments.
73-75:⚠️ Potential issue | 🟠 MajorNormalize and guard
loanIdbefore storing/using it.Line 74 assigns a route param directly to
loanId: number | nulland assumesroute.parentis always defined. This can propagate invalid IDs to API calls.Proposed fix
ngOnInit(): void { - this.loanId = this.route.parent.snapshot.params['loanId']; + const rawLoanId = this.route.parent?.snapshot.paramMap.get('loanId') ?? this.route.snapshot.paramMap.get('loanId'); + const parsedLoanId = rawLoanId !== null ? Number(rawLoanId) : NaN; + this.loanId = Number.isFinite(parsedLoanId) ? parsedLoanId : null; this.route.data.subscribe((data: { loanPaymentRatesData: PeriodPaymentRateChange[] }) => { this.loanPaymentRatesData = data.loanPaymentRatesData; }); }As per coding guidelines,
src/app/**/*.tsmust use strict typing conventions.#!/bin/bash set -euo pipefail # Verify current assignment and declared type in the component rg -n -C2 "loanId\s*:\s*number\s*\|\s*null|snapshot\.params\['loanId'\]|paramMap\.get\('loanId'\)" \ src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts # Verify service method signatures/call sites for loanId expectations rg -n -C3 "addWorkingCapitalPeriodPaymentRate\s*\(|getWorkingCapitalPeriodPaymentRates\s*\(|undoWorkingCapitalPeriodPaymentRate\s*\(" \ src/app/loans/loans.service.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 73 - 75, In ngOnInit of LoanPeriodPaymentRatesComponent, don't assign route.parent.snapshot.params['loanId'] directly to loanId; instead guard for route.parent existing, read the param via this.route.parent?.snapshot?.params or this.route.parent?.paramMap.get('loanId'), parse it to a number (e.g. const id = Number(raw) and check Number.isFinite(id)), and set this.loanId = Number.isFinite(id) ? id : null; also update any call sites in this component (methods like load/get/addWorkingCapitalPeriodPaymentRate/undoWorkingCapitalPeriodPaymentRate) to early-return or disable actions when this.loanId is null so you never pass an invalid ID into the LoansService.
🧹 Nitpick comments (1)
src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts (1)
107-118: Flatten nested subscriptions using RxJS operators.Lines 107-118 contain three chained
subscribe()calls that should be combined usingswitchMapandfilteroperators for better composability and error handling:Refactoring example
+import { filter, switchMap } from 'rxjs/operators'; - dialogRef.afterClosed().subscribe((response: any) => { - if (response?.data) { - const { periodPaymentRate, note } = response.data.value; - const payload = { - periodPaymentRate: periodPaymentRate, - note: note || '', - locale: this.settingsService.language.code, - dateFormat: this.settingsService.dateFormat - }; - this.loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload).subscribe((response: any) => { - this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId).subscribe((data: any) => { - this.loanPaymentRatesData = data; - }); - }); - } - }); + dialogRef + .afterClosed() + .pipe( + filter((response: any) => !!response?.data), + switchMap((response) => { + const { periodPaymentRate, note } = response.data.value; + const payload = { + periodPaymentRate, + note: note || '', + locale: this.settingsService.language.code, + dateFormat: this.settingsService.dateFormat + }; + return this.loanService + .addWorkingCapitalPeriodPaymentRate(this.loanId, payload) + .pipe(switchMap(() => this.loanService.getWorkingCapitalPeriodPaymentRates(this.loanId))); + }) + ) + .subscribe((data: any) => { + this.loanPaymentRatesData = data; + });Type improvements for the
Observable<any>responses align with coding guidelines but are tracked as part of broader API typing enhancements across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts` around lines 107 - 118, The code currently nests three subscribe() calls (dialogRef.afterClosed, loanService.addWorkingCapitalPeriodPaymentRate, loanService.getWorkingCapitalPeriodPaymentRates); refactor by piping dialogRef.afterClosed() through filter(response => !!response?.data), map to build the payload (using settingsService.language.code and settingsService.dateFormat), then switchMap to loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload) and another switchMap to loanService.getWorkingCapitalPeriodPaymentRates(this.loanId), finally subscribe once and assign the result to this.loanPaymentRatesData; optionally add catchError to the pipeline for error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/loans/loans-routing.module.ts`:
- Around line 154-161: Replace the hardcoded user-facing strings in the route
definition for path 'payment-rates' (data.title and data.breadcrumb) with i18n
translation keys (e.g. use keys like 'loans.paymentRates.title' and
'loans.paymentRates.breadcrumb') so the route config that references
LoanPeriodPaymentRatesComponent contains only translation keys; update any
consumers that read route.data to pass keys into the translate service or use
TranslatePipe, and then run the extraction command (npm run
translations:extract) to add the new keys to the translation files.
---
Duplicate comments:
In
`@src/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.ts`:
- Around line 28-30: The resolver's resolve method may pass a null/undefined
loanId into loansService.getWorkingCapitalPeriodPaymentRates and accesses
route.parent without a null check; update resolve( route: ActivatedRouteSnapshot
) to safely obtain loanId (use route.paramMap.get('loanId') ??
route.parent?.paramMap.get('loanId')), validate it and handle the missing case
(e.g. throw an Error, return EMPTY/throwError, or navigate to an error route)
before calling getWorkingCapitalPeriodPaymentRates(loanId) so the service is
never called with a null loanId.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.html`:
- Line 41: The template currently prints a space before the percent sign causing
"1.00 %"; update the loan-period-payment-rates.component.html expressions that
render rates (e.g., the bindings that use item.previousRate and item.rate with
the formatNumber pipe) to remove the hard-coded space so the percent sign is
adjacent to the formatted number (e.g., change "{{ item.previousRate |
formatNumber }} %" to "{{ item.previousRate | formatNumber }}%"; apply the same
change to the binding that displays item.rate).
- Line 12: Replace the hardcoded "gap-20px" on the div with the project's
8px-grid spacing token; locate the element with classes "layout-row
transaction-buttons align-end gap-20px" in
loan-period-payment-rates.component.html and change "gap-20px" to the
appropriate token that follows the 8px grid (e.g., the 16px or 24px spacing
token used across the project), using the established token name/pattern your
codebase uses (CSS variable or utility class) so spacing conforms to the 8px
system.
- Around line 12-21: Add an "Undo" action to the toolbar so users can revert
reversible payment-rate changes: in the template where the Add button calls
addPaymentRate(), add a second button alongside it that calls a new undo handler
(e.g., undoPaymentRate()) on the component and uses the same permission gating
as appropriate; if LoanPeriodPaymentRatesComponent does not yet have
undoPaymentRate(), implement it to perform the existing revert flow (or call the
service method that performs the undo) and wire it to the template, using the
translated label 'labels.buttons.Undo' (and reuse 'labels.inputs.Period Payment
Rate' if desired) so the toolbar shows both Add and Undo.
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 101-103: Replace the concatenation of translated fragments in
LoanPeriodPaymentRatesComponent: instead of building title as
this.translateService.instant('labels.buttons.Add') + ' ' + labelHeadingText,
introduce and use a single i18n key (e.g.
'loans.loanPeriodPaymentRate.dialogTitle') and call
this.translateService.instant('loans.loanPeriodPaymentRate.dialogTitle') for the
title; do the same for layout.addButtonText (use a single key like
'loans.loanPeriodPaymentRate.addButtonText' or embed both strings into one
dialog key if they must match), and add the corresponding entries to your
translation JSON files so all languages get a full-sentence/inflected
translation rather than concatenated fragments.
- Around line 73-75: In ngOnInit of LoanPeriodPaymentRatesComponent, don't
assign route.parent.snapshot.params['loanId'] directly to loanId; instead guard
for route.parent existing, read the param via
this.route.parent?.snapshot?.params or
this.route.parent?.paramMap.get('loanId'), parse it to a number (e.g. const id =
Number(raw) and check Number.isFinite(id)), and set this.loanId =
Number.isFinite(id) ? id : null; also update any call sites in this component
(methods like
load/get/addWorkingCapitalPeriodPaymentRate/undoWorkingCapitalPeriodPaymentRate)
to early-return or disable actions when this.loanId is null so you never pass an
invalid ID into the LoansService.
In `@src/assets/translations/es-CL.json`:
- Line 3167: The Spanish translation for the key "Undo Payment Rate" is missing
the "del período" suffix; update the value for the "Undo Payment Rate" entry
from "Deshacer tasa de pago" to "Deshacer tasa de pago del período" so it
matches the rest of the payment-rate labels and preserves consistency with the
"del período" terminology.
In `@src/assets/translations/it-IT.json`:
- Line 3165: The translation for the key "Undo Payment Rate" is too generic;
update the Italian value for "Undo Payment Rate" to explicitly reference the
period context (e.g., include "periodo" or "del periodo") so it reads as an
action on a period's payment rate—replace "Annulla il tasso di pagamento" with a
phrase like "Annulla il tasso di pagamento del periodo" (or similar) in
src/assets/translations/it-IT.json for the "Undo Payment Rate" entry.
---
Nitpick comments:
In
`@src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.ts`:
- Around line 107-118: The code currently nests three subscribe() calls
(dialogRef.afterClosed, loanService.addWorkingCapitalPeriodPaymentRate,
loanService.getWorkingCapitalPeriodPaymentRates); refactor by piping
dialogRef.afterClosed() through filter(response => !!response?.data), map to
build the payload (using settingsService.language.code and
settingsService.dateFormat), then switchMap to
loanService.addWorkingCapitalPeriodPaymentRate(this.loanId, payload) and another
switchMap to loanService.getWorkingCapitalPeriodPaymentRates(this.loanId),
finally subscribe once and assign the result to this.loanPaymentRatesData;
optionally add catchError to the pipeline for error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bc01e52-cd6d-4bc9-91c9-c67eacdf2941
📒 Files selected for processing (24)
src/app/loans/loans-routing.module.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/working-capital/common-resolvers/loan-period-payment-rates.resolver.tssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.htmlsrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scsssrc/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.tssrc/app/loans/loans.service.tssrc/app/loans/models/working-capital-loan-account.model.tssrc/app/shared/undo-dialog/undo-dialog.component.htmlsrc/app/shared/undo-dialog/undo-dialog.component.scsssrc/app/shared/undo-dialog/undo-dialog.component.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (13)
- src/app/shared/undo-dialog/undo-dialog.component.scss
- src/app/loans/loans.service.ts
- src/assets/translations/ko-KO.json
- src/app/loans/models/working-capital-loan-account.model.ts
- src/assets/translations/es-MX.json
- src/assets/translations/en-US.json
- src/assets/translations/lv-LV.json
- src/assets/translations/lt-LT.json
- src/assets/translations/cs-CS.json
- src/assets/translations/ne-NE.json
- src/assets/translations/sw-SW.json
- src/app/shared/undo-dialog/undo-dialog.component.ts
- src/assets/translations/de-DE.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/shared/undo-dialog/undo-dialog.component.html
- src/app/loans/loans-view/working-capital/loan-period-payment-rates/loan-period-payment-rates.component.scss
- src/app/loans/loans-view/loans-view.component.html
- src/assets/translations/fr-FR.json
- src/assets/translations/pt-PT.json
| { | ||
| path: 'payment-rates', | ||
| component: LoanPeriodPaymentRatesComponent, | ||
| data: { title: 'Period Payment Rates', breadcrumb: 'Period Payment Rates', routeParamBreadcrumb: false }, | ||
| resolve: { | ||
| loanPaymentRatesData: LoanPeriodPaymentRatesResolver | ||
| } | ||
| }, |
There was a problem hiding this comment.
Use translation keys instead of hardcoded route labels.
Line 157 introduces new user-facing strings (title/breadcrumb) as literals. Please switch to i18n keys and update extraction so the labels are translatable.
As per coding guidelines, "Use proper i18n variables from @ngx-translate/core for all user-facing strings instead of hardcoded text" and "Run npm run translations:extract to extract i18n variables whenever new strings are added to code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/loans-routing.module.ts` around lines 154 - 161, Replace the
hardcoded user-facing strings in the route definition for path 'payment-rates'
(data.title and data.breadcrumb) with i18n translation keys (e.g. use keys like
'loans.paymentRates.title' and 'loans.paymentRates.breadcrumb') so the route
config that references LoanPeriodPaymentRatesComponent contains only translation
keys; update any consumers that read route.data to pass keys into the translate
service or use TranslatePipe, and then run the extraction command (npm run
translations:extract) to add the new keys to the translation files.
Description
For Working Capital loans we have a new loan tab for Payment Rates where we can see the different Payment Rate Changes that a WC Loan could have
(WEB-657)WEB-657
Related issues and discussion
Screenshots
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Localization