OptionalTarget exceptions are wrongly silenced and error hash can't be accessed

[markedmondson]

Steps to reproduce

Create an optional target that raises an exception (bad code, missing method, anything)
Call Notification#notify_to with the optional target enabled

Expected behavior

I'd expect to get an exception in all honesty, but #103 implemented a handler that logs the errors.
The problem is, from Notification#notify_to or Notification#notify, you can't get at those returned values, they're swallowed in

activity_notification/lib/activity_notification/apis/notification_api.rb

Line 355 in 4148f8c

notification.publish_to_optional_targets(options[:optional_targets] || {})

Actual behavior

Optional target exceptions are silently swallowed.

Realistically, people should be capturing their own exceptions around calls to Notification#notify, the OptionalTarget classes are implemented manually and are in a better place to handle their own exceptions, it's not the responsibility of the gem to silently fail.

System configuration

activity_notification gem version: 2.2.1
Rails version: 6.0.3.4
ORM (ActiveRecord, Mongoid or Dynamoid): ActiveRecord

[simukappu]

Thank you for your feedback and contribution @markedmondson !

I totally agree with you that optional target implementation should handle their own exceptions. However, I guess many users are using Notification#notify_later as async call rather than Notification#notify or Notification#notify_to. Then application cannot rescue errors thrown in optional target implementation.
We should handle the issue #103 in pre-built optional target like Slack notification. Sending notifications to other targets should not be stoped after one target fails.
What do you think about it?

[markedmondson]

Oh interesting, I hadn't considered notify_later.

I still don't know that the application should silently capture all exceptions though, it's far too easy to miss something important, especially if people aren't checking the return values. For example, a runtime error is probably as a result of something that's going to cause all the subsequent calls to fail too... For example, in the pre-built slack target, what if a configuration value is missing?

[simukappu]

Notification#notify or Notification#notify_to works as same as Notification#notify_later when notify_later option is true. This async call enables you to separate notification process from user interaction in your application.

Yes, I agree with you about exception handling. Your PR looks good, but simple. We also have to consider async call and partial success that we covered from issue #103. One option is to add configuration to activity_notification gem that enables you to select whether ignore exceptions in optional targets or not.
I appreciate it if you have any other good idea. Thank you so much for your contribution!

[simukappu]

I've added rescue_optional_target_errors config option. Now you can choose internal rescue or throwing capturable exception as your configuration. Default value will be true for backward compatibility.

[simukappu]

We've published as v2.2.3.