Display bug flags in buglists

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Query/Bug List
P1
enhancement
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: db48x, Assigned: Frédéric Buclin)

Tracking

unspecified
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

16 years ago
Probably easiest to just iteratate over the set of non-obsolete attachments, 
look for r= and sr= flags.
How will this handle where multiple attachments having different reviewers?

For what purpose do you want this feature?
Target Milestone: --- → Future
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.
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

14 years ago
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 (+/-/?).
Summary: Add column to bug list to show r=/sr=/etc → Add request tracker fields as columns in buglist.cgi
Severity: normal → enhancement
(Assignee)

Comment 5

13 years ago
*** Bug 265354 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

13 years ago
*** Bug 262003 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Summary: Add request tracker fields as columns in buglist.cgi → Add request tracker fields (flags) as columns in buglist.cgi

Comment 7

12 years ago
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

12 years ago
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?

Updated

11 years ago
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Future → ---
(Assignee)

Updated

11 years ago
Assignee: myk → query-and-buglist
(Assignee)

Updated

10 years ago
Duplicate of this bug: 395352
(Assignee)

Updated

10 years ago
OS: Other → All
Hardware: Other → All
Summary: Add request tracker fields (flags) as columns in buglist.cgi → Add flags as columns in buglist.cgi

Comment 10

10 years ago
Where can I find this patch for 3.0.1?

Comment 11

10 years ago
(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

10 years ago
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
(Assignee)

Comment 13

10 years ago
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.
Target Milestone: --- → Bugzilla 4.0

Updated

9 years ago
Blocks: 418953
(Assignee)

Updated

8 years ago
Duplicate of this bug: 481408
(Assignee)

Comment 15

8 years ago
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.
Attachment #183282 - Attachment is obsolete: true
Attachment #392752 - Flags: review?(mkanat)
(Assignee)

Comment 16

8 years ago
Created attachment 392762 [details] [diff] [review]
patch, v1.1

Easy to fix. I forgot to prepend DISTINCT.
Assignee: query-and-buglist → LpSolit
Attachment #392752 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #392762 - Flags: review?(mkanat)
Attachment #392752 - Flags: review?(mkanat)

Comment 17

8 years ago
(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 :/ )
(Assignee)

Comment 18

8 years ago
(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.

Updated

8 years ago
Priority: -- → P1
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6

Updated

8 years ago
Attachment #392762 - Flags: review?(mkanat) → review-

Comment 19

8 years ago
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.
(Assignee)

Comment 20

8 years ago
(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?

Updated

8 years ago
Depends on: 509497

Comment 21

8 years ago
(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.
(Assignee)

Comment 22

8 years ago
Created attachment 393684 [details] [diff] [review]
patch, v2
Attachment #392762 - Attachment is obsolete: true
Attachment #393684 - Flags: review?(mkanat)

Comment 23

8 years ago
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.
Attachment #393684 - Flags: review?(mkanat) → review-
(Assignee)

Comment 24

8 years ago
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).
Attachment #393684 - Attachment is obsolete: true
Attachment #393718 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #393718 - Flags: review?(mkanat) → review+

Comment 25

8 years ago
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. :-)

Updated

8 years ago
Flags: approval+
(Assignee)

Comment 26

8 years ago
(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! :)
(Assignee)

Comment 27

8 years ago
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.
Attachment #393718 - Attachment is obsolete: true
Attachment #394757 - Flags: review?(mkanat)
(Assignee)

Comment 28

8 years ago
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.
Attachment #394757 - Attachment is obsolete: true
Attachment #394759 - Flags: review?(mkanat)
Attachment #394757 - Flags: review?(mkanat)
(Assignee)

Comment 29

8 years ago
Created attachment 395141 [details] [diff] [review]
patch, v2.4

Now using COLUMNS in place of $not_an_alias.
Attachment #394759 - Attachment is obsolete: true
Attachment #395141 - Flags: review?(mkanat)
Attachment #394759 - Flags: review?(mkanat)

Comment 30

8 years ago
Comment on attachment 395141 [details] [diff] [review]
patch, v2.4

Looks fine.

s/converted/convert/ on checkin.
Attachment #395141 - Flags: review?(mkanat) → review+
(Assignee)

Comment 31

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED
(Assignee)

Comment 32

8 years ago
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

8 years ago
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.
(Assignee)

Updated

7 years ago
Summary: Add flags as columns in buglist.cgi → Display bug flags in buglists

Comment 34

7 years ago
Added to the release notes in bug 547466.
Keywords: relnote
Keywords: relnote
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

7 years ago
  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.
Keywords: relnote
(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.
(Assignee)

Updated

5 years ago
Blocks: 780053
You need to log in before you can comment on or make changes to this bug.