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

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()
> + >>> 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.

> === 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.