The default bug view has changed. See this FAQ.

Implement editworkflow.cgi

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Administration
--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

22.07 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 261546 [details] [diff] [review]
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.
Attachment #261546 - Flags: review?(wicked+bz)
Attachment #261546 - Flags: review?(mkanat)
Attachment #261546 - Flags: review?(bugzilla-mozilla)
(Assignee)

Comment 1

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

Err... I meant admin.cgi
(Assignee)

Comment 2

10 years ago
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

10 years ago
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.
Attachment #261546 - Flags: review?(wicked+bz)
Attachment #261546 - Flags: review?(mkanat)
Attachment #261546 - Flags: review?(bugzilla-mozilla)
Attachment #261546 - Flags: review-
(Assignee)

Comment 4

10 years ago
(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.
(Assignee)

Comment 5

10 years ago
Created attachment 263478 [details] [diff] [review]
patch, v1
Attachment #261546 - Attachment is obsolete: true
Attachment #263478 - Flags: review?(mkanat)
Attachment #263478 - Flags: review?(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 <LpSolit@gmail.com>
>+  #                 Gervase Markham <gerv@mozilla.org>

Nit: I am gerv@gerv.net 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
Attachment #263478 - Flags: review?(gerv) → review-
(Assignee)

Comment 7

10 years ago
(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.
(Assignee)

Comment 8

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

re-requesting Gerv now that I answered all his questions.
Attachment #263478 - Flags: review- → review?(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 on attachment 263478 [details] [diff] [review]
patch, v1

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

Gerv
Attachment #263478 - Flags: review?(gerv) → review+

Comment 11

10 years ago
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.)
Attachment #263478 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

10 years ago
Created attachment 265036 [details] [diff] [review]
patch, v1.1

Oops! My code already stores the ID, not the name. I forgot to update Schema.pm accordingly.
Attachment #263478 - Attachment is obsolete: true
Attachment #265036 - Flags: review?(mkanat)

Comment 13

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

Okay, that looks good to me. :-)
Attachment #265036 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 14

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.