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

[Guilherme Salgado]

Update the builder_details view to omit details of the current job if the user has no rights to see it

[James Tunnicliffe (dooferlad)]

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

[lp:~salgado/offspring/builder-details]

69. By Guilherme Salgado on 2011-10-18

Add a comment

[Guilherme Salgado (salgado)]

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

[Guilherme Salgado (salgado)]

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

[James Tunnicliffe (dooferlad)]

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

review: Approve

=== modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
--- lib/offspring/web/queuemanager/tests/test_views.py	2011-10-18 20:40:38 +0000
+++ lib/offspring/web/queuemanager/tests/test_views.py	2011-10-18 20:48:25 +0000
@@ -103,7 +103,12 @@
 
 
 class BuilderListTests(TestCase):
-    view_name = 'offspring.web.queuemanager.views.builders'
+    view_name = 'builder_list'
+
+    def get_url(self, builder):
+        # The builder argument exists only so that the tests here also work
+        # for the builder_details view (see BuilderDetailsTests below).
+        return reverse(self.view_name)
 
     def setUp(self):
         super(BuilderListTests, self).setUp()
@@ -121,7 +126,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)
@@ -133,7 +138,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)
@@ -143,12 +148,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)
 
 
+class BuilderDetailsTests(BuilderListTests):
+    view_name = 'builder_details'
+
+    def get_url(self, builder):
+        return reverse(self.view_name, args=[builder.name])
+
+
 class BuildViewTests(TestCase):
 
     def test_public_build(self):
 
=== modified file 'lib/offspring/web/queuemanager/views.py'
--- lib/offspring/web/queuemanager/views.py	2011-10-18 20:40:38 +0000
+++ lib/offspring/web/queuemanager/views.py	2011-10-18 20:48:25 +0000
@@ -80,6 +80,10 @@
     # shown and the values represent whether or not the information about the
     # builder's current job should be shown to the current user.
     builder_list = {}
+    # Here we don't filter out the builders working on private projects (as we
+    # do with other stuff, using .accessible_by_user()) because we want to
+    # show that the builders exist and just hide the info about private
+    # projects the user is not allowed to see.
     builders = Lexbuilder.all_objects.order_by(
         "-is_active", "-machine_type", "-created_at")
     for builder in builders:
@@ -98,9 +102,18 @@
 
 @login_required
 def builder_details(request, builderName):
-    builder = Lexbuilder.objects.get(name=builderName)
+    # Here we don't filter out the builders working on private projects (as we
+    # do with other stuff, using .accessible_by_user()) because we want to
+    # show that the builders exist and just hide the info about private
+    # projects the user is not allowed to see.
+    builder = Lexbuilder.all_objects.get(name=builderName)
+    shown_job = 'Private build'
+    job = builder.current_job
+    if job is not None and job.is_visible_to(request.user):
+        shown_job = job
     pageData = {
         'builder': builder,
+        'current_job': shown_job,
         'pillar': 'builders',
         }
     return render_to_response(
 
=== modified file 'lib/offspring/web/templates/queuemanager/builder_details.html'
--- lib/offspring/web/templates/queuemanager/builder_details.html	2011-01-22 04:47:32 +0000
+++ lib/offspring/web/templates/queuemanager/builder_details.html	2011-10-18 20:48:25 +0000
@@ -60,7 +60,7 @@
 
 <dl>
     <dt>Job:</dt>
-    <dd>{{builder.current_build}}</dd>
+    <dd>{{current_job}}</dd>
 </dl>
 
 <dl>