30731 - status not kept in mass reassignment of resolved bugs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[David Baron :dbaron:]

I'm filing this since I'm under the impression that Bugzilla is no longer
designed to handle open bugs with a resolution.

It seems that mass reassignment of a bug that is RESOLVED-LATER can cause the
bug to change to NEW without clearing the resolution.  See diffs below.

(BTW, it would be nice to be able to reassign LATER/REMIND bugs without
reopening them.)



http://bugzilla.mozilla.org/show_bug.cgi?id=10713

*** shadow/10713        Thu Jan 20 14:41:41 2000
--- shadow/10713.tmp.3292       Mon Mar  6 11:50:25 2000
***************
*** 3,14 ****
  Version: other
  Platform: All
  OS/Version: All
! Status: RESOLVED   
  Resolution: LATER
  Severity: normal
  Priority: P5
  Component: Layout
! AssignedTo: [e-mail]                            
  ReportedBy: [e-mail]               
  QAContact: [e-mail]
  TargetMilestone: M15
--- 3,14 ----
  Version: other
  Platform: All
  OS/Version: All
! Status: NEW   
  Resolution: LATER
  Severity: normal
  Priority: P5
  Component: Layout
! AssignedTo: [e-mail]                            
  ReportedBy: [e-mail]               
  QAContact: [e-mail]
  TargetMilestone: M15
***************
*** 42,44 ****
--- 42,47 ----
  Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and
{css-moz}
  radars should now be considered deprecated in favour of keywords.
  I am *really* sorry about the spam...
+ 
+ ------- Additional Comments From [e-mail]  2000-03-06 11:50
-------+ mine now

Comment 1

[Matthew Tuck [:CodeMachine]]

Terry does the underlying MySQL bug table have row restrictions?  If so you
might want to add this in case there are any more situations you missed.

Comment 2

[Terry Weissman]

MySQL doesn't have a concept of "row restrictions".

Comment 3

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 4

[Tara Hernandez]

Huh.  Okay, well, changing owner should definitely clear the resolution,
however, the second point doesn't really fly with me.  Why bother reassinging a
bug if it's going to retain the later status?  Especially as the dafault query
is New|Assigned|Reopened, so if you reassigned a bug, but it retained it's
Resolved/Later state, it's entirely possible that the new person would never see
it.  If that's the case, just close the bug out as wontfix and save some effort.

Comment 5

[Matthew Tuck [:CodeMachine]]

Reassigning would generate a notification.

I assume people who use REMIND/LATER have queries and periodically check for
them, so it shouldn't drop away.

Of course, REMIND and LATER should be nuked from orbit, it's the only way to be
sure.

Comment 6

[Tara Hernandez]

go don

Comment 7

[Dave Miller [:justdave]]

moving to real milestones...

Comment 8

[Jacob Steenhagen]

-> New Bugzilla Product

Comment 9

[Dave Miller [:justdave]]

Good lord....

This is still here...  I just did it to 151 bugs.

I did a mass-move on INVALID/WONTFIX/DUPLICATE/WORKSFORME from Webtools/Bugzilla
to Bugzilla/Bugzilla-General, and did reassign to default owner so that if any
of them got reopened someone who still cared would get the email.

We need this on b.m.o's next update before we continue to move bugs.

Expected behavior is that there is no change in bug_status if the bug_status is
already a state that's considered closed, unless the status is also changing for
some other reason as a result of the change (i.e. you clicked the reopen button).

Comment 10

[Dave Miller [:justdave]]

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

Comment 11

[Matthew Tuck [:CodeMachine]]

AIUI you can't reopen and reassign ...

Comment 12

[Jacob Steenhagen]

Attachment: patch

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 47991 [details] [diff] [review]
patch

>Index: process_bug.cgi

>+        if ( ($::FORM{knob} eq 'reassign' || $::FORM eq 'reassignbycomponent') &&
                                               ^^^^^^^
There is no $::FORM...  it's %::FORM unless you're providing a key.

Comment 14

[Jacob Steenhagen]

Attachment: patch v2 -- add missing {knob}

Comment 15

[Jacob Steenhagen]

-> Me

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 47993 [details] [diff] [review]
patch v2 -- add missing {knob}

Jake says he tested it and it works.  My visual inspection says it's good this time.  Since we're dealing with process_bug ickyness, and it's after 2am where I am, I suggest a second review.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

One note on this: not all bugs get sent to NEW.  There is one bug from tonight's
mass move by dave that got sent to UNCO.  Bug 66191.

Comment 18

[Dave Miller [:justdave]]

yeah, someone just "fixed" the unconfirmed one for us.

Comment 19

[timeless]

Comment on attachment 47993 [details] [diff] [review]
patch v2 -- add missing {knob}

s/itslef/itself/

Comment 20

[timeless]

Comment on attachment 47993 [details] [diff] [review]
patch v2 -- add missing {knob}

test

Comment 21

[Jacob Steenhagen]

Attachment: patch v3 - s/itslef/itself/ in comment

Comment 22

[Dave Miller [:justdave]]

Comment on attachment 48108 [details] [diff] [review]
patch v3 - s/itslef/itself/ in comment

r= justdave

Comment 23

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

Comment on attachment 48108 [details] [diff] [review]
patch v3 - s/itslef/itself/ in comment

The code works on my test installation, however...

>Index: process_bug.cgi
  ...
>+        if ( ($::FORM{knob} eq 'reassign' || $::FORM{knob} eq 'reassignbycomponent') &&
  ...
>+            $::query .= "bug_status = IF(bug_status IN($open_state), '$str', bug_status)";
>+            return;
>+        }
>         if (IsOpenedState($str)) {

Returning within the first if statement means the following line of code 
in this script never gets called when the first if conditional is true:

        $::FORM{'bug_status'} = $str; # Used later for call to
                                      # CheckCanChangeField to make sure this
                                      # is really kosher.

That's a security bug.  I suggest removing the return and making the second if
into an elsif to ensure it doesn't get called when the first if conditional
is true.

Comment 24

[Jacob Steenhagen]

Myk, not setting |$::FORM{bug_status} = $str;| isn't a security issue as we
(more likely than not) aren't really changing the status.  Unfortunately, there
is no way in the code to know if we are really changing the status or not (as
this is done before we even enter the loop that determines what bug(s) we are
changing).

Also, reassigning a bug requires the same permission as changing its status. 
While this may not always be true, it is at the moment and I really can't think
of another solution.

Comment 25

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

>Myk, not setting |$::FORM{bug_status} = $str;| isn't a security issue as we
>(more likely than not) aren't really changing the status.

If I read the SQL query correctly, however, it is possible for the statuses of
some of the bugs being reassigned to get changed.

>Also, reassigning a bug requires the same permission as changing its status. 
>While this may not always be true, it is at the moment and I really can't think
>of another solution.

The current solution is fine, it should just include the last line of the
function so the appropriate security check gets done.

Comment 26

[Jacob Steenhagen]

Attachment: patch v4

Comment 27

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

Comment on attachment 48798 [details] [diff] [review]
patch v4

>+            $::query .= "bug_status = IF(bug_status IN($open_state), '$str', bug_status)";
>+            $::FORM{bug_status} = $str;   # To check permissions later on
>+            return;
>+        }
>         if (IsOpenedState($str)) {

If you make this "elsif", then you don't need "$::FORM{bug_status} = $str;" 
or "return;" here, saving you two lines of code.

Comment 28

[Jacob Steenhagen]

Attachment: patch v5

Comment 29

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

Comment on attachment 49353 [details] [diff] [review]
patch v5

works on my test install and looks good.  1st r=myk.  do i have a second?

Comment 30

[Jacob Steenhagen]

Comment on attachment 49353 [details] [diff] [review]
patch v5

r= [e-mail] in IRC

Comment 31

[Jacob Steenhagen]

Checked in.