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

Take Two on Case Sensitive Username and Email - #65 #101

Merged
merged 9 commits into from
Mar 8, 2014

Conversation

bshardi
Copy link
Contributor

@bshardi bshardi commented Feb 27, 2014

After reading the discussion and seeing that we had caught some other cases where email and username needed to be toLowerCase, I decided to make something a little more complete. #65

  1. Added two new settings, emailToLower and usernameToLower to allow customization of the feature. I have set emailToLower to true by default and username to false. (In my personal use I will use both to true).
  2. The settings are checked in signIn and signUp
  3. Have to force username to not be an email address as this could introduce an issue where username could point to two records or get the login confused anyway.

Seems to work well for me. Let me know what other issues I have missed or give suggestions and criticism.

Todo:

  • Needs some tests written
  • I do not like where I put isStringEmail, seems it would be best as an addition to the String.prototype as isEmail. I just could not figure out where to put my String::isEmail = assignment.
  • I attempted to translate my new error.usernameIsEmail text. That needs to be proofed.

@queso
Copy link
Contributor

queso commented Feb 27, 2014

This is related to #65

@queso
Copy link
Contributor

queso commented Feb 27, 2014

/cc @calebl @ryw @softwarerero

@queso
Copy link
Contributor

queso commented Feb 27, 2014

@softwarerero I was hoping you could review the language bits on this PR for me. If polish isn't in your wheelhouse, I can find someone for that too.

@softwarerero
Copy link
Contributor

Sure, I but I can only do English, Spanish and German. Would you merge the PR first. I'm not sure if I can commit to a pull request of somebody else.

@queso
Copy link
Contributor

queso commented Mar 6, 2014

@bshardi hate to say it, but your branch is out of sync... Can you get it up to date with Master so I can merge?

@bshardi
Copy link
Contributor Author

bshardi commented Mar 6, 2014

Josh,

I think i can do it. Have found some articles googling. I think worse case
I can delete my pull request and make a new one if I have to. Give me a few
min and I will let you know how it goes.

Bart

On Thu, Mar 6, 2014 at 9:56 AM, Bart Hardison [email protected]:

I mean make a new pull request.

On Thu, Mar 6, 2014 at 9:56 AM, Bart Hardison [email protected]:

Yes, what do i need to do to do this. Sorry for being new to contributing.

I assume I do a pull from your repository and merge. Then make a new fork?

Thanks

Bart

On Thu, Mar 6, 2014 at 9:41 AM, Josh Owens [email protected]:

@bshardi https://github.com/bshardi hate to say it, but your branch
is out of sync... Can you get it up to date with Master so I can merge?

Reply to this email directly or view it on GitHubhttps://github.com//pull/101#issuecomment-36893700
.

@bshardi
Copy link
Contributor Author

bshardi commented Mar 6, 2014

@queso I have merged changes to my pull. Is this pr good now?

queso added a commit that referenced this pull request Mar 8, 2014
Take Two on Case Sensitive Username and Email - #65
@queso queso merged commit 70a5faa into Differential:master Mar 8, 2014
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.

4 participants