WEB-874 Prevent negative values in numeric fields of Group#3412
Open
JaySoni1 wants to merge 1 commit intoopenMF:devfrom
Open
WEB-874 Prevent negative values in numeric fields of Group#3412JaySoni1 wants to merge 1 commit intoopenMF:devfrom
JaySoni1 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Loans Account Terms Step src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html, src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts |
Added min="0" HTML attributes to numeric inputs and Validators.min(0) to form controls for numberOfRepayments, repaymentEvery, interestRatePerPeriod, inArrearsTolerance, graceOnInterestCharged, graceOnPrincipalPayment, graceOnInterestPayment, and graceOnArrearsAgeing. |
Savings Account Terms Step src/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.html, src/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.ts |
Added min="0" HTML attributes and Validators.min(0) to form controls for nominalAnnualInterestRate, minRequiredOpeningBalance, lockinPeriodFrequency, minRequiredBalance, minOverdraftForInterestCalculation, nominalAnnualInterestRateOverdraft, overdraftLimit, and minBalanceForInterestCalculation. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-772 Prevent negative values in savings product settings fields #3206: Adds identical non-negativity constraints to overlapping overdraft-related form controls (
minOverdraftForInterestCalculation,nominalAnnualInterestRateOverdraft,overdraftLimit). - WEB-568 Negative values allowed for arrears ageing overdue days for npa and principal threshold fields in loan product settings #2983: Applies similar min validation patterns to numeric fields, including
graceOnArrearsAgeingwhich appears in both PRs. - WEB-680 Fix provisioning criteria dialog allows negative values in age percentage fields #3111: Implements centralized min/max validation handling via FormGroupService, providing an alternative approach to field-level constraint management.
Suggested reviewers
- IOhacker
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'WEB-874 Prevent negative values in numeric fields of Group' clearly and specifically summarizes the main change: adding min(0) validation constraints to numeric input fields across loans and savings account forms. |
| 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
📝 Coding Plan
- Generate coding plan for human review comments
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts (1)
452-468:⚠️ Potential issue | 🟠 MajorBug:
Validators.min(0)is not applied wheninterestRatePerPeriodis recreated.The
setAdvancedPaymentStrategyControls()method removes and recreates theinterestRatePerPeriodcontrol dynamically, but only appliesValidators.required. This overwrites theValidators.min(0)defined increateloansAccountTermsForm(), making the min validation ineffective.🐛 Proposed fix to include Validators.min(0)
if (this.loansAccountTermsData.product.fixedLength) { this.loansAccountTermsForm.addControl( 'interestRatePerPeriod', - new UntypedFormControl({ value: 0, disabled: true }, Validators.required) + new UntypedFormControl({ value: 0, disabled: true }, [Validators.required, Validators.min(0)]) ); this.loansAccountTermsForm.addControl( 'fixedLength', new UntypedFormControl(this.loansAccountTermsData.product.fixedLength) ); } else { this.loansAccountTermsForm.addControl( 'interestRatePerPeriod', - new UntypedFormControl(this.loansAccountTermsData.interestRatePerPeriod, Validators.required) + new UntypedFormControl(this.loansAccountTermsData.interestRatePerPeriod, [Validators.required, Validators.min(0)]) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts` around lines 452 - 468, The interestRatePerPeriod control is being removed and re-added in setAdvancedPaymentStrategyControls (inside loans-account-terms-step.component) without reapplying Validators.min(0), which breaks the minimum-value validation defined in createloansAccountTermsForm(); when recreating interestRatePerPeriod (both the disabled and enabled branches) add Validators.min(0) alongside Validators.required so the minimum validation is preserved on the new UntypedFormControl instances on loansAccountTermsForm.src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html (2)
129-145:⚠️ Potential issue | 🟡 MinorChange
min="0"tomin="1"and update validator to match loan product pattern.The
repaymentEveryfield currently allows 0 as a valid value (both in HTMLmin="0"and TypeScriptValidators.min(0)), which is invalid business logic—a loan cannot be repaid every 0 months/days. The loan product stepper correctly enforcesmin="1"in both the HTML attribute andValidators.min(1)in the form control. Align this component with that pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html` around lines 129 - 145, The template allows 0 for repayment interval; change the input min attribute from min="0" to min="1" and then update the form control validator in the loansAccountTermsForm initialization (the control named repaymentEvery) to use Validators.min(1) and the same pattern used by the loan product stepper so the form-level validation matches the HTML constraint and business rule.
69-84:⚠️ Potential issue | 🟡 MinorUpdate "Number of repayments" validation to require a minimum of 1 repayment.
The field currently allows a minimum of 0 via
min="0"andValidators.min(0), but a loan with 0 repayments is invalid. SincecalculateLoanTerm()multipliesnumberOfRepayments × repaymentEveryto set the loan term, allowing 0 would result in a 0 loan term. Change both the HTML attribute and TypeScript validator tomin="1"andValidators.min(1)to enforce at least one repayment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html` around lines 69 - 84, The Number of repayments input and its validator allow 0 which permits a 0-length loan term; update both the template input attribute min and the form validator to require at least 1. Change the HTML input's min="0" to min="1" for the field bound by formControlName="numberOfRepayments", and update the TypeScript form control where Validators.min(0) is used (on loansAccountTermsForm.controls.numberOfRepayments or where the FormControl is constructed) to Validators.min(1); ensure calculateLoanTerm() continues to use loansAccountTermsForm.controls.numberOfRepayments.value safely after this change.
🧹 Nitpick comments (1)
src/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.html (1)
93-96: Consider adding<mat-error>for min validation feedback.Fields like
minRequiredOpeningBalance,lockinPeriodFrequency, andminRequiredBalancenow havemin="0"constraints but lack corresponding<mat-error>elements to display validation feedback when users attempt to enter negative values. While the HTML5minattribute prevents negative input in most browsers, adding error messages improves accessibility and provides clearer feedback.💡 Example: Add mat-error for minRequiredOpeningBalance
<mat-form-field class="flex-48"> <mat-label>{{ 'labels.inputs.Minimum Opening Balance' | translate }}</mat-label> <input type="number" matInput formControlName="minRequiredOpeningBalance" min="0" /> + `@if` (savingsAccountTermsForm.controls.minRequiredOpeningBalance.hasError('min')) { + <mat-error> + {{ 'labels.inputs.Minimum Opening Balance' | translate }} + {{ 'labels.commons.must be' | translate }} + <strong>{{ 'labels.commons.non-negative' | translate }}</strong> + </mat-error> + } </mat-form-field>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.html` around lines 93 - 96, Add validation error UI for the numeric min constraints in the template: for each mat-form-field that uses formControlName="minRequiredOpeningBalance", "lockinPeriodFrequency", and "minRequiredBalance" inside savings-account-terms-step.component.html, add a <mat-error> that displays when the control has a min validation error (e.g., control.hasError('min') or control.errors?.min) and show a translated or clear message like "Minimum value is 0"; ensure the <mat-error> is bound to the same FormControl used by the input so users get accessible feedback when they enter values below 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.html`:
- Around line 129-145: The template allows 0 for repayment interval; change the
input min attribute from min="0" to min="1" and then update the form control
validator in the loansAccountTermsForm initialization (the control named
repaymentEvery) to use Validators.min(1) and the same pattern used by the loan
product stepper so the form-level validation matches the HTML constraint and
business rule.
- Around line 69-84: The Number of repayments input and its validator allow 0
which permits a 0-length loan term; update both the template input attribute min
and the form validator to require at least 1. Change the HTML input's min="0" to
min="1" for the field bound by formControlName="numberOfRepayments", and update
the TypeScript form control where Validators.min(0) is used (on
loansAccountTermsForm.controls.numberOfRepayments or where the FormControl is
constructed) to Validators.min(1); ensure calculateLoanTerm() continues to use
loansAccountTermsForm.controls.numberOfRepayments.value safely after this
change.
In
`@src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.ts`:
- Around line 452-468: The interestRatePerPeriod control is being removed and
re-added in setAdvancedPaymentStrategyControls (inside
loans-account-terms-step.component) without reapplying Validators.min(0), which
breaks the minimum-value validation defined in createloansAccountTermsForm();
when recreating interestRatePerPeriod (both the disabled and enabled branches)
add Validators.min(0) alongside Validators.required so the minimum validation is
preserved on the new UntypedFormControl instances on loansAccountTermsForm.
---
Nitpick comments:
In
`@src/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.html`:
- Around line 93-96: Add validation error UI for the numeric min constraints in
the template: for each mat-form-field that uses
formControlName="minRequiredOpeningBalance", "lockinPeriodFrequency", and
"minRequiredBalance" inside savings-account-terms-step.component.html, add a
<mat-error> that displays when the control has a min validation error (e.g.,
control.hasError('min') or control.errors?.min) and show a translated or clear
message like "Minimum value is 0"; ensure the <mat-error> is bound to the same
FormControl used by the input so users get accessible feedback when they enter
values below 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f62da7e5-cbab-4c93-8772-794502e5fe78
📒 Files selected for processing (4)
src/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.htmlsrc/app/loans/loans-account-stepper/loans-account-terms-step/loans-account-terms-step.component.tssrc/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.htmlsrc/app/savings/savings-account-stepper/savings-account-terms-step/savings-account-terms-step.component.ts
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
1 participant
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.
Changes Made :-
-Add min(0) validation to numeric fields in Savings Account Terms, Loans Account Terms, and GLIM account in GROUP .
WEB-874
Before :-
After :-
Summary by CodeRabbit