CRM-21100 Convert list of test groups into AJAX based select2 like r…

[seamuslee001]

…eceipients box

Overview

Previously the Test Group box was generated using a list of groups put into the DOM. Now it works off an AJAX based API call in the same style as the Recipients box

Technical Details

This removes the need to load the groups in through items that are loaded as the Angular initializes to using AJAX based API call to retrieve a list of groups and their titles

Comments

@mlutfy are you able to test this out, You will need to clear caches for this to work

[eileenmcnaughton]

@seamuslee001 this feels like a variant of what @totten did for Mosaico the other day - is this still required? Does that mean @totten has his head in this code enough to review this?

[seamuslee001]

So this will need a little bit of further work but its an extension of what happened to the recipients group. Tim basically did a hatchet fix for Mosaico to cope with my hatchet fix for test Groups. This is a more permanent and performant fix for test Groups

[eileenmcnaughton]

Perhaps close & re-open once you have done the re-fix? I'm conscious I found this because I was looking at PRs that had slipped into the neglected part of the q

[seamuslee001]

I would rather keep the PR open

[eileenmcnaughton]

ok - we can put [wip] in the header

[seamuslee001]

@eileenmcnaughton changes made just had to back out some changes as they are now required for other parts of the mailing interface. I have retested this locally and confirmed it still works as with the original fix

[seamuslee001]

@davejenx are you able to give this one a test, You will probably need to clear cache after applying to make sure it works. What your checking is that you can still find / select a group to send a test mailing to from the new mailing screen. It should load the page slightly faster maybe not heaps but slightly

[eileenmcnaughton]

@davejenx are you going to be able to test this?

[colemanw]

@seamuslee001 I'm curious to know why there is so much code for this select field. Normally an EntityRef field can be created simply by declaring it. For example:

<input crm-entityref="{entity: 'Group', select: {allowClear:true}}" ng-model="myobj.field" />

[eileenmcnaughton]

@colemanw @davejenx @seamuslee001 how do we progress this?

[colemanw]

I'd like to get clarification from @seamuslee001 in response to my last question.

[seamuslee001]

thanks @colemanw seems to work ok

[colemanw]

By adding to the github url ?w=1 to filter out whitespace changes I was able to understand this change :)
It looks like you're adding an api call to lookup the group name, but that data hasn't actually been removed from the scope so I don't think changes to this file are necessary.

[seamuslee001]

@colemanw yes they are necessary, if you notice in old line 32 we are plucking out the title from an array ids, titles of groups. In this case we are no longer using that array (array is scope.crmMailingConst.testGroupNames so we have to because all we have then at present is the id of the group we have to make the API call to get the title of the group. to set as part of the title of the dialog

[colemanw]

My point was that scope.crmMailingConst.testGroupNames hasn't actually been removed by this PR, so it should still be accessible to this function, no?

[seamuslee001]

The long term goal i see is getting rid of scope.crmMailingConst.testGroupNames eventually to reduce the bloat in the DOM, We started by initially getting agileware to convert the recipients box to be AJAX, now we're fixing the test group stuff here and then need to solve the unsubscribe group. Once all that is done and check that mosaico gets appropriate changes. We can then remove testGroupNames out so that we reduce the amount of data getting pushed into the DOM in the boot. On large sites it has been found the loading of this array and also the message templates slows it down significantly (message template stuff was taken care of in a separate PR)

[colemanw]

Ok. Technically that information is in memory and doesn't have to be fetched from the api, because the Select2 element has already retrieved the group title. Let me think about how best to access it to avoid the extra api call.

[colemanw]

@seamuslee001 I made some additional improvements. See what you think.

[colemanw]

@seamuslee001 I've added one more commit to completely remove testGroupNames from the dom by using another dynamic entityRef field. All looks good in my runtime testing. Why don't you try it out too and we'll call this done. (note: to try out the final commit you need to create a mailing from a search)

[seamuslee001]

@colemanw your changes look good to me However we may need to retain the testGroupNames just for a little bit or at least make it conditional. The reason is because of https://github.com/veda-consulting/uk.co.vedaconsulting.mosaico/blob/f95588b2d1eadde50df51b6247f2049e2683f61b/ang/crmMosaico/BlockMailing.js#L23 https://github.com/veda-consulting/uk.co.vedaconsulting.mosaico/blob/f95588b2d1eadde50df51b6247f2049e2683f61b/ang/crmMosaico/BlockPreview.js#L12 and https://github.com/veda-consulting/uk.co.vedaconsulting.mosaico/search?utf8=%E2%9C%93&q=groupNames+&type= we would need to patch all of those parts in mosaico before we could be satisfied to completely remove it. We could make the loading of it conditional on if template == mosaico ?

[colemanw]

Ok @seamuslee001 I've ammended the commit to not remove the variable. That's probably enough tweaking for one PR.
Let's tackle the Mosaico issue later.

[seamuslee001]

I'm happy with your latest changes, @eileenmcnaughton would you be able to test this?

[colemanw]

She has enough on her plate. I'm happy with our mutual testing.

[colemanw]

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass
• (r-user) Pass
• (r-tech) Pass

[eileenmcnaughton]

thanks guys - agree that the 2 of you are enough testers