Page MenuHomePhabricator

Add tests for mail templates
Closed, ResolvedPublic8 Estimated Story Points

Description

As developer
I want to automatically receive notification (CI) when changes to mail templates introducted bugs
in order to guarantee high quality customer communication

AC:

  • rendered mail templates are automatically tested against a language-independent "golden master" (keys names instead of content/messages)
    • check if the variables from the context were rendered
    • check if the conditionals were applied correctly

Background:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
gabriel-wmde set the point value for this task to 13.
Pablo-WMDE changed the point value for this task from 13 to 8.

Edit: nvm, I unconfoosed myself mostly, see next comments.

I'm afraid I don't understand this well enough to start with it beyond making some random CS changes in https://github.com/wmde/FundraisingFrontend/pull/893

Based on https://github.com/wmde/FundraisingFrontend/pull/884 I figure that what this task is about is to record the output of the command on some templates and then have the CI run the command and compare to see if the output is the same. It's not clear to me if the output should be identical or if a more sophisticated check is in order. It's also not clear to me which templates are relevant. Which ought to not be surprising if you consider I don't know what the RenderMailTemplatesCommand is for. ("A command to check and dump mail templates" does not help me much. Check what? What does dump mean? What mail templates?) If I try to run it on Vagrant I get

php console dump-mail-tpl -o /tmp/a
PHP Notice:  Uninitialized string offset: 0 in /vagrant/src/Factories/FunFunFactory.php on line 1350
PHP Notice:  Uninitialized string offset: 0 in /vagrant/src/Factories/FunFunFactory.php on line 1350

  [WMDE\Fundraising\ContentProvider\SetupException]      
  An exception occurred setting up the ContentProvider.  
                                                                
  [Twig_Error_Loader]                                                                                                  
  The "/vagrant/src/Factories/../..//de_DE/web" directory does not exist ("/vagrant/src/Factories/../..//de_DE/web").

Not sure where to find more info on this. And that's assuming my guess that this issue about that command is correct. For all I know I'm even looking in the wrong repo.

Ok, so my understanding now is:

  • This is about the FunFrontend/app/templates/Mail_*.twig files
  • We want to have tests for the logic they contain (and for nothing else (as far as this ticket is concerned))
  • This testing can be done by using the dump-mail-tpl commands output and comparing it to a saved copy
  • dump-mail-tpl was created for this reason, and perhaps also to manually test/preview when working on templates. It is purely a dev tool and not some caching/optimization step

Is that correct?

And what kind of comparison for the "dumped" (I still don't get this term) content would be good? If the whole things are compared, the tests will break as soon as you change a newline or some such in the template. It is unclear to me what kind of behavior would make the development process actually easier.

dump-mail-tpl was written while we were moving the contents from the emails to the content repo, to have "before and after" comparisons, done manually. I think it's not the right tool for the tests described here because it utilizes the full environment (with i18n and content repo). I18n and content will change frequently and break things. The test should be regular unit tests that use the test case data from tests/Data/mail_templates.php and pre-rendered output and compare the render result to it. The rendered and pre-rendered output should not contain i18n and content, just their keys.

For this, we need a ContentProvider class that returns the content key instead of rendering the content. You can mock the existing class or do a PR to use an interface. What needs to be done with the translation class is not clear yet, probably also some mocking/stubbing. Maybe it library already provides the functionality.

You'll probably end up extracting code from RenderMailTemplatesCommand to be used both in the command and the test.

Status update:

I factored out the code that finds all mail templates from the CLI command into an Iterator (IteratorAggregate implementing class that contains a Generator) that is returned by the top level factory as an iterable (PHP 7.1) type. Burned some time by running into 2 PHP walls:

I can definitely progress without solving those, and that is what I will do if I do not have an answer by tomorrow. This does make Jeroen into a sad Jeroen though. Why is PHP so broken :'(

After a tiny diversion that lead me to a neighboring superclsuter, I finally managed to get something useful done: save rendered templates to disk https://github.com/wmde/FundraisingFrontend/pull/902

Still some way to before this is complete tho