launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/less-restrictive-assign-someone-else-603281 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #603281 bug assignment permissions too restrictive
  https://bugs.launchpad.net/bugs/603281


This branch fixes bug 603281, which notes that when we changed
permissions for who can assign a bug to someone else that we made that a
bit to strict for projects without a bug supervisor.

This branch fixes that by lifting the restrictions if a bug supervisor
is not set for a project/distro.  This means that projects that haven't
yet set a bug supervisor will be able to assign bugs without having to
set a bug supervisor.  The functionality returns to what it once was.

But for projects that have a bug supervisor, the new requirements will
remain, i.e. that you cannot assign a bug to someone else.

This includes a test update and then the couple line change to the
permissions check method to get this working.  Since the web UI uses
this method, nothing had to be changed with the JS widget.
-- 
https://code.launchpad.net/~deryck/launchpad/less-restrictive-assign-someone-else-603281/+merge/33824
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/less-restrictive-assign-someone-else-603281 into lp:launchpad/devel.

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2010-08-26 19:31:02 +0000
@@ -737,8 +737,9 @@
     def userCanSetAnyAssignee(user):
         """Check if the current user can set anybody sa a bugtask assignee.
 
-        Return True for project owner, project drivers, series drivers,
-        bug supervisors and Launchpad admins; return False for other users.
+        Owners, drivers, bug supervisors and Launchpad admins can always
+        assign to someone else.  Other users can assign to someone else if a
+        bug supervisor is not defined.
         """
 
     def userCanUnassign(user):

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-08-24 15:29:01 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-08-26 19:31:02 +0000
@@ -1012,16 +1012,19 @@
     def userCanSetAnyAssignee(self, user):
         """See `IBugTask`."""
         celebrities = getUtility(ILaunchpadCelebrities)
-        return user is not None and (
-            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)
+        if user is not None and self.pillar.bug_supervisor is None:
+            return True
+        else:
+            return user is not None and (
+                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 userCanUnassign(self, user):
         """True if user can set the assignee to None.

=== modified file 'lib/lp/bugs/tests/test_bugtask.py'
--- lib/lp/bugs/tests/test_bugtask.py	2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/tests/test_bugtask.py	2010-08-26 19:31:02 +0000
@@ -541,15 +541,9 @@
         self.regular_user = self.factory.makePerson()
 
         login_person(self.target_owner_member)
+        # Target and bug supervisor creation are deferred to sub-classes.
         self.makeTarget()
-
-        self.supervisor_team = self.factory.makeTeam(
-            owner=self.target_owner_member)
-        self.supervisor_member = self.factory.makePerson()
-        self.supervisor_team.addMember(
-            self.supervisor_member, self.target_owner_member)
-        self.target.setBugSupervisor(
-            self.supervisor_team, self.target_owner_member)
+        self.setBugSupervisor()
 
         self.driver_team = self.factory.makeTeam(
             owner=self.target_owner_member)
@@ -582,6 +576,29 @@
         """
         raise NotImplementedError(self.makeTarget)
 
+    def setBugSupervisor(self):
+        """Set bug supervisor variables.
+
+        This is the standard interface for sub-classes, but this
+        method should return _setBugSupervisorData or
+        _setBugSupervisorDataNone depending on what is required.
+        """
+        raise NotImplementedError(self.setBugSupervisor)
+
+    def _setBugSupervisorData(self):
+        """Helper function used by sub-classes to setup bug supervisors."""
+        self.supervisor_team = self.factory.makeTeam(
+            owner=self.target_owner_member)
+        self.supervisor_member = self.factory.makePerson()
+        self.supervisor_team.addMember(
+            self.supervisor_member, self.target_owner_member)
+        self.target.setBugSupervisor(
+            self.supervisor_team, self.target_owner_member)
+
+    def _setBugSupervisorDataNone(self):
+        """Helper for sub-classes to work around setting a bug supervisor."""
+        self.supervisor_member = None
+
     def test_userCanSetAnyAssignee_anonymous_user(self):
         # Anonymous users cannot set anybody as an assignee.
         login(ANONYMOUS)
@@ -595,12 +612,20 @@
         self.assertFalse(self.series_bugtask.userCanUnassign(None))
 
     def test_userCanSetAnyAssignee_regular_user(self):
-        # Ordinary users cannot set others as an assignee.
+        # If we have a bug supervisor, check that regular user cannot
+        # assign to someone else.  Otherwise, the regular user should
+        # be able to assign to anyone.
         login_person(self.regular_user)
-        self.assertFalse(
-            self.target_bugtask.userCanSetAnyAssignee(self.regular_user))
-        self.assertFalse(
-            self.series_bugtask.userCanSetAnyAssignee(self.regular_user))
+        if self.supervisor_member is not None:
+            self.assertFalse(
+                self.target_bugtask.userCanSetAnyAssignee(self.regular_user))
+            self.assertFalse(
+                self.series_bugtask.userCanSetAnyAssignee(self.regular_user))
+        else:
+            self.assertTrue(
+                self.target_bugtask.userCanSetAnyAssignee(self.regular_user))
+            self.assertTrue(
+                self.series_bugtask.userCanSetAnyAssignee(self.regular_user))
 
     def test_userCanUnassign_regular_user(self):
         # Ordinary users can unassign themselves...
@@ -640,19 +665,23 @@
 
     def test_userCanSetAnyAssignee_bug_supervisor(self):
         # A bug supervisor can assign anybody.
-        login_person(self.supervisor_member)
-        self.assertTrue(
-            self.target_bugtask.userCanSetAnyAssignee(self.supervisor_member))
-        self.assertTrue(
-            self.series_bugtask.userCanSetAnyAssignee(self.supervisor_member))
+        if self.supervisor_member is not None:
+            login_person(self.supervisor_member)
+            self.assertTrue(
+                self.target_bugtask.userCanSetAnyAssignee(
+                    self.supervisor_member))
+            self.assertTrue(
+                self.series_bugtask.userCanSetAnyAssignee(
+                    self.supervisor_member))
 
     def test_userCanUnassign_bug_supervisor(self):
         # A bug supervisor can unassign anybody.
-        login_person(self.supervisor_member)
-        self.assertTrue(
-            self.target_bugtask.userCanUnassign(self.supervisor_member))
-        self.assertTrue(
-            self.series_bugtask.userCanUnassign(self.supervisor_member))
+        if self.supervisor_member is not None:
+            login_person(self.supervisor_member)
+            self.assertTrue(
+                self.target_bugtask.userCanUnassign(self.supervisor_member))
+            self.assertTrue(
+                self.series_bugtask.userCanUnassign(self.supervisor_member))
 
     def test_userCanSetAnyAssignee_driver(self):
         # A project driver can assign anybody.
@@ -733,6 +762,23 @@
         self.target = self.factory.makeProduct(owner=self.target_owner_team)
         self.series = self.factory.makeProductSeries(self.target)
 
+    def setBugSupervisor(self):
+        """Establish a bug supervisor for this target."""
+        self._setBugSupervisorData()
+
+
+class TestProductNoBugSupervisorBugTaskPermissionsToSetAssignee(
+    TestBugTaskPermissionsToSetAssigneeMixin, TestCaseWithFactory):
+
+    def makeTarget(self):
+        """Create a product and a product series without a bug supervisor."""
+        self.target = self.factory.makeProduct(owner=self.target_owner_team)
+        self.series = self.factory.makeProductSeries(self.target)
+
+    def setBugSupervisor(self):
+        """Set bug supervisor to None."""
+        self._setBugSupervisorDataNone()
+
 
 class TestDistributionBugTaskPermissionsToSetAssignee(
     TestBugTaskPermissionsToSetAssigneeMixin, TestCaseWithFactory):
@@ -743,6 +789,24 @@
             owner=self.target_owner_team)
         self.series = self.factory.makeDistroSeries(self.target)
 
+    def setBugSupervisor(self):
+        """Set bug supervisor to None."""
+        self._setBugSupervisorData()
+
+
+class TestDistributionNoBugSupervisorBugTaskPermissionsToSetAssignee(
+    TestBugTaskPermissionsToSetAssigneeMixin, TestCaseWithFactory):
+
+    def makeTarget(self):
+        """Create a distribution and a distroseries."""
+        self.target = self.factory.makeDistribution(
+            owner=self.target_owner_team)
+        self.series = self.factory.makeDistroSeries(self.target)
+
+    def setBugSupervisor(self):
+        """Establish a bug supervisor for this target."""
+        self._setBugSupervisorDataNone()
+
 
 class TestBugTaskSearch(TestCaseWithFactory):