Version 2 #141
Merged
Version 2 #141
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We want to support as many versions of Rails as we can, but anyone running the gem in Rails 5 should really be on the last version. As we start to update the gem to version 2 the latest release of the notificaitons gem is also available, so we update that and all subsequent minor and patch version should be applied.
Pull Request Test Coverage Report for Build 8780635891Details
💛 - Coveralls |
jdudley1123
approved these changes
Apr 19, 2024
There was a problem hiding this comment.
This looks great! Thanks Meyric
I'm not an expert on ActionMailer or the bugs that you were looking to address, so I've reviewed it more from a 'does this seem to make sense and be internally consistent' point of view. I've added a couple of minor queries, but overall looks really good and seems like a big improvement.
|
|
||
| def template_mail(template_id, options) | ||
| raise ArgumentError, "You must specify a Notify template ID" if template_id.blank? | ||
| raise ArgumentError, "You must specify a to address" if options[:to].nil? || options[:to].blank? |
There was a problem hiding this comment.
I think the .nil? check is redundant here as .blank? covers it.
Is options[:to] ever an array? If so, there is some tricksy behaviour where an array of nil values isn't considered to be blank which we might want to account for.
There was a problem hiding this comment.
Great question! I've never seen it be an array and my gut response is Notify has a one-message-to-one-address, but I need to check it! I have to assume the existing implementation does nothing about this either otherwise I hope I would have covered it - but let me look.
There was a problem hiding this comment.
@jdudley1123 Yep, you'll see here that you can pass around an array of to adresses but we only send to the first one.
Whilst this is not made clear anywhere, it is the existing API so I feel like we can probably go with it and make an issue to look into it more - I don't even know what Notify would do with multiple to addresses?
Let me know what you think?
There was a problem hiding this comment.
Note for myself mostly: ActionMailer is passing around a Mail::Message which returns an array for to:
|
@jdudley1123 thank you - this is all I am after 👌 |
yndajas
approved these changes
Apr 19, 2024
Engines are intended to be use inside Rails apps and give us some nice features we will use later in v2. https://guides.rubyonrails.org/engines.html
All this code relates to ActionMailer previews and it is what we think causes the previews to fail in Rails > 7.0.8. Prepending any objectm, as was done here has a high price and is not needed for the previews to work in Rails. v2 will use a different approach to this.
Now we are a Rails engine, we a can replicate the app directory and Rails will find the files within. Here we move the layout used to render previews.
We get a copy of the layout used in Notify emails from: https://github.com/alphagov/notifications-utils/blob/main/notifications_utils/jinja_templates/email_template.jinja2 This updates the layout to the latest version that includes the new tudor crown.
Right now, the simplest way to render the html body that comes back from Notify in a layout so that the user gets the closet preview to the real email is to use the `renderer` from a controller. Whilst we could use ApplicationController, it feels a little brittle, so instead we create a completely empty controller so we can access it's renderer. This will get used later to render previews.
ActionMailer lets us intercept messages before they are delivered or previewed, this is the ideal time to fetch our preview from the Notifications API and update the message with it. Read more in the 'Previewing' section: https://api.rubyonrails.org/classes/ActionMailer/Base.html
We want to store the tempalte id, personalisation, reference and reply to id options inside the `message` object, right now these are added to the header of the message, which feels a little clunky. Here we use the existing module to add the attributes to message so we can set and get the values. We also drop the `preview` method, this method hides the fact that the impelmentation of the `preview` method is in the DeliveryMethod - on balance the extra code to call the method seems worth the price of the understanding where messages are being sent - to be honest, it already feels odd that the message calls a method and passes it self - but that might just be me?
mec
force-pushed
the
v2
branch
2 times, most recently
from
3f47c27 to
3f00ffd
Compare
pezholio
approved these changes
Apr 22, 2024
The Mailer is what is used to provide applcations with Notify emails. This class is used as the parent for new subclasses that provide the application specific emails. Like a module, we have to test this by creating a new TestMailer that inherits from our updated class. The main thing to notice here is that we have to prevent Rails from carrying out the default behaviour of looking for views to provide the parts of the email (the html and plain text content), we do so by calling `mail` with a block that sets the parts to empty object - these are then replaced later with the actual content.
The delivery method is where the actual sending and in our case previewing happens - for that reason it is where the calls to the Notify API are made. We've updated this class to make it work for the new way messages are formed and for simplicity. We've stopped using webmock in the tests as that felt too much intergration, it felt like we are testing the notifications_client rather than our code as long as we pass the correct params to the client we should feel confident the right thing will happen.
If passed a nil in the personalisation, the Notify API will raise an error. The `blank_allowed` method lets developers opt-in to allow a blank value, here 'blank' probably means `nil`. As v2 no longer relies on the `Personalisation` class to handle personalisations we want to remove the class, but this method still relies on it. We want to keep the method in-case it is used. In v2, we really only need to know when a developer is opting in by calling the method and then convert nil into an empty string.
- Version 2.0.0 rewrites most of the gem, without altering the API - The gem no longer prepends code in ActionMailer - Previews are now 100% supported in Rails, but will require a Notify API key - The preview layout has been updated and uses the Tudor Crown
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is it! Version 2 is ready!
This all started with Rails 7.1 was released which added a link to the eml of a email to the preview. It turned out the 1.x implementation broke link path generation and the gem did not support the Rails email preview navigation at all. This was identified a long time ago by @duncanjbrown and demonstrated here. I've since added integration tests for Rails 5,6 and 7 that demonstrate this issue, you can see this now on the main branch.
It's worth noting that the sending of email has never been broken, only the previews, which I have yet to meet anyone who actually uses this feature - but it had to work.
The main issue seemed to be the classic of 'monkey patching', we were
prependingan action in a controller deep inside Rails that handles preview generation.After much research and digging in my spare time I found what we needed: ActionMailer has the concept of 'interceptors'. These are used to modify the email before it is sent and more importantly, before it gets previewed. This gave me the hook I needed to get the preview from Notify and return it back.
It's still a little janky as ActionMailer assumes that the preview (and the sent email) is generated inside Rails, whereas Notify does it for us - but the result is solid. We have to create an 'empty' controller to get the renderer (there is probably a nicer way to do this) and we have to prepare a fake, empty email that will later be populated by the response from the Notify API.
The drawback is that if you want previews, you have to have a live connection to Notify - I think this price is fine as conceptually you have made the choice to shift most of the email handling to Notify already.
One of the main things I felt during this work is that you need a very different mindset working on a gem compared to working on a fast moving project. I've comment a lot of code to try and speed up anyone who comes in the future to understand what is going on. None of this is help by the fact that ActionMailer is not the most glamorous area of Rails!
I feel that it is also important not to update any old dependency update the Renovate or Dependabot finds - we are trying to support a good cross section of both Ruby and Rails versions.
This work closes #101! 🎉