Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
New Update Discount Component src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts, src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html |
New standalone mifosx-update-discount component with reactive form (discountAmount, note), validations (required, min 0, numeric, maxlength 500), submit flow calling LoansService.updateWorkingCapitalLoanDiscount(), success/error handling and navigation. |
Resolver & Action Integration src/app/loans/common-resolvers/loan-action-button.resolver.ts, src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts, src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.html |
Resolver gains branch for "Update discount" to return working-capital loan details; component imports UpdateDiscountComponent; adds 'Update discount' action flag and conditional template rendering. |
Loan view button logic src/app/loans/loans-view/loans-view.component.ts |
Adds conditional "Update discount" action in setConditionalButtons() and new helper canShowWorkingCapitalDiscountUpdate() that requires working-capital product and active loan status. |
Service API src/app/loans/loans.service.ts |
Adds exported WorkingCapitalLoanDiscountUpdateRequest interface and updateWorkingCapitalLoanDiscount(loanId, data) method performing PUT to /working-capital-loans/{loanId}/discount. |
Translations (multiple locales) src/assets/translations/... cs-CS.json, de-DE.json, en-US.json, es-CL.json, es-MX.json, fr-FR.json, it-IT.json, ko-KO.json, lt-LT.json, lv-LV.json, ne-NE.json, pt-PT.json, sw-SW.json |
Adds validation labels (Discount must be greater than or equal to 0, Note is too long, Please enter a valid number), action label Update discount, and messages workingCapitalDiscountUpdated / unableToUpdateDiscount; adjusts messages.validationSaved punctuation in several locales. |
Sequence Diagram
sequenceDiagram
participant User as User
participant UI as Loan Account Actions UI
participant Resolver as LoanActionButtonResolver
participant UpdateComp as UpdateDiscountComponent
participant LoansService as LoansService
participant API as Backend API
User->>UI: Click "Update discount"
UI->>Resolver: resolve(action="Update discount", loanId)
Resolver->>LoansService: getWorkingCapitalLoanDetails(loanId)
LoansService->>API: GET /working-capital-loans/{loanId}
API-->>LoansService: 200 Loan details
LoansService-->>Resolver: loan details
Resolver-->>UI: actionButtonData
UI->>UpdateComp: render with actionButtonData
UpdateComp->>UpdateComp: init form with discount
User->>UpdateComp: submit form (discountAmount, note)
UpdateComp->>LoansService: updateWorkingCapitalLoanDiscount(loanId, payload)
LoansService->>API: PUT /working-capital-loans/{loanId}/discount
alt Success
API-->>LoansService: 200 OK
LoansService-->>UpdateComp: success
UpdateComp->>User: show success & navigate back
else Error
API-->>LoansService: HTTP error
LoansService-->>UpdateComp: error details
UpdateComp->>User: display mapped error message
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-670: Working Capital loan delinquency grace days #3440 — modifies loan resolvers to return working-capital loan details for related action cases; closely related to the resolver change here.
Suggested reviewers
- alberto-art3ch
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'WEB-889: WC - Discount' is vague and generic, using abbreviations and minimal context that don't convey the specific change being made. | Consider a more descriptive title like 'Add ability to update working capital loan discount after disbursement' that clearly explains what functionality is being added. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/assets/translations/it-IT.json (1)
2978-2978: Use a more natural Italian phrasing on Line 2978.
"Aggiorna sconto"is understandable, but"Aggiorna lo sconto"is more idiomatic for a menu action label.✏️ Suggested wording tweak
- "Update discount": "Aggiorna sconto", + "Update discount": "Aggiorna lo sconto",🤖 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 2978, Replace the current Italian value for the translation key "Update discount" with a more natural phrase; locate the JSON entry with the key "Update discount" and change its value from "Aggiorna sconto" to "Aggiorna lo sconto" so the menu/action label reads idiomatically in Italian.src/assets/translations/es-CL.json (1)
2262-2262: Consider wording consistency with existing es-CL prompts.Line 2262 uses “introduzca”, while much of this locale file uses “ingrese”. Consider aligning phrasing for a more consistent UX tone.
🤖 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 2262, The translation for the key "Please enter a valid number" in the es-CL locale uses "introduzca" which is inconsistent with the rest of the file that prefers "ingrese"; update the value to "Por favor, ingrese un número válido" to match existing phrasing and maintain a consistent UX tone (locate the string by the exact key "Please enter a valid number" in the es-CL translations).src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html (1)
47-47: Use 8px-grid spacing token/class instead ofgap-5px.
gap-5pxbreaks the project spacing convention. Please switch this action-row gap to an 8px-grid-compliant spacing utility/style token.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/loan-account-actions/update-discount/update-discount.component.html` at line 47, The mat-card-actions element currently uses a non-compliant class "gap-5px" (see the element with classes layout-row align-center gap-5px responsive-column); replace "gap-5px" with the project's 8px-grid spacing utility/token (for example the approved class such as gap-8 or spacing-8px depending on your stylesheet) so the action row follows the 8px grid spacing convention used across src/**/*.scss and .html.src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts (1)
12-13: Migrate to typed reactive forms for compile-time type safety.The component currently uses
UntypedFormBuilderandUntypedFormGroup, which lose type safety for thediscountAmountandnoteform fields. Migrating to typed forms will enable the TypeScript compiler to catch shape/type drift errors.♻️ Suggested refactor
-import { UntypedFormBuilder, UntypedFormGroup, Validators } from '@angular/forms'; +import { FormBuilder, FormControl, FormGroup, Validators } from '@angular/forms'; ... - private formBuilder = inject(UntypedFormBuilder); + private formBuilder = inject(FormBuilder); ... - updateDiscountForm: UntypedFormGroup; + updateDiscountForm!: FormGroup<{ + discountAmount: FormControl<number | null>; + note: FormControl<string>; + }>;Aligns with coding guideline:
src/app/**/*.ts— "Use TypeScript for all application code with strict typing conventions."Also applies to lines 33–39, 46–59, 70–73.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts` around lines 12 - 13, The component currently uses untyped reactive forms; define a typed interface (e.g., UpdateDiscountForm { discountAmount: number; note: string }) and replace UntypedFormBuilder/UntypedFormGroup with Angular's typed FormBuilder and FormGroup<UpdateDiscountForm> in the UpdateDiscountComponent, changing the injected UntypedFormBuilder to FormBuilder and the discountForm property to FormGroup<UpdateDiscountForm>; update the form creation (where discountAmount and note are initialized—likely in ngOnInit or createForm) to use the typed form controls and keep the existing Validators, and adjust any code that reads/writes discountForm.value or controls to use the typed names so the compiler enforces the form shape.src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts (1)
139-139: Prefer a shared action-name constant to avoid string drift.
'Update discount'is now repeated in multiple places. A shared typed constant keeps routing/action matching safer and easier to maintain.♻️ Suggested refactor
+// e.g. src/app/loans/loan-action.constants.ts +export const LOAN_ACTION_UPDATE_DISCOUNT = 'Update discount' as const;- 'Update discount': boolean; + [LOAN_ACTION_UPDATE_DISCOUNT]: boolean;- 'Update discount': false + [LOAN_ACTION_UPDATE_DISCOUNT]: false-@if (actions['Update discount']) { +@if (actions[LOAN_ACTION_UPDATE_DISCOUNT]) {-} else if (loanActionButton === 'Update discount') { +} else if (loanActionButton === LOAN_ACTION_UPDATE_DISCOUNT) {Also applies to: 180-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts` at line 139, Replace the repeated literal "Update discount" with a single exported constant and use that constant everywhere it appears; e.g., define export const ACTION_UPDATE_DISCOUNT = 'Update discount' in a shared constants file or at top of LoanAccountActionsComponent, change the mapped key from the literal ('Update discount': boolean) to a computed property ([ACTION_UPDATE_DISCOUNT]: boolean) and replace the other occurrences (the comparisons/uses around the 180–181 area) to reference ACTION_UPDATE_DISCOUNT so routing/action matching uses the single source of truth.
🤖 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/loan-account-actions/update-discount/update-discount.component.ts`:
- Around line 24-31: The `@Component` metadata for the UpdateDiscount component is
missing standalone: true which is required when using the imports array; update
the `@Component` decorator (in update-discount.component.ts for the
UpdateDiscountComponent) to include standalone: true alongside selector,
templateUrl and imports so Angular recognizes this as a standalone component and
compilation succeeds.
In `@src/app/loans/loans-view/loans-view.component.ts`:
- Around line 260-266: canShowWorkingCapitalDiscountUpdate() currently only
checks product type and active status; extend it to also (1) validate the loan
disbursement date meets the required window, (2) enforce one-time-only discount
(check existing discount records/flags so discounts cannot be updated more than
once), and (3) block the action when a discount was already added during
creation or approval workflows; update the buttonConfig.addButton call to rely
on this enhanced guard. Additionally, in the UpdateDiscountComponent.submit()
method, add the same three validations (disbursement date, one-time-only, and
creation/approval-added check) and abort submission with an appropriate
error/notification if any fail so the backend/UX cannot apply the update when
the action should be unavailable.
In `@src/app/loans/loans.service.ts`:
- Around line 563-569: Define a typed request payload interface (e.g.,
WorkingCapitalLoanDiscountPayload) and use it for the two methods instead of
data: any; specifically create an interface with the expected properties (for
example: discountAmount: number, currency?: string, reason?: string,
effectiveDate?: string, approverId?: string — adjust names/types to match the
API contract) and update updateWorkingCapitalLoanDiscount(loanId: string |
number, data: WorkingCapitalLoanDiscountPayload) and
updateWorkingCapitalLoanDiscountByExternalId(loanExternalId: string, data:
WorkingCapitalLoanDiscountPayload) while keeping the return type Observable<any>
unchanged.
- Around line 567-568: The new method
updateWorkingCapitalLoanDiscountByExternalId is currently unused; either remove
it or, if you intend to enable it, make the URL construction defensive by
URL-encoding loanExternalId (use encodeURIComponent) before interpolating into
the path in updateWorkingCapitalLoanDiscountByExternalId and apply the same
approach consistently as used in remittances.service.ts; ensure the http.put
call uses the encoded id so special characters won’t break the request.
In `@src/assets/translations/fr-FR.json`:
- Line 1805: Several French translation entries use "remise" while the base
label and nearby keys use "rabais"; update the translation value for the key
"Discount must be greater than or equal to 0" (and the other affected keys noted
in the comment) to use "rabais" instead of "remise" so the UI uses a single
consistent term; locate the JSON entries by their exact message keys (e.g.,
"Discount must be greater than or equal to 0") and replace "remise" with
"rabais" in each affected value, then run your i18n/string-consistency check to
confirm no other keys still use the old term.
In `@src/assets/translations/pt-PT.json`:
- Line 2260: The pt-PT translations use Brazilian variants; update the two
translation values for the English keys "Please enter a valid number" and the
string at the other mention (line with "capital de giro") to pt-PT-consistent
wording: replace "Por favor, insira um número válido" with "Por favor, introduza
um número válido" and replace the "capital de giro" phrasing with "O desconto do
empréstimo de fundo de maneio foi atualizado com sucesso." so both translations
consistently use European Portuguese; locate these by their English source
strings in src/assets/translations/pt-PT.json and update the values accordingly.
---
Nitpick comments:
In
`@src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts`:
- Line 139: Replace the repeated literal "Update discount" with a single
exported constant and use that constant everywhere it appears; e.g., define
export const ACTION_UPDATE_DISCOUNT = 'Update discount' in a shared constants
file or at top of LoanAccountActionsComponent, change the mapped key from the
literal ('Update discount': boolean) to a computed property
([ACTION_UPDATE_DISCOUNT]: boolean) and replace the other occurrences (the
comparisons/uses around the 180–181 area) to reference ACTION_UPDATE_DISCOUNT so
routing/action matching uses the single source of truth.
In
`@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html`:
- Line 47: The mat-card-actions element currently uses a non-compliant class
"gap-5px" (see the element with classes layout-row align-center gap-5px
responsive-column); replace "gap-5px" with the project's 8px-grid spacing
utility/token (for example the approved class such as gap-8 or spacing-8px
depending on your stylesheet) so the action row follows the 8px grid spacing
convention used across src/**/*.scss and .html.
In
`@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts`:
- Around line 12-13: The component currently uses untyped reactive forms; define
a typed interface (e.g., UpdateDiscountForm { discountAmount: number; note:
string }) and replace UntypedFormBuilder/UntypedFormGroup with Angular's typed
FormBuilder and FormGroup<UpdateDiscountForm> in the UpdateDiscountComponent,
changing the injected UntypedFormBuilder to FormBuilder and the discountForm
property to FormGroup<UpdateDiscountForm>; update the form creation (where
discountAmount and note are initialized—likely in ngOnInit or createForm) to use
the typed form controls and keep the existing Validators, and adjust any code
that reads/writes discountForm.value or controls to use the typed names so the
compiler enforces the form shape.
In `@src/assets/translations/es-CL.json`:
- Line 2262: The translation for the key "Please enter a valid number" in the
es-CL locale uses "introduzca" which is inconsistent with the rest of the file
that prefers "ingrese"; update the value to "Por favor, ingrese un número
válido" to match existing phrasing and maintain a consistent UX tone (locate the
string by the exact key "Please enter a valid number" in the es-CL
translations).
In `@src/assets/translations/it-IT.json`:
- Line 2978: Replace the current Italian value for the translation key "Update
discount" with a more natural phrase; locate the JSON entry with the key "Update
discount" and change its value from "Aggiorna sconto" to "Aggiorna lo sconto" so
the menu/action label reads idiomatically in Italian.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e76f89a-3ca3-4353-8293-7138afd4b117
📒 Files selected for processing (20)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.htmlsrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.tssrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.htmlsrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.tssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.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
src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts
Show resolved
Hide resolved
Comment on lines
+260
to
+266
| if (this.canShowWorkingCapitalDiscountUpdate()) { | ||
| this.buttonConfig.addButton({ | ||
| name: 'Update discount', | ||
| icon: 'edit', | ||
| taskPermissionName: 'DISBURSE_LOAN' | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate discount action wiring =="
rg -n --type ts -C3 "Update discount|canShowWorkingCapitalDiscountUpdate|workingCapital.*discount|discount.*update" src/app/loans
echo
echo "== Inspect resolver for action payload =="
fd 'loan-action-button.resolver.ts$' src/app --exec sed -n '1,260p' {}
echo
echo "== Inspect update-discount component for date/one-time/pre-existing checks =="
fd 'update-discount.component.ts$' src/app --exec sed -n '1,320p' {}
echo
echo "== Inspect loans service methods used by resolver/component =="
fd 'loans.service.ts$' src/app --exec rg -n -C4 "getWorkingCapital|updateWorkingCapital|discount|disbursement|businessDate|already" {}Repository: openMF/web-app
Length of output: 19810
🏁 Script executed:
# Get full implementation of canShowWorkingCapitalDiscountUpdate
sed -n '582,600p' src/app/loans/loans-view/loans-view.component.ts
echo "---"
# Check for any eligibility-related fields or conditions
rg -n "discount.*eligible|eligible.*discount|one.*time|disbursement.*date|discount.*created|discount.*approval" src/app/loans/loans-view --type ts
echo "---"
# Look at the loanDetailsData structure and what it contains
rg -n -A5 "loanDetailsData" src/app/loans/loans-view/loans-view.component.ts | head -60
echo "---"
# Check the update-discount template for any additional checks
fd 'update-discount.component.html$' src/app --exec cat {}Repository: openMF/web-app
Length of output: 6507
Update discount action visibility is missing full eligibility checks.
The guard at line 582 checks only working-capital product type and active status, but the PR rules require additional constraints: disbursement date validation, one-time-only enforcement, and blocking when already added during creation/approval. The update-discount component's submit() method (lines 42–65) does not validate these constraints either, exposing the action when it should be unavailable. Ensure all three eligibility criteria are enforced before showing the action and before allowing submission.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/loans/loans-view/loans-view.component.ts` around lines 260 - 266,
canShowWorkingCapitalDiscountUpdate() currently only checks product type and
active status; extend it to also (1) validate the loan disbursement date meets
the required window, (2) enforce one-time-only discount (check existing discount
records/flags so discounts cannot be updated more than once), and (3) block the
action when a discount was already added during creation or approval workflows;
update the buttonConfig.addButton call to rely on this enhanced guard.
Additionally, in the UpdateDiscountComponent.submit() method, add the same three
validations (disbursement date, one-time-only, and creation/approval-added
check) and abort submission with an appropriate error/notification if any fail
so the backend/UX cannot apply the update when the action should be unavailable.
src/app/loans/loans.service.ts
Outdated
Comment on lines
+567
to
+568
| updateWorkingCapitalLoanDiscountByExternalId(loanExternalId: string, data: any): Observable<any> { | ||
| return this.http.put(`/working-capital-loans/external-id/${loanExternalId}/discount`, data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify call sites and whether external IDs are constrained/sanitized before use.
rg -nP --type=ts -C3 '\bupdateWorkingCapitalLoanDiscountByExternalId\s*\('
rg -nP --type=ts -C2 '\b(externalId|loanExternalId|encodeURIComponent|encodeURI)\b'Repository: openMF/web-app
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find callers of updateWorkingCapitalLoanDiscountByExternalId
rg -nP --type=ts 'updateWorkingCapitalLoanDiscountByExternalId' --max-count=20
# Check other external ID path parameter patterns in loans.service.ts
echo "=== Checking loans.service.ts for external ID patterns ==="
sed -n '1,100p' src/app/loans/loans.service.ts | grep -n "external-id\|externalId" || echo "No matches in first 100 lines"
# Check remittances.service.ts which has a similar pattern
echo "=== Checking remittances.service.ts pattern ==="
sed -n '70,75p' src/app/remittances/remittances.service.ts
# Look for other methods with external IDs in URLs across services
echo "=== Searching for other external ID URL patterns ==="
rg -nP "external-id/\$\{" --type=ts -A2 -B2Repository: openMF/web-app
Length of output: 661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for working-capital-loans references (method might be unused or indirectly referenced)
echo "=== Searching for working-capital-loans references ==="
rg -nP "working-capital-loans" --type=ts
# Check if external IDs have format validation anywhere
echo "=== Checking for external ID validation patterns ==="
rg -nP "externalId.*validat|validat.*externalId" --type=ts -i | head -20
# Search for how external IDs are used in form validators
echo "=== Form validators for externalId ==="
rg -nP "externalId.*Validat" --type=ts -A2 | head -30Repository: openMF/web-app
Length of output: 1042
🏁 Script executed:
#!/bin/bash
set -euoripefail
# Get context around the method
echo "=== Context around updateWorkingCapitalLoanDiscountByExternalId ==="
sed -n '560,575p' src/app/loans/loans.service.ts
# Check file structure to see if method is new or part of broader pattern
echo "=== Methods with 'Discount' in loans.service.ts ==="
grep -n "Discount" src/app/loans/loans.service.ts
# Check if there's any test file that might reference this method
echo "=== Looking for test files ==="
find . -name "*loans*.service.spec.ts" -type f
# Search more carefully for any string-based method calls (dynamic invocation)
echo "=== Checking for dynamic method invocations ==="
rg -nP "\[.*updateWorkingCapitalLoanDiscountByExternalId|\"updateWorkingCapitalLoanDiscountByExternalId" --type=tsRepository: openMF/web-app
Length of output: 732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around the method
echo "=== Context around updateWorkingCapitalLoanDiscountByExternalId ==="
sed -n '560,575p' src/app/loans/loans.service.ts
# Check file structure to see if method is new or part of broader pattern
echo "=== Methods with 'Discount' in loans.service.ts ==="
grep -n "Discount" src/app/loans/loans.service.ts
# Check if there's any test file that might reference this method
echo "=== Looking for test files ==="
find . -name "*loans*.service.spec.ts" -type f 2>/dev/null || echo "No spec files found"
# Search for any indirect references
echo "=== Checking for any string-based method references ==="
rg -nP "updateWorkingCapitalLoanDiscount" --type=ts | head -20Repository: openMF/web-app
Length of output: 1629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the update-discount component to understand usage context
echo "=== update-discount.component.ts context ==="
sed -n '70,85p' src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts
# Check if there are other variants being prepared
grep -n "externalId\|updateWorkingCapitalLoanDiscount" src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts
# Check remittances service to see if the unencoded externalId pattern works
echo "=== Remittances service full context ==="
sed -n '65,85p' src/app/remittances/remittances.service.ts
# Look for any Fineract API documentation or type definitions that mention externalId format
echo "=== Checking model files for externalId type hints ==="
grep -n "externalId.*:" src/app/loans/models/loan-account.model.ts | head -5Repository: openMF/web-app
Length of output: 1578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Fineract API documentation or comments about externalId format
echo "=== Searching for externalId format documentation ==="
rg -nP "externalId.*format|externalId.*constraint|externalId.*[A-Za-z0-9\-]{" --type=ts -B2 -A2 | head -30
# Check if there are URL encoding utilities already in use elsewhere
echo "=== Looking for encodeURIComponent usage patterns ==="
rg -nP "encodeURIComponent" --type=ts -B3 -A1 | head -40
# Check the Fineract API path pattern - check a few working methods to see if they encode
echo "=== Checking other path parameters that might contain user input ==="
grep -n "http\.\(get\|put\|post\)\(\`.*\${" src/app/loans/loans.service.ts | head -10Repository: openMF/web-app
Length of output: 111
Method appears unused—if activated, consider encoding the external ID for URL safety.
Line 568 interpolates loanExternalId directly into the path. While a similar unencoded pattern exists in remittances.service.ts (line 73) and works in production, encoding the parameter is defensively sound if the Fineract API could receive special characters. However, this method has no call sites in the codebase; if it's new or will be activated, apply the suggested fix consistently with the encoding approach chosen elsewhere.
Suggested fix
updateWorkingCapitalLoanDiscountByExternalId(loanExternalId: string, data: any): Observable<any> {
- return this.http.put(`/working-capital-loans/external-id/${loanExternalId}/discount`, data);
+ const encodedExternalId = encodeURIComponent(loanExternalId);
+ return this.http.put(`/working-capital-loans/external-id/${encodedExternalId}/discount`, data);
}🤖 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 567 - 568, The new method
updateWorkingCapitalLoanDiscountByExternalId is currently unused; either remove
it or, if you intend to enable it, make the URL construction defensive by
URL-encoding loanExternalId (use encodeURIComponent) before interpolating into
the path in updateWorkingCapitalLoanDiscountByExternalId and apply the same
approach consistently as used in remittances.service.ts; ensure the http.put
call uses the encoded id so special characters won’t break the request.
mariiaKraievska
force-pushed
the
WEB-889/working-capital-loan-discount
branch
2 times, most recently
from
4cca692 to
c54fbf6
Compare
mariiaKraievska
force-pushed
the
WEB-889/working-capital-loan-discount
branch
from
c54fbf6 to
b1dae9c
Compare
mariiaKraievska
force-pushed
the
WEB-889/working-capital-loan-discount
branch
from
b1dae9c to
ed72c07
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts (1)
64-80:⚠️ Potential issue | 🟠 MajorBlock submit when discount eligibility constraints are not met.
Line 65 validates form and state, but it does not enforce the PR constraints (disbursement-date only, one-time only, and no prior creation/approval discount). This allows disallowed requests to still be submitted from the action component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts` around lines 64 - 80, The submit() method currently only checks form validity, isWorkingCapital, and isSubmitting; add explicit pre-submit guard checks for the PR constraints: verify the loan is disbursement-date only, is one-time only, and has no existing or approved discount before calling loanService.updateWorkingCapitalLoanDiscount; if any check fails set submitErrorMessage to a clear user-facing message, leave isSubmitting false, and return early. Locate these checks in update-discount.component.ts inside submit() (referencing updateDiscountForm, isWorkingCapital, isSubmitting, loanId, and loanService.updateWorkingCapitalLoanDiscount) and source the constraint data from the component’s loan model or service methods (e.g., loan object flags or a validation helper) to enforce the three conditions prior to performing the API call.src/app/loans/loans-view/loans-view.component.ts (1)
260-266:⚠️ Potential issue | 🟠 MajorEnforce full eligibility rules before showing “Update discount”.
Line 582 currently checks only working-capital type and active status, so Line 260 can still expose this action when PR rules should block it (disbursement-date only, one-time only, and not allowed if already added during creation/approval).
Also applies to: 582-587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loans-view.component.ts` around lines 260 - 266, The current conditional around buttonConfig.addButton relies on canShowWorkingCapitalDiscountUpdate() which only checks type/active status; update the conditional (or extend canShowWorkingCapitalDiscountUpdate()) so it enforces the full eligibility rules used elsewhere: require disbursement-date-only, one-time-only discount, and disallow if the discount was already added during creation/approval (reuse the same eligibility logic used for showing the action elsewhere instead of the simple type/active check). Ensure buttonConfig.addButton is only called when that comprehensive eligibility function returns true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts`:
- Around line 64-80: The submit() method currently only checks form validity,
isWorkingCapital, and isSubmitting; add explicit pre-submit guard checks for the
PR constraints: verify the loan is disbursement-date only, is one-time only, and
has no existing or approved discount before calling
loanService.updateWorkingCapitalLoanDiscount; if any check fails set
submitErrorMessage to a clear user-facing message, leave isSubmitting false, and
return early. Locate these checks in update-discount.component.ts inside
submit() (referencing updateDiscountForm, isWorkingCapital, isSubmitting,
loanId, and loanService.updateWorkingCapitalLoanDiscount) and source the
constraint data from the component’s loan model or service methods (e.g., loan
object flags or a validation helper) to enforce the three conditions prior to
performing the API call.
In `@src/app/loans/loans-view/loans-view.component.ts`:
- Around line 260-266: The current conditional around buttonConfig.addButton
relies on canShowWorkingCapitalDiscountUpdate() which only checks type/active
status; update the conditional (or extend canShowWorkingCapitalDiscountUpdate())
so it enforces the full eligibility rules used elsewhere: require
disbursement-date-only, one-time-only discount, and disallow if the discount was
already added during creation/approval (reuse the same eligibility logic used
for showing the action elsewhere instead of the simple type/active check).
Ensure buttonConfig.addButton is only called when that comprehensive eligibility
function returns true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90498839-c217-4c73-98a8-c7aa60d3890e
📒 Files selected for processing (20)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.htmlsrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.tssrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.htmlsrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.tssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.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 (4)
- src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.html
- src/assets/translations/ne-NE.json
- src/assets/translations/de-DE.json
- src/assets/translations/it-IT.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts
- src/assets/translations/fr-FR.json
- src/assets/translations/cs-CS.json
- src/assets/translations/es-MX.json
- src/assets/translations/sw-SW.json
- src/assets/translations/en-US.json
- src/assets/translations/lt-LT.json
- src/assets/translations/pt-PT.json
- src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html
alberto-art3ch
requested changes
Apr 1, 2026
Collaborator
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Few small comments, please review
| [ | ||
| Validators.required, | ||
| Validators.min(0), | ||
| Validators.pattern(/^\d+(\.\d+)?$/) |
Collaborator
There was a problem hiding this comment.
Please use the next custom validator for positive amount amountValueValidator() It must be imported
That replace the pattern Validator
| <div class="layout-column"> | ||
| <mat-form-field> | ||
| <mat-label>{{ 'labels.inputs.Discount' | translate }}</mat-label> | ||
| <input matInput type="number" min="0" step="0.01" formControlName="discountAmount" /> |
Collaborator
There was a problem hiding this comment.
use the directive mifosxPositiveNumber
Like:
<input
type="number"
matInput
required
mifosxPositiveNumber
formControlName="breachFrequency"
min="1"
step="1"
/>
| mat-raised-button | ||
| color="primary" | ||
| [disabled]="!updateDiscountForm.valid || isSubmitting" | ||
| *mifosxHasPermission="'DISBURSE_LOAN'" |
Collaborator
There was a problem hiding this comment.
Just double checking the right permission
| } | ||
|
|
||
| submit(): void { | ||
| if (!this.updateDiscountForm.valid || !this.isWorkingCapital || this.isSubmitting) { |
Collaborator
There was a problem hiding this comment.
I think this is not needed It
mariiaKraievska
force-pushed
the
WEB-889/working-capital-loan-discount
branch
from
ed72c07 to
95cff71
Compare
Contributor
Author
|
@alberto-art3ch Please take another look |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/common-resolvers/loan-action-button.resolver.ts`:
- Around line 102-103: The "Update discount" branch in the resolver calls a
misspelled service method getWorkingCapitalLoannDetails which doesn't exist;
change the call in the loanActionButton === 'Update discount' branch to use the
correct service method name getWorkingCapitalLoanDetails on this.loansService
(ensure the argument loanId remains unchanged) so the resolver invokes the real
method.
🪄 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: a8ba5537-de0b-4282-96cc-c6433562b3e1
📒 Files selected for processing (20)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.htmlsrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.tssrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.htmlsrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.tssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.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 (7)
- src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.html
- src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.ts
- src/app/loans/loans-view/loans-view.component.ts
- src/assets/translations/es-MX.json
- src/assets/translations/it-IT.json
- src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html
- src/assets/translations/ko-KO.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/assets/translations/pt-PT.json
- src/app/loans/loans.service.ts
- src/assets/translations/fr-FR.json
- src/assets/translations/en-US.json
- src/assets/translations/de-DE.json
- src/assets/translations/cs-CS.json
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
- src/assets/translations/lv-LV.json
mariiaKraievska
force-pushed
the
WEB-889/working-capital-loan-discount
branch
from
95cff71 to
52d2595
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/loans/loans-view/loans-view.component.ts (1)
582-587:⚠️ Potential issue | 🟠 MajorEnforce full discount eligibility in the guard, not just product type + active status.
Line 583-Line 586 currently allows the action when the loan is working-capital and active, but the feature rules also require: disbursement-date-only, one-time-only, and “not already added during creation/approval”. This incomplete guard can surface an invalid action path (used at Line 260).
[suggested fix] Centralize eligibility into one backend-driven flag (preferred) or explicit checks for all three constraints before returning
true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loans-view.component.ts` around lines 582 - 587, The current guard canShowWorkingCapitalDiscountUpdate only checks loanProductService.isWorkingCapital and loanDetailsData.status.active; update it to enforce full working-capital discount eligibility by either: 1) preferring a single backend-driven boolean (e.g., loanDetailsData.fullWorkingCapitalDiscountEligible) and returning that && loanDetailsData.status.active, or 2) if backend flag is unavailable, add explicit checks inside canShowWorkingCapitalDiscountUpdate for disbursement-date-only (e.g., loanDetailsData.discount?.disbursementDateOnly), one-time-only (e.g., loanDetailsData.discount?.oneTimeOnly), and that the discount was not already added during creation/approval (e.g., !loanDetailsData.discount?.addedDuringCreationOrApproval) in addition to loanProductService.isWorkingCapital and loanDetailsData.status.active; ensure the same consolidated guard is used wherever the feature is invoked (notably the caller that currently relies on this guard).
🧹 Nitpick comments (1)
src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts (1)
12-13: Use typed reactive forms instead ofUntyped*APIs to improve type safety.This component uses
UntypedFormBuilderandUntypedFormGroup, which bypasses compile-time checks for the form's shape. Since the form structure is static with clearly defined fields, typed forms would provide better IDE support and type safety.♻️ Suggested typed-form refactor
-import { UntypedFormBuilder, UntypedFormGroup, Validators } from '@angular/forms'; +import { FormBuilder, FormControl, FormGroup, Validators } from '@angular/forms'; @@ - private formBuilder = inject(UntypedFormBuilder); + private formBuilder = inject(FormBuilder); @@ - updateDiscountForm: UntypedFormGroup; + updateDiscountForm: FormGroup<{ + discountAmount: FormControl<number | null>; + note: FormControl<string>; + }>; @@ - this.updateDiscountForm = this.formBuilder.group({ + this.updateDiscountForm = this.formBuilder.nonNullable.group({ discountAmount: [ this.discountValue, [ Validators.required, Validators.min(0), amountValueValidator() ] ], note: [ '', Validators.maxLength(this.maxNoteLength) ] }); @@ - const formValue = this.updateDiscountForm.value; + const formValue = this.updateDiscountForm.getRawValue();Aligns with coding guideline
src/app/**/*.ts: "Use TypeScript for all application code with strict typing conventions."Also applies to: 38, 44, 75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts` around lines 12 - 13, The component is using UntypedFormBuilder/UntypedFormGroup which removes compile-time shape checks; replace them with typed Reactive Forms: import FormBuilder and FormGroup (not Untyped*), declare an interface for the form model (e.g. UpdateDiscountForm { discountAmount: number; reason?: string; ... } matching the component's form fields), update the constructor injection to use FormBuilder, and change the form property type to FormGroup<UpdateDiscountForm> and initialize it via this.fb.group(...) with typed controls and Validators so all usages (e.g. in update-discount.component class methods that reference the form) are type-checked and the imports updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/loans/loans-view/loans-view.component.ts`:
- Around line 582-587: The current guard canShowWorkingCapitalDiscountUpdate
only checks loanProductService.isWorkingCapital and
loanDetailsData.status.active; update it to enforce full working-capital
discount eligibility by either: 1) preferring a single backend-driven boolean
(e.g., loanDetailsData.fullWorkingCapitalDiscountEligible) and returning that &&
loanDetailsData.status.active, or 2) if backend flag is unavailable, add
explicit checks inside canShowWorkingCapitalDiscountUpdate for
disbursement-date-only (e.g., loanDetailsData.discount?.disbursementDateOnly),
one-time-only (e.g., loanDetailsData.discount?.oneTimeOnly), and that the
discount was not already added during creation/approval (e.g.,
!loanDetailsData.discount?.addedDuringCreationOrApproval) in addition to
loanProductService.isWorkingCapital and loanDetailsData.status.active; ensure
the same consolidated guard is used wherever the feature is invoked (notably the
caller that currently relies on this guard).
---
Nitpick comments:
In
`@src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.ts`:
- Around line 12-13: The component is using UntypedFormBuilder/UntypedFormGroup
which removes compile-time shape checks; replace them with typed Reactive Forms:
import FormBuilder and FormGroup (not Untyped*), declare an interface for the
form model (e.g. UpdateDiscountForm { discountAmount: number; reason?: string;
... } matching the component's form fields), update the constructor injection to
use FormBuilder, and change the form property type to
FormGroup<UpdateDiscountForm> and initialize it via this.fb.group(...) with
typed controls and Validators so all usages (e.g. in update-discount.component
class methods that reference the form) are type-checked and the imports updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0fd95cd-1590-4cf4-befe-3aa6fa1cf4f9
📒 Files selected for processing (20)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.htmlsrc/app/loans/loans-view/loan-account-actions/loan-account-actions.component.tssrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.htmlsrc/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.tssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans.service.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 (5)
- src/assets/translations/ne-NE.json
- src/assets/translations/it-IT.json
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
- src/assets/translations/ko-KO.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/app/loans/common-resolvers/loan-action-button.resolver.ts
- src/app/loans/loans-view/loan-account-actions/loan-account-actions.component.html
- src/app/loans/loans.service.ts
- src/assets/translations/pt-PT.json
- src/assets/translations/cs-CS.json
- src/assets/translations/es-CL.json
- src/assets/translations/de-DE.json
- src/app/loans/loans-view/loan-account-actions/update-discount/update-discount.component.html
- src/assets/translations/en-US.json
- src/assets/translations/fr-FR.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
User should be able to add the discount fee to the loan account even after disbursement.
Discount can be added only as of disbursement date only.
Discount can be added only one time at the time of disbursement.
User cannot add discount after disbursement if the discount was added at the loan creation/approval state.
Related issues and discussion
https://mifosforge.jira.com/browse/WEB-889
Screenshots, if any
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
Bug Fixes