launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02123
lp:~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas into lp:launchpad
Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#676966 Merging people with active PPAs should be blocked
https://bugs.launchpad.net/bugs/676966
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.
--
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas/+merge/43116
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-676966-disallow-merging-people-with-ppas into lp:launchpad.
=== 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:31:24 +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)))
+
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-02 19:45:50 +0000
+++ lib/lp/registry/browser/person.py 2010-12-08 18:31:24 +0000
@@ -331,7 +331,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
@@ -4187,16 +4186,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-03 16:33:03 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py 2010-12-08 18:31:24 +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 (
login_celebrity,
login_person,
@@ -91,8 +94,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):
@@ -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)
+
+
+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_containing_published_packages(self):
+ # The PPA must be removed before the person can 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 with published packages; "
+ "we can't merge it."],
+ view.errors)
=== 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:31:24 +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
@@ -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.
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-11-22 23:08:11 +0000
+++ lib/lp/registry/interfaces/person.py 2010-12-08 18:31:24 +0000
@@ -784,6 +784,10 @@
# Really IArchive, see archive.py
value_type=Reference(schema=Interface)))
+ 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-03 16:33:03 +0000
+++ lib/lp/registry/model/person.py 2010-12-08 18:31:24 +0000
@@ -265,11 +265,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,
@@ -2744,6 +2748,17 @@
return Archive.selectBy(
owner=self, purpose=ArchivePurpose.PPA, orderBy='name')
+ @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()
+
def getPPAByName(self, name):
"""See `IPerson`."""
return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
@@ -3844,6 +3859,10 @@
if getUtility(IEmailAddressSet).getByPerson(from_person).count() > 0:
raise AssertionError('from_person still has email addresses.')
+ if from_person.has_ppa_with_published_packages:
+ raise AssertionError(
+ 'from_person still has ppas with published packages.')
+
if from_person.is_team and from_person.allmembers.count() > 0:
raise AssertionError(
"Only teams without active members can be merged")
