WEB-705 Restrict Negative Values for Office Phone, MTN, and Airtel Fi…#3151
Conversation
…elds in Datatable Forms
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Form Field Min-Validation Logic src/app/core/utils/datatables.ts |
Added isRestrictedField detection by normalizing field names and checking against a list of restricted identifiers (mtn, airtel, officephone, office_phone, office phone). Expanded the min-constraint condition to apply when isMinSavingsAmount, isPriceOneShare, or isRestrictedField is true. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~5 minutes
Possibly related PRs
- WEB-704 Restrict negative values for group rules minimum savings amount and share price fields #3133: Modifies the same form field generation logic in datatables.ts to add min:0 constraint for minSavingsAmount and priceOneShare; this PR extends that same min-validation pattern to additional restricted fields.
Suggested reviewers
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Merge Conflict Detection | ❌ Merge conflicts detected (34 files): ⚔️ README.md (content)⚔️ angular.json (content)⚔️ docker-compose.yml (content)⚔️ env.sample (content)⚔️ proxy.conf.js (content)⚔️ proxy.localhost.conf.js (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.html (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.scss (content)⚔️ src/app/clients/client-stepper/client-general-step/client-general-step.component.ts (content)⚔️ src/app/clients/clients.service.ts (content)⚔️ src/app/clients/edit-client/edit-client.component.html (content)⚔️ src/app/clients/edit-client/edit-client.component.scss (content)⚔️ src/app/clients/edit-client/edit-client.component.ts (content)⚔️ src/app/core/utils/datatables.ts (content)⚔️ src/app/products/share-products/share-product-stepper/share-product-market-price-step/share-product-market-price-step.component.ts (content)⚔️ src/app/shared/footer/footer.component.scss (content)⚔️ src/assets/env.js (content)⚔️ src/assets/env.template.js (content)⚔️ src/assets/translations/cs-CS.json (content)⚔️ src/assets/translations/de-DE.json (content)⚔️ src/assets/translations/en-US.json (content)⚔️ src/assets/translations/es-CL.json (content)⚔️ src/assets/translations/es-MX.json (content)⚔️ src/assets/translations/fr-FR.json (content)⚔️ src/assets/translations/it-IT.json (content)⚔️ src/assets/translations/ko-KO.json (content)⚔️ src/assets/translations/lt-LT.json (content)⚔️ src/assets/translations/lv-LV.json (content)⚔️ src/assets/translations/ne-NE.json (content)⚔️ src/assets/translations/pt-PT.json (content)⚔️ src/assets/translations/sw-SW.json (content)⚔️ src/environments/environment.prod.ts (content)⚔️ src/environments/environment.ts (content)⚔️ src/typings.d.ts (content)These conflicts must be resolved before merging into dev. |
Resolve conflicts locally and push changes to this branch. |
✅ 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 clearly and specifically describes the main change: restricting negative values for Office Phone, MTN, and Airtel fields in datatable forms, 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. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
⚔️ Resolve merge conflicts (beta)
- Auto-commit resolved conflicts to branch
WEB-705-restrict-negative-values-for-office-phone-mtn-and-airtel-fields-in-datatable-forms - Post resolved changes as copyable diffs 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/core/utils/datatables.ts (1)
58-71:⚠️ Potential issue | 🟡 MinorGuard
min: 0to apply only to numeric column types.The
minvalidator is applied to all column types regardless of theirtypeattribute. ForSTRINGandTEXTfields (wheretypeis'text'), the HTMLminattribute is ignored by the browser, and Angular'sValidators.min()may produce unexpected behavior when attempting to validate string values numerically.Apply the proposed fix to ensure
minis set only forINTEGERandDECIMALcolumns:Proposed fix
- if (isMinSavingsAmount || isPriceOneShare || isRestrictedField) { + if ((isMinSavingsAmount || isPriceOneShare || isRestrictedField) && + (column.columnDisplayType === 'INTEGER' || column.columnDisplayType === 'DECIMAL')) { inputOptions.min = 0; }
🧹 Nitpick comments (1)
src/app/core/utils/datatables.ts (1)
50-56: Redundant entries in the restricted-field list.After the
.replace(/[_\s]+/g, '')normalization on Line 56,'office_phone'and'office phone'both collapse to'officephone'—which is already in the list. Those two entries are dead weight.♻️ Suggested simplification
const isRestrictedField = [ 'mtn', 'airtel', 'officephone', - 'office_phone', - 'office phone' ].some((name) => colName.includes(name.replace(/[_\s]+/g, ''))); + ].some((name) => colName.includes(name));Since both
colNameand the literal names are already fully normalized (lowercased, no separators), you can drop the redundant entries and the inner.replace()call.
|
@IOhacker Thank You for the review |
Changes Made :-
-Restrict negative values for Office Phone, MTN, and Airtel fields in datatable forms .
WEB-705
Before :-
After :-
Summary by CodeRabbit