launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05162
[Merge] lp:~stevenk/launchpad/refactor-team-rename into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/refactor-team-rename into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #853710 in Launchpad itself: "Teams that have ever had a PPA cannot be renamed"
https://bugs.launchpad.net/launchpad/+bug/853710
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/refactor-team-rename/+merge/78037
Refactor person and team rename checking into common code.
Currently, when renaming a person, if the person has a deleted PPA, or a PPA with no publications, the rename is allowed. If a team has a PPA *at all*, the rename is forbidden. This is somewhat wrong, and leads the bugs like the one linked.
I have also slightly cleaned up the tests related to this.
--
https://code.launchpad.net/~stevenk/launchpad/refactor-team-rename/+merge/78037
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/refactor-team-rename into lp:launchpad.
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2011-09-29 10:06:09 +0000
+++ lib/lp/registry/browser/person.py 2011-10-04 04:33:26 +0000
@@ -298,6 +298,7 @@
Milestone,
milestone_sort_key,
)
+from lp.registry.model.person import PersonRenameMixin
from lp.services.fields import LocationField
from lp.services.geoip.interfaces import IRequestPreferredLanguages
from lp.services.messages.interfaces.message import (
@@ -322,8 +323,6 @@
from lp.soyuz.browser.archivesubscription import (
traverse_archive_subscription_for_subscriber,
)
-from lp.soyuz.enums import ArchiveStatus
-from lp.soyuz.interfaces.archive import IArchiveSet
from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
@@ -4030,7 +4029,7 @@
page_title = label
-class PersonEditView(BasePersonEditView):
+class PersonEditView(BasePersonEditView, PersonRenameMixin):
"""The Person 'Edit' page."""
field_names = ['displayname', 'name', 'mugshot', 'homepage_content',
@@ -4050,21 +4049,15 @@
def setUpWidgets(self):
"""See `LaunchpadViewForm`.
- When a user has a PPA renames are prohibited.
+ When a user has an active PPA renames are prohibited.
"""
- has_ppa_with_published_packages = (
- getUtility(IArchiveSet).getPPAOwnedByPerson(
- self.context, has_packages=True,
- statuses=[ArchiveStatus.ACTIVE,
- ArchiveStatus.DELETING]) is not None)
- if has_ppa_with_published_packages:
+ reason = super(PersonEditView, self).can_be_renamed()
+ if reason:
# This makes the field's widget display (i.e. read) only.
self.form_fields['name'].for_display = True
super(PersonEditView, self).setUpWidgets()
- if has_ppa_with_published_packages:
- self.widgets['name'].hint = _(
- 'This user has an active PPA with packages published and '
- 'may not be renamed.')
+ if reason:
+ self.widgets['name'].hint = reason
def validate(self, data):
"""If the name changed, warn the user about the implications."""
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py 2011-08-23 23:32:26 +0000
+++ lib/lp/registry/browser/team.py 2011-10-04 04:33:26 +0000
@@ -94,6 +94,7 @@
CyclicalTeamMembershipError,
TeamMembershipStatus,
)
+from lp.registry.model.person import PersonRenameMixin
from lp.services.fields import PublicPersonChoice
from lp.services.propertycache import cachedproperty
@@ -199,7 +200,7 @@
class TeamEditView(TeamFormMixin, HasRenewalPolicyMixin,
- LaunchpadEditFormView):
+ LaunchpadEditFormView, PersonRenameMixin):
"""View for editing team details."""
schema = ITeam
@@ -252,17 +253,11 @@
def setUpWidgets(self):
"""See `LaunchpadViewForm`.
- When a team has a mailing list, a PPA or is private, renames
- are prohibited.
+ When a team has a mailing list or an active PPA, renames are
+ prohibited.
"""
- mailing_list = getUtility(IMailingListSet).get(self.context.name)
- has_mailing_list = (
- mailing_list is not None and
- mailing_list.status != MailingListStatus.PURGED)
- has_ppa = self.context.archive is not None
-
- block_renaming = (has_mailing_list or has_ppa)
- if block_renaming:
+ reason = super(TeamEditView, self).can_be_renamed()
+ if reason:
# This makes the field's widget display (i.e. read) only.
self.form_fields['name'].for_display = True
@@ -270,16 +265,8 @@
# Tweak the field form-help including an explanation for the
# read-only mode if necessary.
- if block_renaming:
- # Group the read-only mode reasons in textual form.
- reasons = []
- if has_mailing_list:
- reasons.append('has a mailing list')
- if has_ppa:
- reasons.append('has a PPA')
- reason = ' and '.join(reasons)
- self.widgets['name'].hint = _(
- 'This team cannot be renamed because it %s.' % reason)
+ if reason:
+ self.widgets['name'].hint = reason
def generateTokenAndValidationEmail(email, team):
=== modified file 'lib/lp/registry/browser/tests/test_team_view.py'
--- lib/lp/registry/browser/tests/test_team_view.py 2011-08-25 01:43:55 +0000
+++ lib/lp/registry/browser/tests/test_team_view.py 2011-10-04 04:33:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""
@@ -21,6 +21,7 @@
PersonVisibility,
TeamSubscriptionPolicy,
TeamMembershipRenewalPolicy)
+from lp.soyuz.enums import ArchiveStatus
from lp.testing import (
login_person,
person_logged_in,
@@ -159,17 +160,32 @@
layer = LaunchpadFunctionalLayer
- def test_cannot_rename_team_with_ppa(self):
- # A team with a ppa cannot be renamed.
+ def test_cannot_rename_team_with_active_ppa(self):
+ # A team with an active PPA that contains publications cannot be
+ # renamed.
owner = self.factory.makePerson()
team = self.factory.makeTeam(owner=owner)
- removeSecurityProxy(team).archive = self.factory.makeArchive()
+ archive = self.factory.makeArchive(owner=team)
+ self.factory.makeSourcePackagePublishingHistory(archive=archive)
+ removeSecurityProxy(team).archive = archive
with person_logged_in(owner):
view = create_initialized_view(team, name="+edit")
self.assertTrue(view.form_fields['name'].for_display)
self.assertEqual(
- 'This team cannot be renamed because it has a PPA.',
- view.widgets['name'].hint)
+ 'This team has an active PPA with packages published and '
+ 'may not be renamed.', view.widgets['name'].hint)
+
+ def test_can_rename_team_with_deleted_ppa(self):
+ # A team with a deleted PPA can be renamed.
+ owner = self.factory.makePerson()
+ team = self.factory.makeTeam(owner=owner)
+ archive = self.factory.makeArchive()
+ self.factory.makeSourcePackagePublishingHistory(archive=archive)
+ removeSecurityProxy(archive).status = ArchiveStatus.DELETED
+ removeSecurityProxy(team).archive = archive
+ with person_logged_in(owner):
+ view = create_initialized_view(team, name="+edit")
+ self.assertFalse(view.form_fields['name'].for_display)
def test_cannot_rename_team_with_active_mailinglist(self):
# Because renaming mailing lists is non-trivial in Mailman 2.1,
@@ -181,7 +197,7 @@
view = create_initialized_view(team, name="+edit")
self.assertTrue(view.form_fields['name'].for_display)
self.assertEqual(
- 'This team cannot be renamed because it has a mailing list.',
+ 'This team has a mailing list and may not be renamed.',
view.widgets['name'].hint)
def test_can_rename_team_with_purged_mailinglist(self):
@@ -203,13 +219,15 @@
owner = self.factory.makePerson()
team = self.factory.makeTeam(owner=owner)
self.factory.makeMailingList(team, owner)
- removeSecurityProxy(team).archive = self.factory.makeArchive()
+ archive = self.factory.makeArchive(owner=team)
+ self.factory.makeSourcePackagePublishingHistory(archive=archive)
+ removeSecurityProxy(team).archive = archive
with person_logged_in(owner):
view = create_initialized_view(team, name="+edit")
self.assertTrue(view.form_fields['name'].for_display)
self.assertEqual(
- ('This team cannot be renamed because it has a mailing list '
- 'and has a PPA.'),
+ 'This team has an active PPA with packages published and '
+ 'a mailing list and may not be renamed.',
view.widgets['name'].hint)
def test_edit_team_view_permission(self):
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-09-27 16:14:09 +0000
+++ lib/lp/registry/model/person.py 2011-10-04 04:33:26 +0000
@@ -20,6 +20,7 @@
'Person',
'person_sort_key',
'PersonLanguage',
+ 'PersonRenameMixin',
'PersonSet',
'SSHKey',
'SSHKeySet',
@@ -115,6 +116,7 @@
SQLBase,
sqlvalues,
)
+from canonical.launchpad import _
from canonical.launchpad.components.decoratedresultset import (
DecoratedResultSet,
)
@@ -4737,3 +4739,36 @@
recipient_ids,
need_validity=True,
need_preferred_email=True)
+
+
+class PersonRenameMixin:
+ """A mixin class that implements the checks if a person or team can be
+ renamed.
+ """
+
+ def can_be_renamed(self):
+ reasons = []
+ atom = 'user'
+ has_ppa = getUtility(IArchiveSet).getPPAOwnedByPerson(
+ self.context, has_packages=True,
+ statuses=[ArchiveStatus.ACTIVE,
+ ArchiveStatus.DELETING]) is not None
+ has_mailing_list = None
+ if ITeam.providedBy(self.context):
+ atom = 'team'
+ mailing_list = getUtility(IMailingListSet).get(self.context.name)
+ has_mailing_list = (
+ mailing_list is not None and
+ mailing_list.status != MailingListStatus.PURGED)
+ if has_ppa:
+ reasons.append('has an active PPA with packages published')
+ if has_mailing_list:
+ if has_ppa:
+ reasons.append('a mailing list')
+ else:
+ reasons.append('has a mailing list')
+ if reasons:
+ return _('This %s %s and may not be renamed.' % (
+ atom, ' and '.join(reasons)))
+ else:
+ return None
