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

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Creating/Changing Bugs
P2
enhancement
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Mike Miller, Assigned: Simon Green)

Tracking

(Blocks: 1 bug)

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

Details

(Whiteboard: [wanted-bmo])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

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

13 years ago
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?
(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.
Yes, it should be possible to change flags on multiple bugs from the "change
multiple bugs" interface.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 298008 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Flags: approval2.22?
Flags: approval2.22?

Updated

10 years ago
Assignee: create-and-change → 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.
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

10 years ago
(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?).
Created attachment 283509 [details] [diff] [review]
Allows to set multiplicable flags, but create a new flag, leaving existing ones alone.
Attachment #283509 - Flags: review?(LpSolit)

Updated

10 years ago
Status: NEW → ASSIGNED
Whiteboard: [wanted-bmo]

Comment 9

10 years ago
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??
Attachment #283509 - Flags: review?(LpSolit) → review-
Created attachment 284398 [details] [diff] [review]
Fixes.
Attachment #283509 - Attachment is obsolete: true
Attachment #284398 - Flags: review?(LpSolit)

Comment 11

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

10 years ago
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.
Attachment #284398 - Flags: review?(LpSolit) → review-
Blocks: 401007
Created attachment 286778 [details] [diff] [review]
Other fixes
Attachment #284398 - Attachment is obsolete: true
Attachment #286778 - Flags: review?(LpSolit)
An interdiff: http://phpfi.com/272780
Created attachment 287093 [details] [diff] [review]
Some missing adjusts
Attachment #286778 - Attachment is obsolete: true
Attachment #287093 - Flags: review?(LpSolit)
Attachment #286778 - Flags: review?(LpSolit)

Updated

10 years ago
Priority: -- → P2
Target Milestone: --- → Bugzilla 3.2

Comment 16

10 years ago
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
Attachment #287093 - Flags: review?(LpSolit) → review-

Comment 17

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

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

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

Updated

10 years ago
Blocks: 312751

Comment 20

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

No, I'm not.
Status: ASSIGNED → NEW

Comment 22

7 years ago
OK, so I take it.
Assignee: timello → LpSolit
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 5.0 → Bugzilla 4.2

Updated

7 years ago
Assignee: LpSolit → create-and-change

Updated

7 years ago
Target Milestone: Bugzilla 4.2 → ---

Updated

7 years ago
Status: ASSIGNED → NEW

Updated

5 years ago
Duplicate of this bug: 783926
(Assignee)

Updated

5 years ago
Assignee: create-and-change → sgreen+mozilla
(Assignee)

Comment 24

5 years ago
Created attachment 658714 [details] [diff] [review]
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
Attachment #287093 - Attachment is obsolete: true
Attachment #658714 - Flags: review?(glob)

Updated

5 years ago
Target Milestone: --- → Bugzilla 5.0

Updated

5 years ago
Duplicate of this bug: 879868
This also applies to "Change Columns" view, it's missing the flags.  will the patch resolve it there also?

Comment 27

5 years ago
(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 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
Attachment #658714 - Flags: review?(glob) → review-
(Assignee)

Comment 29

4 years ago
Created attachment 8362299 [details] [diff] [review]
bug281791-v2.patch

Made the suggested changes.
Attachment #658714 - Attachment is obsolete: true
Attachment #8362299 - Flags: review?(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.
Attachment #8362299 - Flags: review?(glob) → review+
Flags: approval?

Comment 31

4 years ago
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.
Attachment #8362299 - Flags: review-

Updated

4 years ago
Flags: approval?
Keywords: relnote
(Assignee)

Updated

4 years ago
Assignee: sgreen → create-and-change
(Assignee)

Updated

4 years ago
Attachment #8362299 - Attachment is obsolete: true
Duplicate of this bug: 1053512
(Assignee)

Updated

3 years ago
Assignee: create-and-change → sgreen
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Thank you glob and Simon!
(Assignee)

Comment 34

3 years ago
> >+[% 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.
(Assignee)

Comment 35

3 years ago
Created attachment 8473510 [details] [diff] [review]
bug281791-v3.patch
Attachment #8473510 - Flags: review?(glob)
(Assignee)

Updated

3 years ago
Summary: Add ability to change flags in "change multiple bugs at once" → Add ability to change flags in "change several bugs at once"
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
Attachment #8473510 - Flags: review?(glob) → review+
Flags: approval?
(Assignee)

Comment 37

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   92b5ff1..e0fbbde  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: approval? → approval+
Resolution: --- → FIXED

Updated

3 years ago
Blocks: 1061389

Updated

3 years ago
Blocks: 1065444

Updated

3 years ago
Blocks: 1071033

Updated

3 years ago
Blocks: 1103019

Comment 38

3 years ago
Added to relnotes for 5.0rc1.
Keywords: relnote
Blocks: 1182782
You need to log in before you can comment on or make changes to this bug.