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
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py 2011-02-05 06:11:48 +0000
+++ lib/lp/registry/browser/product.py 2011-03-07 20:29:17 +0000
@@ -42,7 +42,6 @@
42 ]42 ]
4343
4444
45from cgi import escape
46from datetime import (45from datetime import (
47 datetime,46 datetime,
48 timedelta,47 timedelta,
@@ -1110,6 +1109,13 @@
1110 return (check_permission('launchpad.Edit', self.context) or1109 return (check_permission('launchpad.Edit', self.context) or
1111 check_permission('launchpad.Commercial', self.context))1110 check_permission('launchpad.Commercial', self.context))
11121111
1112 @cachedproperty
1113 def show_license_info(self):
1114 """Should the view show the extra license information."""
1115 return (
1116 License.OTHER_OPEN_SOURCE in self.context.licenses
1117 or License.OTHER_PROPRIETARY in self.context.licenses)
1118
11131119
1114class ProductPackagesView(LaunchpadView):1120class ProductPackagesView(LaunchpadView):
1115 """View for displaying product packaging"""1121 """View for displaying product packaging"""
11161122
=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
--- lib/lp/registry/browser/tests/product-views.txt 2010-10-29 15:44:25 +0000
+++ lib/lp/registry/browser/tests/product-views.txt 2011-03-07 20:29:17 +0000
@@ -441,37 +441,6 @@
441Product index view441Product index view
442==================442==================
443443
444Programming languages
445---------------------
446
447When the product has no programming languages specified, nothing is displayed
448on the index page.
449
450 >>> login('no-priv@canonical.com')
451 >>> view = create_initialized_view(firefox, name='+index')
452 >>> view.show_programming_languages
453 False
454
455But if the project owner logs in, they will see the programming language
456details, so that they can edit them.
457
458 >>> login_person(firefox.owner)
459 >>> view = create_initialized_view(firefox, name='+index')
460 >>> view.show_programming_languages
461 True
462
463Once the programming languages are specified though, even users who aren't the
464owner will see the list.
465
466 >>> firefox.programminglang = 'C++'
467 >>> transaction.commit()
468
469 >>> login('no-priv@canonical.com')
470 >>> view = create_initialized_view(firefox, name='+index')
471 >>> view.show_programming_languages
472 True
473
474
475+index portlets444+index portlets
476---------------445---------------
477446
478447
=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py 2010-11-10 22:04:20 +0000
+++ lib/lp/registry/browser/tests/test_product.py 2011-03-07 20:29:17 +0000
@@ -23,6 +23,7 @@
23 )23 )
24from lp.testing import (24from lp.testing import (
25 login_person,25 login_person,
26 person_logged_in,
26 TestCaseWithFactory,27 TestCaseWithFactory,
27 )28 )
28from lp.testing.mail_helpers import pop_notifications29from lp.testing.mail_helpers import pop_notifications
@@ -169,3 +170,51 @@
169 view = create_initialized_view(self.product_set, '+new')170 view = create_initialized_view(self.product_set, '+new')
170 message = find_tag_by_id(view.render(), 'staging-message')171 message = find_tag_by_id(view.render(), 'staging-message')
171 self.assertEqual(None, message)172 self.assertEqual(None, message)
173
174
175class TestProductView(TestCaseWithFactory):
176 """Tests the ProductView."""
177
178 layer = DatabaseFunctionalLayer
179
180 def setUp(self):
181 super(TestProductView, self).setUp()
182 self.product = self.factory.makeProduct()
183
184 def test_show_programming_languages_without_languages(self):
185 # show_programming_languages is false when there are no programming
186 # languages set.
187 view = create_initialized_view(self.product, '+index')
188 self.assertEqual(None, self.product.programminglang)
189 self.assertFalse(view.show_programming_languages)
190
191 def test_show_programming_languages_with_languages(self):
192 # show_programming_languages is true when programming languages
193 # are set.
194 with person_logged_in(self.product.owner):
195 self.product.programminglang = 'C++'
196 view = create_initialized_view(self.product, '+index')
197 self.assertTrue(view.show_programming_languages)
198
199 def test_show_license_info_without_other_license(self):
200 # show_license_info is false when one of the "other" licenses is
201 # not selected.
202 view = create_initialized_view(self.product, '+index')
203 self.assertEqual((License.GNU_GPL_V2, ), self.product.licenses)
204 self.assertFalse(view.show_license_info)
205
206 def test_show_license_info_with_other_open_source_license(self):
207 # show_license_info is true when the Other/Open Source license is
208 # selected.
209 view = create_initialized_view(self.product, '+index')
210 with person_logged_in(self.product.owner):
211 self.product.licenses = [License.OTHER_OPEN_SOURCE]
212 self.assertTrue(view.show_license_info)
213
214 def test_show_license_info_with_other_open_proprietary_license(self):
215 # show_license_info is true when the Other/Proprietary license is
216 # selected.
217 view = create_initialized_view(self.product, '+index')
218 with person_logged_in(self.product.owner):
219 self.product.licenses = [License.OTHER_PROPRIETARY]
220 self.assertTrue(view.show_license_info)
172221
=== modified file 'lib/lp/registry/templates/product-index.pt'
--- lib/lp/registry/templates/product-index.pt 2010-11-10 15:33:47 +0000
+++ lib/lp/registry/templates/product-index.pt 2011-03-07 20:29:17 +0000
@@ -41,7 +41,7 @@
41 },41 },
42 window);42 window);
43 });43 });
44 "/>44 "></script>
45 </tal:head-epilogue>45 </tal:head-epilogue>
46</head>46</head>
4747
@@ -162,9 +162,9 @@
162 condition="not:repeat/license/end">,</tal:comma>162 condition="not:repeat/license/end">,</tal:comma>
163 </tal:licenses>163 </tal:licenses>
164 <tal:none condition="not:context/licenses">None specified</tal:none>164 <tal:none condition="not:context/licenses">None specified</tal:none>
165 <tal:has_description condition="context/license_info">165 <tal:has_license_info condition="view/show_license_info">
166 (<tal:description replace="context/license_info" />)166 (<tal:license_info replace="context/license_info" />)
167 </tal:has_description>167 </tal:has_license_info>
168 </dd>168 </dd>
169 <dd id="commercial_subscription"169 <dd id="commercial_subscription"
170 tal:condition="context/commercial_subscription">170 tal:condition="context/commercial_subscription">