66235 - process_bug.cgi: multiple product change misses the groupset bit

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Troy Engel]

When mass editing multiple bugs, the process_bug.cgi script changes the product
name (ane components) from the old to the new, but it fails to set the
'groupset' bit to the new product's group bit.

There are several places to fix it, I added the following right around line
359/360 (from v2.10, inside the "foreach my $field..." loop):

if (defined $::FORM{'product'}) {
  my $name = trim($::FORM{'product'});
  if (($name ne $::dontchange) && ($name ne "")) {
    ConnectToDatabase();
    SendSQL("select bit from groups where name = '$name'");
    my $bit = FetchOneColumn();
    if (defined $bit) {
      DoComma();
      $::query .= "groupset = $bit";
    }
  }
}

Comment 1

[Christian Reis]

I'm willing to take this one if someone tells me kindly if troy's approach is
the better one (special-casing product inside the loops) or if I should break
the loop in a separate way. If nobody complains I'll follow Troy's suggestion.

Comment 2

[Christian Reis]

Adding blocker bug till group fixes have been decided.

Comment 3

[Christian Reis]

Attachment: proposed fix (will probably break when blocker is closed)

Comment 4

[Christian Reis]

I've added an attachment with the change. Troy, do you see anywhere else this
has to be fixed? Apparently this is the only place.

Comment 5

[Troy Engel]

No other places that I know of need it, this is the only one.

Comment 6

[Matthew Tuck [:CodeMachine]]

Not sure how all this works, but does the old product flag drop off, and should
it?

Comment 7

[Dave Miller [:justdave]]

This patch does no checking to find out whether the old bit was set or not, and 
it does nothing to preserve non-product group bits.

I wouldn't bother fixing it though, since, as noted, it's going to break when bug 
68022 is resolved.

Comment 8

[Dave Miller [:justdave]]

I believe this is fixed by the patch on bug 84714, or at least sufficiently so 
that the user has the power to do something about it...  (bug 84714's patch 
enables changing groups on mass-changes)

Comment 9

[Dave Miller [:justdave]]

hmm, no this won't be fixed by the patch on bug 84714...

what needs to happen is on a product change, check to see if the product group
bit is set, and if so, clear it, and set the product group bit for the product
the bug is moving to, if the new product has a group.  What gets into a gray
area is what do we do if the origin product doesn't have a group and the
destination product does?

Comment 10

[Troy Engel]

Well, the initial intent (by me, the user) was to move mass amounts of bugs from
one group to the next (e.g., into an "Archives" group for old stuff).  At this
point, I'm not really sure it matters if the origin group was empty or not, as
the destination is supposed to be set to the group I choose.

Perhaps I'm missing something (most likely), but it seems to be a
straightforward move from A -> B, regardless of A ?

Comment 11

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

This is another of those 2.14 milestone bugs that were depending on 68022, which
has jumped to the 2.16 milestone.  This bug needs to either be decoupled from
that bug (and thus fixed in this milestone) or moved forward.  If it stays in
this milestone, Christian are you still willing and able to work on it or do you
want someone else to take it over?

Comment 12

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

>What gets into a gray
>area is what do we do if the origin product doesn't have a group and the
>destination product does?


When a new bug is created, it gets placed into its product's group by default. 
I imagine, then, that if a bug is moved from one product to another, it will be
placed into the new product's group by default as well.  The only exceptions to
this are if the user doesn't have access to the new product's group or if they
explicitly request that the bug not be moved.

Comment 13

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

I'm working on a fix for this. 

-> over to myself.

Comment 14

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

Attachment: patch to fix v2

Comment 15

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

Attachment: patch v3 (adds removal of bugs from old product groups)

Comment 16

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

hmm, my solution has some problems.  working on a new one.

Comment 17

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

Attachment: patch v4 (works right)

Comment 18

[Jacob Steenhagen]

Multiline if() statements complete with comments... I've never seen that 
before.  Is that why the comments inside the if() end w/a semi-colon?

The print statements added to process_bug.cgi seem more like debug than user 
explainitory, are the supposed to be removed before the final patch?

I would think that GroupNameToBit() should be able to also tell if the group 
exists (return 0 if it doesn't).  Then you wouldn't have to call both 
GroupExists() and GroupNameToBit() [which would result in two queries instead 
of one].

This patch doesn't seem to interact terribly well with with the patch from bug 
39816 (a mass change clears the additional viewing options [although that's a 
bug in the other patch, not this one, as I think about it]).

The interface on the buglist page seems a bit confusing.  Perhaps it should all 
be done from the table. I'm not sure why we would allow a bug to be added to a 
product group for a product that it isn't in. But again, that's not this bug, 
that another one (already marked FIXED, I believe).

I would also recommend that in subs such as GroupNameToBit() you do a 
PushGlobalSqlState() and PopGlobalSqlState() to avoid sometimes mysterious 
dataloss type bugs.

Comment 19

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

Attachment: patch v5: uses "verify" screen for group verification

Comment 20

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

Attachment: patch v6: same as v5 but without option to keep bug in old product group

Comment 21

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

>Multiline if() statements complete with comments... I've never seen that 
>before.  Is that why the comments inside the if() end w/a semi-colon?

Nope.  The semi-colons are strictly semantic.

>The print statements added to process_bug.cgi seem more like debug than user 
>explainitory, are the supposed to be removed before the final patch?

Yep.  They are just debugging statements that I thought would make the patch's
behavior easier to review.

>I would think that GroupNameToBit() should be able to also tell if the group 
>exists (return 0 if it doesn't).  Then you wouldn't have to call both 
>GroupExists() and GroupNameToBit() [which would result in two queries instead 
>of one].

Done.

>This patch doesn't seem to interact terribly well with with the patch from bug 
>39816 (a mass change clears the additional viewing options [although that's a 
>bug in the other patch, not this one, as I think about it]).

I'm looking into that next.

>The interface on the buglist page seems a bit confusing.  Perhaps it should all 
>be done from the table. I'm not sure why we would allow a bug to be added to a 
>product group for a product that it isn't in. But again, that's not this bug, 
>that another one (already marked FIXED, I believe).

The latest version of the patch (and the one that is running on landfill) does
not allow the user to keep the bug in the old product group.

I moved the UI for this patch into the "verify" page that appears when you
change products (currently with options for verifying the version, component,
and target milestone).  This way it appears for both single and multiple bug
changes and is tied to the specific act of changing products.

I thought this made more sense than putting it on one of the other pages, where
it would take up space and attract attention even though it is irrelevant except
during a product change.

I cleaned up the appearance and code for the "verify" screen in the process.

>I would also recommend that in subs such as GroupNameToBit() you do a 
>PushGlobalSqlState() and PopGlobalSqlState() to avoid sometimes mysterious 
>dataloss type bugs.

I added this to the two functions in this patch.  I can see a few other
functions that could use it, but I'll file a separate bug on those since I don't
want to change anything out of the scope of this patch and possibly introduce
more issues.

Comment 22

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

Attachment: patch v7: fixes the "double-add" problem

Comment 23

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

The latest patch fixes the problem Jake discovered where you can add the bug to
the group twice (which results in the bug being added to the group with the next
higher groupset bit) by saying "add to group" on both the "change multiple bugs"
and the "confirm product change" forms.  The bug only gets added to the group
once if you do this now.

Comment 24

[Dave Miller [:justdave]]

Zach checked this in and forgot to mark the bug fixed.  r=Jake in irc.

Comment 25

[Zach Lipton [:zach]]

I didn't forget, just had to go out for a while :)

Comment 26

[Dave Miller [:justdave]]

Moving to Bugzilla product