Background notifications could be DRYer and more complete #32

Closed
jjb opened this Issue Nov 4, 2011 · 7 comments

Comments

Projects
None yet
4 participants

jjb commented Nov 4, 2011

Hi — thanks for reviving this project!

So I'm potentially migrating away from Exceptional, which has Exceptional::Catcher.handle_with_controller and Exceptional.handle, which are analogs to ExceptionNotifier::Notifier.exception_notification and ExceptionNotifier::Notifier.background_exception_notification.

a few things about ExceptionNotifier::Notifier.background_exception_notification

  1. it's sort of a second class citizen — it doesn't allow for custom templates
  2. it has repeated code from ExceptionNotifier::Notifier.exception_notification
  3. it's the only thing that can be appropriately used in a model (right?), so it's sort of misnamed

Maybe I'm wrong on 3, and I should just be using ExceptionNotifier::Notifier.exception_notification with an empty environment hash?

Let me know what you think — I'd be interested in cleaning it up, so that there is some sort of common method that is called in all contexts, doesn't repeat code, and reuses the templates.

Owner

smartinez87 commented Nov 4, 2011

Hey,
not sure if I quite understood you, exceptions raised on a model are catched up by the gem without having to manually call exception_notification.
background_exception_notification is just for code that can't be reached within a request, like background tasks.

On the other hand, you're right and that piece of code is claiming for a refactor, so you're welcome to do so!

jjb commented Nov 4, 2011

For example a model might access a third-party API or the filesystem, and I'd want to do something like this:

 begin
   # access the api
 rescue
   ExceptionNotifier::Notifier.background_exception_notification($!)
 end

jjb commented Nov 12, 2011

@smartinez87 what do you think? Am I doin' it wrong?

Owner

smartinez87 commented Nov 12, 2011

sorry but I still don't understand...
say you have a a controller with something like this:

def some_action
  @user = User.find(params[:id])
  @user.my_model_method
end

and on the model

def my_model_action
  access the api
  update_some_data
end

Now, if something goes wrong there, it will automatically make the ExceptionNotification act and send you an email with the exception raised. Why should you manually invoke it?

jjb commented Nov 12, 2011

Ah, whoops, I see that my example wasn't very good at all :-D. Here's something better:

def my_controller_action
  begin
    FooBarService.api(params[:stuff])
    @message_to_user = "It worked! Your FooBar has been connected!"
  rescue
    ExceptionNotifier::Notifier.background_exception_notification($!)
    @message_to_user =
      "The FooBar service is not available. Please try again later."
  ensure
    render 'remote_thingy_results'
  end
end

jjb commented Nov 12, 2011

(I suppose one might say that this should be caught in a library and not in the controller -- even in that case I like to use ExceptionNotifier for logging the exceptions)

@jjb: It sounds like what you're trying to do is rescue an exception in your controller and still be able to use ExceptionNotifier to be notified of that exception that you rescued. I like to use ExceptionNotifier in this way too.

@smartinez87, the reason he has to manually invoke ExceptionNotifier is because he rescued the exception in his controller. The ExceptionNotifier middleware is only able to intercept and send notifications for exceptions that are _not_ rescued, because they have to bubble all the way up the middleware stack to ExceptionNotifier for it to become aware of them.

@jjb, I haven't used background_exception_notification before, but it sounds like in this case you could actually use the normal ExceptionNotifier::Notifier.exception_notification method instead, as mentioned in the "Manually notify of exception" section of the Readme. Also see how I'm doing it in #59.

You only need to use background_exception_notification if you _don't_ have access to request.env.

jweslley closed this Apr 20, 2013

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