Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get transitionable states from state on Model #17

Merged
merged 8 commits into from
Oct 28, 2019
Merged

Conversation

bjrnblm
Copy link
Contributor

@bjrnblm bjrnblm commented Oct 9, 2019

It would be nice to get allowedTransitions from the current state in a model with HasStates enabled so we can use that to fill for example a dropdown in a form.

Example code:

Article.php

protected function registerStates(): void
{
    $this
        ->addState('state', ArticleState::class)
        ->default(Concept::class)
        ->allowTransitions([
            [Concept::class, SubmittedForApproval::class],
            [Concept::class, Cancelled::class],
        ]);
}

Code:

$article = Article::first();
return $article->getAllowedTransitionsFrom('state', Concept::class);

Result:

[
  "App\\States\\SubmittedForApproval",
  "App\\States\\Cancelled"
]

Improvements

Not completely happy with the end result, would prefer it if:

$allowedTransitionsFrom[] = get_class($this->stateClass::make($to, $model));

Could be:

$allowedTransitionsFrom[] = $this->stateClass::make($to, $model);

So we get the transition classes instead of a string and we can do stuff like:

$allowedTransitions = $article->getAllowedTransitionsFrom('state', Concept::class);
foreach($allowedTransitions as $transition){
    dump((string) $transition, get_class($transition));
}

However I cannot get the tests to work that way, mainly because in HasStates the method getStateConfig is private.
So I am not sure how to load it from the Payment model inside the test.

Any input on this?

This allows to get an array with possible toTransitions from the state and current from field.

This array can for example be used in forms:

$allowedTransactions = $payment->getAllowedTransitionsFrom('state', Created::class)
@brendt
Copy link
Contributor

brendt commented Oct 10, 2019

Could you give an example of when it would be useful to know the allowed transitions?

@bjrnblm
Copy link
Contributor Author

bjrnblm commented Oct 10, 2019

I have a system where people in an organisation can propose articles for their blog and assign these to different people in this organisation. An article goes through multiple states before it gets published on the blog, for example:

Concept -> Submitted For Approval -> Rejected -> Submitted For Approval -> Approved -> Published

On the edit page of an article I would like to get the allowed states from the current state:

public function edit(Request $request, Article $article) : Renderable
{
    $allowedTransitions = $article->getAllowedTransitionsFrom('state', $article->state);

    return view('articles.edit')
        ->with('article', $article)
        ->with('allowedTransitions', $allowedTransitions);
}

So in articles/edit.blade.php I can render a select box with the allowed transitions for current state:

<select id="state" name="state">
    <option value="">Don't change</option>
    @foreach($allowedTransitions as $transition)
        <option value="{{ $transition }}">{{ $transition }}</option>
    @endforeach
</select>

So for an article in "Concept" stage, the correct options are showed:
Screenshot 2019-10-10 at 12 29 34

src/StateConfig.php Outdated Show resolved Hide resolved
src/StateConfig.php Outdated Show resolved Hide resolved
src/StateConfig.php Outdated Show resolved Hide resolved
src/StateConfig.php Outdated Show resolved Hide resolved
src/HasStates.php Outdated Show resolved Hide resolved
src/HasStates.php Outdated Show resolved Hide resolved
@brendt
Copy link
Contributor

brendt commented Oct 10, 2019

I'm ok with adding this. Please check my review, also don't forget to add docs for this feature in the docs folder.

@bjrnblm bjrnblm changed the title Feature getAllowedTransitionsFrom() to get possible transitions from current state Get transitionable states from state on Model Oct 10, 2019
@bjrnblm bjrnblm requested a review from brendt October 10, 2019 14:44
@brendt
Copy link
Contributor

brendt commented Oct 11, 2019

Thanks for all the changes! One more question, would it be possible to also allow this?

$transitionableStates = $payment->state->transitionableStates();

@bjrnblm
Copy link
Contributor Author

bjrnblm commented Oct 11, 2019

I agree, that would be a nicer syntax to use. I will look into it the coming days.

@brendt
Copy link
Contributor

brendt commented Oct 11, 2019

Thank you very much. Great PR up until now!

@bjrnblm bjrnblm requested a review from brendt October 11, 2019 12:26
src/HasStates.php Outdated Show resolved Hide resolved
src/HasStates.php Outdated Show resolved Hide resolved
src/HasStates.php Outdated Show resolved Hide resolved
src/StateConfig.php Outdated Show resolved Hide resolved
@brendt
Copy link
Contributor

brendt commented Oct 11, 2019

It's unfortunate that we'd have to do $payment->state->transitionableStates('fieldName');, in practice fieldName will always be the name of the property it's called upon. I have an idea to fix this though: we can resolve the state config when constructing a state, giving us access to the field name:

    public function __construct(Model $model)
    {
        $this->model = $model;
        
        $this->stateConfig = $model::getStateConfig()[get_parent_class($this)];
    }

Than the transitionableStates on the state class could be refactored to

public function transitionableStates(): array
{
    return $this->model->transitionableStates(get_class($this), $this->stateConfig->field);
}

There are some issues with this approach still (an infinite loop for one), but I believe these could be solved. @bjrnblm if you're not up for it, feel free to let me know, I'll be happy to merge your PR in as is (after you made the last changes I requested), and I'll give it a go myself.

@brendt
Copy link
Contributor

brendt commented Oct 11, 2019

Scratch that, it wouldn't work. I made a wrong assumption

@bjrnblm
Copy link
Contributor Author

bjrnblm commented Oct 11, 2019

@brendt Yes, I was also struggling with this today. The state does not know to which key it is assigned to in the getStateConfig array of the model.

For now I copied your code from the transitionTo method, but with the nullable $field + reset() it doesn't feel like a proper solution.

Tried to work around this but got stuck on several occasions, so it seems like a good idea to fix the docblocks etc and look at the problem later with a fresh mind.

@bjrnblm bjrnblm requested a review from brendt October 14, 2019 08:41
@bjrnblm
Copy link
Contributor Author

bjrnblm commented Oct 22, 2019

Hi @brendt anything I can do for this to PR to get it merged?

@brendt
Copy link
Contributor

brendt commented Oct 22, 2019

Oh sorry, I lost track of this one. I'll do my best to merge it later today!

@Hyra
Copy link

Hyra commented Oct 24, 2019

Ooh, this looks like a great addition! 👏

@brendt brendt merged commit 928b369 into spatie:master Oct 28, 2019
@brendt
Copy link
Contributor

brendt commented Oct 28, 2019

Thanks again, sorry for the delay. It's released as 1.3.0: https://github.com/spatie/laravel-model-states/releases/tag/1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants