devise: Devise::RegistrationsController#create and #update only yield to block on success

Howdy all,

While making use of the excellent functionality introduced by #2728 I found what I think to be a bug/incomplete functionality.

in Devise::RegistrationsController#create at line 16 and Devise::RegistrationsController#update at line 45 the resource is only yielded to the provided block upon successful creation/update and not on failures.

In my specific use-case I need to capture failures in order to register events with my analytics engines. This is currently impossible without completely replacing the method as far as I can tell which is what #2728 was trying to fix.

I think fixing it may be as simple as doing something like this:

def create
  build_resource(sign_up_params)

  saved = resource.save # Save and store result
  yield resource if block_given? # Yield always, block can check resource.persisted? to detect success or failure
  if saved
    if resource.active_for_authentication?
      set_flash_message :notice, :signed_up if is_flashing_format?
      sign_up(resource_name, resource)
      respond_with resource, location: after_sign_up_path_for(resource)
    else
      set_flash_message :notice, :"signed_up_but_#{resource.inactive_message}" if is_flashing_format?
      expire_data_after_sign_in!
      respond_with resource, location: after_inactive_sign_up_path_for(resource)
    end
  else
    clean_up_passwords resource
    respond_with resource
  end
end

Does this seem reasonable? If so I will start working on a pull request.

About this issue

Original URL
State: closed
Created 10 years ago
Reactions: 1
Comments: 16 (12 by maintainers)

Most upvoted comments

@krnjn Good use case.

@josevalim I realize this is a much, much larger idea, but for a long time I’ve wished that devise would decouple all this logic from the controllers and into a service layer so the specific functionality of creating accounts, updating accounts, initiating password resets, etc. would be isolated from controllers. This would make it much easier for people to slice/dice/combine the functionality needed and allow access to it from things like Rake tasks, service objects, custom controllers, etc. Do you have any thoughts on that?

I’d much rather see the Devise::RegistrationsController#create be simplified to look like:
def create
  result = Device::RegistrationService.create(params)
  if result.success?
    ...
  else
    ...
   end
end
Immediate use-cases that come to mind:

For managing users from Rake tasks

For managing users from backend admin areas like ActiveAdmin

For use in other service objects that coordinate user management with other components

For use in API controllers

This is such a great idea

dowi on Sep 23, 2019

@krnjn Good use case.

@josevalim I realize this is a much, much larger idea, but for a long time I’ve wished that devise would decouple all this logic from the controllers and into a service layer so the specific functionality of creating accounts, updating accounts, initiating password resets, etc. would be isolated from controllers. This would make it much easier for people to slice/dice/combine the functionality needed and allow access to it from things like Rake tasks, service objects, custom controllers, etc. Do you have any thoughts on that?

I’d much rather see the Devise::RegistrationsController#create be simplified to look like:

def create
  result = Device::RegistrationService.create(params)
  if result.success?
    ...
  else
    ...
   end
end

Immediate use-cases that come to mind:

For managing users from Rake tasks
For managing users from backend admin areas like ActiveAdmin
For use in other service objects that coordinate user management with other components
For use in API controllers

dpehrson on Mar 7, 2015