Merge lp:~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas into lp:launchpad

[Edwin Grubbs]

[r=jcsackett,sinzui][ui=none][bug=676966] Don't allow a person/team to be merged until its PPAs are deleted.

Summary
-------

To prevent oopses in the PPA publisher, a form error message is
displayed when the user tries to merge a person or team with a PPA
containing published packages.

Implementation details
----------------------

Since the merge views are using the same check as renaming a person with
a PPA, I have moved that check into the model and updated the rename
person view. Person.merge() also checks that the merged person does not
have a PPA with published packages.
    lib/lp/registry/browser/person.py
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/model/person.py

Added check to the AdminMergeBaseView so that the check applies to
+adminpeoplemerge and +adminteammerge.
    lib/lp/registry/browser/peoplemerge.py
    lib/lp/registry/browser/tests/test_peoplemerge.py
    lib/lp/registry/doc/person-merge.txt

Tests
-----

./bin/test -vv -t whatever

Demo and Q/A
------------

* Open http://launchpad.dev/...

Lint
----

Linting changed files:
  lib/lp/registry/browser/peoplemerge.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_peoplemerge.py
  lib/lp/registry/doc/person-merge.txt
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py

./lib/lp/registry/browser/peoplemerge.py
     143: E301 expected 1 blank line, found 2
./lib/lp/registry/doc/person-merge.txt
       1: narrative uses a moin header.
      22: narrative uses a moin header.
      32: narrative uses a moin header.
      61: narrative exceeds 78 characters.
      85: narrative exceeds 78 characters.
     105: narrative uses a moin header.
     122: narrative uses a moin header.
     339: narrative uses a moin header.
./lib/lp/registry/interfaces/person.py
     493: E302 expected 2 blank lines, found 1
./lib/lp/registry/model/person.py
    1480: W291 trailing whitespace
    1480: Line has trailing whitespace.

[j.c.sackett (jcsackett)]

Hey Edwin--

I really like this branch. I just have some suggestions/questions on comments below.

> === modified file 'lib/lp/registry/browser/peoplemerge.py'
> --- lib/lp/registry/browser/peoplemerge.py 2010-12-03 16:33:03 +0000
> +++ lib/lp/registry/browser/peoplemerge.py 2010-12-08 18:32:00 +0000
> @@ -132,6 +132,13 @@
> 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 the teams if there is a PPA with published
> + # packages on the duplicate person, unless that PPA is removed.
> + if dupe_person.has_ppa_with_published_packages:
> + self.addError(_(
> + "${name} has a PPA with published packages; we "
> + "can't merge it.", mapping=dict(name=dupe_person.name)))

It might be good to direct someone to info on what they can do to get around this? In
other places where we throw an error in merging we indicate that a team needs to be removed or something; perhaps "the PPA must be deleted before we can merge ${name}, after the merge a new PPA can be recreated?

> === modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
> --- lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-03 16:33:03 +0000
> +++ lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-08 18:32:00 +0000
> @@ -152,3 +155,51 @@
> view = self.getView()
> self.assertEqual([], view.errors)
> self.assertEqual(self.target_team, self.dupe_team.merged)
> +
> + def test_cannot_merge_team_with_ppa_containing_published_packages(self):
> + # The PPA must be removed before the team can be merged.
> + login_celebrity('admin')
> + self.dupe_team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
> + archive = self.dupe_team.createPPA()
> + self.factory.makeSourcePackagePublishingHistory(archive=archive)
> + login_celebrity('registry_experts')
> + view = self.getView()
> + self.assertEqual(
> + [u"dupe-team has a PPA with published packages; "
> + "we can't merge it."],
> + view.errors)

I'd change the comment to just "A team cannot be merged with published PPA" since the test doesn't demonstrate removing the PPA to make it work.

> === modified file 'lib/lp/registry/doc/person-merge.txt'
> --- lib/lp/registry/doc/person-merge.txt 2010-12-01 23:39:05 +0000
> +++ lib/lp/registry/doc/person-merge.txt 2010-12-08 18:32:00 +0000
> @@ -425,6 +425,8 @@
> >>> test_team.deactivateAllMembers(comment, personset.getByName('name16'))
> >>> for team in test_team.super_teams:
> ... test_team.retractTeamMembership(team, test_team.teamowner)
> +
> +
> >>> personset.merge(test_team, landscape)
>
> # The resulting Landscape-developers no new super teams, has

Thanks for the cleanup.

> @@ -438,3 +440,18 @@
> []
> >>> list(IPollSubset(landscape).getAll())
> []
> +
> +A person with a PPA can't be merged until the PPA is deleted.
> +
> + >>> person_with_ppa = factory.makePerson()
> + >>> ot...

review: Needs Information (code*)

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

Thanks for the review. Here are the changes you asked for.

> Hey Edwin--
>
> I really like this branch. I just have some suggestions/questions on comments
> below.
>
> > === modified file 'lib/lp/registry/browser/peoplemerge.py'
> > --- lib/lp/registry/browser/peoplemerge.py 2010-12-03 16:33:03 +0000
> > +++ lib/lp/registry/browser/peoplemerge.py 2010-12-08 18:32:00 +0000
> > @@ -132,6 +132,13 @@
> > 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 the teams if there is a PPA with published
> > + # packages on the duplicate person, unless that PPA is removed.
> > + if dupe_person.has_ppa_with_published_packages:
> > + self.addError(_(
> > + "${name} has a PPA with published packages; we "
> > + "can't merge it.", mapping=dict(name=dupe_person.name)))
>
> It might be good to direct someone to info on what they can do to get around
> this? In
> other places where we throw an error in merging we indicate that a team needs
> to be removed or something; perhaps "the PPA must be deleted before we can
> merge ${name}, after the merge a new PPA can be recreated?

I did something similar but a little bit shorter.

> > === modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
> > --- lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-03 16:33:03
> +0000
> > +++ lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-08 18:32:00
> +0000
> > @@ -152,3 +155,51 @@
> > view = self.getView()
> > self.assertEqual([], view.errors)
> > self.assertEqual(self.target_team, self.dupe_team.merged)
> > +
> > + def
> test_cannot_merge_team_with_ppa_containing_published_packages(self):
> > + # The PPA must be removed before the team can be merged.
> > + login_celebrity('admin')
> > + self.dupe_team.subscriptionpolicy =
> TeamSubscriptionPolicy.MODERATED
> > + archive = self.dupe_team.createPPA()
> > + self.factory.makeSourcePackagePublishingHistory(archive=archive)
> > + login_celebrity('registry_experts')
> > + view = self.getView()
> > + self.assertEqual(
> > + [u"dupe-team has a PPA with published packages; "
> > + "we can't merge it."],
> > + view.errors)
>
> I'd change the comment to just "A team cannot be merged with published PPA"
> since the test doesn't demonstrate removing the PPA to make it work.

Done.

> > === modified file 'lib/lp/registry/doc/person-merge.txt'
> > --- lib/lp/registry/doc/person-merge.txt 2010-12-01 23:39:05 +0000
> > +++ lib/lp/registry/doc/person-merge.txt 2010-12-08 18:32:00 +0000
> > @@ -425,6 +425,8 @@
> > >>> test_team.deactivateAllMembers(comment,
> personset.getByName('name16'))
> > >>> for team in test_team.super_teams:
> > ... test_team.retractTeamMembership(team, test_team.teamowner)
> > +
> > +
> > >>> personset.merge(test_team, landscape)
> >
> > # The resulting Landscape-developers no new...

[j.c.sackett (jcsackett)]

Edwin--

Thanks for those changes.

review: Approve (code*)

[Curtis Hovey (sinzui)]

As we discussed on IRC "PPA that must be removed" should be "PPA that must be deleted" since that is the link the user must use.

has_ppa_with_published_packages() will allow users to merge active ppas without packages. I think these ppas will show up in searches. Can you talk to someone on the soyuz team

review: Needs Information (code)

[Edwin Grubbs (edwin-grubbs)]

> As we discussed on IRC "PPA that must be removed" should be "PPA that must be
> deleted" since that is the link the user must use.
>
> has_ppa_with_published_packages() will allow users to merge active ppas
> without packages. I think these ppas will show up in searches. Can you talk to
> someone on the soyuz team

Hi Curtis,

Thanks for the review.

I have changed the condition to not allow merges on a person or team with a PPA in the ACTIVE or DELETING status whether or not it has published packages, so that links to merged persons don't show up in https://launchpad.net/~PERSON/+archivesubscriptions

Incremental Diff:

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py 2010-12-15 20:17:39 +0000
+++ lib/lp/registry/browser/peoplemerge.py 2010-12-17 04:08:34 +0000
@@ -134,11 +134,12 @@
                   mapping=dict(name=dupe_person.name)))
         # We cannot merge the teams if there is a PPA with published
         # packages on the duplicate person, unless that PPA is removed.
- if dupe_person.has_ppa_with_published_packages:
+ if dupe_person.has_existing_ppa:
             self.addError(_(
- "${name} has a PPA that must be removed before it "
- "can be merged.", mapping=dict(name=dupe_person.name)))
-
+ "${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)))

     def render(self):
         # Subclasses may define other actions that they will render manually

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-15 20:17:39 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-17 04:07:09 +0000
@@ -156,17 +156,17 @@
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)

- def test_cannot_merge_team_with_ppa_containing_published_packages(self):
- # A team with a published PPA cannot be merged.
+ def test_cannot_merge_team_with_ppa(self):
+ # A team with a PPA cannot be merged.
         login_celebrity('admin')
         self.dupe_team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
         archive = self.dupe_team.createPPA()
- self.factory.makeSourcePackagePublishingHistory(archive=archive)
         login_celebrity('registry_experts')
         view = self.getView()
         self.assertEqual(
- [u'dupe-team has a PPA that must be removed before '
- 'it can be merged.'],
+ [u"dupe-team 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)

     def test_registry_delete_team_with_super_teams(self):
@@ -205,13 +205,13 @@
         return create_initialized_view(
             self.person_set, '+adminpeoplemerge', form=form)

- def test_cannot_merge_person_with_ppa_containing_published_packages(self):
- # The PPA must be r...

[Curtis Hovey (sinzui)]

This branch looks good to land.

review: Approve (code)

[Julian Edwards (julian-edwards)]

I think that the restriction on merging people with PPAs with or without publications is wrong. I originally suggested that we allow the merge to take place even though there's a PPA as long as it has no packages (i.e. it's a brand new one). Deleting these types of PPAs has zero affect other than to change its status; there's nothing to actually delete on disk.

The +archivesubscriptions problem should be solved differently.

Also, has_existing_ppa and has_ppa_with_published_packages
should not be properties on IPerson, they should be on IArchiveSet so
that Soyuz-specific queries are kept in the lp.soyuz domain.

The Bugs team made this mistake with one of their bits of code that knows too
much about Soyuz publishing tables and caused circular imports when running
bin/test. It also makes future changes to soyuz code harder because the coder will not expect this sort of code to be outside the soyuz domain.

I recommend that this be fixed ASAP as tech debt. I filed bug 693357 about this.

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2010-12-13 16:28:55 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2010-12-20 20:45:28 +0000
@@ -132,6 +132,14 @@
         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 the teams if there is a PPA with published
+        # packages on the duplicate person, unless that PPA is removed.
+        if dupe_person.has_existing_ppa:
+            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)))
 
     def render(self):
         # Subclasses may define other actions that they will render manually

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-12-17 17:49:50 +0000
+++ lib/lp/registry/browser/person.py	2010-12-20 20:45:28 +0000
@@ -327,7 +327,6 @@
 from lp.soyuz.browser.archivesubscription import (
     traverse_archive_subscription_for_subscriber,
     )
-from lp.soyuz.enums import ArchiveStatus
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
@@ -4138,16 +4137,13 @@
 
         When a user has a PPA renames are prohibited.
         """
-        active_ppas = [
-            ppa for ppa in self.context.ppas
-            if ppa.status in (ArchiveStatus.ACTIVE, ArchiveStatus.DELETING)]
-        num_packages = sum(
-            ppa.getPublishedSources().count() for ppa in active_ppas)
-        if num_packages > 0:
+        has_ppa_with_published_packages = (
+            self.context.has_ppa_with_published_packages)
+        if has_ppa_with_published_packages:
             # This makes the field's widget display (i.e. read) only.
             self.form_fields['name'].for_display = True
         super(PersonEditView, self).setUpWidgets()
-        if num_packages > 0:
+        if has_ppa_with_published_packages:
             self.widgets['name'].hint = _(
                 'This user has an active PPA with packages published and '
                 'may not be renamed.')

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-13 16:28:55 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-20 20:45:28 +0000
@@ -12,8 +12,11 @@
     )
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.mailinglist import MailingListStatus
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    TeamSubscriptionPolicy,
+    )
 from lp.testing import (
     celebrity_logged_in,
     login_celebrity,
@@ -92,8 +95,8 @@
     def setUp(self):
         super(TestAdminTeamMergeView, self).setUp()
         self.person_set = getUtility(IPersonSet)
-        self.dupe_team = self.factory.makeTeam()
-        self.target_team = self.factory.makeTeam()
+        self.dupe_team = self.factory.makeTeam(name='dupe-team')
+        self.target_team = self.factory.makeTeam(name='target-team')
         login_celebrity('registry_experts')
 
     def getView(self, form=None):
@@ -153,6 +156,19 @@
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)
 
+    def test_cannot_merge_team_with_ppa(self):
+        # A team with a PPA cannot be merged.
+        login_celebrity('admin')
+        self.dupe_team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
+        archive = self.dupe_team.createPPA()
+        login_celebrity('registry_experts')
+        view = self.getView()
+        self.assertEqual(
+            [u"dupe-team 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)
+
     def test_registry_delete_team_with_super_teams(self):
         # Registry admins can delete teams with super team memberships.
         self.target_team = getUtility(ILaunchpadCelebrities).registry_experts
@@ -164,3 +180,38 @@
         view = self.getView()
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)
+
+
+class TestAdminPeopleMergeView(TestCaseWithFactory):
+    """Test the AdminPeopleMergeView rules."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestAdminPeopleMergeView, self).setUp()
+        self.person_set = getUtility(IPersonSet)
+        self.dupe_person = self.factory.makePerson(name='dupe-person')
+        self.target_person = self.factory.makePerson()
+        login_celebrity('registry_experts')
+
+    def getView(self, form=None):
+        if form is None:
+            form = {
+                'field.dupe_person': self.dupe_person.name,
+                'field.target_person': self.target_person.name,
+                'field.actions.reassign_emails_and_merge':
+                    'Reassign E-mails and Merge',
+                }
+        return create_initialized_view(
+            self.person_set, '+adminpeoplemerge', form=form)
+
+    def test_cannot_merge_person_with_ppa(self):
+        # A person with a PPA cannot be merged.
+        login_celebrity('admin')
+        archive = self.dupe_person.createPPA()
+        view = self.getView()
+        self.assertEqual(
+            [u"dupe-person 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)

=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt	2010-12-15 22:11:11 +0000
+++ lib/lp/registry/doc/person-merge.txt	2010-12-20 20:45:28 +0000
@@ -422,6 +422,8 @@
     >>> test_team.deactivateAllMembers(comment, personset.getByName('name16'))
     >>> for team in test_team.super_teams:
     ...     test_team.retractTeamMembership(team, test_team.teamowner)
+
+
     >>> personset.merge(test_team, landscape)
 
     # The resulting Landscape-developers no new super teams and its
@@ -432,3 +434,17 @@
     [u'salgado', u'name12']
     >>> [team.name for team in landscape.super_teams]
     []
+
+A person with a PPA can't be merged.
+
+    >>> person_with_ppa = factory.makePerson()
+    >>> other_person = factory.makePerson()
+    >>> archive = person_with_ppa.createPPA()
+    >>> email = IMasterObject(person_with_ppa.preferredemail)
+    >>> email.status = EmailAddressStatus.VALIDATED
+    >>> email.destroySelf()
+    >>> transaction.commit()
+    >>> personset.merge(person_with_ppa, other_person)
+    Traceback (most recent call last):
+    ...
+    AssertionError: from_person has a ppa in ACTIVE or DELETING status

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-12-16 18:56:02 +0000
+++ lib/lp/registry/interfaces/person.py	2010-12-20 20:45:28 +0000
@@ -862,6 +862,15 @@
             # Really IArchive, see archive.py
             value_type=Reference(schema=Interface)))
 
+    has_existing_ppa = Bool(
+        title=_("Does this person have a ppa that is active or not fully "
+                "deleted?"),
+        readonly=True, required=False)
+
+    has_ppa_with_published_packages = Bool(
+        title=_("Does this person have a ppa with published packages?"),
+        readonly=True, required=False)
+
     entitlements = Attribute("List of Entitlements for this person or team.")
 
     structural_subscriptions = Attribute(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-12-17 04:48:01 +0000
+++ lib/lp/registry/model/person.py	2010-12-20 20:45:28 +0000
@@ -268,11 +268,15 @@
     VOUCHER_STATUSES,
     )
 from lp.services.worlddata.model.language import Language
-from lp.soyuz.enums import ArchivePurpose
+from lp.soyuz.enums import (
+    ArchivePurpose,
+    ArchiveStatus,
+    )
 from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.translations.model.hastranslationimports import (
     HasTranslationImportsMixin,
@@ -2659,6 +2663,29 @@
         return Archive.selectBy(
             owner=self, purpose=ArchivePurpose.PPA, orderBy='name')
 
+    @property
+    def has_existing_ppa(self):
+        """See `IPerson`."""
+        result = Store.of(self).find(
+            Archive,
+            Archive.owner == self.id,
+            Archive.purpose == ArchivePurpose.PPA,
+            Archive.status.is_in(
+                [ArchiveStatus.ACTIVE, ArchiveStatus.DELETING]))
+        return not result.is_empty()
+
+    @property
+    def has_ppa_with_published_packages(self):
+        """See `IPerson`."""
+        result = Store.of(self).find(
+            Archive,
+            SourcePackagePublishingHistory.archive == Archive.id,
+            Archive.owner == self.id,
+            Archive.purpose == ArchivePurpose.PPA,
+            Archive.status.is_in(
+                [ArchiveStatus.ACTIVE, ArchiveStatus.DELETING]))
+        return not result.is_empty()
+
     def getPPAByName(self, name):
         """See `IPerson`."""
         return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
@@ -3759,6 +3786,10 @@
         if getUtility(IEmailAddressSet).getByPerson(from_person).count() > 0:
             raise AssertionError('from_person still has email addresses.')
 
+        if from_person.has_existing_ppa:
+            raise AssertionError(
+                'from_person has a ppa in ACTIVE or DELETING status')
+
         if from_person.is_team and from_person.allmembers.count() > 0:
             raise AssertionError(
                 "Only teams without active members can be merged")