raoul2000 / yii2-workflow

A simple workflow engine for Yii2
BSD 3-Clause "New" or "Revised" License
171 stars 48 forks source link

Endstate with no transitions throws an error #48

Closed dbd5 closed 7 years ago

dbd5 commented 7 years ago

Hi @raoul2000 ,

raoul2000\workflow\base\WorkflowValidationException: One or more end status are not defined : [
    0 => ‘PostWorkflow/archived’
] in /var/www/app/vendor/raoul2000/yii2-workflow/src/source/file/WorkflowArrayParser.php:85

Take the use case in your examples but add the requirement of a business rule that once an article is archived, it is never re-used in any form therefore no transitions can be made from the archived node.

It seems that the simple workflow does not accept an end state that has no transitions? How can I address this use case or am I doing something fundamentally wrong?

Thanks

raoul2000 commented 7 years ago

Hi @dbd5, could you please show me the workflow you are using ? Thanks

dbd5 commented 7 years ago

Sure;

The Definition;

` public function getDefinition() { $status = []; $start_ids = Transition::find()->select('start_status_id')->distinct()->asArray()->all(); $start_ids = ArrayHelper::getColumn($start_ids, 'start_status_id'); foreach ($start_ids as $id) { $end_ids = Transition::find()->select('end_status_id')->where(['start_status_id'=>$id])->asArray()->all(); $end_ids = ArrayHelper::getColumn($end_ids, 'end_status_id');

    $color = Metadata::find()->where([
        'workflow_id' => static::SW_WORKFLOW_NAME,
        'status_id' => $id,
        'key' => 'color',
    ])->one();

    if (isset($color)) {
        $colorValue = $color->value;
    } else {
        $colorValue = Status::getColor($id);
    };

    $status[$id]=[
        'transition' => $end_ids,
        'metadata' => [
            'color'=>$colorValue,
        ],
    ];
}

return [
    'initialStatusId'=>static::getInitialStatus(),
    'status'=>$status,
];

} `

dbd5 commented 7 years ago

The charts;

screen shot 2017-09-14 at 5 30 09 pm screen shot 2017-09-14 at 5 30 43 pm

The engine throws the error when you have open terminating nodes like in the first image but its happy with the second.

The getdefinition implementation calls classes from the project by @cornernote

raoul2000 commented 7 years ago

Hi @dbd5, sorry but I still don't see why this exception is thrown ... It seems to be that status PostWorkflow/archived is not defined, but based on the charts, it is here. As you workflow is stored in a DB, can I ask you to provide the dump (var_dump()) of the value returned by your getDefinition function ?

Thanks

dbd5 commented 7 years ago

Hi @raoul2000 - will do just below but first a few comments;

Here are the 2 charts as well as the definition dumps (Unfortunately, this editor doesn't keep the JSON formatting so you may have to copy the JSON and reformat elsewhere );

screen shot 2017-09-15 at 12 17 41 pm

{ "initialStatusId": "draft", "status": { "approved": { "transition": ["publish"], "metadata": { "color": "#64C408" } }, "draft": { "transition": ["publish", "reviewed"], "metadata": { "color": "#794CB1" } }, "on-hold": { "transition": ["reviewed"], "metadata": { "color": "#641D33" } }, "publish": { "transition": ["archived"], "metadata": { "color": "#844CAD" } }, "reviewed": { "transition": ["approved", "on-hold"], "metadata": { "color": "#27891F" } } } }

dbd5 commented 7 years ago

And the version with the workaround;

screen shot 2017-09-15 at 12 16 40 pm

{ "initialStatusId": "draft", "status": { "approved": { "transition": ["publish"], "metadata": { "color": "#64C408" } }, "archived": { "transition": ["reviewed"], "metadata": { "color": "#537E88" } }, "draft": { "transition": ["publish", "reviewed"], "metadata": { "color": "#794CB1" } }, "on-hold": { "transition": ["reviewed"], "metadata": { "color": "#641D33" } }, "publish": { "transition": ["archived"], "metadata": { "color": "#844CAD" } }, "reviewed": { "transition": ["approved", "on-hold"], "metadata": { "color": "#27891F" } } } }

raoul2000 commented 7 years ago

Hi @dbd5, thanks for your nice comments about yii2-workflow 😄

I have copied the JSON of the first workflow, beautified it and past below. The result show clearly that this workflow definition doesn't contain a status called "archived".... and that's why yii2-workflow validation fails. As you can see, the statusobject has no 'archived' property, so 'archived' status is not defined.

{
    "initialStatusId": "draft",
    "status": {
        "approved": {
            "transition": [
                "publish"
            ],
            "metadata": {
                "color": "#64C408"
            }
        },
        "draft": {
            "transition": [
                "publish",
                "reviewed"
            ],
            "metadata": {
                "color": "#794CB1"
            }
        },
        "on-hold": {
            "transition": [
                "reviewed"
            ],
            "metadata": {
                "color": "#641D33"
            }
        },
        "publish": {
            "transition": [
                "archived"
            ],
            "metadata": {
                "color": "#844CAD"
            }
        },
        "reviewed": {
            "transition": [
                "approved",
                "on-hold"
            ],
            "metadata": {
                "color": "#27891F"
            }
        }
    }
}

But then you may be asking : why is the 'archived' status displayed in the graph view ? .. well this I dont know because this workflow viewer is not part of the release yii2-workflow extension. this viewer probably contains a bug ....

Hope this helped Cheers 😎

dbd5 commented 7 years ago

Hi @raoul2000 , The status table does indeed contain an entry for archived; I take full responsibility for any bug in the getDefinition function if this is not what you intended.

But please clarify:

If we agree on the last 2 questions, then the big question is Should there be an entry in the getDefinition array for a status that has no originating transitions? (note that the getDefinition result correctly represents the transition to archived from publish - what this discussion is really about is that there is no definition of any origination transitions from archived)

That will lead to an empty transition array within the getDefinition result like this; ... "archived": { "transition": [ ], }

Is this the correct way?

raoul2000 commented 7 years ago

hi @dbd5, To the question "Should there be an entry in the getDefinition array for a status that has no originating transitions?", the reply is : yes..... and even if this status has no out-going transition. Such status is called "final status" (or 'terminal status') meaning that once the model enters in this status, it will never be able to leave it (that's the case of the 'archived' status right ?). If you take a look to this chapter in the documentation, wou'll see that"s exactly what is been described. As you can see you don't even have to declare an empty transition array.

Now regarding the getDefinition function let me clarify. I don't know if it has a bug or if the Workflow that is read from DB is not complete (missing 'archived' status definition), but what I wanted to say is that the widget which is in charge or representing this workflow as a graph does certainly contains a bug. It should not represent the 'archived' status because it is not par of the status list definition, but only exist as an end status (which is not enough for a valid workflow definition). I don't know if this widget is the one I have published as yii2-workflow-view but if that's the case, I confirm that this bug is present !! ...and I will try to push a fix as soon as possible (sorry for the inconvenience).

Hop this helped ciao 😎

dbd5 commented 7 years ago

Thanks @raoul2000 , It helped a lot! I'll update the getDefinition function to reflect the proper behavior in the terminal status. I'll also await your update to https://github.com/raoul2000/yii2-workflow-view to address the proper rendering of the chart