Can't send notification manually from ApplicationController #59

Closed
coorasse opened this Issue Feb 10, 2012 · 2 comments

Comments

Projects
None yet
3 participants

No exception_notifier.options in request.env, I have to set it manually.
I have to do something like that:

def log_error(exception)
if notifier = Rails.application.config.middleware.detect { |x| x.klass == ExceptionNotifier }
env = request.env
env['exception_notifier.options'] = notifier.args.first || {}
ExceptionNotifier::Notifier.exception_notification(env, exception).deliver
env['exception_notifier.delivered'] = true
end
end

I agree, @coorasse. Sending send exception notifications manually from ApplicationController, while possible, feels like it's more work than it needs to be.

What's more, it requires duplicating some of the code from the call(env) method in the ExceptionNotifier middleware (not DRY!).

This is what I currently have in my ApplicationController:

  rescue_from Exception,            with: :render_error

  def render_error(exception)
    if exception_notifier_instance = Rails.application.config.middleware.detect {|_| _.name == "ExceptionNotifier"}
      options = (request.env['exception_notifier.options'] ||= {})
      @options = exception_notifier_instance.args.first
      options.reverse_merge!(@options)
      begin
        # If log level is currently :debug, set to :info so it doesn't log this entire (really LONG) message body
        orig_level, Rails.logger.level = Rails.logger.level, Logger::INFO
        ExceptionNotifier::Notifier.exception_notification(request.env, exception).deliver
      ensure
        Rails.logger.level = orig_level
      end
    end
    logger.error("Error: #{exception}")
    render template: "/errors/500.html.haml", status: 500
  end

Pretty similar to what you had, @coorasse.

(I used rescue_from here so that I can render a dynamic 500 error page, but it's also useful when you want to rescue an exception in a controller action and still be notified of that exception (see #32).)

What I'd like is to be able to have something simple, cleaner, and more beautiful in my controller, like this:

  def render_error(exception)
    ExceptionNotifier.notify_of(exception, request).deliver
    logger.error("Error: #{exception}")
    render template: "/errors/500.html.haml", status: 500
  end

Why can't ExceptionNotifier::Notifier.exception_notification just know how to access the ExceptionNotifier configuration that I set in my initializer, so that we don't have to pass it in from the controller?

Is there a cleaner way to access that configuration data?

If not, I would recommend that _instead_ of passing the ExceptionNotifier configuration to the middleware, like this:

Whatever::Application.config.middleware.use ExceptionNotifier,
  :email_prefix => "[Whatever] ",
  :sender_address => %{"notifier" <notifier@example.com>},
  :exception_recipients => %w{exceptions@example.com}

, that we configure ExceptionNotifier similar to how we already configure Active Record and Action Controller — by setting config.active_record.whatever = 'whatever', etc.:

App::Application.configure do
  config.exception_notifier.email_prefix = "[Whatever] "
  config.exception_notifier.sender_address = %{"notifier" <notifier@example.com>}
  config.exception_notifier.exception_recipients = %w{exceptions@example.com}
  config.middleware.use ExceptionNotifier
end

or how Devise does it:

Devise.setup do |config|
  config.router_name = :main_app

  # ==> Mailer Configuration
  # Configure the e-mail address which will be shown in DeviseMailer.
  config.mailer_sender = 'exceptions@example.com'

  # ...
end

Why? Because the middleware isn't the only place that needs access to this ExceptionNotifier configuration. In my case (and, I gather, @coorasse's case), I don't even _use_ the ExceptionNotifier middleware at all!

I only use ExceptionNotifier by manually calling the Notifier... except in rare cases where an exception is raised that isn't a subclass of Exception, or my render_error method itself raises an exception...

Collaborator

jweslley commented Apr 20, 2013

Hi guys, since current exception_notification's version already support this I'm closing this issue.

For more info:
https://github.com/smartinez87/exception_notification/tree/v3.0.1#manually-notify-of-exception

jweslley closed this Apr 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment