heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24k stars 5.55k forks source link

Active Job integration clarification #5672

Open olivier-thatch opened 9 months ago

olivier-thatch commented 9 months ago

Hi,

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

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?

bdewater-thatch commented 9 months ago

👋 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.