devise icon indicating copy to clipboard operation
devise copied to clipboard

Active Job integration clarification

Open olivier-thatch opened this issue 1 year ago • 1 comments

Hi,

I was looking into using Active Job to deliver Devise messages, and I am a bit confused:

  • the README suggests a very simple implementation: https://github.com/heartcombo/devise/blob/e2242a95f3bb2e68ec0e9a064238ff7af6429545/README.md#L694-L704

  • however, source code comments suggest a much more complex implementation: https://github.com/heartcombo/devise/blob/1d6658097e364d45b5e059976f1e822eee7d67da/lib/devise/models/authenticatable.rb#L137-L208

AFAICT, the simple implementation works perfectly fine (at least in our codebase).

Is the complex implementation a leftover from older times? Or are there actually any cases where Devise would try to enqueue messages before the record is persisted to the DB?

olivier-thatch avatar Feb 07 '24 19:02 olivier-thatch

👋 I've been working with Olivier on this and was doing some code archeology from discussion on our internal PR. To help answer his question I'm pretty sure this is a leftover from older times, which turned into a little bit of parallel development. The timeline I put together:

https://github.com/heartcombo/devise/commit/18a18e4c726cd88d031fb7ed53e88fb063b366a6 (Jun 16, 2012) added the send_devise_notification hook. The latest Rails version was 3.2 at the time.

https://github.com/heartcombo/devise/commit/c25312e78ec413f2edb0f50bc03e3472c328d862 (Sep 2, 2014) added the section to the readme, which hasn't changed since. This was just before the release of Rails 4.2 on December 19, 2014 which included Active Job and ActionMailer#deliver_later.

Meanwhile, https://github.com/heartcombo/devise/commit/2e442d81f728d825f712f5ab4140c5ee98681009 (May 12, 2016) removed the deprecation from the send_devise_notification hook example that was introduced in Rails 4.2, and https://github.com/heartcombo/devise/commit/c9a2d0654e9fc1aaebe6f99ef6fbc55c55a91fdd (Mar 23, 2018) changed the example to use deliver_later.

tl;dr I think it's fine to remove the send_devise_notification hook example (or change it to match the readme example). FWIW https://github.com/heartcombo/devise/pull/5610 would make this more intuitive to configure, and hopefully enable a switch of the default to deliver_later in the next major release.

bdewater-thatch avatar Feb 07 '24 20:02 bdewater-thatch