launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/merge-dupe-mutates-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #239838 race condition merging dupe account with multiple email addresses
  https://bugs.launchpad.net/bugs/239838


This is my branch to prevent an oops when the user changes email address
when requesting a merge.

    lp:~sinzui/launchpad/merge-dupe-mutates-0
    Diff size: 82
    Launchpad bug:
          https://bugs.launchpad.net/bugs/239838
    Test command: ./bin/test -vv \
          -t TestRequestPeopleMergeMultipleEmailsView
    Pre-implementation: no one
    Target release: 10.10


Prevent an oops when the user changes email address when requesting a merge
---------------------------------------------------------------------------

A user can modify the email address that belong to a duplicate account
while requesting a merge. The assert in the code that prevents the user from
requesting an impossible merge should be replaced with a rule to abort the
request the explain the user why.


Rules
-----

    * Replace the assert notification to the user that the request must
      be restarted by reselecting addresses.
    * Curtis thinks that the message should not explain the details of the
      email address because the user may be trying to acquire an account
      or guess hidden addresses.

QA
--

    * On Staging, choose an account you can control or a friend controls
      that has several email addresses
    * Visit /people/+requestmerge and enter the user name
    * Verify you can see the visible email addresses
    * You or your friend then deletes one of the visible email addresses
    * Back on the merge request screen, choose the deleted email address and
      one other.
    * Verify you are told to reselect the address and that the deleted
      address is not shown.

Lint
----

Linting changed files:
  lib/lp/registry/browser/peoplemerge.py
  lib/lp/registry/browser/tests/test_peoplemerge.py


Test
----

    * lib/lp/registry/browser/tests/test_peoplemerge.py
      * Added a test that reproduces the events in the oops and verifies
        the user is told to reselect the addresses.


Implementation
--------------

    * lib/lp/registry/browser/peoplemerge.py
      * Replaced the assert with a message for the user and return early.
-- 
https://code.launchpad.net/~sinzui/launchpad/merge-dupe-mutates-0/+merge/36405
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/merge-dupe-mutates-0 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2010-09-23 02:11:13 +0000
@@ -429,7 +429,14 @@
 
                 for emailaddress in emails:
                     email = emailaddrset.getByEmail(emailaddress)
-                    assert email in self.dupeemails
+                    if email is None or email not in self.dupeemails:
+                        # The dupe person has changes his email addresses.
+                        # See bug 239838.
+                        self.request.response.addNotification(
+                            "An address was removed from the duplicate "
+                            "account while you were making this merge "
+                            "request. Select again.")
+                        return
                     email_addresses.append(emailaddress)
 
         for emailaddress in email_addresses:

=== added file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2010-09-23 02:11:13 +0000
@@ -0,0 +1,57 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test the peoplemerge browser module."""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import IPersonSet
+from lp.testing import (
+    login_person,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_view
+
+
+class TestRequestPeopleMergeMultipleEmailsView(TestCaseWithFactory):
+    """Test the RequestPeopleMergeMultipleEmailsView rules."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestRequestPeopleMergeMultipleEmailsView, self).setUp()
+        self.personset = getUtility(IPersonSet)
+        self.dupe_user = self.factory.makePerson()
+        self.email_2 = self.factory.makeEmail(
+            'dupe@xxxxxxxxx', self.dupe_user)
+        self.orginal_user = self.factory.makePerson()
+        login_person(self.orginal_user)
+
+    def test_removed_email(self):
+        # When the duplicate user deletes an email addres while the merge
+        # form is being complete, the view must abort and ask the user
+        # to restart the merge request.
+        form = {
+            'dupe': self.dupe_user.id,
+            }
+        view = create_view(
+            self.personset, name='+requestmerge-multiple', form=form)
+        view.processForm()
+        dupe_emails = [address for address in view.dupeemails]
+        form['selected'] = [address.email for address in dupe_emails]
+        with person_logged_in(self.dupe_user):
+            dupe_emails.remove(self.email_2)
+            self.email_2.destroySelf()
+        view = create_view(
+            self.personset, name='+requestmerge-multiple', form=form,
+            method='POST')
+        view.processForm()
+        self.assertEqual(0, len(view.notified_addresses))
+        self.assertEqual(1, len(view.request.notifications))
+        message = view.request.notifications[0].message
+        self.assertTrue(message.endswith('Select again.'))