WEB 460 Shares per Client Field Allows Zero and Negative Values in Share Product Creation Form #2880
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Share Product Settings — Component & Template src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts, src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html |
Added Validators.required, Validators.min(1), and numeric pattern to minimumShares, nominalShares, and maximumShares controls; added min="1" and step="1" to number inputs; introduced per-field mat-error blocks for required/min/pattern; added a form-level validateSharesOrder validator that sets a sharesOrder error when minimum > nominal or nominal > maximum. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Check numeric pattern correctness and localization/number parsing.
- Validate form-level
validateSharesOrderbehavior with null/undefined/empty values and when fields change. - Verify template error display priority (field vs. cross-field) and accessibility attributes.
Suggested reviewers
- IOhacker
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title directly and specifically describes the main change: adding validation to prevent zero and negative values in the shares fields, which matches the core objective of the PR. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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: 2
🧹 Nitpick comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html (1)
15-15: Consider if HTMLrequiredattribute is necessary.The
requiredattribute on the inputs provides native browser validation, which is somewhat redundant with theValidators.requiredin the TypeScript component. While not incorrect, the Angular validators will handle validation, and the native HTML validation might interfere with Angular Material's error message display timing.Also applies to: 27-27, 39-39
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html(1 hunks)src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts(1 hunks)
🧰 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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.htmlsrc/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-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:23.311Z
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:23.311Z
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:23.311Z
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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.htmlsrc/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts
...share-product-stepper/share-product-settings-step/share-product-settings-step.component.html
Show resolved
Hide resolved
...s/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts
Outdated
Show resolved
Hide resolved
JaySoni1
force-pushed
the
WEB-460-Shares-per-Client-Field-Allows-Zero-and-Negative-Values-in-Share-Product-Creation-Form
branch
from
d4a042b to
b5f7a32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts (1)
94-104: Consider usingValidationErrorstype for improved type safety.The validator logic is correct. The truthy check on line 98 appropriately skips cross-field validation until all fields have valid positive values.
For stricter typing per Angular guidelines, consider using the
ValidationErrorstype:+import { ValidationErrors } from '@angular/forms'; + - private validateSharesOrder(group: UntypedFormGroup): { [key: string]: any } | null { + private validateSharesOrder(group: UntypedFormGroup): ValidationErrors | null {Note:
ValidationErrorsis already available from@angular/formswhich is already imported.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html(1 hunks)src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-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:23.311Z
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:23.311Z
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:23.311Z
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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts
🔇 Additional comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts (1)
55-92: Validation implementation correctly addresses the PR objective.The validators properly enforce positive integers for share fields:
Validators.requiredprevents empty valuesValidators.min(1)prevents zero and negative valuesValidators.pattern(/^[0-9]+$/)ensures integer-only input- Cross-field validator enforces ordering constraints
This implementation addresses the previous review suggestion.
…are Product Creation Form
JaySoni1
force-pushed
the
WEB-460-Shares-per-Client-Field-Allows-Zero-and-Negative-Values-in-Share-Product-Creation-Form
branch
from
b5f7a32 to
fc01a4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts (1)
94-104: Improve type safety and validation logic clarity.The validator works but has room for improvement:
- Type safety: The return type uses
anyinstead ofValidationErrors, which violates the strict type safety guideline for Angular code.- Logic clarity: The truthy check on line 98 relies on implicit behavior where
Number('')returns0(falsy) andNumber('abc')returnsNaN(falsy). While this works because individual validators catch invalid inputs, it's not immediately clear why we're checking truthiness rather than explicitly checking field validity.Apply this diff to improve type safety and logic clarity:
+import { UntypedFormGroup, UntypedFormBuilder, FormControl, Validators, ReactiveFormsModule, ValidationErrors } from '@angular/forms'; -import { UntypedFormGroup, UntypedFormBuilder, FormControl, Validators, ReactiveFormsModule } from '@angular/forms';- private validateSharesOrder(group: UntypedFormGroup): { [key: string]: any } | null { + private validateSharesOrder(group: UntypedFormGroup): ValidationErrors | null { + const minControl = group.get('minimumShares'); + const nominalControl = group.get('nominalShares'); + const maxControl = group.get('maximumShares'); + + // Only validate if all individual fields are valid + if (minControl?.invalid || nominalControl?.invalid || maxControl?.invalid) { + return null; + } + - const min = Number(group.get('minimumShares')?.value); - const nominal = Number(group.get('nominalShares')?.value); - const max = Number(group.get('maximumShares')?.value); + const min = Number(minControl?.value); + const nominal = Number(nominalControl?.value); + const max = Number(maxControl?.value); - if (min && nominal && max) { - if (min > nominal || nominal > max) { - return { sharesOrder: true }; - } + + if (min > nominal || nominal > max) { + return { sharesOrder: true }; } + return null; }Based on coding guidelines, strict type safety should be maintained for Angular code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html(1 hunks)src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-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:23.311Z
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:23.311Z
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:23.311Z
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/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts
🔇 Additional comments (2)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts (2)
58-81: Field validators correctly prevent zero and negative values.The combination of
Validators.required,Validators.min(1), andValidators.pattern(/^[0-9]+$/)properly ensures that only positive integers >= 1 are accepted for shares fields.
88-90: Form-level validator correctly configured.The form-level validator is properly set up to enforce cross-field validation between the three shares fields.
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Why don't use Validators.min and Validators.max? If the field is related to an amount, there is a component as input amount
|
@alberto-art3ch I have used Validators.min(1) and I have used it to ensure only positive values. I have consider these field as counts, not amounts so I have used a standard number input. If you prefer the input amount component I can update it , please let me know . |
2 participants
Changes Made :-
Added validation to ensure the "Shares per Client" fields (Minimum, Default, Maximum) only accept positive integer values in the Create Share Product form.
WEB-460
Before :-
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.