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

add email validate #1999

Closed
wants to merge 2 commits into from
Closed

add email validate #1999

wants to merge 2 commits into from

Conversation

Paarmita
Copy link
Member

@Paarmita Paarmita commented Jan 13, 2018

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

Solves #1996

  • all tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request is descriptively named with #number reference back to original issue

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

Copy link
Member

@SidharthBansal SidharthBansal left a comment

Choose a reason for hiding this comment

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

You need to use the regex in the line mentioned.
You don't need to write it separately

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 13, 2018

We are glad to see your Pull request.
Thanks for the PR. I have mentioned some changes go for it.
Also, write a test to demonstrate that pratima@gmail will not lead to signup in the plots2/test/unit/user_test.rb. Make sure that all the tests pass.

@jywarren
Copy link
Member

Hi! Looks like you're having some issues -- maybe @publiclab/reviewers can help out. You can always look through the output of the tests to see what the errors are, by clicking on the little red X next to your last commit:

https://travis-ci.org/publiclab/plots2/builds/328785772

Thanks for your contribution!! I'm sure we'll figure it out.

@ryzokuken
Copy link
Member

Hi, @Paarmita! Thanks for being so patient and kudos for taking this step to improve publiclab. It really means a lot to all of us.

I see your pull request is getting an error, specifically: SyntaxError: /app/app/models/user.rb:38: empty range in char class: /\A[\w+-.]+@[a-z\d-.]+.[a-z]+\z/i

This most probably means that there's something wrong with the Regex, so I'd advise you to kindly take a look at it again.

@ryzokuken
Copy link
Member

Spoiler alert: \A[-\w+.]+@[\da-z-.]+.[a-z]+\z seemed to work.

@jywarren
Copy link
Member

jywarren commented Jan 16, 2018 via email

@jywarren jywarren mentioned this pull request Jan 17, 2018
@namangupta01
Copy link
Member

@Paarmita Do you need any help with this? Please, let us know! We are happy to help.

@Ankit-Singla
Copy link

@ryzokuken \A[-\w+.]+@[\da-z-.]+.[a-z]+\z doesn't work, because of the . that matches any character except for the newline character. It should be replaced by [.] for it to actually match the . character.

@ryzokuken
Copy link
Member

@Ankit-Singla idk, you should use whatever works in the tests, I tried that Regex on rubular and it seemed to work.

@jywarren
Copy link
Member

jywarren commented Feb 1, 2018

@Paarmita @Ankit-Singla @ryzokuken how's this coming along? Sorry this is a little more complex -- just a suggestion -- you can take a test string and input it in http://rubular.com with the regex you're using, like this:

http://rubular.com/r/vKHN1A7H4k

@Ankit-Singla can you offer an example of why the above regex would fail, and how your proposed improvement would be better? Thank you!

@Ankit-Singla
Copy link

Ankit-Singla commented Feb 1, 2018

@jywarren The regex suggested by @ryzokuken ( \A[-\w+.]+@[\da-z-.]+.[a-z]+\z ) doesn't work because it doesn't match the . in the email address correctly.
http://rubular.com/r/YMcSHIJzAT
You can see here that it gives a match when the user email is "jeff@publiclab", while ideally it shouldn't match. Thus, it doesn't enforce the presence of . in the email address.
I suggested ( \A[-\w+.]+@[\da-z-.]+.[a-z]+\z ), replacing the . by \. or [.], which matches the . literally, and not the characters represented by ..

@ryzokuken
Copy link
Member

@SidharthBansal was this already fixed in a PR?

@SidharthBansal
Copy link
Member

Parmita left and ankit was doing this. So I closed this pr

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

Successfully merging this pull request may close these issues.

6 participants