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
1=== modified file 'lib/lp/bugs/model/bugtask.py'
2--- lib/lp/bugs/model/bugtask.py 2010-10-26 02:02:12 +0000
3+++ lib/lp/bugs/model/bugtask.py 2010-11-02 18:14:35 +0000
4@@ -888,11 +888,14 @@
5 def canTransitionToStatus(self, new_status, user):
6 """See `IBugTask`."""
7 celebrities = getUtility(ILaunchpadCelebrities)
8- if (user.inTeam(self.pillar.bug_supervisor) or
9- user.inTeam(self.pillar.owner) or
10- user.id == celebrities.bug_watch_updater.id or
11- user.id == celebrities.bug_importer.id or
12- user.id == celebrities.janitor.id):
13+ if (self.status == BugTaskStatus.FIXRELEASED and
14+ (user.id == self.bug.ownerID or user.inTeam(self.bug.owner))):
15+ return True
16+ elif (user.inTeam(self.pillar.bug_supervisor) or
17+ user.inTeam(self.pillar.owner) or
18+ user.id == celebrities.bug_watch_updater.id or
19+ user.id == celebrities.bug_importer.id or
20+ user.id == celebrities.janitor.id):
21 return True
22 else:
23 return (self.status not in (
24
25=== modified file 'lib/lp/bugs/model/tests/test_bugtask_status.py'
26--- lib/lp/bugs/model/tests/test_bugtask_status.py 2010-10-25 12:57:30 +0000
27+++ lib/lp/bugs/model/tests/test_bugtask_status.py 2010-11-02 18:14:35 +0000
28@@ -9,7 +9,7 @@
29 from zope.security.proxy import removeSecurityProxy
30
31 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
32-from canonical.testing.layers import LaunchpadFunctionalLayer
33+from canonical.testing.layers import DatabaseFunctionalLayer
34 from lp.bugs.interfaces.bugtask import UserCannotEditBugTaskStatus
35 from lp.bugs.model.bugtask import BugTaskStatus
36 from lp.testing import (
37@@ -21,7 +21,7 @@
38 class TestBugTaskStatusTransitionForUser(TestCaseWithFactory):
39 """Test bugtask status transitions for a regular logged in user."""
40
41- layer = LaunchpadFunctionalLayer
42+ layer = DatabaseFunctionalLayer
43
44 def setUp(self):
45 super(TestBugTaskStatusTransitionForUser, self).setUp()
46@@ -148,10 +148,51 @@
47 False)
48
49
50+class TestBugTaskStatusTransitionForReporter(TestCaseWithFactory):
51+ """Tests for bug reporter status transitions."""
52+
53+ layer = DatabaseFunctionalLayer
54+
55+ def setUp(self):
56+ super(TestBugTaskStatusTransitionForReporter, self).setUp()
57+ self.task = self.factory.makeBugTask()
58+ self.reporter = self.task.bug.owner
59+
60+ def test_reporter_can_unset_fix_released_status(self):
61+ # The bug reporter can transition away from Fix Released.
62+ removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
63+ with person_logged_in(self.reporter):
64+ self.task.transitionToStatus(
65+ BugTaskStatus.CONFIRMED, self.reporter)
66+ self.assertEqual(self.task.status, BugTaskStatus.CONFIRMED)
67+
68+ def test_reporter_canTransitionToStatus(self):
69+ # The bug reporter can transition away from Fix Released, so
70+ # canTransitionToStatus should always return True.
71+ removeSecurityProxy(self.task).status = BugTaskStatus.FIXRELEASED
72+ self.assertEqual(
73+ self.task.canTransitionToStatus(
74+ BugTaskStatus.CONFIRMED, self.reporter),
75+ True)
76+
77+ def test_reporter_team_can_unset_fix_released_status(self):
78+ # The bug reporter can be a team in the case of bug imports
79+ # and needs to be able to transition away from Fix Released.
80+ team = self.factory.makeTeam(members=[self.reporter])
81+ team_bug = self.factory.makeBug(owner=team)
82+ naked_task = removeSecurityProxy(team_bug.default_bugtask)
83+ naked_task.status = BugTaskStatus.FIXRELEASED
84+ with person_logged_in(self.reporter):
85+ team_bug.default_bugtask.transitionToStatus(
86+ BugTaskStatus.CONFIRMED, self.reporter)
87+ self.assertEqual(
88+ team_bug.default_bugtask.status, BugTaskStatus.CONFIRMED)
89+
90+
91 class TestBugTaskStatusTransitionForPrivilegedUserBase:
92 """Base class used to test privileged users and status transitions."""
93
94- layer = LaunchpadFunctionalLayer
95+ layer = DatabaseFunctionalLayer
96
97 def setUp(self):
98 super(TestBugTaskStatusTransitionForPrivilegedUserBase, self).setUp()