Update Resource Forms to Enhance Validation #9062
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe changes involve updates to several components related to resource management. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/components/Form/FormFields/TextFormField.tsx (1)
Line range hint
32-69: Consider enhancing error handling for validation constraints.The current implementation relies on browser-native validation for min/max constraints. To improve user experience, consider adding custom error messages and validation handling.
Suggestions:
- Integrate with the existing form validation system
- Add specific error messages for min/max violations
- Consider adding a
validationModeprop to control when validation occurs (onChange, onBlur, onSubmit)- Add aria-labels for accessibility when validation fails
Would you like me to provide a code example implementing these suggestions?
src/components/Resource/ResourceCreate.tsx (1)
Line range hint
1-394: Consider architectural improvements for better type safety and validation consistency.While the overall implementation is solid, consider these improvements:
- Define a proper type for the form state instead of using
any:interface ResourceFormState { category: string; sub_category: number; approving_facility: { id: string } | null; assigned_facility: { id: string } | null; emergency: string; title: string; reason: string; refering_facility_contact_name: string; refering_facility_contact_number: string; required_quantity: number | null; }
- Create a consistent validation pattern across all fields using a validation schema library like Yup or Zod.
Would you like me to provide a complete example of how to implement these improvements?
src/components/Resource/ResourceDetailsUpdate.tsx (2)
51-59: LGTM! Consider enhancing error messages for better user experience.The required field validations are properly implemented, particularly for the title and description fields. The error messages are clear but could be more user-friendly.
Consider enhancing the error messages to be more descriptive:
assigned_facility_object: { - errorText: "Please Select Facility Type", + errorText: "Please select a facility to assign this resource to", }, title: { - errorText: "Title is required.", + errorText: "Please provide a title for this resource request", },
274-274: Consider adding maximum value constraints for quantity fields.While the
min={1}constraint prevents negative values as required, consider adding a reasonable maximum value to prevent unrealistic quantities.<TextFormField label="Required Quantity" name="requested_quantity" type="number" min={1} + max={9999} value={state.form.requested_quantity} onChange={handleChange} />Also applies to: 283-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Form/FormFields/TextFormField.tsx(2 hunks)src/components/Resource/ResourceCreate.tsx(1 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(8 hunks)
🔇 Additional comments (4)
src/components/Resource/ResourceCreate.tsx (1)
Line range hint 385-394: LGTM: Description field validation is properly implemented.
The Description field implementation meets the PR objectives with:
- Required field validation
- Error handling
- Internationalized labels and error messages
src/components/Resource/ResourceDetailsUpdate.tsx (3)
110-118: LGTM! Robust validation logic implementation.
The enhanced validation logic properly handles both facility objects and string fields, with appropriate type-specific checks.
161-161: LGTM! More intuitive validation check.
The inverted validation logic (!validateForm()) improves code readability by focusing on the positive case (valid form) rather than the negative case.
295-300: LGTM! Required fields properly implemented.
The title and description fields are now properly marked as required, fulfilling the PR objectives. The implementation includes both frontend validation (required attribute) and backend validation (through validateForm).
Also applies to: 311-313
…053/prevent-negative-quantity-and-empty-description
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/resource_spec/ResourcesHomepage.cy.ts (1)
Line range hint
32-45: Add test cases for new validation requirements.The "Create a resource request" test should include additional test cases to verify the new validation requirements:
- Attempt to create a resource with negative quantity (should fail)
- Attempt to create a resource with empty description (should fail)
This ensures the new validation logic is properly tested.
Consider adding these test cases:
it("Should not allow negative quantity in resource request", () => { // ... setup code similar to existing create test ... facilityPage.fillResourceRequestDetails( "Test User", phone_number, "Dummy", "Test title", "-10", // negative quantity "Test description", ); facilityPage.clickSubmitRequestButton(); // Verify error message or validation feedback cy.get('[data-testid="quantity-error"]').should('be.visible') .and('contain', 'Quantity must be positive'); }); it("Should not allow empty description in resource request", () => { // ... setup code similar to existing create test ... facilityPage.fillResourceRequestDetails( "Test User", phone_number, "Dummy", "Test title", "10", "", // empty description ); facilityPage.clickSubmitRequestButton(); // Verify error message or validation feedback cy.get('[data-testid="description-error"]').should('be.visible') .and('contain', 'Description is required'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cypress/e2e/resource_spec/ResourcesHomepage.cy.ts(2 hunks)
🔇 Additional comments (2)
cypress/e2e/resource_spec/ResourcesHomepage.cy.ts (2)
1-1: LGTM!
The import reordering is acceptable and doesn't impact functionality.
71-74: LGTM! Well-structured facility selection step.
The addition of facility selection using a custom command enhances the test coverage and follows Cypress best practices.
…-empty-description
…-empty-description
|
|
|
Have you updated the value or is it already present there? |
|
@JavidSumra i updated the value |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/components/Resource/ResourceDetailsUpdate.tsx (4)
52-60: Consider improving error message clarityWhile the addition of required fields for title and description is good, the error message for
assigned_facility_objectcould be more specific.Consider updating the error message to be more descriptive:
assigned_facility_object: { - errorText: "Please Select Facility Type", + errorText: "Please select an assigned facility", },
111-119: Consider enhancing type safetyWhile the validation logic is improved, consider adding TypeScript interfaces for the facility objects to make the code more type-safe and maintainable.
Consider adding:
interface FacilityObject { name: string; id: string; // other relevant properties } // Then update the form interface interface ResourceForm { approving_facility_object: FacilityObject | null; assigned_facility_object: FacilityObject | null; // other form fields }
275-279: Consider additional quantity validation improvementsWhile the min attribute and handleNegativeValue prevent negative values, consider adding these improvements:
- Add max validation to prevent unreasonably large numbers
- Validate that assigned_quantity doesn't exceed requested_quantity
Consider updating both quantity fields:
<TextFormField label="Required Quantity" name="requested_quantity" type="number" min={1} + max={999999} value={state.form.requested_quantity} onChange={handleChange} onInput={handleNegativeValue} /> <TextFormField name="assigned_quantity" type="number" min={1} + max={state.form.requested_quantity} label="Approved Quantity" value={state.form.assigned_quantity} onChange={handleChange} disabled={state.form.status !== "PENDING"} onInput={handleNegativeValue} />Also applies to: 285-291
298-303: Consider enhancing validation UXThe required attribute is correctly added to title and description fields. However, consider these UX improvements:
- Add visual indicators (like asterisks) for required fields
- Show validation messages immediately on field blur rather than only on form submission
Consider implementing a reusable required field label component:
const RequiredFieldLabel: React.FC<{label: string}> = ({label}) => ( <span> {label} <span className="text-red-500">*</span> </span> );Then use it in your form fields:
<TextFormField name="title" type="text" - label="Request Title" + label={<RequiredFieldLabel label="Request Title" />} // ... other props />Also applies to: 314-316
src/Utils/utils.ts (1)
547-560: Consider moving form-specific utilities to a dedicated moduleThis function is specifically for form validation and is tightly coupled with React's form handling. Consider:
- Moving it to a dedicated form utilities module (e.g.,
src/Utils/formUtils.tsorsrc/components/common/forms/utils.ts)- Grouping it with other form-related utilities for better organization
- Adding JSDoc documentation to explain its purpose and usage
🧰 Tools
🪛 Biome
[error] 557-557: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Utils/utils.ts(1 hunks)src/components/Form/FormFields/TextFormField.tsx(2 hunks)src/components/Resource/ResourceCreate.tsx(2 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Form/FormFields/TextFormField.tsx
- src/components/Resource/ResourceCreate.tsx
🧰 Additional context used
🪛 Biome
src/Utils/utils.ts
[error] 557-557: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
src/components/Resource/ResourceDetailsUpdate.tsx (2)
28-28: LGTM: Good separation of concerns
The addition of the handleNegativeValue utility import aligns with the PR objective of preventing negative values and follows good practices by keeping validation logic in a separate utility file.
Line range hint 1-341: Verify the validation behavior in edge cases
The implementation successfully addresses the PR objectives. However, let's verify the validation behavior in these edge cases:
- Form submission with zero quantities
- Copy-paste of negative values
- Browser form validation bypass
✅ Verification successful
Input validation is properly implemented to prevent negative values
The implementation successfully addresses the validation requirements:
-
The
handleNegativeValueutility function inUtils/utils.tseffectively prevents negative values by:- Removing all instances of '-' from the input value
- Ensuring the input is a valid number
- Resetting to empty if invalid
-
The form fields have proper validation:
- Both quantity fields use
min={1}for HTML5 validation - Both fields implement
handleNegativeValuefor copy-paste prevention - Required fields validation is implemented in
validateForm()
- Both quantity fields use
The implementation provides multiple layers of validation and is consistent with other resource-related components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other quantity validation implementations in the codebase
rg -l "handleNegativeValue|min={[1-9]" --type typescript
# Search for similar form validation patterns
ast-grep --pattern 'validateForm() {
$$$
return $_;
}'
Length of output: 257
Script:
#!/bin/bash
# Search for other quantity validation implementations in the codebase
rg -l "handleNegativeValue" --type typescript
rg -l 'min=\{1\}' --type typescript
# Search for similar form validation patterns
ast-grep --pattern 'validateForm = () => {
$$$
}'
# Search for quantity validation in utils
rg -A 5 "handleNegativeValue" --type typescript
Length of output: 290
Script:
#!/bin/bash
# Search for other quantity validation implementations in the codebase
rg -l "handleNegativeValue"
rg -l 'min=\{1\}'
# Search for similar form validation patterns
ast-grep --pattern 'const handleNegativeValue = ($$$) => {
$$$
}'
# Search for quantity validation in utils
rg -A 5 "handleNegativeValue"
Length of output: 2874
Script:
#!/bin/bash
# Check implementation of handleNegativeValue in utils
cat src/Utils/utils.ts
# Check form validation in ResourceCreate for consistency
rg -A 10 "validateForm" src/components/Resource/ResourceCreate.tsx
Length of output: 16354
…-negative-quantity-and-empty-description' into issues/9053/prevent-negative-quantity-and-empty-description
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Utils/utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome
src/Utils/utils.ts
[error] 553-553: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
…-empty-description
…-empty-description
…-empty-description
…tion' of https://github.com/JavidSumra/care_fe into issues/9053/prevent-negative-quantity-and-empty-description
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
src/components/Resource/ResourceCreate.tsx (1)
Line range hint
127-160: Add explicit validation for requested_quantityThe validateForm function should include explicit validation for requested_quantity to ensure server-side validation matches client-side constraints.
Add this case to the validateForm switch statement:
case "requested_quantity": { const quantity = Number(state.form[field]); if (!state.form[field] || isNaN(quantity) || quantity < 1) { errors[field] = requiredFields[field].invalidText; isInvalidForm = true; } return; }src/components/Resource/ResourceDetailsUpdate.tsx (1)
111-119: Consider consolidating facility object validationThe current validation for facility objects is repetitive. Consider extracting this logic into a helper function for better maintainability.
+const isFacilityValid = (facility: any) => facility && facility.name; + if ( field === "approving_facility_object" || field === "assigned_facility_object" ) { - if (!state.form[field] || !state.form[field]?.name) { + if (!isFacilityValid(state.form[field])) { errors[field] = requiredFields[field].errorText; isInvalidForm = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Utils/utils.ts(1 hunks)src/components/Resource/ResourceCreate.tsx(4 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(8 hunks)
🔇 Additional comments (4)
src/components/Resource/ResourceCreate.tsx (2)
303-307: LGTM! Input constraints are properly implemented
The implementation correctly prevents negative values through:
- type="number" for numeric input
- min={1} to prevent zero/negative values
- sanitizeNumberInput for additional validation
303-307: Verify edge cases for quantity and description validation
Both fields have validation, but let's ensure all paths are tested:
- Quantity validation
- Description validation
Also applies to: 312-322
✅ Verification successful
Validation implementation is comprehensive and properly handles edge cases
The code implements robust validation for both quantity and description fields:
-
Quantity validation:
- HTML
min={1}attribute prevents negative values sanitizeNumberInputfunction removes non-numeric characters and enforces minimum value- Form-level validation through
validateForm()before submission
- HTML
-
Description/required fields validation:
- Comprehensive validation through
requiredFieldsobject - Form-level validation before submission
- Error handling for empty fields and invalid inputs
- Comprehensive validation through
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for tests covering these validations
# Check for quantity validation tests
echo "Searching for quantity validation tests..."
rg -l "requested_quantity.*validation|quantity.*test" --type=typescript --type=javascript
# Check for description validation tests
echo "Searching for description validation tests..."
rg -l "reason.*required|description.*validation" --type=typescript --type=javascript
Length of output: 444
Script:
#!/bin/bash
# Let's try a different approach to search for test files and validation logic
# Search for test files related to ResourceCreate
echo "Searching for ResourceCreate test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | grep -i "resource"
# Search for validation logic in ResourceCreate
echo "Searching for validation logic in ResourceCreate..."
rg "validation|sanitize|required" src/components/Resource/ResourceCreate.tsx
# Search for form state and validation patterns
echo "Searching for form state and validation patterns..."
ast-grep --pattern 'const $_ = useForm<$_>({
$$$
})'
Length of output: 1181
Script:
#!/bin/bash
# Let's examine the validation implementation more closely
# Check the validation logic and required fields implementation
echo "Checking validation implementation..."
rg -A 10 "requiredFields.*=" src/components/Resource/ResourceCreate.tsx
# Look for form submission and error handling
echo "Checking form submission and error handling..."
rg -A 10 "handleSubmit" src/components/Resource/ResourceCreate.tsx
# Check sanitizeNumberInput implementation
echo "Checking number sanitization..."
rg -A 5 "sanitizeNumberInput.*=" src/Utils/utils.ts
Length of output: 1853
src/components/Resource/ResourceDetailsUpdate.tsx (2)
28-28: LGTM: Form validation setup aligns with requirements
The addition of validation requirements for title and description fields, along with the sanitizeNumberInput utility import, properly supports the PR objectives.
Also applies to: 52-60
275-278: LGTM: Form field implementations meet requirements
The implementation properly prevents negative values through:
- min attribute set to 1
- sanitizeNumberInput validation
- server-side validation via default values
And ensures description is mandatory through:
- required attribute
- form validation
- error message display
Also applies to: 285-290, 298-303, 314-316
…-empty-description
|
Hey @nihal467 can you please add |
…-empty-description
…-empty-description
…-empty-description
nihal467
added
needs testing
and removed
question
…-empty-description
…-empty-description
|
@JavidSumra before requesting a review, make sure the PR is working well |
…issues/9053/prevent-negative-quantity-and-empty-description
…uantity-and-empty-description
…tion' of https://github.com/JavidSumra/care_fe into issues/9053/prevent-negative-quantity-and-empty-description
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
requested_quantityandassigned_quantityfields to accept only numeric values with a minimum of 1.titleandreasonin the resource form, ensuring all necessary information is provided before submission.Bug Fixes
Tests