Merge lp:~mbp/launchpad/meta-description into lp:launchpad

[Martin Pool]

[r=rvb][no-qa] add meta description for bugs and bmps

Google sometimes generates lame page summaries for Launchpad pages, containing garbage that occurs near the start of the page.

We can give it a better clue by putting in meta description tags.

For an actual search it will often but not always generate its own summary looking at the page text, but the meta description is sometimes used there. The meta description seems to be very often used when sharing a link on g+ and facebook and at the moment it looks awful.

See:

http://googlewebmastercentral.blogspot.com/2007/09/improve-snippets-with-meta-description.html

g+ before: https://plus.google.com/112646476239496153808/posts/bMVdVYUP6XA
after: https://plus.google.com/112646476239496153808/posts/BRxTsjaUeWu

fb before: https://www.facebook.com/martin.pool/posts/107832539332513
after: https://www.facebook.com/martin.pool/posts/119663848146056

(apparently you must log in even to see fb "public" posts)

hooray!

We could do this for other pages too, but we can start with bugs and merge proposals.

[Martin Pool (mbp)]

If there's a more zopey way to write this, let me know. I was going to try to make all view objects provide a page_description property, but I was defeated.

[Martin Pool (mbp)]

I worked out how to do it with less duplication in the templates: view classes can now just provide a description.

[Martin Pool (mbp)]

of all of these metadata patches, this is probably the most sure to have a beneficial effect

[Raphaël Badin (rvb)]

This looks good to land.

[0]

224 + expected_meta = Tag(
225 + 'meta description',
226 + 'meta', attrs=dict(
227 + name='description',
228 + content=True))

Don't you want to test for the presence of the (potentially cropped) 'description' ?

[1]

243 + def page_description(self):
244 + context = self.context
245 + if context.is_valid_person_or_team:
246 + return (
247 + self.context.homepage_content
248 + or self.context.teamdescription)
249 + else:
250 + return None

Don't you want to return u'' on line 250 here?

[2]

281 + self.assertThat(view.page_description,
282 + Equals(person_description))

A fascist reviewer would ask you to put a new line before "view.page_description," ;)

[3]

300 + self.assertEqual(
301 + None, view.page_description)

I think you can save a line here.

review: Approve

[Martin Pool (mbp)]

Thanks for the review.

I will test the actual content.

On consideration, I think I will strip email addresses always, since the description is commonly used by sharing tools and they shouldn't share the email address, even when started by an authenticated user.

> Don't you want to return u'' on line 250 here?

No I think I do want None, so the description just doesn't appear.

=== 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-22 06:59:27 +0000
@@ -288,6 +288,24 @@
         pass
 
     @property
+    def page_description(self):
+        """Return a string containing a description of the context.
+
+        Typically this is the contents of the most-descriptive text attribute
+        of the context, by default its 'description' attribute if there is one.
+
+        This will be inserted into the HTML meta description, and may
+        eventually end up in search engine summary results, or when a link to
+        the page is shared elsewhere.
+
+        This may be specialized by view subclasses.
+
+        Do not write eg "This is a page about...", just directly describe the
+        object on the page.
+        """
+        return getattr(self.context, 'description', None)
+
+    @property
     def template(self):
         """The page's template, if configured in zcml."""
         return self.index

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2011-11-03 03:24:57 +0000
+++ lib/lp/app/browser/stringformatter.py	2011-11-22 06:59:27 +0000
@@ -803,6 +803,16 @@
             return self._stringtoformat
         return obfuscate_email(self._stringtoformat)
 
+    def strip_email(self):
+        """Strip out things that may be email.
+
+        This is a variation on obfuscate_email for when we are generating a
+        snipped for page metadata: we don't want to waste space spelling out
+        "<email address hidden>", and we do want to strip addresses even for
+        logged-in users in case they use the summary in a sharing tool.
+        """
+        return obfuscate_email(self._stringtoformat, replacement="...")
+
     def linkify_email(self, preloaded_person_data=None):
         """Linkify any email address recognised in Launchpad.
 
@@ -991,6 +1001,8 @@
             return self.email_to_html()
         elif name == 'obfuscate-email':
             return self.obfuscate_email()
+        elif name == 'strip-email':
+            return self.strip_email()
         elif name == 'linkify-email':
             return self.linkify_email()
         elif name == 'shorten':

=== 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-22 06:59:27 +0000
@@ -52,6 +52,10 @@
     </tal:comment>
     <noscript></noscript>
 
+    <meta name="description" 
+      tal:condition="view/page_description | nothing"
+      tal:attributes="content view/page_description/fmt:strip-email/fmt:shorten/500">
+
     <metal:page-javascript
         use-macro="context/@@+base-layout-macros/page-javascript" />
     <metal:block define-slot="head_epilogue"></metal:block>

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-11-16 00:09:49 +0000
+++ lib/lp/bugs/browser/bug.py	2011-11-22 06:59:27 +0000
@@ -544,6 +544,10 @@
     all the pages off IBugTask instead of IBug.
     """
 
+    @cachedproperty
+    def page_description(self):
+        return IBug(self.context).description
+
     @property
     def subscription(self):
         """Return whether the current user is subscribed."""

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-21 15:36:39 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-22 06:59:27 +0000
@@ -661,6 +661,10 @@
         title = FormattersAPI(self.context.bug.title).obfuscate_email()
         return smartquote('%s: "%s"') % (heading, title)
 
+    @cachedproperty
+    def page_description(self):
+        return IBug(self.context).description
+
     @property
     def next_url(self):
         """Provided so returning to the page they came from works."""

=== 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-22 06:59:27 +0000
@@ -11,6 +11,12 @@
 
 from BeautifulSoup import BeautifulSoup
 
+from testtools.matchers import (
+    MatchesAll,
+    Contains,
+    Not,
+    )
+
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -99,25 +105,44 @@
 
     layer = DatabaseFunctionalLayer
 
-    def getBrowserForBugWithEmail(self, email_address, no_login):
-        bug = self.factory.makeBug(
-            title="Title with %s contained" % email_address,
-            description="Description with %s contained." % email_address)
-        return self.getViewBrowser(bug, rootsite="bugs", no_login=no_login)
+    email_address = "[e-mail]"
+
+    def getBrowserForBugWithEmail(self, no_login):
+        self.bug = self.factory.makeBug(
+            title="Title with %s contained" % self.email_address,
+            description="Description with %s contained." % self.email_address)
+        return self.getViewBrowser(
+            self.bug, rootsite="bugs", no_login=no_login)
 
     def test_user_sees_email_address(self):
         """A logged-in user can see the email address on the page."""
-        email_address = "[e-mail]"
-        browser = self.getBrowserForBugWithEmail(
-            email_address, no_login=False)
-        self.assertEqual(7, browser.contents.count(email_address))
+        browser = self.getBrowserForBugWithEmail(no_login=False)
+        self.assertEqual(7, browser.contents.count(self.email_address))
 
     def test_anonymous_sees_not_email_address(self):
         """The anonymous user cannot see the email address on the page."""
-        email_address = "[e-mail]"
-        browser = self.getBrowserForBugWithEmail(
-            email_address, no_login=True)
-        self.assertEqual(0, browser.contents.count(email_address))
+        browser = self.getBrowserForBugWithEmail(no_login=True)
+        self.assertEqual(0, browser.contents.count(self.email_address))
+
+    def test_bug_description_in_meta_description_anonymous(self):
+        browser = self.getBrowserForBugWithEmail(no_login=True)
+        soup = BeautifulSoup(browser.contents)
+        meat = soup.find('meta', dict(name='description'))
+        self.assertThat(meat['content'], MatchesAll(
+            Contains('Description with'),
+            Not(Contains('@')),
+            Contains('...')))  # Ellipsis from hidden address.
+
+    def test_bug_description_in_meta_description_not_anonymous(self):
+        browser = self.getBrowserForBugWithEmail(no_login=False)
+        soup = BeautifulSoup(browser.contents)
+        meat = soup.find('meta', dict(name='description'))
+        # Even logged in users get email stripped from the metadata, in case
+        # they use a tool that copies it out.
+        self.assertThat(meat['content'], MatchesAll(
+            Contains('Description with'),
+            Not(Contains('@')),
+            Contains('...')))  # Ellipsis from hidden address.
 
 
 class TestBugPortletSubscribers(TestCaseWithFactory):

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2011-09-27 13:15:24 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2011-11-22 06:59:27 +0000
@@ -30,6 +30,7 @@
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.interfaces import (
     BrowserNotificationLevel,
     IPrimaryContext,
@@ -843,6 +844,22 @@
         self.assertIn("longpoll", cache.objects)
         self.assertIn("merge_proposal_event_key", cache.objects)
 
+    def test_description_is_meta_description(self):
+        description = (
+            "I'd like to make the bmp description appear as the meta "
+            "description: this does that "
+            + "abcdef " * 300)
+        bmp = self.factory.makeBranchMergeProposal(
+            description=description)
+        browser = self.getUserBrowser(
+            canonical_url(bmp, rootsite='code'))
+        expected_meta = Tag(
+            'meta description',
+            'meta', attrs=dict(
+                name='description',
+                content=description[:497] + '...'))
+        self.assertThat(browser.contents, HTMLContains(expected_meta))
+
 
 class TestBranchMergeProposalChangeStatusOptions(TestCaseWithFactory):
     """Test the status vocabulary generated for then +edit-status view."""

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2011-10-19 12:44:55 +0000
+++ lib/lp/registry/browser/person.py	2011-11-22 06:59:27 +0000
@@ -2950,6 +2950,16 @@
             return "%s does not use Launchpad" % context.displayname
 
     @cachedproperty
+    def page_description(self):
+        context = self.context
+        if context.is_valid_person_or_team:
+            return (
+                self.context.homepage_content
+                or self.context.teamdescription)
+        else:
+            return None
+
+    @cachedproperty
     def enable_xrds_discovery(self):
         """Only enable discovery if person is OpenID enabled."""
         return self.is_delegated_identity

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2011-10-19 12:44:55 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2011-11-22 06:59:27 +0000
@@ -10,9 +10,11 @@
 from storm.store import Store
 from testtools.matchers import (
     DocTestMatches,
+    Equals,
     LessThan,
     Not,
     )
+
 import transaction
 from zope.component import getUtility
 
@@ -97,6 +99,23 @@
             "... Asia/Kolkata (UTC+0530) ..."), doctest.ELLIPSIS
             | doctest.NORMALIZE_WHITESPACE | doctest.REPORT_NDIFF))
 
+    def test_person_view_page_description(self):
+        person_description = self.factory.getUniqueString()
+        person = self.factory.makePerson(
+            homepage_content=person_description)
+        view = create_initialized_view(person, '+index')
+        self.assertThat(view.page_description,
+            Equals(person_description))
+
+    def test_team_page_description(self):
+        description = self.factory.getUniqueString()
+        person = self.factory.makeTeam(
+            description=description)
+        view = create_initialized_view(person, '+index')
+        self.assertThat(
+            view.page_description,
+            Equals(description))
+
 
 class TestPersonViewKarma(TestCaseWithFactory):
 
@@ -891,6 +910,8 @@
         self.assertEqual(1, len(view.errors))
         self.assertEqual(
             'This account is already deactivated.', view.errors[0])
+        self.assertEqual(
+            None, view.page_description)
 
 
 class TestTeamInvitationView(TestCaseWithFactory):

=== modified file 'lib/lp/services/utils.py'
--- lib/lp/services/utils.py	2011-11-09 14:18:29 +0000
+++ lib/lp/services/utils.py	2011-11-22 06:59:27 +0000
@@ -323,18 +323,22 @@
     """, re.VERBOSE)              # ' <- font-lock turd
 
 
-def obfuscate_email(text_to_obfuscate):
+def obfuscate_email(text_to_obfuscate, replacement=None):
     """Obfuscate an email address.
 
-    The email address is obfuscated as <email address hidden>.
+    The email address is obfuscated as <email address hidden> by default,
+    or with the given replacement.
 
     The pattern used to identify an email address is not 2822. It strives
     to match any possible email address embedded in the text. For example,
     mailto:[e-mail] and http://person:[e-mail] both
     match, though the http match is in fact not an email address.
     """
+    if replacement is None:
+        replacement = '<email address hidden>'
     text = re_email_address.sub(
-        r'<email address hidden>', text_to_obfuscate)
+        replacement, text_to_obfuscate)
+    # Avoid doubled angle brackets.
     text = text.replace(
         "<<email address hidden>>", "<email address hidden>")
     return text

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-11-22 00:02:48 +0000
+++ lib/lp/testing/factory.py	2011-11-22 06:59:27 +0000
@@ -609,7 +609,8 @@
         self, email=None, name=None, password=None,
         email_address_status=None, hide_email_addresses=False,
         displayname=None, time_zone=None, latitude=None, longitude=None,
-        selfgenerated_bugnotifications=False, member_of=()):
+        selfgenerated_bugnotifications=False, member_of=(),
+        homepage_content=None):
         """Create and return a new, arbitrary Person.
 
         :param email: The email address for the new person.
@@ -646,6 +647,8 @@
             hide_email_addresses=hide_email_addresses)
         naked_person = removeSecurityProxy(person)
         naked_person._password_cleartext_cached = password
+        if homepage_content is not None:
+            naked_person.homepage_content = homepage_content
 
         assert person.password is not None, (
             'Password not set. Wrong default auth Store?')