Merge lp:~rvb/maas/bug-1413030 into lp:maas/trunk

Proposed by Raphaël Badin on 2015-01-21
Status: Merged
Approved by: Raphaël Badin on 2015-01-21
Approved revision: 3484
Merged at revision: 3482
Proposed branch: lp:~rvb/maas/bug-1413030
Merge into: lp:maas/trunk
Diff against target: 75 lines (+55/-2)
2 files modified
src/maasserver/templates/maasserver/snippets.html (+8/-2)
src/maasserver/views/tests/test_snippets.py (+47/-0)
To merge this branch: bzr merge lp:~rvb/maas/bug-1413030
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2015-01-21 Approve on 2015-01-21
Review via email: mp+247113@code.launchpad.net

Commit message

Also hide the labels when the fields themselves are hidden in the add_node HTML snippet.

Description of the change

The testing for this isn't pretty, I'll admit to that; this is kind of an emergency fix. As part of the current JS-rework, I think we should re-think the usage of snippets.html.

Now, instead of hiding the fields and the label, maybe we should investigate now displaying them at all but the change will be bigger and this is an emergency fix before releasing rc3.

To post a comment you must log in.
Gavin Panella (allenap) wrote :

+ self.assertEqual(
+ ['hidden', 'hidden'],
+ [os_system_class, distro_series_class])

Do this in two assertions, something like:

        os_system_class = add_node_snippet.cssselect(
            "[for=id_osystem]")[0].attrib['class']
        self.expectThat(os_system_class, Equals('hidden'))

        distro_series_class = add_node_snippet.cssselect(
            "[for=id_distro_series]")[0].attrib['class']
        self.expectThat(distro_series_class, Equals('hidden'))

Better still, express what you're trying to test:

        get_class = lambda node: node.attrib['class']
        node_is_hidden = AfterPreprocessing(get_class, Equals('hidden'))

        self.expectThat(
            add_node_snippet.cssselect("[for=id_osystem]")[0],
            node_is_hidden)
        self.expectThat(
            add_node_snippet.cssselect("[for=id_distro_series]")[0],
            node_is_hidden)

Or even:

        self.expectThat(
            add_node_snippet.cssselect(".hidden [for=id_osystem]"),
            Not(HasLength(0)))
        self.expectThat(
            add_node_snippet.cssselect(".hidden [for=id_distro_series]"),
            Not(HasLength(0)))

review: Approve
lp:~rvb/maas/bug-1413030 updated on 2015-01-21
3484. By Raphaël Badin on 2015-01-21

Review fixes.

Raphaël Badin (rvb) wrote :

All right, thanks for the review. I've switched to using two self.expectThat(...) expressions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/templates/maasserver/snippets.html'
2--- src/maasserver/templates/maasserver/snippets.html 2015-01-20 14:38:01 +0000
3+++ src/maasserver/templates/maasserver/snippets.html 2015-01-21 16:07:13 +0000
4@@ -19,11 +19,17 @@
5 </span>
6 </li>
7 <li>
8- <label for="id_osystem">OS</label>
9+ <label
10+ for="id_osystem"
11+ class="{% if node_form.osystem.is_hidden %}hidden{% endif %}">
12+ OS</label>
13 {{ node_form.osystem }}
14 </li>
15 <li>
16- <label for="id_distro_series">Release</label>
17+ <label
18+ for="id_distro_series"
19+ class="{% if node_form.distro_series.is_hidden %}hidden{% endif %}">
20+ Release</label>
21 {{ node_form.distro_series }}
22 </li>
23 <li class="license_key hidden">
24
25=== added file 'src/maasserver/views/tests/test_snippets.py'
26--- src/maasserver/views/tests/test_snippets.py 1970-01-01 00:00:00 +0000
27+++ src/maasserver/views/tests/test_snippets.py 2015-01-21 16:07:13 +0000
28@@ -0,0 +1,47 @@
29+# Copyright 2015 Canonical Ltd. This software is licensed under the
30+# GNU Affero General Public License version 3 (see the file LICENSE).
31+
32+"""Test maasserver snippets."""
33+
34+from __future__ import (
35+ absolute_import,
36+ print_function,
37+ unicode_literals,
38+ )
39+
40+str = None
41+
42+__metaclass__ = type
43+__all__ = []
44+
45+
46+from django.core.urlresolvers import reverse
47+from lxml.html import fromstring
48+from maasserver.testing.testcase import MAASServerTestCase
49+from testtools.matchers import (
50+ HasLength,
51+ Not,
52+ )
53+
54+
55+class SnippetsTest(MAASServerTestCase):
56+
57+ def test_index_page_containts_add_node_snippet(self):
58+ self.client_log_in()
59+ index_page = self.client.get(reverse('index'))
60+ doc = fromstring(index_page.content)
61+ self.assertEqual(
62+ 'text/x-template', doc.cssselect('#add-node')[0].attrib['type'])
63+
64+ def test_add_node_snippet_hides_osystem_distro_series_labels(self):
65+ self.client_log_in()
66+ index_page = self.client.get(reverse('index'))
67+ doc = fromstring(index_page.content)
68+ content_text = doc.cssselect('#add-node')[0].text_content()
69+ add_node_snippet = fromstring(content_text)
70+ self.expectThat(
71+ add_node_snippet.cssselect("label[for=id_osystem].hidden"),
72+ Not(HasLength(0)), "No hidden id_osystem label")
73+ self.expectThat(
74+ add_node_snippet.cssselect("label[for=id_distro_series].hidden"),
75+ Not(HasLength(0)), "No hidden id_distro_series label")