399483 - Support creating a new ticket with status "closed"

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement, P3)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: 

I see enter_bugs.cgi is hardcoded to remove all CLOSED states, even though the workflow interface allows it.  

How come?  I would like to enter tickets with status "CLOSED" as sometimes we've already addressed a bug, but want to have it recorded in Bugzilla.   Right now, this is a two step process (create it, and then close it).  

Reproducible: Always

Comment 1

[Max Kanat-Alexander]

Probably because we don't have a resolution field on enter_bug. We could have a hidden one.

It'd be nice if you could specify the version field when you mention these things--I had to go look at the code to know you were talking about 3.1.

Comment 2

[Albert Ting]

(In reply to comment #1)
> Probably because we don't have a resolution field on enter_bug. We could have a
> hidden one.

Good point, forgot about that detail.

> It'd be nice if you could specify the version field when you mention these
> things--I had to go look at the code to know you were talking about 3.1.

Agreed, I forgot to add that.   

Comment 3

[Frédéric Buclin]

The reason I exclude closed states is that this is in 99% of cases not something you want to do. And having a 2-step process for the remaining 1% of cases is perfectly acceptable. That's a discussion I had with gerv when implementing custom workflow.

Comment 6

[miketosh]

(In reply to comment #3)
> The reason I exclude closed states is that this is in 99% of cases not
> something you want to do. And having a 2-step process for the remaining 1% of
> cases is perfectly acceptable. That's a discussion I had with gerv when
> implementing custom workflow.
> 

Unfortunately, the workflow editor allows setting these flows, so either they need to be prevented (not favored) or they need to work (favored).  My "opinion" is that it would be a bad idea for 3.2 to be released with this problem. :-)

I am currently checking to see how difficult it is to fix.  Possibly not that bad.

Comment 7

[Frédéric Buclin]

It's actually not that simple to fix. For two reasons:

1) letting post_bug.cgi pass 'resolution' to Bugzilla::Bug->create is not enough as we would still have to fix Bugzilla::Bug->run_create_validators to call _check_resolution(). _check_resolution() would need to be modified to take the bug status as second argument as it currently cannot rely on $self->bug_status to decide if a resolution must be defined or not.

2) _check_resolution independently of 1) does this check:

    if (Bugzilla->params->{"noresolveonopenblockers"} && $resolution eq 'FIXED')
    {
        my @dependencies = CountOpenDependencies($self->id);

This is going to be problematic to fix as CountOpenDependencies() can only work with existing bugs. It would need to be either hacked to get a list of bug IDs as argument rather than looking at the DB, or we shouldn't call it for new bugs and do the check in _check_resolution ourselves or even in run_create_validators(). I don't want to mess the code.

mkanat, what do you think? Marking this bug as blocking 3.2 for now as you can indeed file bugs as RESOLVED with no resolution (as Bug->set_status() is not called for new bugs, but is the one doing this verification). We could probably file a separate blocker about this last issue (i.e. temporarily forbidding closed states) but I think it's better to do the correct fix directly.

Comment 8

[Max Kanat-Alexander]

I think for 3.2 we should just forbid closed states on bug entry, and for 4.0 we can fix it right.

Comment 9

[miketosh]

(In reply to comment #7)
> It's actually not that simple to fix. For two reasons:
> 
> 1) letting post_bug.cgi pass 'resolution' to Bugzilla::Bug->create is not
> enough as we would still have to fix Bugzilla::Bug->run_create_validators to
> call _check_resolution(). _check_resolution() would need to be modified to 
> take the bug status as second argument as it currently cannot rely on
> $self->bug_status to decide if a resolution must be defined or not.

True.  _check_resolution would have to be smart enough to work without the bug created yet.

> 2) _check_resolution independently of 1) does this check:
>     if (Bugzilla->params->{"noresolveonopenblockers"} && $resolution eq
> 'FIXED') {
>         my @dependencies = CountOpenDependencies($self->id);
> 
> This is going to be problematic to fix as CountOpenDependencies() can only 
> work with existing bugs. It would need to be either hacked to get a list of 
> bug IDs as argument rather than looking at the DB, or we shouldn't call it for 
> new bugs and do the check in _check_resolution ourselves or even in
> run_create_validators(). I don't want to mess the code.

I like the idea of putting it in run_create_validators, but then that means duplicate code maintenance.


(In reply to comment #8)
> I think for 3.2 we should just forbid closed states on bug entry, and for 4.0
> we can fix it right.
 
I don't really think it will be ALL that much more difficult to do it right than to just forbid the action.  But I'll see if I can't get a patch ready sometime this week that you could apply to your first 3.3 release ;-)

Comment 10

[miketosh]

Attachment: Code Patch Ver 1

Added CountOpenBugs (should be obvious what it does) and added a _check_resolution within the run_create_validators.

The only thing left to do is to add a resolution select in the template/en/default/bug/create/create.html.tmpl template.  But I think ignoring that piece is MUCH better than disabling the entire creating of resolved bugs.  At least this way the user gets an error that they cannot create a closed bug without a resolution.  An advanced user can just add it to the URL, and an advanced admin can just add the field to the template.

max, please review.

Comment 11

[Frédéric Buclin]

Comment on attachment 317029 [details] [diff] [review]
Code Patch Ver 1

>diff -Nru bugz-old/Bugzilla/Bug.pm bugz-new/Bugzilla/Bug.pm

> sub _check_resolution {

>+        my @dependencies = $new ? CountOpenBugs(@$dependson) : 

Pass the list as an arrayref. Else we won't be able to pass additional arguments if we have to in the future.


>+sub CountOpenBugs {

>+    my $sth = $dbh->prepare(

You can combine all this in a single $dbh->selectcol_arrayref().



>diff -Nru bugz-old/enter_bug.cgi bugz-new/enter_bug.cgi

>+# Provide resolutions if there are closed statuses available for use
>+my @resolutions = @{get_legal_field_values('resolution')};
>+$vars->{'resolution'} = \@resolutions if $has_canconfirm && grep {!$_->is_open} @$initial_statuses;

Why do you need $has_canconfirm? Also, do not convert get_legal_... to an array and pass it again as ref later; doesn't make sense.


Your patch looks fine (I didn't test it yet), but please fix the UI as well. Else nobody will know about this fix.

Comment 12

[miketosh]

(In reply to comment #11)
> (From update of attachment 317029 [details] [diff] [review])
> > sub _check_resolution {
> >+        my @dependencies = $new ? CountOpenBugs(@$dependson) : 
> 
> Pass the list as an arrayref. Else we won't be able to pass additional
> arguments if we have to in the future.

Will do, I was just copying from CountOpenDependencies.
 
> >+sub CountOpenBugs { 
> >+    my $sth = $dbh->prepare(
> You can combine all this in a single $dbh->selectcol_arrayref().
 
Ok, I was again just copying from CountOpenDependencies.

> >+# Provide resolutions if there are closed statuses available for use
> >+my @resolutions = @{get_legal_field_values('resolution')};
> >+$vars->{'resolution'} = \@resolutions if $has_canconfirm && grep {!$_->is_open} @$initial_statuses;
> 
> Why do you need $has_canconfirm? 

We don't want to pass resolutions if the user cannot confirm, as code below this removes the closed statuses and isn't exactly something we can check on.  If its not a problem, I'd rather keep this here.

> Also, do not convert get_legal_... to an array
> and pass it again as ref later; doesn't make sense.

This was done amidst testing, so 'oops'.  Will do.
 
> Your patch looks fine (I didn't test it yet), but please fix the UI as well.
> Else nobody will know about this fix.

I was hoping I could get away without that, since it will be difficult to determine if/when to show the resolution field.  Unfortunately the statuses are only being passed as a simple array so it will be difficult to check against it.  Any suggestions?

Comment 13

[Frédéric Buclin]

(In reply to comment #12)

> Will do, I was just copying from CountOpenDependencies.

Yeah, but I don't know how old this function is. :)


> If its not a problem, I'd rather keep this here.

Are we sure the workflow always guarantees an open status to be always available on bug creation?


> I was hoping I could get away without that, since it will be difficult to
> determine if/when to show the resolution field.

pyrzak can help with the UI. He did it for show_bug.cgi. Some JS code is required.

Comment 14

[Guy Pyrzak]

I am happy to provide the UI code to make this work. bug/knob.html.tmpl code shows how to do it, but the other possibility is once the backend code is ready enough I can implement the front end stuff for you and add it as a patch to this bug. But you may be called upon in the future to do me a back-end code favor :D

Comment 15

[miketosh]

Attachment: Code Patch Ver 2

Here's a new patch, Ver 2.  Please re-review LpSolit.

(In reply to comment #13)
> Are we sure the workflow always guarantees an open status to be always
> available on bug creation?
Can't say for sure.  Removed that check.  Sending resolution no matter what.

(In reply to comment #14)
> ... But you may be called upon in the future to do me a back-end code
> favor :D
Pyrzak, I don't have to ask you on "the day of your daughter's wedding", do I?  For now, resolution is just a displayed field next to status defaulting to blank.  Work your magic! ;-)

Comment 16

[Frédéric Buclin]

Comment on attachment 317277 [details] [diff] [review]
Code Patch Ver 2

>+++ bugz-new/Bugzilla/Bug.pm	2008-04-22 14:11:56.595785111 -0400

>+sub CountOpenBugs {
>+    my $bug_list = shift;
>+    my $dbh = Bugzilla->dbh;
>+    my @openbugs;
>+
>+    return @openbugs if !scalar(@$bug_list);

Do not declare @openbugs yet (you declare it below). Simply write return () if !scalar(...).


>+    my @openbugs = @{$dbh->selectcol_arrayref(
>+         "SELECT bug_id FROM bugs " .
>+         "WHERE " . $dbh->sql_in('bug_id', \@$bug_list) .
>+         "AND bug_status IN (" . join(', ', map {$dbh->quote($_)} BUG_STATE_OPEN)  . ") ")};

Nit: do not enclose the SQL call in @{ }. You can return @$openbugs later. Moreover, the SQL is not the one desired. Look at what CountOpenDependencies() returns.
Also, please remove these "..." . "..." . "...". It's fine to have newlines in the query (this makes the code more readable).



>+++ bugz-new/template/en/default/bug/create/create.html.tmpl	2008-04-23 08:48:24.684361497 

>+    [% PROCESS resolution_select IF sel.name == "bug_status" %]

Do not write this hack. Simply call resolution_select at the right place earlier in the code (around line 305).


Note that your patch doesn't work. If you file a bug as open, you get an error that the resolution is not defined.

Comment 17

[Frédéric Buclin]

miketosh, any progress?

Comment 18

[miketosh]

no, but it is on my list of todo's for this week.

(Don't ask if it was on my todo's for last week though.  Or the week before...)

Comment 19

[Frédéric Buclin]

miketosh, we would like to release 3.2 RC1 very soon now. Any progress? If not, could you reassign the bug to me?

Comment 20

[miketosh]

Attachment: Code Patch Ver 3

progress...

Comment 21

[Max Kanat-Alexander]

Comment on attachment 327439 [details] [diff] [review]
Code Patch Ver 3

>@@ -1322,25 +1325,30 @@
>-    my ($self, $resolution) = @_;
>+    my ($self, $resolution, $bug_status, $dependson) = @_;
>     $resolution = trim($resolution);
>+    my $new = defined($bug_status);

  Instead of doing that you should rename $self to $invocant and check "ref $invocant", like we do in all the other _check functions.

>+    my $status = $new ? new Bugzilla::Status({name => $bug_status}) : $self->status;

  That should be Bugzilla::Status->check, no?

>+sub CountOpenBugs {

  Two things:

  New functions should not be named with CamelCaps--see perlstyle.

  CountOpenDependencies is only used in that one place in Bugzilla::Bug, you could replace it entirely with this function, or just a call to Bugzilla::Bug->new_from_list.

>+    my $openbugs = $dbh->selectcol_arrayref(
>+         "SELECT bug_id FROM bugs " .

  If you don't replace the function--this should be just COUNT(bug_id) and you should do selectrow_array.

>+         "WHERE " . $dbh->sql_in('bug_id', \@$bug_list) .

  And you don't need to dereference the buglist just to make it into a reference again! :-)

> [% ELSE %]
>     [% sel = { description => 'Initial State', name => 'bug_status' } %]
>     [% INCLUDE select %]
>+    [% PROCESS resolution_select  %]
> [% END %]

  I'm pretty sure that instead of this you should be reusing the code from bug/edit.html.tmpl. (Not copy/pasting it, but putting it into its own template and using it in both places.)

Comment 22

[Max Kanat-Alexander]

And with everything I just said, that's enough change that this should go into 4.0, not 3.2. I've filed bug 442821 for the simple fix for 3.2.

Comment 23

[miketosh]

Comment on attachment 327439 [details] [diff] [review]
Code Patch Ver 3

This old patch is no longer even close to the patch needed to implement this.  With the new 'workflow editor', there are many other places that need to be updated.

Comment 24

[Max Kanat-Alexander]

(In reply to comment #23)
> This old patch is no longer even close to the patch needed to implement this. 
> With the new 'workflow editor', there are many other places that need to be
> updated.

  Also, note that much of the work is now done in the blocker.

Comment 25

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 26

[miketosh]

I haven't started on a new patch for this, so let someone else get started if they wish.