Merge lp:~salgado/offspring/builder_details into lp:offspring

[Guilherme Salgado]

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

Also shows a small padlock icon next to private projects on both the list of projects and the list of builds

[lp:~salgado/offspring/builder_details]

108. By Guilherme Salgado on 2011-12-15

merge list-views branch

[Kevin McDermott (bigkevmcd)]

=== modified file 'lib/offspring/web/queuemanager/tests/test_views.py'

+ # Stub Lexbuilder.average_load because it executes some SQL that's not
+ # supported by sqlite3 and we're not interested in testing it here.

Can you file a bug to fix this?

I would have fixed it when I was fixing the metrics code, but I'm not sure I fully understand what it's calculating (and the docstring isn't very helpful).

It looks to be calculating the % of the day that builds take for all builds that finish the same day as they start?

=== modified file 'lib/offspring/web/queuemanager/views.py'

+def builders(request):
+ # builder_list is a dictionary where keys are Lexbuilder instances to be
+ # shown and the values represent whether or not the information about the
+ # builder's current job should be shown to the current user.

Why no docstring?

+def builder_details(request, builderName):
+ # Here we don't filter out the builders working on private projects (as we

Again, no docstring, and this doesn't handle unknown builder names very well...

Other than that it looks pretty good...

My usual annoyances with things like...

+ projects = projects.select_related('project_group').order_by(
+ "project_group")

Why change between different quote types in the same statement?

And running PEP8 on view.py is horrific...but hey :-)

review: Needs Fixing

[Guilherme Salgado (salgado)]

Hi Kevin,

Thanks for the review!

On 11/01/12 05:59, Kevin McDermott wrote:
> Review: Needs Fixing
>
> === modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
>
> + # Stub Lexbuilder.average_load because it executes some SQL that's not
> + # supported by sqlite3 and we're not interested in testing it here.
>
> Can you file a bug to fix this?
>
> I would have fixed it when I was fixing the metrics code, but I'm not sure I fully understand what it's calculating (and the docstring isn't very helpful).
>
> It looks to be calculating the % of the day that builds take for all builds that finish the same day as they start?

Hmm, this was not introduced on this branch, and it was not clear to me
we want to restrict ourselves to the subset of SQL implemented by sqlite3.

>
> === modified file 'lib/offspring/web/queuemanager/views.py'
>
> +def builders(request):
> + # builder_list is a dictionary where keys are Lexbuilder instances to be
> + # shown and the values represent whether or not the information about the
> + # builder's current job should be shown to the current user.
>
> Why no docstring?
>
> +def builder_details(request, builderName):
> + # Here we don't filter out the builders working on private projects (as we
>
> Again, no docstring, and this doesn't handle unknown builder names very well...

Mostly because, IMHO, there wouldn't be much to say other than what one
can easily infer from the function name, but I've added them regardless
as it certainly won't hurt.

>
> Other than that it looks pretty good...
>
> My usual annoyances with things like...
>
> + projects = projects.select_related('project_group').order_by(
> + "project_group")
>
> Why change between different quote types in the same statement?

This also wasn't introduced here...

Ok, now I understand, this branch depends on list-view
(https://code.launchpad.net/~salgado/offspring/list-views/+merge/84291),
which is still up for review. I'll change this on that branch.

[lp:~salgado/offspring/builder_details]

109. By Guilherme Salgado on 2012-01-11

merge list-view branch

[Guilherme Salgado (salgado)]

I've actually done all the changes on the list-view branch as the relevant code was introduced there, but I've also merged that branch here again, so if you check the MP page you'll see the diff has the changes you suggested.

[lp:~salgado/offspring/builder_details]

110. By Guilherme Salgado on 2012-01-11

Fix the builder_details view to raise a 404 when the builder name is not known

[Kevin McDermott (bigkevmcd)]

Thanks for the fixes, +1

review: Approve

=== modified file 'Makefile'
--- Makefile	2011-12-12 22:24:42 +0000
+++ Makefile	2012-01-11 16:50:40 +0000
@@ -74,10 +74,10 @@
 --cover-package=offspring.web
 
 test-web: web web-test install-test-runner
-	./bin/offspring-web test offspring.web.queuemanager --settings=offspring.web.settings_test
+	./bin/offspring-web test offspring.web.queuemanager -s --settings=offspring.web.settings_test
 
 test-web-postgres: web web-test install-test-runner
-	./bin/offspring-web test queuemanager --settings=offspring.web.settings_devel
+	./bin/offspring-web test offspring.web.queuemanager -s --settings=offspring.web.settings_devel
 
 test: master slave web install-test-runner test-web master-test
 	./.virtualenv/bin/nosetests -e 'queuemanager.*'

=== modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
--- lib/offspring/web/queuemanager/tests/test_views.py	2012-01-11 14:50:20 +0000
+++ lib/offspring/web/queuemanager/tests/test_views.py	2012-01-11 16:50:40 +0000
@@ -146,7 +146,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()
@@ -168,7 +173,7 @@
         """
         builder = factory.make_lexbuilder()
         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)
@@ -184,7 +189,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)
@@ -201,15 +206,28 @@
         job = factory.make_build_result(project=project)
         builder = factory.make_lexbuilder(current_job=job)
         make_user_and_login(self.client)
-        response = self.client.get(reverse(self.view_name))
-        self.assertContains(
-            response, builder.name, status_code=200,
-            msg_prefix=response.content)
+        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])
+
+    def test_builder_details_with_unknown_builder_name(self):
+        """
+        If an unknown builder is specified, we should get a 404.
+        """
+        make_user_and_login(self.client)
+        url = reverse(self.view_name, args=[u"unknown-builder"])
+        response = self.client.get(url)
+        self.assertEqual(404, response.status_code)
+
+
 class BuildResultViewTests(TestCase):
 
     def test_public_build(self):

=== modified file 'lib/offspring/web/queuemanager/views.py'
--- lib/offspring/web/queuemanager/views.py	2012-01-11 14:50:20 +0000
+++ lib/offspring/web/queuemanager/views.py	2012-01-11 16:50:40 +0000
@@ -85,6 +85,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:
@@ -108,10 +112,19 @@
     If the Lexbuilder is building a private project the user is not allowed to
     see, the current job details are omitted.
     """
-    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 = get_object_or_404(Lexbuilder.all_objects, 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,
         "pillar": "builders",
+        "current_job": shown_job,
         }
     return render_to_response(
         "queuemanager/builder_details.html", pageData,

=== 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	2012-01-11 16:50:40 +0000
@@ -60,7 +60,7 @@
 
 <dl>
     <dt>Job:</dt>
-    <dd>{{builder.current_build}}</dd>
+    <dd>{{current_job}}</dd>
 </dl>
 
 <dl>

=== modified file 'lib/offspring/web/templates/queuemanager/builds.html'
--- lib/offspring/web/templates/queuemanager/builds.html	2011-08-13 06:30:36 +0000
+++ lib/offspring/web/templates/queuemanager/builds.html	2012-01-11 16:50:40 +0000
@@ -31,6 +31,10 @@
                 <a title="View project details" href="{% url offspring.web.queuemanager.views.project_details build_result.project.name %}">
                     {{ build_result.project.title }}
                 </a>
+                {% if build_result.project.is_private %}
+                  <img src="/assets/images/padlock-small.png"
+                       title="This project is private" />
+                {% endif %}
             </td>
             <td> 
                 {% if build_result.name %}

=== modified file 'lib/offspring/web/templates/queuemanager/projects.html'
--- lib/offspring/web/templates/queuemanager/projects.html	2011-08-13 06:30:36 +0000
+++ lib/offspring/web/templates/queuemanager/projects.html	2012-01-11 16:50:40 +0000
@@ -41,6 +41,10 @@
             </td>
             <td> 
                 <a href="{% url offspring.web.queuemanager.views.project_details project.name %}">{{ project.title }}</a>
+                {% if project.is_private %}
+                  <img src="/assets/images/padlock-small.png"
+                       title="This project is private" />
+                {% endif %}
             </td>
             <td> 
                 {{project.get_status_display|default_if_none:"Unknown"}}