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.

Sign up for GitHub

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

Jump to bottom

dev/core#1108 Fix unsubscribe bug #15815

Merged
merged 2 commits into from
Nov 17, 2019
Merged

dev/core#1108 Fix unsubscribe bug #15815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76d57ff
Unsubscribe bug: Add failing test for Gitlab Issue 1108
Nov 15, 2019
1e63ea1
Unsubscribe bug: Fix gitlab issue 1108 (and clean up old db code)
Nov 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 60 additions & 69 deletions CRM/Mailing/Event/BAO/Unsubscribe.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,75 +140,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(
artfulrobot marked this conversation as resolved.
Show resolved Hide resolved
"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 @@ -218,19 +209,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 @@ -249,24 +240,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,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 @@ -130,4 +134,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.");
}

}