Launchpad itself

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

  1. merge-mailing-list-bug-471770
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/merge-mailing-list-bug-471770
Merge into: lp:launchpad
Diff against target: 272 lines (+109/-32)
5 files modified
lib/canonical/launchpad/emailtemplates/person-merged.txt (+15/-0)
lib/lp/registry/doc/person-merge.txt (+26/-2)
lib/lp/registry/model/person.py (+14/-15)
lib/lp/registry/tests/test_personset.py (+52/-11)
lib/lp/testing/menu.py (+2/-4)
To merge this branch: bzr merge lp:~sinzui/launchpad/merge-mailing-list-bug-471770
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+14806@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

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.

Revision history for this message
Abel Deuring (adeuring) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'lib/canonical/launchpad/emailtemplates/person-merged.txt'
2--- lib/canonical/launchpad/emailtemplates/person-merged.txt 1970-01-01 00:00:00 +0000
3+++ lib/canonical/launchpad/emailtemplates/person-merged.txt 2009-11-13 13:12:15 +0000
4@@ -0,0 +1,15 @@
5+
6+The Launchpad account named '%(dupename)s' was merged into the account
7+named '%(person)s'. You can confirm the merged email addresses to
8+associate them with your account. Merged email addresses are unsubscribed
9+from all Launchpad mailing lists during the merge. All team memberships
10+for the merged account were also transferred to your account, and
11+those team may have mailing lists.
12+
13+You can review and update your email and subscription settings at:
14+
15+ https://launchpad.net/~%(person)s/+editemails
16+
17+Thank you,
18+
19+The Launchpad team
20
21=== renamed file 'lib/lp/registry/doc/xx-coc-views.txt' => 'lib/lp/registry/browser/tests/coc-views.txt'
22=== renamed file 'lib/lp/registry/doc/person-edit-views.txt' => 'lib/lp/registry/browser/tests/person-edit-views.txt'
23=== renamed file 'lib/lp/registry/doc/person-karma-views.txt' => 'lib/lp/registry/browser/tests/person-karma-views.txt'
24=== renamed file 'lib/lp/registry/doc/team-hierarchy-views.txt' => 'lib/lp/registry/browser/tests/team-hierarchy-views.txt'
25=== renamed file 'lib/lp/registry/doc/teammembership-views.txt' => 'lib/lp/registry/browser/tests/teammembership-views.txt'
26=== modified file 'lib/lp/registry/doc/person-merge.txt'
27--- lib/lp/registry/doc/person-merge.txt 2009-08-13 19:03:36 +0000
28+++ lib/lp/registry/doc/person-merge.txt 2009-11-13 13:12:15 +0000
29@@ -199,8 +199,32 @@
30 >>> results.get_one()[0] is None
31 True
32
33-
34-== Person decoration ==
35+An email is sent to the user informing him that he should review his email
36+and mailing list subscription settings.
37+
38+ >>> from canonical.launchpad.interfaces.personnotification import (
39+ ... IPersonNotificationSet)
40+
41+ >>> notification_set = getUtility(IPersonNotificationSet)
42+ >>> notifications = notification_set.getNotificationsToSend()
43+ >>> notifications.count()
44+ 1
45+ >>> notification = notifications[0]
46+ >>> print notification.person.name
47+ name12
48+ >>> print notification.subject
49+ Launchpad accounts merged
50+ >>> print notification.body
51+ The Launchpad account named 'marilize-merged' was merged into the account
52+ named 'name12'. ...
53+
54+ You can review and update your email and subscription settings at:
55+
56+ https://launchpad.net/name12/+editemails ...
57+
58+
59+Person decoration
60+-----------------
61
62 Several tables "extend" the Person table by having additional information
63 that is UNIQUEly keyed to Person.id. We have a utility function that merges
64
65=== modified file 'lib/lp/registry/model/person.py'
66--- lib/lp/registry/model/person.py 2009-10-16 09:25:45 +0000
67+++ lib/lp/registry/model/person.py 2009-11-13 13:12:15 +0000
68@@ -2861,21 +2861,9 @@
69 name=name, new_name=new_name, product=product))
70
71 def _mergeMailingListSubscriptions(self, cur, from_id, to_id):
72- # Update MailingListSubscription. Note that no remaining records
73- # will have email_address set, as we assert earlier that the
74- # from_person has no email addresses.
75- # Update records that don't conflict.
76- cur.execute('''
77- UPDATE MailingListSubscription
78- SET person=%(to_id)d
79- WHERE person=%(from_id)d
80- AND mailing_list NOT IN (
81- SELECT mailing_list
82- FROM MailingListSubscription
83- WHERE person=%(to_id)d
84- )
85- ''' % vars())
86- # Then trash the remainders.
87+ # Update MailingListSubscription. Note that since all the from_id
88+ # email addresses are set to NEW, all the subscriptions must be
89+ # removed because the user must confirm them.
90 cur.execute('''
91 DELETE FROM MailingListSubscription WHERE person=%(from_id)d
92 ''' % vars())
93@@ -3472,6 +3460,17 @@
94 # flush its caches.
95 store.invalidate()
96
97+ # Inform the user of the merge changes.
98+ if not to_person.isTeam():
99+ mail_text = get_email_template('person-merged.txt')
100+ mail_text = mail_text % {
101+ 'dupename': from_person.name,
102+ 'person': to_person.name,
103+ }
104+ subject = 'Launchpad accounts merged'
105+ getUtility(IPersonNotificationSet).addNotification(
106+ to_person, subject, mail_text)
107+
108 def getValidPersons(self, persons):
109 """See `IPersonSet.`"""
110 person_ids = [person.id for person in persons]
111
112=== modified file 'lib/lp/registry/tests/test_personset.py'
113--- lib/lp/registry/tests/test_personset.py 2009-09-29 19:00:28 +0000
114+++ lib/lp/registry/tests/test_personset.py 2009-11-13 13:12:15 +0000
115@@ -7,46 +7,56 @@
116
117 from unittest import TestLoader
118
119+import transaction
120 from zope.component import getUtility
121 from zope.security.proxy import removeSecurityProxy
122
123 from lp.registry.model.person import PersonSet
124+from lp.registry.interfaces.mailinglistsubscription import (
125+ MailingListAutoSubscribePolicy)
126 from lp.registry.interfaces.person import (
127 PersonCreationRationale, IPersonSet)
128-from lp.testing import TestCaseWithFactory
129+from lp.testing import TestCaseWithFactory, login_person, logout
130+
131+from canonical.database.sqlbase import cursor
132 from canonical.launchpad.testing.databasehelpers import (
133 remove_all_sample_data_branches)
134-from canonical.testing import LaunchpadFunctionalLayer
135+from canonical.testing import DatabaseFunctionalLayer
136
137
138 class TestPersonSetBranchCounts(TestCaseWithFactory):
139
140- layer = LaunchpadFunctionalLayer
141+ layer = DatabaseFunctionalLayer
142
143 def setUp(self):
144 TestCaseWithFactory.setUp(self)
145 remove_all_sample_data_branches()
146+ self.person_set = getUtility(IPersonSet)
147
148 def test_no_branches(self):
149 """Initially there should be no branches."""
150- self.assertEqual(0, PersonSet().getPeopleWithBranches().count())
151+ self.assertEqual(0, self.person_set.getPeopleWithBranches().count())
152
153 def test_five_branches(self):
154 branches = [self.factory.makeAnyBranch() for x in range(5)]
155 # Each branch has a different product, so any individual product
156 # will return one branch.
157- self.assertEqual(5, PersonSet().getPeopleWithBranches().count())
158- self.assertEqual(1, PersonSet().getPeopleWithBranches(
159+ self.assertEqual(5, self.person_set.getPeopleWithBranches().count())
160+ self.assertEqual(1, self.person_set.getPeopleWithBranches(
161 branches[0].product).count())
162
163
164 class TestPersonSetEnsurePerson(TestCaseWithFactory):
165
166- layer = LaunchpadFunctionalLayer
167+ layer = DatabaseFunctionalLayer
168 email_address = 'testing.ensure.person@example.com'
169 displayname = 'Testing ensurePerson'
170 rationale = PersonCreationRationale.SOURCEPACKAGEUPLOAD
171
172+ def setUp(self):
173+ TestCaseWithFactory.setUp(self)
174+ self.person_set = getUtility(IPersonSet)
175+
176 def test_ensurePerson_returns_existing_person(self):
177 # IPerson.ensurePerson returns existing person and does not
178 # override its details.
179@@ -54,7 +64,7 @@
180 testing_person = self.factory.makePerson(
181 email=self.email_address, displayname=testing_displayname)
182
183- ensured_person = getUtility(IPersonSet).ensurePerson(
184+ ensured_person = self.person_set.ensurePerson(
185 self.email_address, self.displayname, self.rationale)
186 self.assertEquals(testing_person.id, ensured_person.id)
187 self.assertIsNot(
188@@ -67,7 +77,7 @@
189 def test_ensurePerson_hides_new_person_email(self):
190 # IPersonSet.ensurePerson creates new person with
191 # 'hide_email_addresses' set.
192- ensured_person = getUtility(IPersonSet).ensurePerson(
193+ ensured_person = self.person_set.ensurePerson(
194 self.email_address, self.displayname, self.rationale)
195 self.assertTrue(ensured_person.hide_email_addresses)
196
197@@ -78,7 +88,7 @@
198 self.displayname, email=self.email_address)
199 self.assertIs(None, test_account.preferredemail.person)
200
201- ensured_person = getUtility(IPersonSet).ensurePerson(
202+ ensured_person = self.person_set.ensurePerson(
203 self.email_address, self.displayname, self.rationale)
204 self.assertEquals(test_account.id, ensured_person.account.id)
205 self.assertTrue(ensured_person.hide_email_addresses)
206@@ -98,7 +108,7 @@
207 self.assertIs(None, testing_account.preferredemail.person)
208 self.assertIs(None, testing_person.preferredemail)
209
210- ensured_person = getUtility(IPersonSet).ensurePerson(
211+ ensured_person = self.person_set.ensurePerson(
212 self.email_address, self.displayname, self.rationale)
213
214 # The existing Person was retrieved and the Account
215@@ -108,5 +118,36 @@
216 ensured_person.preferredemail.id)
217
218
219+class TestPersonSetMerge(TestCaseWithFactory):
220+
221+ layer = DatabaseFunctionalLayer
222+
223+ def setUp(self):
224+ TestCaseWithFactory.setUp(self)
225+ # Use the unsecured PersonSet so that private methods can be tested.
226+ self.person_set = PersonSet()
227+ self.from_person = self.factory.makePerson()
228+ self.to_person = self.factory.makePerson()
229+ self.cur = cursor()
230+
231+ def test__mergeMailingListSubscriptions_no_subscriptions(self):
232+ self.person_set._mergeMailingListSubscriptions(
233+ self.cur, self.from_person.id, self.to_person.id)
234+ self.assertEqual(0, self.cur.rowcount)
235+
236+ def test__mergeMailingListSubscriptions_with_subscriptions(self):
237+ self.from_person.mailing_list_auto_subscribe_policy = (
238+ MailingListAutoSubscribePolicy.ALWAYS)
239+ self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
240+ 'test-mailinglist', 'team-owner')
241+ login_person(self.team.teamowner)
242+ self.team.addMember(self.from_person, reviewer=self.team.teamowner)
243+ logout()
244+ transaction.commit()
245+ self.person_set._mergeMailingListSubscriptions(
246+ self.cur, self.from_person.id, self.to_person.id)
247+ self.assertEqual(1, self.cur.rowcount)
248+
249+
250 def test_suite():
251 return TestLoader().loadTestsFromName(__name__)
252
253=== modified file 'lib/lp/testing/menu.py'
254--- lib/lp/testing/menu.py 2009-09-10 20:35:24 +0000
255+++ lib/lp/testing/menu.py 2009-11-13 13:12:15 +0000
256@@ -13,7 +13,6 @@
257
258 def check_menu_links(menu):
259 context = menu.context
260- is_sane_menu = True
261 for link in menu.iterlinks():
262 if '?' in link.target:
263 view_name, _args = link.target.split('?')
264@@ -24,6 +23,5 @@
265 try:
266 view = getMultiAdapter((context, request), name=view_name)
267 except:
268- is_sane_menu = False
269- print 'Bad link %s: %s' % (link.name, url)
270- print is_sane_menu
271+ return 'Bad link %s: %s' % (link.name, url)
272+ return True