Deprecate queuing PD application mailers and send all right away by megcrenshaw · Pull Request #51411 · code-dot-org/code-dot-org

megcrenshaw · 2023-04-19T17:15:01Z

As part of the refactor to clean up cron jobs 0a78244, the cron job was running Pd::Application::Email.send_all_queued_emails. That line was removed, as the earlier parts of the cron job were no longer queuing emails, but rather sending them right away.

This change introduced a bug (thread) in which queued emails from other parts of our system –– namely the decision emails –– were not being sent at all. I put the line back in in this PR #51201.

At this point, only one type of email –– the decision email –– does not get sent right away:

code-dot-org/dashboard/app/models/pd/application/teacher_application.rb

Line 389 in de87cfc

queue_email(status) if should_send_decision_email?

Since that's the only place that is not using the deliver_now: true param, I got confirmation that these emails can also be sent right away.

This PR does the following:

Refactors the functionality so that deliver_now is no longer a param, but rather the only option. All emails will get sent right away for PD applications.
Now that all emails get sent right away, there is no email queue, so removes all references to "queuing" an email and "unsent" emails for PD Applications
Moves functionality to send "decision" emails (accepted and declined) from the model to the controller to avoid callbacks interfering with each other. These unit tests moved from the model test to the controller test, and certain other tests needed to change to trigger sending the email since .update(status: accepted) no longer sends the email.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

dmcavoy · 2023-04-20T15:17:43Z

Thank you Meg for driving for this change! Excited to see us making this system better for us and the users

davidsbailey · 2023-04-20T18:45:33Z

+1 Meg, very exciting to see this simplification!

pre-review question: do we send nag mail for incomplete applications, and how does that fit into the picture here? I thought maybe we did, but all I am seeing at first glance is that we show a banner on the homepage if they have an incomplete application.

megcrenshaw · 2023-04-20T18:49:58Z

+1 Meg, very exciting to see this simplification!

pre-review question: do we send nag mail for incomplete applications, and how does that fit into the picture here? I thought maybe we did, but all I am seeing at first glance is that we show a banner on the homepage if they have an incomplete application.

What does "nag mail" mean? For incomplete applications, we only send reminders to send in their applications. That logic happens in CompleteApplicationReminder

UPDATE: oh "nag" like we're nagging them? I think that's the CompleteApplicationReminder –– happens with a cron job and sends one reminder at 7 days without an update and a second reminder at 14 days without an update

davidsbailey · 2023-04-20T18:52:45Z

+1 Meg, very exciting to see this simplification!
pre-review question: do we send nag mail for incomplete applications, and how does that fit into the picture here? I thought maybe we did, but all I am seeing at first glance is that we show a banner on the homepage if they have an incomplete application.

What does "nag mail" mean? For incomplete applications, we only send reminders to send in their applications. That logic happens in CompleteApplicationReminder

UPDATE: oh "nag" like we're nagging them? I think that's the CompleteApplicationReminder –– happens with a cron job and sends one reminder at 7 days without an update and a second reminder at 14 days without an update

haha, sorry for the made up nickname. yes CompleteApplicationReminder is what I was looking for, thanks!

davidsbailey

awesome work Meg! Good for users, good for engineers 🤝

davidsbailey · 2023-04-20T18:50:43Z


-      email.send! if deliver_now
+      email.send!
      email.save!


do we still need to save? it looks like send! will take care of it via update!: https://github.com/code-dot-org/code-dot-org/blob/2650e3e42b51d0635b1b9b2f2d2d324887b03cca/dashboard/app/models/pd/application/email.rb

my recommendation would be to try removing email.save! and see if that passes drone. this can be optional or done in a later PR, though!

Ah nice –– will try that

Removing the save works, as expected!

davidsbailey · 2023-04-20T19:05:38Z


      put :create, params: @test_params
+      application = TEACHER_APPLICATION_CLASS.last
+      assert_equal 1, application.emails.where.not(sent_at: nil).where(email_type: 'confirmation').count


this could be hard to debug if the create fails. I would suggest leaving the assert_response :success right after the put, here and below

davidsbailey · 2023-04-20T19:10:40Z


+    setup_all do
+      Pd::Application::ApplicationBase.any_instance.stubs(:deliver_email)
+    end


do you have an understanding of what happens to this stub after this test case runs (and finishes) later in this file?

code-dot-org/dashboard/test/models/pd/application/application_base_test.rb

Line 273 in 2650e3e

application.expects(:deliver_email).once

for example, when that test case finishes, do we go back to actually calling deliver_email in subsequent test cases, or does the original stub from setup_all get restored?

I'm not sure I could say for sure from looking at it. one idea (as an alternative to investigating) would be to use setup instead of setup_all here.

Oh great question –– I'll investigate

I looked into trying to do an expect of a method that runs before the deliver_email stub, but nothing really made sense. So I went ahead and change the setup_all to setup as you recommended

Meg Crenshaw added 13 commits April 19, 2023 13:09

Rename queue_email to send_pd_application_email

0824116

Remove deliver_now parameter

4adbd57

Update tests

ecc710c

Remove send_all_queued_emails

1c6cc3b

Update comments from queue to send

5020d3a

Adjust course to be consistent with hash

2084986

Stub out deliver_email

07ed27c

Remove deliver_now param in change principal approval

f2ee51f

Fix variable name

0f0498c

Send decision emails in controller

e85aef6

Require status change to send email

3f1e847

Fix tests

75a2abe

Remove clearing queue from cron job

52d7057

megcrenshaw changed the title ~~Meg/deprecate queuing pd application mailers~~ Deprecate queuing PD application mailers and send all right away Apr 19, 2023

Meg Crenshaw added 4 commits April 19, 2023 13:29

Update tests

ba409be

Only log status change if status is present in params

a27e26c

Use post method to update applications in tests

3d7b2ca

Remove param in complete_application_reminder

2650e3e

megcrenshaw marked this pull request as ready for review April 20, 2023 12:29

megcrenshaw requested a review from a team April 20, 2023 12:29

davidsbailey approved these changes Apr 20, 2023

View reviewed changes

Meg Crenshaw added 3 commits April 20, 2023 20:38

Remove saving after sending

0fe86fe

Assert response first

483cc28

Stub on each test to allow expect assertion

9607073

megcrenshaw merged commit d5fdceb into staging Apr 24, 2023

megcrenshaw deleted the meg/deprecate-queuing-pd-application-mailers branch April 24, 2023 14:27

TurnerRiley mentioned this pull request Apr 26, 2023

Add 'deliver_now' to teacher application mailer #51551

Merged

8 tasks

Navigation Menu

Uh oh!

Deprecate queuing PD application mailers and send all right away - #51411

Deprecate queuing PD application mailers and send all right away#51411
megcrenshaw merged 20 commits into
stagingfrom
meg/deprecate-queuing-pd-application-mailers

megcrenshaw commented Apr 19, 2023 •

edited

Loading

Uh oh!

dmcavoy commented Apr 20, 2023

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

megcrenshaw commented Apr 20, 2023 •

edited

Loading

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

davidsbailey left a comment

Uh oh!

davidsbailey Apr 20, 2023

Uh oh!

megcrenshaw Apr 20, 2023

Uh oh!

megcrenshaw Apr 24, 2023

Uh oh!

davidsbailey Apr 20, 2023

Uh oh!

megcrenshaw Apr 20, 2023

Uh oh!

davidsbailey Apr 20, 2023

Uh oh!

megcrenshaw Apr 20, 2023

Uh oh!

megcrenshaw Apr 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

megcrenshaw commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

dmcavoy commented Apr 20, 2023

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

megcrenshaw commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

megcrenshaw commented Apr 19, 2023 •

edited

Loading

megcrenshaw commented Apr 20, 2023 •

edited

Loading