281791 - Add ability to change flags in "change several bugs at once"

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P2)

Description

[Mike Miller]

User-Agent:       Opera/8.00 (Windows NT 5.1; U; en)
Build Identifier: 

I perform a search that gives me a list of bugs.  Then I select the "change 
multiple bugs at once" link, and then I would like the ability to change (i.e., 
set or clear) a flag or flags in those bugs.

Reproducible: Always

Comment 1

[Frédéric Buclin]

This would require that all bugs belong to the same product/component or that
these flag types are available for all of them. You can have specific flag types
for different products and/or components. That's IMO the reason why you cannot
access them from the "change multiple bugs at once" page.

Adding myk to the CC list in order to have his opinion. myk, should we offer the
possibility to modify flags if they are available for all bugs displayed in the
bug list? Or will this bug be WONTFIXed?

Comment 2

[Marc Schumann [:Wurblzap]]

(In reply to comment #1)
> This would require that all bugs belong to the same product/component or that
> these flag types are available for all of them.

Well, the same logic is being applied to the version field, isn't it? If all
bugs are in the same product, it's displayed, otherwise, it's hidden.

Comment 3

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

Yes, it should be possible to change flags on multiple bugs from the "change
multiple bugs" interface.

Comment 4

[Marc Schumann [:Wurblzap]]

*** Bug 298008 has been marked as a duplicate of this bug. ***

Comment 5

[Tiago Mello [:timello]]

I was writing a patch for this bug and some questions appear.
The better case for this feature is when we are only requesting flags for several bugs that are in the same product/component or have the same flags with unique names. Then it is just a matter of add those requests for each bug in the bug list. The UI will have the same look as we have when changing a single bug.

As I was talking with LpSolit and kiko in the IRC, here are some points we would need to care about:

 * The statuses and requestees of the flags can vary and we can't have a UI in common for bugs in the bug list;
 * How do you distinguish two flags having the same name?
 * Even if all bugs are in the same product/component, how do you do if some flags are not multiplicable and already set in some bugs?
 * How do you manage flags if you change their product/component?

Some comments here would be helpful.

Comment 6

[Tiago Mello [:timello]]

Well, I was thinking in solve this bug by considering the worth case, that is, only show the flag fields if all points below are covered:

 * Bugs products/componets must have the same flags with no common name between them;
 * Bugs must have common flag status and requestees;
 * If the flags are changed, block the product/component change and vice-versa (javascript to guarantee that);

Unless all these points are covered the flag field in edit several bugs at once won't appear.

Someone tell me if I'm not considering any other point.

Does that sound like a plan?

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
>  * Bugs must have common flag status and requestees;
>  * If the flags are changed, block the product/component change and vice-versa
> (javascript to guarantee that);

This seems too restrictive to me. And the JS trick is going to irritate a lot of people (how do you undo it?).

Comment 8

[Tiago Mello [:timello]]

Attachment: Allows to set multiplicable flags, but create a new flag, leaving existing ones alone.

Comment 9

[Frédéric Buclin]

Comment on attachment 283509 [details] [diff] [review]
Allows to set multiplicable flags, but create a new flag, leaving existing ones alone.

>Index: buglist.cgi

>+    foreach my $bug (@bugs) {
>+        my $prod = new Bugzilla::Product({name => $bug->{'product'}});
>+        my $comp = new Bugzilla::Component({
>+            product => $prod,
>+            name => $bug->{'component'}
>+        });

Are you sure $bug->{'component'} is always defined? AFAIK, it's not the case, unless the component column is displayed.


>+    my $flag_types = Bugzilla::FlagType::match({
>+        'product_ids'   => \@product_ids,
>+        'component_ids' => \@component_ids  
>+    });

You don't need this code, see editflagtypes.cgi line 111-131: $prod->flag_types->{'bug'} already contains the data you want.



>Index: process_bug.cgi

>+# Validate flags in all cases even when changing several
>+# bugs at once.
>+foreach my $bug_id (@idlist || [$cgi->param('id')]) {

@idlist is always defined, even when editing a single bug. Moreover, your syntax is wrong as [$foo] is an arrayref, not an array.



>Index: Bugzilla/FlagType.pm

>+    if ($criteria->{product_ids} && $criteria->{component_ids}) {

This code is useless, see my comment above.



>Index: template/en/default/global/code-error.html.tmpl

>   [% ELSIF error == "flags_not_available" %]
>     [% title = "Flag Editing not Allowed" %]
>     [% IF type == "b" %]
>-      Flags cannot be set or changed when
>-      changing several [% terms.bugs %] at once.
>+      References to existing flags when editing
>+      or creating a [% terms.bug %] are invalid.

Huh? You cannot edit flags when editing a bug??

Comment 10

[Tiago Mello [:timello]]

Attachment: Fixes.

Comment 11

[Frédéric Buclin]

For the record, here is the discussion we had on IRC on October 2:

(23:17:09) mkanat: I can't think of a use case for mass-editing multiplicable flags.
(23:17:23) mkanat: Other than clearing them all and replacing them with one value.
(23:17:41) mkanat: We could have a checkbox for multiplicable flags that says "clear all and replace with this".
(23:17:57) mkanat: Or a select box or radio buttons.
(23:18:10) mkanat: But I don't even think that's useful enough to have a UI.
(23:18:23) timello: maybe we could be restrictive LpSolit... we don't even have flags in massive change nowadays... having it for the best case won't hurt.
(23:18:55) mkanat: But the choices would be: "add this as a new flag" vs. "clear all currently set and replace with this value"
(23:19:06) LpSolit: timello: allow to set multiplicable flags, but create a new flag, leaving existing ones alone
(23:19:26) LpSolit: and we can implement mkanat's last comment in another bug
(23:19:36) mkanat: LpSolit: Yeah, let's do that.

Comment 12

[Frédéric Buclin]

Comment on attachment 284398 [details] [diff] [review]
Fixes.

>Index: buglist.cgi

>+    foreach my $bug_id (@bugidlist) {
>+        my $bug = new Bugzilla::Bug($bug_id);
>+        push @flag_types_ids,
>+            map($_->id, @{$bug->component_obj->flag_types->{'bug'}});
>+    }

Having to create one bug object then one component object for each bug in the list is heavy. You should rather add component_id to the list of mandatory columns when tweak=1 and cache component objects (loop over each component ID and store their corresponding object in a hash). This will severely decrease the load on the DB.

Also, your patch only works with multiplicable flags despite the UI shows all applicable flags, including non-multiplicable ones which are already set. I have a few comments about this:
- First, you should be able to overwrite existing non-multiplicable flags. This means some work in process_bug.cgi (eventually in Flag.pm if well coded) to go from the flag type to the already set flag for the given bug. This is not something you can code in buglist.cgi.
- Second, you shouldn't display flag types which are not editable, such as flag types you don't belong to the grant group nor the request group. You see a non-editable select menu, which is confusing.


>+    foreach my $id (@flag_types_ids) {
>+        my $occurrence = scalar grep(/$id/, @flag_types_ids);
>+        $flag_types{$id} = new Bugzilla::FlagType($id)
>+            if $occurrence == scalar @bugidlist;
>+    }

Using the cache above and another hash would make this easier, I guess.



>Index: template/en/default/global/code-error.html.tmpl

>   [% ELSIF error == "flags_not_available" %]
>     [% title = "Flag Editing not Allowed" %]
>-    [% IF type == "b" %]
>-      Flags cannot be set or changed when
>-      changing several [% terms.bugs %] at once.
>-    [% ELSE %]
>       References to existing flags when creating
>       a new attachment are invalid.
>     [% END %] 

Remove [% END %] too. Else the template doesn't compile.



>Index: template/en/default/list/edit-multiple.html.tmpl

>+    <th><label for="flags">Flags:</label></th>

Flags appear at the wrong place. They should appear besides the comment textarea, to match more closely what we have in show_bug.cgi.


>+      [% IF !flagtypes %]
>+        [% dontchange FILTER html %]

I don't think having "--do-not-change--" being displayed makes sense. If no flag type is available, show nothing, not even the headers.

I didn't try your patch while moving bugs to another product or component. Be sure to test this too.

Comment 13

[Tiago Mello [:timello]]

Attachment: Other fixes

Comment 14

[Tiago Mello [:timello]]

An interdiff: http://phpfi.com/272780

Comment 15

[Tiago Mello [:timello]]

Attachment: Some missing adjusts

Comment 16

[Frédéric Buclin]

Comment on attachment 287093 [details] [diff] [review]
Some missing adjusts

With this patch applied, I cannot edit attachment flags anymore, not delete them.
Moreover, editing several bugs at once throws:

DBD::mysql::st execute failed: Unknown column 'map_components.id' in 'field list' [for Statement "SELECT bugs.bug_id, bugs.bug_severity, bugs.priority, bugs.bug_status, bugs.resolution, bugs.creation_ts, bugs.delta_ts, map_assigned_to.login_name, map_reporter.login_name, bugs.bug_status, bugs.resolution, map_products.name, bugs.short_desc, map_components.id FROM bugs  INNER JOIN profiles AS map_assigned_to ON (bugs.assigned_to = map_assigned_to.userid) INNER JOIN profiles AS map_reporter ON (bugs.reporter = map_reporter.userid) INNER JOIN products AS map_products ON (bugs.product_id = map_products.id) LEFT JOIN bug_group_map  ON bug_group_map.bug_id = bugs.bug_id  AND bug_group_map.group_id NOT IN (16,11,6,12,18,3,1,4,13,10,2,20,8,14,5,15,7,9)  LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = 1 WHERE ((bugs.bug_id IN ('1113','1110','1'))) AND bugs.creation_ts IS NOT NULL AND ((bug_group_map.group_id IS NULL)    OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1)     OR (bugs.cclist_accessible = 1 AND cc.who IS NOT NULL)     OR (bugs.assigned_to = 1) OR (bugs.qa_contact = 1) ) GROUP BY bugs.bug_id ORDER BY bugs.creation_ts desc,bugs.delta_ts desc,bugs.bug_id"] at /var/www/html/bugzilla/buglist.cgi line 1017

Comment 17

[Frédéric Buclin]

(In reply to comment #16)
> (From update of attachment 287093 [details] [diff] [review])
> With this patch applied, I cannot edit attachment flags anymore, not delete
> them.

s/ not/ nor/

Comment 18

[Martin Pröfrock]

A *very* useful use case in several projects I am currently working on would be:

A flag with the meaning: "This bugfix is deployed on test environment <name>"

The deploying person lists all fixed bugs, gets the actual code from the version control system, deploys it to a specific test environment and marks the bugs with the flag (which means a *lot* of clicks right now)

The test persons then know they can test the bugs on this specific test environment (there are some).

For this use case, some of the restrictions mentioned above are no problem:

- The bugs all belong to the same product.
- The flag itself is a simple "yes/no" without any of the other features. Therefore, a simple "leave unchanged", "set to +", "set to -" would be enough.

Comment 19

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 20

[Frédéric Buclin]

timello: are you still working on this bug?

Comment 21

[Tiago Mello [:timello]]

(In reply to comment #20)
> timello: are you still working on this bug?

No, I'm not.

Comment 22

[Frédéric Buclin]

OK, so I take it.

Comment 24

[Simon Green]

Attachment: v1 patch

My first attempt at this. Developed against trunk, but should also go into 4.4 (IMO). Hopefully I have taken on board the suggestions made by LpSolit in the past.

  -- simon

Comment 26

[Tony Chung [:tchung]]

This also applies to "Change Columns" view, it's missing the flags.  will the patch resolve it there also?

Comment 27

[Frédéric Buclin]

(In reply to Tony Chung [:tchung] from comment #26)
> This also applies to "Change Columns" view, it's missing the flags.

No, flags are there.

Comment 28

[:glob ✱]

Comment on attachment 658714 [details] [diff] [review]
v1 patch

Review of attachment 658714 [details] [diff] [review]:
-----------------------------------------------------------------

overall this looks good.  a few minor issues to address..

::: buglist.cgi
@@ +313,5 @@
> +
> +    # We only want flags that appear in all components
> +    my %common_flag_types;
> +    foreach my $id (keys %flag_types) {
> +        my $flag_type_count = scalar grep(/^$id$/, @flag_types_ids);

use grep { $_ == $id } instead of the regex

@@ +577,4 @@
>  # Severity, priority, resolution and status are required for buglist
>  # CSS classes.
>  my @selectcolumns = ("bug_id", "bug_severity", "priority", "bug_status",
> +                     "resolution", "product", "component_id");

there's no need to select component_id in most cases, so move adding the component_id to the |if ($dotweak)| block about 30 lines down.

::: template/en/default/list/edit-multiple.html.tmpl
@@ +303,5 @@
>  %]<br>
>  
> +[% IF flag_types %]
> +<b><label for="flags">Flags:</label></b><br>
> +[% PROCESS "flag/list.html.tmpl"  

trailing whitespace

Comment 29

[Simon Green]

Attachment: bug281791-v2.patch

Made the suggested changes.

Comment 30

[:glob ✱]

Comment on attachment 8362299 [details] [diff] [review]
bug281791-v2.patch

Review of attachment 8362299 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: template/en/default/flag/list.html.tmpl
@@ +170,5 @@
> +                   name="flags_add-[% type.id %]"
> +                   id="flags_add-[% type.id %]">
> +            <a class="field_help_link"
> +               title="This flag has multiplicity. Checking this option will always create a new flag. Leaving it unchecked will update existing flag(s) and add a new flag if it does not exist">
> +               Always add?

nit: remove the after add. labels shouldn't be questions.

Comment 31

[Frédéric Buclin]

Comment on attachment 8362299 [details] [diff] [review]
bug281791-v2.patch

>=== modified file 'template/en/default/flag/list.html.tmpl'

>+      [% IF type.is_multiplicable && edit_multiple_bugs %]
>+        <td>

You cannot simply add a new cell here and there. That's inconsistent and the HTML5 validator will complain that not all rows have the same number of cells. Move <td></td> outside the IF block.


>+          <label>
>+            <input type="checkbox"

Almost everywhere else we use: <label for="foo">Foo:</label> <input id="foo" name="foo"> instead of <label><input></label>. There are only a few old templates which use the latter. The latter is a perfectly legal HTML5 construct, but for consistency, maybe it would be good to follow the former way.


>+               title="This flag has multiplicity. Checking this option will always create a new flag. Leaving it unchecked will update existing flag(s) and add a new flag if it does not exist">

This line is way too long.



>=== modified file 'template/en/default/list/edit-multiple.html.tmpl'

>+[% IF flag_types %]
>+<b><label for="flags">Flags:</label></b><br>
>+[% PROCESS "flag/list.html.tmpl"

No <b> around <label>, please. Use classes instead. I spent enough time fixing the styling of templates during the migration to HTML5. :-/
Also fix the indentation, for readability. You are inside a IF block.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Thank you glob and Simon!

Comment 34

[Simon Green]

> >+[% IF flag_types %]
> >+<b><label for="flags">Flags:</label></b><br>
> >+[% PROCESS "flag/list.html.tmpl"
> 
> No <b> around <label>, please. Use classes instead. I spent enough time
> fixing the styling of templates during the migration to HTML5. :-/
> Also fix the indentation, for readability. You are inside a IF block.

This is how the comments and groups labels are formatted. If is this required to change, a separate bug should be filed IMO.

Comment 35

[Simon Green]

Attachment: bug281791-v3.patch

Comment 36

[:glob ✱]

Comment on attachment 8473510 [details] [diff] [review]
bug281791-v3.patch

Review of attachment 8473510 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with the nits to be fixed on commit.

::: template/en/default/flag/list.html.tmpl
@@ +177,5 @@
> +                 name="flags_add-[% type.id %]"
> +                 id="flags_add-[% type.id %]">
> +          <label for="flags_add-[% type.id %]">
> +            <a class="field_help_link"
> +               title="If ticked, always create a new flag. Leaving it unchecked will update existing flag(s) and add a new flag if it does not exist">

change "ticked" to "checked".

@@ +178,5 @@
> +                 id="flags_add-[% type.id %]">
> +          <label for="flags_add-[% type.id %]">
> +            <a class="field_help_link"
> +               title="If ticked, always create a new flag. Leaving it unchecked will update existing flag(s) and add a new flag if it does not exist">
> +               Always add?

remove the "?" from the label; it doesn't make sense to ask a question here

Comment 37

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   92b5ff1..e0fbbde  master -> master

Comment 38

[Frédéric Buclin]

Added to relnotes for 5.0rc1.