DRY out AuthorizationsController#render_success

[kmayer]

In our own deployment, we have some custom exception handling around the create method. By calling the more "public" create method in render_success, we get to have a single code path that also supports skip_authorization? and can_authorize_response?. The code on L35 is identical to L16. That suggests identical intent, but not always. This might be coincidental, in which case, the pull request should be declined.

[nbulaj]

Can we better extract redirect_or_render(authorize_response) into a method like respond_with_authorization or similar to avoid action calling inside another methods/actions?

[kmayer]

Possibly, but redirect_or_render is also another private in the same controller, does not live anywhere else and calls controller-owned methods ... I don't have any better ideas for where a public method would get defined? Calling create seems public enough behavior?

For our purposes, we created our own AuthorizationsController class that inherits from Doorkeeper, modified the routes accordingly and then overrode create:

module Ours
  class AuthorizationsController < Doorkeeper::AuthorizationsController
    # Override app/controllers/doorkeeper/authorizations_controller.rb

    # Override the #create method
    # To catch and render errors for the ActiveRecord validation
    def create
      super # <= This is why we're proposing this pull request
    rescue ActiveRecord::RecordInvalid => exception
      case exception.message
      when /Validation failed: Tenant has already been taken/
        pre_auth.error = Ours::Errors::DuplicateAuthorization
        redirect_or_render(pre_auth.error_response)
      else
        raise exception
      end
    end
  end
end

[kmayer]

@nbulaj Any more thoughts on this? We've been running this on a private branch for 3 weeks and haven't encountered any adverse side effects.