-
Notifications
You must be signed in to change notification settings - Fork 37
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
Prevent going to an invalid transition when using setter directly #49
base: master
Are you sure you want to change the base?
Prevent going to an invalid transition when using setter directly #49
Conversation
4a8dfea
to
e8f9178
Compare
@jairovm Thank you for a fabulous patch! I guess I totally understand where you came from, and yes I'm also hoping to solve that problem. However, I generally don't want my models to raise an error when I just put an attribute value. How about changing the implementation to just automatically add a simple model validation? |
@amatsuda thanks for taking the time to review this. It's funny that after having this fix running for a few days in our rails app my team and I decided to move this patch to a rails validation 👍 So yes, I'll work on that and let you know when it's ready. |
@jairovm 👍👍👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amatsuda before me spending more time on fixing tests, I'd like to know your thoughts about the changes I'm making here.
Pls let me know your thoughts when you have a chance.
Thank you
@@ -18,6 +18,13 @@ def initialize(model, column, states, prefix, suffix, &block) | |||
@model.send :undef_method, "#{@prefix}#{state}#{@suffix}!" | |||
end | |||
|
|||
model.validate(if: -> { send("#{column}_changed?") && send("#{column}_was") }) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amatsuda I think this now really feels the rails way :D
In order to accomplish that I had to add this new method StateInspector#stateful_enum_for
it allows possible_states
to be filtered by the current enum column
@@ -16,7 +16,9 @@ def enum(definitions, &block) | |||
if block | |||
definitions.each_key do |column| | |||
states = enum[column] | |||
(@_defined_stateful_enums ||= []) << StatefulEnum::Machine.new(self, column, (states.is_a?(Hash) ? states.keys : states), prefix, suffix, &block) | |||
(@_defined_stateful_enums ||= {}).tap do |hash| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to a Hash
so it's possible to do this later to get StatefulEnum::Machine
instances
StateInspector.new(self.class.stateful_enum[column.to_sym], self)
@@ -13,18 +13,28 @@ def stateful_enum | |||
end | |||
|
|||
def stateful_enum | |||
StateInspector.new(self.class.stateful_enum, self) | |||
self.class.stateful_enum.map do |column, defined_stateful_enum| | |||
StateInspector.new(defined_stateful_enum, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm proposing to make StateInspector
to work only with one defined_stateful_enum
instance at a time, that'll simplify the logic inside of it
a5192ab
to
8e6f8e8
Compare
Hi @amatsuda! I'd like to continue working on these changes, please let me know your comments when you get a chance 👌🏼 Thanks for your time 😃 |
Hi @amatsuda !
I've been playing around with this gem and I really like how AR
enum
is extended and how that brings usstate_machine
core functionality while still making use of anInteger
column in the DB side.WHY
There should be something that prevents records from transitioning to an
invalid
state through defaultAR
methods likeupdate(status: 1), status = 1
HOW
AR
enum settersDesired
state is being validated againststateful_enum.possible_states
state
is not included in thepossible_states
array an exception is throwncurrent state
is not blankdesired
state is different thatcurrent state
Also, I fixed/added tests.
P.S. This PR does not add support for triggering
before/after
callbacks when using ARsetters
Please, let me know you thoughts about this.
Thank you