state-machines / state_machines-audit_trail

Log transitions on a state_machines gem to support auditing and business process analytics.
https://github.com/state-machines/state_machines-audit_trail
MIT License
75 stars 24 forks source link

dynamic context can pollute object with methods, allow for hash of fields as an alternative #2

Closed rosskevin closed 9 years ago

rosskevin commented 9 years ago

Relevent code that tightly couples context method name and field name.

I was thinking about this before, now I've run into it:

I want to be able to log a security context for spam/fraud detection and acknowledgment of key actions (for terms of service).

Here is a current example (with growing list of attributes):

audit_trail context: [:state, :payment_state, :user_id, :remote_addr, :session, :user_agent]
...
end

def user_id # makes no sense in my model
...
end

def remote_addr # makes no sense in my model
...
end

def session # makes no sense in my model
...
end

def user_agent # makes no sense in my model
...
end

I propose that I add the ability for the linked code to detect if a hash is returned from the context method. If it is a hash, use the hash key as the state transition field, and use the value as value. It's simple and backwards compatible, This would clean up the declaration and the superfluous methods.

Proposed result:

audit_trail context: [:state, :payment_state, :security_context]
...
end

private
def security_context
...
 {user_id: s.user_id, remote_addr: s.remote_addr, session: s.session, user_agent: s.user_agent}
end

or even one better (with my code):

audit_trail context: [:state, :payment_state, :security_context]
...
end

private
def security_context
...
 s.attributes
end

All enhancement, no breakage. Thoughts @seuros?

smudge commented 9 years ago

If I'm reading this correctly, in that 'proposed result' example, does this mean that because security_context returns a hash, the state transition object will receive updates for all four of those fields? (i.e. instead of updating a security_context field, it updates user_id, remote_addr, session, and user_agent)

The only problem with this solution is if I want to store a serialized hash as the logged context. Say I have an Event class with a json/hstore/yml-serialized properties column (any would do) and a state machine, and each time the state changes I want to log all of the event properties at the time of the transition (into a properties field on the transition object). Typically in Rails such a column would be deserialized into a ruby Hash. I choose this example in particular because this is a case where I may not know exactly what the fields contained in the hash will be (as the structure of the data might depend on a dynamic/external source), so I can't even work around this by having separate fields for each key in the event's properties hash.

Thoughts?

rosskevin commented 9 years ago

@smudge I agree that could be a problem. What's a better solution?

rosskevin commented 9 years ago

Hmm, I guess my state transition could have (as-is):

class FooStateTransition
  def security_context = (s)
        self.user_id = s.user_id
        ...etc...
  end 

This would look like the proposed declaration but break it down into fields as desired. This is still a tightly coupled naming approach, but I can work with it.

smudge commented 9 years ago

Yes, that would work. The alternative I had in mind was something like this:

SECURITY_CONTEXT = :user_id, :remote_addr, :session, :user_agent
audit_trail context: [:state, :payment_state] + SECURITY_CONTEXT
delegate SECURITY_CONTEXT, to: :security_context

But this simply masks the list of methods that get defined on the model, so I think your approach makes more sense -- let it be the transition object's responsibility to interpret the context values. This keeps the scope of the context-logging feature a bit simpler (i.e. it doesn't have to care what the type of the returned value is -- it just passes it through to the transition object).

rosskevin commented 9 years ago

No changes necessary, it works as expected with the #security_context= inside the state transition object.