CRM-21279: Rebuild recipient list and calculate count on demand, store result in cache

[monishdeb]

Overview

In CiviMail compose screen, there are few issues with how the recipient count and list are fetched, which are:

1. Every time mailing groups are included or excluded from recipient field, this causes a repetitive call to mailingRecipient.get API to fetch count and list of recipients.
2. Four mailing fields are constantly monitored at a timespan of 100ms and in the process, it call previewRecipientCount angular service which eventually triggers mailingRecipient.getcount API to calculate recipient count.

In order to avoid such repetitive API call, a new CiviMail component setting is introduced called auto_recipient_rebuild which is set by default, that allows you to rebuild recipient data automatically or manually. Let me show you affect on mail screen if this new setting is enabled/disabled.

Auto recipient rebuild

Manual recipient rebuild

Technical Details

1. Created a angular service to enable $cacheFactory usage in 'EditRecipCtrl' controller. This factory constructs Cache objects and is persistent to current session
2. If the new setting is disabled then it provides a recipient rebuild icon beside recipient count text as shown in above screenshot.
3. On clicking the rebuild icon, underlying angular service fetch and store recipient count and list in cache, and then used to reflect the latest count and recipient list on 'Preview recipients' page
4. Change in recipient count placement on basis of manual/auto mode.

The idea behind is to avoid the repetitive call to APIs and fetch recipient data on demand to improve performance, but this feature is controlled by setting ```` auto_recipient_rebuild```

---

• CRM-21279: Rebuild recipient list and calculate count on demand, store result in $cacheFactory

[monishdeb]

ping @totten @colemanw @lcdservices @JoeMurray

[lcdservices]

This looks good. I implemented the PR and everything worked as expected (though I didn't yet monitor the API calls to confirm the reduced number of calls). The one thing I think we should tweak is terminology in the count block. "No recipients" as a default is misleading. Really, the recipient count just hasn't been generated. And I think the circular icon and "refresh" terminology makes sense if you build the count, then add new groups and want to rebuild the count -- but not if you're building it for the first time.

What if we had a button that said: "Estimate recipient count" followed by the number, where the default is "(unknown)". If we wanted to distinguish between the original count attempt and subsequent ones, we could have the button text change to "Refresh recipient count" after the original count is built. And we don't need the text "recipients" as part of the count link.

I'm concerned, in particular, for existing users who are used to seeing the counts auto-generated. I think we want to be more explicit about the fact that an action needs to be taken.

[seamuslee001]

I agree with Brian here, I haven't tested this but my instinct is that we need to handle that conversion from the autogenerated to the manual refresh. I like Brian's choice of button text. I was also wondering about whether or not we wanted to have an auto run when we removed groups. The reason is i think people may get confused thinking they are still sending to a higher number but hmm.

[lcdservices]

I'd vote against any auto-run -- that's largely the goal of this PR -- to reduce how many times the rebuildRecipients function is hit (as it's potentially a big query, depending on how many groups are included and if any are complex smart groups). Everything should be on-demand. But if we change the button text to "refresh" after the initial build, I think that may be sufficient to alert people to the potential outdatedness of the count.

Or...
It would be easy to monitor when the recipients field is changed in any way. We could add a visual queue on the "estimate/refresh" button and count value whenever the recipients field is changed (e.g. a dark yellow border) -- something that would alert the user that the count may no longer be accurate.

[monishdeb]

@lcdservices @seamuslee001 thanks for your feedback :). Actually now the result is stored in cache so in spite of repeated auto call to rebuildRecipients() it doesn't call any API or involve any big search query in the process but simply fetch the result from cache here against key mailing-1-recipient-count (here 1 is the mailing.id). So I kept the logic intact about estimating the recipient count ONLY on demand. Let me explain by patch:

1. First, when user clicks on that rebuild icon we are calling service crmMailingMgr.previewRecipientCount(...., true). Note the third parameter passed by value which is eventually used here to forcefully calculate the count and recipient list here.
2. The recipient count and list built in the previous step is stored in cache against respective key and on auto call of the same service now fetches the values from cache. This is the point where auto-call is made and note the third parameter rebuild=false.
   So, in my opinion, repetitive auto call to fetch count isn't a performance issue anymore as it now fetches the value from cache NOT api.

I agree with your suggestions and to derive a conclusive change to recipient text, following need to be done:

1. On initial page load the recipient text should be Estimate recipient count: (unknown)
2. On group selection, text unchanged until and unless the text link is clicked and will be refreshed to Refresh recipient count: ~29 if there are 29 distinct recipients in selected groups.
3. On group selection, text unchanged but now the text-color will turn red OR some pop-up status message to notify the user about the outdatedness of the count.

[seamuslee001]

I'm happy with the points as Brian and Monish have put. As long as there is a visual cue that the recipient count is no longer "valid" per se then i think it will fine.

[monishdeb]

@seamuslee001 @lcdservices I have updated the PR. As per the latest change:

Wasn't able to show initial text 'Estimate recipient count: (unknown)' on page load, as I didn't found any technique to track the initial page load :(

[monishdeb]

@totten @seamuslee001 I need your help in how to present recipient text as:

1. On initial page load the recipient text should be Estimate recipient count: (unknown)

Is there any way to tell when a page is initially loaded? which I can use here in IF condition.

[lcdservices]

Personally, I'd rather the estimate/refresh action be displayed as a button rather than a link. Visually, we want to make sure the user understands clearly that an action must be taken in order to build or refresh the count.

And the action behavior should be swapped -- currently if you click the text "estimate/refresh count" it opens the popup with the list of recipients, and if you click the count it refreshes the count. I think the "estimate/refresh count" should be styled as a button and should be what actually regenerates the count. Once a count exists, clicking the count number should open the popup list.

[JKingsnorth]

Sorry, I'm a bit late to the party here. But it seems a shame to lose the 'automatic' refresh of the recipient count.

I appreciate that we want to reduce load on the server, but is there a better way of doing that than forcing a manual update? Can we...

• Trigger an update only when the relevant fields are changed
• Don't 're-estimate' if nothing changes
• Add a delay to the refresh (while the text still says 'Estimating...') in case the user changes the recipients box twice in quick succession

(I'm not sure how many of these are already in place).

I think it's a shame to have to sacrifice usability for a 'technical' issue. I know this is something that our communications team feel quite strongly about too!

[lcdservices]

@monishdeb just ran through a test and it looks great
@JKingsnorth those suggestions were already in place, and yet we found the getRecipients function would still get hit much more frequently than was justified. If your mailing is with a relatively small group of recipients, and you were using regular groups, it likely wasn't an issue. But if you are sending to a large group and are basing it on smart groups that are potentially complex, it had the potential to really hammer the DB.

I honestly don't think it's a step back in terms of usability. I just applied the PR to my test site and even though it does require a click to generate the estimate, it "feels" like a pretty normal and intuitive experience. Also, there's caching in place, so if you're building a mailing, save and continue, then return to the mailing later, the cached estimate loads immediately. I suggest you apply the PR and test it.

[JKingsnorth]

@lcdservices I will apply the patch tomorrow and test it, and run it past our communications team.

Looking at the screencap I think there is still room for improvement, if this is the approach we want to take. For example, it looks like the text colour changes from green to red when the estimate is potentially no longer valid? This raises accessibility (using colour alone to indicate something) and usability (why did that just change colour?) concerns.

I'd be happy to do a full test of this tomorrow, running it past our comms team too, and I'll provide a review here.

Edit: There are concepts here (like caching the recipient count/list!) that I fully support [=

[JKingsnorth]

Can we try leaving in the auto-calculation, but:

• Cache the recipient number and list, so it doesn't need to rebuild when the mailing is loaded later, and every time someone clicks to preview the list of recipients.
• For some reason, one change on the 'recipients' dropdown causes previewRecipientCount to be run twice? I tested this by putting a debug statement at the top of the function.
• Increase the RECIPIENTS_DEBOUNCE_MS - it's currently 100ms, which isn't enough time for the user to do anything anyway - 3000(?) seems better. (But ideally we should still change the text to say 'Estimating...' as soon as the user makes a change).

Going further:

• The process seems inefficient anyway since it 'saves' the mailing, counts recipients, and then rolls back.

I appreciate the current solution has improved a lot but, having spoken to the comms team as well, we do still have some reservations:

• There is no user feedback after clicking 'Refresh recipient count' - we could change the text to 'Estimating...'?
• We shouldn't show inaccurate data at all - instead of turning the count red, just remove it, this would fix the accessibility and usability concerns
• We're losing out on functionality due to a technical limitation

[monishdeb]

@JKingsnorth thanks for your feedback :) Let me cover all your concern pointwise:

For some reason, one change on the 'recipients' dropdown causes previewRecipientCount to be run twice? I tested this by putting a debug statement at the top of the function.

This is not occurring due to the changes I made, it's because the crmMailing controller is called twice, tried to fix it earlier but didn't get any success.

Increase the RECIPIENTS_DEBOUNCE_MS - it's currently 100ms, which isn't enough time for the user to do anything anyway - 3000(?) seems better. (But ideally, we should still change the text to say 'Estimating...' as soon as the user makes a change).

Yup agree on 3sec lag, need to consult with @totten if there's any side effect. Will change the text during rebuild to 'Estimating...'

The process seems inefficient anyway since it 'saves' the mailing, counts recipients, and then rolls back.

And I am going to create a separate JIRA for that. The fix will involve

1. Avoid rollback by using created mailing instance, available as $scope.mailing
2. Find an efficient way to fetch recipient count/list NOT by creating mailing job
3. On recipient selection/change in any mailing attribute, which affects recipient listing will trigger respective service to fetch recipient data but using existing mailing instance available as $scope.mailing

We shouldn't show inaccurate data at all - instead of turning the count red, just remove it, this would fix the accessibility and usability concerns

What if instead of removing the count we replace it will bold red text (unknown) ?

We're losing out on functionality due to a technical limitation

How about enabling/disabling the entire cache and rebuild feature under a new CiviMail setting allow_recipient_refresh or permit_recipient_rebuild ?

[JKingsnorth]

Thanks for the comprehensive response @monishdeb

It all sounds good to me.

I like the idea that the automatic refresh could be enabled via a CiviMail setting - this keeps everyone happy (although it does add some overhead). When the auto-refresh is enabled the 'refresh' button would ideally be hidden.

[JKingsnorth]

Thanks @monishdeb . I haven't tested the patch, but the screencast you prepared looks good workflow-wise.

I think the setting would make more sense 'flipped' around to 'Enable automatic recipient counting' (ticked by default).

'Enable recipient rebuild' isn't as clear as if we say 'manual' or 'automatic' explicitly?

Thanks for the changes and work on this Monish!

[JoeMurray]

So picking up on @JKingsnorth points, let's

1. Change setting name to 'Enable automatic CiviMail recipient count display'
2. Default to True on initial install and on upgrade. This is consistent with maintaining the current behaviour, but has the downside of being a regularly expensive behaviour that negatively impacts user experience when it is slow, as it will often be. I'm of mixed feelings. @lcdservices what do you think of this suggestion? Whether we default it on or off, maybe we should add a help text indicating where it can be enabled/disabled??

[lcdservices]

@JoeMurray I agree with those points, and agree that it should default to automatic counts so that existing experience is maintained. I suspect the caching work @monishdeb did will alleviate some of the load even on automatic counts. We could also add a message during the upgrade process to alert people to the new setting.

[JKingsnorth]

Thanks everyone, this is all sounding very positive!

[monishdeb]

@lcdservices @JKingsnorth I have changed the setting title and set default to TRUE:

Can you please review my patch?

[lcdservices]

@monishdeb implemented the PR and had a few issues:

• the setting logic is reversed. with "enable automatic" checked, the button to refresh is present and the count is not automatically updated. it should be reversed.
• when the button changes to estimating it should have three periods (currently two)

I think that's it. the rest seems to be working correctly.

[monishdeb]

@lcdservices I have fixed the remaining two issues, here's the screencast http://www.screencast.com/t/AzZRt9vvik . As per changes:

1. Button has three periods instead of two i.e. Estimating...
2. I have automated the process to fetch the new setting value at a duration of 5sec. So now after changing the setting, you don't need to clear cache but need to wait for 5sec to see the effect.

[davejenx]

Hi @monishdeb, I have tested this patch on the site that I mentioned in CRM-21260, focusing on looking for any improvement in the speed of initialising the UI. (See also results reported for #11142.)
I used Chrome, using Inspector -> Network to check the time elapsed until the UI finishes loading, including the WYSIWYG: in my testing this is always the last thing to load.

Previous testing for CRM-21260 showed that the number of message templates makes a big difference. So I tested with & without the patch and with different numbers of message templates enabled (all vs just the reserved ones). 3 trials in each condition.

Results - initialisation speed

Testing 4.7.26RC-201710090252 without patch

Message templates: 31:
41s average (38s, 40s, 44s)
Total transferred: 150KB XHR / 1.3MB All
XHR requests: 22, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Message templates: 1212:
117s average (115s, 110s, 127s)
Total transferred: 150KB XHR / 1.4MB All
XHR requests: 22, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Testing 4.7.26RC-201710090252 + PR 11091

Auto recipient count update disabled.

Message templates: 31:
38s average (40s, 35s, 38s)
Total transferred: 151KB XHR / 1.3MB All
XHR requests: 24, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Message templates: 1212:
120s average (120s, 122s, 118s)
Total transferred: 151KB XHR / 1.4MB All
XHR requests: 24, biggest: 106KB builder?an=angular-modules.json
Biggest API response: 18KB (Mailing.getfields)

Results Summary - initialisation speed

Not a significant difference in the time to initialise the UI (before selecting any recipients), which is the thing I'm focusing on here. (Slightly faster with 31 templates, slightly slower with 1212.) I appreciate that the patch is mainly aimed at improving performance once the user starts selecting recipients, so these results don't go against that.

Testing the new functionality

I had a bit of a shock the first time I measured the time to populate the count with automatic CiviMail recipient count display disabled, using a static mailing group with 150k contacts: it took 28 minutes. 26 minutes of that was a ROLLBACK query. Subsequent trials were much faster though, 45-60 seconds, whether the automatic count was disabled or enabled. The civicrm_mailing_recipients table in this db has 54 million records, so I'm assuming the slow first time was due to this table warming up.

I think disabling automatic CiviMail recipient count display should reduce server load when selecting multiple large recipient groups for a mailing.

Issues encountered:

While the CiviMail compose form is loading, the count displays as ~%1 recipients . This happens whether automatic CiviMail recipient count display is enabled or disabled.

With automatic CiviMail recipient count display enabled, after selecting a recipient group there is no visual feedback that the count is being calculated, it kept showing "No recipients" until the count was populated, even though the query was in progress. Similarly if the count has populated for the first group selected and is now being re-calculated when a second group is added, there is no indication that the count is being updated. If this takes some time, which it can for large groups or complex smart groups, then the user could be misled into thinking thinking they are mailing 100 people when it's actually 150k.

When the Estimate Recipient Count button has focus / is highlighted, the top of the highlight border is hidden behind the Recipients field (Chrome on Mac). Maybe add a little spacing above the button?

[JKingsnorth]

Thanks for the detailed analysis @davejenx - the thing that stood out for me was what you found about the ROLLBACK taking ages. We've also noticed this, though I haven't run such detailed analysis on it. This is due to the way recipients are calculated (generate the recipients rows, get the count, then rollback) see https://issues.civicrm.org/jira/browse/CRM-21368.

[monishdeb]

@JKingsnorth please look at my other PR #11142 ( at L319) which removes the rollback logic from angular js. As per the patch we are not doing any rollback but refreshing the recipient list whenever angularJs services are called to fetch recipient count/list.

@davejenx thanks for your feedback but this patch doesn't improve the initialization speed. It improves the speed during mail compose, by storing the recipient data in cache. And this feature is controlled by a CiviMail component setting auto_recipient_rebuild.

[JKingsnorth]

@monishdeb Cool I'll take a look, sorry I missed this, thanks!

[davejenx]

@monishdeb Only the first half of my comment was about initialisation speed and as I said, "I appreciate that the patch is mainly aimed at improving performance once the user starts selecting recipients, so these results don't go against that."
Did you see my notes under "Testing the new functionality" and "Issues encountered" where I specifically tested the new feature with automatic CiviMail recipient count display enabled & disabled?

[monishdeb]

@davejenx sorry I didn't give reply to the "Issues encountered", after reading your comment I found three issues as per your feedback, let me cover each pointwise:

1. While the CiviMail compose form is loading, the count displays as ~%1 recipients . This happens whether automatic CiviMail recipient count display is enabled or disabled.

I have fixed this issue and this is the commit -79e7971 . So now if the setting is disabled then it won't show ~%1 recipients but now as per fix it will (unknown).

2. With automatic CiviMail recipient count display enabled, after selecting a recipient group there is no visual feedback that the count is being calculated, it kept showing "No recipients" until the count was populated

Unfortunately, I wasn't able to fix this yet. Will keep trying.

3. When the Estimate Recipient Count button has focus / is highlighted, the top of the highlight border is hidden behind the Recipients field (Chrome on Mac). Maybe add a little spacing above the button?

This is as per how other buttons are rendered just below the select2/text (like 'Send test' button below) field I kept the same format but I agree with you that a little space above the button would be cleaner to represent. Will bring this changes too.

[mlutfy]

Can you ts() this string?

[mlutfy]

And this one too, even if it was not added by you. :)

[monishdeb]

@mlutfy I have added ts(). Thanks for notifying :) Is there anything else that needs to be done to get this PR merged?

[mlutfy]

Thanks for fixing the ts() strings, @monishdeb

Merging based on the reviews so far by @davejenx @JKingsnorth @seamuslee001 @JoeMurray and @lcdservices also mentioning that they are running this patch in production.

[spalmstr]

See also #** (dev/mail/4) Recipient groups in a scheduled mail return no recipients.**, which appears to affect non-US language systems.