Merge lp:~mbp/launchpad/888353-microformats into lp:launchpad

Proposed by Martin Pool
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the source branch.
Merged at revision: 14377
Proposed branch: lp:~mbp/launchpad/888353-microformats
Merge into: lp:launchpad
Diff against target: 173 lines (+50/-12)
7 files modified
lib/lp/app/doc/hierarchical-menu.txt (+1/-1)
lib/lp/app/templates/base-layout.pt (+2/-0)
lib/lp/app/templates/launchpad-hierarchy.pt (+1/-0)
lib/lp/bugs/browser/tests/test_bugcomment.py (+29/-0)
lib/lp/bugs/stories/bugs/xx-bug-comments-truncated.txt (+3/-3)
lib/lp/bugs/templates/bugcomment-box.pt (+13/-7)
lib/lp/bugs/templates/bugtask-index.pt (+1/-1)
To merge this branch: bzr merge lp:~mbp/launchpad/888353-microformats
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+82767@code.launchpad.net

Commit message

[r=rvb][bug=888353] add schema.org microformats for bugs and breadcrumbs

Description of the change

This adds a small amount of schemas.org to the main bug page and to the generic breadcrumbs, with a goal of getting search engines to give better summaries snippets of bug pages that turn up in search results.

To see whether it actually does work or not we'll need to deploy this and wait for it to be indexed. We could do a lot more but this is an experiment to see if it has a noticeable effect.

There are not any (intentional) changes is normal page content or behaviour.

You can see Google's test parser on it at <http://www.google.com/webmasters/tools/richsnippets?url=http%3A%2F%2Fsourcefrog.net%2Ftmp%2F1w%2Fbug1.html&view=cse>.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

Hi Martin, this looks like a very good idea but I've got a few remarks. I'm no microformat expert so please bear with me ;).

= Real questions/remarks =

[0]

8 + itemscope itemtype="http://schema.org/WebPage"

You only include the breadcrumbs here, don't you think it would be useful (for the test to be complete), to also include a name (http://schema.org/Thing) and a URL (http://schema.org/Thing) [and maybe a description (http://schema.org/Thing)]? Obviously the parser can take the url as an identifier but maybe it's better if we explicitly provide one.

[1]

38 + itemprop="commentTime"

I'm not sure "on XXX" is correctly parsed as a date, and I'm pretty sure "a moment ago" won't be. Maybe it would be worth to add the date properly formatted in the tag with an attribute such as """ datetime="2007-10-20" """.

[2]

I cannot help but notice that there is no test for this. I understand this is a test so it's probably ok… Maybe you could include an empty (or almost empty) test for this with a reference to this bug just so that we don't forget to add tests should we develop this further.

= Details =

[0]

37 <span tal:attributes="title comment/datecreated/fmt:datetime"
38 + itemprop="commentTime"
39 tal:content="comment/datecreated/fmt:displaydate">

Weird formatting here.

[1]

45 <td class="bug-comment-index">
46 - <a tal:attributes="href comment/fmt:url"
47 - tal:content="string: #${comment/index}" />
48 + <a itemprop="url"
49 + tal:attributes="href comment/fmt:url"
50 + tal:content="string: #${comment/index}" />

And here.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

2011/11/21 Raphaël Badin <email address hidden>:
> Review: Approve
>
> Hi Martin, this looks like a very good idea but I've got a few remarks.  I'm no microformat expert so please bear with me ;).

Thanks for being willing to review it anyhow.

>
> = Real questions/remarks =
>
> [0]
>
> 8       +    itemscope itemtype="http://schema.org/WebPage"
>
> You only include the breadcrumbs here, don't you think it would be useful (for the test to be complete), to also include a name (http://schema.org/Thing) and a URL (http://schema.org/Thing) [and maybe a description (http://schema.org/Thing)]? Obviously the parser can take the url as an identifier but maybe it's better if we explicitly provide one.

Putting in the description, probably using the same thing for the meta
description, might be useful. (Though, also kind of redundant
perhaps.)

Putting the URL for the page itself seems a bit redundant too, but
that could go off the canoncial_url_recommended.

A place where I can imagine the url being useful is in eg comments on
a bug, which each have a scope and a url of their own.

To some extent we're guessing how the black box of search engines will
behave - in the first cut I'm trying to do something large enough we
may see some difference without spending too much time or making too
intrusive changes.

>
> [1]
>
> 38      +                 itemprop="commentTime"
>
> I'm not sure "on XXX" is correctly parsed as a date, and I'm pretty sure "a moment ago" won't be.  Maybe it would be worth to add the date properly formatted in the tag with an attribute such as """ datetime="2007-10-20" """.

Good point.

>
> [2]
>
> I cannot help but notice that there is no test for this.  I understand this is a test so it's probably ok…  Maybe you could include an empty (or almost empty) test for this with a reference to this bug just so that we don't forget to add tests should we develop this further.

Good idea too.

--
Martin

Revision history for this message
Raphaël Badin (rvb) wrote :

> A place where I can imagine the url being useful is in eg comments on
> a bug, which each have a scope and a url of their own.

Right.

> To some extent we're guessing how the black box of search engines will
> behave - in the first cut I'm trying to do something large enough we
> may see some difference without spending too much time or making too
> intrusive changes.

True… I'm really looking forward to see how this impacts Goo^Wsearch engines. I'm subscribed to the related bug… would you be kind enough to post a small summary of your findings in the bug comments when you'll have assessed the impact of this change?

Revision history for this message
Martin Pool (mbp) wrote :

ok, this update
 * changes to a <time datetime=> tag
 * uses a machine readable date
 * adds a test for that tag

Is there a better way to write this test?

thanks.

Revision history for this message
Raphaël Badin (rvb) wrote :

> ok, this update
> * changes to a <time datetime=> tag
> * uses a machine readable date
> * adds a test for that tag
>
> Is there a better way to write this test?
>
> thanks.

It's a little bit verbose, I'll give you that… but fairly readable still I think.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/app/doc/hierarchical-menu.txt'
--- lib/lp/app/doc/hierarchical-menu.txt 2011-05-27 18:27:59 +0000
+++ lib/lp/app/doc/hierarchical-menu.txt 2011-11-24 01:04:28 +0000
@@ -248,7 +248,7 @@
248represents the current location.248represents the current location.
249249
250 >>> print markup250 >>> print markup
251 <ol class="breadcrumbs">251 <ol itemprop="breadcrumb" class="breadcrumbs">
252 <li>252 <li>
253 <a href="http://launchpad.dev/joy-of-cooking">Joy of cooking</a>253 <a href="http://launchpad.dev/joy-of-cooking">Joy of cooking</a>
254 </li>254 </li>
255255
=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt 2011-11-20 06:38:50 +0000
+++ lib/lp/app/templates/base-layout.pt 2011-11-24 01:04:28 +0000
@@ -62,6 +62,8 @@
62 </head>62 </head>
6363
64 <body id="document"64 <body id="document"
65 itemscope=""
66 itemtype="http://schema.org/WebPage"
65 tal:attributes="class string:tab-${view/menu:selectedfacetname}67 tal:attributes="class string:tab-${view/menu:selectedfacetname}
66 ${view/macro:pagetype}68 ${view/macro:pagetype}
67 ${view/context/fmt:public-private-css}69 ${view/context/fmt:public-private-css}
6870
=== modified file 'lib/lp/app/templates/launchpad-hierarchy.pt'
--- lib/lp/app/templates/launchpad-hierarchy.pt 2010-07-21 18:03:44 +0000
+++ lib/lp/app/templates/launchpad-hierarchy.pt 2011-11-24 01:04:28 +0000
@@ -1,4 +1,5 @@
1<ol1<ol
2 itemprop="breadcrumb"
2 class="breadcrumbs"3 class="breadcrumbs"
3 xmlns:tal="http://xml.zope.org/namespaces/tal"4 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"5 xmlns:metal="http://xml.zope.org/namespaces/metal"
56
=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py 2011-11-16 23:46:01 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py 2011-11-24 01:04:28 +0000
@@ -15,6 +15,14 @@
15from zope.component import getUtility15from zope.component import getUtility
16from zope.security.proxy import removeSecurityProxy16from zope.security.proxy import removeSecurityProxy
1717
18from soupmatchers import (
19 HTMLContains,
20 Tag,
21 )
22
23from canonical.launchpad.ftests import (
24 login_person,
25 )
18from canonical.launchpad.testing.pages import find_tag_by_id26from canonical.launchpad.testing.pages import find_tag_by_id
19from canonical.testing.layers import DatabaseFunctionalLayer27from canonical.testing.layers import DatabaseFunctionalLayer
20from lp.app.interfaces.launchpad import ILaunchpadCelebrities28from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -287,3 +295,24 @@
287 naked_bugtask = removeSecurityProxy(context.default_bugtask)295 naked_bugtask = removeSecurityProxy(context.default_bugtask)
288 removeSecurityProxy(naked_bugtask.pillar).security_contact = person296 removeSecurityProxy(naked_bugtask.pillar).security_contact = person
289 self._test_hide_link_visible(context, person)297 self._test_hide_link_visible(context, person)
298
299
300class TestBugCommentMicroformats(BrowserTestCase):
301
302 layer = DatabaseFunctionalLayer
303
304 def test_bug_comment_metadata(self):
305 owner = self.factory.makePerson()
306 login_person(owner)
307 bug_comment = self.factory.makeBugComment()
308 browser = self.getViewBrowser(bug_comment)
309 iso_date = bug_comment.datecreated.isoformat()
310 self.assertThat(
311 browser.contents,
312 HTMLContains(Tag(
313 'comment time tag',
314 'time',
315 attrs=dict(
316 itemprop='commentTime',
317 title=True,
318 datetime=iso_date))))
290319
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-comments-truncated.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-comments-truncated.txt 2010-03-22 22:56:44 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-comments-truncated.txt 2011-11-24 01:04:28 +0000
@@ -30,14 +30,14 @@
30 ... print div_tag30 ... print div_tag
31 >>> print_comments(browser.contents) #doctest: -ELLIPSIS31 >>> print_comments(browser.contents) #doctest: -ELLIPSIS
32 <div class="boardCommentBody">32 <div class="boardCommentBody">
33 <div class="bug-comment"><p>This33 <div class="bug-comment" itemprop="commentText"><p>This
34 would be a real killer feature. If there...</p></div>34 would be a real killer feature. If there...</p></div>
35 <p>35 <p>
36 <a href="/tomcat/+bug/2/comments/1">Read more...</a>36 <a href="/tomcat/+bug/2/comments/1">Read more...</a>
37 </p>37 </p>
38 </div>38 </div>
39 <div class="boardCommentBody">39 <div class="boardCommentBody">
40 <div class="bug-comment"><p>Oddly40 <div class="bug-comment" itemprop="commentText"><p>Oddly
41 enough the bug system seems only capabl...</p></div>41 enough the bug system seems only capabl...</p></div>
42 <p>42 <p>
43 <a href="/tomcat/+bug/2/comments/2">Read more...</a>43 <a href="/tomcat/+bug/2/comments/2">Read more...</a>
@@ -57,7 +57,7 @@
5757
58 >>> print_comments(browser.contents) #doctest: -ELLIPSIS58 >>> print_comments(browser.contents) #doctest: -ELLIPSIS
59 <div class="boardCommentBody">59 <div class="boardCommentBody">
60 <div class="bug-comment"><p>This60 <div class="bug-comment" itemprop="commentText"><p>This
61 would be a real killer feature. If there is already code61 would be a real killer feature. If there is already code
62 to make it possible, why aren't there tons of press announcements62 to make it possible, why aren't there tons of press announcements
63 about the secuirty possibilities. Imagine - no more embarrassing63 about the secuirty possibilities. Imagine - no more embarrassing
6464
=== modified file 'lib/lp/bugs/templates/bugcomment-box.pt'
--- lib/lp/bugs/templates/bugcomment-box.pt 2011-09-01 10:54:38 +0000
+++ lib/lp/bugs/templates/bugcomment-box.pt 2011-11-24 01:04:28 +0000
@@ -2,6 +2,8 @@
2 xmlns="http://www.w3.org/1999/xhtml"2 xmlns="http://www.w3.org/1999/xhtml"
3 xmlns:tal="http://xml.zope.org/namespaces/tal"3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 itemscope=""
6 itemtype="http://schema.org/UserComments"
5 tal:define="comment context;7 tal:define="comment context;
6 remote_bug_comment_class8 remote_bug_comment_class
7 python: comment.bugwatch and 'remoteBugComment' or '';9 python: comment.bugwatch and 'remoteBugComment' or '';
@@ -19,10 +21,13 @@
19 <tal:comment_owner21 <tal:comment_owner
20 replace="structure comment/owner/fmt:link-display-name-id" />22 replace="structure comment/owner/fmt:link-display-name-id" />
21 wrote23 wrote
22 <span tal:attributes="title comment/datecreated/fmt:datetime"24 <time
23 tal:content="comment/datecreated/fmt:displaydate">25 itemprop="commentTime"
24 7 minutes ago26 tal:attributes="title comment/datecreated/fmt:datetime;
25 </span>:27 datetime comment/datecreated/fmt:isodate"
28 tal:content="comment/datecreated/fmt:displaydate">
29 7 minutes ago
30 </time>:
26 <a tal:attributes="href comment/fmt:url"31 <a tal:attributes="href comment/fmt:url"
27 tal:condition="comment/display_title">32 tal:condition="comment/display_title">
28 <strong tal:content="comment/title" />33 <strong tal:content="comment/title" />
@@ -46,8 +51,9 @@
46 </a>51 </a>
47 </td>52 </td>
48 <td class="bug-comment-index">53 <td class="bug-comment-index">
49 <a tal:attributes="href comment/fmt:url"54 <a itemprop="url"
50 tal:content="string: #${comment/index}" />55 tal:attributes="href comment/fmt:url"
56 tal:content="string: #${comment/index}" />
51 </td>57 </td>
52 </tr>58 </tr>
53 </tbody>59 </tbody>
@@ -79,7 +85,7 @@
79 </li>85 </li>
80 </ul>86 </ul>
8187
82 <div class="bug-comment"88 <div class="bug-comment" itemprop="commentText"
83 tal:content="structure89 tal:content="structure
84 comment/text_for_display/fmt:obfuscate-email/fmt:email-to-html">90 comment/text_for_display/fmt:obfuscate-email/fmt:email-to-html">
85 Comment text.91 Comment text.
8692
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2011-11-14 04:07:15 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2011-11-24 01:04:28 +0000
@@ -129,7 +129,7 @@
129 <div><!-- id="nonportlets"> -->129 <div><!-- id="nonportlets"> -->
130 <div class="top-portlet">130 <div class="top-portlet">
131131
132 <div class="report">132 <div itemprop="mainContentOfPage" class="report">
133 <tal:description133 <tal:description
134 define="global description context/bug/description/fmt:obfuscate-email/fmt:text-to-html" />134 define="global description context/bug/description/fmt:obfuscate-email/fmt:text-to-html" />
135135