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

User validation (Fixes #1996) #2169

Closed
wants to merge 3 commits into from

Conversation

Ankit-Singla
Copy link

@Ankit-Singla Ankit-Singla commented Jan 30, 2018

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

  • 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!

@PublicLabBot
Copy link

PublicLabBot commented Jan 30, 2018

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
2 Messages
📖 @Ankit-Singla Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@Ankit-Singla Ankit-Singla changed the title User validation (fixes #1996) User validation (Fixes #1996) Jan 30, 2018
@Ankit-Singla
Copy link
Author

Will this do?
I forgot to rebase the feature branch on top of the master branch this time. Or should I open another pull request after doing that?
Rest everything is fine, I suppose.

@namangupta01
Copy link
Member

@Ankit-Singla Just remove the commits of email validation and rebase it with plots master. if you need help doing so you can ask!

@Ankit-Singla
Copy link
Author

@namangupta01 Why do I need to remove commits? And how can I do that?

@namangupta01
Copy link
Member

@Ankit-Singla you have three commits here 0c86186, 7b41e61 and 53f3f61 where 7b41e61 and 53f3f61 belongs to your pr #2164 . We only need 0c86186 this commit here. Doesn't we?

@Ankit-Singla
Copy link
Author

@namangupta01
Okay, got it.
And how can I remove those commits?

@namangupta01
Copy link
Member

For removing commits you can type git rebase -i HEAD~3 here 3 represents the number of previous 2 commits from the current head and including current head and then you will get three commits info with commit hash, message and description then delete the whole hash of the commits that you want to delete. I think that will works for you.
P.S For trying and learning first and to know hows it works try the above stuff on any demo branch so that in case something went wrong your work will not be lost! :)

@Ankit-Singla
Copy link
Author

@SidharthBansal Please review this pull request.

@ViditChitkara
Copy link
Member

This is working perfectly here http://rubular.com/r/Pyb899szdt
Thanks @Ankit-Singla 👍

@jywarren
Copy link
Member

jywarren commented Feb 2, 2018

This looks great! But i wanted to note -- what about non-latin characters? Sorry because this is pretty tough, but consider:

What do you think?

@Ankit-Singla
Copy link
Author

Ankit-Singla commented Feb 3, 2018

@jywarren We can use POSIX bracket expressions to solve this issue. Sorry that I wasn't careful this time. Therefore, a better regex would be /\A[-[:alnum:]+.]+@[\da-z-.]+[.][a-z]+\z/, if we want to allow non-latin characters only before the @ symbol, otherwise /\A[-[:alnum:]+.]+@[[:alnum:]-.]+[.][[:alpha:]]+\z/ would be a better choice.
Do you approve of any of the following, so that I could make the required changes in the Pull Request?
Both of your test strings work fine with either of the regex:

@jywarren
Copy link
Member

jywarren commented Feb 5, 2018

@ViditChitkara what do you think? This looks good to me!

@ViditChitkara
Copy link
Member

Looks super!! :+1

@SidharthBansal
Copy link
Member

SidharthBansal commented Feb 5, 2018

Looks good to me too

@Ankit-Singla
Copy link
Author

@jywarren So I have made the required changes to include non-latin characters in email validation. Please review the pull request.

@jywarren
Copy link
Member

jywarren commented Feb 6, 2018

This is an odd error in Travis:

1.72s$ if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then danger --verbose; fi
You used `puts` in your Dangerfile. To print out text to GitHub use `message` instead
There was an error with Danger bot's Junit parsing:
undefined method `value' for nil:NilClass
⚠️ 1 Warning. Don't worry, everything is fixable.
Danger does not have write access to the PR to set a PR status.

https://travis-ci.org/publiclab/plots2/builds/337619181#L3377

I think we have a problem in our Dangerfile that's preventing it from reporting out the error correctly.

password: 'test1234',
password_confirmation: 'test1234',
email: 'ankit@gmail')
assert_not user.save({})
Copy link
Member

Choose a reason for hiding this comment

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

Ah! Oops, you forgot an end here -- that should fix it!

Copy link
Member

Choose a reason for hiding this comment

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

@Ankit-Singla everything all right? I think adding end will fix this and we'll be good to go. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Hi @Ankit-Singla -- no rush, just making sure you've seen this and if you need help we can pitch in! :-)

@SidharthBansal
Copy link
Member

Issue fixed #2506

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

Successfully merging this pull request may close these issues.

6 participants