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

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 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()
> > + >>> 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()
> > + >>> transaction.commit()
> > + >>> personset.merge(person_with_ppa, other_person)
> > + Traceback (most recent call last):
> > + ...
> > + AssertionError: from_person still has ppas with published packages.
>
> Again, the comment and the demonstration aren't exactly in sync; I'd like the
> comment to be along the lines of just "A person with a PPA can't be merged" or
> to have the story show the merge working after the delete. Given how slow
> stories are, I think just changing the comment would be the better approach.

Done.

> > === modified file 'lib/lp/registry/model/person.py'
> > --- lib/lp/registry/model/person.py 2010-12-03 16:33:03 +0000
> > +++ lib/lp/registry/model/person.py 2010-12-08 18:32:00 +0000
> > + @property
> > + def has_ppa_with_published_packages(self):
> > + 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()
> > +
>
> Adding this to the model was a perfect idea; thanks for reducing the
> repetition in the merge path.

Diff:
------------------------------------------

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py 2010-12-15 20:07:54 +0000
+++ lib/lp/registry/browser/peoplemerge.py 2010-12-15 20:09:11 +0000
@@ -136,8 +136,8 @@
         # 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)))
+ "${name} has a PPA that must be removed before it "
+ "can be merged.", mapping=dict(name=dupe_person.name)))

     def render(self):

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-15 20:07:54 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-15 20:16:13 +0000
@@ -157,7 +157,7 @@
         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.
+ # A team with a published PPA cannot be merged.
         login_celebrity('admin')
         self.dupe_team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
         archive = self.dupe_team.createPPA()
@@ -165,8 +165,8 @@
         login_celebrity('registry_experts')
         view = self.getView()
         self.assertEqual(
- [u"dupe-team has a PPA with published packages; "
- "we can't merge it."],
+ [u'dupe-team has a PPA that must be removed before '
+ 'it can be merged.'],
             view.errors)

     def test_registry_delete_team_with_super_teams(self):
@@ -212,6 +212,6 @@
         self.factory.makeSourcePackagePublishingHistory(archive=archive)
         view = self.getView()
         self.assertEqual(
- [u"dupe-person has a PPA with published packages; "
- "we can't merge it."],
+ [u'dupe-person has a PPA that must be removed before '
+ 'it can be merged.'],
             view.errors)

=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt 2010-12-08 04:54:34 +0000
+++ lib/lp/registry/doc/person-merge.txt 2010-12-15 20:12:07 +0000
@@ -441,7 +441,7 @@
     >>> list(IPollSubset(landscape).getAll())
     []

-A person with a PPA can't be merged until the PPA is deleted.
+A person with a PPA can't be merged.

     >>> person_with_ppa = factory.makePerson()
     >>> other_person = factory.makePerson()