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.

Sign up for GitHub

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

Jump to bottom

fix: Added Validation for Insurance input fields in patient registration issue (#8672) #8681

Open

syedfardeenjeelani wants to merge 26 commits into ohcnetwork:develop from syedfardeenjeelani:issues/8672/fix-issue

Contributor

syedfardeenjeelani commented Oct 1, 2024 •

edited

Loading

Proposed Changes

Fixes Add Validation for Insurance input fields in patient registration form #8672
Added frontend validation for insurance input fields (Member ID, Policy ID, and Insurer Name) in the patient registration form.

All.fields.are.mandatory.1.mp4

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

Prep screenshot or demo video for changelog entry, and attach it to the issue.


          fix: Add Validation for Insurance input fields in patient registratio…

d1c1cd5

…n form (ohcnetwork#8672)

syedfardeenjeelani requested a review from a team as a code owner

October 1, 2024 05:53

netlify bot commented Oct 1, 2024 •

edited

Loading

❌ Deploy Preview for care-ohc failed.

Name	Link
🔨 Latest commit	`d7da9f8`
🔍 Latest deploy log	https://app.netlify.com/sites/care-ohc/deploys/67135586dc4c840008785e62

Member

rithviknishad commented Oct 1, 2024

@syedfardeenjeelani is this ready for review?

Contributor Author

syedfardeenjeelani commented Oct 1, 2024

@syedfardeenjeelani is this ready for review?

we are getting the error from the validator.ts which is something like this
if (
!value.policy_id.trim() ||
!value.subscriber_id.trim() ||
(enable_hcx && (!value.insurer_id?.trim() || !value.insurer_name?.trim()))
)
return "All fields are mandatory";

so should i use field_required from the i18n locale here instead of "All fields are mandatory"

Member

rithviknishad commented Oct 1, 2024

yes

Contributor Author

syedfardeenjeelani commented Oct 1, 2024

yes

Thanks I'll do the changes and push

Contributor Author

syedfardeenjeelani commented Oct 1, 2024

@rithviknishad if we do it like this
if (
!value.policy_id.trim() ||
!value.subscriber_id.trim() ||
(enable_hcx && (!value.insurer_id?.trim() || !value.insurer_name?.trim()))
)
return t("field_required");
};

then it will also show it right here

so instead should a make a key which says "all_fields_mandatory": "All fields are mandatory"

or should we handle both cases seprately

Member

rithviknishad commented Oct 1, 2024 •

edited

Loading

Let's keep the error texts same as before itself instead of showing the error on ALL fields, just that ensure ALL fields required validation happens as mentioned in the issue.


          fix: Add Validation for Insurance input fields in patient registratio…

96b810f

…n form (ohcnetwork#8672)

Contributor Author

syedfardeenjeelani commented Oct 1, 2024

@rithviknishad I've made some changes based on your feedback, but I'm not sure if I got it right. Could you take a look at my PR when you get a chance and let me know if i am headed in the right direction or not ?

rithviknishad requested changes

View reviewed changes

Member

rithviknishad left a comment •

edited

Loading

Only validation logic in this file needs to be modified: https://github.com/ohcnetwork/care_fe/blob/develop/src/Components/HCX/validators.ts

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

		label={t("policy__insurer_name")}
		error={error}

Member

rithviknishad Oct 3, 2024

Suggested change

error={error}

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

@@ @@ -133,6 +138,7 @@ const InsuranceDetailEditCard = ({ @@
                       <TextFormField
                         required
                         name="policy_id"
+                        error={error}

Member

rithviknishad Oct 3, 2024

Suggested change

error={error}

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

@@ @@ -125,6 +129,7 @@ const InsuranceDetailEditCard = ({ @@
                       <TextFormField
                         required
                         name="subscriber_id"
+                        error={error}

Member

rithviknishad Oct 3, 2024

Suggested change

error={error}

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

               }: {
                 policy: HCXPolicyModel;
                 handleUpdate: (event: FieldChangeEvent<unknown>) => void;
                 handleUpdates: (diffs: object) => void;
                 handleRemove: () => void;
                 gridView?: boolean;
+                error: FieldError;

Member

rithviknishad Oct 3, 2024

Suggested change

error: FieldError;

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

@@ @@ -91,12 +93,14 @@ const InsuranceDetailEditCard = ({ @@
                 handleUpdates,
                 handleRemove,
                 gridView,
+                error,

Member

rithviknishad Oct 3, 2024

Suggested change

error,

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

                         >
                           <InsuranceDetailEditCard
                             policy={policy}
+                            error={props.error}

Member

rithviknishad Oct 3, 2024

Suggested change

error={props.error}

src/Components/HCX/InsuranceDetailsBuilder.tsx Outdated

@@ @@ -15,6 +15,7 @@ import request from "../../Utils/request/request"; @@
               import routes from "../../Redux/api";
               import { useTranslation } from "react-i18next";
               import careConfig from "@careConfig";
+              import { FieldError } from "../Form/FieldValidators";

Member

rithviknishad Oct 3, 2024

Suggested change

import { FieldError } from "../Form/FieldValidators";

src/Redux/fireRequest.tsx Outdated

Comment on lines 145 to 149

+                          console.log(error.response.data.detaillocal_ip_address);
+                          // Notification.BadRequest({
+                          //   errs: error.response.data,
+                          // });
+                          // return error.response;

Member

rithviknishad Oct 3, 2024

Why was this changed?

Contributor Author

syedfardeenjeelani commented Oct 3, 2024

Only validation logic in this file needs to be modified: https://github.com/ohcnetwork/care_fe/blob/develop/src/Components/HCX/validators.ts

hey @rithviknishad should it be something like this
if (!value.policy_id.trim()) {
return "some err message";
} else if (!value.subscriber_id.trim()) {
return "some err message";
}
if (enable_hcx) {
if (!value.insurer_id?.trim()) {
return "some err message";
} else if (!value.insurer_name?.trim()) {
return "some err message";
}
}

also do we only have to change this particular file and nothing else

Member

rithviknishad commented Oct 3, 2024

also do we only have to change this particular file and nothing else

yes

syedfardeenjeelani and others added 4 commits

October 4, 2024 20:33


          fix: fixed the validation logic of the insurance form

90dcac7


          fix: fixed the validation logic of the insurance form

6bc35c4


          fix: fixed the validator.ts

8768f3b


          Update fireRequest.tsx

nihal467 added the needs testing label

Member

nihal467 commented Oct 4, 2024

@rithviknishad can you verify is it ready for testing

nihal467 added needs review and removed needs testing labels

rithviknishad requested changes

View reviewed changes

Member

rithviknishad left a comment

texts to be in i18n locale files

https://github.com/ohcnetwork/care_fe#translations

Contributor Author

syedfardeenjeelani commented Oct 5, 2024

texts to be in i18n locale files

https://github.com/ohcnetwork/care_fe#translations

@rithviknishad hello should i put them in common.json

syedfardeenjeelani added 2 commits

October 6, 2024 13:08


          fix: Move validation messages to i18n locale files and refactor HCXPo…

64963bf

…licyValidator


          Merge branch 'issues/8672/fix-issue' of https://github.com/syedfardee…

0ce16b3

…njeelani/care_fe into issues/8672/fix-issue

github-actions bot added the merge conflict label

github-actions bot commented Oct 7, 2024

👋 Hi, @syedfardeenjeelani,
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.

Member

rithviknishad commented Oct 8, 2024

Put in en.json

Contributor Author

syedfardeenjeelani commented Oct 8, 2024

Put in en.json

okay


          avoid passing t as param

ea0291d

rithviknishad approved these changes

View reviewed changes

rithviknishad added needs testing and removed needs review labels

github-actions bot added the merge conflict label

github-actions bot commented Oct 16, 2024

👋 Hi, @syedfardeenjeelani,
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.


          Merge branch 'develop' into issues/8672/fix-issue

10f9aba

github-actions bot added Deploy-Failed and removed merge conflict labels


          Merge branch 'develop' into issues/8672/fix-issue

a33a94a

Member

nihal467 commented Oct 16, 2024 •

edited

Loading

Even though the validation is added, it's not blocking the user from submitting a patient registration form when the insurance fields are kept blank

nihal467 added test failed and removed needs testing Deploy-Failed labels

Contributor

AdityaJ2305 commented Oct 16, 2024

Hey @syedfardeenjeelani , Please check by adding this line errors["insurance_details"] = insuranceDetailsError; in validateForm func in PatientRegister.tsx

github-actions bot added the merge conflict label

github-actions bot commented Oct 16, 2024

👋 Hi, @syedfardeenjeelani,
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.

Member

rithviknishad commented Oct 17, 2024

@syedfardeenjeelani could you resolve the merge conflicts?

Member

rithviknishad commented Oct 17, 2024

ensure the en.json file has keys sorted. pre-commit should do it for you if setup properly.


          fixed validation

f5a2651

syedfardeenjeelani force-pushed the issues/8672/fix-issue branch from 968340c to f5a2651 Compare

October 17, 2024 04:48

syedfardeenjeelani and others added 2 commits

October 17, 2024 10:29


          sorted en.json alphabetically

9bdfdc6


          Merge branch 'develop' into issues/8672/fix-issue

2f75935

github-actions bot added Deploy-Failed and removed merge conflict labels

Contributor Author

syedfardeenjeelani commented Oct 19, 2024

@nihal467 please test this

rithviknishad requested changes

View reviewed changes

src/Locale/en.json Outdated

Member

rithviknishad Oct 19, 2024

why is the entire file having a change?

Contributor Author

syedfardeenjeelani Oct 19, 2024

pre-commit was not sorting keys for me so i used https://codeshack.io/json-sorter/

Member

rithviknishad Oct 19, 2024

use npm run sort-locales instead; also why is pre-commit not sorting?

Contributor Author

syedfardeenjeelani Oct 19, 2024 •

edited

Loading

i will look into it @rithviknishad also can you please tell me if i should hard reset to commit f5a265172e499e4f4016be1453751680b7eb416b (fixed validation)

rithviknishad added changes required and removed test failed labels


          Merge branch 'develop' into issues/8672/fix-issue

d7da9f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes required Deploy-Failed