Closed
Bug 369429
Opened 17 years ago
Closed 17 years ago
bug reports should contain microformat markup in order to make the information in them more useable.
Categories
(Bugzilla :: User Interface, enhancement)
Bugzilla
User Interface
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: db48x, Assigned: db48x)
References
Details
Attachments
(1 file, 9 obsolete files)
5.17 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Attachment #254108 -
Flags: review?(myk)
Comment 2•17 years ago
|
||
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
Assignee | ||
Comment 3•17 years ago
|
||
Heh, oops. Thanks for catching that Frédéric. Do you want to do the review?
Attachment #254108 -
Attachment is obsolete: true
Attachment #254166 -
Flags: review?(myk)
Attachment #254108 -
Flags: review?(myk)
Comment 4•17 years ago
|
||
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.
Attachment #254166 -
Flags: review?(myk) → review-
Assignee | ||
Comment 5•17 years ago
|
||
Fixes that and tweaks the truncating case so that it uses an abbr instead of a span.
Attachment #254166 -
Attachment is obsolete: true
Attachment #254196 -
Flags: review?(myk)
Comment 6•17 years ago
|
||
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.
Attachment #254166 -
Attachment is obsolete: false
Attachment #254166 -
Flags: review-
Comment 7•17 years ago
|
||
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.
Attachment #254196 -
Flags: review?(myk) → review-
Comment 8•17 years ago
|
||
(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.
Target Milestone: --- → Bugzilla 3.2
Assignee | ||
Comment 9•17 years ago
|
||
better fix. should be fine, but I want to test it on landfill first.
Attachment #254166 -
Attachment is obsolete: true
Attachment #254196 -
Attachment is obsolete: true
Assignee | ||
Comment 10•17 years ago
|
||
This one's been tested on http://landfill.bugzilla.org/bug369429/ and works
Attachment #254362 -
Attachment is obsolete: true
Attachment #254465 -
Flags: review?(myk)
Comment 11•17 years ago
|
||
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.
Attachment #254465 -
Flags: review?(myk) → review-
Comment 12•17 years ago
|
||
(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. :)
Assignee | ||
Comment 13•17 years ago
|
||
(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.
Assignee | ||
Comment 14•17 years ago
|
||
(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.
Assignee | ||
Comment 15•17 years ago
|
||
Attachment #254465 -
Attachment is obsolete: true
Attachment #254609 -
Flags: review?(myk)
Comment 16•17 years ago
|
||
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.
Attachment #254609 -
Flags: review?(myk) → review+
Comment 17•17 years ago
|
||
Setting the approval flag so this gets noticed when the trunk gets unfrozen.
Flags: approval?
Comment 18•17 years ago
|
||
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().
Attachment #254609 -
Flags: review-
Assignee | ||
Comment 19•17 years ago
|
||
Attachment #254609 -
Attachment is obsolete: true
Attachment #254868 -
Flags: review?(myk)
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #254868 -
Attachment is obsolete: true
Attachment #254873 -
Flags: review?(LpSolit)
Attachment #254868 -
Flags: review?(myk)
Comment 21•17 years ago
|
||
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.
Attachment #254873 -
Flags: review?(LpSolit) → review-
Updated•17 years ago
|
Flags: approval?
Assignee | ||
Comment 22•17 years ago
|
||
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?
Attachment #254873 -
Attachment is obsolete: true
Attachment #254877 -
Flags: review?(LpSolit)
Assignee | ||
Comment 23•17 years ago
|
||
per irc discussion
Attachment #254877 -
Attachment is obsolete: true
Attachment #256492 -
Flags: review?(LpSolit)
Attachment #254877 -
Flags: review?(LpSolit)
Comment 24•17 years ago
|
||
Comment on attachment 256492 [details] [diff] [review] patch Works fine. r=LpSolit
Attachment #256492 -
Flags: review?(LpSolit) → review+
Updated•17 years ago
|
Flags: approval+
Assignee | ||
Comment 25•17 years ago
|
||
checked in to the trunk
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•17 years ago
|
||
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•16 years ago
|
||
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•