Merge lp:~deryck/launchpad/allow-reporter-fixed-released-unsetting-664096 into lp:launchpad

[Deryck Hodge]

Allow the bug reporter to transition away from Fix Released bug task status.

This is the final branch required to completely fix bug 664096. Previously, the tests were updated in a branch and then the status was locked so that only bug supervisor or owner could transition away from Fix Released. This branch adds the ability for the bug reporter to transition away from fix released, so that the bug can be reopened by the reporter if it is not fixed.

Thanks for the review!

[Gavin Panella (allenap)]

Cool :) A few trivial comments, that's it.

[1]

+ 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

Not your code, but the following lines don't line up.

[2]

+ user == self.bug.owner):

Here (and elsewhere) inTeam() is not used. Works but seems odd to not
use inTeam() because it will DTRT for individuals and teams.

[3]

+ layer = LaunchpadFunctionalLayer

You might be able to get away with DatabaseFunctionalLayer here.

[4]

+ self.assertEqual(
+ self.task.canTransitionToStatus(
+ BugTaskStatus.CONFIRMED, self.reporter),
+ True)

self.assertTrue() would work here too.

review: Approve

[Robert Collins (lifeless)]

On Tue, Nov 2, 2010 at 5:46 AM, Gavin Panella
<email address hidden> wrote:
> Review: Approve
> Cool :) A few trivial comments, that's it.
> [2]
>
> +            user == self.bug.owner):
>
> Here (and elsewhere) inTeam() is not used. Works but seems odd to not
> use inTeam() because it will DTRT for individuals and teams.

It would be better to write this as

+ user.id == self.bug.ownerID):

Using the ID column avoids doing a dereference-and-load which you
don't want here. We've had timeouts fixed by little tweaks like this.

-Rob

[Gavin Panella (allenap)]

> It would be better to write this as
>
> + user.id == self.bug.ownerID):
>
> Using the ID column avoids doing a dereference-and-load which you
> don't want here. We've had timeouts fixed by little tweaks like this.

For bug owner it's okay, because I think it will only ever be an
individual. However, for assignee it can be a team, and comparing ID
won't work. Something like the following might be an okay compromise
for now:

    user.id == self.bug.assigneeID or
    user.inTeam(self.bug.assignee)

inTeam() could be changed to accept an ID and encapsulate this (and
perhaps also short-circuit to a query - instead of dereference first -
in the case of a team).

[Robert Collins (lifeless)]

On Tue, Nov 2, 2010 at 6:21 AM, Gavin Panella
<email address hidden> wrote:
> For bug owner it's okay, because I think it will only ever be an
> individual. However, for assignee it can be a team, and comparing ID
> won't work. Something like the following might be an okay compromise
> for now:
>
>    user.id == self.bug.assigneeID or
>    user.inTeam(self.bug.assignee)
>
> inTeam() could be changed to accept an ID and encapsulate this (and
> perhaps also short-circuit to a query - instead of dereference first -
> in the case of a team).

Something like that might be nice.

-Rob

[Gavin Panella (allenap)]

> > inTeam() could be changed to accept an ID and encapsulate this (and
> > perhaps also short-circuit to a query - instead of dereference first -
> > in the case of a team).
>
> Something like that might be nice.

Bug 669643 for anyone that's interested.

[Deryck Hodge (deryck)]

Thanks for the comments guys. I'm fixing things up and also adding a test for the corner case of bug reporter being a team, which can happen in bug imports. Gavin remembered this on IRC this morning.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-10-26 02:02:12 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-11-02 18:14:35 +0000
@@ -888,11 +888,14 @@
     def canTransitionToStatus(self, new_status, user):
         """See `IBugTask`."""
         celebrities = getUtility(ILaunchpadCelebrities)
-        if (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):
+        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):
             return True
         else:
             return (self.status not in (

=== modified file 'lib/lp/bugs/model/tests/test_bugtask_status.py'
--- lib/lp/bugs/model/tests/test_bugtask_status.py	2010-10-25 12:57:30 +0000
+++ lib/lp/bugs/model/tests/test_bugtask_status.py	2010-11-02 18:14:35 +0000
@@ -9,7 +9,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.interfaces.bugtask import UserCannotEditBugTaskStatus
 from lp.bugs.model.bugtask import BugTaskStatus
 from lp.testing import (
@@ -21,7 +21,7 @@
 class TestBugTaskStatusTransitionForUser(TestCaseWithFactory):
     """Test bugtask status transitions for a regular logged in user."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestBugTaskStatusTransitionForUser, self).setUp()
@@ -148,10 +148,51 @@
             False)
 
 
+class TestBugTaskStatusTransitionForReporter(TestCaseWithFactory):
+    """Tests for bug reporter status transitions."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugTaskStatusTransitionForReporter, self).setUp()
+        self.task = self.factory.makeBugTask()
+        self.reporter = self.task.bug.owner
+
+    def test_reporter_can_unset_fix_released_status(self):
+        # The bug reporter can transition away from Fix Released.
+        removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
+        with person_logged_in(self.reporter):
+            self.task.transitionToStatus(
+                BugTaskStatus.CONFIRMED, self.reporter)
+            self.assertEqual(self.task.status, BugTaskStatus.CONFIRMED)
+
+    def test_reporter_canTransitionToStatus(self):
+        # The bug reporter can transition away from Fix Released, so
+        # canTransitionToStatus should always return True.
+        removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
+        self.assertEqual(
+            self.task.canTransitionToStatus(
+                BugTaskStatus.CONFIRMED, self.reporter),
+            True)
+
+    def test_reporter_team_can_unset_fix_released_status(self):
+        # The bug reporter can be a team in the case of bug imports
+        # and needs to be able to transition away from Fix Released.
+        team = self.factory.makeTeam(members=[self.reporter])
+        team_bug = self.factory.makeBug(owner=team)
+        naked_task = removeSecurityProxy(team_bug.default_bugtask)
+        naked_task.status = BugTaskStatus.FIXRELEASED
+        with person_logged_in(self.reporter):
+            team_bug.default_bugtask.transitionToStatus(
+                BugTaskStatus.CONFIRMED, self.reporter)
+            self.assertEqual(
+                team_bug.default_bugtask.status, BugTaskStatus.CONFIRMED)
+
+
 class TestBugTaskStatusTransitionForPrivilegedUserBase:
     """Base class used to test privileged users and status transitions."""
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestBugTaskStatusTransitionForPrivilegedUserBase, self).setUp()