Launchpad itself

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

  1. allow-reporter-fixed-released-unsetting-664096
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 11849
Proposed branch: lp:~deryck/launchpad/allow-reporter-fixed-released-unsetting-664096
Merge into: lp:launchpad
Prerequisite: lp:~deryck/launchpad/lock-fix-released-status-126516
Diff against target: 98 lines (+52/-8)
2 files modified
lib/lp/bugs/model/bugtask.py (+8/-5)
lib/lp/bugs/model/tests/test_bugtask_status.py (+44/-3)
To merge this branch: bzr merge lp:~deryck/launchpad/allow-reporter-fixed-released-unsetting-664096
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+39754@code.launchpad.net

Commit message

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

Description of the change

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!

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Gavin Panella (allenap) wrote :

> 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).

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Gavin Panella (allenap) wrote :

> > 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.

Revision history for this message
Deryck Hodge (deryck) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
888 def canTransitionToStatus(self, new_status, user):888 def canTransitionToStatus(self, new_status, user):
889 """See `IBugTask`."""889 """See `IBugTask`."""
890 celebrities = getUtility(ILaunchpadCelebrities)890 celebrities = getUtility(ILaunchpadCelebrities)
891 if (user.inTeam(self.pillar.bug_supervisor) or891 if (self.status == BugTaskStatus.FIXRELEASED and
892 user.inTeam(self.pillar.owner) or892 (user.id == self.bug.ownerID or user.inTeam(self.bug.owner))):
893 user.id == celebrities.bug_watch_updater.id or893 return True
894 user.id == celebrities.bug_importer.id or894 elif (user.inTeam(self.pillar.bug_supervisor) or
895 user.id == celebrities.janitor.id):895 user.inTeam(self.pillar.owner) or
896 user.id == celebrities.bug_watch_updater.id or
897 user.id == celebrities.bug_importer.id or
898 user.id == celebrities.janitor.id):
896 return True899 return True
897 else:900 else:
898 return (self.status not in (901 return (self.status not in (
899902
=== 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 @@
9from zope.security.proxy import removeSecurityProxy9from zope.security.proxy import removeSecurityProxy
1010
11from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities11from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
12from canonical.testing.layers import LaunchpadFunctionalLayer12from canonical.testing.layers import DatabaseFunctionalLayer
13from lp.bugs.interfaces.bugtask import UserCannotEditBugTaskStatus13from lp.bugs.interfaces.bugtask import UserCannotEditBugTaskStatus
14from lp.bugs.model.bugtask import BugTaskStatus14from lp.bugs.model.bugtask import BugTaskStatus
15from lp.testing import (15from lp.testing import (
@@ -21,7 +21,7 @@
21class TestBugTaskStatusTransitionForUser(TestCaseWithFactory):21class TestBugTaskStatusTransitionForUser(TestCaseWithFactory):
22 """Test bugtask status transitions for a regular logged in user."""22 """Test bugtask status transitions for a regular logged in user."""
2323
24 layer = LaunchpadFunctionalLayer24 layer = DatabaseFunctionalLayer
2525
26 def setUp(self):26 def setUp(self):
27 super(TestBugTaskStatusTransitionForUser, self).setUp()27 super(TestBugTaskStatusTransitionForUser, self).setUp()
@@ -148,10 +148,51 @@
148 False)148 False)
149149
150150
151class TestBugTaskStatusTransitionForReporter(TestCaseWithFactory):
152 """Tests for bug reporter status transitions."""
153
154 layer = DatabaseFunctionalLayer
155
156 def setUp(self):
157 super(TestBugTaskStatusTransitionForReporter, self).setUp()
158 self.task = self.factory.makeBugTask()
159 self.reporter = self.task.bug.owner
160
161 def test_reporter_can_unset_fix_released_status(self):
162 # The bug reporter can transition away from Fix Released.
163 removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
164 with person_logged_in(self.reporter):
165 self.task.transitionToStatus(
166 BugTaskStatus.CONFIRMED, self.reporter)
167 self.assertEqual(self.task.status, BugTaskStatus.CONFIRMED)
168
169 def test_reporter_canTransitionToStatus(self):
170 # The bug reporter can transition away from Fix Released, so
171 # canTransitionToStatus should always return True.
172 removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
173 self.assertEqual(
174 self.task.canTransitionToStatus(
175 BugTaskStatus.CONFIRMED, self.reporter),
176 True)
177
178 def test_reporter_team_can_unset_fix_released_status(self):
179 # The bug reporter can be a team in the case of bug imports
180 # and needs to be able to transition away from Fix Released.
181 team = self.factory.makeTeam(members=[self.reporter])
182 team_bug = self.factory.makeBug(owner=team)
183 naked_task = removeSecurityProxy(team_bug.default_bugtask)
184 naked_task.status = BugTaskStatus.FIXRELEASED
185 with person_logged_in(self.reporter):
186 team_bug.default_bugtask.transitionToStatus(
187 BugTaskStatus.CONFIRMED, self.reporter)
188 self.assertEqual(
189 team_bug.default_bugtask.status, BugTaskStatus.CONFIRMED)
190
191
151class TestBugTaskStatusTransitionForPrivilegedUserBase:192class TestBugTaskStatusTransitionForPrivilegedUserBase:
152 """Base class used to test privileged users and status transitions."""193 """Base class used to test privileged users and status transitions."""
153194
154 layer = LaunchpadFunctionalLayer195 layer = DatabaseFunctionalLayer
155196
156 def setUp(self):197 def setUp(self):
157 super(TestBugTaskStatusTransitionForPrivilegedUserBase, self).setUp()198 super(TestBugTaskStatusTransitionForPrivilegedUserBase, self).setUp()