launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/808282-canonical-url into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #808282 in Launchpad itself: "Google could give better search results if pages use link rel=canonical"
  https://bugs.launchpad.net/launchpad/+bug/808282

For more details, see:
https://code.launchpad.net/~mbp/launchpad/808282-canonical-url/+merge/82811

Per discussion in bug 808282, it's possible Launchpad can help search engines give better results by sending <link rel=canonical> on bug pages.  This adds a general mechanism for doing this and does it on bug pages.
-- 
https://code.launchpad.net/~mbp/launchpad/808282-canonical-url/+merge/82811
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/808282-canonical-url into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2011-11-17 17:03:52 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-11-20 08:38:32 +0000
@@ -384,8 +384,35 @@
         # Those that can be must override this method.
         raise NotFound(self, name, request=request)
 
+<<<<<<< TREE
     # Names of feature flags which affect a view.
     related_features = ()
+=======
+    @property
+    def canonical_url_recommended(self):
+        """Canonical URL to be recommended in metadata.
+
+        Used to generate <link rel="canonical"> to hint that pages
+        with different URLs are actually (at least almost) functionally
+        and semantically identical.
+
+        See http://www.google.com/support/webmasters/bin/\
+            answer.py?answer=139394
+        "Canonical is a long word that means my preferred or my
+        primary."
+
+        Google (at least) will primarily, but not absolutely certainly,
+        treat these pages as duplicates, so don't use this if there's any
+        real chance the user would want to specifically find one of the
+        non-duplicate pages.
+
+        Most views won't need this.
+        """
+        return None
+
+    # Flags for new features in beta status which affect a view.
+    beta_features = ()
+>>>>>>> MERGE-SOURCE
 
     @property
     def related_feature_info(self):

=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt	2011-09-06 15:31:28 +0000
+++ lib/lp/app/templates/base-layout.pt	2011-11-20 08:38:32 +0000
@@ -28,6 +28,9 @@
     lp_js string:${icingroot}/build">
     <title tal:content="view/fmt:pagetitle">Page Title</title>
     <link rel="shortcut icon" href="/@@/launchpad.png" />
+    <link rel="canonical"
+      tal:condition="view/canonical_url_recommended | nothing"
+      tal:attributes="href view/canonical_url_recommended" />
 
     <tal:atomfeeds define="feed_links view/feed_links | nothing">
       <link rel="alternate" type="application/atom+xml"

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-19 05:44:39 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-20 08:38:32 +0000
@@ -691,6 +691,10 @@
     def api_request(self):
         return IWebServiceClientRequest(self.request)
 
+    @cachedproperty
+    def canonical_url_recommended(self):
+        return canonical_url(self.context.bug, rootsite='bugs')
+
     def initialize(self):
         """Set up the needed widgets."""
         bug = self.context.bug

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2011-11-15 12:55:27 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2011-11-20 08:38:32 +0000
@@ -11,6 +11,11 @@
 
 from BeautifulSoup import BeautifulSoup
 
+from soupmatchers import (
+    HTMLContains,
+    Tag,
+    )
+
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -423,3 +428,23 @@
             "subscribers:\n.*%s \(%s\)"
             % (naked_subscriber.displayname, naked_subscriber.name),
             view_text)
+
+
+class TestBugCanonicalUrl(BrowserTestCase):
+    """Bugs give a link rel=canoncial to a standard url.
+
+    See https://bugs.launchpad.net/launchpad/+bug/808282
+    """
+    layer = DatabaseFunctionalLayer
+
+    def test_bug_canonical_url(self):
+        bug = self.factory.makeBug()
+        browser = self.getViewBrowser(bug, rootsite="bugs")
+        # Hardcode this to avoid be sure we've really got what we expected.
+        expected_url = 'http://bugs.launchpad.dev/bugs/%d' % bug.id
+        self.assertThat(
+            browser.contents,
+            HTMLContains(Tag(
+                'link rel=canonical',
+                'link',
+                dict(rel='canonical', href=expected_url))))