Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#3890 Rebuild smart groups included in mailing on scheduling #24681

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented Oct 5, 2022

Overview

Smart group recipients should be totally up to date when a mailing is scheduled.

Before

Smart group recipients for a mailing were based on the smart group cache, potentially resulting in out of date recipients.

After

Smart groups that are included or excluded from a mailing are rebuilt at time of scheduling, so recipients are fully up to date. Parent groups are rebuilt as well. This also includes hidden smart groups (mailings created from search results using All NNN Contacts).

Technical Details

This may not look like the most logical place for this to happen (but if this is in getRecipients, it will run a dozen times by the time a mailing is created and sent) or the simplest way to go about it (you'd think the params we'd need would be there - they are earlier, but they aren't at scheduling time).

@civibot
Copy link

civibot bot commented Oct 5, 2022

(Standard links)

@civibot civibot bot added the master label Oct 5, 2022
@larssandergreen larssandergreen force-pushed the force-refresh-cache-for-smart-groups-when-scheduling-mailing branch 2 times, most recently from c08b5c4 to 6eefa69 Compare October 6, 2022 01:41
@larssandergreen larssandergreen marked this pull request as draft October 6, 2022 03:49
@larssandergreen larssandergreen changed the title dev/core#3890 Rebuild smart groups included in mailing on scheduling WIP: dev/core#3890 Rebuild smart groups included in mailing on scheduling Oct 6, 2022
@larssandergreen larssandergreen force-pushed the force-refresh-cache-for-smart-groups-when-scheduling-mailing branch from 94e8977 to a6ac412 Compare October 6, 2022 04:04
@larssandergreen larssandergreen marked this pull request as ready for review October 6, 2022 05:48
@larssandergreen larssandergreen changed the title WIP: dev/core#3890 Rebuild smart groups included in mailing on scheduling dev/core#3890 Rebuild smart groups included in mailing on scheduling Oct 6, 2022
@agileware-justin
Copy link
Contributor

Smart groups that are included or excluded from a mailing are rebuilt at time of scheduling, so recipients are fully up to date.

@larssandergreen thanks very much for working on this issue. Would it be possible to move the rebuild recipients list to just before the mailing is sent, rather than scheduled? The reason being that end users expect that the included / excluded mailing groups will be used at the time the mailing is sent, not when it was scheduled.

This is particularly relevant if you schedule the mailing to be sent few days / weeks / months - there can be a LOT of changes to the included / excluded mailing groups. And not using the latest recipient list can be very problematic.

For example: monthly membership newsletter is scheduled on 14th and will be sent out on the 21st to all current members. 50 new members join between the 14th and 20th. On the 21st the newsletter is sent, those 50 new members do not receive the newsletter and complain to the management committee. The management committee have reduced confidence that CiviCRM is functioning correctly.

There is an extension available which did solve this CiviCRM problem, however this should really be a core feature change not an option, https://github.com/3sd/civicrm-recalculate-recipients

Thanks for listening 🎧

@seamuslee001
Copy link
Contributor

@agileware-justin whilst that probably would work with most systems (because in most systems everyone has visibility of all contacts), if you had a system that had a set of ACLs it might be a bit more difficult because I presume the user that would be triggering the re-calculation at sending time would likely be the cron user and not the possibly ACLed user that scheduled the mailing so the 'which contacts can this user see' becomes a problem.

@larssandergreen
Copy link
Contributor Author

@agileware-justin I totally agree with you that recipients should be determined at sending time. More than one person in my organization has been pretty surprised when I explained that's not how this works.

It does seem like a somewhat controversial issue though and every time it is discussed, it seems to gather a bunch of different opinions on what should happen (even in the issue for this change). I'd welcome a full discussion on how this should work and if there's agreement to change it, would be willing to help out with that. It does seem like at the end of the day, most people probably expect the recipients to be calculated at send time and we should meet that expectation. Leaving smart groups aside, it's even strange that if you remove a bunch of contacts from a group after scheduling but before sending, they will still receive a mailing.

But this change here in this PR is pretty simple, just rebuilding the smart group caches for the relevant groups right before building the recipients list. If the building of the recipients list is changed from scheduling to sending time, then this change should be moved along with it.

@larssandergreen
Copy link
Contributor Author

@seamuslee001 We could, in theory, manually add the appropriate where clauses to the queries that select the recipients based on the approver_id for the mailing.

@agileware-justin
Copy link
Contributor

agileware-justin commented Oct 7, 2022

whilst that probably would work with most systems (because in most systems everyone has visibility of all contacts), if you had a system that had a set of ACLs it might be a bit more difficult because I presume the user that would be triggering the re-calculation at sending time would likely be the cron user and not the possibly ACLed user that scheduled the mailing so the 'which contacts can this user see' becomes a problem.

I guess you're referring to the Australian Greens here, in which case sure I understand their specific requirement for restricting mailings.

Storing the mailing user id and/or mailing approver id and using this for querying the recipients at the time of send would be the logical change here. Sure you've thought of that too @seamuslee001

@tschuettler
Copy link
Contributor

@agileware-justin there is a dedicated issue for this: https://lab.civicrm.org/dev/core/-/issues/2351 following an earlier discussion at: https://lists.civicrm.org/lists/arc/civicrm-dev/2017-12/msg00012.html
There are legitimate use cases for both recalculating at time of scheduling and at time of sending the mail.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@larssandergreen
Copy link
Contributor Author

jenkins test this please

@larssandergreen
Copy link
Contributor Author

@agileware-justin @seamuslee001 @tschuettler Anyone able to give this a review so we can get it merged? Just to recap, there has been some discussion here about the age-old question of when the mailing recipients should be calculated, but all this PR does is make sure the smart groups are up to date when the recipients are calculated, to avoid cached smart groups causing unexpected recipients. This doesn't change when the recipients are calculated.

@agileware-justin
Copy link
Contributor

@larssandergreen yep can do next week. Will have some free time to review.

@agileware-justin
Copy link
Contributor

Tested looks good to me, thanks @larssandergreen

Happy dance

CiviCRM Review Standards

  • General standards
    • (r-explain) Pass
    • (r-user) Pass:
      Behaviour matches the CiviCRM End User documentation, see https://docs.civicrm.org/user/en/latest/email/mass-mailings-using-civimail/#choosing-recipients-when-mailing-recipients-are-set
    • (r-doc) Pass
    • (r-run) Pass:
      Test 1: Created a Mailing, set Recipients based on Smart Group. Scheduled Mailing. Changed some Contacts to meet Smart Group criteria. When Mailing was sent the new Contacts did NOT receive the email. This is correct (but I do not like!)
      Test 2: Created a Mailing, set Recipients based on Smart Group. Saved Mailing. Changed some Contacts to meet Smart Group criteria. When Mailing was Scheduled the Smart Group was re-calculated and included the new Contacts. When Mailing was sent the new Contacts received the email. This is correct.
  • Developer standards
    • (r-tech) Pass:
      The change reflects the end user documentation, so it is likely already assumed by CiviCRM Developers and CiviCRM End Users alike that this is the current behaviour.
    • (r-code) Pass
    • (r-maint) Pass
    • (r-test) Pass

@agileware-justin
Copy link
Contributor

@eileenmcnaughton ready for you to do what you do best 😄

@larssandergreen
Copy link
Contributor Author

Thanks for the review, @agileware-justin!

@eileenmcnaughton
Copy link
Contributor

Thanks @larssandergreen & @agileware-justin

@eileenmcnaughton eileenmcnaughton merged commit 70ed595 into civicrm:master Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants