Skip to content
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

Update Resource Forms to Enhance Validation #9062

Open
wants to merge 63 commits into
base: develop
Choose a base branch
from

Conversation

JavidSumra
Copy link
Contributor

@JavidSumra JavidSumra commented Nov 8, 2024

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation (if applicable).
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

Summary by CodeRabbit

  • New Features

    • Enhanced resource creation component with improved form handling and validation.
    • New localization strings added for better user guidance and error handling.
  • Bug Fixes

    • Updated input field selectors to align with the current HTML structure, ensuring accurate data capture during resource requests.
  • Tests

    • Modified test suite to include new functionality for updating resource status.
  • Chores

    • Refactored routing logic to streamline resource update process.

@JavidSumra JavidSumra requested a review from a team as a code owner November 8, 2024 14:52
Copy link

netlify bot commented Nov 8, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 785bfa4
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/675fc7f41f2f2d00081667e9
😎 Deploy Preview https://deploy-preview-9062--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

Walkthrough

The changes in this pull request primarily focus on the ResourceCreate component, which has been refactored to improve resource management functionalities. Modifications include making facilityId optional, introducing a new resourceId, and defining a new ResourceData type. The form handling and validation logic have been enhanced, alongside the addition of new localization strings for error messages. The routing for resource updates has shifted from ResourceDetailsUpdate to ResourceCreate, and test cases have been updated to reflect these changes.

Changes

File Change Summary
src/components/Resource/ResourceCreate.tsx Updated resourceProps to make facilityId optional and add resourceId. Defined ResourceData type and modified initForm. Refactored form handling and validation logic.
cypress/e2e/resource_spec/ResourcesHomepage.cy.ts Added a line in the test suite to select an option from the assigned_facility_object input field during the "Update the status of resource" test case.
public/locale/en.json Added multiple new localization strings for error handling and user guidance.
src/Routers/routes/ResourceRoutes.tsx Changed routing logic to use ResourceCreate for resource updates instead of ResourceDetailsUpdate, passing resourceId as a parameter.
cypress/pageobject/Facility/FacilityCreation.ts Updated selectors in the fillResourceRequestDetails method to reflect changes in input field identifiers.

Assessment against linked issues

Objective Addressed Explanation
Required Quantity field should not accept negative values (9053)
Description field should be mandatory, preventing updates if left empty (9053)

Possibly related PRs

Suggested labels

tested

Suggested reviewers

  • nihal467
  • rithviknishad
  • shivankacker

🐰 In the meadow where changes bloom,
A new form rises, dispelling gloom.
With fields that validate and errors that guide,
Resource creation, now with pride!
Hop along, let's test and play,
For a smoother path, we pave the way! 🌼

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Integrate with the existing form validation system
  2. Add specific error messages for min/max violations
  3. Consider adding a validationMode prop to control when validation occurs (onChange, onBlur, onSubmit)
  4. 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:

  1. 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;
}
  1. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 66ef112 and 46ec289.

📒 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

src/components/Form/FormFields/TextFormField.tsx Outdated Show resolved Hide resolved
src/components/Form/FormFields/TextFormField.tsx Outdated Show resolved Hide resolved
src/components/Resource/ResourceCreate.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 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:

  1. Attempt to create a resource with negative quantity (should fail)
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 46ec289 and 41c1b1c.

📒 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.

@nihal467
Copy link
Member

image

@JavidSumra
Copy link
Contributor Author

Have you updated the value or is it already present there?
cc @nihal467

@nihal467
Copy link
Member

@JavidSumra i updated the value

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
src/components/Resource/ResourceDetailsUpdate.tsx (4)

52-60: Consider improving error message clarity

While the addition of required fields for title and description is good, the error message for assigned_facility_object could 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 safety

While 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 improvements

While the min attribute and handleNegativeValue prevent negative values, consider adding these improvements:

  1. Add max validation to prevent unreasonably large numbers
  2. 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 UX

The required attribute is correctly added to title and description fields. However, consider these UX improvements:

  1. Add visual indicators (like asterisks) for required fields
  2. 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 module

This function is specifically for form validation and is tightly coupled with React's form handling. Consider:

  1. Moving it to a dedicated form utilities module (e.g., src/Utils/formUtils.ts or src/components/common/forms/utils.ts)
  2. Grouping it with other form-related utilities for better organization
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 41c1b1c and 098ba35.

📒 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:

  1. Form submission with zero quantities
  2. Copy-paste of negative values
  3. Browser form validation bypass
✅ Verification successful

Input validation is properly implemented to prevent negative values

The implementation successfully addresses the validation requirements:

  1. The handleNegativeValue utility function in Utils/utils.ts effectively prevents negative values by:

    • Removing all instances of '-' from the input value
    • Ensuring the input is a valid number
    • Resetting to empty if invalid
  2. The form fields have proper validation:

    • Both quantity fields use min={1} for HTML5 validation
    • Both fields implement handleNegativeValue for copy-paste prevention
    • Required fields validation is implemented in validateForm()

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

src/Utils/utils.ts Outdated Show resolved Hide resolved
…-negative-quantity-and-empty-description' into issues/9053/prevent-negative-quantity-and-empty-description
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 098ba35 and 7f5e000.

📒 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)

src/Utils/utils.ts Outdated Show resolved Hide resolved
src/components/Form/FormFields/TextFormField.tsx Outdated Show resolved Hide resolved
src/components/Form/FormFields/TextFormField.tsx Outdated Show resolved Hide resolved
src/Utils/utils.ts Outdated Show resolved Hide resolved
src/Utils/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (1)
public/locale/en.json (1)

358-358: Standardize quantity validation messages.

The error messages for quantity validation should follow a consistent pattern:

  • assigned_quantity_error: "Value can't be smaller than 0"
  • requested_quantity_error: "Value can't be smaller than 1"

Apply this diff to standardize the messages:

-  "assigned_quantity_error": "Value can't be smaller than 0",
+  "assigned_quantity_error": "Value must be 0 or greater",
-  "requested_quantity_error": "Value can't be smaller than 1",
+  "requested_quantity_error": "Value must be 1 or greater",

Also applies to: 1195-1195

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c689d1b and 2fd83cc.

📒 Files selected for processing (1)
  • public/locale/en.json (9 hunks)
🔇 Additional comments (3)
public/locale/en.json (3)

1342-1343: LGTM: Title validation messages are clear and consistent.

The error messages for title validation are well-defined:

  • Mandatory field validation: "Title for the resource request is mandatory"
  • User prompt: "Please enter a title for the resource request"

671-672: LGTM: Description validation messages are clear and consistent.

The error messages for description validation are well-defined:

  • Mandatory field validation: "Description is required field"
  • Empty resource error: "Resource approving facility cannot be empty"

1170-1172: LGTM: Contact information validation messages are clear and consistent.

The error messages for contact information validation are well-defined:

  • Name validation: "Name of the contact at the referring facility is required"
  • Number validation: "Phone number of the contact at the referring facility is required"
  • Number format validation: "Please enter a valid phone number"

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (2)
src/components/Resource/ResourceCreate.tsx (1)

407-410: Consider removing the min attribute from quantity inputs.

While the min attribute provides client-side validation, it's better to rely on the validateForm function for consistent validation across all browsers. The HTML min attribute can be inconsistent across browsers and might conflict with the form validation.

-  min={1}

Also applies to: 494-496

public/locale/en.json (1)

1196-1196: Standardize quantity validation messages.

The error messages for quantity validation should follow a consistent format:

-  "assigned_quantity_error": "Value can't be smaller than 0",
+  "assigned_quantity_error": "Value must be 0 or greater",
-  "requested_quantity_error": "Value can't be smaller than 1",
+  "requested_quantity_error": "Value must be 1 or greater",

Also applies to: 359-359

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2fd83cc and 8fd7a56.

📒 Files selected for processing (2)
  • public/locale/en.json (9 hunks)
  • src/components/Resource/ResourceCreate.tsx (5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9062
File: src/Routers/routes/ResourceRoutes.tsx:19-21
Timestamp: 2024-12-10T02:49:05.600Z
Learning: In the `ResourceCreate` component, `facilityId` can be optional.
🪛 Biome (1.9.4)
src/components/Resource/ResourceCreate.tsx

[error] 246-246: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 247-247: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
src/components/Resource/ResourceCreate.tsx (3)

211-220: LGTM: Validation logic for quantity fields is well-implemented.

The validation correctly enforces:

  • Minimum threshold of 1 for requested quantity
  • Minimum threshold of 0 for assigned quantity
  • Proper type conversion using parseFloat

255-256: ⚠️ Potential issue

Remove fallback values in submission handler.

The submission handler shouldn't provide fallback values for quantities as it bypasses the validation rules. Let the validation handle empty values instead.

-  requested_quantity: parseFloat(form.requested_quantity || "1"),
-  assigned_quantity: parseFloat(form.assigned_quantity || "0"),
+  requested_quantity: parseFloat(form.requested_quantity),
+  assigned_quantity: parseFloat(form.assigned_quantity),

Likely invalid or redundant comment.


74-75: 🛠️ Refactor suggestion

Initialize quantity fields as undefined for better type safety.

Using empty strings for numeric fields could lead to validation issues. Consider using undefined for better type safety and validation.

-  requested_quantity: "",
-  assigned_quantity: "",
+  requested_quantity: undefined,
+  assigned_quantity: undefined,

Likely invalid or redundant comment.

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Dec 12, 2024
Copy link

👋 Hi, @JavidSumra,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (3)
src/components/Resource/ResourceCreate.tsx (2)

217-226: Enhance quantity validation logic

The current validation could be improved by:

  1. Handling NaN values from parseFloat
  2. Adding maximum value validation to prevent unreasonable quantities
 case "requested_quantity":
 case "assigned_quantity": {
   if (!resourceId && field === "assigned_quantity") break;
   const value = form[field as keyof ResourceData];
   const minVal = field === "assigned_quantity" ? 0 : 1;
-  if (!value || parseFloat(String(value)) < minVal) {
+  const numValue = parseFloat(String(value));
+  if (!value || isNaN(numValue) || numValue < minVal || numValue > 1000000) {
     errors[field as keyof ResourceData] = errorText;
   }
   break;
 }

413-416: Consider removing the min attribute from quantity inputs

While the min attribute prevents negative values in modern browsers, it's better to handle validation in validateForm for a consistent user experience across all browsers.

 <TextFormField
   label={t("required_quantity")}
   type="number"
-  min={1}
 />

Also applies to: 491-494

public/locale/en.json (1)

364-364: Standardize quantity validation messages

The error messages for quantity validation should be consistent:

  • assigned_quantity_error: "Value can't be smaller than 0"
  • requested_quantity_error: "Value can't be smaller than 1"
-  "assigned_quantity_error": "Value can't be smaller than 0",
+  "assigned_quantity_error": "Value must be 0 or greater",
-  "requested_quantity_error": "Value can't be smaller than 1",
+  "requested_quantity_error": "Value must be 1 or greater",

Also applies to: 1276-1276

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f82661 and d37047a.

📒 Files selected for processing (3)
  • cypress/pageobject/Facility/FacilityCreation.ts (1 hunks)
  • public/locale/en.json (11 hunks)
  • src/components/Resource/ResourceCreate.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cypress/pageobject/Facility/FacilityCreation.ts
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9062
File: src/Routers/routes/ResourceRoutes.tsx:19-21
Timestamp: 2024-12-10T02:49:05.600Z
Learning: In the `ResourceCreate` component, `facilityId` can be optional.
🪛 Biome (1.9.4)
src/components/Resource/ResourceCreate.tsx

[error] 252-252: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 253-253: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
src/components/Resource/ResourceCreate.tsx (2)

74-75: 🛠️ Refactor suggestion

Initialize quantity fields as undefined instead of empty strings

Empty strings could bypass the validation logic. Using undefined would make the validation more robust.

-  requested_quantity: "",
-  assigned_quantity: "",
+  requested_quantity: undefined,
+  assigned_quantity: undefined,

Likely invalid or redundant comment.


261-262: ⚠️ Potential issue

Remove fallback values in submission handler

The submission handler shouldn't provide fallback values for quantities. Instead, rely on proper validation and initialization.

-      requested_quantity: parseFloat(form.requested_quantity || "1"),
-      assigned_quantity: parseFloat(form.assigned_quantity || "0"),
+      requested_quantity: parseFloat(form.requested_quantity),
+      assigned_quantity: parseFloat(form.assigned_quantity),

Likely invalid or redundant comment.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Dec 13, 2024
@JavidSumra
Copy link
Contributor Author

@nihal467 need your review

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Dec 16, 2024
Copy link

👋 Hi, @JavidSumra,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (3)
src/components/Resource/ResourceCreate.tsx (2)

217-226: Enhance quantity validation

While the minimum value validation is implemented, consider adding:

  1. Maximum value validation to prevent unreasonably large quantities
  2. Type validation to ensure numeric input
 case "requested_quantity":
 case "assigned_quantity": {
   if (!resourceId && field === "assigned_quantity") break;
   const value = form[field as keyof ResourceData];
   const minVal = field === "assigned_quantity" ? 0 : 1;
-  if (!value || parseFloat(String(value)) < minVal) {
+  const numValue = parseFloat(String(value));
+  if (!value || isNaN(numValue) || numValue < minVal || numValue > 999999) {
     errors[field as keyof ResourceData] = errorText;
   }
   break;
 }

413-416: Consider removing HTML5 validation attributes

While HTML5 validation provides a quick solution, it's better to handle all validation through the form validator to ensure consistent behavior and proper error messages.

-  min={1}

Also applies to: 492-493

public/locale/en.json (1)

356-356: Standardize quantity validation messages

The error messages for quantity validation should follow a consistent pattern:

-  "assigned_quantity_error": "Value can't be smaller than 0",
+  "assigned_quantity_error": "Value must be 0 or greater",
-  "requested_quantity_error": "Value can't be smaller than 1",
+  "requested_quantity_error": "Value must be 1 or greater",

Also applies to: 1268-1268

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d37047a and 785bfa4.

📒 Files selected for processing (4)
  • cypress/e2e/resource_spec/ResourcesHomepage.cy.ts (1 hunks)
  • cypress/pageobject/Facility/FacilityCreation.ts (1 hunks)
  • public/locale/en.json (9 hunks)
  • src/components/Resource/ResourceCreate.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • cypress/e2e/resource_spec/ResourcesHomepage.cy.ts
  • cypress/pageobject/Facility/FacilityCreation.ts
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9062
File: src/Routers/routes/ResourceRoutes.tsx:19-21
Timestamp: 2024-12-10T02:49:05.600Z
Learning: In the `ResourceCreate` component, `facilityId` can be optional.
🪛 Biome (1.9.4)
src/components/Resource/ResourceCreate.tsx

[error] 252-252: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 253-253: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
src/components/Resource/ResourceCreate.tsx (2)

74-75: 🛠️ Refactor suggestion

Initialize quantity fields as undefined instead of empty strings

Empty string initialization could bypass validation logic and cause type coercion issues. Using undefined would make the validation more robust and type-safe.

-  requested_quantity: "",
-  assigned_quantity: "",
+  requested_quantity: undefined,
+  assigned_quantity: undefined,

Likely invalid or redundant comment.


261-262: ⚠️ Potential issue

Remove fallback values in submission handler

Using fallback values in the submission handler could bypass validation and lead to unexpected behavior. Instead, rely on proper validation and initialization.

-  requested_quantity: parseFloat(form.requested_quantity || "1"),
-  assigned_quantity: parseFloat(form.assigned_quantity || "0"),
+  requested_quantity: parseFloat(form.requested_quantity),
+  assigned_quantity: parseFloat(form.assigned_quantity),

Likely invalid or redundant comment.

@Jacobjeevan
Copy link
Contributor

Resources was cleaned up in #9439. Hold this for now (you are free to refer to it and make your changes based on those before encountering any merge conflicts).

@github-actions github-actions bot added merge conflict pull requests with merge conflict and removed merge conflict pull requests with merge conflict labels Dec 16, 2024
Copy link

👋 Hi, @JavidSumra,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold merge conflict pull requests with merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource Update Allows Negative Quantity and Empty Description Fields
4 participants