From 76d57ffcc6ace48693bf711fcc998e847167ee8d Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Fri, 15 Nov 2019 07:38:41 +0000 Subject: [PATCH 1/2] Unsubscribe bug: Add failing test for Gitlab Issue 1108 --- .../phpunit/CRM/Mailing/MailingSystemTest.php | 183 ++++++++++++++++++ 1 file changed, 183 insertions(+) diff --git a/tests/phpunit/CRM/Mailing/MailingSystemTest.php b/tests/phpunit/CRM/Mailing/MailingSystemTest.php index 571140c0b0a0..de2f17308f59 100644 --- a/tests/phpunit/CRM/Mailing/MailingSystemTest.php +++ b/tests/phpunit/CRM/Mailing/MailingSystemTest.php @@ -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']); } @@ -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."); + } + } From 1e63ea14b964258ac227c988e580df4122ed9a30 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Fri, 15 Nov 2019 07:39:02 +0000 Subject: [PATCH 2/2] Unsubscribe bug: Fix gitlab issue 1108 (and clean up old db code) --- CRM/Mailing/Event/BAO/Unsubscribe.php | 129 ++++++++++++-------------- 1 file changed, 60 insertions(+), 69 deletions(-) diff --git a/CRM/Mailing/Event/BAO/Unsubscribe.php b/CRM/Mailing/Event/BAO/Unsubscribe.php index 82537a650af9..628e689efdaf 100644 --- a/CRM/Mailing/Event/BAO/Unsubscribe.php +++ b/CRM/Mailing/Event/BAO/Unsubscribe.php @@ -140,67 +140,58 @@ 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; } @@ -208,7 +199,7 @@ public static function &unsub_from_mailing($job_id, $queue_id, $hash, $return = $groups[$do->entity_id] = NULL; } } - elseif ($do->entity_table == $mailing) { + elseif ($do->entity_table === $mailingTableName) { $mailings[] = $do->entity_id; } } @@ -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; } } @@ -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 )");