377485 - Implement editworkflow.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

Attachment: patch, v0.6

This bug is only about the implementation of editworkflow.cgi. The next steps will be to force process_bug.cgi and post_bug.cgi to look at the workflow table to get allowed transitions, which shouldn't be too hard.

I intentionally omit its entry in action.cgi as the workflow defined here is not yet effective. I will add it when process_bug.cgi and post_bug.cgi use it.

Note that you still cannot edit the list of bug statuses, nor change their open/close attribute. This will be the very last part of the implementation, when all scripts and templates are ready to use custom statuses.

Comment 1

[Frédéric Buclin]

(In reply to comment #0)
> I intentionally omit its entry in action.cgi as the workflow defined here is

Err... I meant admin.cgi

Comment 2

[Frédéric Buclin]

Comment on attachment 261546 [details] [diff] [review]
patch, v0.6

>Index: skins/standard/admin.css

>+.forbidden {

Please read |td.forbidden| here, so that it doesn't conflict with admin.cgi (which uses dt.forbidden for lists).

Comment 3

[Max Kanat-Alexander]

Comment on attachment 261546 [details] [diff] [review]
patch, v0.6

>+sub get_statuses {

  This makes it pretty clear that we need a Bugzilla::Status. It should be trivial to write. Also, it would make it easier to also have a "to_statuses" and "from_statuses" so that we wouldn't need get_workflow below.

>+sub load_template {

  This is clever, I like this. :-) It makes me wonder if we can write generic code to handle all of our edit* cgis.

  The rest of it looks fine, although it would be nicer to have create() and update() functions for Bugzilla::Status (which otherwise we'll just have to go back and write later).

>+<script type="text/javascript">
>+<!--
>+  function toggle_cell(cell) {

  I also agree with myk's comment here, that we should use classes instead of manually adjusting the color in JS.

>@@ -671,6 +672,19 @@ use constant ABSTRACT_SCHEMA => {
>         ],
>     },
> 
>+    workflow => {

  Have you thought about calling it status_workflow or workflow_status? There might be other tables that are part of the whole "workflow" thing.

  Oh, also, "type" here should instead be "require_comment."

>+sub _initialize_workflow {
>+    # excepted to be open statuses.

  Nit: "expected"

>+    my @statuses =
>+      @{$dbh->selectcol_arrayref('SELECT DISTINCT bug_status FROM bugs
>+                                  WHERE resolution != ?', undef, '')};

  I think custom statuses should be open by default. That's actually much more common. ("NEEDINFO" is the most common custom status.)

>+    my @workflow = ([undef, 'UNCONFIRMED', $create],

  What about the @statuses statuses? How do they get into @workflow?

>Index: skins/standard/admin.css
>+.col-header {
>+    width: 70px;
>+}

  Don't use px widths unless you're sizing something that holds a graphic. Use em.

>+.checkbox-cell {
>+    border: 1px black solid;

  "1px" for borders is fine, though. :-)

>+.open-status {
>+    color: #286;
>+}

  Nit: Put a comment here explaining what color this is. (And the same for all of the other number colors.)

  I didn't review any other part of the UI.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
>   This makes it pretty clear that we need a Bugzilla::Status. It should be
> trivial to write. Also, it would make it easier to also have a "to_statuses"
> and "from_statuses" so that we wouldn't need get_workflow below.

I don't see why get_statuses() makes clear that we need Bugzilla::Status, but get_workflow() doesn't make clear we need Bugzilla::Workflow.


>   The rest of it looks fine, although it would be nicer to have create() and
> update() functions for Bugzilla::Status (which otherwise we'll just have to go
> back and write later).

This code is unrelated to Bugzilla::Status. I neither create nor update a bug status, but the workflow.


>   I think custom statuses should be open by default. That's actually much more
> common. ("NEEDINFO" is the most common custom status.)

That's what my code is doing already: consider all bug statuses as open, and mark those having a resolution as closed.


>   What about the @statuses statuses? How do they get into @workflow?

They don't get into @workflow. Only known (aka default) statuses are in @workflow. The role of custom statuses is unknown and so we make no assumption about valid transitions. The admin will have to go to editworkflow.cgi himself and set the workflow correctly. That's the best way to make sure the workflow is as desired.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

Comment 6

[Gervase Markham [:gerv]]

Comment on attachment 263478 [details] [diff] [review]
patch, v1

>+print $cgi->header();

This is too early; I think we'll get two headers if you get an error.

>+        $workflow{$old || 0}{$new} = $type;

Could I mess things up by having a status whose name was the string "0"?



>+    foreach my $old ($initial_state, @$statuses) {
>+        # Hashes cannot have undef as a key, so we use 0. But the DB
>+        # must store undef, for referential integrity.
>+        my $old_id = $old->{'id'} || undef;

You set up this variable and then don't always use it. In particular, you use $old->{'id'} even in a situation where you are looking for a hash key:

>+                  unless defined $workflow->{$old->{'id'}}->{$new->{'id'}};

Why is that? Is it intentional? You should use $old_id throughout unless there's a good reason not to.

Same comment for this occurrence:

>+    foreach my $old (keys %$workflow) {
>+        # Hashes cannot have undef as a key, so we use 0. But the DB
>+        # must store undef, for referential integrity.
>+        my $old_id = $old || undef;

Which also uses different variable names. I woukd do:

foreach my $old_id (keys %$workflow) {
..
$old_id = ||= undef;
...

>+  # Contributor(s): Frédéric Buclin <[e-mail]>
>+  #                 Gervase Markham <[e-mail]>

Nit: I am [e-mail] in Bugzilla.

>+      [% FOREACH new_status = statuses %]
(etc)

Have you looked at whether it's possible to factor out some of the common code between your two templates?

>+    # This is the default workflow.

How do you know? Did you go through each state and check carefully, or did you rely on documentation?

>+    my @workflow = ([undef, 'UNCONFIRMED', $create],
>+                    [undef, 'NEW', $create],
>+                    [undef, 'ASSIGNED', $create],
>+                    ['UNCONFIRMED', 'NEW', $confirm],
>+                    ['UNCONFIRMED', 'ASSIGNED', $accept],
>+                    ['UNCONFIRMED', 'RESOLVED', $resolve],
>+                    ['NEW', 'ASSIGNED', $accept],
>+                    ['NEW', 'RESOLVED', $resolve],
>+                    ['ASSIGNED', 'NEW', $reassign],
>+                    ['ASSIGNED', 'RESOLVED', $resolve],
>+                    ['REOPENED', 'NEW', $reassign],
>+                    ['REOPENED', 'ASSIGNED', $accept],
>+                    ['REOPENED', 'RESOLVED', $resolve],
>+                    ['RESOLVED', 'UNCONFIRMED', $reopen],
>+                    ['RESOLVED', 'REOPENED', $reopen],
>+                    ['RESOLVED', 'VERIFIED', $verify],
>+                    ['RESOLVED', 'CLOSED', $close],
>+                    ['VERIFIED', 'UNCONFIRMED', $reopen],
>+                    ['VERIFIED', 'REOPENED', $reopen],
>+                    ['VERIFIED', 'CLOSED', $close],
>+                    ['CLOSED', 'UNCONFIRMED', $reopen],
>+                    ['CLOSED', 'REOPENED', $reopen]);
>+
>+    print "Now filling the 'status_workflow' table with valid bug status transitions...\n";

We used to have "unless $debug" on these... do we still?

Also, s/valid/default/.

>+    # XXX - It's time to remove obsolete commenton* parameters.

You rather need to write this code before checkin, don't you? :-)

Is there anywhere I can try it out?

r-, because I have points which I'd like you to comment on.

Gerv

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> (From update of attachment 263478 [details] [diff] [review])
> >+print $cgi->header();
> 
> This is too early; I think we'll get two headers if you get an error.

No this is fine. It behaves correctly and we do it all the time in other CGI scripts. Stop thinking as if you were still looking at Bugzilla 2.16 with all its hacks. This is 3.0. ;)


> 
> >+        $workflow{$old || 0}{$new} = $type;
> 
> Could I mess things up by having a status whose name was the string "0"?

No, editvalues.cgi doesn't allow "" nor 0 as value.


> >+    foreach my $old ($initial_state, @$statuses) {
> >+        # Hashes cannot have undef as a key, so we use 0. But the DB
> >+        # must store undef, for referential integrity.
> >+        my $old_id = $old->{'id'} || undef;
> 
> You set up this variable and then don't always use it. In particular, you use
> $old->{'id'} even in a situation where you are looking for a hash key:
> 
> >+                  unless defined $workflow->{$old->{'id'}}->{$new->{'id'}};
> 
> Why is that? Is it intentional? You should use $old_id throughout unless
> there's a good reason not to.

As the comment says, you cannot use undef as a hash key. That's why I use $workflow->{$old->{'id'}} which may be $workflow->{0} instead of $workflow->{$old_id} which may be $workflow->{undef}. So yes, I use it correctly.


> Same comment for this occurrence:
> 
> >+    foreach my $old (keys %$workflow) {
> >+        # Hashes cannot have undef as a key, so we use 0. But the DB
> >+        # must store undef, for referential integrity.
> >+        my $old_id = $old || undef;
> 
> Which also uses different variable names. I woukd do:
> 
> foreach my $old_id (keys %$workflow) {
> ..
> $old_id = ||= undef;
> ...

No, because I'm still looking at the hash a few lines later in the code, in which case I want 0, not undef. $old_id is used in the SQL statement.


> >+      [% FOREACH new_status = statuses %]
> (etc)
> 
> Have you looked at whether it's possible to factor out some of the common code
> between your two templates?

Nah, it's pretty hard without doing some hacks. The code would look even worse with them.


> >+    # This is the default workflow.
> 
> How do you know? Did you go through each state and check carefully, or did you
> rely on documentation?

I checked!


> >+    my @workflow = ([undef, 'UNCONFIRMED', $create],
> >+                    [undef, 'NEW', $create],
> >+                    [undef, 'ASSIGNED', $create],
> >+                    ['UNCONFIRMED', 'NEW', $confirm],
> >+                    ['UNCONFIRMED', 'ASSIGNED', $accept],
> >+                    ['UNCONFIRMED', 'RESOLVED', $resolve],
> >+                    ['NEW', 'ASSIGNED', $accept],
> >+                    ['NEW', 'RESOLVED', $resolve],
> >+                    ['ASSIGNED', 'NEW', $reassign],
> >+                    ['ASSIGNED', 'RESOLVED', $resolve],
> >+                    ['REOPENED', 'NEW', $reassign],
> >+                    ['REOPENED', 'ASSIGNED', $accept],
> >+                    ['REOPENED', 'RESOLVED', $resolve],
> >+                    ['RESOLVED', 'UNCONFIRMED', $reopen],
> >+                    ['RESOLVED', 'REOPENED', $reopen],
> >+                    ['RESOLVED', 'VERIFIED', $verify],
> >+                    ['RESOLVED', 'CLOSED', $close],
> >+                    ['VERIFIED', 'UNCONFIRMED', $reopen],
> >+                    ['VERIFIED', 'REOPENED', $reopen],
> >+                    ['VERIFIED', 'CLOSED', $close],
> >+                    ['CLOSED', 'UNCONFIRMED', $reopen],
> >+                    ['CLOSED', 'REOPENED', $reopen]);
> >+
> >+    print "Now filling the 'status_workflow' table with valid bug status transitions...\n";
> 
> We used to have "unless $debug" on these... do we still?

You mean $silent? $silent is only for common messages (those you get every time you run checksetup.pl). The messages you get only once when something special happens must be displayed in all cases.


> >+    # XXX - It's time to remove obsolete commenton* parameters.
> 
> You rather need to write this code before checkin, don't you? :-)

No, I plan to do it in a separate bug. Either me or Max.


> r-, because I have points which I'd like you to comment on.

So I will ask you for review again now that I answered all your questions.

Comment 8

[Frédéric Buclin]

Comment on attachment 263478 [details] [diff] [review]
patch, v1

re-requesting Gerv now that I answered all his questions.

Comment 9

[Gervase Markham [:gerv]]

(In reply to comment #7)
> No this is fine. It behaves correctly and we do it all the time in other CGI
> scripts. Stop thinking as if you were still looking at Bugzilla 2.16 with all
> its hacks. This is 3.0. ;)

Well, we definitely call it twice in the error case, because I checked. Presumably the code is smart enough not to output two headers, then?

> > >+    foreach my $old ($initial_state, @$statuses) {
> > >+        # Hashes cannot have undef as a key, so we use 0. But the DB
> > >+        # must store undef, for referential integrity.
> > >+        my $old_id = $old->{'id'} || undef;
> > 
> > You set up this variable and then don't always use it. In particular, you use
> > $old->{'id'} even in a situation where you are looking for a hash key:
> > 
> > >+                  unless defined $workflow->{$old->{'id'}}->{$new->{'id'}};
> > 
> > Why is that? Is it intentional? You should use $old_id throughout unless
> > there's a good reason not to.
> 
> As the comment says, you cannot use undef as a hash key. That's why I use
> $workflow->{$old->{'id'}} which may be $workflow->{0} instead of
> $workflow->{$old_id} which may be $workflow->{undef}. 

Ah, I read this the other way round (i.e. that $old_id could never be undef). Could you call it $old_id_for_db? That would make it a whole lot clearer.

> > >+    # XXX - It's time to remove obsolete commenton* parameters.
> > 
> > You rather need to write this code before checkin, don't you? :-)
> 
> No, I plan to do it in a separate bug. Either me or Max.

But, as you've written it here, such code would never be executed, because:

+    my $count = $dbh->selectrow_array('SELECT COUNT(*) FROM status_workflow');
+    return if $count;

So you'll only ever go past where that comment is once, at the time you do the initial migration. So I guess you could do it later, but you'll need to do more than just insert the code where that comment is.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 263478 [details] [diff] [review]
patch, v1

r=gerv; fixing the variable name is a strong request, not a requirement.

Gerv

Comment 11

[Max Kanat-Alexander]

Comment on attachment 263478 [details] [diff] [review]
patch, v1

status_workflow should be using ids, not varchars, unless you have some *extremely* good reason for using varchars.

In addition to being better DB design, using ids makes the table a fixed-length table, which is better for performance. (Not that performacne really matters on a table that small.)

Comment 12

[Frédéric Buclin]

Attachment: patch, v1.1

Oops! My code already stores the ID, not the name. I forgot to update Schema.pm accordingly.

Comment 13

[Max Kanat-Alexander]

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

Okay, that looks good to me. :-)

Comment 14

[Frédéric Buclin]

RCS file: /cvsroot/mozilla/webtools/bugzilla/editworkflow.cgi,v
done
Checking in editworkflow.cgi;
/cvsroot/mozilla/webtools/bugzilla/editworkflow.cgi,v  <--  editworkflow.cgi
initial revision: 1.1
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.33; previous revision: 1.32
done
Checking in skins/standard/admin.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/admin.css,v  <--  admin.css
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.102; previous revision: 1.101
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/workflow/comment.html.tmpl,v
done
Checking in template/en/default/admin/workflow/comment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/workflow/comment.html.tmpl,v  <--  comment.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/workflow/edit.html.tmpl,v
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
initial revision: 1.1
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.57; previous revision: 1.56
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.209; previous revision: 1.208
done