launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01484
[Merge] lp:~bac/launchpad/bug-652280-pg-trans into lp:launchpad/devel
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-652280-pg-trans into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#652280 project group shows unneeded information
https://bugs.launchpad.net/bugs/652280
= 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
--
https://code.launchpad.net/~bac/launchpad/bug-652280-pg-trans/+merge/38195
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-652280-pg-trans into lp:launchpad/devel.
=== modified file 'lib/lp/testing/views.py'
--- lib/lp/testing/views.py 2010-09-19 03:09:49 +0000
+++ lib/lp/testing/views.py 2010-10-12 09:01:19 +0000
@@ -85,7 +85,8 @@
def create_initialized_view(context, name, form=None, layer=None,
server_url=None, method=None, principal=None,
query_string=None, cookie=None, request=None,
- path_info='/', rootsite=None):
+ path_info='/', rootsite=None,
+ current_request=False):
"""Return a view that has already been initialized."""
if method is None:
if form is None:
@@ -94,7 +95,8 @@
method = 'POST'
view = create_view(
context, name, form, layer, server_url, method, principal,
- query_string, cookie, request, path_info, rootsite=rootsite)
+ query_string, cookie, request, path_info, rootsite=rootsite,
+ current_request=current_request)
view.initialize()
return view
=== modified file 'lib/lp/translations/browser/project.py'
--- lib/lp/translations/browser/project.py 2010-08-20 20:31:18 +0000
+++ lib/lp/translations/browser/project.py 2010-10-12 09:01:19 +0000
@@ -21,6 +21,7 @@
from canonical.launchpad.webapp.menu import NavigationMenu
from lp.registry.browser.project import ProjectEditView
from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.services.propertycache import cachedproperty
from lp.translations.browser.translations import TranslationsMixin
@@ -56,6 +57,10 @@
all_products = set(self.context.products)
return list(all_products - translatables)
+ @cachedproperty
+ def has_translatables(self):
+ return self.context.translatables().count() > 0
+
class ProjectSettingsView(TranslationsMixin, ProjectEditView):
label = "Set permissions and policies"
=== added file 'lib/lp/translations/browser/tests/test_noindex.py'
--- lib/lp/translations/browser/tests/test_noindex.py 1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/tests/test_noindex.py 2010-10-12 09:01:19 +0000
@@ -0,0 +1,221 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+
+from BeautifulSoup import BeautifulSoup
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.enums import ServiceUsage
+from lp.testing import (
+ BrowserTestCase,
+ login_person,
+ )
+from lp.testing.views import create_initialized_view
+from lp.translations.publisher import TranslationsLayer
+
+
+class TestRobotsMixin:
+ """Test the inclusion of the meta "noindex,nofollow" directives."""
+
+ layer = DatabaseFunctionalLayer
+
+ def set_usage(self, usage):
+ self.naked_translatable.translations_usage = usage
+
+ def get_view(self):
+ """Return an initialized view's rendered contents."""
+ raise NotImplementedError
+
+ def get_rendered_contents(self):
+ """Return an initialized view's rendered contents."""
+ url = canonical_url(self.target, rootsite='translations')
+ browser = self.getUserBrowser(url)
+ return browser.contents
+
+ @property
+ def target(self):
+ """The target of this test.
+
+ Must be overridden.
+ """
+ raise NotImplementedError
+
+ @property
+ def naked_translatable(self):
+ """The actual object that is translatable, not necessarily the target.
+
+ For instance, for a ProjectGroup the translatable is the product.
+ Must be overridden.
+ """
+ raise NotImplementedError
+
+ def get_soup_and_robots(self, usage):
+ self.set_usage(usage)
+ contents = self.get_rendered_contents()
+ soup = BeautifulSoup(contents)
+ robots = soup.find('meta', attrs={'name': 'robots'})
+ return soup, robots
+
+ def verify_robots_are_blocked(self, usage):
+ soup, robots = self.get_soup_and_robots(usage)
+ self.assertIsNot(None, robots,
+ "Robot blocking meta information not present.")
+ self.assertEqual('noindex,nofollow', robots['content'])
+
+ def verify_robots_not_blocked(self, usage):
+ soup, robots = self.get_soup_and_robots(usage)
+ self.assertIs(
+ None, robots,
+ "Robot blocking metadata present when it should not be.")
+
+ def test_UNKNOWN_blocks_robots(self):
+ self.verify_robots_are_blocked(ServiceUsage.UNKNOWN)
+
+ def test_EXTERNAL_blocks_robots(self):
+ self.verify_robots_are_blocked(ServiceUsage.EXTERNAL)
+
+ def test_NOT_APPLICABLE_blocks_robots(self):
+ self.verify_robots_are_blocked(ServiceUsage.NOT_APPLICABLE)
+
+ def test_LAUNCHPAD_does_not_block_robots(self):
+ self.verify_robots_not_blocked(ServiceUsage.LAUNCHPAD)
+
+
+class TestRobotsProduct(BrowserTestCase, TestRobotsMixin):
+ """Test noindex,nofollow for products."""
+
+ def setUp(self):
+ super(TestRobotsProduct, self).setUp()
+ self.product = self.factory.makeProduct()
+ self.factory.makePOTemplate(
+ productseries=self.product.development_focus)
+
+ @property
+ def target(self):
+ return self.product
+
+ @property
+ def naked_translatable(self):
+ return removeSecurityProxy(self.product)
+
+ def get_view(self):
+ view = create_initialized_view(
+ self.target, '+translations',
+ current_request=True, layer=TranslationsLayer)
+ return view
+
+
+class TestRobotsProjectGroup(TestRobotsProduct):
+ """Test noindex,nofollow for project groups."""
+
+ def setUp(self):
+ super(TestRobotsProjectGroup, self).setUp()
+ self.projectgroup = self.factory.makeProject()
+ self.product = self.factory.makeProduct()
+ self.factory.makePOTemplate(
+ productseries=self.product.development_focus)
+ self.naked_translatable.project = self.projectgroup
+
+ @property
+ def target(self):
+ return self.projectgroup
+
+ def test_has_translatables_true(self):
+ self.set_usage(ServiceUsage.LAUNCHPAD)
+ view = self.get_view()
+ self.assertTrue(view.has_translatables)
+
+ def test_has_translatables_false(self):
+ self.set_usage(ServiceUsage.UNKNOWN)
+ view = self.get_view()
+ self.assertFalse(view.has_translatables)
+
+
+class TestRobotsProductSeries(TestRobotsProduct):
+ """Test noindex,nofollow for product series."""
+
+ def setUp(self):
+ super(TestRobotsProductSeries, self).setUp()
+ self.product = self.factory.makeProduct()
+ self.productseries = self.product.development_focus
+ self.factory.makePOTemplate(
+ productseries=self.productseries)
+
+ @property
+ def target(self):
+ return self.productseries
+
+
+class TestRobotsDistroSeries(BrowserTestCase, TestRobotsMixin):
+ """Test noindex,nofollow for distro series."""
+
+ def setUp(self):
+ super(TestRobotsDistroSeries, self).setUp()
+ self.owner = self.factory.makePerson()
+ login_person(self.owner)
+ self.distro = self.factory.makeDistribution(
+ name="whobuntu", owner=self.owner)
+ self.distroseries = self.factory.makeDistroSeries(
+ name="zephyr", distribution=self.distro)
+ self.distroseries.hide_all_translations = False
+ new_sourcepackagename = self.factory.makeSourcePackageName()
+ self.factory.makePOTemplate(
+ distroseries=self.distroseries,
+ sourcepackagename=new_sourcepackagename)
+
+ @property
+ def target(self):
+ return self.distroseries
+
+ @property
+ def naked_translatable(self):
+ return removeSecurityProxy(self.distro)
+
+ def get_view(self):
+ view = create_initialized_view(
+ self.target, '+translations',
+ layer=TranslationsLayer,
+ rootsite='translations',
+ current_request=True)
+ return view
+
+ def get_rendered_contents(self):
+ # Using create_initialized_view for distroseries causes an error when
+ # rendering the view due to the way the view is registered and menus
+ # are adapted. Getting the contents via a browser does work.
+ url = canonical_url(self.target, rootsite='translations')
+ browser = self.getUserBrowser(url)
+ return browser.contents
+
+ def test_LAUNCHPAD_does_not_block_robots(self):
+ self.verify_robots_not_blocked(ServiceUsage.LAUNCHPAD)
+
+
+class TestRobotsDistro(TestRobotsDistroSeries):
+ """Test noindex,nofollow for distro."""
+
+ def setUp(self):
+ super(TestRobotsDistroSeries, self).setUp()
+ self.owner = self.factory.makePerson()
+ login_person(self.owner)
+ self.distro = self.factory.makeDistribution(
+ name="whobuntu", owner=self.owner)
+ self.distroseries = self.factory.makeDistroSeries(
+ name="zephyr", distribution=self.distro)
+ self.distroseries.hide_all_translations = False
+ new_sourcepackagename = self.factory.makeSourcePackageName()
+ self.factory.makePOTemplate(
+ distroseries=self.distroseries,
+ sourcepackagename=new_sourcepackagename)
+
+ @property
+ def target(self):
+ return self.distro
+
+ @property
+ def naked_translatable(self):
+ return removeSecurityProxy(self.distro)
=== modified file 'lib/lp/translations/templates/distroseries-translations.pt'
--- lib/lp/translations/templates/distroseries-translations.pt 2010-09-24 15:30:10 +0000
+++ lib/lp/translations/templates/distroseries-translations.pt 2010-10-12 09:01:19 +0000
@@ -10,10 +10,8 @@
use-macro="context/@@+translations-macros/translations-js" />
<metal:languages-table-js
use-macro="context/@@+translations-macros/languages-table-js" />
- <tal:robots
- condition="not:context/translations_usage/enumvalue:LAUNCHPAD">
- <meta name="robots" content="noindex,nofollow" />
- </tal:robots>
+ <meta tal:condition="not:context/translations_usage/enumvalue:LAUNCHPAD"
+ name="robots" content="noindex,nofollow" />
</div>
</head>
<body>
@@ -156,4 +154,3 @@
</div>
</body>
</html>
-
=== modified file 'lib/lp/translations/templates/project-translations.pt'
--- lib/lp/translations/templates/project-translations.pt 2010-04-19 08:11:52 +0000
+++ lib/lp/translations/templates/project-translations.pt 2010-10-12 09:01:19 +0000
@@ -4,16 +4,26 @@
xmlns:metal="http://xml.zope.org/namespaces/metal";
metal:use-macro="view/macro:page/main_only"
>
+
+<head>
+ <tal:head_epilogue metal:fill-slot="head_epilogue">
+ <meta tal:condition="not:view/has_translatables"
+ name="robots" content="noindex,nofollow" />
+ </tal:head_epilogue>
+</head>
+
<body>
<div metal:fill-slot="main">
<div class="top-portlet">
- <p tal:condition="context/translatables">
+ <p tal:condition="view/has_translatables">
Select a project you want to translate into your own language.
</p>
- <p tal:condition="not:context/translatables">
+ <p tal:condition="not:view/has_translatables">
+ <strong>
There are no translatable projects in
<span tal:content="context/displayname">Project Group</span>.
+ </strong>
</p>
<p tal:condition="not:view/required:launchpad.Edit">
If a project for <span tal:content="context/displayname">Project Group
@@ -34,7 +44,7 @@
<div class="yui-u first">
<div class="portlet" id="translatable-projects">
<h3>Translatable projects</h3>
- <dl tal:condition="context/translatables">
+ <dl tal:condition="view/has_translatables">
<tal:project repeat="product context/translatables">
<dt>
<a tal:replace="structure product/fmt:link/+translations">
@@ -80,8 +90,6 @@
<a class="add sprite" href="+newproduct">Add another project</a>
</p>
</div>
-
-
</div><!--main-->
</body>
</html>
