Timestamps for event transitions about transitions

[qoobaa]

Hm, looks nice, but I'm not sure if it fits in definition of "simple state_machine" ;-). You can always create a fork and implement it if you need that feature.

[troessner]

@Bodacious this seems like a great addition, however, I don't see how this can be implemented without rewriting major parts of the gem and / or having to do nasty things like a column for each state (which is certainly out of the question) or serializing a :state => :date hash in one column.

If you really need something like this, maybe https://github.com/airblade/paper_trail fits your requirements.

I'm closing this for now, please feel free to re-open it in case you find an elegant solution...:-)

[troessner]

Actually, I'm re-opening this since this issue seems like an incredibly useful addition.
The thing is, how can we implement this in the least intrusive way?
Of the top of my head I can think of the following approach:

1.) Make versioning optional a la

  #....

2. If versioning is switched on there needs to be an additional column "state_changes_at" (or something like that).

3. We hook into the transitions code and additionally store a serialized hash in "state_changes_at" with state as the key and the datetime as value.

4. We generate the necessary dynamic getters and setters.

5. We check for removed states on start up and update "state_changes_at" accordingly.

What do you think?
Sounds to me like this could be implemented in a minimal intrusive without changing any of the underlying logic.
Ideas?

[Bodacious]

what I had in mind (and eventually implemented in the app this issue was related to)

was that on state change, the code checks for the existence of "#{transition}_at" and, if present updates it, if not, it just carries on as normal.

For other applications this could be somewhat confusing if the developer wasn't aware of the behaviour and had added that column for another purpose.

How about something like:

# will check for the existence of paid_at or paid_on and set them on transition
# will raise an exception if neither are defined
event :pay, :timestamp => true do
  transitions :to => :paid
end

# will check for the existence of dispatch_date and update it if present
# will raise an exception if dispatch_date is not defined
event :deliver, :timestamp => :dispatch_date do
  transitions :to => :delivered
end

[troessner]

@Bodacious I see your point, but I have to say, I don't think that we should go down that road.

1.) As you already pointed out, it will be a source of confusion for other developers, even if it was implemented the way you described, especially if you can switch this feature on and off for each transition.

2.) Your approach would imply having a separate database column for each transition you want to log.
Now say I have an order model with 5 fields and 10 states and I want to log each state transition, that would mean the corresponding DB table would have (5 + 10 + 1) fields, so basically an overhead of ~60% just for logging when a transition occurs. So all of a sudden a rather slim model has exploded into a rather fat model, which, at this scale, will have performance implications.

[Bodacious]

1. if it's not set by default but can be turned on by passing the :timestamp option, it would be backwards compatible and would not get in the way for those who do not wish to use it or do not know about it.

event :first_transition, :timestamp => :state_changed_at do
  # it would be quite possible
end


event :second_transition, :timestamp => :state_changed_at do
  # to use the same DB column
end


event :third_transition, :timestamp => :state_changed_at do
  # using the approach I suggested
end

[troessner]

[1]

if it's not set by default but can be turned on by passing the :timestamp option, it would be backwards compatible and would not get in the way for those who do not wish to use it or do not know about it.

a) Doing something like

state_machine :remember_state_changes => true

with "false" as default argument would also be backwards compatible.

b) Doing that on a per-transition basis will be error-prone if you have more than a few states.
"Did I tell AR::Transitions to remember state changes for this transition or not?"
If you have a lot of states, you'd permanently end up looking up your transition definitions - and there's a good chance that you'll just wrongly assume that you did tell Transitions to do so when you actually didn't.

When doing something like

state_machine :remember_state_changes => true

it's crystal clear - it's either on or off.

[2] Regarding your second point:

I am not sure how you mean that - are you referring to having one column which just gets overwritten every time a corresponding state changes or are you talking about storing a serialized hash (which would preserve all other state changes)?

If it's the first one, I don't see the use of this feature, if it's only possible to remember the very last state change.

[Bodacious]

sorry @troessner - I misread your earlier point.

I thought you had suggested one timestamp for state_changed_at, the time of the last change - not a serialised hash.

I think a serialised hash would work but comes with it's own limitations.

# This wouldn't work with a serialised hash
@orders = Order.where("paid_at > ?", 1.week.ago) 

[troessner]

I think a serialised hash would work but comes with it's own limitations.

This wouldn't work with a serialised hash

@orders = Order.where("paid_at > ?", 1.week.ago)

Ack, very good point. I drop my former objections and agree to your suggestion.

what I had in mind (and eventually implemented in the app this issue was related to)

Would you mind whipping up a pull request?

[Bodacious]

No problem

Give me a few days

On 13 Aug 2011, at 17:02, troessner wrote:

I think a serialised hash would work but comes with it's own limitations.

This wouldn't work with a serialised hash

@orders = Order.where("paid_at > ?", 1.week.ago)

Ack, very good point. I drop my former objections and agree to your suggestion.

what I had in mind (and eventually implemented in the app this issue was related to)

Would you mind whipping up a pull request?

Reply to this email directly or view it on GitHub:
https://github.com/qoobaa/transitions/issues/23#issuecomment-1797823

[troessner]

@Bodacious Any updates on this issue?

[Bodacious]

@troessner - Sorry, been away on a couple of business trips these last few weeks.

At the moment I'm documenting the bulk of the code just to make sure I have a clear understanding of how it all works. After that, I'll be in a better position to make the changes agreed above.

[troessner]

Feature has been merged, thanks again -> closed.

[Bodacious]

YW