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

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

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

Edwin--

Thanks for those changes.

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

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

This branch looks good to land.

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.