Active Job integration clarification

[olivier-thatch]

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:

  devise/README.md

  Lines 694 to 704 in e2242a9

  	### Active Job Integration
  	If you are using Active Job to deliver Action Mailer messages in the
  	background through a queuing back-end, you can send Devise emails through your
  	existing queue by overriding the `send_devise_notification` method in your model.
  	```ruby
  	def send_devise_notification(notification, *args)
  	devise_mailer.send(notification, self, *args).deliver_later
  	end
  	```

• however, source code comments suggest a much more complex implementation:

  devise/lib/devise/models/authenticatable.rb

  Lines 137 to 208 in 1d66580

  	# This is an internal method called every time Devise needs
  	# to send a notification/mail. This can be overridden if you
  	# need to customize the e-mail delivery logic. For instance,
  	# if you are using a queue to deliver e-mails (active job, delayed
  	# job, sidekiq, resque, etc), you must add the delivery to the queue
  	# just after the transaction was committed. To achieve this,
  	# you can override send_devise_notification to store the
  	# deliveries until the after_commit callback is triggered.
  	#
  	# The following example uses Active Job's `deliver_later` :
  	#
  	# class User
  	# devise :database_authenticatable, :confirmable
  	#
  	# after_commit :send_pending_devise_notifications
  	#
  	# protected
  	#
  	# def send_devise_notification(notification, *args)
  	# # If the record is new or changed then delay the
  	# # delivery until the after_commit callback otherwise
  	# # send now because after_commit will not be called.
  	# # For Rails < 6 use `changed?` instead of `saved_changes?`.
  	# if new_record? || saved_changes?
  	# pending_devise_notifications << [notification, args]
  	# else
  	# render_and_send_devise_message(notification, *args)
  	# end
  	# end
  	#
  	# private
  	#
  	# def send_pending_devise_notifications
  	# pending_devise_notifications.each do |notification, args|
  	# render_and_send_devise_message(notification, *args)
  	# end
  	#
  	# # Empty the pending notifications array because the
  	# # after_commit hook can be called multiple times which
  	# # could cause multiple emails to be sent.
  	# pending_devise_notifications.clear
  	# end
  	#
  	# def pending_devise_notifications
  	# @pending_devise_notifications ||= []
  	# end
  	#
  	# def render_and_send_devise_message(notification, *args)
  	# message = devise_mailer.send(notification, self, *args)
  	#
  	# # Deliver later with Active Job's `deliver_later`
  	# if message.respond_to?(:deliver_later)
  	# message.deliver_later
  	# # Remove once we move to Rails 4.2+ only, as `deliver` is deprecated.
  	# elsif message.respond_to?(:deliver_now)
  	# message.deliver_now
  	# else
  	# message.deliver
  	# end
  	# end
  	#
  	# end
  	#
  	def send_devise_notification(notification, *args)
  	message = devise_mailer.send(notification, self, *args)
  	# Remove once we move to Rails 4.2+ only.
  	if message.respond_to?(:deliver_now)
  	message.deliver_now
  	else
  	message.deliver
  	end
  	end

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]

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

18a18e4 (Jun 16, 2012) added the send_devise_notification hook. The latest Rails version was 3.2 at the time.

c25312e (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, 2e442d8 (May 12, 2016) removed the deprecation from the send_devise_notification hook example that was introduced in Rails 4.2, and c9a2d06 (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 #5610 would make this more intuitive to configure, and hopefully enable a switch of the default to deliver_later in the next major release.