Code review comment for lp:~wallyworld/launchpad/reassign-reviewer-cancel-option

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