Skip to content

Commit

Permalink
Merge pull request #15815 from artfulrobot/issue-1108-fix-unsubscribe
Browse files Browse the repository at this point in the history 
dev/core#1108 Fix unsubscribe bug
  • Loading branch information
@seamuslee001
seamuslee001 authored Nov 17, 2019
2 parents 08ff628 + 1e63ea1 commit 5ec753e
Show file tree
Hide file tree
Showing 2 changed files with 243 additions and 69 deletions.
129 changes: 60 additions & 69 deletions CRM/Mailing/Event/BAO/Unsubscribe.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,75 +124,66 @@ public static function &unsub_from_mailing($job_id, $queue_id, $hash, $return =
$contact_id = $q->contact_id;
$transaction = new CRM_Core_Transaction();

$do = new CRM_Core_DAO();
$mgObject = new CRM_Mailing_DAO_MailingGroup();
$mg = $mgObject->getTableName();
$jobObject = new CRM_Mailing_BAO_MailingJob();
$job = $jobObject->getTableName();
$mailingObject = new CRM_Mailing_BAO_Mailing();
$mailing = $mailingObject->getTableName();
$groupObject = new CRM_Contact_BAO_Group();
$group = $groupObject->getTableName();
$gcObject = new CRM_Contact_BAO_GroupContact();
$gc = $gcObject->getTableName();
$abObject = new CRM_Mailing_DAO_MailingAB();
$ab = $abObject->getTableName();

$mailing_id = civicrm_api3('MailingJob', 'getvalue', ['id' => $job_id, 'return' => 'mailing_id']);
$mailing_type = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_Mailing', $mailing_id, 'mailing_type', 'id');
$entity = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_MailingGroup', $mailing_id, 'entity_table', 'mailing_id');

// If $entity is null and $mailing_Type is either winner or experiment then we are deailing with an AB test
$abtest_types = ['experiment', 'winner'];
if (empty($entity) && in_array($mailing_type, $abtest_types)) {
$mailing_id_a = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_MailingAB', $mailing_id, 'mailing_id_a', 'mailing_id_b');
$field = 'mailing_id_b';
if (empty($mailing_id_a)) {

$groupObject = new CRM_Contact_BAO_Group();
$groupTableName = $groupObject->getTableName();

$mailingObject = new CRM_Mailing_BAO_Mailing();
$mailingTableName = $mailingObject->getTableName();

// We need a mailing id that points to the mailing that defined the recipients.
// This is usually just the passed-in mailing_id, however in the case of AB
// tests, it's the variant 'A' one.
$relevant_mailing_id = $mailing_id;

// Special case for AB Tests:
if (in_array($mailing_type, ['experiement', 'winner'])) {
// The mailing belongs to an AB test.
// See if we can find an AB test where this is variant B.
$mailing_id_a = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_MailingAB', mailing_id, 'mailing_id_a', 'mailing_id_b');
if (!empty($mailing_id_a)) {
// OK, we were given mailing B and we looked up variant A which is the relevant one.
$relevant_mailing_id = $mailing_id_a;
}
else {
// No, it wasn't variant B, let's see if we can find an AB test where
// the given mailing was the winner (C).
$mailing_id_a = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_MailingAB', $mailing_id, 'mailing_id_a', 'mailing_id_c');
$field = 'mailing_id_c';
if (!empty($mailing_id_a)) {
// OK, this was the winner and we looked up variant A which is the relevant one.
$relevant_mailing_id = $mailing_id_a;
}
// (otherwise we were passed in variant A so we already have the relevant_mailing_id correct already.)
}
$jobJoin = "INNER JOIN $ab ON $ab.mailing_id_a = $mg.mailing_id
INNER JOIN $job ON $job.mailing_id = $ab.$field";
$entity = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_MailingGroup', $mailing_id_a, 'entity_table', 'mailing_id');
}
else {
$jobJoin = "INNER JOIN $job ON $job.mailing_id = $mg.mailing_id";
}

$groupClause = '';
if ($entity == $group) {
$groupClause = "AND $group.is_hidden = 0";
}

$do->query("
SELECT $mg.entity_table as entity_table,
$mg.entity_id as entity_id,
$mg.group_type as group_type
FROM $mg
$jobJoin
INNER JOIN $entity
ON $mg.entity_id = $entity.id
WHERE $job.id = " . CRM_Utils_Type::escape($job_id, 'Integer') . "
AND $mg.group_type IN ('Include', 'Base') $groupClause"
);

// Make a list of groups and a list of prior mailings that received
// this mailing.
// Make a list of groups and a list of prior mailings that received this
// mailing. Nb. the 'Base' group is called the 'Unsubscribe group' in the
// UI.
// Just to definitely make it SQL safe.
$relevant_mailing_id = (int) $relevant_mailing_id;
$do = CRM_Core_DAO::executeQuery(
"SELECT entity_table, entity_id, group_type
FROM civicrm_mailing_group
WHERE mailing_id = $relevant_mailing_id
AND group_type IN ('Include', 'Base')");

$groups = [];
$base_groups = [];
$mailings = [];

while ($do->fetch()) {
if ($do->entity_table == $group) {
if ($do->entity_table === $groupTableName) {
if ($do->group_type == 'Base') {
$base_groups[$do->entity_id] = NULL;
}
else {
$groups[$do->entity_id] = NULL;
}
}
elseif ($do->entity_table == $mailing) {
elseif ($do->entity_table === $mailingTableName) {
$mailings[] = $do->entity_id;
}
}
Expand All @@ -202,19 +193,19 @@ public static function &unsub_from_mailing($job_id, $queue_id, $hash, $return =

while (!empty($mailings)) {
$do = CRM_Core_DAO::executeQuery("
SELECT $mg.entity_table as entity_table,
$mg.entity_id as entity_id
FROM civicrm_mailing_group $mg
WHERE $mg.mailing_id IN (" . implode(', ', $mailings) . ")
AND $mg.group_type = 'Include'");
SELECT entity_table as entity_table,
entity_id as entity_id
FROM civicrm_mailing_group
WHERE mailing_id IN (" . implode(', ', $mailings) . ")
AND group_type = 'Include'");

$mailings = [];

while ($do->fetch()) {
if ($do->entity_table == $group) {
if ($do->entity_table === $groupTableName) {
$groups[$do->entity_id] = TRUE;
}
elseif ($do->entity_table == $mailing) {
elseif ($do->entity_table === $mailing) {
$mailings[] = $do->entity_id;
}
}
Expand All @@ -233,24 +224,24 @@ public static function &unsub_from_mailing($job_id, $queue_id, $hash, $return =
// base groups from search based mailings.
$baseGroupClause = '';
if (!empty($baseGroupIds)) {
$baseGroupClause = "OR $group.id IN(" . implode(', ', $baseGroupIds) . ")";
$baseGroupClause = "OR grp.id IN(" . implode(', ', $baseGroupIds) . ")";
}
$groupIdClause = '';
if ($groupIds || $baseGroupIds) {
$groupIdClause = "AND $group.id IN (" . implode(', ', array_merge($groupIds, $baseGroupIds)) . ")";
$groupIdClause = "AND grp.id IN (" . implode(', ', array_merge($groupIds, $baseGroupIds)) . ")";
}
$do = CRM_Core_DAO::executeQuery("
SELECT $group.id as group_id,
$group.title as title,
$group.description as description
FROM civicrm_group $group
LEFT JOIN civicrm_group_contact $gc
ON $gc.group_id = $group.id
WHERE $group.is_hidden = 0
SELECT grp.id as group_id,
grp.title as title,
grp.description as description
FROM civicrm_group grp
LEFT JOIN civicrm_group_contact gc
ON gc.group_id = grp.id
WHERE grp.is_hidden = 0
$groupIdClause
AND ($group.saved_search_id is not null
OR ($gc.contact_id = $contact_id
AND $gc.status = 'Added')
AND (grp.saved_search_id is not null
OR (gc.contact_id = $contact_id
AND gc.status = 'Added')
$baseGroupClause
)");

Expand Down
183 changes: 183 additions & 0 deletions tests/phpunit/CRM/Mailing/MailingSystemTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ public function hook_alterMailParams(&$params, $context = NULL) {
}

public function tearDown() {
global $dbLocale;
if ($dbLocale) {
CRM_Core_I18n_Schema::makeSinglelingual('en_US');
}
parent::tearDown();
$this->assertNotEmpty($this->counts['hook_alterMailParams']);
}
Expand Down Expand Up @@ -114,4 +118,183 @@ public function testMailingActivityCreate() {
], 1);
}

/**
* Data provider for testGitLabIssue1108
*
* First we run it without multiLingual mode, then with.
*
* This is because we test table names, which may have been translated in a
* multiLingual context.
*
*/
public function multiLingual() {
return [[0], [1]];
}

/**
* - unsubscribe used dodgy SQL that only checked half of the polymorphic
* relationship in mailing_group, meaning it could match 'mailing 123'
* against _group_ 123.
*
* - also, an INNER JOIN on the group table hid the mailing-based
* mailing_group records.
*
* - in turn this inner join meant the query returned nothing, which then
* caused the code that is supposed to find the contact within those groups
* to basically find all the groups that the contact in or were smart groups.
*
* - in certain situations (which I have not been able to replicate in this
* test) it caused the unsubscribe to fail to find *any* groups to unsubscribe
* people from, thereby breaking the unsubscribe.
*
* @dataProvider multiLingual
*
*/
public function testGitLabIssue1108($isMultiLingual) {

// We need to make sure the mailing IDs are higher than the groupIDs.
// We do this by adding mailings until the mailing.id value is at least 10
// higher than the highest group.id
// Note that creating a row in a transaction then rolling back the
// transaction still increments the AUTO_INCREMENT counter for the table.
// (If this behaviour ever changes we throw an exception.)
if ($isMultiLingual) {
$this->enableMultilingual();
}
$max_group_id = CRM_Core_DAO::singleValueQuery("SELECT MAX(id) FROM civicrm_group");
$max_mailing_id = 0;
while ($max_mailing_id < $max_group_id + 10) {
CRM_Core_Transaction::create()->run(function($tx) use (&$max_mailing_id) {
CRM_Core_DAO::executeQuery("INSERT INTO civicrm_mailing (name) VALUES ('dummy');");
$_ = (int) CRM_Core_DAO::singleValueQuery("SELECT MAX(id) FROM civicrm_mailing");
if ($_ === $max_mailing_id) {
throw new RuntimeException("Expected that creating a new row would increment ID, but it did not. This could be a change in MySQL's implementation of rollback");
}
$max_mailing_id = $_;
$tx->rollback();
});
}

// Because our parent class marks the _groupID as private, we can't use that :-(
$group_1 = $this->groupCreate([
'name' => 'Test Group 1108.1',
'title' => 'Test Group 1108.1',
]);
$this->createContactsInGroup(2, $group_1);

// Also _mut is private to the parent, so we have to make our own:
$mut = new CiviMailUtils($this, TRUE);

// Create initial mailing to the group.
$mailingParams = [
'name' => 'Issue 1108: mailing 1',
'subject' => 'Issue 1108: mailing 1',
'created_id' => 1,
'groups' => ['include' => [$group_1]],
'scheduled_date' => 'now',
'body_text' => 'Please just {action.unsubscribe}',
];

// The following code is exactly the same as runMailingSuccess() except that we store the ID of the mailing.
$mailing_1 = $this->callAPISuccess('mailing', 'create', $mailingParams);
$mut->assertRecipients(array());
$this->callAPISuccess('job', 'process_mailing', array('runInNonProductionEnvironment' => TRUE));

$allMessages = $mut->getAllMessages('ezc');
// There are exactly two contacts produced by setUp().
$this->assertEquals(2, count($allMessages));

// We need a new group
$group_2 = $this->groupCreate([
'name' => 'Test Group 1108.2',
'title' => 'Test Group 1108.2',
]);

// Now create the 2nd mailing to the recipients of the first,
// excluding our new albeit empty group.
$mailingParams = [
'name' => 'Issue 1108: mailing 2',
'subject' => 'Issue 1108: mailing 2',
'created_id' => 1,
'mailings' => ['include' => [$mailing_1['id']]],
'groups' => ['exclude' => [$group_2]],
'scheduled_date' => 'now',
'body_text' => 'Please just {action.unsubscribeUrl}',
];
$this->callAPISuccess('mailing', 'create', $mailingParams);
$_ = $this->callAPISuccess('job', 'process_mailing', array('runInNonProductionEnvironment' => TRUE));

$allMessages = $mut->getAllMessages('ezc');
// We should have 2+2 messages sent by the mail system now.
$this->assertEquals(4, count($allMessages));

// So far so good.
// Now extract the unsubscribe details.
$message = end($allMessages);
$this->assertTrue($message->body instanceof ezcMailText);
$this->assertEquals('plain', $message->body->subType);
$this->assertEquals(1, preg_match(
'@mailing/unsubscribe.*jid=(\d+)&qid=(\d+)&h=([0-9a-z]+)@',
$message->body->text,
$matches
));

// Create a group that has nothing to do with this mailing.
$group_3 = $this->groupCreate([
'name' => 'Test Group 1108.3',
'title' => 'Test Group 1108.3',
]);
// Add contacts from group 1 to group 3.
$gcQuery = new CRM_Contact_BAO_GroupContact();
$gcQuery->group_id = $group_1;
$gcQuery->status = 'Added';
$gcQuery->find();
while ($gcQuery->fetch()) {
$this->callAPISuccess('group_contact', 'create',
['group_id' => $group_3, 'contact_id' => $gcQuery->contact_id, 'status' => 'Added']);
}

// Part of the issue is caused by the fact that (at time of writing) the
// SQL joined the mailing_group table on just the entity_id, assuming it to
// be a group, but actually it could be a mailing.
// The difficulty in testing this is that because all our IDs are very low
// and contiguous the SQL looking for a match for 'mailing 1' does match a
// group ID of '1', which is created in this class's parent's setUp().
// Strictly speaking we don't know that it has ID 1, but as we can't access _groupID
// we'll have to assume that.
//
// So by deleting that group the SQL then matches nothing which is what we
// need for this case.
$_ = new CRM_Contact_BAO_Group();
$_->id = 1;
$_->delete();

$hooks = \CRM_Utils_Hook::singleton();
$found = [];
$hooks->setHook('civicrm_unsubscribeGroups',
function ($op, $mailingId, $contactId, &$groups, &$baseGroups) use (&$found) {
$found['groups'] = $groups;
$found['baseGroups'] = $baseGroups;
});

// Now test unsubscribe groups.
$groups = CRM_Mailing_Event_BAO_Unsubscribe::unsub_from_mailing(
$matches[1],
$matches[2],
$matches[3],
TRUE
);

// We expect that our group_1 was found.
$this->assertEquals(['groups' => [$group_1], 'baseGroups' => []], $found);

// We *should* get an array with just our $group_1 since this is the only group
// that we have included.
// $group_2 was only used to exclude people.
// $group_3 has nothing to do with this mailing and should not be there.
$this->assertNotEmpty($groups, "We should have received an array.");
$this->assertEquals([$group_1], array_keys($groups),
"We should have received an array with our group 1 in it.");
}

}

0 comments on commit 5ec753e

Please sign in to comment.