Annotate tracker page with RDFa

setting for review

+++ modules/tracker/tracker.pages.inc	12 Nov 2009 19:17:55 -0000
@@ -55,17 +55,52 @@ function tracker_page($account = NULL, $
+      $num_replies = array(

we should be consistent here and use the same name throughout the patch. comment_count is what is used elsewhere in Drupal.

+++ modules/tracker/tracker.pages.inc	12 Nov 2009 19:17:55 -0000
@@ -55,17 +55,52 @@ function tracker_page($account = NULL, $
+        'data' => t('!time ago', array('!time' => format_interval(REQUEST_TIME - $node->last_activity))),

last_activity is not the changed value of the node, but comes from tracker_node.changed and reflects the date the node was last saved or commented on. I suggest to add a mapping to the default node mapping, called last_activity and mapped to sioc:last_reply_date by default.

This review is powered by Dreditor.

+++ modules/node/node.module	13 Nov 2009 16:24:55 -0000
@@ -773,16 +773,26 @@ function node_rdf_mapping() {
+        'replies' => array(
+          'predicates' => array('sioc:num_replies'),
+        ),
+        'last_activity' => array(
+          'predicates' => array('sioc:last_reply_date'),
+          'datatype' => 'xsd:dateTime',
+          'callback' => 'date_iso8601',
+        )

Minor comment: the last closing parenthesis usually also has a comma in the end.

+++ modules/tracker/tracker.pages.inc	24 Nov 2009 11:19:13 -0000
@@ -59,13 +59,53 @@ function tracker_page($account = NULL, $
+      $comment_count = array(
+        'class' => array('replies'),
+        'data' => $comments,
+      );

I like $comment_count as variable name for the amount of replies.

+++ modules/node/node.module	24 Nov 2009 11:19:13 -0000
@@ -781,16 +781,26 @@ function node_rdf_mapping() {
+        'replies' => array(
+          'predicates' => array('sioc:num_replies'),
+        ),

Let's use it here too instead of 'replies'. It's also what is used in #632092: Number of replies of a node in RDFa

- Elaborated on some of the documentation.
- Adds the number of replies in a content attribute (to avoid the "X new" being included in the count).
- Fix a bug in rdf_rdfa_attributes() where a callback function should not required when $data is provided or a datatype is specified in the mapping.

I ran some benchmarking with 100 nodes, 100 comments on each and ab -n 500 -c 1 against the tracker page:

HEAD        : 5.11 requests/sec
HEAD patched: 4.99 requests/sec

which is 2.5% impact.

remove trailing whitespaces.

removing the number of replies in a content attribute and will create a separate issue for that. so from #18, we now have:
- Elaborated on some of the documentation.
- Removed trailing whitespaces.

The benchmark is not affected.

EDIT: see separate issue #642122: Allow for RDFa datatype attribute without callback function

fixed all of Dries' comments.

fixed more documentation in rdf.test

updated the patch to take advantage of #642122-4: Allow for RDFa datatype attribute without callback function

Thanks sambrin for chiming in. You have a point, we should annotate the last activity regardless since it is displayed on the tracker page anyways. rerolled the patch to take this into account.

I've open a discussion on this topic on the sioc-dev mailing list: http://groups.google.com/group/sioc-dev/browse_thread/thread/5dec36e7e43... - let's see what comes out of it.

The sioc:last_activity_date property was added to the SIOC vocabulary (thanks to John Breslin). We can go ahead and update the patch:

• when comment_count = 0, we use sioc:last_activity_date and dc:modified (we know for sure that the last activity was triggered by a node creation or update)
• when comment_count > 0, we only use sioc:last_activity_date because we don't know whether this last activity is due to a node modification or a comment which has been added.

not sure what's up with the bot. rerolling.

I was about to RTBC this, but I though I would try to clean up the logic in the previous patch instead. Not much change in the RDFa output, and so this should be easy to RTBC after someone else reviews it.

In #26, this issue became dependent on #642122: Allow for RDFa datatype attribute without callback function so that the content attribute is taken care by rdf_rdfa_attributes(), hence the rdf_passthrough callback in the comment_count mapping of the recent patches. (which saves us having to add the content attribute manually like Lin did in #42). However, after some thinking, I'm not sure we should use this rdf_passthrough callback: the fact that we need to add a content attribute is only due to the fact that we have extra markup in the td tag which is added by the tracker module, but has nothing to do with the RDF mapping. I think in this particular case the content attribute should be added outside rdf_rdfa_attributes(), like in #42. thoughts?

We should also add this content attribute to the tests.

The patch improvements look good.

+++ modules/rdf/rdf.test	5 Jan 2010 14:38:23 -0000
@@ -188,3 +188,121 @@ class RdfMappingDefinitionTestCase exten
+    // Need to create a registered user.

Is this comment appropriate or necessary here? where is the registered user created?

+++ modules/rdf/rdf.test	5 Jan 2010 14:38:23 -0000
@@ -188,3 +188,121 @@ class RdfMappingDefinitionTestCase exten
+    // Check to see whether there is a table row for the node with the about
+    // attribute set, whether it contains a c

The comment is not finished.

+++ modules/rdf/rdf.test	5 Jan 2010 14:38:23 -0000
@@ -188,3 +188,121 @@ class RdfMappingDefinitionTestCase exten
+	// Navigate to tracker page.
+    $this->drupalGet('tracker');

indentation issue

+++ modules/rdf/rdf.test	5 Jan 2010 14:38:23 -0000
@@ -188,3 +188,121 @@ class RdfMappingDefinitionTestCase exten
+    // Tests whether the property has been set for number of comments.
+    $tracker_replies = $this->xpath("//tr[@about='$url']//td[contains(@property, 'sioc:num_replies') and contains(@content, '0')]");

- we could simply force @content='0' here because the content attribute is supposed to only contain that value.
- we should also check the value of content when a comment has been posted (@content='1') like it is done further for the last_activity attribute.

9 days to code freeze. Better review yourself.

looks good, setting for review.

I like the patch. A few minor documentation fixes below. (leaving as needs review since these are really minor).

+++ modules/rdf/rdf.test	5 Jan 2010 17:26:36 -0000
@@ -188,3 +188,118 @@ class RdfMappingDefinitionTestCase exten
+   * Navigate to tracker page and ensure the mapping is used.

Fix comment: this function does not actually navigate to the tracker page, this is done by the helper functions.

+++ modules/rdf/rdf.test	5 Jan 2010 17:26:36 -0000
@@ -188,3 +188,118 @@ class RdfMappingDefinitionTestCase exten
+    // Pass both the anonymously posted node and the administrator posted node
+    // through to test for the RDF properties.

Nitpick: "to test for the RDFa attributes" would be more appropriate - strictly speaking, we not only test for RDF properties, but also RDF subject and objects in the RDFa ;)

+++ modules/rdf/rdf.test	5 Jan 2010 17:26:36 -0000
@@ -188,3 +188,118 @@ class RdfMappingDefinitionTestCase exten
+    $this->assertTrue($tracker_replies, t('Num replies property and content attributes found on @user content. Content attribute is numeric.', array('@user'=> $user)));

"Content attributes is numeric" does not really apply any longer.

+++ modules/rdf/rdf.test	5 Jan 2010 17:26:36 -0000
@@ -188,3 +188,118 @@ class RdfMappingDefinitionTestCase exten
+    // posted, meaning the latest activity is changes to the node itself

missing ending period. Maybe could be rewritten as "the latest activity reflects the changes of the node itself"?

+++ modules/rdf/rdf.test	5 Jan 2010 17:26:36 -0000
@@ -188,3 +188,118 @@ class RdfMappingDefinitionTestCase exten
+    $this->assertTrue($tracker_replies, t('Num replies property and content attributes found on @user content. Content attribute is numeric.', array('@user'=> $user)));

"Content attributes is numeric" does not really apply any longer.

9 days to code freeze. Better review yourself.

This will need a reroll since #652588: Update RDF mapping definition in the core modules was just committed :)

Thanks Lin for your patience! patch looks good to go now.