1083258 - The size check for input <select>s on show_bug.cgi doesn't take into account is_active

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Ed Morley [:emorley]]

If the version field for a product has only one possible value, then we display the value inline, and not as a <select>.

eg: bug 746383 for the "Tracking" product, which has only one possible value "---" for version.:

    <tr>
      <th class="field_label">
        <label for="version">Version</label>:
      </th>
<td>---
  </td>
    </tr>

Compare this to the "Tree Management" product, which after bug 1083110 has multiple values for version, but only one is active - and yet we still display the <select> - eg bug 103216:

    <tr>
      <th class="field_label">
        <label for="version">Version</label>:
      </th>
<td>
      <input type="hidden" id="version_dirty">
      <select id="version" name="version">
          <option value="---" selected>---
          </option>
      </select>
  </td>
    </tr>

This is due to |bug.choices.${selname}| having not been filtered by is_active prior to checking for |.size > 1|:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/cf7ddf8474fa15955e409d28b2956c6749407f6f/template/en/default/bug/edit.html.tmpl#L1249

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1083258-v1.patch

We can first count the active choices, and display if that count is > 1.

Comment 2

[Frédéric Buclin]

Comment on attachment 8506651 [details] [diff] [review]
bug-1083258-v1.patch

>+    [% FOREACH x = bug.choices.${selname} %]
>+        [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]
>+        [% choice_count = choice_count + 1 %]
>+    [% END %]

For performance reasons, you should leave the loop once choice_count > 1.

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8506651 [details] [diff] [review]
bug-1083258-v1.patch

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

::: template/en/default/bug/edit.html.tmpl
@@ +1192,5 @@
> +    [% choice_count = 0 %]
> +    [% FOREACH x = bug.choices.${selname} %]
> +        [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]
> +        [% choice_count = choice_count + 1 %]
> +    [% END %]

Or you could do:

[% multiple_active_found = 0 %]
[% FOREACH x = bug.choices.${selname} %]
  [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]
  [% IF loop.count > 1 %]
    [% multiple_active_found = 1 %]
    [% LAST %]
  [% END %]
[% END %]

[% IF bug.check_can_change_field(selname, 0, 1) AND multiple_active_found %]
multiple_active_found = 0 %]

But I agree with LpSolit that we should break the loop early if more than one are found.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1022707-v2.patch

This is dkl's proposed change, verbatim. :-)

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1083258-v2.patch

Oy. The file selection menu over ssh is exceptionally slow, so I didn't notice that I attached the wrong patch. Glad I name them consistently.

Comment 6

[Frédéric Buclin]

Comment on attachment 8507582 [details] [diff] [review]
bug-1083258-v2.patch

>+    [% FOREACH x = bug.choices.${selname} %]
>+        [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]
>+        [% IF loop.count > 1 %]
>+            [% multiple_active_found = 1 %]

Err... what's that? So if only the last element passes, then .count will be higher than 1 and you set multiple_active_found = 1 despite there was only one element?? Did you test this patch?

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8507582 [details] [diff] [review]
bug-1083258-v2.patch

I did test this patch, and it does exactly what the code says, which is the opposite of what it is supposed to do.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1083258-v2.5.patch

The count-the-flags, bail out once you have enough. Also, a more accurate variable name.

Comment 9

[Ed Morley [:emorley]]

Why don't we just filter the variable first, then check for size, then if >1, populate the select using the filtered variable?

In the "there is only one active option" case (ie we don't want to create the select), we have to iterate through every value with the proposed patch anyway - and with the other case ("there is more than one valid option") whilst the proposed patch will break early, it then later iterates through every value to populate the select anyway...

Comment 10

[Ed Morley [:emorley]]

Also, in the else, we assume there is only one value in bug.${selname}, which with the current patch isn't true, since there can be multiple values, but just one is active. As such, it seems like we need to filter first for correctness, even ignoring performance.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8507886 [details] [diff] [review]
bug-1083258-v2.5.patch

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

::: template/en/default/bug/edit.html.tmpl
@@ +1246,5 @@
>  [% BLOCK select %]
>    <td>
> +    [% active_count = 0 %]
> +    [% FOREACH x = bug.choices.${selname} %]
> +      [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]

We are repeating this line later again in the next loop to generate the select. So let's just do it once like this:

[% select_choices = [] %]
[% FOREACH choice = bug.choices.${selname} %]
  [% NEXT IF NOT choice.is_active AND choice.name != bug.${selname} %]
  [% select_choices.push(x) %]
[% END %]
[% IF bug.check_can_change_field(selname, 0, 1) AND select_choices > 1 %]
  <input type="hidden" id="[% selname %]_dirty">
  <select id="[% selname %]" name="[% selname %]">
    [% FOREACH choice = select_choices %]
      <option value="[% x.name FILTER html %]"
        [% " selected" IF x.name == bug.${selname} %]>
            [%- x.name FILTER html %]
      </option>
    [% END %]
  </select>
[% ELSE %]
  [% bug.${selname} FILTER html %]
[% END %]

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1083258-v3.patch

How about a different approach: fixing it in the model layer rather than the template.

Comment 13

[Frédéric Buclin]

Comment on attachment 8509200 [details] [diff] [review]
bug-1083258-v3.patch

>+++ b/Bugzilla/Bug.pm

>+    foreach my $key (keys %choices) {
>+        $choices{$key} = [grep { $_->is_active } @{ $choices{$key} }];
>+    }

This won't work, because if the current field value is disabled but used in the bug, then this value will be excluded anyway.



>+++ b/template/en/default/bug/edit.html.tmpl

>-          [% NEXT IF NOT x.is_active AND x.name != bug.${selname} %]
>+          [% NEXT IF x.name != bug.${selname} %]

This logic is wrong. Now you exclude all values except the current one. No way to edit the field anymore.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

(In reply to Frédéric Buclin from comment #13)
> Comment on attachment 8509200 [details] [diff] [review]
> bug-1083258-v3.patch
> 
> >+++ b/Bugzilla/Bug.pm
> 
> >+    foreach my $key (keys %choices) {
> >+        $choices{$key} = [grep { $_->is_active } @{ $choices{$key} }];
> >+    }
> 
> This won't work, because if the current field value is disabled but used in
> the bug, then this value will be excluded anyway.
Perhaps $_->is_active || $_->name eq $self->$key ?


> This logic is wrong. Now you exclude all values except the current one. No
> way to edit the field anymore.
Gah, I made this change as after thought and didn't re-check it against multiple-value fields.
The correct solution is the removal of the NEXT entirely.

Comment 15

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #14)
> Perhaps $_->is_active || $_->name eq $self->$key ?

Something like that, yes.


> The correct solution is the removal of the NEXT entirely.

Right, with the fix above.

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1083258-v3.5.patch

My stupidity is only eclipsed my capacity to keep trying. This one works under the conditions of:

1) one select value
2) multiple select values
3) one select value and the value on the bug that is different
4) no select values(!) and a bug value that is different.

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 8509606 [details] [diff] [review]
bug-1083258-v3.5.patch

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

r=dkl

::: Bugzilla/Bug.pm
@@ +3895,5 @@
>      my @resolutions = grep($_->name, @{ $resolution_field->legal_values });
>      $choices{'resolution'} = \@resolutions;
>  
> +    foreach my $key (keys %choices) {
> +        my $name = $self->$key;

nit s/$name/$value/

Comment 18

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   e50bddb..424bbc8  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   e50bddb..d4146c5  5.0 -> 5.0

Comment 19

[Ed Morley [:emorley]]

Please may this be backported to BMO? (I currently often open the wrong select on treeherder show_bug.cgi pages, since many of them look the same, since value of "---"  etc)