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

add metadata parameter to allowed_transitions method #123

Closed
wants to merge 2 commits into from

Conversation

gravityblast
Copy link

This might be useful to check available transitions with guards, like checking valid transitions for the current user role.

@isaacseymour
Copy link
Contributor

This seems pretty sensible - @barisbalic @Sinjo @jackfranklin what do you think? Would need to add some tests to make sure it works before merging.

@jackfranklin
Copy link
Contributor

No objections to the change from me.

@gravityblast
Copy link
Author

@isaacseymour I added some tests, what do you think?

@Sinjo
Copy link
Contributor

Sinjo commented Feb 11, 2015

Good addition to the API. 👍 from me.

@jackfranklin
Copy link
Contributor

@pilu looks really good to me. Will let @isaacseymour make the final call but big +1. Thanks :D

@gravityblast
Copy link
Author

@jackfranklin cool, thank you guys for this gem 🤘

@isaacseymour
Copy link
Contributor

I'm 👍 - will merge if @barisbalic is 👍 on the API change

@barisbalic
Copy link
Contributor

@pilu thanks very much for raising this PR, it prompted some serious internal discussion about an aspect of Statesman we had taken for granted. In the current context everything you've suggested makes sense and the code is good, but we're going to have to decline this PR.

It's become clear that we should have been more careful about passing metadata around; it makes a lot of sense to be able to store context about transitions, but it also feels like a really bad idea to alter behaviour based on this somewhat "coincidental", loosely attached data.

If there's some piece of information that could change the outcome of a transition, it should be explicitly attached (as a field) to the transition itself, or more likely the class/model that houses the state machine. In the context of your user role example, from within a guard clause, if you want to check a user role, it makes more sense to consult the user/permissions structure than it does to pull something out of metadata. We're making use of Service objects in these types of situations, so it would look something like...

guard_transition(to: :deleted) do |item|
  user_access_control.can_delete_item?(current_user, item)
end

As a result of this debate, we're going to remove the argument from the can_transition_to? in the next version of Statesman, which should be coming in the next month or two.

Thanks again for your efforts on this, they are greatly appreciated and have pushed us to make Statesman a better state machine. If you have any further feelings or thoughts on the matter please continue to comment, but for now I'll close the thread.

@barisbalic barisbalic closed this Mar 9, 2015
@gravityblast
Copy link
Author

@barisbalic cool thank you very much for the explanation, it makes sense!

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

Successfully merging this pull request may close these issues.

None yet

5 participants