385415 - Bugs being marked as dupes or moved to another installation always get the RESOLVED status, even if RESOLVED no longer belongs to the workflow

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 

I didn't see a similar ticket so posting this one.

I have setup my workflow to never enter RESOLVED (using bug 101179).  However, when I mark a bug as DUPLICATE, it still goes into the RESOLVED state.

It would seem we should be able to indicate which state a DUPLICATE bug show transition into, rather than hard code it into RESOLVED?

On a similar note, what does bugzilla do if I delete the RESOLVED state and I attempt to mark a bug as DUPLICATE?  

Reproducible: Always

Comment 1

[Max Kanat-Alexander]

This is a good point.

I think we should possibly forbid the deletion of RESOLVED. I'm sure LpSolit has some ideas on this, though.

Comment 2

[Albert Ting]

(In reply to comment #1)
> I think we should possibly forbid the deletion of RESOLVED. I'm sure LpSolit
> has some ideas on this, though.

Actually I hope we do support the ability to delete RESOLVED.  In fact, my company wants to simplify and remove both RESOLVED and VERIFIED, and just have only one closed state, CLOSED.

Comment 3

[Frédéric Buclin]

Yes, you can remove RESOLVED from your list of valid bug statuses, but I left RESOLVED hardcoded in Bug.pm when marking a bug as a dupe. I will have to fix that before we release 3.1.1.

Comment 4

[Frédéric Buclin]

Some brainstorming:

Assume there is no NEW -> (any closed state) transition, but a user marks a bug as a dupe. As the bug's resolution is DUPLICATE, we have to change the bug's status to a closed state, but the workflow says there is no such transition allowed. What do we do?

1) Forbid to mark the bug as a dupe? This sounds ridiculous to me.
2) If there is at least one closed state available based on the workflow, use it ("RESOLVED" if this one exists); else any closed state, but you may use one which is not expected.
3) In editworkflow.cgi, explicitly requests the admin to select the status (say FOO) which should be used for open bugs being marked as a dupe. In this case, we could make sure that all (open status) -> FOO transitions are valid, and complain about (or force) the missing transitions.

3) seems to be the best alternative to me. If some transitions to FOO are missing, we could add them automatically and a message should explain why Bugzilla added them ("In order to use FOO as the bug status when marking bugs as dupes, all open status -> FOO transitions must be valid. Bugzilla added missing transitions.").

Comment 5

[Max Kanat-Alexander]

Yes, I think #3 sounds the best.

Comment 6

[Frédéric Buclin]

We have to decide where to store the bug status (FOO) to use for bugs being marked as a dupe (FOO DUPLICATE) or being moved to another installation (FOO MOVED). I first thought about data/params, but this would mean we would have to set the bug status from editparams.cgi, which sounds inappropriate to me as this information really belongs to editworkflow.cgi. But the advantage is that we have all the required code in place already, both in editparams.cgi and in editvalues.cgi.

On the other hand, we could add a new column (of type boolean) to the bug_status table, and set it to 1 for the bug status which must be used when marking a bug as a dupe or moved. The main disadvantage is that this requires a DB schema change and some (despite easy to write) logic in editvalues.cgi to prevent its deletion. But the main advantage is consistency.

The more I think about this problem, the more I think an additional column to the bug_status table is the best solution. Comments?

Comment 7

[Albert Ting]

I think an additional column would be more generic, especially if the column stated what the status value.  Plus one might want a different status for DUPLICATE as opposed to MOVED.  In fact, this could allow bugzilla to support additional "events", although I can't think of anything useful right now...

Comment 8

[Max Kanat-Alexander]

The only thing I don't like about the column is that it's a column that must be all zeros except for one.

That is, there can be only one DUPLICATE status, so it's not like each row needs its own column. If there were just some easy way to store it in the DB, that would be ideal, but logically data/params makes more sense--perhaps we could just display that particular parameter on editworkflow.cgi. All of the functions for loading and saving params are modular and can be used anywhere, so it wouldn't be too hard.

Comment 9

[Frédéric Buclin]

Attachment: patch, v1

Comment 10

[Frédéric Buclin]

Attachment: patch, v1.1

Fix 2 issues I just discovered.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 271898 [details] [diff] [review]
patch, v1.1

>Index: editparams.cgi
> [snip]
>+# We have to allow all transitions to this bug status.
>+sub add_missing_bug_status_transitions {
>+    my $bug_status = shift;
>+    my $dbh = Bugzilla->dbh;
>+    # We already know this bug status is valid.
>+    my $new_status = new Bugzilla::Status({name => $bug_status});
>+
>+    my $missing_statuses = $dbh->selectcol_arrayref('SELECT id
>+                                                       FROM bug_status
>+                                                  LEFT JOIN status_workflow
>+                                                         ON old_status = id
>+                                                        AND new_status = ?
>+                                                      WHERE old_status IS NULL',
>+                                                      undef, $new_status->id);

  Isn't there some object-oriented way to do this? Hrm, maybe not...

>Index: Bugzilla/Status.pm
> [snip]

> sub can_change_to {

  I don't understand the modifications to this function. Could you explain them?

>+ Description: Returns a list of bug status objects which can have

  Should say "C<Bugzilla::Status> objects"

>Index: Bugzilla/Config/BugChange.pm
> [snip]
> sub get_param_list {
>   my $class = shift;
>+
>+  my @closed_bug_statuses = map {$_->name} Bugzilla::Status::closed_bug_statuses();

  Params are created before the DB is created, as I recall. So this will make checksetup fail just like things did in the past, right?

>Index: template/en/default/bug/knob.html.tmpl
> [snip]
>+    [% NEXT IF bug_status.name == bug.bug_status %]

  Why is that in there? There should never be allowed a transition from one status to itself, so the template should never be receiving it.

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> > sub can_change_to {
> 
>   I don't understand the modifications to this function. Could you explain
> them?

The change I did allows FOO -> FOO "transitions" (e.g. NEW -> NEW). This way, a bug which has the RESOLVED status can still be marked as a dupe as its status would be changed to RESOLVED (or left unchanged, as you want). Else it would see RESOLVED -> RESOLVED as an invalid transition, and you would be unable to mark RESOLVED bugs as dupes. If we don't want this logic in Perl, then we should add FOO -> FOO transitions in the status_workflow table itself (which is currently not the case as I left this check for Perl).


>   Params are created before the DB is created, as I recall. So this will make
> checksetup fail just like things did in the past, right?

Maybe. I didn't check this as I saw that Bugzilla/Config/BugFields.pm was doing the same. But I will test this to be sure.


> >+    [% NEXT IF bug_status.name == bug.bug_status %]
> 
>   Why is that in there? There should never be allowed a transition from one
> status to itself, so the template should never be receiving it.

With my change explained above, you now get FOO -> FOO transitions (so that $bug->set_status() doesn't fail if the status doesn't change), and so you have to filter them here.

Comment 13

[Max Kanat-Alexander]

Okay, thanks for the explanation. I'd really like to see the logic in Perl, and not allow FOO -> FOO transitions in the DB. Anyhow, it's not really a status change, it's just a resolution change. So it shouldn't ever appear that the status is changing. (I understand why it does, but I'd prefer the logic in Perl, not in the DB.)

Comment 14

[Frédéric Buclin]

Attachment: patch, v2

Not only this patch fixes crashes in checksetup.pl when upgrading, but it also fixes major problems found when you do a fresh installation of 3.1 (no bug status got the 'closed' state if you where upgrading from an installation which had no bug yet or if you were doing a clean installation).

/me has never done so many tests for a single patch.

Comment 15

[Max Kanat-Alexander]

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

Looks good.

<mkanat> LpSolit: I still don't think the template should be getting information that statuses can change to themselves.
 But I'm not going to r- over it.

Comment 16

[Frédéric Buclin]

Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.45; previous revision: 1.44
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.22; previous revision: 1.21
done
Checking in editworkflow.cgi;
/cvsroot/mozilla/webtools/bugzilla/editworkflow.cgi,v  <--  editworkflow.cgi
new revision: 1.3; previous revision: 1.2
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.375; previous revision: 1.374
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.189; previous revision: 1.188
done
Checking in Bugzilla/Status.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v  <--  Status.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Config/BugChange.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/BugChange.pm,v  <--  BugChange.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.15; previous revision: 1.14
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.36; previous revision: 1.35
done
Checking in template/en/default/admin/params/bugchange.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/bugchange.html.tmpl,v  <--  bugchange.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/workflow/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/workflow/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--  knob.html.tmpl
new revision: 1.33; previous revision: 1.32
done