process_bug.cgi: multiple product change misses the groupset bit

RESOLVED FIXED in Bugzilla 2.14

Status

()

Bugzilla
Bugzilla-General
P1
critical
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Troy Engel, Assigned: myk)

Tracking

unspecified
Bugzilla 2.14

Details

(URL)

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
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";
    }
  }
}
Target Milestone: --- → Bugzilla 2.14
(Assignee)

Updated

16 years ago
Whiteboard: code

Comment 1

16 years ago
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

16 years ago
Adding blocker bug till group fixes have been decided.
Depends on: 68022

Comment 3

16 years ago
Created attachment 38695 [details] [diff] [review]
proposed fix (will probably break when blocker is closed)

Comment 4

16 years ago
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.
(Reporter)

Comment 5

16 years ago
No other places that I know of need it, this is the only one.
Not sure how all this works, but does the old product flag drop off, and should
it?

Updated

16 years ago
Priority: -- → P1
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.
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)
Depends on: 84714
No longer depends on: 68022
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?
(Reporter)

Comment 10

16 years ago
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 ?
(Assignee)

Comment 11

16 years ago
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?
(Assignee)

Comment 12

16 years ago
>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.
(Assignee)

Comment 13

16 years ago
I'm working on a fix for this. 

-> over to myself.
Assignee: tara → myk
(Assignee)

Comment 14

16 years ago
Created attachment 43710 [details] [diff] [review]
patch to fix v2
(Assignee)

Comment 15

16 years ago
Created attachment 43715 [details] [diff] [review]
patch v3 (adds removal of bugs from old product groups)
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: patch, review
Whiteboard: code
(Assignee)

Comment 16

16 years ago
hmm, my solution has some problems.  working on a new one.
(Assignee)

Comment 17

16 years ago
Created attachment 43843 [details] [diff] [review]
patch v4 (works right)
(Assignee)

Updated

16 years ago

Comment 18

16 years ago
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.
(Assignee)

Comment 19

16 years ago
Created attachment 44302 [details] [diff] [review]
patch v5: uses "verify" screen for group verification
(Assignee)

Comment 20

16 years ago
Created attachment 44303 [details] [diff] [review]
patch v6: same as v5 but without option to keep bug in old product group
(Assignee)

Comment 21

16 years ago
>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.
(Assignee)

Comment 22

16 years ago
Created attachment 44823 [details] [diff] [review]
patch v7: fixes the "double-add" problem
(Assignee)

Comment 23

16 years ago
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.
Zach checked this in and forgot to mark the bug fixed.  r=Jake in irc.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 25

16 years ago
I didn't forget, just had to go out for a while :)
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.