CRM-21383 - Inclusion of a directive to replace the existing select f…

[nbrettell]

…ield for loading Message templates on demand in the CiviMail compose UI

Overview

This is to replace the current selection field for choosing a message template in the CiviMail compose UI screen, so that templates are loaded on demand rather than on the initial state.

Before

Please see @davejenx comment regarding performance issues #on the following pull request #11091 (comment)

After

Templates loaded on demand, with the ability to search for a specific template.

Technical Details

Removed the origin field for the choosing of templates, this has now been written as a AngularJS directive. Comments I believe how I have included the LoadMessage template function is incorrect, I don't like how I am having to use the grandparent scope, but I couldn't see how to avoid this. Overall, I have seen good performance improvements. However, I have also noticed a slight delay in loading the field on the latest master (seen in the above GIF).

[civicrm-builder]

Can one of the admins verify this patch?

[nbrettell]

Hi @seamuslee001

I had a brief conversation with you about this at the sprint in Bampton. I would appreciate your advice on a few small points which would help to improve this further.

1. On Selecting a message template, the following snippet is used to load the template html into the body

   scope.$parent.loadTemplate(scope.$parent.$parent.mailing, entity_id);

   Could you think of a better way of accessing the mailing object needed to load the template, rather than getting these values from the scopes grandparent 'MsgTemplateCtrl.js'?

2. There is a very slight delay when loading the CiviMail Compose UI on the new Message Template field compared to the others, as you can see in the GIF. I can't see the reason for this, it wasn't happening in my alternate demo site on 4.7.23. (I will continue too look into this).

[seamuslee001]

Jenkins ok to test

[seamuslee001]

probably unneeded here

[nbrettell]

Agreed, this was only needed for grouping the inclusion of recipients/mailing groups.

[seamuslee001]

@nbrettell Nathan, good to see you following on with work here, can you fix the style problem found by the checker https://test.civicrm.org/job/CiviCRM-Core-PR/17981/checkstyleResult/new/ and on point 1 I don't think so, Re 2. not sure will need to test it out myself to see what can happen

[davejenx]

@nbrettell Have done some testing on the dev copy of the site we used for testing #11091 & #11142. I tested vanilla 4.7.27 vs 4.7.27 + this PR.

Results
(Time to finish loading compose UI, average over a few trials)...

Condition 1:
Message templates: 1212
Mailing groups: 2706
Non-archived mailings: 13597

Average UI load time:
Vanilla 4.2.27: 123s
4.2.27 + PR 11267: 38s

• Note: template select2 doesn't appear until UI finishes loading.

Condition 2:
Message templates: 1212
Mailing groups: 1
Non-archived mailings: 13597

Average UI load time:
Vanilla 4.2.27: 75s
4.2.27 + PR 11267: 20s

• Note: template select2 appears before UI finishes loading: after about 15s.

Summary

Excellent! That's a reduction of 85 seconds (69%) with 2706 mailing groups; a reduction of 55 seconds (73%) with 2706 mailing groups.

As noted, I found quite a long delay before the template select2 field loaded, taking the full 38 seconds in condition 1 and 15s out of the total 20s in condition 2.

As I noted at #11142, it's interesting that altering the number of mailing groups still makes a big difference to the load time, even with mailing groups loading on demand. Also of interest here is that the number of mailing groups affects how long it takes the new template select2 field to load. During this wait, no AJAX requests are in progress, server is idle, client using 100% CPU. So there's some client-side processing going on that is sensitive to the number of enabled mailing groups. Would be good to have @seamuslee001's thoughts on that.

Really pleased with these results and I haven't come across any issues other than the delay in loading the select2 field.

[eileenmcnaughton]

merging based on @davejenx review

[monishdeb]

Sorry for being late in reviewing this PR. Great work @nbrettell :)

[colemanw]

I'm curious to know why the crm-entityref directive couldn't be used instead of all this custom code.

[mattwire]

@nbrettell @davejenx This seems to break wordpress as it can't load the file http://wpmaster.demo.civicrm.org/wp-admin/~/crmMailing/Templates.html when creating a new mailing and you are not able to select a template.

[seamuslee001]

@mattwire #11676