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

Refactor NotificationMailer to use parameterization #25718

Merged
merged 12 commits into from
Jul 10, 2023

Conversation

mjankowski
Copy link
Contributor

These methods were all pretty similar with a lot of common setup, so I extracted that into a parameterized setup using with when the mailer is called and before_actions in the mailer to set i-vars.

Also did some readability refactor in the spec and preview while in there.

If diff seems excessive - going one commit at a time is probably more sensical.

I think the Admin and User mailers both have similar opportunity and will do those after this if merged.

@renchap renchap added refactoring Improving code quality ruby Pull requests that update Ruby code labels Jul 6, 2023
@Gargron Gargron merged commit f3fca78 into mastodon:main Jul 10, 2023
26 checks passed
@mjankowski mjankowski deleted the mailer-params branch July 10, 2023 11:48
@ClearlyClaire
Copy link
Contributor

Doesn't it break jobs queued after the code change?

@mjankowski
Copy link
Contributor Author

I'm not sure! My original thought was that it would be ok because we didn't explicitly change any job method signatures ... but here's more research...

Rails 7-0 branch, regular MessageDelivery enqueuing delivery -- https://github.com/rails/rails/blob/7-0-stable/actionmailer/lib/action_mailer/message_delivery.rb#L143

Same thing but in the parameterized class -- https://github.com/rails/rails/blob/7-0-stable/actionmailer/lib/action_mailer/parameterized.rb#L144

The parameterized version calls perform_later with an extra params arg, which would mean:

  • This value would be absent in any jobs sitting in queue to be retried, and thus not passed in to new code once reload/restart happens
  • Old jobs in queue would have MORE items in args than the new code expects, which might also trip it up

So, I think maybe it is an issue. Is there typically a large backlog of mailer jobs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improving code quality ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants