mhfs/devise-async — pull request #52 by mrnugget (2014-06-25 11:32:33+00:00)
Title: Always send notifications after commit if possible

This changes the behaviour of Devise::Async::Model to send devise notifications after commits instead of right away when the resource has not changed and `after_commit` is available.

The reason for that is Devise's behaviour when generating new confirmation tokens: Devise generates a new token and calls `#save` on the model. This marks the model as clean and `model.changed?` returns
false. This can happen inside a transaction.

The problem: if the Devise::Async worker is enqueued inside the transaction, because `model#changed?` returns false, the transaction is not committed and the changes are not visible to the worker yet.

This leads to a race condition where the worker might not see the changes if it's fast enough.

I've experienced this behaviour on our production server, where we are using sidekiq as background processor. Sidekiq, as it turns out, is pretty fast and sometimes sends emails with confirmation instructions with empty `confirmation_tokens`, since it cannot see the `confirmation_token` yet, since the transaction is not committed yet.

Check out these two files of the Devise source code (including Devise 2.2 here, since that's what we're using in production, where I stumbled upon the error):
- [confirmable.rb in Devise 2.2](https://github.com/plataformatec/devise/blob/v2.2/lib/devise/models/confirmable.rb#L214-L216)
- [confirmable.rb in Devise master](https://github.com/plataformatec/devise/blob/master/lib/devise/models/confirmable.rb#L227-L229)

And consider this:

```ruby
User.transaction do
user.firstname = 'newname'
puts user.changed? # => true
user.save!
puts user.changed? # => false
end
```

My reasoning behind the changes I did: it does not hurt to use the "pending notifications queue" instead of sending it right way, since both methods (the hook and the `send_devise_notification` method) are called anyway. And using the `after_commit` hook seems a lot safer to me than passing the resource to the worker right away.

What do you think?