launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05596
[Merge] lp:~stevenk/launchpad/less-powerful-bug-supervisors into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/less-powerful-bug-supervisors into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #885692 in Launchpad itself: "bug supervisors have more power than maintainers and admins"
https://bugs.launchpad.net/launchpad/+bug/885692
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/less-powerful-bug-supervisors/+merge/82632
This branch is somewhat misnamed. It is not about making bug supervisors less powerful or taking away abilities they have today, it is about making sure that pillar owners, drivers and admins have the same rights, so make sure everyone has the same level of power.
The only place I've not fixed is the browser code inside bugtarget, which is a little difficult and requires thought since we don't actually have a bugtask yet -- but it has been marked with an XXX.
I have discussed this branch with Curtis and William, they like the approach.
I have cleaned up some lint that I noticed.
--
https://code.launchpad.net/~stevenk/launchpad/less-powerful-bug-supervisors/+merge/82632
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/less-powerful-bug-supervisors into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py 2011-07-28 17:31:16 +0000
+++ lib/lp/bugs/browser/bugtarget.py 2011-11-18 04:11:27 +0000
@@ -131,7 +131,6 @@
from lp.bugs.interfaces.bugtracker import IBugTracker
from lp.bugs.interfaces.malone import IMaloneApplication
from lp.bugs.interfaces.securitycontact import IHasSecurityContact
-from lp.bugs.model.bugtask import BugTask
from lp.bugs.model.structuralsubscription import (
get_structural_subscriptions_for_target,
)
@@ -402,6 +401,8 @@
# If the context is a project group we want to render the optional
# fields since they will initially be hidden and later exposed if the
# selected project supports them.
+ # XXX: StevenK 2011-11-18 bug=885692 This should make use of
+ # IBugTask.userHasPrivileges().
include_extra_fields = IProjectGroup.providedBy(context)
if not include_extra_fields and IHasBugSupervisor.providedBy(context):
include_extra_fields = self.user.inTeam(context.bug_supervisor)
@@ -627,6 +628,8 @@
if extra_data.private:
params.private = extra_data.private
+ # XXX: StevenK 2011-11-18 bug=885692 This should make use of
+ # IBugTask.userHasPrivileges().
# Apply any extra options given by a bug supervisor.
if IHasBugSupervisor.providedBy(context):
if self.user.inTeam(context.bug_supervisor):
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-11-17 17:17:38 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-11-18 04:11:27 +0000
@@ -1278,7 +1278,20 @@
}
-class BugTaskEditView(LaunchpadEditFormView, BugTaskBugWatchMixin):
+class BugTaskPrivilegeMixin:
+
+ @cachedproperty
+ def user_has_privileges(self):
+ """Is the user privileged? That is, an admin, pillar owner, driver
+ or bug supervisor.
+
+ If yes, return True, otherwise return False.
+ """
+ return self.context.userHasPrivileges(self.user)
+
+
+class BugTaskEditView(LaunchpadEditFormView, BugTaskBugWatchMixin,
+ BugTaskPrivilegeMixin):
"""The view class used for the task +editstatus page."""
schema = IBugTask
@@ -1343,26 +1356,24 @@
# XXX: Brad Bollenbach 2006-09-29 bug=63000: Permission checking
# doesn't belong here!
- if ('milestone' in editable_field_names and
- not self.userCanEditMilestone()):
- editable_field_names.remove("milestone")
-
- if ('importance' in editable_field_names and
- not self.userCanEditImportance()):
- editable_field_names.remove("importance")
+ if not self.context.userHasPrivileges(self.user):
+ if 'milestone' in editable_field_names:
+ editable_field_names.remove("milestone")
+ if 'importance' in editable_field_names:
+ editable_field_names.remove("importance")
else:
editable_field_names = set(('bugwatch', ))
if self.context.bugwatch is None:
editable_field_names.update(('status', 'assignee'))
if ('importance' in self.default_field_names
- and self.userCanEditImportance()):
+ and self.context.userHasPrivileges(self.user)):
editable_field_names.add('importance')
else:
bugtracker = self.context.bugwatch.bugtracker
if bugtracker.bugtrackertype == BugTrackerType.EMAILADDRESS:
editable_field_names.add('status')
if ('importance' in self.default_field_names
- and self.userCanEditImportance()):
+ and self.context.userHasPrivileges(self.user)):
editable_field_names.add('importance')
if self.show_target_widget:
@@ -1516,10 +1527,8 @@
if self.context.target_uses_malone:
read_only_field_names = []
- if not self.userCanEditMilestone():
+ if not self.context.userHasPrivileges(self.user):
read_only_field_names.append("milestone")
-
- if not self.userCanEditImportance():
read_only_field_names.append("importance")
else:
editable_field_names = self.editable_field_names
@@ -1529,20 +1538,6 @@
return read_only_field_names
- def userCanEditMilestone(self):
- """Can the user edit the Milestone field?
-
- If yes, return True, otherwise return False.
- """
- return self.context.userCanEditMilestone(self.user)
-
- def userCanEditImportance(self):
- """Can the user edit the Importance field?
-
- If yes, return True, otherwise return False.
- """
- return self.context.userCanEditImportance(self.user)
-
def validate(self, data):
if self.show_sourcepackagename_widget and 'sourcepackagename' in data:
data['target'] = self.context.distroseries
@@ -3763,7 +3758,8 @@
return True
-class BugTaskTableRowView(LaunchpadView, BugTaskBugWatchMixin):
+class BugTaskTableRowView(LaunchpadView, BugTaskBugWatchMixin,
+ BugTaskPrivilegeMixin):
"""Browser class for rendering a bugtask row on the bug page."""
is_conjoined_slave = None
@@ -3811,7 +3807,7 @@
target_link_title=self.target_link_title,
user_can_delete=self.user_can_delete_bugtask,
delete_link=delete_link,
- user_can_edit_importance=self.user_can_edit_importance,
+ user_can_edit_importance=self.user_has_privileges,
importance_css_class='importance' + self.context.importance.name,
importance_title=self.context.importance.title,
# We always look up all milestones, so there's no harm
@@ -3950,9 +3946,7 @@
If yes, return True, otherwise return False.
"""
- bugtask = self.context
- return (self.user_can_edit_status
- and bugtask.userCanEditImportance(self.user))
+ return self.user_can_edit_status and self.user_has_privileges
@cachedproperty
def user_can_edit_status(self):
@@ -3977,14 +3971,6 @@
return self.user is not None
@cachedproperty
- def user_can_edit_milestone(self):
- """Can the user edit the Milestone field?
-
- If yes, return True, otherwise return False.
- """
- return self.context.userCanEditMilestone(self.user)
-
- @cachedproperty
def user_can_delete_bugtask(self):
"""Can the user delete the bug task?
@@ -4058,9 +4044,9 @@
request=self.api_request)
if cx.milestone else None),
user_can_edit_assignee=self.user_can_edit_assignee,
- user_can_edit_milestone=self.user_can_edit_milestone,
+ user_can_edit_milestone=self.user_has_privileges,
user_can_edit_status=self.user_can_edit_status,
- user_can_edit_importance=self.user_can_edit_importance,
+ user_can_edit_importance=self.user_has_privileges,
)
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-11-14 00:11:41 +0000
+++ lib/lp/bugs/configure.zcml 2011-11-18 04:11:27 +0000
@@ -199,8 +199,7 @@
isSubscribed
getPackageComponent
maybeConfirm
- userCanEditImportance
- userCanEditMilestone
+ userHasPrivileges
userCanSetAnyAssignee
userCanUnassign"/>
<require
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-11-04 18:55:42 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-11-18 04:11:27 +0000
@@ -913,11 +913,12 @@
not a package task, returns None.
"""
- def userCanEditMilestone(user):
- """Can the user edit the Milestone field?"""
+ def userHasPrivileges(user):
+ """Is the user a privledged one, allowed to changed details on a
+ bug?.
- def userCanEditImportance(user):
- """Can the user edit the Importance field?"""
+ :return: A boolean.
+ """
# Set schemas that were impossible to specify during the definition of
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-11-17 12:42:16 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-11-18 04:11:27 +0000
@@ -109,6 +109,7 @@
from lp.bugs.interfaces.bug import IBugSet
from lp.bugs.interfaces.bugattachment import BugAttachmentType
from lp.bugs.interfaces.bugnomination import BugNominationStatus
+from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
from lp.bugs.interfaces.bugtask import (
BUG_SUPERVISOR_BUGTASK_STATUSES,
BugBlueprintSearch,
@@ -155,6 +156,7 @@
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.registry.interfaces.role import IPersonRoles
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
from lp.registry.model.pillar import pillar_sort_key
@@ -825,7 +827,7 @@
def transitionToMilestone(self, new_milestone, user):
"""See `IBugTask`."""
- if not self.userCanEditMilestone(user):
+ if not self.userHasPrivileges(user):
raise UserCannotEditBugTaskMilestone(
"User does not have sufficient permissions "
"to edit the bug task milestone.")
@@ -834,7 +836,7 @@
def transitionToImportance(self, new_importance, user):
"""See `IBugTask`."""
- if not self.userCanEditImportance(user):
+ if not self.userHasPrivileges(user):
raise UserCannotEditBugTaskImportance(
"User does not have sufficient permissions "
"to edit the bug task importance.")
@@ -902,15 +904,10 @@
def canTransitionToStatus(self, new_status, user):
"""See `IBugTask`."""
new_status = normalize_bugtask_status(new_status)
- celebrities = getUtility(ILaunchpadCelebrities)
if (self.status == BugTaskStatus.FIXRELEASED and
(user.id == self.bug.ownerID or user.inTeam(self.bug.owner))):
return True
- elif (user.inTeam(self.pillar.bug_supervisor) or
- user.inTeam(self.pillar.owner) or
- user.id == celebrities.bug_watch_updater.id or
- user.id == celebrities.bug_importer.id or
- user.id == celebrities.janitor.id):
+ elif self.userHasPrivileges(user):
return True
else:
return (self.status not in (
@@ -1055,20 +1052,6 @@
if new_status < BugTaskStatus.FIXRELEASED:
self.date_fix_released = None
- def _userCanSetAssignee(self, user):
- """Used by methods to check if user can assign or unassign bugtask."""
- celebrities = getUtility(ILaunchpadCelebrities)
- return (
- user.inTeam(self.pillar.bug_supervisor) or
- user.inTeam(self.pillar.owner) or
- user.inTeam(self.pillar.driver) or
- (self.distroseries is not None and
- user.inTeam(self.distroseries.driver)) or
- (self.productseries is not None and
- user.inTeam(self.productseries.driver)) or
- user.inTeam(celebrities.admin)
- or user == celebrities.bug_importer)
-
def userCanSetAnyAssignee(self, user):
"""See `IBugTask`."""
if user is None:
@@ -1076,7 +1059,7 @@
elif self.pillar.bug_supervisor is None:
return True
else:
- return self._userCanSetAssignee(user)
+ return self.userHasPrivileges(user)
def userCanUnassign(self, user):
"""True if user can set the assignee to None.
@@ -1086,7 +1069,7 @@
Launchpad admins can always unassign.
"""
return user is not None and (
- user.inTeam(self.assignee) or self._userCanSetAssignee(user))
+ user.inTeam(self.assignee) or self.userHasPrivileges(user))
def canTransitionToAssignee(self, assignee):
"""See `IBugTask`."""
@@ -1354,25 +1337,34 @@
else:
return None
- def _userIsPillarEditor(self, user):
- """Can the user edit this tasks's pillar?"""
- if user is None:
+ def userHasPrivileges(self, user):
+ """See `IBugTask`."""
+ if not user:
return False
- pillar = self.pillar
- return ((pillar.bug_supervisor is not None and
- user.inTeam(pillar.bug_supervisor)) or
- pillar.userCanEdit(user))
-
- def userCanEditMilestone(self, user):
- """See `IBugTask`."""
- return self._userIsPillarEditor(user)
-
- def userCanEditImportance(self, user):
- """See `IBugTask`."""
- celebs = getUtility(ILaunchpadCelebrities)
- return (self._userIsPillarEditor(user) or
- user == celebs.bug_watch_updater or
- user == celebs.bug_importer)
+ role = IPersonRoles(user)
+ # Admins can always change bug details.
+ if role.in_admin:
+ return True
+
+ # Similar to admins, the Bug Watch Updater, Bug Importer and
+ # Janitor can always change bug details.
+ if (
+ role.in_bug_watch_updater or role.in_bug_importer or
+ role.in_janitor):
+ return True
+
+ # Otherwise, if you're a member of the pillar owner, drivers, or the
+ # bug supervisor, you can change bug details.
+ bugsupervisor = None
+ if IHasBugSupervisor.providedBy(self.pillar):
+ bugsupervisor = self.pillar.bug_supervisor
+ return (
+ role.isOwner(self.pillar) or role.isOneOfDrivers(self.pillar) or
+ role.inTeam(bugsupervisor) or
+ (self.distroseries is not None and
+ role.isDriver(self.distroseries)) or
+ (self.productseries is not None and
+ role.isDriver(self.productseries)))
def __repr__(self):
return "<BugTask for bug %s on %r>" % (self.bugID, self.target)
=== modified file 'lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt'
--- lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt 2011-11-14 08:30:52 +0000
+++ lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt 2011-11-18 04:11:27 +0000
@@ -11,6 +11,7 @@
of the bug task table lead him to the bug task edit forms of the
respective bug task.
+<<<<<<< TREE
>>> admin_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
>>> print extract_text(
... find_tag_by_id(admin_browser.contents, 'affected-software'))
@@ -57,6 +58,52 @@
...
Comment on this change (optional)
...
+=======
+>>> admin_browser.open('http://bugs.launchpad.dev/firefox/+bug/1')
+>>> print extract_text(
+... find_tag_by_id(admin_browser.contents, 'affected-software'))
+Affects Status Importance Assigned to...
+Mozilla Firefox... New Low Mark Shuttleworth...
+mozilla-firefox (Debian)... Confirmed Low debbugs #304014...
+mozilla-firefox (Ubuntu)... New Medium Unassigned
+...
+
+>>> print admin_browser.getLink('New', index=0).url
+http://bugs.launchpad.dev/firefox/+bug/1/+editstatus
+>>> print admin_browser.getLink('Low', index=0).url
+http://bugs.launchpad.dev/firefox/+bug/1/+editstatus
+
+>>> print admin_browser.getLink('Confirmed').url
+Traceback (most recent call last):
+...
+LinkNotFoundError
+>>> print admin_browser.getLink('Low', index=1).url
+http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/1/+editstatus
+
+>>> print admin_browser.getLink('New', index=1).url
+http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/1/+editstatus
+>>> print admin_browser.getLink('Medium').url
+http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/1/+editstatus
+
+>>> admin_browser.getLink('New').click()
+>>> print extract_text(admin_browser.contents)
+Edit status...
+...
+Affecting: Mozilla Firefox
+Filed here by: Sample Person
+When: 2004-01-02
+Assigned: 2005-01-02
+Target
+Distribution
+...
+Project (Find…)
+Status Importance Milestone
+New... Low... (no value)...
+Assigned to... Mark Shuttleworth (mark)
+...
+Comment on this change (optional)
+...
+>>>>>>> MERGE-SOURCE
For more details of the +editstatus page, see xx-bug-privileged-statuses.txt,
xx-view-editable-bug-task.txt.
=== modified file 'lib/lp/bugs/templates/bugtask-edit-form.pt'
--- lib/lp/bugs/templates/bugtask-edit-form.pt 2011-08-11 06:03:15 +0000
+++ lib/lp/bugs/templates/bugtask-edit-form.pt 2011-11-18 04:11:27 +0000
@@ -179,14 +179,14 @@
<td tal:content="structure view/widgets/status" />
<td title="Changeable only by a project maintainer or bug supervisor">
<span
- tal:condition="not:view/userCanEditImportance"
+ tal:condition="not:view/user_has_privileges"
class="sprite read-only"></span>
<tal:widget content="structure view/widgets/importance" />
</td>
<td tal:condition="view/widgets/milestone"
title="Changeable only by a project maintainer or bug supervisor">
<span
- tal:condition="not:view/userCanEditMilestone"
+ tal:condition="not:view/user_has_privileges"
class="sprite read-only"></span>
<tal:widget content="structure view/widgets/milestone" />
</td>
=== modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-11-04 18:55:42 +0000
+++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-11-18 04:11:27 +0000
@@ -164,7 +164,7 @@
<div class="milestone-content"
tal:condition="data/target_has_milestones"
style="width: 100%; float: left">
- <a tal:condition="view/user_can_edit_milestone"
+ <a tal:condition="view/user_has_privileges"
class="nulltext addicon js-action sprite add"
tal:attributes="href data/edit_link;
style view/style_for_add_milestone">
@@ -173,7 +173,7 @@
<a class="value"
tal:attributes="href context/milestone/fmt:url | nothing"
tal:content="context/milestone/title | nothing" />
- <a tal:condition="view/user_can_edit_milestone"
+ <a tal:condition="view/user_has_privileges"
tal:attributes="href data/edit_link"
><img class="editicon" src="/@@/edit"
tal:attributes="style view/style_for_edit_milestone"
