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

Fix “Email changed” notification sometimes having wrong e-mail #13475

Merged

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented Apr 14, 2020

Fixes #6778

The root of the issue is that send_devise_notification was called before
the changes were properly commited to the database, causing the mailer to
pick previous values if running too early.

Devise's documentation provides guidance on how to handle 1 2, however,
I have found it to not be working, as the following happens, in that order:

  • send_devise_notification is called for the email_changed notification.
    In that case, changed? is false and saved_changes? is true, so
    if we use the former, we have the same issue.
  • the after_commit hook is called
  • send_devise_notification is called for the confirmation_instructions
    notification.
    In that case, changed? is still false, and saved_changes? still true,
    so if we use the latter, that second notification email is simply not
    going to be sent.

This may arguably be a bug in Devise or Rails, but I haven't dug further
and instead opted for the following workaround: just postpone calling the
mailer by a few seconds
.

EDIT: I ended up following Devise's documentation, except instead of relying on changed? or saved_changes?, which can't differentiate between a committed save and an uncommitted save, checking whether a transaction is open. This correctly queues the notification from after_update and immediately sends the one from after_commit.

@ClearlyClaire
Copy link
Contributor Author

Looking at Devise's source code, the email_changed notification is sent in an after_update hook, so after save but before the transaction is committed, and the confirmation_instructions is sent in an after_commit hook. This explains why changed? would be false (the save did occur) and saved_changes? would be true (last save did save changes). I'm not sure how we could reliably tell whether the changes were committed though, so for now my best solution is still to delay running the mailer :/

Fixes mastodon#6778

The root of the issue is that `send_devise_notification` was called before
the changes were properly commited to the database, causing the mailer to
pick previous values if running too early.

Devise's documentation provides guidance on how to handle that[1][2], however,
I have found it to not be working, as the following happens, in that order:
- `send_devise_notification` is called for the `email_changed` notification.
  In that case, `changed?` is false and `saved_changes?` is true, so
  if we use the former, we have the same issue.
- the `after_commit` hook is called
- `send_devise_notification` is called for the `confirmation_instructions`
  notification.
  In that case, `changed?` is still false, and `saved_changes?` still true,
  so if we use the latter, that second notification email is simply not
  going to be sent (as we would be queuing the notification *after*
  executing the after_commit hook).

This is because it may be called from either an `after_update` or
`after_commit` hook, the difference not being a call to `save` but the
transaction actually being committed to the database. This may arguably
be a bug in Devise, or Devise's notification.

The proposed workaround is inspired by Devise's documentation but checks
whether a transaction is open to make the call whether to immediately
send the notification or defer it to the `after_commit` hook.

[1]: https://www.rubydoc.info/github/plataformatec/devise/Devise%2FModels%2FAuthenticatable:send_devise_notification
[2]: https://github.com/heartcombo/devise/blob/406915cb781e38255a30ad2a0609e33952b9ec50/lib/devise/models/authenticatable.rb#L133-L194
@ClearlyClaire ClearlyClaire force-pushed the fixes/devise-mailer-notifications branch from e920f16 to bc73536 Compare April 15, 2020 11:15
@ClearlyClaire
Copy link
Contributor Author

Found a better way and updated the PR accordingly!

@ClearlyClaire ClearlyClaire requested a review from ykzts April 15, 2020 11:24
@ClearlyClaire ClearlyClaire force-pushed the fixes/devise-mailer-notifications branch from a81dbdf to 00ed8f0 Compare April 15, 2020 11:49
@ClearlyClaire ClearlyClaire force-pushed the fixes/devise-mailer-notifications branch from 00ed8f0 to 68081aa Compare April 15, 2020 12:05
@ClearlyClaire ClearlyClaire force-pushed the fixes/devise-mailer-notifications branch from 18d0f7b to 203fe0b Compare April 15, 2020 12:38
@ClearlyClaire ClearlyClaire force-pushed the fixes/devise-mailer-notifications branch from 203fe0b to 6dd7d25 Compare April 15, 2020 12:40
@ClearlyClaire
Copy link
Contributor Author

An update on the last changes I made: I figured send_devise_notification could be called when not creating/changing the user struct (e.g., re-sending the confirmation e-mail), and worse, it could be called when not changing it, but still being in a transaction (as is the case of one of the tests, apparently).

Therefore, I changed it to get queued when the record itself is in a pending transaction. I have tested this does save the issue at end, and it should not add any delay, and I guess the test could possibly be too strict (inclusion, instead of checking a record of that id exists), in which case it could possibly perform as badly as before, but not worse.

@Gargron Gargron merged commit 5524258 into mastodon:master Apr 15, 2020
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.

Mastodon "Email changed" email does not show the new email address
3 participants