dev/core#1108 Fix unsubscribe bug #15815

artfulrobot · 2019-11-11T15:40:52Z

Overview

This is detailed at https://lab.civicrm.org/dev/core/issues/1108

Before

Some people would sometimes be unable to unsubscribe due to SQL bugs. Test CRM/Mailing/MailingSystemTest.php has a test for this issue to prove brokenness.

After

Unsubscrbe works again.

civibot · 2019-11-11T15:40:55Z

(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).

tests/phpunit/CRM/Mailing/MailingSystemTest.php

CRM/Mailing/Event/BAO/Unsubscribe.php

seamuslee001 · 2019-11-12T20:06:19Z

CRM/Mailing/Event/BAO/Unsubscribe.php


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

    while ($do->fetch()) {
-      if ($do->entity_table == $group) {
+      if ($do->entity_table == 'civicrm_group') {


this strikes me a potentially problematic

@seamuslee001 why - we save the table as entity_table in that field not the internationalised version don't we?

@eileenmcnaughton nope its the internationalised version posting comment shortly

@seamuslee001 ug - we shouldn't - that isn't a pattern anywhere else. We should fix on upgrade

its been like that for ages, hence why we have put in mulitlingual testing into api/v3/mailing etc. it would be better to sort it long term but that will take some hand holding as there are a number of places in the mailing code where it assumes the internationalised table name is stored

Wow - it's an ongoing problem isn't it :-(

Still for this PR we can make all the other changes except for that & it will still clean it up a lot

Yep agreed and if we make the test changes as per my comment below then we will have test coverage in single and mulitlingual mode for whenever we get around to doing the cleanup work

Does it make sense for you to do a PR against this branch?

done here @eileenmcnaughton artfulrobot#1

seamuslee001 · 2019-11-12T20:21:18Z

@artfulrobot @eileenmcnaughton If you make the following changes in your test

diff --git a/tests/phpunit/CRM/Mailing/MailingSystemTest.php b/tests/phpunit/CRM/Mailing/MailingSystemTest.php
index ab1d28c165..049dba67c8 100644
--- a/tests/phpunit/CRM/Mailing/MailingSystemTest.php
+++ b/tests/phpunit/CRM/Mailing/MailingSystemTest.php
@@ -72,6 +72,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
   }

   public function tearDown() {
+    global $dbLocale;
+    if ($dbLocale) {
+      CRM_Core_I18n_Schema::makeSinglelingual('en_US');
+    }
     parent::tearDown();
     $this->assertNotEmpty($this->counts['hook_alterMailParams']);
   }
@@ -130,6 +134,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
     ], 1);
   }

+  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'
@@ -145,9 +153,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
    * - 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() {
+  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
@@ -155,6 +164,9 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
     // 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) {

you will note that when it enables the Multilingual it fails the test because we store the internationalised name of the table in the database in entity_table because reasons

So the way i fixed it locally was applying this

diff --git a/CRM/Mailing/Event/BAO/Unsubscribe.php b/CRM/Mailing/Event/BAO/Unsubscribe.php
index 1bc8995138..36499300e4 100644
--- a/CRM/Mailing/Event/BAO/Unsubscribe.php
+++ b/CRM/Mailing/Event/BAO/Unsubscribe.php
@@ -143,6 +143,10 @@ WHERE  email = %2
     $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');

+    $groupObject = new CRM_Contact_BAO_Group();
+    $group = $groupObject->getTableName();
diff --git a/CRM/Mailing/Event/BAO/Unsubscribe.php b/CRM/Mailing/Event/BAO/Unsubscribe.php
index 1bc8995138..36499300e4 100644
--- a/CRM/Mailing/Event/BAO/Unsubscribe.php
+++ b/CRM/Mailing/Event/BAO/Unsubscribe.php
@@ -143,6 +143,10 @@ WHERE  email = %2
     $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');

+    $groupObject = new CRM_Contact_BAO_Group();
+    $group = $groupObject->getTableName();
+    $mailingObject = new CRM_Mailing_BAO_Mailing();
+    $mailing = $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.
@@ -185,7 +189,7 @@ WHERE  email = %2
     $mailings = [];

     while ($do->fetch()) {
-      if ($do->entity_table == 'civicrm_group') {
+      if ($do->entity_table === $group) {
         if ($do->group_type == 'Base') {
           $base_groups[$do->entity_id] = NULL;
         }
@@ -193,7 +197,7 @@ WHERE  email = %2
           $groups[$do->entity_id] = NULL;
         }
       }
-      elseif ($do->entity_table == 'civicrm_mailing') {
+      elseif ($do->entity_table === $mailing) {
         $mailings[] = $do->entity_id;
       }
     }
@@ -212,10 +216,10 @@ WHERE  email = %2
       $mailings = [];

       while ($do->fetch()) {
-        if ($do->entity_table == 'civicrm_group') {
+        if ($do->entity_table === $group) {
           $groups[$do->entity_id] = TRUE;
         }
-        elseif ($do->entity_table == 'civicrm_mailing') {
+        elseif ($do->entity_table === $mailing) {
           $mailings[] = $do->entity_id;
         }
       }

artfulrobot · 2019-11-12T23:04:20Z

Hee hee, so not completely, but nearly back to what it was! Ok, I can apply those changes. At CiviCamp London tomorrow, not sure when I'll get time.

eileenmcnaughton · 2019-11-13T00:29:56Z

@artfulrobot right so use the variable when it's saved to the table (value in entiy_table) because ... yuck. But use the table name in the FROM clause

CRM/Mailing/Event/BAO/Unsubscribe.php

artfulrobot · 2019-11-13T08:28:00Z

@seamuslee001 thanks for showing me the multilingual stuff.

Seems like it might have been a better decision to stick to a single language for table names (as we have to do for the code, for example). But I do say that as a privileged English speaker.

Anyway, resubmitted the 2 commits. I didn't squah them so that if needed someone could run the tests agains the old code, but let me know if you want them squishing.

tests/phpunit/CRM/Mailing/MailingSystemTest.php

eileenmcnaughton · 2019-11-17T02:02:14Z

@seamuslee001 is this good to go now?

seamuslee001 · 2019-11-17T02:04:51Z

Yes and when AUG last had issues Jitendra added a unit test also which is here https://github.com/civicrm/civicrm-core/pull/10165/files so that verifies that this doesn't break that so i'm good with this now merging

artfulrobot · 2019-11-17T19:41:02Z

Thanks

civibot bot added the master label Nov 11, 2019

artfulrobot force-pushed the issue-1108-fix-unsubscribe branch from f687c32 to ff3ff16 Compare November 11, 2019 15:44

seamuslee001 reviewed Nov 11, 2019

View reviewed changes

tests/phpunit/CRM/Mailing/MailingSystemTest.php Outdated Show resolved Hide resolved

eileenmcnaughton reviewed Nov 12, 2019

View reviewed changes

CRM/Mailing/Event/BAO/Unsubscribe.php Outdated Show resolved Hide resolved

artfulrobot force-pushed the issue-1108-fix-unsubscribe branch from ff3ff16 to be54484 Compare November 12, 2019 09:54

seamuslee001 reviewed Nov 12, 2019

View reviewed changes

artfulrobot force-pushed the issue-1108-fix-unsubscribe branch from be54484 to 488e72a Compare November 13, 2019 08:24

eileenmcnaughton reviewed Nov 13, 2019

View reviewed changes

CRM/Mailing/Event/BAO/Unsubscribe.php Show resolved Hide resolved

seamuslee001 reviewed Nov 15, 2019

View reviewed changes

tests/phpunit/CRM/Mailing/MailingSystemTest.php Outdated Show resolved Hide resolved

Rich Lott / Artful Robot added 2 commits November 15, 2019 07:38

Unsubscribe bug: Add failing test for Gitlab Issue 1108

76d57ff

Unsubscribe bug: Fix gitlab issue 1108 (and clean up old db code)

1e63ea1

artfulrobot force-pushed the issue-1108-fix-unsubscribe branch from 488e72a to 1e63ea1 Compare November 15, 2019 07:39

mlutfy changed the title ~~fix unsubscribe bug Issue #1108~~ dev/core#1108 Fix unsubscribe bug Nov 15, 2019

seamuslee001 merged commit 5ec753e into civicrm:master Nov 17, 2019

agileware-justin mentioned this pull request Aug 24, 2021

CiviCRM Mailing, function unsub_from_mailing has spelling error, "experiement" impacts A/B Mailing unsubscribes #21245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#1108 Fix unsubscribe bug #15815

dev/core#1108 Fix unsubscribe bug #15815

artfulrobot commented Nov 11, 2019

civibot bot commented Nov 11, 2019

seamuslee001 Nov 12, 2019

eileenmcnaughton Nov 12, 2019

seamuslee001 Nov 12, 2019

eileenmcnaughton Nov 12, 2019

seamuslee001 Nov 12, 2019

eileenmcnaughton Nov 12, 2019

eileenmcnaughton Nov 12, 2019

seamuslee001 Nov 12, 2019

eileenmcnaughton Nov 12, 2019

seamuslee001 Nov 12, 2019

seamuslee001 commented Nov 12, 2019

artfulrobot commented Nov 12, 2019

eileenmcnaughton commented Nov 13, 2019

artfulrobot commented Nov 13, 2019

eileenmcnaughton commented Nov 17, 2019

seamuslee001 commented Nov 17, 2019

artfulrobot commented Nov 17, 2019

dev/core#1108 Fix unsubscribe bug #15815

dev/core#1108 Fix unsubscribe bug #15815

Conversation

artfulrobot commented Nov 11, 2019

Overview

Before

After

civibot bot commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Nov 12, 2019

artfulrobot commented Nov 12, 2019

eileenmcnaughton commented Nov 13, 2019

artfulrobot commented Nov 13, 2019

eileenmcnaughton commented Nov 17, 2019

seamuslee001 commented Nov 17, 2019

artfulrobot commented Nov 17, 2019