Changing email to confirmed email should erase unconfirmed_email #2006

Closed
ronalchn opened this Issue Aug 5, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@ronalchn
Contributor

ronalchn commented Aug 5, 2012

When changing email to itself, devise does nothing, because it thinks there is nothing to change. However, it is better to erase unconfirmed email so that it doesn't confuse the user, if for example confirmation instructions are sent.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Aug 5, 2012

Owner

Sorry, I did not understand completely your report. In which screen are you changing the e-mail? Also, which devise version are you using? Thanks.

Owner

josevalim commented Aug 5, 2012

Sorry, I did not understand completely your report. In which screen are you changing the e-mail? Also, which devise version are you using? Thanks.

@ronalchn

This comment has been minimized.

Show comment Hide comment
@ronalchn

ronalchn Aug 5, 2012

Contributor

There is no screen to change email in devise. But I hooked up another screen to change the email, using a devise function:

resource.update_with_password(params[resource_name]), with params[resource_name] = {:email => "email", :current_password => "password"}

If "email" is the same as current email, nothing happens. If "email" is different, it gets put into resource.unconfirmed_email

However, if resource.unconfirmed_email already has another email from a previous change which was never confirmed, then if you then update the email, but using what is currently in resource.email, then unconfirmed_email should be cleared.


Let resource.email = "email1"
We change the email to email2, which causes resource.unconfirmed_email = "email2"

Now, we request another change, and change the email to email1.

However after this change, we still have resource = {:email => "email1", :unconfirmed_email => "email2"}.

Ideally, we should have resource = {:email => "email1", :unconfirmed_email => nil}, because there is no longer any email that needs to be confirmed.

Contributor

ronalchn commented Aug 5, 2012

There is no screen to change email in devise. But I hooked up another screen to change the email, using a devise function:

resource.update_with_password(params[resource_name]), with params[resource_name] = {:email => "email", :current_password => "password"}

If "email" is the same as current email, nothing happens. If "email" is different, it gets put into resource.unconfirmed_email

However, if resource.unconfirmed_email already has another email from a previous change which was never confirmed, then if you then update the email, but using what is currently in resource.email, then unconfirmed_email should be cleared.


Let resource.email = "email1"
We change the email to email2, which causes resource.unconfirmed_email = "email2"

Now, we request another change, and change the email to email1.

However after this change, we still have resource = {:email => "email1", :unconfirmed_email => "email2"}.

Ideally, we should have resource = {:email => "email1", :unconfirmed_email => nil}, because there is no longer any email that needs to be confirmed.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Aug 5, 2012

Owner

Perfect. Thanks for the detailed report, it makes sense, we will work on
fixing it. :)

Owner

josevalim commented Aug 5, 2012

Perfect. Thanks for the detailed report, it makes sense, we will work on
fixing it. :)

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Oct 28, 2012

Owner

@ronalchn I think this should be handled in your application and not Devise. Here is the thing, imagine another application contains a screen to edit the user, where you can change his name and also the e-mail. Now imagine that for both e-mail and name fields, we show the current valid name and e-mail values.

Now, if the user changes his e-mail, will set the unconfirmed e-mail, the screen will be reloaded, and we will still show the current e-mail on the field, because the unconfirmed one was not confirmed yet. Now, if the user changes his name, he will send the same current e-mail in the server, but he's still waiting for his unconfirmed e-mail and he doesn't want it to be cancelled as you proposed.

That said, this is coupled to the UI and we should not take an automatic measure. An application that has the e-mail field set to empty by default could afford to do the guessing as you proposed, but for some it would not. That said, I believe you should handle this scenario in your application.

Owner

josevalim commented Oct 28, 2012

@ronalchn I think this should be handled in your application and not Devise. Here is the thing, imagine another application contains a screen to edit the user, where you can change his name and also the e-mail. Now imagine that for both e-mail and name fields, we show the current valid name and e-mail values.

Now, if the user changes his e-mail, will set the unconfirmed e-mail, the screen will be reloaded, and we will still show the current e-mail on the field, because the unconfirmed one was not confirmed yet. Now, if the user changes his name, he will send the same current e-mail in the server, but he's still waiting for his unconfirmed e-mail and he doesn't want it to be cancelled as you proposed.

That said, this is coupled to the UI and we should not take an automatic measure. An application that has the e-mail field set to empty by default could afford to do the guessing as you proposed, but for some it would not. That said, I believe you should handle this scenario in your application.

@josevalim josevalim closed this Oct 28, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment