dev/core#448 Fix issue where when building mailings with smart groups, removed members of the smart group were being included

[seamuslee001]

Overview

When using a smart group as a mailing list, users who unsubscribe from the smart group are still included in the mailing.

Before

When creating a mailing using a smart group as a mailing list, users who have previously unsubscribed from the smart group mailing list will still receive the mailing.

No Unit Test on this particular part of the recipient building logic

After

Users who have unsubscribed form smart group mailing lists and are marked as removed in the civicrm_group_contact table, no longer receive the mailings.

Unit test exists

Technical Details

In CRM/Mailing/BAO/Mailing.php the query below doesn't join to the civicrm_group_contact to check if the recipient has been removed.

if (count($includeSmartGroupIDs)) {
      $query = CRM_Utils_SQL_Select::from($contact)
        ->select("$contact.id as contact_id, $entityTable.id as $entityColumn")
        ->join($entityTable, " INNER JOIN $entityTable ON $entityTable.contact_id = $contact.id ")
        ->join('gc', " INNER JOIN civicrm_group_contact_cache gc ON $contact.id = gc.contact_id ")
        ->join('mg', " INNER JOIN civicrm_mailing_group mg  ON  gc.group_id = mg.entity_id AND mg.search_id IS NULL ")
        ->join('temp', " LEFT JOIN $excludeTempTablename temp ON $contact.id = temp.contact_id ")
        ->where('gc.group_id IN (#groups)')
        ->merge($criteria)
        ->replaceInto($includedTempTablename, array('contact_id', $entityColumn))
        ->param('#groups', $includeSmartGroupIDs)
        ->param('#mailingID', $mailingID)
        ->execute();
    }

Comments

This PR replaces #12943 which was against the wrong branch & had no test but had the correct fix, which has been cherry-picked in.

This appears to be a semi-recent regression - having seemingly worked in 5.2

ping @eileenmcnaughton @monishdeb

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

also pinging @johntwyman @andrew-cormick-dockery as you two will want to be on top of this (surprised we haven't seen this surface yet)

[seamuslee001]

Jenkins re test this please

[jusfreeman]

@seamuslee001 I think you mean "deliberately" - not "deliberly"

[eileenmcnaughton]

or he was slurring....

[jusfreeman]

@seamuslee001 any idea when this started happening? Was this bug recently introduced by some other change?

[eileenmcnaughton]

@jusfreeman the bug seems to have been introduced in 5.3 or earlier.

[eileenmcnaughton]

Test looks good. The fix by @tmannell makes sense & has been tested by @seamuslee001 in creating the test - merging

[eileenmcnaughton]

@seamuslee001 I just merged & then realised we talked about this going agaiinst the rc!