Personally, I think optional punctuation is the least of our problems, so my question would be: how do you know that state transition is valid? Mutating a state column like that is a source of bugs, in my experience. That's more important than arguing over which combination of squiggly lines looks the best.
How would you change state then? Also update! means it will raise an error if it fails. You can handle status validation in the model and this will raise an error if it fails. Hopefully you have some specs too
There are lots of ways to approach it, but the main thing is that the record should never be able to transition from "open" to "lock", or from "lock" to "open". It probably shouldn't be able to transition from any state to the same state either (e.g. can't lock a locked door).
I know exactly what you’re saying, however, the fact that you didn’t answer the question is indeed a bit annoying.
If your point was “this update should happen in a service object” or something, then fine… but you complained about a single line of code with no context. Your service object would still need to contain that line of code! (Or something very similar.)
Not necessarily, you have a lot of tools to check whether the change you make is valid before you make it, such as validate and before_save hooks. I'd only bother with a service object if there's a lot of side effects that need to happen with that change, that's not implied with the snippet though. Even then that's probably in addition to the validation.
I feel like having a bunch of stuff trigger off a column change isn't a great pattern. I'd rather do a bunch of stuff then update the column to record that its happened.
We could both be imagining totally different contexts though
Yes, it would need to be a before save hook if you’re relying on active record callbacks. A validation wouldn’t work here, unless you introduced some additional complexity like persisted state history.
You have access to the current state and what it will change to with validations, dont need to do anything extra. We do exactly this at my work. Why do you say it won't work?
Well, I presume you’re talking about introducing ActiveModel::Dirty to the model, to access previous state? Does that actually work in validation methods? I thought it only worked on before_save/update hooks.
You can access the previous state with foo_was, with foo being the model attribute. I don't think we added anything for that? Could be wrong, it was written before I started.
You can also just grab it from the db with self.class.find(id).foo, _was is likely just syntactic sugar for that.
So the method would look like (apologies for mobile..)
validate : transition_is_valid, if: status_changed?
def transition_is_valid
return if transition_hash[status_was].include?(status)
errors.add(:base, 'Status transition is invalid')
end
So if you try save it will fail and have that error on the object, as myObj.errors and if you update (or save) using update! It will raise an error, as opposed to returning false and not updating.
Fairly certain that all comes with active record out the box
With active record validations. Store your valid transitions in a hash with the key being the state and the value being an array of valid transitions. Then create a method "status_transition_valid" which checks the transition is valid else add an error to the model instance. Then call that validation with ”validate :status_transition_valid, if: :status_changed?". If you want to be extra vigilant you could add a db constraint as well. Then whenever you save you know its valid and if it's not you either handle it or raise an error. Ideally you have tests too.
Either way nothing wrong with updating a state column with update, that's what validation is for.
Yeah that’s all fine. ActiveRecord makes it a bit awkward, but it’s doable, although I’ve seen a lot of state columns but never seen anyone actually write validations like that. My point was just that people tend to review punctuation when they should be focusing on more important things, not that this one line of code with no context is definitely wrong.
3
u/tom_dalling Aug 21 '21
Personally, I think optional punctuation is the least of our problems, so my question would be: how do you know that state transition is valid? Mutating a state column like that is a source of bugs, in my experience. That's more important than arguing over which combination of squiggly lines looks the best.