DEFRA / waste-carriers-frontend

Waste Carrier Registrations Frontend Code
Other
1 stars 1 forks source link

Fix last email cache body extraction #228

Closed Cruikshanks closed 5 years ago

Cruikshanks commented 5 years ago

It turns out my understanding of how the main content or 'body' of an email is stored in the email message was flawed. It's actually even more complex as you have to factor in whether an attachment has been added or not.

If you've created a multipart email then you'll have both a text and a html version (determined by adding the relevant erb views). If you do so then my_mail.parts.length will at least equal 2. If you set only one of them when creating the email then parts doesn't get populated. ActionMailer just populates the root body and sets the content type accordingly.

However any attachments will cause ActionMailer to use parts irrespective of whether you set one, both or neither! So for example if we have a text only email with an attached image, then parts will be of length 2; one being the content and the other being the attachment.

This meant 3 main changes

The last one was mainly out of frustration. As far as I could see we should be able to generate both standard and multipart emails dynamically. Below is an example I found in a couple of places

  mail(to: 'example@example.com', subject: 'Welcome to My Awesome Site') do |format|
    format.html { render html: '<h1>Welcome XYZ!</h1>'.html_safe }
    format.text { render plain: 'Welcome XYZ' }
  end

https://stackoverflow.com/a/49630370

So this shouldn't need to rely on a template existing. However no matter what changes I made I always got ActionView::MissingTemplate. And it seems I'm not the only one to get this result. The comments in that stackoverflow post refer to differing results dependending on the version of Rails used, plus I found another hitting the same problem.

So in order to be able to test it can extract the content from multiple email types my only recourse was to ensure we have both a text and html template for testing. And if we were having those then we might as well move all the test emails into a Testmailer associated with the templates.

Cruikshanks commented 5 years ago

When this gets approved I intend to follow it up by refactoring the existing rake test email action to make use of the new TestMailer and tidy up the existing RegistrationMailer.

Longer term as @cintamani pointed out, moving this stuff to its own gem would hide this sort of stuff from the actual application logic, so it won't feel so messy.