Prevent going to an invalid transition when using setter directly #49
Conversation
jairovm
force-pushed
the
prevent-going-to-an-invalid-transition-on-skipping-transaction-methods
branch
from
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 👍👍👍 |
| @@ -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.
@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.
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.
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
jairovm
force-pushed
the
prevent-going-to-an-invalid-transition-on-skipping-transaction-methods
branch
from
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
enumis extended and how that brings usstate_machinecore functionality while still making use of anIntegercolumn in the DB side.WHY
There should be something that prevents records from transitioning to an
invalidstate through defaultARmethods likeupdate(status: 1), status = 1HOW
ARenum settersDesiredstate is being validated againststateful_enum.possible_statesstateis not included in thepossible_statesarray an exception is throwncurrent stateis not blankdesiredstate is different thatcurrent stateAlso, I fixed/added tests.
P.S. This PR does not add support for triggering
before/aftercallbacks when using ARsettersPlease, let me know you thoughts about this.
Thank you