We're using the workflow module with the publishing module to set up a publishing process a little more complicated than that built into the publishing module. It's working fine, but the client becomes confused when making a workflow state change by the fact that the available workflow states aren't in order. The workflow admin page with the workflow and all the states lists them in the proper order and the weight column in the workflow_states table has them in the appropriate order. But when the client goes to a workflow tab, the order might say:

approved
public
rework
review
board

I see in the module several calls to workflow states that do not include an Order by Weight clause. Would adding one solve the problem? Would it cause others?

This is a Drupal 4.7 site, php 5.1.6, Linux, MySQL 4.1.21-standard.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Category: support » bug

Can not imagine why ordering the items would cause any problems... If you know how, feel free to provide a patch, otherwise I will create one within the next week.

Pancho’s picture

Status: Active » Reviewed & tested by the community
FileSize
867 bytes

Enclosed is a small two-line patch against HEAD. Please check whether the problem occurs in the 4.7 version as well.

I'm being so bold and set this to RTBC, as I can't imagine there could be any negative side effects.

rickvug’s picture

I changed the lines of code and the ordering still does not work. Maybe this is a due to a difference between 4.7 and HEAD?

mfredrickson’s picture

Status: Reviewed & tested by the community » Needs work

Demoting to code needs work b/c of rickvug's comments.

rickvug’s picture

Pancho,

Any news about this patch? Am I off base in thinking about what this does perhaps? The ordering bug is definitely a really glaring problem IMO.

Rick

mrsocks’s picture

Version: 4.7.x-1.0 » 5.x-1.1
Priority: Normal » Critical

has there been any update on this?
I am on drupal 5.1
php 5.2.1

rickvug’s picture

@mrsocks - Have you tried the latest development build to see if this problem still exists? If it does we know that this patch still needs to make it in.

nessumsara’s picture

The problem also exists on the node edit tab. This is a glaring problem that my client is constantly bugging me about when it will be fixed. Please provide an update and ETA or an explanation as to why it is difficult to solve.

mrsocks’s picture

I am using the 5.x-1.1 version and

replaced line
. "t.%s WHERE t.%s = %d", $field, $field, $field_where, $sid);

with
. "t.%s WHERE t.%s = %d ORDER BY s.weight ASC, s.state ASC",
$field, $field, $field_where, $sid);

and still nothing.

I also went through the entire module and added 'order by weight' and it didnt change.
It is showing in the order that the states are in the table, but not by weight.

Is there another function calling these same fields, mysql reserved word killing the ordering etc....?

mrsocks’s picture

OK OK OK..... I am getting closer.

it has something to do with these two functions (and probably the allowable trasitions, but this is a start hopefully)

function workflow_field_choices($node) {
  global $user;
  $wid = workflow_get_workflow_for_type($node->type);
  if (!$wid) { // no workflow for this type
    return array();
  }
  $states = workflow_get_states($wid); // so as of right here, the results are in the correct order, by weight, but missing current state.
  $roles = array_keys($user->roles);
  $current_sid = workflow_node_current_state($node); // and now sorted by weight as well.
  
  // if the node author or this is a new page, give the authorship role
  if (($user->uid == $node->uid && $node->uid > 0) || (arg(0) == 'node' && arg(1) == 'add')) { 
    $roles += array('author' => 'author');
  }
  if ($user->uid == 1) { // if the superuser
    $roles = 'ALL';
  }
  $transitions = workflow_allowable_transitions($current_sid, 'to', $roles);

  if ($current_sid != _workflow_creation_state($wid)) { // include current state if not (creation)
//    $transitions[$current_sid] = 'BOOYAH'.$states[$current_sid];  // I added this line as the way to check same funciton.
//    $transitions = array($current_sid => 'BOOYAH'.$states[$current_sid]) + $transitions; // why add an element to an array this way?
  }
  return $transitions;
}

AND

function workflow_allowable_transitions($sid, $dir = 'to', $roles = 'ALL') {
  $transitions = array();
  $field       = $dir == 'to' ? 'target_sid' : 'sid';
  $field_where = $dir != 'to' ? 'target_sid' : 'sid';
  $result = db_query("SELECT t.tid, t.%s as state_id, s.state as state_name FROM "
    . "{workflow_transitions} t INNER JOIN {workflow_states} s ON s.sid = "
    . "t.%s WHERE t.%s = %d ORDER BY s.weight ASC, s.state ASC",
    $field, $field, $field_where, $sid);

  while ($t = db_fetch_object($result)) {
    if ($roles == 'ALL' || workflow_transition_allowed($t->tid, $roles)) {
      //$state_id = str_pad($t->state_id, 5, '0', STR_PAD_LEFT);
      $state_id = $t->state_id;
      $transitions[$state_id] = 'here'.$t->state_name;
    }
  }
  return $transitions;
}

The user I am viewing this through has access to send the node to any state from any state.
Is there a way (not toally familiar with drupal api yet) to gather all the available states in one call and then when the radio buttons redner, have it check the state that it is in...

It;s probably doing this or similar to this now, but just not catching why.

Let me know if this helps or you need more info from me.

Thanks.

mfredrickson’s picture

This is a good bug report. I'll see if I can find time to use your findings and possibly shed some light.

I appreciate good bug reports (with apologies to the dude who submitted a screen cast of his bug. That was an historically great report, but I never touched the bug. :-) .).

mrsocks’s picture

I see in the documentation for the workflow_allowable_transitions() function,
there is a note:

* @return
* Associative array of states (sid=>name pairs), excluding current state.
*

I originally thought this was intentional, but now realize that there is no choice in the table for that transition.... because when you choose transitions, there is no group when the X and Y are the same in the transition matrix.

This is what is causing the issue. when this array (which is properly ordered at this time) is combined with the $transitions array in: workflow_field_choices() , the only fields that are there to use for sorting are the state id / sid (key) and the state name (value).

so it sticks it in there at the end....

THEN

function workflow_tab_form(&$node, $wid, $states, $current) {
/* ..... */  
    ksort($choices);

}

the ksort sorts the array by the key... the state id /sid.

.. i have an idea for now.... brb

mrsocks’s picture

Title: workflow state ordering on workflow tab » WORKING.... but not best solution

OK,

I have gotten this to work:

I created this function:

/**
 * set correct order of the states by weight
 * BUT also commented out the 'ksort()' in the workflow_tab_form() 
 * AND  workflow_form_alter() .
 * 
 * For me, not using this and just removing the ksort functions LOOKS
 * like it does the job, but that's just becuase the final state / current state
 * is the final step in the process.
 *
 * @param $wid
 *   The ID of the workflow.
 *
 * @param array $avail_states
 *    The states availabe to this user for this transaction.
 * 
 * @return
 *   An array of workflow states keyed by state ID ordered by weight
 * 
 * @author
 *  bp
 */
function workflow_get_states_order_by_weight($avail_states, $wid) {
  $correct_order = array();
  $avail_states = implode(',', array_flip($avail_states));
  $result = db_query("SELECT sid, state, weight FROM {workflow_states} WHERE sid IN (%s) 
                      AND wid = %d ORDER BY weight", $avail_states, intval($wid) );
  while ($data = db_fetch_object($result)) {
    $correct_order[$data->sid] = $data->state;
  }
  return $correct_order;
}

Then made these changes in the workflow_field_choices() function

function workflow_field_choices($node) {
/* . everything up here is the same ... */

  if ($current_sid != _workflow_creation_state($wid)) { // include current state if not (creation)
    $transitions[$current_sid] = $states[$current_sid]; // added by bp
//    $transitions = array($current_sid => $states[$current_sid]) + $transitions;  // removed by bp 
  }
  
  return workflow_get_states_order_by_weight($transitions, $wid);  // added by bp
//  return $transitions; // removed by bp
}

then removed the ksort in the workflow_tab_form() AND workflow_form_alter() functions.

and now everything seems to look correct.

everything SHOULD work the same because all i really did was just resort the transitions array with the keys (state ids/ sid) from the allowed transations group.

Let me know if anyone seems any issues with it.
I dont think this is the best solution, but something to get it looking better for now.

hope this helps.

mrsocks’s picture

Title: WORKING.... but not best solution » workflow state ordering on workflow tab

changed the title of the thread by accident.
so i changed it back here.

buchanae’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

my patch, simple and seems to work for me

sammos’s picture

I manually applied the patch and it worked for me in terms of addressing the problem. I haven't widely tested it yet. I hope removing the ksort doesn't change any other behavior but it doesn't seem like it would. Thank you!

wmostrey’s picture

Status: Needs review » Reviewed & tested by the community

Works perfectly, doesn't seem to affect any other processes.

shrop’s picture

Fixes sorting issues on both the edit node pages and workflow tabs for me.

Thanks!
Shrop

jeffomac’s picture

i applied this patch.
i noticed the following.

on content that was created prior to the patch, the sorting was still out of order.

On new content, the sorting was correct.

But when i edit the node (on new and old content) and change the workflow state, the order gets updated but not to the correct order.

whatever state I choose goes to the top of the list.

Has this happened to anyone else?

This is a drupal 5.2 site.

JacobSingh’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.79 KB

Yes, I see the same bug.

Here is another patch which should resolve it. The issue was that not only was there no weight clause in the query, the current state was just tacked on. I added a UNION query to get it in there.

jbjaaz’s picture

applied the patch and it cleared up the issue identified in comment #19.

good work

jbjaaz’s picture

JacobSingh,

I think I found something.

The patch correctly fixed issue from #19, but when I visited the "Workflow actions" page I noticed extra states listed.

It originally was

(creation) -> draft
draft -> review
review -> draft
review -> published

now, it looks like

(creation) -> (creation)
(creation) -> draft
draft -> draft
draft -> review
review -> draft
review -> review
review -> published
published-> published

jbjaaz’s picture

The patch provided in comment #20 did fix the issue described in #19, but it broke other parts of the module. I observed one of the issues in comment #22.

I attached a patch that fixes the issue in comment #19 and seems to keep everything else intact. The fix was a one liner. Where ksorts were removed elsewhere, one is needed when the form is being built.

I hope this settles this bug once a for all :-)

JacobSingh’s picture

Status: Needs review » Reviewed & tested by the community

I like it, looks good.

JacobSingh’s picture

Status: Reviewed & tested by the community » Needs review

Would like to get a once over from someone else.

JacobSingh’s picture

Status: Needs review » Needs work

Sorry, take it back, that last patch doesn't work because it only orders the states on the workflow admin screen, it does nothing for the node form which still orders by sid. And adding the order clause there won't help because it tacks on the current state which is why I did the query above and killed the ksort.

I'll have to go back in and fix this.

JacobSingh’s picture

While not exactly a workflow bug, but an issue with actions integration, I've been able to fix the problem noted in #22:

Instead of adding the current state (as was being done before), I'm now removing same state transactions from the actions screen. I don't much like this, as I think same state transactions could in fact fire actions, but for now, this is what is happening to not fundamentally change too much.

Please review ASAP. I want to release a new version as soon as this critical (and old) bug is taken care of.

jvandyk’s picture

This seems to have been committed in 1.54.2.11 but the commit was not noted in this thread. This commit seems to have added a case as noted here that tests

$t->tid == $t->state_id

which makes no sense. Testing a transition ID against a state ID is absurd. Transition IDs are sequential and are just keys to from-to state IDs.

JacobSingh’s picture

Version: 5.x-1.1 » 5.x-1.2
FileSize
2.28 KB

Hi John,

Indeed, this is messed up :(

Sorry I pushed it through, would have liked some more review. Here is a fix that might sort it.

Note: I also changed the syntax of the function slightly (switching to and from)
Please change back if you feel it has BC implications, but I found the previous way to be very unclear.

I also changed the statement in question which was a typo I think, but was part of allowing same state transitions.

Best,
Jacob

JacobSingh’s picture

d'oh! Left a debug statement in, sorry.

jvandyk’s picture

Version: 5.x-1.2 » 5.x-2.x-dev
Status: Needs work » Fixed

Applied patch based on the above work by JacobSingh and others.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.