Merge ~twom/launchpad:commit-to-MP-link-previews into launchpad:master

[Tom Wardill]

Add opengraph_description as an overridable attribute

Some pages may wish to override the description displayed via opengraph to something that makes more sense in context.

Add this as an attribute on the base view, use the new property in the Merge Proposal template and view.

[Kristian Glass (doismellburning)]

No objection, but I'd assumed people would just override `page_description` and would rarely want the two to be to be different (404 pages were the only real counter-example I had) - any particular reason to make the distinction here?

[Ioana Lasc (ilasc)]

I would add a unit test but apart from that it looks good to me.

review: Approve

[Tom Wardill (twom)]

My thought was that in general, the description tag makes more sense to be the 'description' field, it's slightly less surprising.
We're overriding it in this context specifically because it's shorter and makes a little more sense in an opengraph preview.
The 404 page having them different is a stronger argument, admittedly, but that gives us at least two instances that they might be different, so for the small overhead, they might as well be separate.

[Kristian Glass (doismellburning)]

review: Approve

diff --git a/lib/lp/app/templates/base-layout.pt b/lib/lp/app/templates/base-layout.pt
index 08df3dc..4eb0133 100644
--- a/lib/lp/app/templates/base-layout.pt
+++ b/lib/lp/app/templates/base-layout.pt
@@ -50,8 +50,8 @@
         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:condition="view/opengraph_description | nothing"
+        tal:attributes="content view/opengraph_description/fmt:strip-email/fmt:shorten/500" />
     </tal:view>
 
     <tal:comment condition="nothing">
diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
index 6d33f97..ff86b13 100644
--- a/lib/lp/code/browser/branchmergeproposal.py
+++ b/lib/lp/code/browser/branchmergeproposal.py
@@ -767,6 +767,12 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
             mp, description, title, edit_view='+edit-description')
 
     @property
+    def opengraph_description(self):
+        if self.context.commit_message:
+            return self.context.commit_message
+        return self.page_description
+
+    @property
     def edit_commit_message_link_class(self):
         if self.context.commit_message:
             return "hidden"
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index e42a9c5..515b2d4 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -2203,6 +2203,20 @@ class TestBranchMergeProposal(BrowserTestCase):
         view = create_initialized_view(bmp, '+index')
         self.assertFalse(view.show_rescan_link)
 
+    def test_opengraph_description_is_commit(self):
+        bmp = self.factory.makeBranchMergeProposalForGit()
+        target_job = getUtility(IGitRefScanJobSource).create(
+            bmp.target_git_repository)
+        removeSecurityProxy(target_job).job._status = JobStatus.COMPLETED
+        source_job = getUtility(IGitRefScanJobSource).create(
+            bmp.source_git_repository)
+        removeSecurityProxy(source_job).job._status = JobStatus.COMPLETED
+        view = create_initialized_view(bmp, '+index')
+        self.assertEqual(
+            view.opengraph_description, view.context.commit_message)
+        self.assertEqual(
+            view.page_description, bmp.description)
+
 
 class TestBranchMergeProposalRescanView(BrowserTestCase):
 
diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index fccb857..a3b8e70 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -369,6 +369,17 @@ class LaunchpadView(UserAttributeCache):
         return getattr(self.context, 'description', None)
 
     @property
+    def opengraph_description(self):
+        """Return a string for the description used in the OpenGraph metadata
+
+        Some pages may wish to override the values used in the OpenGraph
+        metadata to provide more useful link previews.
+
+        Default to the page_description in the base views.
+        """
+        return self.page_description
+
+    @property
     def template(self):
         """The page's template, if configured in zcml."""
         return self.index