WEB-467 Interest rates field allows zero and negative values in loan product creation form #2891
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Interest rate validators — TypeScript src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts |
Added Validators.min(0) and a decimal-pattern validator (up to 6 decimals) to minInterestRatePerPeriod, interestRatePerPeriod (retains Validators.required), and maxInterestRatePerPeriod. loanProductTerms getter now normalizes commas to dots for these fields before returning. |
Template updates — HTML src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html |
Added step="0.01" and min attributes to numeric inputs; replaced previous error blocks with separate conditional <mat-error> elements for required, min, and pattern validations and adjusted messages to reference decimal precision. |
Translations — locale JSONs src/assets/translations/... src/assets/translations/en-US.json, de-DE.json, fr-FR.json, es-CL.json, es-MX.json, it-IT.json, pt-PT.json, ko-KO.json, lt-LT.json, lv-LV.json, ne-NE.json, sw-SW.json, cs-CS.json |
Added labels.commons.Only up to 6 decimal places allowed localized entry across many locale files (some files show duplicate insertions). No behavioral changes beyond added strings. |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
- IOhacker
- alberto-art3ch
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title directly and accurately describes the main change: adding validation to prevent zero and negative values in the interest rate fields of the loan product creation form. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.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 as they are similar to previous changes (13)
- src/assets/translations/fr-FR.json
- src/assets/translations/pt-PT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/sw-SW.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lt-LT.json
- src/assets/translations/ne-NE.json
- src/assets/translations/de-DE.json
- src/assets/translations/lv-LV.json
- src/assets/translations/it-IT.json
- src/assets/translations/en-US.json
- src/assets/translations/es-MX.json
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🔇 Additional comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts (1)
506-521: Good approach to locale-aware decimal handling.The normalization logic correctly handles different locale decimal separators (comma vs dot) by converting to dot for backend compatibility. The string type check prevents errors, and since the pattern validation prevents multiple separators, the single
replacecall is sufficient.Optional: Consider explicit handling of empty values
While the current implementation works correctly, you could make the intent clearer for empty/falsy values:
const normalizeDecimal = (value: any) => { if (!value || typeof value !== 'string') { return value; } return value.replace(',', '.'); };This is purely stylistic; the existing logic is functionally correct.
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.
|
So the minimum value will be 0 and it can also be in decimal like 0.1 , etc ? |
|
Yes, value can be 0. Also values like 0.1 and .01 some regulation requieres
up to 6 decimals
El vie., 12 de diciembre de 2025 5:16 p. m., Soni Jay <
***@***.***> escribió:
… *JaySoni1* left a comment (openMF/web-app#2891)
<#2891 (comment)>
So the minimum value will be 0 and it can also be in decimal like 0.1 ,
etc ?
—
Reply to this email directly, view it on GitHub
<#2891 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALD2ZASKHQRAMFXTEGN6RST4BNEDBAVCNFSM6AAAAACO4K4WUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNBYGQ3TIMZUGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Ok @IOhacker I will update the PR soon |
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
13ff6b0 to
2b0e2ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html (2)
165-191: Min-error copy forinterestRatePerPeriodreads like a “required” sentence and is inconsistent with the other two
ForinterestRatePerPeriod, the min error currently renders “Default Nominal interest rate is 0” (Lines 185-188), while min/max use “Minimum Value must be 0” (Lines 166-168, 204-206). Consider aligning the message semantics (or introduce a dedicated “Minimum value must be X” message for all three).
162-210: Consider binding the min value in error messages instead of hardcodingThe component currently displays hardcoded
<strong>0</strong>in three min-error messages forminInterestRatePerPeriod,interestRatePerPeriod, andmaxInterestRatePerPeriod. Other components in the codebase (e.g.,formfield.component.htmlandinput-amount.component.html) use bound values like{{ formfield.min }}or{{ minVal }}for validator error messages. Consider extracting the min value to a component property and binding it in the error text to align with established patterns and improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html(3 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
🔇 Additional comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html (1)
157-210: The template code for interest rate inputs is correct and consistent with the TypeScript validators. All three fields (minInterestRatePerPeriod,interestRatePerPeriod,maxInterestRatePerPeriod) properly enforcemin=0via both the[min]="0"HTML attribute andValidators.min(0)in the component, and this is intentional to support the zero-interest loan product option. The pattern validator correctly enforces 6 decimal places. No misalignment exists between the template and component validators.Likely an incorrect or invalid review comment.
...products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
2b0e2ab to
94f7fb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html(4 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts(1 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(2 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(2 hunks)src/assets/translations/fr-FR.json(2 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(2 hunks)src/assets/translations/lv-LV.json(2 hunks)src/assets/translations/ne-NE.json(2 hunks)src/assets/translations/pt-PT.json(2 hunks)src/assets/translations/sw-SW.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/assets/translations/de-DE.json
- src/assets/translations/es-CL.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
🔇 Additional comments (3)
src/assets/translations/it-IT.json (1)
84-84: Translation addition is correct, but underlying validation requirement inconsistency needs clarification.The Italian translation for "Only up to 6 decimal places allowed" is accurate and properly formatted. However, the PR objectives state a minimum value of 1 (with example 1.1), while the PR comments confirm that the value should be allowed to be 0 with decimals like 0.1. You acknowledged IOhacker's feedback and indicated you'd update the PR soon—please ensure the form validation in the loan product component will allow both zero and decimal values (0.1, 0.01, etc.) up to 6 decimal places, rather than enforcing a minimum of 1.
src/assets/translations/cs-CS.json (1)
81-81: ✓ Translation addition is accurate and well-placed.The Czech translation correctly conveys the English message about the 6-decimal-place constraint. The key is appropriately placed in the
labels.commonssection alongside other validation messages.However, per the PR discussion thread, verify that the validator implementation aligns with the agreed-upon behavior: The comments indicate values ≥ 0 (including 0.1) should be allowed, but the PR title mentions "minimum value of 1"—this logic discrepancy should be resolved in the related validator/form files.
src/assets/translations/ko-KO.json (1)
84-84: ✅ Translation entry is well-formed and consistent.The new Korean translation for the 6-decimal-place validation message is correctly placed within the
labels.commonsnamespace and follows established naming conventions. The translation "6자리 이하의 소수점만 허용됩니다." accurately conveys the intent and aligns with the PR objective to enforce decimal place validation for interest rate fields.
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
94f7fb1 to
a2ff81f
Compare
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
a2ff81f to
4b90f24
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/assets/translations/en-US.json (1)
84-84: Duplicate translation key—remove to fix the previous review issue.This is a continuation of the issue flagged in the past review. The key
"labels.commons.Only up to 6 decimal places allowed"is being added at line 84, but it already exists at the root level elsewhere in the file (around line 3800). In JSON, duplicate keys at the root level cause the second definition to overwrite the first, making the earlier entry unreachable and creating dead code.Action: Remove the entry at line 84. Keep only the root-level definition (line ~3800).
Note: The same duplicate pattern likely exists across all locale files (cs-CS, de-DE, es-CL, es-MX, fr-FR, it-IT, ko-KO, lt-LT, lv-LV, ne-NE, pt-PT, sw-SW). Please clean up duplicates in those files as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html(3 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts(1 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/assets/translations/pt-PT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/it-IT.json
- src/assets/translations/lt-LT.json
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
- src/assets/translations/de-DE.json
- src/assets/translations/sw-SW.json
- src/assets/translations/es-CL.json
- src/assets/translations/lv-LV.json
- src/assets/translations/cs-CS.json
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🔇 Additional comments (2)
src/assets/translations/es-MX.json (1)
84-84: Inconsistency between AI summary and actual code content.The AI summary states: "The new entry is added in two distinct locations within the file, resulting in a duplicate of the same key." However, the provided code shows only one occurrence of
"labels.commons.Only up to 6 decimal places allowed"at line 84 within the errors section, and the file structure is valid JSON ending at line 3697.A previous review flagged a duplicate at line 3693 placed outside the proper JSON structure. The current code does not exhibit this issue—the file ends correctly with the countries section and closing braces.
If the duplicate still exists: Please verify the full file content and remove the out-of-structure duplicate at or near line 3693.
If the duplicate has been removed: The entry at line 84 is correctly positioned within the errors section, and the JSON structure is valid.
src/assets/translations/fr-FR.json (1)
84-84: No action needed. The translation key at line 84 is correctly placed with no duplicates in this file or across other locale files. Each locale file (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) contains exactly one occurrence of this key.
...n-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
Show resolved
Hide resolved
|
@IOhacker I have updated this PR please review |
|
LGTM |
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Please review my comments
...products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
Outdated
Show resolved
Hide resolved
...products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
Outdated
Show resolved
Hide resolved
...n-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
4b90f24 to
80b6998
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts (1)
325-330: Validators are lost when controls are dynamically re-created.When
isLinkedToFloatingInterestRatestoggles fromtruetofalse, the interest rate controls are re-added without themin(0)andpatternvalidators defined increateLoanProductTermsForm(). This creates an inconsistency where validation only works on initial load but not after toggling.🔎 Proposed fix to preserve validators
} else { - this.loanProductTermsForm.addControl('minInterestRatePerPeriod', new UntypedFormControl('')); + this.loanProductTermsForm.addControl('minInterestRatePerPeriod', new UntypedFormControl('', [ + Validators.min(0), + Validators.pattern(/^\d+([.,]\d{1,6})?$/) + ])); this.loanProductTermsForm.addControl( 'interestRatePerPeriod', - new UntypedFormControl('', Validators.required) + new UntypedFormControl('', [ + Validators.required, + Validators.min(0), + Validators.pattern(/^\d+([.,]\d{1,6})?$/) + ]) ); - this.loanProductTermsForm.addControl('maxInterestRatePerPeriod', new UntypedFormControl('')); + this.loanProductTermsForm.addControl('maxInterestRatePerPeriod', new UntypedFormControl('', [ + Validators.min(0), + Validators.pattern(/^\d+([.,]\d{1,6})?$/) + ]));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html(3 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts(2 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/assets/translations/fr-FR.json
- src/assets/translations/cs-CS.json
- src/assets/translations/es-CL.json
- src/assets/translations/ne-NE.json
- src/assets/translations/ko-KO.json
- src/assets/translations/lv-LV.json
- src/assets/translations/sw-SW.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lt-LT.json
- src/assets/translations/de-DE.json
- src/assets/translations/en-US.json
- src/assets/translations/it-IT.json
- src/assets/translations/es-MX.json
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🔇 Additional comments (4)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts (2)
233-254: Validators added correctly for interest rate fields.The
Validators.min(0)and pattern validators are properly configured for the three interest rate fields. The pattern/^\d+([.,]\d{1,6})?$/handles both comma and dot decimal separators, which aligns with internationalization requirements raised by the reviewer.Note: The previous review flagged that leading decimals like
.01won't be accepted by this pattern. Verify if this is acceptable per requirements or if the pattern needs adjustment to/^(\d+([.,]\d{1,6})?|[.,]\d{1,6})$/.
506-521: Decimal normalization handles comma-to-dot conversion for backend compatibility.The
normalizeDecimalhelper correctly converts comma decimal separators to dots. However,String.replace()without a regex only replaces the first occurrence. If a user enters1,234,56(thousand separator + decimal), it becomes1.234,56.This is acceptable since the pattern validator would reject such invalid input upstream, so the normalizer only handles valid single-comma decimals.
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html (2)
160-169: Validation error messages properly implemented for minimum interest rate.The granular error handling with separate
*ngIfconditions forminandpatternerrors provides clear feedback. Thestep="0.000001"attribute guides users on allowed decimal precision.
205-211: Maximum interest rate error messages correctly implemented.The pattern and min error handling is consistent with the other interest rate fields.
...products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
80b6998 to
30c75a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts (2)
324-341: Validators lost when interest rate controls are dynamically re-created.When
isLinkedToFloatingInterestRatestoggles back tofalse, the interest rate controls are re-added without themin(0)andpatternvalidators. This means validation works on initial load but fails after the user toggles the floating rates checkbox.🔎 Proposed fix
} else { + const interestRatePattern = /^\d*[.,]?\d{1,6}$|^\d+$/; - this.loanProductTermsForm.addControl('minInterestRatePerPeriod', new UntypedFormControl('')); + this.loanProductTermsForm.addControl('minInterestRatePerPeriod', new UntypedFormControl('', [ + Validators.min(0), + Validators.pattern(interestRatePattern) + ])); this.loanProductTermsForm.addControl( 'interestRatePerPeriod', - new UntypedFormControl('', Validators.required) + new UntypedFormControl('', [ + Validators.required, + Validators.min(0), + Validators.pattern(interestRatePattern) + ]) ); - this.loanProductTermsForm.addControl('maxInterestRatePerPeriod', new UntypedFormControl('')); + this.loanProductTermsForm.addControl('maxInterestRatePerPeriod', new UntypedFormControl('', [ + Validators.min(0), + Validators.pattern(interestRatePattern) + ])); this.loanProductTermsForm.addControl(Consider extracting the pattern to a class constant to avoid duplication.
233-254: Regex pattern still rejects leading decimals like.01.The pattern
/^\d+([.,]\d{1,6})?$/requires at least one digit before the decimal point. Per the PR discussion, values like.01should be allowed (IOhacker confirmed ".01" is valid). The pattern needs to make the leading digit optional.🔎 Proposed fix
minInterestRatePerPeriod: [ '', [ Validators.min(0), - Validators.pattern(/^\d+([.,]\d{1,6})?$/) + Validators.pattern(/^\d*[.,]?\d{1,6}$|^\d+$/) ] ], interestRatePerPeriod: [ '', [ Validators.required, Validators.min(0), - Validators.pattern(/^\d+([.,]\d{1,6})?$/) + Validators.pattern(/^\d*[.,]?\d{1,6}$|^\d+$/) ] ], maxInterestRatePerPeriod: [ '', [ Validators.min(0), - Validators.pattern(/^\d+([.,]\d{1,6})?$/) + Validators.pattern(/^\d*[.,]?\d{1,6}$|^\d+$/) ] ],This pattern allows:
.01,.123456(leading decimal)0.01,1.123456(standard decimal)1,100(integers)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html(3 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts(2 hunks)src/assets/translations/cs-CS.json(1 hunks)src/assets/translations/de-DE.json(1 hunks)src/assets/translations/en-US.json(1 hunks)src/assets/translations/es-CL.json(1 hunks)src/assets/translations/es-MX.json(1 hunks)src/assets/translations/fr-FR.json(1 hunks)src/assets/translations/it-IT.json(1 hunks)src/assets/translations/ko-KO.json(1 hunks)src/assets/translations/lt-LT.json(1 hunks)src/assets/translations/lv-LV.json(1 hunks)src/assets/translations/ne-NE.json(1 hunks)src/assets/translations/pt-PT.json(1 hunks)src/assets/translations/sw-SW.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/assets/translations/de-DE.json
- src/assets/translations/fr-FR.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lt-LT.json
- src/assets/translations/en-US.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lv-LV.json
- src/assets/translations/sw-SW.json
- src/assets/translations/ko-KO.json
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
🔇 Additional comments (4)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html (3)
153-170: LGTM: Minimum interest rate validation looks correct.The
step="0.000001"attribute properly guides users to 6 decimal places, and the conditional error messages forminandpatternvalidators are well-structured with proper i18n usage.
171-194: LGTM: Default interest rate validation and i18n properly implemented.The three separate error blocks handle
required,min, andpatternvalidation states correctly. The previously flagged hardcoded "is required" text has been replaced with the translate pipe.
195-212: LGTM: Maximum interest rate validation is consistent with other fields.The implementation follows the same pattern as the minimum and default fields, with proper step attribute and conditional error messages.
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts (1)
505-521: LGTM: Decimal normalization for backend compatibility.The getter properly normalizes comma decimal separators to dots for backend compatibility. This addresses the localization concern raised in past reviews. The implementation is clean and the single
replace()call is sufficient since the pattern validator prevents multiple decimal separators.
|
@alberto-art3ch I have updated the PR please review |
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
30c75a8 to
edcdae4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/assets/translations/es-MX.json (1)
84-84: Translation key is in the wrong section (previously flagged).The key
"labels.commons.Only up to 6 decimal places allowed"is placed in theerrorssection, but the key name indicates it should be located in thelabels>commonssection (around line 769). This violates the file's organizational structure where key names match their section hierarchy.🔎 Suggested fix
Remove the key from line 84 and add it to the appropriate location in the
labels>commonssection:Remove from errors section (line 84):
"validation.msg.loan.principal.is.less.than.min": "El importe principal {{params[0].value}} no es válido. Debe ser un monto mayor o igual al monto mínimo de capital {{params[1].value}}.", "validation.msg.loanproduct.accountingType.cannot.be.blank": "El tipo de contabilidad es obligatorio.", - "labels.commons.Only up to 6 decimal places allowed": "Solo se permiten hasta 6 decimales.", "validation.msg.loanproduct.accountingType.is.not.within.expected.range": "El tipo de contabilidad no es válido. Debe ser un número entre {{params[1].value}} y {{params[2].value}} inclusive.",Add to labels > commons section (around line 769):
"commons": { "50 characters long": "50 caracteres de largo", "12 characters long": "12 caracteres de largo", + "Only up to 6 decimal places allowed": "Solo se permiten hasta 6 decimales.", "Amount must be": "La cantidad debe ser",Based on learnings, the key naming convention in this file requires that the hierarchical structure in the key name matches the actual section placement in the JSON file.
🧹 Nitpick comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html (1)
163-169: Consider reading minimum value dynamically from validator.Lines 165, 189, and 207 hard-code the minimum value as
<strong>0</strong>. For consistency with other fields in this form (e.g., line 16), consider reading the value from the validator's error object:<strong>{{ loanProductTermsForm.get('minInterestRatePerPeriod')?.errors?.['min']?.min || 0 }}</strong>This makes the error message self-maintaining if the validator's min value ever changes.
Also applies to: 186-193, 205-211
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.htmlsrc/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.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 as they are similar to previous changes (13)
- src/assets/translations/es-CL.json
- src/assets/translations/ne-NE.json
- src/assets/translations/de-DE.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lt-LT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/pt-PT.json
- src/assets/translations/fr-FR.json
- src/assets/translations/sw-SW.json
- src/assets/translations/it-IT.json
- src/assets/translations/lv-LV.json
- src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.ts
- src/assets/translations/en-US.json
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
🧠 Learnings (2)
📓 Common learnings
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
📚 Learning: 2025-12-09T14:24:31.429Z
Learnt from: alberto-art3ch
Repo: openMF/web-app PR: 2876
File: src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts:91-94
Timestamp: 2025-12-09T14:24:31.429Z
Learning: In the LoanReagingComponent (src/app/loans/loans-view/loan-account-actions/loan-reaging/loan-reaging.component.ts), the transactionAmount field uses both Validators.min and Validators.max set to the same value (loanTransactionData.amount). This is intentional - it requires users to enter the exact amount as a confirmation/validation step before submitting the re-age operation.
Applied to files:
src/app/products/loan-products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
...products/loan-product-stepper/loan-product-terms-step/loan-product-terms-step.component.html
Outdated
Show resolved
Hide resolved
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Please see my comments
| matInput | ||
| formControlName="minInterestRatePerPeriod" | ||
| [min]="0" | ||
| step="0.01" |
There was a problem hiding this comment.
I'm not sure if always the IR is only for two digits
There was a problem hiding this comment.
@alberto-art3ch I have chnaged 0.01 to 0.0001 so now it will be correct ?
| formControlName="interestRatePerPeriod" | ||
| required | ||
| [min]="0" | ||
| step="0.01" |
There was a problem hiding this comment.
I'm not sure if always the IR is only for two digits
There was a problem hiding this comment.
@alberto-art3ch I have chnaged 0.01 to 0.0001 so now it will be correct ?
| Validators.pattern(/^\d+([.,]\d{1,6})?$/) | ||
| ] | ||
| ], | ||
| maxInterestRatePerPeriod: [ |
There was a problem hiding this comment.
Please add a custom validator to review the maxInterestRatePerPeriod must be equal or greater than minInterestRatePerPeriod
…Product Creation Form
JaySoni1
force-pushed
the
WEB-467-interest-rates-field-allows-zero-and-negative-values-in-loan-product-creation-form
branch
from
edcdae4 to
88b52c0
Compare
|
@alberto-art3ch Thank You for the review |
3 participants
Changes Made :-
-Added validation to ensure the "Interest Rates" fields (minimum, default, and maximum nominal interest rates) only accept positive values in the Create Loan Product form.
WEB-467
Before :-
After :-
Summary by CodeRabbit
Bug Fixes
New Features
Documentation / Localization
✏️ Tip: You can customize this high-level summary in your review settings.