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

Prevent Unnecessary Update Request in all usages of Form components and Facilities Section #8956

Closed
Closed
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
6644cdb
Added isDirty state to track form changes
kihan2518B Oct 28, 2024
b6dc6bf
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Oct 28, 2024
b2400f7
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Oct 29, 2024
086b282
Added 'save & add more' button in Form component
kihan2518B Oct 29, 2024
3c81bb1
Removed unnecessary logs and commits
kihan2518B Oct 29, 2024
a414d1f
solved wrong condition in BedCapacity component
kihan2518B Oct 29, 2024
5151974
fixed the form component to update the submit button label
kihan2518B Oct 29, 2024
56dcc10
formated submit button label in StaffCapacity and BedCapacity
kihan2518B Oct 30, 2024
f7726bc
removed unnecessary conditions
kihan2518B Oct 30, 2024
e52f76d
addded meaningfull variable names
kihan2518B Nov 2, 2024
971ad4d
updated shifDetailsUpdate page to use Form Component
kihan2518B Nov 4, 2024
4ff3dcf
solved merge conflicts in ShifDetailsUpdate component
kihan2518B Nov 4, 2024
def4813
updated ResourceDetailsUpdate component to use Form Component
kihan2518B Nov 4, 2024
be714d4
updated package-lock with latest one
kihan2518B Nov 4, 2024
f40317d
removed unnecessary types
kihan2518B Nov 5, 2024
cbea55e
removed unnecessary conditions
kihan2518B Nov 5, 2024
d77cd8f
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Nov 6, 2024
f8d191e
solved merge conflicts
kihan2518B Nov 6, 2024
7ae6649
Merge branch 'develop' into issues/8918/fix-form-validation
nihal467 Nov 6, 2024
be0102e
Merge branch 'develop' into issues/8918/fix-form-validation
nihal467 Nov 8, 2024
b9a7d19
Merge branch 'ohcnetwork:develop' into issues/8918/fix-form-validation
kihan2518B Nov 14, 2024
6b0c8be
Additional button text bug solved.
kihan2518B Nov 14, 2024
3177157
merge develop branch
kihan2518B Nov 21, 2024
79e8cb8
BedCapacity and Staff capacity tests failures.
kihan2518B Nov 25, 2024
20174b7
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Nov 25, 2024
fdbfa11
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Nov 27, 2024
656e776
completed requested changes.
kihan2518B Nov 27, 2024
99bd4e8
Changed cypress tests for Facility creation.
kihan2518B Nov 27, 2024
797f226
Solved PatintConsultationCreation test, and bugs in Bedcapacity,Staff…
kihan2518B Nov 28, 2024
be5d5a4
Solved Comparison bug in Form
kihan2518B Nov 28, 2024
e4e3cd4
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Nov 28, 2024
ba73bbe
efficient logic for value comparision in Form.
kihan2518B Nov 28, 2024
12c1a70
Updated tests to meet new feature.
kihan2518B Nov 29, 2024
48e3d16
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Nov 29, 2024
3d10120
modified tests functions
kihan2518B Nov 30, 2024
f488615
Modify cypress tests which are failing.
kihan2518B Dec 2, 2024
a99a443
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Dec 2, 2024
0681ed0
Modify cypress tests which are failing.
kihan2518B Dec 2, 2024
c480714
Logicaly disabling submit button in update patient.
kihan2518B Dec 2, 2024
9fef710
modified cypress tests to verify error messages.
kihan2518B Dec 4, 2024
86bb1b2
Added tests for disabled submit button
kihan2518B Dec 5, 2024
92d699d
Merge branch 'develop' into issues/8918/fix-form-validation
kihan2518B Dec 10, 2024
40fb41d
removed Unwanted changes
kihan2518B Dec 10, 2024
6ad1f06
added minor validations
kihan2518B Dec 13, 2024
57073e8
merge develop branch into issues/8918/fix-form-validation
kihan2518B Dec 13, 2024
5c39fc3
modified FacilityCreation test and some validation in BedCapacity.
kihan2518B Dec 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cypress/e2e/facility_spec/FacilityCreation.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,11 +267,13 @@ describe("Facility Creation", () => {
facilityPage.submitForm();
// add no bed capacity and verify form error message
facilityPage.isVisibleselectBedType();
facilityPage.selectBedType("Oxygen Supported Bed");
kihan2518B marked this conversation as resolved.
Show resolved Hide resolved
facilityPage.saveAndExitBedCapacityForm();
cy.verifyErrorMessages(bedErrorMessage);
facilityPage.clickcancelbutton();
// add no doctor capacity and verify form error message
facilityPage.isVisibleAreaOfSpecialization();
facilityPage.selectAreaOfSpecialization("Pulmonology");
facilityPage.clickdoctorcapacityaddmore();
cy.verifyErrorMessages(doctorErrorMessage);
facilityPage.clickcancelbutton();
Expand Down
6 changes: 4 additions & 2 deletions cypress/e2e/facility_spec/FacilityManage.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ describe("Facility Manage Functions", () => {
facilityManage.verifyTotalDoctorCapacity(doctorCapacity);
// edit a existing doctor
facilityManage.clickEditFacilityDoctorCapacity();
cy.get('button[type="submit"]').should("be.disabled");
facilityPage.fillDoctorCount(doctorModifiedCapacity);
facilityPage.clickdoctorcapacityaddmore();
facilityPage.saveAndExitDoctorForm();
facilityManage.verifySuccessMessageVisibilityAndContent(
"Staff count updated successfully",
);
Expand Down Expand Up @@ -157,9 +158,10 @@ describe("Facility Manage Functions", () => {
facilityManage.verifyFacilityBedCapacity(currentOccupied);
// edit a existing bed
facilityManage.clickEditFacilityBedCapacity();
cy.get('button[type="submit"]').should("be.disabled");
facilityPage.fillTotalCapacity(totalUpdatedCapacity);
facilityPage.fillCurrentlyOccupied(currentUpdatedOccupied);
facilityPage.clickbedcapcityaddmore();
facilityPage.saveAndExitBedCapacityForm();
facilityManage.verifySuccessMessageVisibilityAndContent(
"Bed capacity updated successfully",
);
Expand Down
215 changes: 113 additions & 102 deletions src/components/Facility/BedCapacity.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import { useEffect, useReducer, useState } from "react";
import { useTranslation } from "react-i18next";

import { Cancel, Submit } from "@/components/Common/ButtonV2";
import { CapacityModal, OptionsType } from "@/components/Facility/models";
import { SelectFormField } from "@/components/Form/FormFields/SelectFormField";
import TextFormField from "@/components/Form/FormFields/TextFormField";
import { FieldChangeEvent } from "@/components/Form/FormFields/Utils";

import { BED_TYPES } from "@/common/constants";

import * as Notification from "@/Utils/Notifications";
import routes from "@/Utils/request/api";
import request from "@/Utils/request/request";

import Form from "../Form/Form";
kihan2518B marked this conversation as resolved.
Show resolved Hide resolved

interface BedCapacityProps extends CapacityModal {
facilityId: string;
handleClose: () => void;
Expand Down Expand Up @@ -61,11 +61,6 @@ export const BedCapacity = (props: BedCapacityProps) => {
);
const [isLoading, setIsLoading] = useState(false);

const headerText = !id ? "Add Bed Capacity" : "Edit Bed Capacity";
const buttonText = !id
? `Save ${!isLastOptionType ? "& Add More" : "Bed Capacity"}`
: "Update Bed Capacity";

async function fetchCapacityBed() {
setIsLoading(true);
if (!id) {
Expand Down Expand Up @@ -117,64 +112,58 @@ export const BedCapacity = (props: BedCapacityProps) => {
fetchCapacityBed();
}, []);

useEffect(() => {
const lastBedType =
bedTypes.filter((i: OptionsType) => i.disabled).length ===
BED_TYPES.length - 1;
setIsLastOptionType(lastBedType);
}, [bedTypes]);

const handleChange = (e: FieldChangeEvent<unknown>) => {
const form = { ...state.form };
form[e.name] = e.value;
dispatch({ type: "set_form", form });
};

const validateData = () => {
//checking validation of the new form data comming from handle submit
const validateData = (form: typeof initForm) => {
const errors = { ...initForm };
let invalidForm = false;
Object.keys(state.form).forEach((field) => {
if (!state.form[field]) {
let validForm = true;
Object.keys(form).forEach((field) => {
if (!form[field]) {
errors[field] = t("field_required");
invalidForm = true;
} else if (
field === "currentOccupancy" &&
Number(state.form[field] < 0)
) {
validForm = false;
} else if (field === "currentOccupancy" && Number(form[field] < 0)) {
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix logical error in number conversion within conditional checks

There is a logical error in the way Number() is used within the conditional statements. The expression Number(form[field] < 0) converts the boolean result of form[field] < 0 into a number (0 or 1), which is not the intended comparison.

To fix this issue, you should convert form[field] to a number before performing the comparison.

Apply this diff to correct the conditionals:

- } else if (field === "currentOccupancy" && Number(form[field] < 0)) {
+ } else if (field === "currentOccupancy" && Number(form[field]) < 0) {
...
- } else if (field === "totalCapacity" && Number(form[field] < 0)) {
+ } else if (field === "totalCapacity" && Number(form[field]) < 0) {

Also applies to: 126-126

Copy link
Contributor

@Jacobjeevan Jacobjeevan Dec 11, 2024

Choose a reason for hiding this comment

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

Apply this 👍

Edit: convert to number first before comparing to 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a not IsNan check for these as well, no? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jacobjeevan Great! Thank you for applying the fix.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jacobjeevan, you're absolutely right. Adding a !isNaN check would improve the validation logic by ensuring that we only perform comparisons when the input is a valid number. We can update the conditional statements to include !isNaN(Number(form[field])).

For example:

} else if (
  field === "currentOccupancy" &&
  !isNaN(Number(form[field])) &&
  Number(form[field]) < 0
) {
  errors[field] = "Occupied cannot be negative";
  validForm = false;
}

This way, we ensure that form[field] is a valid number before performing the comparison, which prevents unexpected behavior if a non-numeric value is entered. We should apply this check to all numeric validations to enhance the robustness of the form validation.

errors[field] = "Occupied cannot be negative";
invalidForm = true;
validForm = false;
} else if (
field === "currentOccupancy" &&
Number(state.form[field]) > Number(state.form.totalCapacity)
Number(form[field]) > Number(form.totalCapacity)
) {
errors[field] = "Occupied must be less than or equal to total capacity";
invalidForm = true;
validForm = false;
}
if (field === "totalCapacity" && Number(state.form[field]) === 0) {
if (field === "totalCapacity" && Number(form[field]) === 0) {
errors[field] = "Total capacity cannot be 0";
invalidForm = true;
} else if (field === "totalCapacity" && Number(state.form[field]) < 0) {
validForm = false;
} else if (field === "totalCapacity" && Number(form[field]) < 0) {
errors[field] = "Total capacity cannot be negative";
invalidForm = true;
validForm = false;
}
});
if (invalidForm) {
if (!validForm) {
dispatch({ type: "set_error", errors });
return false;
}
dispatch({ type: "set_error", errors });
return true;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve form validation implementation

The validation logic needs several improvements:

  1. The number conversion syntax is incorrect in the conditions
  2. Error messages should be internationalized
  3. Type safety could be enhanced

Apply these changes:

 const validateData = (form: typeof initForm) => {
   const errors = { ...initForm };
   let validForm = true;
   Object.keys(form).forEach((field) => {
     if (!form[field]) {
-      errors[field] = t("field_required");
+      errors[field] = t("required_field");
       validForm = false;
-    } else if (field === "currentOccupancy" && Number(form[field] < 0)) {
+    } else if (field === "currentOccupancy" && Number(form[field]) < 0) {
-      errors[field] = "Occupied cannot be negative";
+      errors[field] = t("occupied_cannot_be_negative");
       validForm = false;
     } else if (
       field === "currentOccupancy" &&
       Number(form[field]) > Number(form.totalCapacity)
     ) {
-      errors[field] = "Occupied must be less than or equal to total capacity";
+      errors[field] = t("occupied_exceeds_capacity");
       validForm = false;
     }
     if (field === "totalCapacity" && Number(form[field]) === 0) {
-      errors[field] = "Total capacity cannot be 0";
+      errors[field] = t("capacity_cannot_be_zero");
       validForm = false;
-    } else if (field === "totalCapacity" && Number(form[field] < 0)) {
+    } else if (field === "totalCapacity" && Number(form[field]) < 0) {
-      errors[field] = "Total capacity cannot be negative";
+      errors[field] = t("capacity_cannot_be_negative");
       validForm = false;
     }
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//checking validation of the new form data comming from handle submit
const validateData = (form: typeof initForm) => {
const errors = { ...initForm };
let invalidForm = false;
Object.keys(state.form).forEach((field) => {
if (!state.form[field]) {
let validForm = true;
Object.keys(form).forEach((field) => {
if (!form[field]) {
errors[field] = t("field_required");
invalidForm = true;
} else if (
field === "currentOccupancy" &&
Number(state.form[field] < 0)
) {
validForm = false;
} else if (field === "currentOccupancy" && Number(form[field] < 0)) {
errors[field] = "Occupied cannot be negative";
invalidForm = true;
validForm = false;
} else if (
field === "currentOccupancy" &&
Number(state.form[field]) > Number(state.form.totalCapacity)
Number(form[field]) > Number(form.totalCapacity)
) {
errors[field] = "Occupied must be less than or equal to total capacity";
invalidForm = true;
validForm = false;
}
if (field === "totalCapacity" && Number(state.form[field]) === 0) {
if (field === "totalCapacity" && Number(form[field]) === 0) {
errors[field] = "Total capacity cannot be 0";
invalidForm = true;
} else if (field === "totalCapacity" && Number(state.form[field]) < 0) {
validForm = false;
} else if (field === "totalCapacity" && Number(form[field]) < 0) {
errors[field] = "Total capacity cannot be negative";
invalidForm = true;
validForm = false;
}
});
if (invalidForm) {
if (!validForm) {
dispatch({ type: "set_error", errors });
return false;
}
dispatch({ type: "set_error", errors });
return true;
};
//checking validation of the new form data comming from handle submit
const validateData = (form: typeof initForm) => {
const errors = { ...initForm };
let validForm = true;
Object.keys(form).forEach((field) => {
if (!form[field]) {
errors[field] = t("required_field");
validForm = false;
} else if (field === "currentOccupancy" && Number(form[field]) < 0) {
errors[field] = t("occupied_cannot_be_negative");
validForm = false;
} else if (
field === "currentOccupancy" &&
Number(form[field]) > Number(form.totalCapacity)
) {
errors[field] = t("occupied_exceeds_capacity");
validForm = false;
}
if (field === "totalCapacity" && Number(form[field]) === 0) {
errors[field] = t("capacity_cannot_be_zero");
validForm = false;
} else if (field === "totalCapacity" && Number(form[field]) < 0) {
errors[field] = t("capacity_cannot_be_negative");
validForm = false;
}
});
if (!validForm) {
dispatch({ type: "set_error", errors });
return false;
}
dispatch({ type: "set_error", errors });
return true;
};


const handleSubmit = async (e: any, btnType = "Save") => {
e.preventDefault();
const valid = validateData();
const headerText = !id ? "Add Bed Capacity" : "Edit Bed Capacity";
const buttonText = !id ? "Save Bed Capacity" : "Update Bed Capacity";

useEffect(() => {
const lastBedType =
bedTypes.filter((i: OptionsType) => i.disabled).length ===
BED_TYPES.length - 1;
setIsLastOptionType(lastBedType);
kihan2518B marked this conversation as resolved.
Show resolved Hide resolved
}, [bedTypes]);

const handleSubmit = async (form: typeof initForm, source?: string) => {
const valid = validateData(form);
if (valid) {
setIsLoading(true);
const bodyData = {
room_type: Number(state.form.bedType),
total_capacity: Number(state.form.totalCapacity),
current_capacity: Number(state.form.currentOccupancy),
room_type: Number(form.bedType),
total_capacity: Number(form.totalCapacity),
current_capacity: Number(form.currentOccupancy),
};
const { data } = await request(
id ? routes.updateCapacity : routes.createCapacity,
Expand All @@ -184,8 +173,9 @@ export const BedCapacity = (props: BedCapacityProps) => {
},
);
setIsLoading(false);
let updatedBedTypes;
if (data) {
const updatedBedTypes = bedTypes.map((type) => {
updatedBedTypes = bedTypes.map((type) => {
return {
...type,
disabled: data.room_type !== type.id ? type.disabled : true,
Expand All @@ -206,10 +196,23 @@ export const BedCapacity = (props: BedCapacityProps) => {
}
handleUpdate();
}
if (btnType == "Save and Exit") handleClose();
const disabledBedTypesLength = updatedBedTypes?.filter(
(item) => item.disabled,
).length;

if (
source !== "bed-capacity-save" ||
disabledBedTypesLength === BED_TYPES.length
)
handleClose();
}
};

const additionalButtonLabel =
!isLastOptionType && headerText === "Add Bed Capacity"
? "Save & Add More"
: "";

return (
<div className={className}>
{isLoading ? (
Expand All @@ -236,64 +239,72 @@ export const BedCapacity = (props: BedCapacityProps) => {
</div>
) : (
<div className={className}>
<SelectFormField
name="bedType"
id="bed-type"
label="Bed Type"
required
value={state.form.bedType}
options={bedTypes.filter((type) => !type.disabled)}
optionLabel={(option) => option.text}
optionValue={(option) => option.id}
onChange={handleChange}
disabled={!!id}
error={state.errors.bedType}
/>
<div className="flex flex-col gap-7 md:flex-row">
<TextFormField
className="w-full"
id="total-capacity"
name="totalCapacity"
label="Total Capacity"
required
type="number"
value={state.form.totalCapacity}
onChange={handleChange}
error={state.errors.totalCapacity}
min={1}
/>
<TextFormField
className="w-full"
id="currently-occupied"
label="Currently Occupied"
required
name="currentOccupancy"
type="number"
value={state.form.currentOccupancy}
onChange={handleChange}
error={state.errors.currentOccupancy}
min={0}
max={state.form.totalCapacity}
/>
</div>

<div className="cui-form-button-group mt-4">
<Cancel onClick={handleClose} />
{headerText === "Add Bed Capacity" && (
<Submit
id="bed-capacity-save-and-exit"
onClick={(e) => handleSubmit(e, "Save and Exit")}
Jacobjeevan marked this conversation as resolved.
Show resolved Hide resolved
label="Save Bed Capacity"
/>
)}
{!isLastOptionType && (
<Submit
id="bed-capacity-save"
onClick={(e) => handleSubmit(e)}
label={buttonText}
/>
<Form
defaults={state.form}
onSubmit={handleSubmit}
submitBtnId="bed-capacity-save-and-exit"
onCancel={handleClose}
submitLabel={buttonText}
className="my-auto p-0"
noPadding
hideRestoreDraft
additionalButtons={
isLastOptionType || headerText !== "Add Bed Capacity"
? []
: [
{
id: "bed-capacity-save",
type: "submit",
label: additionalButtonLabel,
},
]
}
>
{(field) => (
<>
<SelectFormField
name="bedType"
id="bed-type"
label="Bed Type"
required
value={field("bedType").value}
options={bedTypes.filter((type) => !type.disabled)}
optionLabel={(option) => option.text}
optionValue={(option) => option.id}
onChange={(e: any) => field("bedType").onChange(e)}
disabled={!!id}
error={state.errors.bedType}
/>
<div className="flex flex-col gap-7 md:flex-row">
<TextFormField
className="w-full"
id="total-capacity"
name="totalCapacity"
label="Total Capacity"
required
type="number"
value={field("totalCapacity").value}
onChange={(e: any) => field("totalCapacity").onChange(e)}
error={state.errors.totalCapacity}
min={1}
/>
<TextFormField
className="w-full"
id="currently-occupied"
label="Currently Occupied"
required
name="currentOccupancy"
type="number"
value={field("currentOccupancy").value}
onChange={(e: any) => field("currentOccupancy").onChange(e)}
error={state.errors.currentOccupancy}
min={0}
max={state.form.totalCapacity}
/>
</div>
</>
)}
</div>
</Form>
</div>
)}
</div>
Expand Down
1 change: 1 addition & 0 deletions src/components/Facility/FacilityBedCapacity.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export const FacilityBedCapacity = (props: any) => {
id="facility-add-bedtype"
className="w-full md:w-auto"
onClick={() => setBedCapacityModalOpen(true)}
disabled={BED_TYPES.length === capacityQuery.data?.count}
authorizeFor={NonReadOnlyUsers}
>
<CareIcon icon="l-bed" className="mr-2 text-lg text-white" />
Expand Down
Loading
Loading