Unite `Template` and `Renderer` classes #94

quis · 2016-12-01T15:22:03Z

So this is the work that I wanted to do as part of #84 but wasn’t sure whether it was worth the effort. But @leohemsted’s comments on that PR got me thinking again, and I had 2 uninterrupted hours on the train this morning, so here we are.

This is a big refactor. It’s addressing three issues which were making the existing code bloated and overly complicated:

The interaction between the Template and Renderer classes was still weird, because an instance of Renderer had effectively become a method of a Template instance
The Renderer classes themselves weren’t really proper classes, they were basically just functions that took a template and turned it into a string, but architected in a slightly odd way. There are people who consider a class that only has __init__ and __call__ methods an anti-pattern (see Jack Diedrich’s talk Stop writing classes
There were methods on the Template class (eg content_count) which were very specific to one kind of template (eg a text message).

This commit makes three main changes to address these issues:

Makes Template into an abstract-ish base class, and makes the various Renderers into Templates which inherit from the base template class (ie HTMLEmailTemplate instead of a renderer called HTMLEmail).
Moves the functionality of the renderers to __str__ methods on template instances. This is a better model of what they do (turn templates into strings).
Methods that are only relevant to one type of template (eg text messages) now live only on that type of template.

There is one thing that’s still slightly weird about this. Service-level things on a template (eg the text message sender name) aren’t set through an __init__ method any more because there’s no appropriate place for one to exist.

Instead they are set directly on the instance (eg template.sms_sender = 'Service name'). This means doing some slightly funny jiggery pokery around default and required ‘arguments’, which
would be more intuitive with an __init__. But I think the tradeoff is worth it for the simpler code elsewhere.

super() considered super!

Also, add subject property back onto certain templates

It’s really ugly having to do this sort of thing all over the API:

subject=str(Field(plain_text_email.subject, notification.personalisation))

But it doesn’t really belong in the base Template class because not all templates have subjects.

So this commit re-adds subjects (and the highlighting/replacing thereof) to those templates which do have subject lines.

So the API code can just change to:

subject=template.subject

The renderers are going to become part of the `Template` class, so there are going to be more tests of the various types of template than just the ones in this file. But I don’t wanna merge them all into one file because they’ll be testing different things, and it could get very long.

Just so it doesn’t get lost amongst all the other template stuff.

This is a big refactor. It’s addressing three issues which were making the existing code bloated and overly complicated: 1. The interaction between the `Template` and `Renderer` classes was still weird, because an instance of `Renderer` had effectively become a method of a `Template` instance 2. The `Renderer` classes themselves weren’t really proper classes, they were bsically just functions that took a template and turned it into a string, but architected in a slightly odd way. There are people who consider a class that only has `__init__` and `__call__` methods an anti-pattern [1] 3. There were methods on the `Template` class (eg `content_count`) which were very specific to one kind of template (eg a text message). This commit makes two main changes to address these issues: 1. Makes `Template` into an abstract-ish base class, and makes the various `Renderer`s into `Template`s which inherit from the base template class (ie `HTMLEmailTemplate` instead of a renderer called `HTMLEmail`). 2. Moves the functionality of the renderers to `__str__` methods on template instances. This is a better model of what they do (turn templates into strings). 3. Methods that are only relevant to one type of template (eg text messages) now live only on that type of template. --- 1. Stop writing classes – Jack Diedrich: https://www.youtube.com/watch?v=o9pEzgHorH0

Brings in: - [ ] alphagov/notifications-utils#94

It’s really ugly having to do this sort of thing all over the API: ```python subject=str(Field(plain_text_email.subject, notification.personalisation)) ``` But it doesn’t really belong in the base `Template` class because not all templates have subjects. So this commit re-adds subjects (and the highlighting/replacing thereof) to those templates which do have subject lines. So the API code can just change to: ```python subject=template.subject ```

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

leohemsted

(unrelated to current PR but there's some unreachable code in recipients.py#L269)

tested locally with admin and api checked out to respective PRs. seems mostly reasonable. code wise seems fine but there's too much code change here for me to critique layout without getting a headache ;)

but it doesn't honour sms placeholders exceeding max length and firetext throws a 400 at us

leohemsted · 2016-12-12T17:05:19Z

notifications_utils/template.py

-            raise TypeError("The template needs to have a template type of 'sms'")
-        return get_sms_fragment_count(self.content_count)
+    def content_count(self):
+        return len(str(self).encode(self.encoding))

    @property
    def content_too_long(self):


unused property? it certainly errors

Yeah, the API was doing this comparison itself. Removed in: 7997fc7

leohemsted · 2016-12-12T17:06:47Z

notifications_utils/template.py

+            'subject': self.subject,
+            'from_name': self.from_name,
+            'from_address': self.from_address,
+            'recipient': Field("((email address))", template.get('values', {}), with_brackets=False),


think this should be self._template? it's currently crashing when i try and send an email on admin app, so make sure there's a test for this

self.values. I think I’d fixed this already and squashed it out but not pushed it or something ¯_(ツ)_/¯

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

This method was actually broken by recent changes, and the API wasn’t using it anyway – it was just doing the comparison itself. This requires a slight change to `RecipientCSV` to also do the comparison itself.

Significant, breaking changes: - changes the interface to the `Template` class, including removal of the `drop_values` argument for all template types - removes all `Renderer` classes and moves them to methods of subclasses of the `Template` class

quis · 2016-12-13T10:24:06Z

Addresses unreachable code in a separate PR: #101

Brings in: - [ ] alphagov/notifications-utils#94

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

Includes: - [x] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR) The test for `get_sms_fragment_count` has been removed because this method is already tested in utils here: https://github.com/alphagov/notifications-utils/blob/ac20f7e99ece4935fccf39ed9d5c0fb40e45bfbc/tests/test_base_template.py#L140-L159

Brings in: - [ ] alphagov/notifications-utils#94

quis force-pushed the refactor-template-renderer branch 10 times, most recently from 4ebfa41 to 8dfc4d8 Compare December 6, 2016 11:36

quis added 2 commits December 6, 2016 16:58

Extract TemplateChange class into its own package

2a2b903

Just so it doesn’t get lost amongst all the other template stuff.

quis force-pushed the refactor-template-renderer branch from 8dfc4d8 to dafc322 Compare December 6, 2016 16:58

quis mentioned this pull request Dec 8, 2016

Make letter previews reflect what the letter will look like #100

Merged

1 task

quis added a commit to alphagov/notifications-admin that referenced this pull request Dec 9, 2016

Incorporate breaking utils changes

f710dba

Brings in: - [ ] alphagov/notifications-utils#94

quis added a commit to alphagov/notifications-admin that referenced this pull request Dec 9, 2016

Incorporate breaking utils changes

fa7d2e3

Brings in: - [ ] alphagov/notifications-utils#94

quis mentioned this pull request Dec 9, 2016

Render letter templates as PNGs and PDFs alphagov/notifications-admin#1037

Merged

5 tasks

quis added a commit to alphagov/notifications-api that referenced this pull request Dec 9, 2016

Update utils to 12.0.0

3061f2c

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

quis mentioned this pull request Dec 9, 2016

Update utils to 12.1.1 alphagov/notifications-api#764

Merged

2 tasks

leohemsted reviewed Dec 12, 2016

View reviewed changes

quis added a commit to alphagov/notifications-api that referenced this pull request Dec 12, 2016

Update utils to 12.0.0

121c007

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

quis added 2 commits December 12, 2016 20:24

Remove content length limit method

7997fc7

This method was actually broken by recent changes, and the API wasn’t using it anyway – it was just doing the comparison itself. This requires a slight change to `RecipientCSV` to also do the comparison itself.

Major version bump to 12.0.0

0a781c2

Significant, breaking changes: - changes the interface to the `Template` class, including removal of the `drop_values` argument for all template types - removes all `Renderer` classes and moves them to methods of subclasses of the `Template` class

quis force-pushed the refactor-template-renderer branch from dafc322 to 0a781c2 Compare December 12, 2016 20:26

servingUpAces approved these changes Dec 13, 2016

View reviewed changes

quis merged commit ac20f7e into master Dec 13, 2016

quis deleted the refactor-template-renderer branch December 13, 2016 10:30

quis added a commit to alphagov/notifications-admin that referenced this pull request Dec 13, 2016

Incorporate breaking utils changes

e7a47ac

Brings in: - [ ] alphagov/notifications-utils#94

quis added a commit to alphagov/notifications-api that referenced this pull request Dec 13, 2016

Update utils to 12.0.0

98aafeb

Includes: - [ ] alphagov/notifications-utils#94 (breaking changes which are responsible for all the changes to the API in this PR)

quis added a commit to alphagov/notifications-admin that referenced this pull request Dec 14, 2016

Incorporate breaking utils changes

3ed6240

Brings in: - [ ] alphagov/notifications-utils#94

quis added a commit to alphagov/notifications-admin that referenced this pull request Dec 14, 2016

Incorporate breaking utils changes

dc63de0

Brings in: - [ ] alphagov/notifications-utils#94

andyhd pushed a commit to crossgovernmentservices/notifications-admin that referenced this pull request Feb 22, 2017

Incorporate breaking utils changes

bc36328

Brings in: - [ ] alphagov/notifications-utils#94

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unite `Template` and `Renderer` classes #94

Unite `Template` and `Renderer` classes #94

quis commented Dec 1, 2016 •

edited

Loading

leohemsted left a comment

leohemsted Dec 12, 2016

quis Dec 13, 2016

leohemsted Dec 12, 2016

quis Dec 13, 2016

quis commented Dec 13, 2016

Unite Template and Renderer classes #94

Unite Template and Renderer classes #94

Conversation

quis commented Dec 1, 2016 • edited Loading

Also, add subject property back onto certain templates

leohemsted left a comment

Choose a reason for hiding this comment

leohemsted Dec 12, 2016

Choose a reason for hiding this comment

quis Dec 13, 2016

Choose a reason for hiding this comment

leohemsted Dec 12, 2016

Choose a reason for hiding this comment

quis Dec 13, 2016

Choose a reason for hiding this comment

quis commented Dec 13, 2016

Unite `Template` and `Renderer` classes #94

Unite `Template` and `Renderer` classes #94

quis commented Dec 1, 2016 •

edited

Loading