Code review comment for lp:~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas

> 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 removed before the person can be merged.
+ def test_cannot_merge_person_with_ppa(self):
+ # A person with a PPA cannot be merged.
         login_celebrity('admin')
         archive = self.dupe_person.createPPA()
- self.factory.makeSourcePackagePublishingHistory(archive=archive)
         view = self.getView()
         self.assertEqual(
- [u'dupe-person has a PPA that must be removed before '
- 'it can be merged.'],
+ [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 20:17:39 +0000
+++ lib/lp/registry/doc/person-merge.txt 2010-12-17 04:07:56 +0000
@@ -446,7 +446,6 @@
     >>> person_with_ppa = factory.makePerson()
     >>> other_person = factory.makePerson()
     >>> archive = person_with_ppa.createPPA()
- >>> ignore = factory.makeSourcePackagePublishingHistory(archive=archive)
     >>> email = IMasterObject(person_with_ppa.preferredemail)
     >>> email.status = EmailAddressStatus.VALIDATED
     >>> email.destroySelf()
@@ -454,4 +453,4 @@
     >>> personset.merge(person_with_ppa, other_person)
     Traceback (most recent call last):
     ...
- AssertionError: from_person still has ppas with published packages.
+ 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-15 20:07:54 +0000
+++ lib/lp/registry/interfaces/person.py 2010-12-17 01:05:21 +0000
@@ -862,6 +862,11 @@
             # 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)

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-12-15 20:07:54 +0000
+++ lib/lp/registry/model/person.py 2010-12-17 04:09:17 +0000
@@ -2752,7 +2752,19 @@
             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,
@@ -3862,9 +3874,9 @@
         if getUtility(IEmailAddressSet).getByPerson(from_person).count() > 0:
             raise AssertionError('from_person still has email addresses.')

- if from_person.has_ppa_with_published_packages:
+ if from_person.has_existing_ppa:
             raise AssertionError(
- 'from_person still has ppas with published packages.')
+ 'from_person has a ppa in ACTIVE or DELETING status')

         if from_person.is_team and from_person.allmembers.count() > 0:
             raise AssertionError(