Skip to content
Snippets Groups Projects
  • ThibG's avatar
    5524258d
    Fix “Email changed” notification sometimes having wrong e-mail (#13475) · 5524258d
    ThibG authored
    * Fix “Email changed” notification sometimes having wrong e-mail
    
    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 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
    
    * Fix cases when sending notifications without changing the model
    
    * Defer sending if and only if in transaction including current record
    Fix “Email changed” notification sometimes having wrong e-mail (#13475)
    ThibG authored
    * Fix “Email changed” notification sometimes having wrong e-mail
    
    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 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
    
    * Fix cases when sending notifications without changing the model
    
    * Defer sending if and only if in transaction including current record