Merge lp:~stevenk/launchpad/less-powerful-bug-supervisors into lp:launchpad

[Steve Kowalik]

[r=wallyworld][bug=885692] Consolidate who has access to change details on bugtasks.

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.

[Ian Booth (wallyworld)]

Looks good. Thanks for adding the tests. I think the one which asserts that any old user doesn't have privileges is particularly important.

review: Approve (code)

=== 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-21 00:29:30 +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-20 23:50:47 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-21 00:29:30 +0000
@@ -1281,7 +1281,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
@@ -1346,26 +1359,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.user_has_privileges:
+                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.user_has_privileges):
                     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.user_has_privileges):
                         editable_field_names.add('importance')
 
         if self.show_target_widget:
@@ -1519,10 +1530,8 @@
         if self.context.target_uses_malone:
             read_only_field_names = []
 
-            if not self.userCanEditMilestone():
+            if not self.user_has_privileges:
                 read_only_field_names.append("milestone")
-
-            if not self.userCanEditImportance():
                 read_only_field_names.append("importance")
         else:
             editable_field_names = self.editable_field_names
@@ -1532,20 +1541,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
@@ -3776,7 +3771,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
@@ -3824,7 +3820,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
@@ -3963,9 +3959,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):
@@ -3990,14 +3984,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?
 
@@ -4071,9 +4057,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/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-19 05:44:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-21 00:29:30 +0000
@@ -185,7 +185,7 @@
         self.invalidate_caches(bug.default_bugtask)
         self.getUserBrowser(url, owner)
         # At least 20 of these should be removed.
-        self.assertThat(recorder, HasQueryCount(LessThan(100)))
+        self.assertThat(recorder, HasQueryCount(LessThan(101)))
         count_with_no_branches = recorder.count
         for sp in sourcepackages:
             self.makeLinkedBranchMergeProposal(sp, bug, owner)

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-11-16 07:25:58 +0000
+++ lib/lp/bugs/configure.zcml	2011-11-21 00:29:30 +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-21 00:29:30 +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-19 05:44:39 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-11-21 00:29:30 +0000
@@ -155,6 +155,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
@@ -835,7 +836,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.")
@@ -844,7 +845,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.")
@@ -912,15 +913,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 (
@@ -1065,20 +1061,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:
@@ -1086,7 +1068,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.
@@ -1096,7 +1078,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`."""
@@ -1364,25 +1346,31 @@
         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.
+        return (
+            role.isOwner(self.pillar) or role.isOneOfDrivers(self.pillar) or
+            role.isBugSupervisor(self.pillar) 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/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-11-14 12:23:59 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-11-21 00:29:30 +0000
@@ -2590,3 +2590,68 @@
         # Check the delete really worked.
         with person_logged_in(removeSecurityProxy(db_bug).owner):
             self.assertEqual([db_bug.default_bugtask], db_bug.bugtasks)
+
+
+class TestBugTaskUserHasPriviliges(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugTaskUserHasPriviliges, self).setUp()
+        self.celebrities = getUtility(ILaunchpadCelebrities)
+
+    def test_admin_is_allowed(self):
+        # An admin always has privileges.
+        bugtask = self.factory.makeBugTask()
+        self.assertTrue(bugtask.userHasPrivileges(self.celebrities.admin))
+
+    def test_bug_celebrities_are_allowed(self):
+        # The three bug celebrities (bug watcher, bug importer and
+        # janitor always have privileges.
+        bugtask = self.factory.makeBugTask()
+        for celeb in (
+            self.celebrities.bug_watch_updater,
+            self.celebrities.bug_importer, self.celebrities.janitor):
+            self.assertTrue(bugtask.userHasPrivileges(celeb))
+
+    def test_pillar_owner_is_allowed(self):
+        # The pillar owner has privileges.
+        pillar = self.factory.makeProduct()
+        bugtask = self.factory.makeBugTask(target=pillar)
+        self.assertTrue(bugtask.userHasPrivileges(pillar.owner))
+
+    def test_pillar_driver_is_allowed(self):
+        # The pillar driver has privileges.
+        pillar = self.factory.makeProduct()
+        removeSecurityProxy(pillar).driver = self.factory.makePerson()
+        bugtask = self.factory.makeBugTask(target=pillar)
+        self.assertTrue(bugtask.userHasPrivileges(pillar.driver))
+
+    def test_pillar_bug_supervisor(self):
+        # The pillar bug supervisor has privileges.
+        pillar = self.factory.makeProduct()
+        bugsupervisor = self.factory.makePerson()
+        removeSecurityProxy(pillar).setBugSupervisor(
+            bugsupervisor, self.celebrities.admin)
+        bugtask = self.factory.makeBugTask(target=pillar)
+        self.assertTrue(bugtask.userHasPrivileges(bugsupervisor))
+
+    def test_productseries_driver_is_allowed(self):
+        # The series driver has privileges.
+        series = self.factory.makeProductSeries()
+        removeSecurityProxy(series).driver = self.factory.makePerson()
+        bugtask = self.factory.makeBugTask(target=series)
+        self.assertTrue(bugtask.userHasPrivileges(series.driver))
+
+    def test_distroseries_driver_is_allowed(self):
+        # The series driver has privileges.
+        distroseries = self.factory.makeDistroSeries()
+        removeSecurityProxy(distroseries).driver = self.factory.makePerson()
+        bugtask = self.factory.makeBugTask(target=distroseries)
+        self.assertTrue(bugtask.userHasPrivileges(distroseries.driver))
+
+    def test_random_has_no_privileges(self):
+        # Joe Random has no privileges.
+        bugtask = self.factory.makeBugTask()
+        self.assertFalse(
+            bugtask.userHasPrivileges(self.factory.makePerson()))

=== 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-21 00:29:30 +0000
@@ -29,11 +29,9 @@
     Traceback (most recent call last):
     ...
     LinkNotFoundError
+
     >>> print admin_browser.getLink('Low', index=1).url
-    Traceback (most recent call last):
-    ...
-    LinkNotFoundError
-
+    http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/1/+editstatus
     >>> print admin_browser.getLink('New', index=1).url
     http://bugs...dev/ubuntu/+source/mozilla-firefox/+bug/1/+editstatus
     >>> print admin_browser.getLink('Medium').url

=== 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-21 00:29:30 +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-21 00:29:30 +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"