Skip to content

Conversation

@joffotron
Copy link

We came into a case where we needed to do state transitions on our Ecto models, but the new state would be received from user input (e.g an API call)

So instead of using a large conditional, and then using the can_x? / event functions to change state, we put together a simple ecto validation to check that the transition was a valid one.

It would be great if this could be considered for inclusion in the main repo - any feedback is welcome!

Thanks!
Joff

asiniy and others added 30 commits July 14, 2016 08:57
Validate changeset instead of raising exception on prohibited actions
Force ex 1.3 & ecto 2.0 to avoid conflicts
fix function calls ambiguities in mix config
Copy link
Owner

@asiniy asiniy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joffotron Hello Joe!

Thanks for proposition - it looks promising and definitely missing in the package functionality. I made couple of comments to the codebase - wdyt about them?

Plus, could you rebase your commits into 1, please?

test "changeset without state change" do
changeset = %User{} |> Ecto.Changeset.change(%{foo: "bar"})
|> User.validate_rules_change
assert changeset.valid? == true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be valid, since this bar state doesn't present in the states list.

assert changeset.valid? == true
end

test "invalid state change", context do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test all of the possible transitions. It's a little bit paranoic, but it's the testing convention right now

@joffotron joffotron force-pushed the feature/validate_change_without_event branch from 7afde42 to b515721 Compare May 22, 2017 05:38
@joffotron
Copy link
Author

@asiniy Hi! Sorry this has taken a while!

I think we have a more comprehensive set of tests now - let me know if there's anything else you need :-)

@asiniy asiniy force-pushed the master branch 7 times, most recently from ae477a8 to 69abe29 Compare July 22, 2017 13:06
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.

4 participants