Merge lp:~sinzui/launchpad/hide-license-info into lp:launchpad

Proposed by Curtis Hovey on 2011-03-07
Status: Merged
Approved by: Curtis Hovey on 2011-03-07
Approved revision: no longer in the source branch.
Merged at revision: 12546
Proposed branch: lp:~sinzui/launchpad/hide-license-info
Merge into: lp:launchpad
Diff against target: 157 lines (+60/-36)
4 files modified
lib/lp/registry/browser/product.py (+7/-1)
lib/lp/registry/browser/tests/product-views.txt (+0/-31)
lib/lp/registry/browser/tests/test_product.py (+49/-0)
lib/lp/registry/templates/product-index.pt (+4/-4)
To merge this branch: bzr merge lp:~sinzui/launchpad/hide-license-info
Reviewer Review Type Date Requested Status
Benji York (community) code 2011-03-07 Approve on 2011-03-07
Review via email: mp+52467@code.launchpad.net

Description of the change

Hide license_info when Other/* licenses are not selected.

    Launchpad bug: https://bugs.launchpad.net/bugs/490148
    Pre-implementation: no one
    Test command: ./bin/test -vv -t TestProductView

The Other/Open Source and Other/Proprietary license info is shown even
when Other/Open Source and Other/Proprietary are not selected. The
information in the field is often historical, and may contradict the
current license. Some users do not know that they need to manually delete
the text when editing the licenses, and they may not see the field once
they have unselected the license. Some users do not want to delete the
historical info, they want it hidden.

I have spent several hours this year helping users with this. I do not
want to explain this again. I can fix the display issue in less time that
it takes to help two more users.

--------------------------------------------------------------------

RULES

    * Add a rule to the view to tell the template to show the field
      when Other/Open Source or Other/Proprietary is selected.

QA

    * Visit https://launchpad.net/javaframework
    * Verify it does not state it is both GPL and CC-NC.

LINT

    lib/lp/registry/browser/product.py
    lib/lp/registry/browser/tests/product-views.txt
    lib/lp/registry/browser/tests/test_product.py
    lib/lp/registry/templates/product-index.pt

IMPLEMENTATION

Replaced an doctest for the ProductView with a unittest.
    lib/lp/registry/browser/tests/product-views.txt
    lib/lp/registry/browser/tests/test_product.py

Added a property to the ProductView to tell the template when to show
the license_info field.
    lib/lp/registry/browser/product.py
    lib/lp/registry/browser/tests/test_product.py

Updated the template to use the new property. Fixed a subtle issue with
validating script tag...a html-aware checker fails the template because
the script looks empty, which is invalid HTML markup.
    lib/lp/registry/templates/product-index.pt

To post a comment you must log in.
Benji York (benji) wrote :

Looks good. Straightforward and well tested.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/product.py'
2--- lib/lp/registry/browser/product.py 2011-02-05 06:11:48 +0000
3+++ lib/lp/registry/browser/product.py 2011-03-07 20:29:17 +0000
4@@ -42,7 +42,6 @@
5 ]
6
7
8-from cgi import escape
9 from datetime import (
10 datetime,
11 timedelta,
12@@ -1110,6 +1109,13 @@
13 return (check_permission('launchpad.Edit', self.context) or
14 check_permission('launchpad.Commercial', self.context))
15
16+ @cachedproperty
17+ def show_license_info(self):
18+ """Should the view show the extra license information."""
19+ return (
20+ License.OTHER_OPEN_SOURCE in self.context.licenses
21+ or License.OTHER_PROPRIETARY in self.context.licenses)
22+
23
24 class ProductPackagesView(LaunchpadView):
25 """View for displaying product packaging"""
26
27=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
28--- lib/lp/registry/browser/tests/product-views.txt 2010-10-29 15:44:25 +0000
29+++ lib/lp/registry/browser/tests/product-views.txt 2011-03-07 20:29:17 +0000
30@@ -441,37 +441,6 @@
31 Product index view
32 ==================
33
34-Programming languages
35----------------------
36-
37-When the product has no programming languages specified, nothing is displayed
38-on the index page.
39-
40- >>> login('no-priv@canonical.com')
41- >>> view = create_initialized_view(firefox, name='+index')
42- >>> view.show_programming_languages
43- False
44-
45-But if the project owner logs in, they will see the programming language
46-details, so that they can edit them.
47-
48- >>> login_person(firefox.owner)
49- >>> view = create_initialized_view(firefox, name='+index')
50- >>> view.show_programming_languages
51- True
52-
53-Once the programming languages are specified though, even users who aren't the
54-owner will see the list.
55-
56- >>> firefox.programminglang = 'C++'
57- >>> transaction.commit()
58-
59- >>> login('no-priv@canonical.com')
60- >>> view = create_initialized_view(firefox, name='+index')
61- >>> view.show_programming_languages
62- True
63-
64-
65 +index portlets
66 ---------------
67
68
69=== modified file 'lib/lp/registry/browser/tests/test_product.py'
70--- lib/lp/registry/browser/tests/test_product.py 2010-11-10 22:04:20 +0000
71+++ lib/lp/registry/browser/tests/test_product.py 2011-03-07 20:29:17 +0000
72@@ -23,6 +23,7 @@
73 )
74 from lp.testing import (
75 login_person,
76+ person_logged_in,
77 TestCaseWithFactory,
78 )
79 from lp.testing.mail_helpers import pop_notifications
80@@ -169,3 +170,51 @@
81 view = create_initialized_view(self.product_set, '+new')
82 message = find_tag_by_id(view.render(), 'staging-message')
83 self.assertEqual(None, message)
84+
85+
86+class TestProductView(TestCaseWithFactory):
87+ """Tests the ProductView."""
88+
89+ layer = DatabaseFunctionalLayer
90+
91+ def setUp(self):
92+ super(TestProductView, self).setUp()
93+ self.product = self.factory.makeProduct()
94+
95+ def test_show_programming_languages_without_languages(self):
96+ # show_programming_languages is false when there are no programming
97+ # languages set.
98+ view = create_initialized_view(self.product, '+index')
99+ self.assertEqual(None, self.product.programminglang)
100+ self.assertFalse(view.show_programming_languages)
101+
102+ def test_show_programming_languages_with_languages(self):
103+ # show_programming_languages is true when programming languages
104+ # are set.
105+ with person_logged_in(self.product.owner):
106+ self.product.programminglang = 'C++'
107+ view = create_initialized_view(self.product, '+index')
108+ self.assertTrue(view.show_programming_languages)
109+
110+ def test_show_license_info_without_other_license(self):
111+ # show_license_info is false when one of the "other" licenses is
112+ # not selected.
113+ view = create_initialized_view(self.product, '+index')
114+ self.assertEqual((License.GNU_GPL_V2, ), self.product.licenses)
115+ self.assertFalse(view.show_license_info)
116+
117+ def test_show_license_info_with_other_open_source_license(self):
118+ # show_license_info is true when the Other/Open Source license is
119+ # selected.
120+ view = create_initialized_view(self.product, '+index')
121+ with person_logged_in(self.product.owner):
122+ self.product.licenses = [License.OTHER_OPEN_SOURCE]
123+ self.assertTrue(view.show_license_info)
124+
125+ def test_show_license_info_with_other_open_proprietary_license(self):
126+ # show_license_info is true when the Other/Proprietary license is
127+ # selected.
128+ view = create_initialized_view(self.product, '+index')
129+ with person_logged_in(self.product.owner):
130+ self.product.licenses = [License.OTHER_PROPRIETARY]
131+ self.assertTrue(view.show_license_info)
132
133=== modified file 'lib/lp/registry/templates/product-index.pt'
134--- lib/lp/registry/templates/product-index.pt 2010-11-10 15:33:47 +0000
135+++ lib/lp/registry/templates/product-index.pt 2011-03-07 20:29:17 +0000
136@@ -41,7 +41,7 @@
137 },
138 window);
139 });
140- "/>
141+ "></script>
142 </tal:head-epilogue>
143 </head>
144
145@@ -162,9 +162,9 @@
146 condition="not:repeat/license/end">,</tal:comma>
147 </tal:licenses>
148 <tal:none condition="not:context/licenses">None specified</tal:none>
149- <tal:has_description condition="context/license_info">
150- (<tal:description replace="context/license_info" />)
151- </tal:has_description>
152+ <tal:has_license_info condition="view/show_license_info">
153+ (<tal:license_info replace="context/license_info" />)
154+ </tal:has_license_info>
155 </dd>
156 <dd id="commercial_subscription"
157 tal:condition="context/commercial_subscription">