332149 - Ability to have symbols placed next to user names based on group membership (group icons)

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Dave Miller [:justdave]]

There are some things that you might want symbols to appear next to a user's name in Bugzilla for (think of the stars and so forth on eBay).

For example:  This user has CVS Commit access.  This user is a superreviewer.  Etc.

It'd be a cool option on the groups editor probably, to upload an icon to be associate with that group, and if that icon is present, it would be displayed next to users who have that group membership.

Comment 1

[Frédéric Buclin]

Read also https://bugzilla.mozilla.org/show_bug.cgi?id=322086#c2

Comment 2

[Asa Dotzler [:asa]]

things that we thought would be valuable for new patch contributors:

be able to tell from this avatar that someone has or does not have CVS access, and that the user is or isn't a module owner/domain expert (even cooler if you could get a titletip on mouseover with details.)

Comment 3

[Gervase Markham [:gerv]]

Linking this to groups does seem to be the right way to go. We probably want to be able to define an icon and a tooltip for that icon per group.

Gerv

Comment 4

[Asa Dotzler [:asa]]

I'd like it if we could work with a designer and a usability person to get this down to a single icon/avatar that conveys as much information as possible (assisted, of course, by a tooltip or similar for details)

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Just adding this to my design radar. :)

Comment 6

[Gervase Markham [:gerv]]

Mike: are you doing UI design for Bugzilla? Cool :-)

Gerv

Comment 7

[Frédéric Buclin]

I will implement the backend and some very ugly icons. This will force UI-guys to attach nice icons asap. :)

Comment 8

[Frédéric Buclin]

Attachment: patch, v1

Comment 9

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

Comment on attachment 258001 [details] [diff] [review]
patch, v1


>-        $comment{'email'} .= Bugzilla->params->{'emailsuffix'};
>+        $comment{'commenter'} = new Bugzilla::User($comment{'userid'});

Nit: seems like this should be called "author" or the like.


>+            my $commenter = $comment->{'commenter'};
>             $result .= "\n\n--- Comment #$count from ";
>-            if ($comment->{'name'}) {
>-                $result .= $comment->{'name'} . " <" . $comment->{'email'} . ">";
>+            if ($commenter->name) {
>+                $result .= $commenter->name . " <" . $commenter->email . ">";
>             } else {
>-                $result .= $comment->{'email'};
>+                $result .= $commenter->email;
>             }

Nit: it'd be nice to factor this out with the other cases of it in templates.


>+    <th>Icon URL:</th>
>+    <td colspan="3"><input type="text" size="70" id="icon_url" name="icon_url"></td>

Nit: it would be useful to add a maxlength="255" here and in edit.html.tmpl to match the TINYTEXT length restriction.


>+<p>
>+  <b>Icon URL</b> is optional, and is the URL pointing to the icon (small image)
>+  used to identify the group. It may be either a relative URL to the base URL
>+  of this installation or an absolute URL.
>+</p>

Nit: I think you don't have to explain what an icon is. :-)  But it would be useful to explain where the icon appears.


>+            <img src="[% group.icon_url FILTER html %]"
>+                 alt="[% group.name FILTER html %]"
>+                 title="[% group.name FILTER html %] - [% group.description FILTER html %]"
>+                 width="16" height="16">

I don't think it makes sense to specify the width and height.  Specifying these values will distort the icon if it isn't exactly this size, and installations should be able to choose icons of different sizes (and match their icon size to the font size they specify in a custom skin).

Otherwise, this looks good.

Comment 11

[Frédéric Buclin]

Attachment: patch, v2

Addressing myk's comments.

Comment 12

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

Comment on attachment 271870 [details] [diff] [review]
patch, v2

>Index: Bugzilla/BugMail.pm

>+            my $author = $comment->{'author'};
>             $result .= "\n\n--- Comment #$count from ";
>-            if ($comment->{'name'}) {
>-                $result .= $comment->{'name'} . " <" . $comment->{'email'} . ">";
>+            if ($author->name) {
>+                $result .= $author->name . " <" . $author->email . ">";
>             } else {
>-                $result .= $comment->{'email'};
>+                $result .= $author->email;

Nit: shouldn't this use $author->identity?


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

>+        [% commenter = comment.author %]
>         <span class="bz_comment_head">
>           <span class="comment_rule">-------</span> <i>Comment
>           <a name="c[% count %]" href="show_bug.cgi?id=[% bug.bug_id %]#c[% count %]">
>             #[% count %]</a> From 
>           <span class="vcard">
>-            <a class="fn email" href="mailto:[% comment.email FILTER html %]">
>-              [% (comment.name || comment.email) FILTER html %]
>+            <a class="fn email" href="mailto:[% commenter.email FILTER html %]">
>+              [% (commenter.name || commenter.login) FILTER html %]
>             </a>
>           </span>
>+          [% FOREACH group = commenter.direct_group_membership %]

Nit: the temporary variable seems unnecessary here.  Seems like you could just use comment.author everywhere directly, f.e. comment.author.name.  Or, if you do want a temporary variable, then "author" seems like a better name for it.


>Index: template/en/default/global/messages.html.tmpl

>+          [% CASE 'icon_url' %]
>+            <li>The URL pointing to the group icon has been updated.</li>

Nit: I think "the group icon URL" is clearer than "the URL pointing to the group icon".

Otherwise looks great!  Looking forward to this functionality. r=myk

Comment 13

[Frédéric Buclin]

I will fix the nits on checkin.

Comment 14

[Frédéric Buclin]

(In reply to comment #12)
> Nit: shouldn't this use $author->identity?

Note that doing it removes the emailsuffix from bugmail (except for the To: field, of course). This won't affect installations not using it, but for those using it, they will now see the exact same name as they see from the web, e.g. Frédéric Buclin <LpSolit> instead of Frédéric Buclin <[e-mail]> if we all had a @gmail.com email address and emailsuffix was set to @gmail.com. That's a reasonable change IMO.

The CVS server seems to have some problems today. I had to commit the patch twice to have all changes uploaded. Here is what I could collect:

mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm 	1.39
mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm 	1.88
mozilla/webtools/bugzilla/Bugzilla/Bug.pm 	1.197
mozilla/webtools/bugzilla/Bugzilla/BugMail.pm 	1.108
mozilla/webtools/bugzilla/Bugzilla/Group.pm 	1.21
mozilla/webtools/bugzilla/Bugzilla/User.pm 	1.158
mozilla/webtools/bugzilla/editgroups.cgi 	1.84
mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl 	1.31
mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl 	1.21
mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl 	1.63
mozilla/webtools/bugzilla/template/en/default/admin/groups/create.html.tmpl 	1.9
	mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tmpl 	1.15

Comment 15

[Frédéric Buclin]

Attachment: checked in patch, v2.1

For the record, here is the patch I committed.

Comment 16

[Dave Miller [:justdave]]

(In reply to comment #14)
> The CVS server seems to have some problems today. I had to commit the patch
> twice to have all changes uploaded. Here is what I could collect:

Probably email issues, since it tries to send email to bonsai during the commit.  Our mail servers have been under a DDoS attack this morning.

Comment 17

[Max Kanat-Alexander]

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