Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ActionMailer plugin to add rescue_from to the correct class #1143

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

waltjones
Copy link
Contributor

@waltjones waltjones commented Dec 27, 2023

Description of the change

This PR has two fixes.

  • The ActiveJob plugin is now a true plugin, with all of the methods and capabilities that should be present. It now loads at the correct load time, and can be disabled from the Rollbar config. d3e0029
  • The rescue_from handler is added to the correct base class. 4d4f391

This PR also refactors the tests to test Rails 6+, which previously didn't get coverage that included the mailer class. Where possible, the tests are now in shared examples called by both the old and new contexts.

Background facts about rescue_from handlers.

  • These have precedence in the order they are added, with the last added having the highest precedence.
  • Only one handler is called for an exception. Handlers are not chained or cascaded.
  • The first handler in order of precedence that matches the exception class is the one called.

When MailDeliveryJob was added in Rails 6.x, it included a rescue_from handler that calls the rescue_from of the Mailer class.

When the Rollbar ActiveJob plugin installs its handler on MailDeliveryJob, it is added after the one in the MailDeliveryJob class, and has precedence. This causes the intended handler to never get called, and therefore never pass the error to the Mailer class.

ActionMailer::Base is the base class for all Mailers, and this is the correct place to add the Rollbar handler. With this fix, the application's handler will be used if it matches the exception class, and if not, the Rollbar handler is called.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Related issues

Fixes #1126

Copy link

@mudetroit mudetroit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, thanks for the description in the PR help clear up the context

@waltjones waltjones merged commit dc2a396 into master Jan 3, 2024
25 checks passed
def self.included(base)
base.send :rescue_from, Exception do |exception|
args = if self.class.respond_to?(:log_arguments?) && !self.class.log_arguments?
arguments.map(&Rollbar::Scrubbers.method(:scrub_value))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @waltjones, I noticed that the rollbar update to 3.5.0 broke one of the mailers of our app and tracked it down here: ActionMailer::Base doesn't have an arguments method and NameError: undefined local variable or method 'arguments' for #<YourMailerClass:0x0000000036ada8>> here

I assume this was fixed on the tests here with the attr_accessor :arguments on the TestMailer, but that doesn't happen on a regular rails app

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasmazza Thank you for the report. I'm working to get a fix out today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else
arguments
end
Rollbar.plugins.define('active_job') do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 - this change produces:

NameError: uninitialized constant Rollbar::ActiveJob (NameError)

  include Rollbar::ActiveJob
                 ^^^^^^^^^^^

which means that https://docs.rollbar.com/docs/activejob-integration is no longer true. Is there a change required to the documentation?

https://docs.rollbar.com/docs/gem-plugins is unclear on how plugins are loaded, so we were unable to experiment with a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using rescue_from in ApplicationMailer has unexpected results under rails 6 and rollbar 3.4.0
4 participants