Merge lp:~jcsackett/launchpad/merge-ppas-697685 into lp:launchpad

[j.c.sackett]

[r=adeuring][ui=none][bug=697685] Adds validation to RequestPeopleMergeView so that persons with PPAs cannot be merged.

Summary
=======

For a merge to happen, the duplicate entity (team or person) being merged can't have PPAs. This was already prevented from happening in the AdminMerge views, but not in the RequestPeopleMergeView, which is generally used by a person trying to merge another person into their account. The RequestPeopleMergeView needs to have the same validation checks as the AdminMerge views.

Proposed Fix
============

Add the validation step used in AdminMerge views to the other view.

Preimplementation Talk
======================

Spoke with Curtis Hovey about the validation needs.

Implementation
==============

lib/lp/registry/browser/peoplemerge.py
--------------------------------------

A new class, ValidatingMergeView was created that contains the validation step used in AdminMergeBaseView. AdminMergeBaseView and RequestPeopleMergeView both now inherit from this, instead of directly from LaunchpadFormView.

Additionally, the step in validation checking for PPAs now first checks that the dupe person is not None; if the dupe is None, the error is handled by the vocabularies governing merges, so this is safe to do, and prevents an error when the validator tries to find PPAs for NoneType.

Lastly, RequestPeopleMergeView has been grouped with the other RequestPeople merge views, so it's clear in a read through that there are related classes.

lib/lp/registry/browser/tests/test_peoplemerge.py
-------------------------------------------------

Create a new test case to test that duplicate people with ppas cause an error on the RequestPeopleMergeView.

Tests
=====

bin/test -t peoplemerge

Demo & QA
=========

Attempt to merge a person with a PPA into the account you're logged into through +requestmerge. It should provide an error saying you need to delete the PPA.

Obviously, do this on qastaging or launchpad.dev :-P

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

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

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2011-01-05 01:19:24 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2011-01-13 15:51:12 +0000
@@ -52,59 +52,31 @@
 from lp.soyuz.interfaces.archive import IArchiveSet
 
 
-class RequestPeopleMergeView(LaunchpadFormView):
-    """The view for the page where the user asks a merge of two accounts.
-
-    If the dupe account have only one email address we send a message to that
-    address and then redirect the user to other page saying that everything
-    went fine. Otherwise we redirect the user to another page where we list
-    all email addresses owned by the dupe account and the user selects which
-    of those (s)he wants to claim.
-    """
-
-    label = 'Merge Launchpad accounts'
-    page_title = label
-    schema = IRequestPeopleMerge
-
-    @property
-    def cancel_url(self):
-        return canonical_url(getUtility(IPersonSet))
-
-    @action('Continue', name='continue')
-    def continue_action(self, action, data):
-        dupeaccount = data['dupe_person']
-        if dupeaccount == self.user:
-            # Please, don't try to merge you into yourself.
-            return
-
-        emails = getUtility(IEmailAddressSet).getByPerson(dupeaccount)
-        emails_count = emails.count()
-        if emails_count > 1:
-            # The dupe account have more than one email address. Must redirect
-            # the user to another page to ask which of those emails (s)he
-            # wants to claim.
-            self.next_url = '+requestmerge-multiple?dupe=%d' % dupeaccount.id
-            return
-
-        assert emails_count == 1
-        email = emails[0]
-        login = getUtility(ILaunchBag).login
-        logintokenset = getUtility(ILoginTokenSet)
-        # Need to remove the security proxy because the dupe account may have
-        # hidden email addresses.
-        token = logintokenset.new(
-            self.user, login, removeSecurityProxy(email).email,
-            LoginTokenType.ACCOUNTMERGE)
-
-        # XXX: SteveAlexander 2006-03-07: An experiment to see if this
-        #      improves problems with merge people tests.
-        import canonical.database.sqlbase
-        canonical.database.sqlbase.flush_database_updates()
-        token.sendMergeRequestEmail()
-        self.next_url = './+mergerequest-sent?dupe=%d' % dupeaccount.id
-
-
-class AdminMergeBaseView(LaunchpadFormView):
+class ValidatingMergeView(LaunchpadFormView):
+
+    def validate(self, data):
+        """Check that user is not attempting to merge a person into itself."""
+        dupe_person = data.get('dupe_person')
+        target_person = data.get('target_person')
+
+        if dupe_person == target_person and dupe_person is not None:
+            self.addError(_("You can't merge ${name} into itself.",
+                  mapping=dict(name=dupe_person.name)))
+        # We cannot merge if there is a PPA with published
+        # packages on the duplicate, unless that PPA is removed.
+        if dupe_person is not None:
+            dupe_person_ppas = getUtility(IArchiveSet).getPPAOwnedByPerson(
+                dupe_person, statuses=[ArchiveStatus.ACTIVE,
+                                       ArchiveStatus.DELETING])
+            if dupe_person_ppas is not None:
+                self.addError(_(
+                    "${name} has a PPA that must be deleted before it "
+                    "can be merged. It may take ten minutes to remove the "
+                    "deleted PPA's files.",
+                    mapping=dict(name=dupe_person.name)))
+
+
+class AdminMergeBaseView(ValidatingMergeView):
     """Base view for the pages where admins can merge people/teams."""
 
     page_title = 'Merge Launchpad accounts'
@@ -507,3 +479,55 @@
     def email_hidden(self):
         """Does the duplicate account hide email addresses?"""
         return self.dupe.hide_email_addresses
+
+
+class RequestPeopleMergeView(ValidatingMergeView):
+    """The view for the page where the user asks a merge of two accounts.
+
+    If the dupe account have only one email address we send a message to that
+    address and then redirect the user to other page saying that everything
+    went fine. Otherwise we redirect the user to another page where we list
+    all email addresses owned by the dupe account and the user selects which
+    of those (s)he wants to claim.
+    """
+
+    label = 'Merge Launchpad accounts'
+    page_title = label
+    schema = IRequestPeopleMerge
+
+    @property
+    def cancel_url(self):
+        return canonical_url(getUtility(IPersonSet))
+
+    @action('Continue', name='continue')
+    def continue_action(self, action, data):
+        dupeaccount = data['dupe_person']
+        if dupeaccount == self.user:
+            # Please, don't try to merge you into yourself.
+            return
+
+        emails = getUtility(IEmailAddressSet).getByPerson(dupeaccount)
+        emails_count = emails.count()
+        if emails_count > 1:
+            # The dupe account have more than one email address. Must redirect
+            # the user to another page to ask which of those emails (s)he
+            # wants to claim.
+            self.next_url = '+requestmerge-multiple?dupe=%d' % dupeaccount.id
+            return
+
+        assert emails_count == 1
+        email = emails[0]
+        login = getUtility(ILaunchBag).login
+        logintokenset = getUtility(ILoginTokenSet)
+        # Need to remove the security proxy because the dupe account may have
+        # hidden email addresses.
+        token = logintokenset.new(
+            self.user, login, removeSecurityProxy(email).email,
+            LoginTokenType.ACCOUNTMERGE)
+
+        # XXX: SteveAlexander 2006-03-07: An experiment to see if this
+        #      improves problems with merge people tests.
+        import canonical.database.sqlbase
+        canonical.database.sqlbase.flush_database_updates()
+        token.sendMergeRequestEmail()
+        self.next_url = './+mergerequest-sent?dupe=%d' % dupeaccount.id
 
=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-21 19:42:00 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2011-01-13 15:51:12 +0000
@@ -30,6 +30,35 @@
     )
 
 
+class TestRequestPeopleMergeView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestRequestPeopleMergeView, self).setUp()
+        self.person_set = getUtility(IPersonSet)
+        self.dupe = self.factory.makePerson(name='dupe')
+        self.target = self.factory.makeTeam(name='target')
+
+    def test_cannot_merge_person_with_ppas(self):
+        # A team with a PPA cannot be merged.
+        login_celebrity('admin')
+        archive = self.dupe.createPPA()
+        login_celebrity('registry_experts')
+        form = {
+            'field.dupe_person': self.dupe.name,
+            'field.target_person': self.target.name,
+            'field.actions.continue': 'Continue',
+            }
+        view = create_initialized_view(
+            self.person_set, '+requestmerge', form=form)
+        self.assertEqual(
+            [u"dupe has a PPA that must be deleted before it can be "
+              "merged. It may take ten minutes to remove the deleted PPA's "
+              "files."],
+            view.errors)
+
+
 class TestRequestPeopleMergeMultipleEmailsView(TestCaseWithFactory):
     """Test the RequestPeopleMergeMultipleEmailsView rules."""