369429 - bug reports should contain microformat markup in order to make the information in them more useable.

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Daniel Brooks [:db48x]]

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]]

Attachment: patch

Comment 2

[Frédéric Buclin]

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]]

Attachment: patch

Heh, oops. Thanks for catching that Frédéric. Do you want to do the review?

Comment 4

[Frédéric Buclin]

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]]

Attachment: patch

Fixes that and tweaks the truncating case so that it uses an abbr instead of a span.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

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]]

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]

(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]]

Attachment: patch

better fix. should be fine, but I want to test it on landfill first.

Comment 10

[Daniel Brooks [:db48x]]

Attachment: working patch

This one's been tested on http://landfill.bugzilla.org/bug369429/ and works

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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]

(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]]

(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]]

(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]]

Attachment: tweaked based on review comments and a conversation on irc

Comment 16

[Myk Melez [:myk] [@mykmelez]]

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]]

Setting the approval flag so this gets noticed when the trunk gets unfrozen.

Comment 18

[Frédéric Buclin]

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]]

Attachment: patch

Comment 20

[Daniel Brooks [:db48x]]

Attachment: patch

Comment 21

[Frédéric Buclin]

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]]

Attachment: 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]]

Attachment: patch

per irc discussion

Comment 24

[Frédéric Buclin]

Comment on attachment 256492 [details] [diff] [review]
patch

Works fine. r=LpSolit

Comment 25

[Daniel Brooks [:db48x]]

checked in to the trunk

Comment 26

[Daniel Brooks [:db48x]]

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]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.