Skip to content

LG-7643: Use deliver_later for in-person proofing results emails#7007

Merged
tomas-nava merged 1 commit intomainfrom
tomas/lg-7643-always-deliver-later
Sep 22, 2022
Merged

LG-7643: Use deliver_later for in-person proofing results emails#7007
tomas-nava merged 1 commit intomainfrom
tomas/lg-7643-always-deliver-later

Conversation

@tomas-nava
Copy link
Contributor

🎫 Ticket

LG-7643

🛠 Changes

Uses deliver_later for sending in-person proofing results emails instead of deliver_now_or_later.

Why?
The deliver_mail_async flag is off in production, but we want to send these emails after a delay. This change lets us do that without turning on the feature flag and causing all emails to be sent async.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

email_address,
enrollment: enrollment,
).deliver_now_or_later(**mail_delivery_params)
).deliver_later(**mail_delivery_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we update the lint to detect if we send params to deliver_now_or_later because those will get ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean if we pass params to deliver_now_or_later adding a warning that says something like "Parameters sent to deliver_now_or_later will be ignored if the deliver_mail_async flag is off"?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup! (either that, or change the behavior inside deliver_now_or_later to use the params if they are passed)

@tomas-nava tomas-nava merged commit 063d350 into main Sep 22, 2022
@tomas-nava tomas-nava deleted the tomas/lg-7643-always-deliver-later branch September 22, 2022 16:49
@svalexander svalexander mentioned this pull request Feb 2, 2024
14 tasks
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.

3 participants