Merge lp://qastaging/~salgado/offspring/builder-details into lp://qastaging/~linaro-automation/offspring/private-builds

On 18 October 2011 18:25, Guilherme Salgado
<email address hidden> wrote:
> Guilherme Salgado has proposed merging lp:~salgado/offspring/builder-details into lp:~linaro-infrastructure/offspring/private-builds with lp:~salgado/offspring/builder-list as a prerequisite.
>
> Requested reviews:
>  Linaro Infrastructure (linaro-infrastructure)
>
> For more details, see:
> https://code.launchpad.net/~salgado/offspring/builder-details/+merge/79719
>
> Update the builder_details view to omit details of the current job if the user has no rights to see it
> --
> https://code.launchpad.net/~salgado/offspring/builder-details/+merge/79719
> Your team Linaro Infrastructure is requested to review the proposed merge of lp:~salgado/offspring/builder-details into lp:~linaro-infrastructure/offspring/private-builds.
>
> === modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
> --- lib/offspring/web/queuemanager/tests/test_views.py  2011-10-18 17:24:46 +0000
> +++ lib/offspring/web/queuemanager/tests/test_views.py  2011-10-18 17:24:46 +0000
> @@ -106,7 +106,10 @@
>
>
>  class BuilderListTests(TestCase):
> -    view_name = 'offspring.web.queuemanager.views.builders'
> +    view_name = 'builder_list'
> +
> +    def get_url(self, builder):
> +        return reverse(self.view_name)

What purpose does sending builder to get_url have? I see that get_url
is a 1:1 replacement for the "reverse(self.view_name)" that it
replaces in the tests below. Is this part way through a change?

>     def setUp(self):
>         super(BuilderListTests, self).setUp()
> @@ -124,7 +127,7 @@
>     def test_build_details_always_shown_when_building_public_project(self):
>         builder = factory.makeLexbuilder()
>         make_user_and_login(self.client)
> -        response = self.client.get(reverse(self.view_name))
> +        response = self.client.get(self.get_url(builder))
>         self.assertContains(
>             response, builder.current_job.project.name, status_code=200,
>             msg_prefix=response.content)
> @@ -136,7 +139,7 @@
>         user = project.owner
>         self.assertTrue(
>             self.client.login(username=user.username, password=user.username))
> -        response = self.client.get(reverse(self.view_name))
> +        response = self.client.get(self.get_url(builder))
>         self.assertContains(
>             response, builder.current_job.project.name, status_code=200,
>             msg_prefix=response.content)
> @@ -146,12 +149,19 @@
>         job = factory.makeBuildResult(project=project)
>         builder = factory.makeLexbuilder(current_job=job)
>         make_user_and_login(self.client)
> -        response = self.client.get(reverse(self.view_name))
> +        response = self.client.get(self.get_url(builder))
>         self.assertNotContains(
>             response, builder.current_job.project.name, status_code=200,
>             msg_prefix=response.content)

<snip>

Thanks,

--
James Tunnicliffe

[...]
> > class BuilderListTests(TestCase):
> > - view_name = 'offspring.web.queuemanager.views.builders'
> > + view_name = 'builder_list'
> > +
> > + def get_url(self, builder):
> > + return reverse(self.view_name)
>
> What purpose does sending builder to get_url have? I see that get_url
> is a 1:1 replacement for the "reverse(self.view_name)" that it
> replaces in the tests below. Is this part way through a change?

Oh, no, it's because on the other test class in this diff I inherit this
one, and there I need the builder to get the view's URL.

I've added a comment to make that clear.

James, does this one look ok with the change I did?

Sorry, didn't spot that. Yes, that is fine.