Closed Bug 30731 Opened 23 years ago Closed 22 years ago

status not kept in mass reassignment of resolved bugs

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: dbaron, Assigned: jacob)

References

Details

(Whiteboard: mass_change)

Attachments

(1 file, 4 obsolete files)

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: kipp@netscape.com                            
  ReportedBy: tom@dizoglio.com               
  QAContact: vidur@netscape.com
  TargetMilestone: M15
--- 3,14 ----
  Version: other
  Platform: All
  OS/Version: All
! Status: NEW   
  Resolution: LATER
  Severity: normal
  Priority: P5
  Component: Layout
! AssignedTo: buster@netscape.com                            
  ReportedBy: tom@dizoglio.com               
  QAContact: vidur@netscape.com
  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 buster@netscape.com  2000-03-06 11:50
-------+ mine now
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.
MySQL doesn't have a concept of "row restrictions".
Status: NEW → ASSIGNED
Priority: P3 → P1
tara@tequilarista.org 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 .)
Assignee: terry → tara
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
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.
go don
Assignee: tara → donm
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Whiteboard: 2.14
Whiteboard: 2.14 → 2.16
moving to real milestones...
Target Milestone: --- → Bugzilla 2.16
-> New Bugzilla Product
Assignee: donm → myk
Status: ASSIGNED → NEW
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Whiteboard: 2.16
Version: other → unspecified
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).
Severity: normal → critical
*** Bug 77136 has been marked as a duplicate of this bug. ***
AIUI you can't reopen and reassign ...
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #47991 - Flags: review-
Attached patch patch v2 -- add missing {knob} (obsolete) — Splinter Review
-> Me
Assignee: myk → jake
Status: NEW → ASSIGNED
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.
Attachment #47993 - Flags: review+
Keywords: patch, review
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.
Summary: resolution kept in mass reassignment of resolved bugs → status not kept in mass reassignment of resolved bugs
yeah, someone just "fixed" the unconfirmed one for us.
Comment on attachment 47993 [details] [diff] [review]
patch v2 -- add missing {knob}

s/itslef/itself/
Attachment #47993 - Attachment is obsolete: true
Attachment #47993 - Flags: review+ → review-
Comment on attachment 47993 [details] [diff] [review]
patch v2 -- add missing {knob}

test
Attachment #47993 - Flags: review+
Attachment #47991 - Attachment is obsolete: true
Comment on attachment 48108 [details] [diff] [review]
patch v3 - s/itslef/itself/ in comment

r= justdave
Attachment #48108 - Flags: review+
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.
Attachment #48108 - Flags: review-
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.
>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.

Whiteboard: mass_change
Attachment #48108 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attached patch patch v5Splinter Review
Attachment #48798 - Attachment is obsolete: true
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?
Attachment #49353 - Flags: review+
Comment on attachment 49353 [details] [diff] [review]
patch v5

r= zach@zachlipton.com in IRC
Attachment #49353 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.