Skip to content

Update ses delivery and bounce notifications to handle weekly deliveries#1981

Merged
SamJamCul merged 3 commits intomainfrom
update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries
Mar 25, 2026
Merged

Update ses delivery and bounce notifications to handle weekly deliveries#1981
SamJamCul merged 3 commits intomainfrom
update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries

Conversation

@SamJamCul
Copy link
Copy Markdown
Contributor

@SamJamCul SamJamCul commented Mar 20, 2026

What problem does this pull request solve?

https://trello.com/c/Z4ZkhRch/2884-update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries

Makes sure the bounces and complaints job can handle a weekly delivery schedule

Accounts for weekly delivery batches in the rake tasks that deal with bounced deliveries.

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Do the end to end tests need updating before these changes will pass?
  • Has all relevant documentation been updated?

@SamJamCul SamJamCul force-pushed the update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries branch 2 times, most recently from bdf828a to e1f5acd Compare March 24, 2026 09:33
@SamJamCul SamJamCul marked this pull request as ready for review March 24, 2026 09:52
@SamJamCul SamJamCul force-pushed the update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries branch 2 times, most recently from 107cf0d to 196de77 Compare March 24, 2026 10:20
stephencdaly
stephencdaly previously approved these changes Mar 24, 2026
Copy link
Copy Markdown
Contributor

@stephencdaly stephencdaly left a comment

Choose a reason for hiding this comment

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

Changes all look good to me.

I think we should also update submissions:redeliver_daily_batches_by_date to be able to also re-deliver weekly batches. Currently it looks for submissions created between 2 timestamps and gets their corresponding deliveries. But I wonder whether it's more useful to get deliveries created between 2 timestamps to account for weekly?

And either re-deliver all daily or weekly batch deliveries found between the two timestamps, or take an argument to specify "daily" or "weekly"?

@SamJamCul
Copy link
Copy Markdown
Contributor Author

Changes all look good to me.

I think we should also update submissions:redeliver_daily_batches_by_date to be able to also re-deliver weekly batches. Currently it looks for submissions created between 2 timestamps and gets their corresponding deliveries. But I wonder whether it's more useful to get deliveries created between 2 timestamps to account for weekly?

And either re-deliver all daily or weekly batch deliveries found between the two timestamps, or take an argument to specify "daily" or "weekly"?

I'm not even sure we really need this task, it's a bit confusing at this point. We would already have redelivered them if we'd used the retry_bounced_deliveries job.

I can update the redeliver_daily_batches_by_date to instead look for all batches that were created between the dates, I don't think we need finer control as it's already rare we'd ever use this.

@stephencdaly
Copy link
Copy Markdown
Contributor

stephencdaly commented Mar 24, 2026

Changes all look good to me.
I think we should also update submissions:redeliver_daily_batches_by_date to be able to also re-deliver weekly batches. Currently it looks for submissions created between 2 timestamps and gets their corresponding deliveries. But I wonder whether it's more useful to get deliveries created between 2 timestamps to account for weekly?
And either re-deliver all daily or weekly batch deliveries found between the two timestamps, or take an argument to specify "daily" or "weekly"?

I'm not even sure we really need this task, it's a bit confusing at this point. We would already have redelivered them if we'd used the retry_bounced_deliveries job.

I can update the redeliver_daily_batches_by_date to instead look for all batches that were created between the dates, I don't think we need finer control as it's already rare we'd ever use this.

I think the intention is to re-deliver deliveries that didn't bounce in the case that they were lost or malformed. I think we've used it before when we sent out emails without CSV attachments and needed to resend them. So agree it shouldn't be needed often but might be sometimes. It might be worth updating the description to make clear it shouldn't be used for bounces?

@SamJamCul SamJamCul force-pushed the update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries branch 2 times, most recently from b6a55dd to 934278f Compare March 24, 2026 11:40
@SamJamCul
Copy link
Copy Markdown
Contributor Author

I think the intention is to re-deliver deliveries that didn't bounce in the case that they were lost or malformed. I think we've used it before when we sent out emails without CSV attachments and needed to resend them. So agree it shouldn't be needed often but might be sometimes. It might be worth updating the description to make clear it shouldn't be used for bounces?

I've updated it so it will catch all non-immediate deliveries between the time range for the form, and the description makes it clear that it's re-delivering batch deliveries.

The redelivery task now handles all non-immediate deliveries for a given
form and time range, and looks for deliveries created between the time
range, rather than submissions within the time range that have a
corresponding batch delivery.
@SamJamCul SamJamCul force-pushed the update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries branch from 934278f to 07beb61 Compare March 24, 2026 12:17
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-1981.submit.review.forms.service.gov.uk/

It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready
after 5 minutes, there may be something wrong with the ECS task. You will need to go to the integration AWS account
to debug, or otherwise ask an infrastructure person.

For the sign in details and more information, see the review apps wiki page.

@SamJamCul SamJamCul added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit dd09a44 Mar 25, 2026
5 checks passed
@SamJamCul SamJamCul deleted the update-ses-delivery-and-bounce-notifications-to-handle-weekly-deliveries branch March 25, 2026 09:44
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.

2 participants