Launchpad itself

Merge lp:~bac/launchpad/bug-652280-pg-trans into lp:launchpad

  1. bug-652280-pg-trans
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11740
Proposed branch: lp:~bac/launchpad/bug-652280-pg-trans
Merge into: lp:launchpad
Diff against target: 320 lines (+191/-14)
7 files modified
lib/lp/registry/doc/projectgroup.txt (+11/-0)
lib/lp/registry/model/projectgroup.py (+8/-2)
lib/lp/testing/views.py (+4/-2)
lib/lp/translations/browser/project.py (+1/-0)
lib/lp/translations/browser/tests/test_noindex.py (+152/-0)
lib/lp/translations/templates/distroseries-translations.pt (+2/-5)
lib/lp/translations/templates/project-translations.pt (+13/-5)
To merge this branch: bzr merge lp:~bac/launchpad/bug-652280-pg-trans
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Jeroen T. Vermeulen (community) code Approve
Curtis Hovey code Pending
Review via email: mp+38195@code.launchpad.net

Description of the change

= Summary =

The +translations page for project groups did not have the
"noindex,nofollow" meta data. It was added as was a new test that tests
all translations pages to ensure the meta data is included properly.

== Proposed fix ==

As above.

== Pre-implementation notes ==

Talks with Curtis and detailed work with Henning.

== Implementation details ==

It's pretty straightforward, as above. However, for distroseries
getting a view via 'create_initialized_view' and trying to render it
does not work, due, I think, to the way the pages are registered in the
ZCML with respect to layers and facets. The template uses the
navigation menus which are not adapted properly.

In the end I punted and used getUserBrowser in order to get the rendered
content of the pages which is a fine approach but leaves the mystery of
why getting the view directly and rendering it does not work. Is there
a hidden bug that I've discovered but didn't crack?

== Tests ==

bin/test -vvm lp.translations -t test_noindex

== Demo and Q/A ==

Go to +translations and inspect the HTML for the anti-robots meta
instructions.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/templates/distroseries-translations.pt
  lib/lp/translations/browser/project.py
  lib/lp/translations/browser/tests/test_noindex.py
  lib/lp/testing/views.py
  lib/lp/translations/templates/project-translations.pt

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Hi Brad,

Nice to see a branch with a solid cover letter again.

You found some interesting bugs in Storm's SQLObject compatibility layer while working on this: is_empty returns the inverse of what it should, and then __nonzero__ compounds that sin by inverting it again. I hope you'll file a bug about this.

I have no objections to the code overall. There are a few things in the test that I'd like to see fixed, but nothing to stop an approval:

 * You use lower_case_with_underscores for their helper methods, whereas our coding standard mandates sayItWithCapitals for methods (but methods only). The requirement may be going away though; I haven't kept track.

 * You pointed out that there's a redundant get_rendered_contents in the products test. To be removed.

 * You also pointed out that there's already a has_translatables on your context object, so no need to duplicate it in the view.

 * Why do you bother making "target" and "naked_translatable" properties instead of attributes that you initialize in setUp? I do appreciate the docstring but a comment would be fine as far as I'm concerned. You could scratch self.product, self.projectgroup, self.distro, and self.distroseries to make up for it; they would become unnecessary.

 * I like the orthogonal test setup, with a mixin for the test scenarios and leaf classes for the different alternate setups. But I find the inheritance graph a little confusing. I think it'd be more manageable if the actual test cases were all leaf classes, without test cases inheriting from other classes that will also run as separate test cases. I'd also put the layer specifications in those leaf classes, so that there's a clear separation between the classes that "go into" a test case and the individual test cases.

 * You have your own verification methods (verify_robots_are_blocked, verify_robots_are_not_blocked) that invoke other assertion methods. I normally avoid this myself because it hides the "where did it go wrong" information deeper in the traceback, but looking at this I quite like the way the naming documents the intent of the assertion.

 * Isn't testing for exactly "noindex,nofollow" more brittle than needed? Would "nofollow,noindex" be equally valid? If so, it'd be more robust to assertContentEqual on robots['content'].split(','). I'll admit it's far-fetched, but brittle tests have given us enough headaches to make us worry about these things.

 * Out of interest, why do you pass rootsite to create_initialized_view for distroseries but not for productseries?

Jeroen

review: Approve
Revision history for this message
Jeroen T. Vermeulen (jtv) :
review: Approve (code)
Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (3.6 KiB)

On Oct 12, 2010, at 18:34 , Jeroen T. Vermeulen wrote:

> Review: Approve
> Hi Brad,

Thanks for the review Jeroen.

>
> Nice to see a branch with a solid cover letter again.

'bzr lp-send'

It's so easy I'm not sure why everyone doesn't do it.

>
> You found some interesting bugs in Storm's SQLObject compatibility layer while working on this: is_empty returns the inverse of what it should, and then __nonzero__ compounds that sin by inverting it again. I hope you'll file a bug about this.

Filed and fixed by you in record time! Thanks for the follow through.

>
> * You use lower_case_with_underscores for their helper methods, whereas our coding standard mandates sayItWithCapitals for methods (but methods only). The requirement may be going away though; I haven't kept track.

No, I was just not paying attention. Fixed.

>
> * You pointed out that there's a redundant get_rendered_contents in the products test. To be removed.

Gone.

>
> * You also pointed out that there's already a has_translatables on your context object, so no need to duplicate it in the view.

Gone along with the view tests. Added missing tests to doc/projectgroup.txt. (Renamed from project.txt.)

>
> * Why do you bother making "target" and "naked_translatable" properties instead of attributes that you initialize in setUp? I do appreciate the docstring but a comment would be fine as far as I'm concerned. You could scratch self.product, self.projectgroup, self.distro, and self.distroseries to make up for it; they would become unnecessary.

I did that before I converted the base class to a mixin to force the base to throw a NotImplementedError for those as I thought it was more explicit than a comment stating the contract. Overkill? Probably, so I have removed it.

>
> * I like the orthogonal test setup, with a mixin for the test scenarios and leaf classes for the different alternate setups. But I find the inheritance graph a little confusing. I think it'd be more manageable if the actual test cases were all leaf classes, without test cases inheriting from other classes that will also run as separate test cases. I'd also put the layer specifications in those leaf classes, so that there's a clear separation between the classes that "go into" a test case and the individual test cases.

OK, changed to all extend BrowserTestCase and the mixin directly. I disagree with setting the layer on each one, though, as it is the same. That just seems redundant and silly.

>
> * You have your own verification methods (verify_robots_are_blocked, verify_robots_are_not_blocked) that invoke other assertion methods. I normally avoid this myself because it hides the "where did it go wrong" information deeper in the traceback, but looking at this I quite like the way the naming documents the intent of the assertion.
>
> * Isn't testing for exactly "noindex,nofollow" more brittle than needed? Would "nofollow,noindex" be equally valid? If so, it'd be more robust to assertContentEqual on robots['content'].split(','). I'll admit it's far-fetched, but brittle tests have given us enough headaches to make us worry about these things.

Yes, that probably is too brittle. I'll ...

Read more...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

This looks just fine to me. Nice to see how much boilerplate you were able to strip off the tests. The layers thing was just a suggestion; I'm fine with your choice.

Revision history for this message
Brad Crittenden (bac) wrote :

Hi Curtis,

This branch includes a lot of good stuff...but it only addresses one part of the original bug. There was disagreement between you and Danilo about what UI changes needed to be done. What are your current thoughts?

Revision history for this message
Curtis Hovey (sinzui) wrote :

The Translations team is also changing these pages. There is not need to change something that the team will remove or alter in a few weeks anyway.

Revision history for this message
Robert Collins (lifeless) wrote :

I'm very happy with one test:

169 + def test_service_status_controls_robots(self):
170 + self.verifyRobotsAreBlocked(ServiceUsage.UNKNOWN)
173 + self.verifyRobotsAreBlocked(ServiceUsage.EXTERNAL)
176 + self.verifyRobotsAreBlocked(ServiceUsage.NOT_APPLICABLE)
179 + self.verifyRobotsNotBlocked(ServiceUsage.LAUNCHPAD)

or however you wish to spell it; using cachedproperty should be ok if you want to, but be sure to do browser.open() after you change the service usage. How many seconds difference does the cachedproperty make (perhaps we need a bug on the performance of canonical_url?)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== renamed file 'lib/lp/registry/doc/project.txt' => 'lib/lp/registry/doc/projectgroup.txt'
2--- lib/lp/registry/doc/project.txt 2010-10-09 16:36:22 +0000
3+++ lib/lp/registry/doc/projectgroup.txt 2010-10-18 02:57:48 +0000
4@@ -331,10 +331,21 @@
5 >>> import transaction
6 >>> transaction.abort()
7
8+A project group with no translatable products is shown by
9+'has_translatables' being false.
10+
11+ >>> product = factory.makeProduct()
12+ >>> project_group = factory.makeProject()
13+ >>> product.project = project_group
14+ >>> project_group.has_translatable()
15+ False
16+
17 GNOME Project is a good example that has translations.
18 It has one translatable product.
19
20 >>> gnome = getUtility(IProjectGroupSet)['gnome']
21+ >>> gnome.has_translatable()
22+ True
23 >>> translatables = gnome.translatables()
24 >>> translatables.count()
25 1
26
27=== modified file 'lib/lp/registry/model/projectgroup.py'
28--- lib/lp/registry/model/projectgroup.py 2010-10-07 13:26:53 +0000
29+++ lib/lp/registry/model/projectgroup.py 2010-10-18 02:57:48 +0000
30@@ -206,9 +206,11 @@
31
32 def translatables(self):
33 """See `IProjectGroup`."""
34- # XXX j.c.sackett 2010-08-30 bug=627631 Once data migration has
35+ # XXX j.c.sackett 2010-08-30 bug=627631: Once data migration has
36 # happened for the usage enums, this sql needs to be updated to
37- # check for the translations_usage, not official_rosetta.
38+ # check for the translations_usage, not official_rosetta. At that
39+ # time it should also be converted to a Storm query and the issue with
40+ # has_translatables resolved.
41 return Product.select('''
42 Product.project = %s AND
43 Product.official_rosetta = TRUE AND
44@@ -220,6 +222,10 @@
45
46 def has_translatable(self):
47 """See `IProjectGroup`."""
48+ # XXX: BradCrittenden 2010-10-12 bug=659078: The test should be
49+ # converted to use is_empty but the implementation in storm's
50+ # sqlobject wrapper is broken.
51+ # return not self.translatables().is_empty()
52 return self.translatables().count() != 0
53
54 def _getBaseQueryAndClauseTablesForQueryingSprints(self):
55
56=== modified file 'lib/lp/testing/views.py'
57--- lib/lp/testing/views.py 2010-09-19 03:09:49 +0000
58+++ lib/lp/testing/views.py 2010-10-18 02:57:48 +0000
59@@ -85,7 +85,8 @@
60 def create_initialized_view(context, name, form=None, layer=None,
61 server_url=None, method=None, principal=None,
62 query_string=None, cookie=None, request=None,
63- path_info='/', rootsite=None):
64+ path_info='/', rootsite=None,
65+ current_request=False):
66 """Return a view that has already been initialized."""
67 if method is None:
68 if form is None:
69@@ -94,7 +95,8 @@
70 method = 'POST'
71 view = create_view(
72 context, name, form, layer, server_url, method, principal,
73- query_string, cookie, request, path_info, rootsite=rootsite)
74+ query_string, cookie, request, path_info, rootsite=rootsite,
75+ current_request=current_request)
76 view.initialize()
77 return view
78
79
80=== modified file 'lib/lp/translations/browser/project.py'
81--- lib/lp/translations/browser/project.py 2010-08-20 20:31:18 +0000
82+++ lib/lp/translations/browser/project.py 2010-10-18 02:57:48 +0000
83@@ -21,6 +21,7 @@
84 from canonical.launchpad.webapp.menu import NavigationMenu
85 from lp.registry.browser.project import ProjectEditView
86 from lp.registry.interfaces.projectgroup import IProjectGroup
87+from lp.services.propertycache import cachedproperty
88 from lp.translations.browser.translations import TranslationsMixin
89
90
91
92=== added file 'lib/lp/translations/browser/tests/test_noindex.py'
93--- lib/lp/translations/browser/tests/test_noindex.py 1970-01-01 00:00:00 +0000
94+++ lib/lp/translations/browser/tests/test_noindex.py 2010-10-18 02:57:48 +0000
95@@ -0,0 +1,152 @@
96+# Copyright 2010 Canonical Ltd. This software is licensed under the
97+# GNU Affero General Public License version 3 (see the file LICENSE).
98+
99+__metaclass__ = type
100+
101+
102+from BeautifulSoup import BeautifulSoup
103+from zope.security.proxy import removeSecurityProxy
104+
105+from canonical.launchpad.webapp import canonical_url
106+from canonical.testing.layers import DatabaseFunctionalLayer
107+from lp.app.enums import ServiceUsage
108+from lp.testing import (
109+ BrowserTestCase,
110+ login_person,
111+ )
112+from lp.services.propertycache import cachedproperty
113+
114+
115+class TestRobotsMixin:
116+ """Test the inclusion of the meta "noindex,nofollow" directives.
117+
118+ Subclasses using this mixin must set the following attributes:
119+ target - the pillar under test
120+ naked_translatable - the translatable object for the test, with the
121+ security proxy removed. It may be the target or a subordinate object.
122+ (For example when testing a ProjectGroup the translatable is one of the
123+ products in the project group.)
124+ """
125+
126+ layer = DatabaseFunctionalLayer
127+
128+ def setUsage(self, usage):
129+ self.naked_translatable.translations_usage = usage
130+
131+ @cachedproperty
132+ def url(self):
133+ return canonical_url(self.target, rootsite='translations')
134+
135+ @cachedproperty
136+ def user_browser(self):
137+ return self.getUserBrowser(user=self.user)
138+
139+ def getRenderedContents(self):
140+ """Return an initialized view's rendered contents."""
141+ # Using create_initialized_view for distroseries causes an error when
142+ # rendering the view due to the way the view is registered and menus
143+ # are adapted. Getting the contents via a browser does work.
144+ self.user_browser.open(self.url)
145+ return self.user_browser.contents
146+
147+ def getRobotsDirective(self):
148+ contents = self.getRenderedContents()
149+ soup = BeautifulSoup(contents)
150+ return soup.find('meta', attrs={'name': 'robots'})
151+
152+ def verifyRobotsAreBlocked(self, usage):
153+ self.setUsage(usage)
154+ robots = self.getRobotsDirective()
155+ self.assertIsNot(None, robots,
156+ "Robot blocking meta information not present.")
157+ self.assertEqual('noindex,nofollow', robots['content'])
158+ expected = ('noindex', 'nofollow')
159+ actual = robots['content'].split(',')
160+ self.assertContentEqual(expected, actual)
161+
162+ def verifyRobotsNotBlocked(self, usage):
163+ self.setUsage(usage)
164+ robots = self.getRobotsDirective()
165+ self.assertIs(
166+ None, robots,
167+ "Robot blocking metadata present when it should not be.")
168+
169+ def test_robots(self):
170+ # Robots are blocked for usage that is not Launchpad.
171+ self.verifyRobotsAreBlocked(ServiceUsage.UNKNOWN)
172+ self.verifyRobotsAreBlocked(ServiceUsage.EXTERNAL)
173+ self.verifyRobotsAreBlocked(ServiceUsage.NOT_APPLICABLE)
174+ # Robots are not blocked for Launchpad usage.
175+ self.verifyRobotsNotBlocked(ServiceUsage.LAUNCHPAD)
176+
177+
178+class TestRobotsProduct(BrowserTestCase, TestRobotsMixin):
179+ """Test noindex,nofollow for products."""
180+
181+ def setUp(self):
182+ super(TestRobotsProduct, self).setUp()
183+ self.target = self.factory.makeProduct()
184+ self.factory.makePOTemplate(
185+ productseries=self.target.development_focus)
186+ self.naked_translatable = removeSecurityProxy(self.target)
187+
188+
189+class TestRobotsProjectGroup(BrowserTestCase, TestRobotsMixin):
190+ """Test noindex,nofollow for project groups."""
191+
192+ def setUp(self):
193+ super(TestRobotsProjectGroup, self).setUp()
194+ self.target = self.factory.makeProject()
195+ self.product = self.factory.makeProduct()
196+ self.factory.makePOTemplate(
197+ productseries=self.product.development_focus)
198+ self.naked_translatable = removeSecurityProxy(self.product)
199+ self.naked_translatable.project = self.target
200+
201+
202+class TestRobotsProductSeries(BrowserTestCase, TestRobotsMixin):
203+ """Test noindex,nofollow for product series."""
204+
205+ def setUp(self):
206+ super(TestRobotsProductSeries, self).setUp()
207+ self.product = self.factory.makeProduct()
208+ self.target = self.product.development_focus
209+ self.factory.makePOTemplate(
210+ productseries=self.target)
211+ self.naked_translatable = removeSecurityProxy(self.product)
212+
213+
214+class TestRobotsDistroSeries(BrowserTestCase, TestRobotsMixin):
215+ """Test noindex,nofollow for distro series."""
216+
217+ def setUp(self):
218+ super(TestRobotsDistroSeries, self).setUp()
219+ login_person(self.user)
220+ self.distro = self.factory.makeDistribution(
221+ name="whobuntu", owner=self.user)
222+ self.target = self.factory.makeDistroSeries(
223+ name="zephyr", distribution=self.distro)
224+ self.target.hide_all_translations = False
225+ new_sourcepackagename = self.factory.makeSourcePackageName()
226+ self.factory.makePOTemplate(
227+ distroseries=self.target,
228+ sourcepackagename=new_sourcepackagename)
229+ self.naked_translatable = removeSecurityProxy(self.distro)
230+
231+
232+class TestRobotsDistro(BrowserTestCase, TestRobotsMixin):
233+ """Test noindex,nofollow for distro."""
234+
235+ def setUp(self):
236+ super(TestRobotsDistro, self).setUp()
237+ login_person(self.user)
238+ self.target = self.factory.makeDistribution(
239+ name="whobuntu", owner=self.user)
240+ self.distroseries = self.factory.makeDistroSeries(
241+ name="zephyr", distribution=self.target)
242+ self.distroseries.hide_all_translations = False
243+ new_sourcepackagename = self.factory.makeSourcePackageName()
244+ self.factory.makePOTemplate(
245+ distroseries=self.distroseries,
246+ sourcepackagename=new_sourcepackagename)
247+ self.naked_translatable = removeSecurityProxy(self.target)
248
249=== modified file 'lib/lp/translations/templates/distroseries-translations.pt'
250--- lib/lp/translations/templates/distroseries-translations.pt 2010-09-24 15:30:10 +0000
251+++ lib/lp/translations/templates/distroseries-translations.pt 2010-10-18 02:57:48 +0000
252@@ -10,10 +10,8 @@
253 use-macro="context/@@+translations-macros/translations-js" />
254 <metal:languages-table-js
255 use-macro="context/@@+translations-macros/languages-table-js" />
256- <tal:robots
257- condition="not:context/translations_usage/enumvalue:LAUNCHPAD">
258- <meta name="robots" content="noindex,nofollow" />
259- </tal:robots>
260+ <meta tal:condition="not:context/translations_usage/enumvalue:LAUNCHPAD"
261+ name="robots" content="noindex,nofollow" />
262 </div>
263 </head>
264 <body>
265@@ -156,4 +154,3 @@
266 </div>
267 </body>
268 </html>
269-
270
271=== modified file 'lib/lp/translations/templates/project-translations.pt'
272--- lib/lp/translations/templates/project-translations.pt 2010-04-19 08:11:52 +0000
273+++ lib/lp/translations/templates/project-translations.pt 2010-10-18 02:57:48 +0000
274@@ -4,16 +4,26 @@
275 xmlns:metal="http://xml.zope.org/namespaces/metal"
276 metal:use-macro="view/macro:page/main_only"
277 >
278+
279+<head>
280+ <tal:head_epilogue metal:fill-slot="head_epilogue">
281+ <meta tal:condition="not:context/has_translatable"
282+ name="robots" content="noindex,nofollow" />
283+ </tal:head_epilogue>
284+</head>
285+
286 <body>
287 <div metal:fill-slot="main">
288
289 <div class="top-portlet">
290- <p tal:condition="context/translatables">
291+ <p tal:condition="context/has_translatable">
292 Select a project you want to translate into your own language.
293 </p>
294- <p tal:condition="not:context/translatables">
295+ <p tal:condition="not:context/has_translatable">
296+ <strong>
297 There are no translatable projects in
298 <span tal:content="context/displayname">Project Group</span>.
299+ </strong>
300 </p>
301 <p tal:condition="not:view/required:launchpad.Edit">
302 If a project for <span tal:content="context/displayname">Project Group
303@@ -34,7 +44,7 @@
304 <div class="yui-u first">
305 <div class="portlet" id="translatable-projects">
306 <h3>Translatable projects</h3>
307- <dl tal:condition="context/translatables">
308+ <dl tal:condition="context/has_translatable">
309 <tal:project repeat="product context/translatables">
310 <dt>
311 <a tal:replace="structure product/fmt:link/+translations">
312@@ -80,8 +90,6 @@
313 <a class="add sprite" href="+newproduct">Add another project</a>
314 </p>
315 </div>
316-
317-
318 </div><!--main-->
319 </body>
320 </html>