-
Notifications
You must be signed in to change notification settings - Fork 800
WEB-464 Lock in Period Field Allows Zero and Negative Values in Share Product Creation Form #2882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…g Product Creation Form
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Lock-in Period Frequency Validation 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 form control validators (Validators.required, Validators.min(1)) to lockinPeriodFrequency. Updated template to enforce minimum value 1, step 1, and display mat-error messages for required and minimum value validation failures. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~5 minutes
- Straightforward validation enhancement with consistent pattern across two files
- No complex logic or control flow changes
- Standard Angular form validation approach
Suggested reviewers
- IOhacker
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Title check | ✅ Passed | The title accurately summarizes the main change: adding validation to prevent zero and negative values in the Lock-in Period field. It is specific, clear, and directly reflects the changes in both the HTML and TypeScript files. |
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.html (1)
66-72: Localize the new validation messages for consistency with the rest of the formThe two new
mat-errormessages are hard-coded in English, while the rest of this template uses translation keys/pipes. Please switch these to use the translation system (e.g., similar to the nominalShares error above) so validation feedback is localized as well.
📜 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
🔇 Additional comments (1)
src/app/products/share-products/share-product-stepper/share-product-settings-step/share-product-settings-step.component.ts (1)
65-71: Verify integer validation requirement for lockinPeriodFrequency fieldThe suggestion to add
Validators.pattern(/^[1-9]\d*$/)for integer validation is technically sound—Validators.min(1)alone does permit decimal values like1.5. However, without confirming whether thelockinPeriodFrequencyfield must strictly be a positive integer (versus a numeric frequency that may accept decimals), this change cannot be verified against actual requirements.Check the following before applying this change:
- Is
lockinPeriodFrequencydocumented or typed to require a whole number?- Do similar frequency fields elsewhere in the codebase use pattern validators for integer enforcement?
- Does the backend API expect an integer or can it accept decimals?
If integer values are indeed required, the proposed pattern validator is the appropriate solution.
alberto-art3ch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes Made :-
Added validation to the "Lock-in Period" frequency field in the Create Share Product form to ensure only positive integer values greater than zero are allowed.
WEB-464
Before :-

After :-

Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.