Last Comment Bug 108243 - Display bug flags in buglists
: Display bug flags in buglists
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: unspecified
: All All
: P1 enhancement with 3 votes (vote)
: Bugzilla 3.6
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 262003 265354 395352 481408 (view as bug list)
Depends on: 509497
Blocks: 418953 780053
  Show dependency treegraph
 
Reported: 2001-11-02 13:23 PST by Daniel Brooks [:db48x]
Modified: 2013-12-27 14:37 PST (History)
16 users (show)
mkanat: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
This is a patch against bugzilla 2.18 to allow flags to be displayed in the buglist (4.01 KB, patch)
2005-05-11 06:30 PDT, Matthew Sackman
no flags Details | Diff | Splinter Review
patch, v1 (5.88 KB, patch)
2009-08-05 11:00 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v1.1 (5.90 KB, patch)
2009-08-05 11:27 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2 (5.60 KB, patch)
2009-08-10 18:16 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2.1 (5.67 KB, patch)
2009-08-10 23:32 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
patch, v2.2 (5.91 KB, patch)
2009-08-16 18:26 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2.3 (7.12 KB, patch)
2009-08-16 19:34 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2.4 (6.02 KB, patch)
2009-08-18 14:04 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
post-checkin fix (971 bytes, patch)
2009-08-19 07:26 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review

Description Daniel Brooks [:db48x] 2001-11-02 13:23:43 PST
Probably easiest to just iteratate over the set of non-obsolete attachments, 
look for r= and sr= flags.
Comment 1 Matthew Tuck [:CodeMachine] 2001-11-09 08:42:27 PST
How will this handle where multiple attachments having different reviewers?

For what purpose do you want this feature?
Comment 2 Myk Melez [:myk] [@mykmelez] 2001-11-12 21:39:18 PST
How about putting the name of the reviewer (or a suitably shortened version of
it) in parentheses next to the status? i.e.:

Status
has-review (joe)
has-super-review (jane)
has-approval (asa)

Matty, the reporter probably wants this feature because attachment statuses are
used on b.m.o. to mark whether or not patches have code review, and if so what
kind.  It is often useful to know not only that a patch has some kind of review
but also who reviewed it.
Comment 3 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-01-17 21:04:23 PST
This bug hasn't been touched in over a year....

Is this any easier now that we have flags (rather than requests) that store this
data?
Comment 4 Asa Dotzler [:asa] 2003-11-24 16:09:05 PST
This has changed substantially with the introduction of the attachment manager
so I'm morphing the bug some. 

We now have three fields that we need to represent (I think). The first is the
"setter" field and that includes the requester or the fulfiller. The second
field is the flag itself (and its status, +/-/?). And the third field is the
requestee.

I think we should have a column for each of these 3 pieces of information.
Alternately, we might want to combine the setter and flag fields into a single
column like we've done in showbug and edit attachment.

I don't think there's any significant value in seperating out the flag name from
the flag states (+/-/?).
Comment 5 Frédéric Buclin 2004-11-24 17:02:00 PST
*** Bug 265354 has been marked as a duplicate of this bug. ***
Comment 6 Frédéric Buclin 2004-11-24 17:58:54 PST
*** Bug 262003 has been marked as a duplicate of this bug. ***
Comment 7 Matthew Sackman 2005-05-11 06:30:16 PDT
Created attachment 183282 [details] [diff] [review]
This is a patch against bugzilla 2.18 to allow flags to be displayed in the buglist

The attached patch is largely based on the patch by Stuart Donaldson in bug
265354 

However, it differs in that it only presents a single additional column and
displays all flags for each bug in this column in a hopefully intuitive way.
Comment 8 Julian Gorfajn 2005-08-02 07:20:27 PDT
flags that were set and then unset will still show up in your list. Perhaps you
should add  "...and flags.is_active=1" to the sql call?
Comment 9 Frédéric Buclin 2007-09-07 02:31:40 PDT
*** Bug 395352 has been marked as a duplicate of this bug. ***
Comment 10 Tomasz Jaszowski 2007-09-20 00:23:23 PDT
Where can I find this patch for 3.0.1?
Comment 11 Max Kanat-Alexander 2007-09-20 07:45:56 PDT
(In reply to comment #10)
> Where can I find this patch for 3.0.1?

  You can find it when you write it. :-)
Comment 12 Tomasz Jaszowski 2007-09-20 21:22:20 PDT
I hoped that it was merged into 3.0 :)

btw. Is there any way to search bugs without any flag set?

for me this 'show flag on buglist.cgi page' is workaround to find bug without any flag set
Comment 13 Frédéric Buclin 2007-11-20 12:39:37 PST
I may work on it for Bugzilla 4.0, unless someone is faster than me and wants it for 3.2. It should be pretty easy now that we have $bug->flag_types->{flags} implemented.

I wonder if we want one column for bug flags and another one for attachments flags (only for non-obsolete attachments) or a single one with all flags.
Comment 14 Frédéric Buclin 2009-03-04 08:12:52 PST
*** Bug 481408 has been marked as a duplicate of this bug. ***
Comment 15 Frédéric Buclin 2009-08-05 11:00:40 PDT
Created attachment 392752 [details] [diff] [review]
patch, v1

This patch implements the ability to watch bug flags in buglists. This works on both MySQL and PostgreSQL (no idea about Oracle). The only problem with this patch is that when you use "NOT flag is equal to ..." in queries, flags are duplicated in the buglist. No idea why (yet).

I didn't add attachment flags for two reasons: we display no attachment attributes in buglists, and I don't want to add extra security checks to exclude private attachments if you cannot view them.
Comment 16 Frédéric Buclin 2009-08-05 11:27:02 PDT
Created attachment 392762 [details] [diff] [review]
patch, v1.1

Easy to fix. I forgot to prepend DISTINCT.
Comment 17 Tomasz Jaszowski 2009-08-06 03:30:15 PDT
(In reply to comment #16)
> Created an attachment (id=392762) [details]
> patch, v1.1

which stable version can be patched with this patch?


(as for version 3.2.2 it doesn't work :/ )
Comment 18 Frédéric Buclin 2009-08-06 03:38:19 PDT
(In reply to comment #17)
> which stable version can be patched with this patch?

The patch applies cleanly on 3.4.1, but I didn't test it there. It will be implemented in Bugzilla 3.6.
Comment 19 Max Kanat-Alexander 2009-08-10 10:37:38 PDT
Comment on attachment 392762 [details] [diff] [review]
patch, v1.1

>+sub sql_group_concat {
>+    my ($self, $text) = @_;
>+    return "group_concat($text)";
>+}

  This is missing the possibility of a separator, which is important because we actually specify the separator in the one place we currently use group_concat. 

>@@ -224,6 +229,24 @@ sub bz_setup_database {
>+    # Add custom functions here.
>+    my $exists = $self->selectrow_array('SELECT 1 FROM pg_proc WHERE proname = ?',
>+                                         undef, 'group_concat');

  Let's call it GROUP_CONCAT to be consistent with MySQL.

>+                     ELSE $1 || \', \' || $2

  The MySQL default is just a comma (no space), so let's do the same thing here.

>+    my $flag_sql = $dbh->sql_group_concat('DISTINCT ' . $dbh->sql_string_concat('flagtypes.name', 'flags.status'), '", "');

  Either our Oracle driver will have to implement GROUP_CONCAT or you have to call UNIVERSAL::can($dbh, 'sql_group_concat') everywhere you use it.

  Also, are you sure that DISTINCT works on PostgreSQL?

>+        flags                => { title => 'Flags', name => $flag_sql },

  Instead of this, enable "buglist" on the Flag column in fielddefs, and then put that SQL in %special_sql below.


  I'm going to do some testing on the code, now, too.
Comment 20 Frédéric Buclin 2009-08-10 10:45:07 PDT
(In reply to comment #19)
>   Either our Oracle driver will have to implement GROUP_CONCAT or you have to
> call UNIVERSAL::can($dbh, 'sql_group_concat') everywhere you use it.

xiaoou said he would implement it as soon as this patch is ready.


>   Also, are you sure that DISTINCT works on PostgreSQL?

Of course. If I write a function for Pg, I test it on Pg. :)


>   Instead of this, enable "buglist" on the Flag column in fielddefs, and then
> put that SQL in %special_sql below.

There is only "flagtypes.name" in fielddefs, not "flags". Isn't this going to have side-effects as the SQL query appends the flag status to the flag name?
Comment 21 Max Kanat-Alexander 2009-08-10 16:49:50 PDT
(In reply to comment #20)
> There is only "flagtypes.name" in fielddefs, not "flags". Isn't this going to
> have side-effects as the SQL query appends the flag status to the flag name?

  I don't think so, no. The "buglist" stuff and the actual searching stuff are pretty separate.
Comment 22 Frédéric Buclin 2009-08-10 18:16:29 PDT
Created attachment 393684 [details] [diff] [review]
patch, v2
Comment 23 Max Kanat-Alexander 2009-08-10 21:32:54 PDT
Comment on attachment 393684 [details] [diff] [review]
patch, v2

>+    # Aliases cannot contain dots in them. We convert them to underscores.
>+    my @sql_fields = map { my $alias = $_; $alias =~ s/\./_/g; $_ eq EMPTY_COLUMN ? EMPTY_COLUMN
>+                           : COLUMNS->{$_}->{name} . ' AS ' . $alias } @fields;

  If we're going do to that much in a map, let's just make it a foreach loop where we build the array. :-)

  You know, now that I've made you switch it to flagtypes.name, I'm a bit concerned about what happens if we enable showing attachment flags in the future? Maybe it would be clearer to just call it bug_flags and make it custom? This isn't a showstopper at this point, though, so if you want to just fix the above bit and resubmit, that would be fine too.
Comment 24 Frédéric Buclin 2009-08-10 23:32:33 PDT
Created attachment 393718 [details] [diff] [review]
patch, v2.1

Let's think about attachment flags when it's time to think about them. :) For now, I keep the code as is.
Also, I keep Bugzilla->has_flags in colchange.cgi for now. It should go away in a separate bug (one advantage of ->has_flags over ->any_exist is that it's cached, and it's already used before this call, in the header).
Comment 25 Max Kanat-Alexander 2009-08-11 10:07:57 PDT
Comment on attachment 393718 [details] [diff] [review]
patch, v2.1

>+        push(@sql_fields, $field eq EMPTY_COLUMN ? EMPTY_COLUMN
>+                          : COLUMNS->{$field}->{name} . " AS $alias");

  On checkin, I think it would be nice to expand that to an if/else block, since we've expanded the whole thing to a foreach.

  And that's fine about has_flags. Probably what we should do with it is cache it for the templates like we do for use_keywords (we can call it use_flags or change use_keywords to have_keywords) and then use Bugzilla::Flag->any_exist in the cgi code.

  Also, maybe we should be adding the requestee in, for requestable flags? Otherwise, it all looks fine and works fine over here. :-)
Comment 26 Frédéric Buclin 2009-08-11 15:12:48 PDT
(In reply to comment #25)
>   Also, maybe we should be adding the requestee in, for requestable flags?

I thought about that too, but most if not all *bug* flags I can think of have no requestee (except e.g. the "testcase" flag on b.m.o). I think we can add it later if we really see a need for it.

Thanks for the review! :)
Comment 27 Frédéric Buclin 2009-08-16 18:26:38 PDT
Created attachment 394757 [details] [diff] [review]
patch, v2.2

I forgot to also fix the "order by" list. As we converted dots to underscores in aliases, I have to do the same in the "order by" list.
Comment 28 Frédéric Buclin 2009-08-16 19:34:05 PDT
Created attachment 394759 [details] [diff] [review]
patch, v2.3

Only aliases must be converted. My previous patch broke that. Also, I fixed your "fix on checkin" while I was on it. Now it's working fine.
Comment 29 Frédéric Buclin 2009-08-18 14:04:14 PDT
Created attachment 395141 [details] [diff] [review]
patch, v2.4

Now using COLUMNS in place of $not_an_alias.
Comment 30 Max Kanat-Alexander 2009-08-18 15:01:17 PDT
Comment on attachment 395141 [details] [diff] [review]
patch, v2.4

Looks fine.

s/converted/convert/ on checkin.
Comment 31 Frédéric Buclin 2009-08-18 15:10:17 PDT
Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.175; previous revision: 1.174
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.34; previous revision: 1.33
done
Checking in template/en/default/list/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.47; previous revision: 1.46
done
Comment 32 Frédéric Buclin 2009-08-19 07:26:18 PDT
Created attachment 395307 [details] [diff] [review]
post-checkin fix

I reverted a change in field-descs.none.tmpl: I initally wrote flagtypes_name, but it's really flagtypes.name. I found that when I noticed that the french templates were still displaying "Flags" in english. It's a one-character change, so I'm not requesting review for this fix.

Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.35; previous revision: 1.34
done
Comment 33 bigstijn 2009-12-22 05:52:42 PST
I suggest to change the bug title into "Add bug flags as column in buglist.cgi".

Bug flag due to comment 23
column (and not columns) due to comment 7

At first sight, we would favor more the "columns" (1 per flag), but ok.
Comment 34 Max Kanat-Alexander 2010-02-22 12:20:02 PST
Added to the release notes in bug 547466.
Comment 35 Reed Loden [:reed] (use needinfo?) 2010-02-27 01:44:46 PST
Comment on attachment 395141 [details] [diff] [review]
patch, v2.4

>-    {name => 'flagtypes.name',        desc => 'Flag'},
>+    {name => 'flagtypes.name',        desc => 'Flags', buglist => 1},
...
>-                   "flagtypes.name"          => "Flag",
>+                   "flagtypes_name"          => "Flags",

Changing "Flag" to "Flags" is going to break scripts/bots that parse bugmail such as mozbot. If you insist on making this change, please make sure it's relnoted in a large bold print, as people will need to fix their code to work with this change.
Comment 36 Max Kanat-Alexander 2010-02-27 14:52:16 PST
  Well, it affects approximately two people--me and Wolf, the only bugmail parser maintainers that I know of. So we don't need to relnote it, since Wolf and I now both already know about it.

  Bugmail is not an API.
Comment 37 Reed Loden [:reed] (use needinfo?) 2010-02-27 17:33:29 PST
(In reply to comment #36)
>   Well, it affects approximately two people--me and Wolf, the only bugmail
> parser maintainers that I know of.

Just because you're not aware of others parsing bugmail is not a valid excuse to not just add a one-line mention of this change to the relnotes.

>  Bugmail is not an API.

Sure, but that's only because Bugzilla's current API doesn't yet meet the needs of its consumers who, at least for now, still have to resort to such "hacks" in many cases.

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