launchpad-reviewers team mailing list archive

[Kristian Glass <kristian.glass@xxxxxxxxxxxxx>]

Kristian Glass has proposed merging ~doismellburning/launchpad:ogp into launchpad:master.

Commit message:
Add Open Graph protocol metadata to pages for previews etc.

https://ogp.me/

og:url is a required property - falling back to request/URL in the
(frequent) absence of recommended_canonical_url seems the least-worst
option

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~doismellburning/launchpad/+git/launchpad/+merge/385644
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~doismellburning/launchpad:ogp into launchpad:master.

diff --git a/lib/lp/app/browser/tests/test_base_layout.py b/lib/lp/app/browser/tests/test_base_layout.py
index ea7a422..db768e2 100644
--- a/lib/lp/app/browser/tests/test_base_layout.py
+++ b/lib/lp/app/browser/tests/test_base_layout.py
@@ -251,3 +251,22 @@ class TestBaseLayout(TestCaseWithFactory):
         referrer = content.find('meta', content="origin-when-cross-origin")
         self.assertIsNone(referrer)
 
+    def test_opengraph_metadata(self):
+        view = self.makeTemplateView('main_side')
+        content = BeautifulSoup(view())
+
+        # https://ogp.me/ - "The four required properties for every page are:"
+        og_title = content.find('meta', {'property': 'og:title'})
+        self.assertIsNotNone(og_title)
+        og_type = content.find('meta', {'property': 'og:type'})
+        self.assertIsNotNone(og_type)
+        og_image = content.find('meta', {'property': 'og:image'})
+        self.assertIsNotNone(og_image)
+        og_url = content.find('meta', {'property': 'og:url'})
+        self.assertIsNotNone(og_url)
+
+        # And some basic validity checks
+        self.assertEqual(og_type.get('content'), 'website')
+        self.assertIn('png', og_image.get('content'))
+        self.assertIn('Test', og_title.get('content'))
+        self.assertIn('http', og_url.get('content'))
diff --git a/lib/lp/app/templates/base-layout.pt b/lib/lp/app/templates/base-layout.pt
index c864afc..b54d36d 100644
--- a/lib/lp/app/templates/base-layout.pt
+++ b/lib/lp/app/templates/base-layout.pt
@@ -49,8 +49,17 @@
       <meta name="description"
         tal:condition="view/page_description | nothing"
         tal:attributes="content view/page_description/fmt:strip-email/fmt:shorten/500" />
+      <meta property="og:description"
+        tal:condition="view/page_description | nothing"
+        tal:attributes="content view/page_description/fmt:strip-email/fmt:shorten/500" />
     </tal:view>
 
+    <meta property="og:title" tal:attributes="content view/fmt:pagetitle" />
+    <meta property="og:type" content="website" />
+    <meta property="og:image" content="/@@/launchpad.png" />
+    <meta property="og:url" tal:attributes="content python: view.recommended_canonical_url or request.URL" />
+    <meta property="og:site_name" content="Launchpad" />
+
     <tal:view condition="view/private">
       <meta name="referrer" content="origin-when-cross-origin"/>
     </tal:view>
diff --git a/lib/lp/bugs/browser/tests/test_bug_views.py b/lib/lp/bugs/browser/tests/test_bug_views.py
index 1877018..d6c4eaf 100644
--- a/lib/lp/bugs/browser/tests/test_bug_views.py
+++ b/lib/lp/bugs/browser/tests/test_bug_views.py
@@ -621,12 +621,18 @@ class TestBugCanonicalUrl(BrowserTestCase):
     """
     layer = DatabaseFunctionalLayer
 
-    def test_bug_canonical_url(self):
+    def _create_bug_with_url(self):
         bug = self.factory.makeBug()
-        browser = self.getViewBrowser(bug, rootsite="bugs")
         # Hardcode this to be sure we've really got what we expected, with no
         # confusion about lp's own url generation machinery.
         expected_url = 'http://bugs.launchpad.test/bugs/%d' % bug.id
+
+        return bug, expected_url
+
+    def test_bug_canonical_url(self):
+        bug, expected_url = self._create_bug_with_url()
+        browser = self.getViewBrowser(bug, rootsite="bugs")
+
         self.assertThat(
             browser.contents,
             HTMLContains(Tag(
@@ -634,6 +640,19 @@ class TestBugCanonicalUrl(BrowserTestCase):
                 'link',
                 dict(rel='canonical', href=expected_url))))
 
+    def test_bug_opengraph_canonical_url(self):
+        # As test_bug_canonical_url, but ensure it's used correctly in
+        # OpenGraph metadata too
+        bug, expected_url = self._create_bug_with_url()
+        browser = self.getViewBrowser(bug, rootsite="bugs")
+
+        self.assertThat(
+            browser.contents,
+            HTMLContains(Tag(
+                'meta property=og:url',
+                'meta',
+                {'property': 'og:url', 'content': expected_url})))
+
 
 class TestBugMessageAddFormView(TestCaseWithFactory):
     """Tests for the add message to bug view."""