Merge lp:~sinzui/launchpad/merge-mailing-list-bug-471770 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to remove mailing list subscriptions from merged accounts.

    lp:~sinzui/launchpad/merge-mailing-list-bug-471770
    Diff size: 247
    Launchpad bug: https://bugs.launchpad.net/bugs/471770
    Test command: ./bin/test -vvt "registry.*test_pesonset"
    Pre-implementation: bac, salgado
    Target release: 3.1.11

= Remove mailing list subscriptions from merged accounts =

OOPS-1402EB708 shows an oops that occurs when ~lhavelund tries to edit his
email addresses. This failure is SimpleVocabulary(terms). lhavelund is
subscribed to a mailing list under a NEW address. The email address came
from a merged account. Merge sets the email addresses to NEW so that they
can be reconfirmed--but that is not possible in this case because the user
cannot request a validation email from +editemail.

== Rules ==

    * Update PersonSet._mergeMailingListSubscriptions() to delete the
      subscriptions
    * Send an email to the user explaining that he should confirm the
      merged email addresses and update his mailing list subscriptions
      * This ensure that users are notified regardless of if the merge was
        admin-assisted or the user did it himself.
      * This is a step towards making all merges happen offline to avoid
        time outs.

== QA ==

    * On staging, merge an account that has a mailing list.
    * Verify that there is a notification email in the staging inbox.
    * Verify that you can access your +editemails
    * Update your mailing list subscriptions.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/emailtemplates/person-merged.txt
  lib/lp/registry/doc/person-merge.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_personset.py

== Test ==

    * lib/lp/registry/doc/person-merge.txt
      * Added a test to verify the user is sent an email after the merge.
    * lib/lp/registry/tests/test_personset.py
      * Moved the tests to the DatabaseFunctionalLayer
      * Removed the duplicate ProductSet lines
      * Added a test to verify that that mailing list subscriptions are
        deleted.

== Implementation ==

    * lib/canonical/launchpad/emailtemplates/person-merged.txt
      * Added an email to inform the user to review his email settings.
    * lib/lp/registry/model/person.py
      * Removed the mailing list transfer SQL. The delete SQL was there to
        clean up duplicate subscriptions, but now deletes them all
      * Add a PersonNotification to the merged user so that he knows to
        review his email settings.

[Abel Deuring (adeuring)]

Hi Curtis,

nice branch. I have only two spelling questions/suggestions.

Abel

> === added file 'lib/canonical/launchpad/emailtemplates/person-merged.txt'
> --- lib/canonical/launchpad/emailtemplates/person-merged.txt 1970-01-01 00:00:00 +0000
> +++ lib/canonical/launchpad/emailtemplates/person-merged.txt 2009-11-12 21:15:25 +0000
> @@ -0,0 +1,15 @@
> +
> +The Launchpad account named '%(dupename)s' was merged into the account
> +named '%(person)s'. You can confirm the merged email addresses to
> +associate them with your account. Merged email address are unsubscribed

I think that should be "email addresses are".

> +from all Launchpad mailing lists during the merge. All team memberships
> +for the merged account were also transfered to your account, and

I am not sure, but shouldn't this be "transferred"?

> +those team may have mailing lists.
> +
> +You can review and update your email and subscription settings at:
> +
> + https://launchpad.net/~%(person)s/+editemails
> +
> +Thank you,
> +
> +The Launchpad team

review: Approve (code)

=== added file 'lib/canonical/launchpad/emailtemplates/person-merged.txt'
--- lib/canonical/launchpad/emailtemplates/person-merged.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/emailtemplates/person-merged.txt	2009-11-13 13:12:15 +0000
@@ -0,0 +1,15 @@
+
+The Launchpad account named '%(dupename)s' was merged into the account
+named '%(person)s'. You can confirm the merged email addresses to
+associate them with your account. Merged email addresses are unsubscribed
+from all Launchpad mailing lists during the merge. All team memberships
+for the merged account were also transferred to your account, and
+those team may have mailing lists.
+
+You can review and update your email and subscription settings at:
+
+    https://launchpad.net/~%(person)s/+editemails
+
+Thank you,
+
+The Launchpad team
 
=== renamed file 'lib/lp/registry/doc/xx-coc-views.txt' => 'lib/lp/registry/browser/tests/coc-views.txt'
=== renamed file 'lib/lp/registry/doc/person-edit-views.txt' => 'lib/lp/registry/browser/tests/person-edit-views.txt'
=== renamed file 'lib/lp/registry/doc/person-karma-views.txt' => 'lib/lp/registry/browser/tests/person-karma-views.txt'
=== renamed file 'lib/lp/registry/doc/team-hierarchy-views.txt' => 'lib/lp/registry/browser/tests/team-hierarchy-views.txt'
=== renamed file 'lib/lp/registry/doc/teammembership-views.txt' => 'lib/lp/registry/browser/tests/teammembership-views.txt'
=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt	2009-08-13 19:03:36 +0000
+++ lib/lp/registry/doc/person-merge.txt	2009-11-13 13:12:15 +0000
@@ -199,8 +199,32 @@
     >>> results.get_one()[0] is None
     True
 
-
-== Person decoration ==
+An email is sent to the user informing him that he should review his email
+and mailing list subscription settings.
+
+    >>> from canonical.launchpad.interfaces.personnotification import (
+    ...     IPersonNotificationSet)
+
+    >>> notification_set = getUtility(IPersonNotificationSet)
+    >>> notifications = notification_set.getNotificationsToSend()
+    >>> notifications.count()
+    1
+    >>> notification = notifications[0]
+    >>> print notification.person.name
+    name12
+    >>> print notification.subject
+    Launchpad accounts merged
+    >>> print notification.body
+    The Launchpad account named 'marilize-merged' was merged into the account
+    named 'name12'. ...
+
+    You can review and update your email and subscription settings at:
+
+        https://launchpad.net/name12/+editemails ...
+
+
+Person decoration
+-----------------
 
 Several tables "extend" the Person table by having additional information
 that is UNIQUEly keyed to Person.id. We have a utility function that merges
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2009-10-16 09:25:45 +0000
+++ lib/lp/registry/model/person.py	2009-11-13 13:12:15 +0000
@@ -2861,21 +2861,9 @@
                           name=name, new_name=new_name, product=product))
 
     def _mergeMailingListSubscriptions(self, cur, from_id, to_id):
-        # Update MailingListSubscription. Note that no remaining records
-        # will have email_address set, as we assert earlier that the
-        # from_person has no email addresses.
-        # Update records that don't conflict.
-        cur.execute('''
-            UPDATE MailingListSubscription
-            SET person=%(to_id)d
-            WHERE person=%(from_id)d
-                AND mailing_list NOT IN (
-                    SELECT mailing_list
-                    FROM MailingListSubscription
-                    WHERE person=%(to_id)d
-                    )
-            ''' % vars())
-        # Then trash the remainders.
+        # Update MailingListSubscription. Note that since all the from_id
+        # email addresses are set to NEW, all the subscriptions must be
+        # removed because the user must confirm them.
         cur.execute('''
             DELETE FROM MailingListSubscription WHERE person=%(from_id)d
             ''' % vars())
@@ -3472,6 +3460,17 @@
         # flush its caches.
         store.invalidate()
 
+        # Inform the user of the merge changes.
+        if not to_person.isTeam():
+            mail_text = get_email_template('person-merged.txt')
+            mail_text = mail_text % {
+                'dupename': from_person.name,
+                'person': to_person.name,
+                }
+            subject = 'Launchpad accounts merged'
+            getUtility(IPersonNotificationSet).addNotification(
+                to_person, subject, mail_text)
+
     def getValidPersons(self, persons):
         """See `IPersonSet.`"""
         person_ids = [person.id for person in persons]
 
=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2009-09-29 19:00:28 +0000
+++ lib/lp/registry/tests/test_personset.py	2009-11-13 13:12:15 +0000
@@ -7,46 +7,56 @@
 
 from unittest import TestLoader
 
+import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.registry.model.person import PersonSet
+from lp.registry.interfaces.mailinglistsubscription import (
+    MailingListAutoSubscribePolicy)
 from lp.registry.interfaces.person import (
     PersonCreationRationale, IPersonSet)
-from lp.testing import TestCaseWithFactory
+from lp.testing import TestCaseWithFactory, login_person, logout
+
+from canonical.database.sqlbase import cursor
 from canonical.launchpad.testing.databasehelpers import (
     remove_all_sample_data_branches)
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import DatabaseFunctionalLayer
 
 
 class TestPersonSetBranchCounts(TestCaseWithFactory):
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         TestCaseWithFactory.setUp(self)
         remove_all_sample_data_branches()
+        self.person_set = getUtility(IPersonSet)
 
     def test_no_branches(self):
         """Initially there should be no branches."""
-        self.assertEqual(0, PersonSet().getPeopleWithBranches().count())
+        self.assertEqual(0, self.person_set.getPeopleWithBranches().count())
 
     def test_five_branches(self):
         branches = [self.factory.makeAnyBranch() for x in range(5)]
         # Each branch has a different product, so any individual product
         # will return one branch.
-        self.assertEqual(5, PersonSet().getPeopleWithBranches().count())
-        self.assertEqual(1, PersonSet().getPeopleWithBranches(
+        self.assertEqual(5, self.person_set.getPeopleWithBranches().count())
+        self.assertEqual(1, self.person_set.getPeopleWithBranches(
                 branches[0].product).count())
 
 
 class TestPersonSetEnsurePerson(TestCaseWithFactory):
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
     email_address = '[e-mail]'
     displayname = 'Testing ensurePerson'
     rationale = PersonCreationRationale.SOURCEPACKAGEUPLOAD
 
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.person_set = getUtility(IPersonSet)
+
     def test_ensurePerson_returns_existing_person(self):
         # IPerson.ensurePerson returns existing person and does not
         # override its details.
@@ -54,7 +64,7 @@
         testing_person = self.factory.makePerson(
             email=self.email_address, displayname=testing_displayname)
 
-        ensured_person = getUtility(IPersonSet).ensurePerson(
+        ensured_person = self.person_set.ensurePerson(
             self.email_address, self.displayname, self.rationale)
         self.assertEquals(testing_person.id, ensured_person.id)
         self.assertIsNot(
@@ -67,7 +77,7 @@
     def test_ensurePerson_hides_new_person_email(self):
         # IPersonSet.ensurePerson creates new person with
         # 'hide_email_addresses' set.
-        ensured_person = getUtility(IPersonSet).ensurePerson(
+        ensured_person = self.person_set.ensurePerson(
             self.email_address, self.displayname, self.rationale)
         self.assertTrue(ensured_person.hide_email_addresses)
 
@@ -78,7 +88,7 @@
             self.displayname, email=self.email_address)
         self.assertIs(None, test_account.preferredemail.person)
 
-        ensured_person = getUtility(IPersonSet).ensurePerson(
+        ensured_person = self.person_set.ensurePerson(
             self.email_address, self.displayname, self.rationale)
         self.assertEquals(test_account.id, ensured_person.account.id)
         self.assertTrue(ensured_person.hide_email_addresses)
@@ -98,7 +108,7 @@
         self.assertIs(None, testing_account.preferredemail.person)
         self.assertIs(None, testing_person.preferredemail)
 
-        ensured_person = getUtility(IPersonSet).ensurePerson(
+        ensured_person = self.person_set.ensurePerson(
             self.email_address, self.displayname, self.rationale)
 
         # The existing Person was retrieved and the Account
@@ -108,5 +118,36 @@
                           ensured_person.preferredemail.id)
 
 
+class TestPersonSetMerge(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        # Use the unsecured PersonSet so that private methods can be tested.
+        self.person_set = PersonSet()
+        self.from_person = self.factory.makePerson()
+        self.to_person = self.factory.makePerson()
+        self.cur = cursor()
+
+    def test__mergeMailingListSubscriptions_no_subscriptions(self):
+        self.person_set._mergeMailingListSubscriptions(
+            self.cur, self.from_person.id, self.to_person.id)
+        self.assertEqual(0, self.cur.rowcount)
+
+    def test__mergeMailingListSubscriptions_with_subscriptions(self):
+        self.from_person.mailing_list_auto_subscribe_policy = (
+            MailingListAutoSubscribePolicy.ALWAYS)
+        self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
+            'test-mailinglist', 'team-owner')
+        login_person(self.team.teamowner)
+        self.team.addMember(self.from_person, reviewer=self.team.teamowner)
+        logout()
+        transaction.commit()
+        self.person_set._mergeMailingListSubscriptions(
+            self.cur, self.from_person.id, self.to_person.id)
+        self.assertEqual(1, self.cur.rowcount)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/testing/menu.py'
--- lib/lp/testing/menu.py	2009-09-10 20:35:24 +0000
+++ lib/lp/testing/menu.py	2009-11-13 13:12:15 +0000
@@ -13,7 +13,6 @@
 
 def check_menu_links(menu):
     context = menu.context
-    is_sane_menu = True
     for link in menu.iterlinks():
         if '?' in link.target:
             view_name, _args = link.target.split('?')
@@ -24,6 +23,5 @@
         try:
             view = getMultiAdapter((context, request), name=view_name)
         except:
-            is_sane_menu = False
-            print 'Bad link %s: %s' % (link.name, url)
-    print is_sane_menu
+            return 'Bad link %s: %s' % (link.name, url)
+    return True