fix: allows deleted users to signup with same email #5924

shreyanshdwivedi · 2019-05-18T11:55:37Z

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:

Currently, if a user deletes his/her account or the account is deleted by an admin, he/she can't signup with same email.

Changes proposed in this pull request:

Revamps the deletion of a user on server.
adds a function to add .deleted to a user's email which is to be deleted
removes .deleted from user's email which is to be restored.

app/api/helpers/user.py

shreyanshdwivedi · 2019-05-18T12:07:19Z

@uds5501 @mrsaicharan1 @iamareebjamal everything is working fine but when I update the email of user in modify_email_for_user_to_be_restored function, it is not getting reflected on database. Can you please help me out with this?

iamareebjamal · 2019-05-18T12:17:20Z

app/api/helpers/user.py

+            if userEmail.endswith('.deleted'):
+                userEmail = userEmail[:-8]
+            else:
+                userEmail = userEmail + '.new'


Suppose a user is restored but he/she already signed up so the constraint is violated as 2 users cannot have same email. So I just added a '.new' to the email just for this sake.
This case however may happen only once or twice

This should not be automated. Admin should first change the email manually to something and then restore

@iamareebjamal I'll remove this condition and if you want I can open a separate issue to handle this.

when I update the email of user in modify_email_for_user_to_be_restored function, it is not getting reflected on database

In the meantime can you please give this a look? ^^

@iamareebjamal any help?

Will look tomorrow. @prateekj117 @uds5501 Can you please take a look as well?

@iamareebjamal @shreyanshdwivedi will take a look tonight

@shreyanshdwivedi The code looks correct. Any error logs during implementation?

No error logs. @uds5501
@iamareebjamal can you take a look as well please

codecov · 2019-05-18T12:19:46Z

Codecov Report

Merging #5924 into development will decrease coverage by 0.1%.
The diff coverage is 20.93%.

@@               Coverage Diff               @@
##           development    #5924      +/-   ##
===============================================
- Coverage        66.48%   66.37%   -0.11%     
===============================================
  Files              286      287       +1     
  Lines            13933    13969      +36     
===============================================
+ Hits              9263     9272       +9     
- Misses            4670     4697      +27

Impacted Files	Coverage Δ
app/api/helpers/user.py	`25% <25%> (ø)`
app/api/users.py	`32.07% <9.09%> (-0.19%)`	⬇️

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 215b471...64675f1. Read the comment docs.

codecov · 2019-05-18T12:19:46Z

Codecov Report

Merging #5924 into development will decrease coverage by 0.1%.
The diff coverage is 19.04%.

@@               Coverage Diff               @@
##           development    #5924      +/-   ##
===============================================
- Coverage        66.46%   66.35%   -0.11%     
===============================================
  Files              284      285       +1     
  Lines            13907    13941      +34     
===============================================
+ Hits              9243     9251       +8     
- Misses            4664     4690      +26

Impacted Files	Coverage Δ
app/api/helpers/user.py	`24.13% <24.13%> (ø)`
app/api/users.py	`31.87% <7.69%> (-0.39%)`	⬇️

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 6d87e0e...892a5ed. Read the comment docs.

shreyanshdwivedi · 2019-05-23T22:33:45Z

@iamareebjamal @uds5501 I've fixed the issue due to which the email was not getting restored while restoring the user. Also, I've added a migration file to update the email of past deleted users.
Please review

shreyanshdwivedi · 2019-05-26T23:07:53Z

@iamareebjamal @CosmicCoder96 please review this :)

app/api/helpers/user.py

app/api/users.py

shreyanshdwivedi · 2019-05-29T20:25:41Z

@iamareebjamal please take a look at the updated conditions and review the PR :)

app/api/helpers/user.py

shreyanshdwivedi · 2019-06-02T04:32:58Z

@iamareebjamal please review. I've updated the conditions

app/api/helpers/user.py

shreyanshdwivedi · 2019-06-03T03:58:12Z

@iamareebjamal removed the duplicate error message. Please review

uds5501

This looks good 👍

iamareebjamal · 2019-06-03T15:33:50Z

app/api/helpers/user.py

+    """
+    user_email = user.email
+    if user_email.endswith('.deleted'):
+        remove_str = '.deleted'


Move this statement a line above and replace '.deleted' with remove_str in if statement. That was the point of extracting it to a variable

shreyanshdwivedi · 2019-06-03T15:41:51Z

@iamareebjamal updated the PR. Please review

iamareebjamal · 2019-06-03T15:47:25Z

Build is failing

shreyanshdwivedi · 2019-06-03T15:48:52Z

@iamareebjamal this is some random failure and I don't understand the cause 😅

error: Hooks handler process 'dredd-hooks-python ./tests/hook_main.py' exited with status: 1
warn: Hook handling timed out.
error: Hooks handler process 'dredd-hooks-python ./tests/hook_main.py' exited with status: 1
info: Backend server process exited
The command "dredd" failed and exited with 1 during .

iamareebjamal · 2019-06-03T16:05:55Z

Multiple Migration Heads. Not a random error

uds5501 · 2019-06-03T16:13:37Z

@shreyanshdwivedi It failed again. OH. kindly change the revises of your migration file to the current head. :)

fixed hound issues updates the condition for restoring user Adds migration file to update email of all deleted users updates condition for modify_email_for_user_to_be_restored

shreyanshdwivedi · 2019-06-03T16:19:32Z

@uds5501 yep. Was fixing that only 😅
Good to know the real cause of such errors. Thanks @iamareebjamal
Let's wait for the test to pass

iamareebjamal · 2019-06-03T16:26:16Z

https://github.com/fossasia/open-event-server/blob/development/scripts/test_multiple_heads.sh is not working correctly. Open an issue

shreyanshdwivedi · 2019-06-03T17:13:10Z

@iamareebjamal tests passed. Please merge

houndci-bot reviewed May 18, 2019

View reviewed changes

app/api/helpers/user.py Show resolved Hide resolved

app/api/helpers/user.py Outdated Show resolved Hide resolved

app/api/helpers/user.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the deletedUser branch from 1d3d69c to 3cdd126 Compare May 18, 2019 11:56

iamareebjamal reviewed May 18, 2019

View reviewed changes

shreyanshdwivedi force-pushed the deletedUser branch from 858b1b0 to 9ee663f Compare May 18, 2019 16:20

fossasia deleted a comment May 18, 2019

shreyanshdwivedi force-pushed the deletedUser branch 3 times, most recently from 52a74da to fc2c4d8 Compare May 23, 2019 22:31

shreyanshdwivedi changed the title ~~[WIP] fix: allows deleted users to signup with same email~~ fix: allows deleted users to signup with same email May 23, 2019

auto-label bot added the fix label May 23, 2019

iamareebjamal reviewed May 27, 2019

View reviewed changes

app/api/helpers/user.py Outdated Show resolved Hide resolved

houndci-bot reviewed May 27, 2019

View reviewed changes

app/api/users.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the deletedUser branch from 1ea3e5e to 40b9e0c Compare May 27, 2019 04:43

shreyanshdwivedi force-pushed the deletedUser branch from 53fc927 to dbb4076 Compare May 29, 2019 20:35

fossasia deleted a comment May 29, 2019

iamareebjamal reviewed May 30, 2019

View reviewed changes

app/api/helpers/user.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the deletedUser branch 2 times, most recently from 4f105e6 to 143d995 Compare May 30, 2019 13:20

iamareebjamal reviewed Jun 2, 2019

View reviewed changes

app/api/helpers/user.py Outdated Show resolved Hide resolved

shreyanshdwivedi force-pushed the deletedUser branch from f6c437f to 85d39f6 Compare June 3, 2019 03:42

uds5501 previously approved these changes Jun 3, 2019

View reviewed changes

iamareebjamal reviewed Jun 3, 2019

View reviewed changes

shreyanshdwivedi dismissed uds5501’s stale review via 6c8199c June 3, 2019 15:39

shreyanshdwivedi force-pushed the deletedUser branch 2 times, most recently from 6c8199c to fb1bf51 Compare June 3, 2019 15:40

iamareebjamal previously approved these changes Jun 3, 2019

View reviewed changes

shreyanshdwivedi dismissed iamareebjamal’s stale review via 25c89d1 June 3, 2019 16:17

shreyanshdwivedi force-pushed the deletedUser branch from 215e473 to 25c89d1 Compare June 3, 2019 16:17

shreyanshdwivedi added 2 commits June 3, 2019 21:48

fix: allows deleted users to signup with same email

623cf37

fixed hound issues updates the condition for restoring user Adds migration file to update email of all deleted users updates condition for modify_email_for_user_to_be_restored

fixes multiple heads

25c89d1

Merge branch 'development' into deletedUser

892a5ed

iamareebjamal approved these changes Jun 3, 2019

View reviewed changes

iamareebjamal merged commit a93e784 into fossasia:development Jun 3, 2019

shreyanshdwivedi deleted the deletedUser branch June 14, 2019 08:36

fix: allows deleted users to signup with same email #5924

fix: allows deleted users to signup with same email #5924

Uh oh!

Conversation

shreyanshdwivedi commented May 18, 2019

Checklist

Short description of what this resolves:

Changes proposed in this pull request:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyanshdwivedi commented May 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 18, 2019

Codecov Report

Uh oh!

codecov bot commented May 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shreyanshdwivedi commented May 23, 2019

Uh oh!

shreyanshdwivedi commented May 26, 2019

Uh oh!

Uh oh!

Uh oh!

shreyanshdwivedi commented May 29, 2019

Uh oh!

Uh oh!

shreyanshdwivedi commented Jun 2, 2019

Uh oh!

Uh oh!

shreyanshdwivedi commented Jun 3, 2019

Uh oh!

uds5501 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyanshdwivedi commented Jun 3, 2019

Uh oh!

iamareebjamal commented Jun 3, 2019

Uh oh!

shreyanshdwivedi commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamareebjamal commented Jun 3, 2019

Uh oh!

uds5501 commented Jun 3, 2019

Uh oh!

shreyanshdwivedi commented Jun 3, 2019

Uh oh!

iamareebjamal commented Jun 3, 2019

Uh oh!

shreyanshdwivedi commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov bot commented May 18, 2019 •

edited

Loading

shreyanshdwivedi commented Jun 3, 2019 •

edited

Loading