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

AQTS230 Save failed emails to MailDeliveryFailure table #2311

Merged
merged 8 commits into from
Sep 3, 2024

Conversation

syed87
Copy link
Contributor

@syed87 syed87 commented Jul 17, 2024

ticket

MailDeliveryFailure model has been merged into the codebase, this pr will provide the functionality to create entries for the table by recording errors when sending emails.

@syed87 syed87 requested a review from a team as a code owner July 17, 2024 09:00
@syed87 syed87 force-pushed the AQTS-230-modify-MailDeliveryJob branch from 03a80a4 to 6a0ab77 Compare August 8, 2024 13:42
@syed87 syed87 force-pushed the AQTS-230-modify-MailDeliveryJob branch 2 times, most recently from fcd2f85 to ddc005c Compare August 15, 2024 14:21
Copy link
Contributor

@thomasleese thomasleese left a comment

Choose a reason for hiding this comment

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

A few small comments, but otherwise looks good.

app/views/anonymous/test_notify_error.text.erb Outdated Show resolved Hide resolved
spec/mailers/application_mailer_spec.rb Outdated Show resolved Hide resolved
raise
end

def notify_email(headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this method here? It looks like a convenience method for calling view_mail with the template ID already provided, but it's not used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was originally going to add the mailer_action_method and mailer_class to the header but that was done automatically. However It's still used in the application mailer spec on line 15 to set some dummy info.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should have a method that's only used by the tests, I'd suggest the tests can be updated to call view_mail directly like our other mailers to:

view_mail(GOVUK_NOTIFY_TEMPLATE_ID, to: "[email protected]", subject: "Some subject")

@syed87 syed87 changed the title add class which extends the behaviour of MailDeliveryJob by recording… AQTS230 Save failed emails to MailDeliveryFailure table Aug 16, 2024
@syed87 syed87 force-pushed the AQTS-230-modify-MailDeliveryJob branch from 52bce21 to bc18745 Compare September 3, 2024 14:12
@syed87 syed87 merged commit 7456e95 into main Sep 3, 2024
12 checks passed
@syed87 syed87 deleted the AQTS-230-modify-MailDeliveryJob branch September 3, 2024 14:25
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