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

(not working yet) Change deliver_now to deliver_later for all emails #5311

Closed
2 of 6 tasks
jywarren opened this issue Mar 30, 2019 · 12 comments · Fixed by #5533 or #5451
Closed
2 of 6 tasks

(not working yet) Change deliver_now to deliver_later for all emails #5311

jywarren opened this issue Mar 30, 2019 · 12 comments · Fixed by #5533 or #5451
Labels
fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet help wanted requires help by anyone willing to contribute multiple-use Ruby

Comments

@jywarren
Copy link
Member

jywarren commented Mar 30, 2019

We just successfully converted the admin_controller to use deliver_later to enqueue email sending, which should really help keep the website running smoother -- it uses Redis automatically to send emails on another thread.

https://github.com/publiclab/plots2/pull/4967/files shows how we did it -- it did involve wrapping all failing tests with:

perform_enqueued_jobs do 
  # all test contents
end

Let's do this for each controller, and each time we use deliver_now -- you can find all instances using this search:

https://github.com/publiclab/plots2/search?q=deliver_now

We'd love help with this, and it's fine for it to be over a series of pull requests, one at a time!

  • users_controller (@divyabaid16)
  • answer_like_controller(@AnthoniaOkafor)
  • notes_controller
  • user_sessions_controller
  • answers_controller
  • admin_controller
@jywarren jywarren added help wanted requires help by anyone willing to contribute Ruby fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet multiple-use labels Mar 30, 2019
@divyabaid16
Copy link
Contributor

Can I work on this?

@namangupta01
Copy link
Member

namangupta01 commented Apr 1, 2019 via email

@divyabaid16
Copy link
Contributor

Hi! I had a small confusion. I wanted to know whether I should change it for each controller only?
Or do I need to change the test so that the tests do not fail?

@AnthoniaOkafor
Copy link
Contributor

@jywarren @namangupta01 please can I have an fto_candidate issue on Ruby. Thanks

@divyabaid16
Copy link
Contributor

@AnthoniaOkafor if you want you can solve this issue for any controller.
Thanks!

@AnthoniaOkafor
Copy link
Contributor

Ok, I will. Thanks

@jywarren
Copy link
Member Author

jywarren commented Apr 30, 2019 via email

@jywarren
Copy link
Member Author

jywarren commented May 8, 2019

OK, we are seeing these deliver_later pile up in sidekiq!

https://publiclab.org/sidekiq/queues/mailers

image

@jywarren jywarren changed the title Change deliver_now to deliver_later for all emails (not working yet) Change deliver_now to deliver_later for all emails May 8, 2019
@jywarren
Copy link
Member Author

jywarren commented May 8, 2019

So, I think this may actually be all good, but just we're never actually triggering the job.

https://api.rubyonrails.org/classes/ActionMailer/MessageDelivery.html#method-i-deliver_later

OK, more on this here: https://stackoverflow.com/a/51642031

We need: config.active_job.queue_adapter = :sidekiq

https://gist.github.com/maxivak/690e6c353f65a86a4af9#configure-sidekiq

I'm not sure if config.action_mailer.perform_deliveries = true is necessary...

Trying the rest, meaning also a config/sidekiq.yml in #5687

@jywarren
Copy link
Member Author

jywarren commented May 8, 2019

If it passes, i'll merge into stable, and we can publish along with the big Bootstrap upgrade; we'll see how it does in production, since these emails are not critical!

@jywarren
Copy link
Member Author

jywarren commented May 8, 2019

@jywarren
Copy link
Member Author

Confirmed! With the code publication we did tonight, this deliver_later system is now working! We can start the rest of the controller conversions now. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fto-candidate issues which are meant to be solved by first timers but aren't well-formatted yet help wanted requires help by anyone willing to contribute multiple-use Ruby
Projects
None yet
5 participants