WEB-920 Add missing validation error messages and prevent negative values in recurring deposits form#3503
Merged
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Recurring Deposits Template src/app/deposits/recurring-deposits/.../recurring-deposits-account-settings-step.component.html |
Added mifosxPositiveInteger to numeric inputs and min="0" for recurringFrequency; added <mat-error> messages for depositPeriod, depositPeriodFrequencyId, and conditional expectedFirstDepositOnDate. |
Component Class src/app/deposits/recurring-deposits/.../recurring-deposits-account-settings-step.component.ts |
Imported/added PositiveIntegerDirective to imports; changed recurringDepositAccountSettingsForm to definite-assignment (!); added non-null assertions when accessing form controls (get(...)!) and in valueChanges subscriptions. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-921 Restrict negative values in fixed deposits frequency and period fields #3505 — Same pattern applied: add PositiveIntegerDirective and tighten form nullability in fixed-deposits component.
- WEB-462 Lock in period field allows zero and negative values in save product creation form #2888 — Adds/strengthens positive-integer validation for lock-in period fields in deposit product forms.
Suggested reviewers
- alberto-art3ch
- IOhacker
🚥 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 accurately describes the main changes: adding missing validation error messages and preventing negative values in the recurring deposits form. |
| 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.
alberto-art3ch
requested changes
Apr 13, 2026
Collaborator
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
We have added a shared control InputPositiveIntegerComponent and directive mifosxPositiveNumber to positive numbers, please review this and use them
Contributor
Author
|
Ok @alberto-art3ch I will update the PR soon |
…lues in recurring deposits form
JaySoni1
force-pushed
the
WEB-920-add-missing-validation-error-messages-and-prevent-negative-values-in-recurring-deposits-form
branch
from
6fcabde to
2af924c
Compare
Contributor
Author
|
@alberto-art3ch I have updated the PR please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts (1)
221-231:⚠️ Potential issue | 🟠 MajorRestore required validators when re-adding frequency controls.
When
isCalendarInheritedtoggles from true to false, the frequency controls are re-added withoutValidators.required, causing a silent loss of validation rules that exist in the initial form definition. This inconsistency allows invalid form submission when the calendar is no longer inherited.Update lines 228–230 to match the initial validation rules:
Preserve validation rules on control re-addition
- this.recurringDepositAccountSettingsForm.addControl('expectedFirstDepositOnDate', new UntypedFormControl()); - this.recurringDepositAccountSettingsForm.addControl('recurringFrequency', new UntypedFormControl('')); - this.recurringDepositAccountSettingsForm.addControl('recurringFrequencyType', new UntypedFormControl('')); + this.recurringDepositAccountSettingsForm.addControl('expectedFirstDepositOnDate', new UntypedFormControl('')); + this.recurringDepositAccountSettingsForm.addControl('recurringFrequency', new UntypedFormControl('', Validators.required)); + this.recurringDepositAccountSettingsForm.addControl('recurringFrequencyType', new UntypedFormControl('', Validators.required));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts` around lines 221 - 231, When handling isCalendarInherited valueChanges in the recurring-deposits-account-settings-step component, re-add the removed controls with the same validators they originally had instead of plain UntypedFormControl instances; update the branch that runs when isCalendarInherited is false to add expectedFirstDepositOnDate, recurringFrequency and recurringFrequencyType using new UntypedFormControl(..., Validators.required) (or the exact validator set used in the initial form definition) so recurringDepositAccountSettingsForm regains the original required validation rules.
🧹 Nitpick comments (1)
src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts (1)
57-57: Migrate form to typedFormGroupto enforce type safety.Line 57's
UntypedFormGroupweakens compile-time type guarantees throughout the component. Define a typedFormGroupinterface for this form shape and useFormGroup<T>instead, ensuring strict type safety aligned with coding guidelines requiring strict TypeScript conventions forsrc/app/**/*.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts` at line 57, The form property recurringDepositAccountSettingsForm is declared as UntypedFormGroup which loses compile-time safety; define a TypeScript interface (e.g., RecurringDepositAccountSettingsFormValue) describing the controls and their value types used in this component, replace UntypedFormGroup with FormGroup<RecurringDepositAccountSettingsFormValue>, and update all places that call this.recurringDepositAccountSettingsForm.get, .controls, .value, patchValue, etc., to use the typed shape (or cast to the interface types) so the component (and methods like any validators or submit handlers) benefit from strict typing and compile-time checks.
🤖 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/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts`:
- Around line 139-140: The patchValue call for the form control retrieved via
get('withHoldTax') is using this.recurringDepositsAccountTemplate and can pick
the wrong/undefined source when isNew uses
recurringDepositsAccountProductTemplate; change the source to the selected
account object (this.recurringDepositsAccount.withHoldTax) when patching the
control in the RecurringDepositsAccountSettingsStepComponent, and add a
safe/null check before calling patchValue so you only patch when
this.recurringDepositsAccount is defined.
- Around line 125-137: The component creates subscriptions to
withHoldTax.valueChanges in ngOnChanges/constructor without cleanup causing
leaks; implement OnDestroy, add a private destroy$: Subject<void> (or similar),
import takeUntil, and pipe all valueChanges subscriptions (including the one on
recurringDepositAccountSettingsForm.get('withHoldTax')) with
takeUntil(this.destroy$); call this.destroy$.next(); this.destroy$.complete() in
ngOnDestroy; alternatively, before creating a subscription check the control
exists and that you haven't already subscribed (e.g., guard on
recurringDepositAccountSettingsForm.contains('withHoldTax') or track
subscription refs) and unsubscribe/remove them when controls are removed (e.g.,
when removing 'taxGroupId').
---
Outside diff comments:
In
`@src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts`:
- Around line 221-231: When handling isCalendarInherited valueChanges in the
recurring-deposits-account-settings-step component, re-add the removed controls
with the same validators they originally had instead of plain UntypedFormControl
instances; update the branch that runs when isCalendarInherited is false to add
expectedFirstDepositOnDate, recurringFrequency and recurringFrequencyType using
new UntypedFormControl(..., Validators.required) (or the exact validator set
used in the initial form definition) so recurringDepositAccountSettingsForm
regains the original required validation rules.
---
Nitpick comments:
In
`@src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts`:
- Line 57: The form property recurringDepositAccountSettingsForm is declared as
UntypedFormGroup which loses compile-time safety; define a TypeScript interface
(e.g., RecurringDepositAccountSettingsFormValue) describing the controls and
their value types used in this component, replace UntypedFormGroup with
FormGroup<RecurringDepositAccountSettingsFormValue>, and update all places that
call this.recurringDepositAccountSettingsForm.get, .controls, .value,
patchValue, etc., to use the typed shape (or cast to the interface types) so the
component (and methods like any validators or submit handlers) benefit from
strict typing and compile-time checks.
🪄 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: 37f521a8-a035-40b2-b500-5e0de938e81b
📒 Files selected for processing (2)
src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.htmlsrc/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/deposits/recurring-deposits/recurring-deposits-account-stepper/recurring-deposits-account-settings-step/recurring-deposits-account-settings-step.component.html
Contributor
Author
|
@alberto-art3ch Thank You for the review |
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.
Changes Made :-
-Add validation error messages to required fields and restrict numeric inputs to non-negative values in recurring deposits account settings form when creating recurring deposit applications for client .
WEB-920
Before :-
After :-
Summary by CodeRabbit