Launchpad itself

Merge lp:~wallyworld/launchpad/reassign-reviewer-cancel-option into lp:launchpad

  1. reassign-reviewer-cancel-option
  2. Merge into devel
Proposed by Ian Booth
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 11649
Proposed branch: lp:~wallyworld/launchpad/reassign-reviewer-cancel-option
Merge into: lp:launchpad
Diff against target: 99 lines (+30/-13)
2 files modified
lib/lp/code/browser/codereviewvote.py (+7/-2)
lib/lp/code/browser/tests/test_codereviewvote.py (+23/-11)
To merge this branch: bzr merge lp:~wallyworld/launchpad/reassign-reviewer-cancel-option
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Steve Kowalik (community) code* Approve
Launchpad code reviewers code Pending
Review via email: mp+36671@code.launchpad.net

Commit message

Add Cancel URL to the merge proposal Reassign Reviewer page

Description of the change

Bug 640193 requires that a Cancel link be on the merge proposal Reassign Reviewer page.

Implementation

    Simple fix - assign cancel_url in CodeReviewVoteReassign initialize() method. Also a few drive-by lint fixes.

Tests

    test -vvt test_codereviewvote

    Add new test_view_attributes() test to test_codereviewvote.py

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/codereviewvote.py
  lib/lp/code/browser/tests/test_codereviewvote.py

To post a comment you must log in.
Revision history for this message
Steve Kowalik (stevenk) wrote :

This change looks good to me.

review: Approve (code*)
Revision history for this message
Tim Penhey (thumper) wrote :

Steve, when reviewing browser we try to avoid overriding initialize unless it
is really necessary (I don't entirely recall why), and provide as much as we
can using properties.

Also when writing tests for views there is a very useful test method called:
    create_initialized_view

So... with these two thoughts in mind I have the following suggestions:

    @property
    def next_url(self):
        return canonical_url(self.context.branch_merge_proposal)

    cancel_url = next_url

This means that you don't have to override the initialize method.

Also in the test, you should never have any database IDs, like +merge/1.

Better to assert that the cancel_url matches the canonical_url of the merge
proposal.

    self.assertEqual(canonical_url(bmp), view.cancel_url)

Also... use create_initialized_view rather than manually creating the review.

    view = create_initialized_view(vote, '+reassign')

I also find the flow of the test a little harder to follow with the multiple
login commands. I think it would read slightly better to use
with_person_logged_in (don't forget the future import if it isn't there
already).

    def test_view_cancel_url(self):
        # Check that the cancel url exists.
        bmp = self.factory.makeBranchMergeProposal()
        reviewer = self.factory.makePerson()
        with person_logged_in(bmp.registrant):
            vote = bmp.nominateReviewer(
                reviewer=reviewer, registrant=bmp.registrant)
        with person_logged_in(reviewer):
            view = create_initialized_view(vote, '+reassign')
            self.assertEqual(canonical_url(bmp), view.cancel_url)

A side note: we often use comments instead of docstrings for the test methods
because it isn't always easy to get a single line to describe the test.

review: Needs Fixing
Revision history for this message
Ian Booth (wallyworld) wrote :

Thanks for the tips. I'll address the issues raised. The approach taken
was based on looking at what had been done previously for say
MergeProposalEditView. However, I see now that case is in the minority
and most other implementations use properties :-) Trust me to find as a
template to copy from the "wrong" one :-)

The structure of the test itself, with the multiple logins and use of
docstring etc, was based on the test case just above it, the only other
one in that test class. I thought it best to maintain implementation
consistency between the two test cases. What I'll look to do then is
refactor the existing test case as well so that they are both a)
implemented as per crrent best practice, and b) consistent.

On 28/09/10 08:24, Tim Penhey wrote:
> Review: Needs Fixing
> Steve, when reviewing browser we try to avoid overriding initialize unless it
> is really necessary (I don't entirely recall why), and provide as much as we
> can using properties.
>
> Also when writing tests for views there is a very useful test method called:
> create_initialized_view
>
> So... with these two thoughts in mind I have the following suggestions:
>
> @property
> def next_url(self):
> return canonical_url(self.context.branch_merge_proposal)
>
> cancel_url = next_url
>
> This means that you don't have to override the initialize method.
>
> Also in the test, you should never have any database IDs, like +merge/1.
>
> Better to assert that the cancel_url matches the canonical_url of the merge
> proposal.
>
> self.assertEqual(canonical_url(bmp), view.cancel_url)
>
> Also... use create_initialized_view rather than manually creating the review.
>
> view = create_initialized_view(vote, '+reassign')
>
> I also find the flow of the test a little harder to follow with the multiple
> login commands. I think it would read slightly better to use
> with_person_logged_in (don't forget the future import if it isn't there
> already).
>
> def test_view_cancel_url(self):
> # Check that the cancel url exists.
> bmp = self.factory.makeBranchMergeProposal()
> reviewer = self.factory.makePerson()
> with person_logged_in(bmp.registrant):
> vote = bmp.nominateReviewer(
> reviewer=reviewer, registrant=bmp.registrant)
> with person_logged_in(reviewer):
> view = create_initialized_view(vote, '+reassign')
> self.assertEqual(canonical_url(bmp), view.cancel_url)
>
> A side note: we often use comments instead of docstrings for the test methods
> because it isn't always easy to get a single line to describe the test.
>

Revision history for this message
Tim Penhey (thumper) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/codereviewvote.py'
2--- lib/lp/code/browser/codereviewvote.py 2010-08-20 20:31:18 +0000
3+++ lib/lp/code/browser/codereviewvote.py 2010-09-27 23:15:58 +0000
4@@ -24,7 +24,7 @@
5 class ReassignSchema(Interface):
6 """Schema to use when reassigning the reviewer for a requested review."""
7
8- reviewer = PublicPersonChoice( title=_('Reviewer'), required=True,
9+ reviewer = PublicPersonChoice(title=_('Reviewer'), required=True,
10 description=_('A person who you want to review this.'),
11 vocabulary='ValidPersonOrTeam')
12
13@@ -36,11 +36,16 @@
14
15 page_title = label = 'Reassign review request'
16
17+ @property
18+ def next_url(self):
19+ return canonical_url(self.context.branch_merge_proposal)
20+
21+ cancel_url = next_url
22+
23 @action('Reassign', name='reassign')
24 def reassign_action(self, action, data):
25 """Use the form data to change the review request reviewer."""
26 self.context.reassignReview(data['reviewer'])
27- self.next_url = canonical_url(self.context.branch_merge_proposal)
28
29 def validate(self, data):
30 """Make sure that the reassignment can happen."""
31
32=== modified file 'lib/lp/code/browser/tests/test_codereviewvote.py'
33--- lib/lp/code/browser/tests/test_codereviewvote.py 2010-08-20 20:31:18 +0000
34+++ lib/lp/code/browser/tests/test_codereviewvote.py 2010-09-27 23:15:58 +0000
35@@ -1,22 +1,22 @@
36 # Copyright 2009 Canonical Ltd. This software is licensed under the
37 # GNU Affero General Public License version 3 (see the file LICENSE).
38
39-
40 """Unit tests for CodeReviewVoteReferences."""
41
42+from __future__ import with_statement
43
44 __metaclass__ = type
45
46
47 import unittest
48
49-from canonical.launchpad.webapp.servers import LaunchpadTestRequest
50+from canonical.launchpad.webapp import canonical_url
51 from canonical.testing import DatabaseFunctionalLayer
52-from lp.code.browser.codereviewvote import CodeReviewVoteReassign
53 from lp.testing import (
54- login_person,
55+ person_logged_in,
56 TestCaseWithFactory,
57 )
58+from lp.testing.views import create_initialized_view
59
60
61 class TestReassignReviewer(TestCaseWithFactory):
62@@ -25,18 +25,30 @@
63 layer = DatabaseFunctionalLayer
64
65 def test_reassign(self):
66- """A reviewer can reassign their vote to someone else."""
67+ # A reviewer can reassign their vote to someone else.
68 bmp = self.factory.makeBranchMergeProposal()
69 reviewer = self.factory.makePerson()
70- login_person(bmp.registrant)
71- vote = bmp.nominateReviewer(
72- reviewer=reviewer, registrant=bmp.registrant)
73+ with person_logged_in(bmp.registrant):
74+ vote = bmp.nominateReviewer(
75+ reviewer=reviewer, registrant=bmp.registrant)
76 new_reviewer = self.factory.makePerson()
77- login_person(reviewer)
78- view = CodeReviewVoteReassign(vote, LaunchpadTestRequest())
79- view.reassign_action.success({'reviewer': new_reviewer})
80+ with person_logged_in(reviewer):
81+ view = create_initialized_view(vote, '+reassign')
82+ view.reassign_action.success({'reviewer': new_reviewer})
83 self.assertEqual(vote.reviewer, new_reviewer)
84
85+ def test_view_attributes(self):
86+ # Check various urls etc on view are correct.
87+ # At the moment, there's just the one: cancel_url
88+ bmp = self.factory.makeBranchMergeProposal()
89+ reviewer = self.factory.makePerson()
90+ with person_logged_in(bmp.registrant):
91+ vote = bmp.nominateReviewer(
92+ reviewer=reviewer, registrant=bmp.registrant)
93+ with person_logged_in(reviewer):
94+ view = create_initialized_view(vote, '+reassign')
95+ self.assertEqual(canonical_url(bmp), view.cancel_url)
96+
97
98 def test_suite():
99 return unittest.TestLoader().loadTestsFromName(__name__)