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

force username to be lowercase #49

Closed
wants to merge 1 commit into from
Closed

Conversation

gnulnx
Copy link

@gnulnx gnulnx commented Sep 4, 2015

No description provided.

pipe.delete(get_username_attempt_cache_key(username))
pipe.delete(get_username_blocked_cache_key(username))
pipe.delete(get_username_attempt_cache_key(username.lower()))
pipe.delete(get_username_blocked_cache_key(username.lower()))
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure why these lines where here twice?

Does it make sense to leave them and then add two more that do the same with the username.lower()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I didn't see this sooner. I think it makes sense to always store usernames in the same case, lower probably.

We could so add the .lower() inside of the get_username_attempt_cache_key() and get_username_blocked_cache_key() this way we don't have to always pass in a lower case username, and it keep it all in one place.

Copy link
Author

Choose a reason for hiding this comment

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

hey sorry guy's I guess we all dropped the ball on this one. My apologies for not seeing this sooner as well!

@kencochrane
Copy link
Collaborator

Should we close this, it has been open for over a year and a half with no update.

@MattBlack85
Copy link
Contributor

It seems to me like a reasonable use case tough, can grab this, it doesn't look like a big amount of work

@kencochrane
Copy link
Collaborator

@MattBlack85 I agree, thank you!

@kencochrane
Copy link
Collaborator

closing since #90 is replacing this one. Thanks for the help.

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.

None yet

3 participants