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

Corner case handling for non existing email address #601

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spuetz
Copy link

@spuetz spuetz commented Feb 19, 2020

Special handling if there is no corresponding email address object for the EmailAddress model,
this could happen if the rest_auth app has been installed after some users had already been created.

@coveralls
Copy link

coveralls commented Feb 19, 2020

Coverage Status

Coverage decreased (-0.3%) to 95.788% when pulling a68fb24 on weitblicker:master into 624ad01 on Tivix:master.

@spuetz spuetz changed the title Corner case handling for non existing email adress Corner case handling for non existing email address Feb 19, 2020
Special handling if there is no corresponding email address object for the EmailAddress model,
this could happen if the rest_auth app has been installed after some users had already been created.
@mariodev
Copy link
Contributor

mariodev commented Feb 20, 2020

As convenient as this method is, I don't think, that marking emails as verified should be DRA default behaviour.
It should be the developers responsibility (imho), to either:

  • send confirmation e-mails to those users, or
  • create a custom one-time script to automatically mark them as verified (so doing similar as above code, but outside of the DRA lib)

@spuetz
Copy link
Author

spuetz commented Feb 20, 2020

But then you just argue against the default value of verified, this could be set to false. Just creating missing instances of the EmailAddress object. Thereby, your two arguments are still valid also with this PR. We also could add a parameter for that or simply use verified=False. But the current state just breaks and raises this error, which should not happen, right? ;)

@spuetz
Copy link
Author

spuetz commented Apr 28, 2020

@mariodev @aasimmk any news?

@BarnabasSzabolcs
Copy link

Hi,
as a user of rest-auth, thanks for the contribution!
This repo is not maintained anymore, so the development moved to dj-rest-auth. (reference: #568)
I think it is best, if you move this PR there.

new repo link: https://github.com/jazzband/dj-rest-auth (I'm not the upkeeper of that repo, it just makes sense for me to help you merge your PR)

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.

5 participants