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

Improve validation check for phone number in patient list page #8189 #8394

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

Conversation

Nithin9585
Copy link
Contributor

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • 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 Q
phno-issue.mp4

@Nithin9585 Nithin9585 requested a review from a team as a code owner August 22, 2024 09:54
Copy link

vercel bot commented Aug 22, 2024

@Nithin9585 is attempting to deploy a commit to the Open Healthcare Network Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit d4c4368
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/66c70ae0a032c500086844f6
😎 Deploy Preview https://deploy-preview-8394--care-egov-staging.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.

Comment on lines 128 to 131
const isValidPhoneNumber = (phoneNumber: string) => {
const phoneNumberRegex = /^\+91[0-9]{10}$/;
return phoneNumberRegex.test(phoneNumber);
};
Copy link
Member

Choose a reason for hiding this comment

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

We already have a reusable validator for Phone Number form field. Avoid re-defining the validations for common ones.

https://github.com/coronasafe/care_fe/blob/develop/src/Components/Form/FieldValidators.tsx#L100

Copy link

netlify bot commented Aug 24, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit f3745cb
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/6712458176630100084574c0
😎 Deploy Preview https://deploy-preview-8394--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
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

We could remove the phoneNumberError states if not being used right? We are not setting the state with a value anywhere. We are only clearing it. So no reason to keep the state right?

@@ -141,8 +139,6 @@ export const PatientManager = () => {
updateQuery({ emergency_phone_number: null });
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To continue with the error, i.e., 8-digit and 9-digit phone numbers are considered valid. This is because it is validating telephone numbers and other numbers that are more than 10 digits 13 digits. Can I get some suggestions to correct this?

Copy link
Member

@rithviknishad rithviknishad Aug 26, 2024

Choose a reason for hiding this comment

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

We don't need to clear the error if we are not setting the error state with a value in the first place right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but what about this issue 8 digit and 9 digit numbers are considered as valid because it is validating telephone number also !.

Copy link
Member

Choose a reason for hiding this comment

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

You can trigger the search as soon as the number is considered valid. Use the PhoneNumberValidator utility to achieve that.

Copy link
Member

@gigincg gigincg left a comment

Choose a reason for hiding this comment

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

Needs Cypress Tests

@nihal467 Can you look into this

@nihal467 nihal467 self-assigned this Sep 2, 2024
@nihal467
Copy link
Member

nihal467 commented Sep 3, 2024

image

  • When we enter an 8-digit number, it doesn't throw an error, even though there are no valid 8-digit numbers in India.

@nihal467 nihal467 removed their assignment Sep 3, 2024
@nihal467
Copy link
Member

nihal467 commented Sep 6, 2024

@Nithin9585 are you still working on this issue

@github-actions github-actions bot added the stale label Sep 14, 2024
Copy link

github-actions bot commented Oct 1, 2024

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

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 1, 2024
@nihal467
Copy link
Member

nihal467 commented Oct 1, 2024

image

  • When we enter an 8-digit number, it doesn't throw an error, even though there are no valid 8-digit numbers in India.

@Nithin9585 this issue still exist

@rithviknishad
Copy link
Member

@nihal467 if that's an issue isn't that issue relevant in all other Phone Number Form Fields too? The code now uses the same validator as the one used in all other phone number fields.

@bodhish bodhish added question Further information is requested and removed test failed labels Oct 7, 2024
@bodhish
Copy link
Member

bodhish commented Oct 7, 2024

@nihal467 check this out ☝️

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Oct 8, 2024
@rithviknishad rithviknishad added needs testing and removed question Further information is requested labels Oct 15, 2024
@nihal467
Copy link
Member

@Nithin9585 @rithviknishad can you fix the cypress failure

@nihal467
Copy link
Member

image

@rithviknishad why data is filtered, when we type 8 digits, in India, we don't have a valid 8-digit phone or landline number, right ?

@nihal467 nihal467 added the question Further information is requested label Oct 18, 2024
@nihal467 nihal467 removed their assignment Oct 18, 2024
@rithviknishad
Copy link
Member

@nihal467 Updated the indian landline number regex validation to ensure it's a 10 digit number as per TRAI docs.

References: https://trai.gov.in/sites/default/files/Recommendations_29052020.pdf

image

@rithviknishad rithviknishad removed the question Further information is requested label Oct 18, 2024
@nihal467
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the Validation check for phone number in patient list page
5 participants