309167 - change to product which has same component name results in no prompt for component and incorrect assignee

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Wayne Mery (:wsmwk)]

component 1 exists in both product A and Product B

1. change from product A to product B
2. select component 1 in component drop down
3. commit

actual results: bz committed the change
expected results: bz prompted with component list for product B

I didn't know component existed in both products and I didn't want the original
component.

example: bug 285123 has 2 commits on 2005-09-19 05:52, one at :06 and one at
:54.  second commit was needed because I didn't get prompted for component after
making the product change

Comment 1

[Wayne Mery (:wsmwk)]

incorrect assignee and QA for the new product+component is a direct result

Comment 2

[lɛʁi לערי ריינהארט]

*question*
Is this the same as
http://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=3236
== REAL BUG 309167

this bug is referenced at 
Bugzilla Bug 319803 feature request: when changing product, component etc. display old product, old component, other fields in all required steps

Comment 3

[Reed Loden [:reed]]

We really need this bug fixed, as it is causing even more problems (see bug 332905 comment #15).

Comment 4

[Elijah Newren]

Attachment: One way to fix the bug

Here's one way to fix the bug that we've been using on bugzilla.gnome.org for months.  Pretty short/trival patch.  Might be considered a little hacky -- perhaps we should just consider $vok, $cok, and $mok to always be equal (and have the value defined($cgi->param('component_verified'))) and simplify the code accordingly.

Comment 5

[Olav Vitters]

Comment on attachment 225501 [details] [diff] [review]
One way to fix the bug

>Index: process_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v
>retrieving revision 1.322
>diff -p -u -r1.322 process_bug.cgi
>--- process_bug.cgi	6 Jun 2006 22:55:29 -0000	1.322
>+++ process_bug.cgi	13 Jun 2006 23:38:09 -0000
>@@ -368,6 +368,15 @@ if (((defined $cgi->param('id') && $cgi-
>        $mok = lsearch(\@milestone_names, $cgi->param('target_milestone')) >= 0;
>     }
> 
>+    # Don't make the stupid assumption that reassigning from a component
>+    # named 'general' in one product to the 'general' component of another
>+    # product is what the user wants.  It just makes the user take multiple
>+    # steps to get bugs reassigned.  Same goes for version and milestone.
>+    # See #309167.
>+    if (!defined $cgi->param('component_verified')) {
>+        $vok = $cok = $mok = 0;

This is wrong. The dropdown fields will default to the first value if $vok etc is set to 0. Instead of doing this, make !defined $cgi->param('component_verified') part if the if statement that is just below this quoted code.

>+    }
>+
>     # If the product-specific fields need to be verified, or we need to verify
>     # whether or not to add the bugs to their new product's group, display
>     # a verification form.

Comment 6

[Olav Vitters]

Attachment: Patch v2

Basically doing what I suggested in my earlier review. Stealing bug as well as I know Elijah is busy thinking about a certain operator being close to an orthogonal projector onto the space of G-free vector fields in the B_1 induced norm space and junk like that.

I sneaked in a small UI change; add a size=10 to the select fields. As the fields need to be verified, it is far easier to have them already dropped down.

Comment 7

[Colin Ogilvie [:cso]]

Comment on attachment 235602 [details] [diff] [review]
Patch v2

>+    # We cannot be sure if the component is the same by only checking $cok; the
>+    # the current component name could exists in the new product. So always

Duplicate the.

Comment 8

[Frédéric Buclin]

Comment on attachment 235602 [details] [diff] [review]
Patch v2

>Index: process_bug.cgi

>+    if (!$vok || !$cok || !$mok || !defined $cgi->param('component_verified')
>+        || (AnyDefaultGroups() && !defined $cgi->param('addtonewgroup'))) {
>         
>+        if (!$vok || !$cok || !$mok
>+            || !defined $cgi->param('component_verified'))


>Index: template/en/default/bug/process/verify-new-product.html.tmpl
> [% PROCESS "global/hidden-fields.html.tmpl"
>+     exclude=(verify_fields ? "^version|component|target_milestone|component_verified$" : "") %]
> 
>+<input type="hidden" name="component_verified" value="[% verify_fields ? 1 : 0 %]">

I don't understand what you are trying to do here. In process_bug.cgi, you look whether component_verified is defined or not, not whether its value is 0 or 1. Also, I don't see why you are excluding it, per my previous comment.

Also, I'm not sure "component_verified" is the right name for this variable as I could as well see it as version_verified or milestone_verified. Maybe "confirm_product_change"?

Comment 9

[Olav Vitters]

(In reply to comment #8)
> (From update of attachment 235602 [details] [diff] [review] [edit])
> >Index: process_bug.cgi
> 
> >+    if (!$vok || !$cok || !$mok || !defined $cgi->param('component_verified')
> >+        || (AnyDefaultGroups() && !defined $cgi->param('addtonewgroup'))) {
> >         
> >+        if (!$vok || !$cok || !$mok
> >+            || !defined $cgi->param('component_verified'))
> 
> 
> >Index: template/en/default/bug/process/verify-new-product.html.tmpl
> > [% PROCESS "global/hidden-fields.html.tmpl"
> >+     exclude=(verify_fields ? "^version|component|target_milestone|component_verified$" : "") %]
> > 
> >+<input type="hidden" name="component_verified" value="[% verify_fields ? 1 : 0 %]">
> 
> I don't understand what you are trying to do here. In process_bug.cgi, you look
> whether component_verified is defined or not, not whether its value is 0 or 1.

Yeah, that is broken. It should always be 1.

> Also, I don't see why you are excluding it, per my previous comment.

I was trying to protect against clients where you could possibly deselect a radio button (the verify_bug_group). In that case the form would be shown again and will end up with two 'component_verified' hidden input fields.

That is also why I did the weird 0 / 1 stuff, but that was not correct/broken.

> Also, I'm not sure "component_verified" is the right name for this variable as
> I could as well see it as version_verified or milestone_verified. Maybe
> "confirm_product_change"?

That is better, yes.

Comment 10

[Frédéric Buclin]

Comment on attachment 235602 [details] [diff] [review]
Patch v2

r- per the previous comment

Comment 11

[Olav Vitters]

Attachment: Patch v3

Changes:
 - no more duplicate 'the' (slightly changed comment as well)
 - s/component_verified/confirm_product_change/g
 - hidden confirm_product_change field is always set to 1
 - does not exclude confirm_product_change anymore

Comment 12

[Olav Vitters]

Attachment: Patch v3.1

Fix bitrot due to bug 353994.

Comment 13

[Frédéric Buclin]

Comment on attachment 242550 [details] [diff] [review]
Patch v3.1

>Index: template/en/default/bug/process/verify-new-product.html.tmpl

>+        [% PROCESS "global/select-menu.html.tmpl" name="version" options=versions default=defaults.version size=10 %]
>+        [% PROCESS "global/select-menu.html.tmpl" name="component" options=components default=defaults.component size=10 %]
>+        [% PROCESS "global/select-menu.html.tmpl" name="target_milestone" options=milestones default=defaults.target_milestone size=10 %]

Nit: on checkin, please split these long lines.



>Index: template/en/default/global/select-menu.html.tmpl

>+        [% IF multiple %] multiple [% END %][% IF size %] size="[% size %]" [% END %]>

Nit: this should be split on two distinct lines now that 'multiplie' and 'size' are treated independently.


Your patch is working fine. r=LpSolit

Comment 14

[Olav Vitters]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.346; previous revision: 1.345
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v  <--  verify-new-product.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/global/select-menu.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/select-menu.html.tmpl,v  <--  select-menu.html.tmpl
new revision: 1.5; previous revision: 1.4
done

Comment 15

[Frédéric Buclin]

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