search

state-machines / state_machines-activerecord

StateMachines Active Record Integration
https://github.com/state-machines/state_machines-activerecord
MIT License
401 stars 83 forks source link

Map attribute aliases to column names when initializing #95

Open joelvh opened 3 years ago

joelvh commented 3 years ago

Fixes https://github.com/state-machines/state_machines-activemodel/issues/2

joelvh commented 3 years ago

@seuros @rafaelfranca there seem to be some job errors (not test failures) - can you re-run the jobs?

seuros commented 3 years ago

@joelvh can you rebase/merge master to this branch ?

seuros commented 3 years ago

@joelvh I think alias_attribute should be in active-model

cc @rafaelfranca

joelvh commented 3 years ago

@seuros do you have a suggestion of how to accomplish this in ActiveModel? Using Pry, I didn't see any calls from the constructor handling the attributes using ActiveModel, from what I recall because I also thought it might fit there. The place to implement this in AR was much clearer.

joelvh commented 3 years ago

@seuros I've come up with a fix for ActiveModel in https://github.com/state-machines/state_machines-activemodel/pull/28. However, my tests to see if ActiveRecord is fixed by this indicate the params handled by ActiveModel don't get passed to ActiveRecord.

I'm doing some investigating, but it would appear my previous comment still stands, that AR either doesn't use the AM initialization or the transformed params are not passed along properly.

joelvh commented 3 years ago

@seuros my latest investigation points to define_state_initializer defined in ActiveRecord and ActiveModel implementations, but they don't inherit from each other. Is that a correct observation? That would explain what I saw using pry and why the AR implementation is not fixed by this PR alone.

It would seem that we need separate fixes (e.g. the two PRs I've created) unless there's a way to to have AR's initializer trigger AM's inside the state machine.

joelvh commented 3 years ago

@seuros I've rebased this

joelvh commented 3 years ago

@seuros just checking to see if this is the route to go or the ActiveModel PR.

joelvh commented 3 years ago

@seuros will this be merged?