Last Comment Bug 369429 - bug reports should contain microformat markup in order to make the information in them more useable.
: bug reports should contain microformat markup in order to make the informatio...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Interface (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Bugzilla 3.2
Assigned To: Daniel Brooks [:db48x]
: default-qa
:
Mentors:
Depends on: 456058
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-05 18:51 PST by Daniel Brooks [:db48x]
Modified: 2008-09-24 01:11 PDT (History)
6 users (show)
LpSolit: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.50 KB, patch)
2007-02-05 18:55 PST, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
patch (3.50 KB, patch)
2007-02-06 08:05 PST, Daniel Brooks [:db48x]
LpSolit: review-
myk: review-
Details | Diff | Splinter Review
patch (4.18 KB, patch)
2007-02-06 12:57 PST, Daniel Brooks [:db48x]
myk: review-
Details | Diff | Splinter Review
patch (4.31 KB, patch)
2007-02-07 16:24 PST, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
working patch (3.53 KB, patch)
2007-02-08 15:55 PST, Daniel Brooks [:db48x]
myk: review-
Details | Diff | Splinter Review
tweaked based on review comments and a conversation on irc (4.15 KB, patch)
2007-02-09 19:10 PST, Daniel Brooks [:db48x]
myk: review+
LpSolit: review-
Details | Diff | Splinter Review
patch (4.90 KB, patch)
2007-02-12 14:57 PST, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
patch (5.18 KB, patch)
2007-02-12 15:13 PST, Daniel Brooks [:db48x]
LpSolit: review-
Details | Diff | Splinter Review
patch (5.20 KB, patch)
2007-02-12 16:23 PST, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
patch (5.17 KB, patch)
2007-02-26 11:51 PST, Daniel Brooks [:db48x]
LpSolit: review+
Details | Diff | Splinter Review

Description Daniel Brooks [:db48x] 2007-02-05 18:51:38 PST
The first thing I can think of is to wrap the names and email addresses in hCard.  Individual bug posts could be events in hCalendar format and reviews could use hReview, but that might be going overboard.

At any rate, I've got a patch for the email addresses.
Comment 1 Daniel Brooks [:db48x] 2007-02-05 18:55:37 PST
Created attachment 254108 [details] [diff] [review]
patch
Comment 2 Frédéric Buclin 2007-02-06 03:56:58 PST
Comment on attachment 254108 [details] [diff] [review]
patch

>Index: webtools/bugzilla/template/en/default/bug/edit.html.tmpl

>+        <aclass="email"  href="mailto:[% bug.assigned_to.email FILTER html %]">

<aclass -> <a class
Comment 3 Daniel Brooks [:db48x] 2007-02-06 08:05:56 PST
Created attachment 254166 [details] [diff] [review]
patch

Heh, oops. Thanks for catching that Frédéric. Do you want to do the review?
Comment 4 Frédéric Buclin 2007-02-06 10:40:17 PST
Comment on attachment 254166 [details] [diff] [review]
patch

>Index: webtools/bugzilla/template/en/default/bug/edit.html.tmpl

>             [% IF bug.qa_contact.login && bug.qa_contact.login.length > 30 %]
>-              <span title="[% bug.qa_contact.login FILTER html %]">
>+              <span title="[% bug.qa_contact.login FILTER html %]" class="fn">
>                 [% bug.qa_contact.identity FILTER truncate(30) FILTER html %]
>               </span>
>             [% ELSE %]

... [% ELSE %] you still want to enclose the name of the QA contact between <span class="fn"> </span>. Your patch missed the ELSE part.

Else it looks good, but I will let myk r+ it as he knows microformats much better than me.
Comment 5 Daniel Brooks [:db48x] 2007-02-06 12:57:58 PST
Created attachment 254196 [details] [diff] [review]
patch

Fixes that and tweaks the truncating case so that it uses an abbr instead of a span.
Comment 6 Myk Melez [:myk] [@mykmelez] 2007-02-06 13:10:17 PST
Comment on attachment 254166 [details] [diff] [review]
patch

>-      <td>
>-        <a href="mailto:[% bug.reporter.email FILTER html %]">
>-          [% bug.reporter.identity FILTER html %]</a>
>+      <td class="vcard">
>+        <a class="email" href="mailto:[% bug.reporter.email FILTER html %]">
>+          <span class="fn">[% bug.reporter.identity FILTER html %]</span></a>

"fn" should refer here to just the reporter's name, not her entire identity string, since her identity  string might also include her email address.  This means that you can't use the user::identity getter here anymore and need to instead generate identity values in template code.

The best way to do that is to create a block that generates the identity (using the same logic as in the identity getter) but adds the appropriate vCard foo.  Then you can reuse that block each place an identity is displayed.

The block should probably go into its own file, i.e. template/en/default/global/user.html.tmpl. (lpsolit: does that seem like the right place to put it?)


>             [% IF bug.qa_contact.login && bug.qa_contact.login.length > 30 %]
>-              <span title="[% bug.qa_contact.login FILTER html %]">
>+              <span title="[% bug.qa_contact.login FILTER html %]" class="fn">
>                 [% bug.qa_contact.identity FILTER truncate(30) FILTER html %]
>               </span>

Per the hCard spec, this <span> should be an <abbr>, since the human-readable information it contains is being truncated.

Otherwise this looks good.  Note that the hCard spec says, "for 'FN's with... three or more [words], the author MUST explicitly markup [sic] the 'N'," but doing this in Bugzilla will be impossible, given that users can name themselves as they like.

Switching from email addresses to arbitrary usernames (or implementing nicknames) and providing a separate "status" message field (which we could put into the vCard "note" field) for messages about users being away and the like would mitigate the problem significantly, but that's out of scope for this fix.  We'll just have to resign ourselves to not necessarily validating.
Comment 7 Myk Melez [:myk] [@mykmelez] 2007-02-06 13:16:44 PST
Comment on attachment 254196 [details] [diff] [review]
patch

Oops, I reviewed the wrong patch.  Sorry about that.  It looks like the <abbr> issue has been taken care of, but the identity issue still remains in this new version of the patch.
Comment 8 Frédéric Buclin 2007-02-06 13:35:36 PST
(In reply to comment #6)

> The block should probably go into its own file, i.e.
> template/en/default/global/user.html.tmpl. (lpsolit: does that seem like the
> right place to put it?)

No. It should be a block within the same template. This can be refactored later to display user accounts in a consistent way across all pages, but that's out of the topic of this bug.

As an enhancement request, we won't take it for 3.0.
Comment 9 Daniel Brooks [:db48x] 2007-02-07 16:24:40 PST
Created attachment 254362 [details] [diff] [review]
patch

better fix. should be fine, but I want to test it on landfill first.
Comment 10 Daniel Brooks [:db48x] 2007-02-08 15:55:46 PST
Created attachment 254465 [details] [diff] [review]
working patch

This one's been tested on http://landfill.bugzilla.org/bug369429/ and works
Comment 11 Myk Melez [:myk] [@mykmelez] 2007-02-09 14:22:11 PST
Comment on attachment 254465 [details] [diff] [review]
working patch

>Index: template/en/default/bug/comments.html.tmpl

>-          <a href="mailto:[% comment.email FILTER html %]">
>-            [% comment.name FILTER html %]</a>
>-          [%+ comment.time FILTER time %] 
>+          <span class="vcard">
>+            <a class="email" href="mailto:[% comment.email FILTER html %]">
>+              <span class="fn">[% comment.name FILTER html %]</span></a>
>+            [%+ comment.time FILTER time %] 
>+          </span>

When comment.name is the user's email address, this is going to cause the "fn" field to contain that email address, which is contrary to the intent of that field.

To fix this, remove line 1478 of Bugzilla, which puts the commenter's email address into comment.name if comment.name is empty.  Then, test for comment.name here: if there is one, do exactly what you're already doing (i.e. put comment.name inside <span class="fn">); otherwise, just put comment.email inside the anchor tag (but without the <span class="fn">).

This way, we only specify "fn" for users that actually provide a name.  As a side effect, the code is less opaque, since comment.name always means "name" instead of sometimes meaning "email address".

Ultimately, we should probably switch from comment.name and comment.email to a comment.author property referencing a User object that we can just pass to the new user_identity block, but that's another bug.


>Index: template/en/default/bug/edit.html.tmpl

>-          <a href="mailto:[% bug.qa_contact.email FILTER html %]">
>-            [% IF bug.qa_contact.login && bug.qa_contact.login.length > 30 %]
>-              <span title="[% bug.qa_contact.login FILTER html %]">
>-                [% bug.qa_contact.identity FILTER truncate(30) FILTER html %]
>-              </span>
>-            [% ELSE %]
>-              [% bug.qa_contact.identity FILTER html %]
>-            [% END %]
>-          </a>
>+          [% PROCESS user_identity user => bug.qa_contact %]

As we discussed on IRC, this means we will no longer be truncating the QA Contact, since the user_identity block performs to truncation.

Based on the current output of this template, that doesn't seem like a problem.  In fact, I'd venture to say that we're better off not truncating this value, which is being given the same amount of space as the un-truncated Reporter and Assignee values.

But I wanted to note it here for the record so folks who care about the change have a chance to comment.


>+[% BLOCK user_identity %]
>+  [% IF user.name %]
>+    <span class="vcard">
>+      <a class="email" href="mailto:[% user.email FILTER html %]">
>+        <abbr class="fn" title="[% user.name FILTER html %]">
>+          [% user.identity FILTER html %]
>+        </abbr>

Use <span> instead of <abbr> (and leave off the "title" attribute), since you aren't abbreviating the name.

Also, use user.name instead of user.identity inside the <span class="fn"> tag, since the user's "fn" should not include their email address, and user.identity includes both name and email address.


>+  [% ELSE %]
>+    <a href="mailto:[% user.email FILTER html %]">
>+      [% user.identity FILTER html %]

user.identity is exactly the same as user.email in this case, but user.email is easier to understand and more precise, so use user.email instead of user.identity here.

Also, make this version also be an hcard by moving the <span class="vcard"> tag outside the IF-ELSE conditional and adding class="email" to the anchor tag, since an hcard that only provides an email address is still a valid hcard.

In fact, you can further simplify by moving the anchor tag outside the conditional and only modifying the content of that tag via the conditional, since it's the only part of the hcard should vary depending on whether or not the user has a name.
Comment 12 Frédéric Buclin 2007-02-09 14:39:18 PST
(In reply to comment #11)

> As we discussed on IRC, this means we will no longer be truncating the QA
> Contact, since the user_identity block performs to truncation.

Why this change? For the record, this limit has been set as part of bug 357490. But as I cannot remember why I set it, it's probably fine to remove it. :)
Comment 13 Daniel Brooks [:db48x] 2007-02-09 15:02:30 PST
(In reply to comment #11)
> (From update of attachment 254465 [details] [diff] [review])
> >Index: template/en/default/bug/comments.html.tmpl
>
> When comment.name is the user's email address, this is going to cause the "fn"
> field to contain that email address, which is contrary to the intent of that
> field.
> 
> To fix this, remove line 1478 of Bugzilla, which puts the commenter's email
> address into comment.name if comment.name is empty.  Then, test for
> comment.name here: if there is one, do exactly what you're already doing (i.e.
> put comment.name inside <span class="fn">); otherwise, just put comment.email
> inside the anchor tag (but without the <span class="fn">).

Ah, I see.

> Also, make this version also be an hcard by moving the <span class="vcard"> tag
> outside the IF-ELSE conditional and adding class="email" to the anchor tag,
> since an hcard that only provides an email address is still a valid hcard.
> 

Cool, I hadn't realized that.

Patch coming up.
Comment 14 Daniel Brooks [:db48x] 2007-02-09 15:04:20 PST
(In reply to comment #12)
> (In reply to comment #11)
> 
> > As we discussed on IRC, this means we will no longer be truncating the QA
> > Contact, since the user_identity block performs to truncation.
> 
> Why this change? For the record, this limit has been set as part of bug 357490.
> But as I cannot remember why I set it, it's probably fine to remove it. :)
> 

At first I was going to keep it, but that was only because for some reason I had thought that it was truncating the names in the comments. Once I realized that it was just the QA field and not the reporter and assigned to fields I just dropped it. Doing that cleaned the code up a bit too.
Comment 15 Daniel Brooks [:db48x] 2007-02-09 19:10:22 PST
Created attachment 254609 [details] [diff] [review]
tweaked based on review comments and a conversation on irc
Comment 16 Myk Melez [:myk] [@mykmelez] 2007-02-12 13:48:55 PST
Comment on attachment 254609 [details] [diff] [review]
tweaked based on review comments and a conversation on irc

Looks great, r=myk

> Also, make this version also be an hcard by moving the <span class="vcard">
> tag outside the IF-ELSE conditional and adding class="email" to the anchor
> tag, since an hcard that only provides an email address is still a valid
> hcard.

Note: Daniel corrected me about this on IRC; actually, a valid hcard must have an "fn" field which contains something other than whitespace, which is why this patch puts user email addresses into the "fn" field for users who have not provided their names.
Comment 17 Myk Melez [:myk] [@mykmelez] 2007-02-12 14:23:37 PST
Setting the approval flag so this gets noticed when the trunk gets unfrozen.
Comment 18 Frédéric Buclin 2007-02-12 14:30:03 PST
Comment on attachment 254609 [details] [diff] [review]
tweaked based on review comments and a conversation on irc

>Index: Bugzilla/Bug.pm

>-        $comment{'name'} = $comment{'name'} || $comment{'email'};

Sorry, but $comment{'name'} is required by BugMail::prepare_comments().
Comment 19 Daniel Brooks [:db48x] 2007-02-12 14:57:34 PST
Created attachment 254868 [details] [diff] [review]
patch
Comment 20 Daniel Brooks [:db48x] 2007-02-12 15:13:57 PST
Created attachment 254873 [details] [diff] [review]
patch
Comment 21 Frédéric Buclin 2007-02-12 15:45:33 PST
Comment on attachment 254873 [details] [diff] [review]
patch

For some reason, this patch completely breaks sudo sessions when the user being impersonated has no privs and you try to access a bug for which the user plays no role. That's a severe regression.
Comment 22 Daniel Brooks [:db48x] 2007-02-12 16:23:56 PST
Created attachment 254877 [details] [diff] [review]
patch

Yes, that is. If Frédéric's hunch is correct (and it seems likely), then this patch should fix it. Is there a testcase I can follow on landfill, or do I have to create a new user with no privs and so on?
Comment 23 Daniel Brooks [:db48x] 2007-02-26 11:51:01 PST
Created attachment 256492 [details] [diff] [review]
patch

per irc discussion
Comment 24 Frédéric Buclin 2007-02-26 12:01:29 PST
Comment on attachment 256492 [details] [diff] [review]
patch

Works fine. r=LpSolit
Comment 25 Daniel Brooks [:db48x] 2007-02-26 12:05:50 PST
checked in to the trunk
Comment 26 Daniel Brooks [:db48x] 2007-02-26 12:18:02 PST
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.172; previous revision: 1.171
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.105; previous revision: 1.104
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.29; previous revision: 1.28
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.98; previous revision: 1.97
done
Comment 27 Max Kanat-Alexander 2008-07-01 00:07:45 PDT
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.

Note You need to log in before you can comment on or make changes to this bug.