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

fix: Ticket Order: User able to add invalid email #5690

Merged
merged 10 commits into from
Nov 19, 2020
Merged

fix: Ticket Order: User able to add invalid email #5690

merged 10 commits into from
Nov 19, 2020

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Nov 18, 2020

Fixes #5277

Short description of what this resolves:

Email are only added if they are valid

Changes proposed in this pull request:

  • Added the redExp for email

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Nov 18, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/6ayhzhylc
✅ Preview: https://open-event-frontend-git-validate-email-5277.eventyay.now.sh

@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #5690 (ade6c83) into development (c874482) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5690      +/-   ##
===============================================
+ Coverage        23.77%   23.78%   +0.01%     
===============================================
  Files              509      509              
  Lines             5414     5415       +1     
  Branches            59       59              
===============================================
+ Hits              1287     1288       +1     
+ Misses            4112     4111       -1     
- Partials            15       16       +1     
Impacted Files Coverage Δ
app/components/forms/orders/order-form.js 0.00% <ø> (ø)
app/utils/validators.js 90.90% <100.00%> (+0.90%) ⬆️
app/services/cache.ts 64.15% <0.00%> (-5.67%) ⬇️
app/components/tabbed-navigation.js 53.33% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c874482...ade6c83. Read the comment docs.

@divyamtayal
Copy link
Member Author

@iamareebjamal Please see the change I have made. And also I have one more suggestion, there are other places as well besides this where email validation is failing, so should I fix them also??

Screen Shot 2020-11-18 at 20 58 17 Screen Shot 2020-11-18 at 20 58 22
Screen Shot 2020-11-18 at 20 58 25 Screen Shot 2020-11-18 at 20 58 29
Screen Shot 2020-11-18 at 20 58 34 Screen Shot 2020-11-18 at 20 58 42

@iamareebjamal
Copy link
Member

Failing for valid email sad:jamal.areeb@mail.ab

@divyamtayal
Copy link
Member Author

Failing for valid email sad:jamal.areeb@mail.ab

is it so?

@divyamtayal
Copy link
Member Author

@iamareebjamal pls validate and see if its working now?

@divyamtayal
Copy link
Member Author

@iamareebjamal ofcourse we can't validate every possible case b for that validating through OTP would be best but for now this sol looks good to me then before where invalid emails where not validated

@iamareebjamal
Copy link
Member

Exactly. It's better to allow invalid emails than not allow valid emails

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 18, 2020

Exactly. It's better to allow invalid emails than not allow valid emails

So should I do now anything for it? bcz acc to @mariobehling in the issue OTP feature will be done later and he was asking to validate whether .com is there so what I'm thing is to use simple regExp that validates this which which take care of .com as well as not ignore valid emails bcz now foreg. 'tz@g' is valid which is for sure we can take care by using simple regExp.

@iamareebjamal Whats say?

@iamareebjamal
Copy link
Member

The regex should not filter out any valid email

@maze-runnar
Copy link
Contributor

I think current validation is ok because adding any regex for validating accurate email address will surely filter out valid ones. Because the validation of email is also a part of the provider. some emails are valid but not acceptable by one provider while acceptable by another provider. Even RFC can't validate 100% emails.

@iamareebjamal
Copy link
Member

iamareebjamal commented Nov 19, 2020

https://ihateregex.io/expr/email/

Simple enough and does not filter out most of valid emails.

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 19, 2020

@iamareebjamal

https://ihateregex.io/expr/email/

Simple enough and does not filter out most of valid emails.

OK

there are other places as well besides this where email validation is failing, so should I fix them also??

? like in loginForm etc

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging 6ce3701 into 9314043 - view on LGTM.com

new alerts:

  • 1 for Useless regular-expression character escape

@iamareebjamal
Copy link
Member

No, just here

@divyamtayal
Copy link
Member Author

divyamtayal commented Nov 19, 2020

No, just here

ok I'll do it here only

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging 0d225ea into 9314043 - view on LGTM.com

new alerts:

  • 1 for Useless regular-expression character escape

@divyamtayal
Copy link
Member Author

@iamareebjamal I have done the changes, and eslint error error is coming.

@iamareebjamal
Copy link
Member

yarn lint

@divyamtayal
Copy link
Member Author

yarn lint

how to fix this yarn lint error?

@iamareebjamal
Copy link
Member

iamareebjamal commented Nov 19, 2020

yarn lint

@divyamtayal
Copy link
Member Author

yarn lint

Screenshot from 2020-11-19 19-43-46
This is coming

@divyamtayal
Copy link
Member Author

either we have to disable no-control-regex

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging c9c20ad into c874482 - view on LGTM.com

new alerts:

  • 1 for Useless regular-expression character escape

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging ade6c83 into c874482 - view on LGTM.com

new alerts:

  • 1 for Useless regular-expression character escape

@iamareebjamal iamareebjamal changed the title Ticket Order: User able to add invalid email fix: Ticket Order: User able to add invalid email Nov 19, 2020
@auto-label auto-label bot added the fix label Nov 19, 2020
@iamareebjamal iamareebjamal merged commit 7d00804 into fossasia:development Nov 19, 2020
@divyamtayal divyamtayal deleted the validate-email-5277 branch November 23, 2020 00:45
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.

Ticket Order: User able to add invalid email
3 participants