288663 - Flag inclusion wrong when product has component called "Any"

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Rick Wright]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: 

If a Product has multiple components, and one of those components is "Any", then
the flag inclusion will get reset from Product:__Any__ to Product:Any when a
flag is edited.

Reproducible: Always

Steps to Reproduce:
1. Create a product in Bugzilla with multiple components.
2. Add a component called "Any"
3. Create a new flag type, remove __Any__:__Any__ from the inclusion list
4. Add Product:__Any__ to the inclusion list
5. Click on "Create"
6. At this point, if you look at all bugs assigned to "Product" you will see the
flag.
7. Return to the edit flag types page.
8. Edit the flag you just created
9. Note that the inclusions list now shows Product:Any instead of Product:__Any__
10. Click on "Save Changes"
11. Now, note that the new flag will only show up on bugs that are assigned to
the "Any" component of Product.

Actual Results:  
Flags get removed from all components except for the "Any" component

Expected Results:  
Flags should still be available on all components of Product

The workaround is to fix the flaginclusions table directly in the database.

Comment 1

[Rick Wright]

Attachment: Patch against Bugzilla tip

This patch appears to solve the problem.

Comment 2

[Frédéric Buclin]

I can reproduce on both 2.19.2+ and 2.18+.

Your patch is not the correct one. What happens if you create a product or a
component with the name "__Any__"? I think you get the same behavior. Actually,
I tried (without your patch applied) to create a component with the name
"__Any__". Now everytime __Any__ appears, Buzilla thinks I talk about the
component "__Any__" even if I mean "all components".

The right fix is to use the product/component ID instead of their name. For
example, using my local installation:

<option value="dead_prod:__Any__">dead_prod:__Any__

should be:

<option value="30:22">dead_prod:__Any__

where 30 is the ID of my product "dead_prod" and 22 the one of my component
"__Any__". As IDs cannot be 0, using "0" for "__Any__" (meaning "all" either as
product or component) would avoid any confusion.


myk, I will submit a patch using my suggestion above (using IDs). Have a better
idea?

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

I'm not completely satisfied with this solution, mainly because of the
duplication of the code for the conversion of the array
@clusions('prod_ID:comp_ID') back to a hash of the form
%clusions{'prod_name:comp_name'} = 'prod_ID:comp_ID', and also because I can't
avoid all these tests in validateAndSubmit().

Consider this as a first attempt...

Comment 4

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

Comment on attachment 180995 [details] [diff] [review]
patch, v1

This seems to work, and the code looks OK at first glance, but I'll wait to do
a complete review until you're satisfied with the code. :-)

Comment 5

[Frédéric Buclin]

Comment on attachment 180995 [details] [diff] [review]
patch, v1

OK, I definitely have no better idea. Using something like:

foreach my $table ("inclusions", "exclusions") {
    foreach my $ids (@$table) {

is not accepted due to restrictions applied:

Can't use string ("inclusions") as an ARRAY ref while "strict refs" in use

... unfortunately. :(

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

Now I'm satisfied with this solution. :)

Comment 7

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

Comment on attachment 181754 [details] [diff] [review]
patch, v2

>+    else {
>+        my %inclusions;
>+        $inclusions{"__Any__:__Any__"} = "0:0";

Nit: simpler as:

my %inclusions = ("__Any__:__Any__" => "0:0");

But it seems like the original would be simpler still, given that we don't use
%inclusions elsewhere.

Otherwise this looks good. r=myk

Comment 8

[Frédéric Buclin]

Attachment: backport to 2.18, v1

Same patch as the previous one. Tested, works!

Comment 9

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

Comment on attachment 182028 [details] [diff] [review]
backport to 2.18, v1

Looks good. r=myk

Comment 10

[Frédéric Buclin]

Tip:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.19; previous revision: 1.18
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.16; previous revision: 1.15
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.11; previous revision: 1.10
done


2.18:

Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.7.2.5; previous revision: 1.7.2.4
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.7.2.1; previous revision: 1.7
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.6.2.4; previous revision: 1.6.2.3
done