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

Make email ID's case insensitive #5728

Merged
merged 8 commits into from Apr 13, 2019

Conversation

@prateekj117
Copy link
Member

prateekj117 commented Mar 28, 2019

Fixes #5643

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests 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)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

Email IDs are case-sensitive, allowing multiple accounts with one handle

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Mar 28, 2019

@iamareebjamal Getting this error when trying to create migrations:
Screenshot from 2019-03-29 02-13-44

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #5728 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5728   +/-   ##
============================================
  Coverage        64.35%   64.35%           
============================================
  Files              274      274           
  Lines            13189    13189           
============================================
  Hits              8488     8488           
  Misses            4701     4701

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 7900f6e...7c2cc1c. Read the comment docs.

@prateekj117 prateekj117 force-pushed the prateekj117:email-lowercase branch from 360a664 to f40d1e5 Mar 30, 2019
for repeated emails
@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Mar 30, 2019

@iamareebjamal Please review the changes made till now.
After this, one problem after making index is that user is still allowed to register with email address not totally in lowercase. So, for that, I will see if there is any way to make it as a constraint.

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Mar 30, 2019

There's no lower case unique index migration

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Mar 30, 2019

@iamareebjamal Using something like check(translate) will do the job I think.

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Mar 30, 2019

@iamareebjamal
ALTER TABLE users ADD CONSTRAINT users_email_lowercase_ck CHECK (_email = lower(_email));

I think this will do.

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Mar 31, 2019

Yes, so add that

Along with adding deleted_at, change the email before adding the constraint

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Mar 31, 2019

Use citext for email column

https://stackoverflow.com/a/25438489

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Mar 31, 2019

Also, add migration to convert all values to lowercase after creating index

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Mar 31, 2019

@iamareebjamal Please review. Tested. Everything works as required.

op.execute(" UPDATE users SET deleted_at = current_timestamp FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",
op.execute("UPDATE users SET deleted_at = current_timestamp FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",
execution_options=None)
op.execute("UPDATE users SET _email = concat(_email, '_') FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Mar 31, 2019

Member

This can be done in above statement only, no need to run the query again

execution_options=None)
op.execute("update users set _email = lower(_email);",
execution_options=None)
op.execute("alter table users add check ( lower(_email) = _email );",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Mar 31, 2019

Member

No need to add check

Again, there is no index here, which makes this migration useless. Use citext extension to add a case insensitive column

execution_options=None)


def downgrade():
pass
op.execute("UPDATE users SET deleted_at = null FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Mar 31, 2019

Member

Won't work

pass
op.execute("UPDATE users SET deleted_at = null FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",
execution_options=None)
op.execute("UPDATE users SET _email = replace(_email, right(_email, 1), '') FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Mar 31, 2019

Member

Won't work

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Apr 1, 2019

@iamareebjamal Please review.



def upgrade():
op.execute("UPDATE users SET deleted_at = current_timestamp, _email = concat(_email, '_') FROM (SELECT lower(_email) FROM users GROUP BY lower(_email) HAVING COUNT(*) > 1) as email where lower(_email) != _email;",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Apr 1, 2019

Member

This query updated 313 users, where only 72 users had different case emails

…nto email-lowercase
@Anupam-dagar Anupam-dagar mentioned this pull request Apr 1, 2019
6 of 6 tasks complete


def downgrade():
op.execute("alter table users alter column _email type text;",

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Apr 4, 2019

Member

Add an option of undeleting users and removing underscore from their emails

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Apr 4, 2019

@iamareebjamal Please review.

@prateekj117

This comment has been minimized.

Copy link
Member Author

prateekj117 commented Apr 6, 2019

@CosmicCoder96 Please review and merge.

@iamareebjamal iamareebjamal mentioned this pull request Apr 13, 2019
6 of 6 tasks complete
@iamareebjamal iamareebjamal merged commit cd1caa4 into fossasia:development Apr 13, 2019
5 checks passed
5 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Hound No violations found. Woof!
codecov/patch Coverage not affected when comparing 7900f6e...7c2cc1c
Details
codecov/project 64.35% remains the same compared to 7900f6e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.