39120 - Can't mass change product specific field if bug list includes bugs across products.

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P3)

Description

[selmer (gone)]

I've lost more time to this than any other single thing in bugzilla, even poor 
performance.  Having more than one product in a query result and then choosing 
change several bugs at once results in a page that can't change milestones.  
Unfortunately, if you forget to check in advance, you spend another half hour 
marking bugs only to find it won't let you change the @#$! things.

Please make one of these fixes:
1.  Allow the superset of milestones in the drop-down, but only apply the ones 
that are legal.  On the summary page simply note the ones that failed or better 
yet take me to a page where I can operate on just those bugs.
2.  Allow the strict subset of proper milestone values in the drop-down.  In 
this way, only legal operations are allowed and the bogus over-restriction we 
have today would go away.  I'll still lose on some bugs, but won't have wasted 
*all* my time.  As with prior solution, can still go to a page where the others 
can be corrected easily.
3.  Pick the product with the most hits and show only its values, don't give the 
other product's bugs checkboxes.
4.  Give some visual warning that there is a problem and make it obvious without 
having to scroll or take any other action.  This is the one good application of 
the <blink> tag :-)  I'd even be happy if it just failed my query and made me go 
do what I meant, but then you'd have to invent some override for when I *don't* 
mean that...

Personally, I like these options roughly in the order given.  Option 3 is 
probably the optimum solution based on cost/benefit.

Thanks for listening to my rant, I get cranky when tools encourage me to waste 
time.

Comment 1

[Matthew Tuck [:CodeMachine]]

Allow me to deprecate these solutions and prevent what I believe is "the right
thing".

My understanding is that we currently have a file somewhere defining the
milestones for each product.  Therefore, we have something like this:

Browser

M1, M2, etc.

MailNews

M1, M2, etc.

Component-X

M1, M2, etc.

Component-Y

T1, T2, etc.

How about if we change the structure of the file to:

Browser, MailNews

M1, M2, etc.

Component-X

M1, M2, etc.

Component-Y

T1, T2, etc.

Then if all of your bugs are in Browser and MailNews, a bulk change can still
handle milestones, since you're declared the lists are identical.  Notice how I
didn't merge Component-X even though it has the same milestone list?  This
allows you to specify that even though they may be the same milestone list, they
correspond to different dates/criteria.  So you can choose whether you want to
merge them.

I believe this should resolve your problem?

Comment 2

[selmer (gone)]

If it truly solves the problem I can't complain :-)  I've been told that merging
the product definitions was too hard for the short-term, but what you're
suggesting might be less work.  I hope the simplification doesn't ignore cases
other people care about.  It seems your proposal wouldn't allow incongruent
products to coexist and I could _still_ get unusable "change many" pages.
Perhaps in combination with the visual warning this would suffice.

Comment 3

[Chris Yeh]

reassigning to me, think this is related to a schema change

Comment 4

[Jesse Ruderman]

How about a warning at the top of the page, in red, that says:

You're changing bugs that include multiple products, so you won't be able to 
change the target milestone or version fields.

Would that solve the problem?

Comment 5

[selmer (gone)]

That would reduce my frustration, but not solve the problem.  It's definitely
better than option 5: do nothing.

Is option 3 too hard?  That solution allows me to get some utility from waiting
for the query to come back.

Comment 6

[Matthew Tuck [:CodeMachine]]

OK.  My current opinion is that we should display the superset in the field and
fail to make any changes if:

- if we don't have a single product, ie we've selected across multiple products
and we haven't reassigned to a new product.
- if we have a single product, but the milestone is not applicable to the single
product.

The stuff I said above milestone groups earlier is now filed as bug #69262, and
would help even further since even with the above you would have to make the
change both to Browser & MailNews.  I am very much against allowing it otherwise
since we have no guarantee M1 in Browser in the same as M1 in MailNews, except
for '---' and 'Future'.

It should do exactly the same thing for component (bug #37396) and any other
values-per-product field.

Comment 7

[Matthew Tuck [:CodeMachine]]

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

Comment 8

[Stephan Niemz]

moving to real milestones...

Comment 9

[Matthew Tuck [:CodeMachine]]

Consider this bug to cover component, version and milestone since the solution
is exactly the same and they should all be fixed at the same time.

Comment 10

[Dave Miller [:justdave]]

Taking all of cyeh's Bugzilla bugs...

Comment 11

[Dave Miller [:justdave]]

Taking all of cyeh's Bugzilla bugs.

Comment 12

[Matthew Tuck [:CodeMachine]]

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

Comment 13

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

Moving to the new "Bugzilla" product and cc:ing myself.

Comment 14

[Gervase Markham [:gerv]]

RFE with no patch; -> 2.18.

Gerv

Comment 15

[Joel Peshkin]

Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.

Comment 16

[Max Kanat-Alexander]

Is this still an issue? I haven't seen it happen on my local 2.17.1.

Comment 17

[Dave Miller [:justdave]]

(In reply to comment #16)
> Is this still an issue? I haven't seen it happen on my local 2.17.1.

That's the point.  You can't do it. :)  The reporter is asking to be able to.

We solved this problem for Zippy by placing all of the product-specific fields
in a separate box from all of the general fields in the form, and if there was
more than one product chosen, then the product-specific box contained a sentence
stating that product-specific fields can only be changed when the results
contain only bugs from a single product, and a dropdown box of the products that
were found in the list with a button to redisplay the list with only bugs from
that product.

Comment 20

[Simon Green]

Attachment: v1 patch

13½ years since this bug was submitted. About time we fix it, eh? :)

Comment 21

[Frédéric Buclin]

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

>+            foreach my $field ('components', 'versions', 'targetmilestones') {
>+                my $list = $field eq 'targetmilestones'
>+                    ? $product->milestones

Would be much better to rename targetmilestones to milestones in list/edit-multiple.html.tmpl and remove this hack.


>+                        if (not exists $values{$field}{$item->name}) {
>+                            $values{$field}{$item->name} = 0;
>+                        }

Perl automatically handles this for you. You don't need to set a default value before incrementing it, and no warning is thrown in the error log. So these 3 lines can go away, making the code easier to read.


>+            my @keys = $field eq 'version'
>+                ? sort { vers_cmp(lc($a), lc($b)) } keys %{$values{$field}}
>+                : sort { lc($a) cmp lc($b) } keys %{$values{$field}};

Is it intentional to ignore the sortkey of target milestones? I know it can be different across products.

Comment 22

[Simon Green]

(In reply to Frédéric Buclin from comment #21)
> Would be much better to rename targetmilestones to milestones in
> list/edit-multiple.html.tmpl and remove this hack.

Good thinking. This has been changed.

> >+            my @keys = $field eq 'version'
> >+                ? sort { vers_cmp(lc($a), lc($b)) } keys %{$values{$field}}
> >+                : sort { lc($a) cmp lc($b) } keys %{$values{$field}};
> 
> Is it intentional to ignore the sortkey of target milestones? I know it can
> be different across products.

Yes, this uses the same sorting code that the 'Advanced Search' page uses. Because of the reasons stated above, the sortkey is ignored.

Comment 23

[Simon Green]

Attachment: bug39120-v2.patch

Comment 25

[Frédéric Buclin]

Comment on attachment 8347819 [details] [diff] [review]
bug39120-v2.patch

>=== modified file 'buglist.cgi'

>+                    if ($item->is_active) {
>+                        ++$values{$field}{$item->name};
>+                    }

Nit: these 3 lines could be written on a single line:

    $values{$field}{$item->name}++ if $item->is_active;


>+            my @keys = $field eq 'version'
>+                ? sort { vers_cmp(lc($a), lc($b)) } keys %{$values{$field}}
>+                : sort { lc($a) cmp lc($b) } keys %{$values{$field}};
>+            foreach my $value (@keys) {
>+                if ($values{$field}{$value} == $product_count) {
>+                    push @{$vars->{$field}}, $value;
>+                }
>+            }

You should filter values before sorting them, as the unfiltered list can be quite long and there is no need to sort values which we are going to exclude later, i.e. execute:

    my @keys = grep { $values{$field}{$_} == $product_count } keys %{$values{$field}};

and then you sort @keys if there is anything left to sort.



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

If there are excluded values, you should display a message right above the form (below <div class="bz_info"></div>) explaining that only values available for all products are displayed, else a user may not understand why not all values are displayed.

Comment 26

[Simon Green]

Attachment: bug39120-v3.patch

Now with the changes LpSolit suggested.

Comment 27

[Simon Green]

Attachment: bug39120-v3.patch

Now without trailing whitespace on new lines.

Comment 28

[Frédéric Buclin]

Comment on attachment 8422211 [details] [diff] [review]
bug39120-v3.patch

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

>+[% IF excluded_values %]
>+<p><b>Only values that are available for all products of the above
>+[%+ terms.bugs %] are shown.</b></p>
>+[% END %]

Wrong indentation inside the IF block. Also, using class="extra_info" for this text would be better than the hardcoded <b></b>.

Comment 29

[Simon Green]

Attachment: bug39120-v4.patch

Made the changes suggested by Frédéric.

Comment 30

[:glob ✱]

Comment on attachment 8422795 [details] [diff] [review]
bug39120-v4.patch

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

r=glob
nice.

::: buglist.cgi
@@ +993,4 @@
>          $vars->{'versions'} = [map($_->name, grep($_->is_active, @{ $one_product->versions }))];
>          $vars->{'components'} = [map($_->name, grep($_->is_active, @{ $one_product->components }))];
>          if (Bugzilla->params->{'usetargetmilestone'}) {
> +            $vars->{'milestones'} = [map($_->name, grep($_->is_active,  

remove trailing whitespace.
please fix on commit

Comment 31

[Simon Green]

Unfortunately this got committed along with bug 1008764. Naughty me.

Comment 32

[Ben Bucksch (:BenB)]

Simon, always a pleasure to see old, long-standing bugs getting fixed. And the corresponding ticket getting found and marked FIXED. Thank you!

Comment 33

[Simon Green]

(In reply to Ben Bucksch (:BenB) from comment #32)
> Simon, always a pleasure to see old, long-standing bugs getting fixed.

Thanks.

> And the corresponding ticket getting found and marked FIXED. Thank you!

For clarification, this bug is unrelated to bug 1008764. Due to some mindlessness on my part, I accidentally committed both bugs at the same time.

I not ashamed to admit that once in a while, I do stuff up :)

  -- simon