CRM-21206 Ensure that the recipients field of AB Test results is fill…

[seamuslee001]

…ed out

Overview

As groupNames is now dynamically populated when generating Emails, we need to dynamically generate here so that AB tests can still output the groups.

Comments

@eileenmcnaughton @totten @monishdeb I would argue this should be in the RC because the change to the dynamic recipient loading has only occured in either v4.7.24 or 4.7.25 (can't remember which) ping @johntwyman this is what i have us testing right now

[seamuslee001]

@johntwyman

[eileenmcnaughton]

I was able to replicate this & view it fixed - although I found the process deeply confusing.

Before

Note how recipients is blank

After

Note how recipients is no longer blank on mailing A. However, it IS still blank on mailing B - which is very confusing, but also reflects the database storage. I'm unsure what to make of that.

Also when I first load the page the groups do not show. On refresh they do.

I tried with a mix of Include + exclude with an include & an exclude of each type selected. The result is that only the excludes are visible.

So, based on UI testing this is not quite working correctly. The code looks straight forward but I also need to establish what screens share this functionality

[seamuslee001]

Thanks @eileenmcnaughton so its at least improves the situation right?

[eileenmcnaughton]

Apart from this report BlockReview seems to be the only think that uses this code chunk. I guess that's used when creating of possibly viewing mailings. I feel like I'm getting odd behaviour on new mailings / continue / re-use mailings - e.g unresolved search. I think for acceptance both creating a new mailing from A-b test & from new mailing & from continue / re-use need to be checked

[seamuslee001]

@eileenmcnaughton that is expected, the change was made that when you get the list of recipients it does a API call to retrieve list of groups and mailings based on what is typed in. The purpose of this was so that not all the groups were loaded into the DOM in an array which for sites like AUG is pretty large

[seamuslee001]

@eileenmcnaughton are you ok with the latest changes?

[eileenmcnaughton]

I'm back to the 'pre' behaviour with this :-(

[seamuslee001]

@eileenmcnaughton did you do a hard refresh and clear caches?

[eileenmcnaughton]

Ok looks like my browser caches were cleared adequately but I hadn't done Civi clear caches. Works fine on A/B test page now & I assume the fact the list only shows on mailing A & not B is a 'feature' or at least non-regressive.

I'm getting some weird behaviour on selecting recipients on new mailing. Will test a bit more

[eileenmcnaughton]

So on select recipients on a new mailing it feels like I'm getting some weirdness. Note in the below no include, only exclude although none are selected

And the 'None' in this shot

I'd really like some other people to test this as well

[seamuslee001]

v strange i have put this patch onto AUG dev box and appears to be happy there but one thing might help you when testing is to disable asset caching in debugging

[eileenmcnaughton]

Every time I try to test this my fan starts getting overworked & it gets unresponsive. I'm not sure if it's just a laptop problem or something about this is making my cpu work too hard.

I'm happy with the code & the behaviour on the A-b test screen but I'd like some other people to confirm their experience of the New Mailing & Continue mailing screens. Maybe @colemanw @MegaphoneJon @agh1 or @lcdservices would help?

[seamuslee001]

@eileenmcnaughton and others, I feel like i have improved the performance of this and should stop the fan running mad. tl/dr crazy angular. Doing some testing on AUG suggests this should be working well now. I don't see any errors on the new mailing either

[eileenmcnaughton]

@seamuslee001 I'm not convinced my issue reflects a real problem - but I'd like to get some more people test & confirm that I am alone in my misery

[colemanw]

@totten I think this is one for you.

[eileenmcnaughton]

@colemanw It's probably mostly more people doing UI testing rather than a code expertise thing

[mepps]

@seamuslee001 I can't recreate the original error, but I can recreate what @eileenmcnaughton saw in her test in the After screen (both on up to date master and with this patch). Test B doesn't have recipients assigned either in the summary view or the db. That seems like a bug but it's occurring both with and without this patch.

[seamuslee001]

@mepps Thanks Maggie, not sure what to do about the B but i think to me it means patch doesn't make things worse at leaset

[eileenmcnaughton]

@mepps when I tested this I got performance issues & weirdness on the new mailing screen when playing around there (adding & removing groups & included/excluded mailings)- but I wasn't sure if others would also have that or it was something odd locally...

Did you try that screen.

(shared the pic from the sprint at standup :-)

[mepps]

@seamuslee001 @eileenmcnaughton I can't find that this patch does anything so my inclination would be not to merge it, especially if there are possible performance issues. However, I'm thinking the recipient for mailing b being blank may be a bug we should fix separately.

[seamuslee001]

@mepps can i ask if in Administration -> System Settings -> asset caching if that was still set to auto or was on debug? i would recommend re-testing after stetting it to debug. I Can confirm this is on AUG's production system at the moment and we are seeing the issue fixed

[mepps]

Good point @seamuslee001! However, I have now tested multiple times with asset caching disabled and continue to see the same bug behavior.

[seamuslee001]

@mepps Maggie this isn't designed to fix the issue with B mailing, When in debug mode do you experience the issue reported which is that on AB test mailings the recipients field is not filled out. That is what we were experiencing. Applying the patch has fixed the issue for us

[seamuslee001]

@JohnFF are you able to review this one, I would recommend you turn off asset caching first

[eileenmcnaughton]

@seamuslee001 horse bolted on this one - wanna redo against master?

[seamuslee001]

@eileenmcnaughton have rebased against current master. I have re-tested locally and still works

[eileenmcnaughton]

@davejenx any chance you could review this. I previously reviewed it & agreed that it met the

• (r-jira) PASS
• (r-test) PASS
• (r-code) PASS
• (r-doc) PASS
• (r-maint) PASS
• (r-run) Undecided: I tested this previously & was able to verify the bug & the fix. My reservation is that I had weird performance on it - which may have been some special local foo. @davejenx any chance you could give this a UI spin since I know you care about mailing performance.
• (r-user) PASS:
• (r-tech) PASS:

[davejenx]

@eileenmcnaughton Will do. So the main thing is to see whether it impacts the performance of the New Mailing UI?

[eileenmcnaughton]

@davejenx yep - that 'failed' in my testing - but I wasn't convinced that all was well locally

[davejenx]

@eileenmcnaughton I'm not seeing any significant difference in performance of the New Mailing UI on a standard dmaster setup with the usual small number of mailing groups, templates etc. I'll give it a go on a big site when I get a chance.

[seamuslee001]

Jenkins test this please

[eileenmcnaughton]

Thanks @davejenx - the site I tested on did not have significant data so I think that invalidates my concerns on that. I'm going to proceed to merge this. I was going to give it the merge-ready flag to give it a cool-down but I guess it's had that :-) If you do hit further performance issues when you test further then we will address them