Added configurable support to log extra state machine attributes in the audit trail #6

Merged
merged 5 commits into from Jul 26, 2012

Conversation

Projects
None yet
4 participants
Contributor

augustj commented Apr 13, 2012

I added the ability to log an attribute/method of the state machine's model into the audit trail. For example, if the model goes into an error state, you can log something about why such as the error message. There is currently only support for ActiveRecord. The audit trail model will need to have an attribute with the same name as the state machine model's property that will be logged.

Any thoughts on this @wvanbergen ? I was just thinking along the same lines (if it goes to error, you really want to know why) and definitely +1 the concept.

Owner

wvanbergen commented Apr 19, 2012

Sorry, this pull request got lost in my email black hole. Really like this idea!
Also pinging @jstorimer to take a look at it.

Contributor

augustj commented Apr 20, 2012

I'm using on a project - it was very useful early on in development. Even after a couple months it was nice to have more context when unusual errors happened.

Contributor

augustj commented May 2, 2012

You guys still interested in this? As my project goes into production, I'd prefer to be referencing master on your repository rather than my fork.
Cheers.

Owner

wvanbergen commented May 8, 2012

Yeah, I certainly am interested in this but I have very little time to do a code review at the moment. Sorry about that.

@wvanbergen wvanbergen commented on an outdated diff Jul 26, 2012

lib/state_machine/audit_trail/backend/active_record.rb
def log(object, event, from, to, timestamp = Time.now)
# Let ActiveRecord manage the timestamp for us so it does the
# right thing with regards to timezones.
- transition_class.create(foreign_key_field(object) => object.id, :event => event, :from => from, :to => to)
+ params = {foreign_key_field(object) => object.id, :event => event, :from => from, :to => to}
+
+ #filtered = (transition_class.attribute_names.collect { |key| key.to_sym } - params.keys - [:id, :created_at])
+ #filtered.each { |param| params[param] = object.send(param) }
@wvanbergen

wvanbergen Jul 26, 2012

Owner

Can you get rid of the commented lines?

@wvanbergen wvanbergen and 1 other commented on an outdated diff Jul 26, 2012

lib/state_machine/audit_trail/backend/active_record.rb
def log(object, event, from, to, timestamp = Time.now)
# Let ActiveRecord manage the timestamp for us so it does the
# right thing with regards to timezones.
- transition_class.create(foreign_key_field(object) => object.id, :event => event, :from => from, :to => to)
+ params = {foreign_key_field(object) => object.id, :event => event, :from => from, :to => to}
+
+ #filtered = (transition_class.attribute_names.collect { |key| key.to_sym } - params.keys - [:id, :created_at])
+ #filtered.each { |param| params[param] = object.send(param) }
+ if context_to_log.is_a?(Array.class)
@wvanbergen

wvanbergen Jul 26, 2012

Owner

what is the idea behind this if statement?

@augustj

augustj Jul 26, 2012

Contributor

I had worked on allowing multiple contexts to be logged - and this would have been in an Array. Since I backed out from doing that, this check isn't necessary.

@wvanbergen wvanbergen commented on an outdated diff Jul 26, 2012

spec/helpers/active_record.rb
t.integer owner_class.name.foreign_key
t.string :event
t.string :from
t.string :to
+ t.string :context if add_context #(owner_class.instance_methods.include? :context)
@wvanbergen

wvanbergen Jul 26, 2012

Owner

Can you get rid of the comment again?

@wvanbergen wvanbergen commented on the diff Jul 26, 2012

spec/state_machine/active_record_spec.rb
@@ -18,21 +18,36 @@
last_transition.event.to_s.should == 'start'
last_transition.from.should == 'waiting'
last_transition.to.should == 'started'
+ #last_transition.context.should_not be_nil
@wvanbergen

wvanbergen Jul 26, 2012

Owner

More comments.

@wvanbergen wvanbergen commented on an outdated diff Jul 26, 2012

spec/state_machine/active_record_spec.rb
lambda { state_machine.stop }.should_not change(ActiveRecordTestModelStateTransition, :count)
end
end
-
+
+ context 'on an object with a single state machine that wants to log a single context' do
+ before do
+ backend = StateMachine::AuditTrail::Backend.create_for_transition_class(ActiveRecordTestModelWithContextStateTransition, :context)
+ end
+
+ let!(:state_machine) { ActiveRecordTestModelWithContext.create! }
+
+ it "should log an event with all fields set correctly" do
+ state_machine.start!
+ last_transition = ActiveRecordTestModelWithContextStateTransition.where(:active_record_test_model_with_context_id => state_machine.id).last
+ last_transition.context.should_not be_nil
@wvanbergen

wvanbergen Jul 26, 2012

Owner

Maybe you want to check this field for an actual value instead of just any value to make sure nothing weird is going on.

Owner

wvanbergen commented Jul 26, 2012

OK< I finally had time to go over your pull request. Except for some minor issues it's looking good.

Contributor

augustj commented Jul 26, 2012

I'll remove the comments and the unnecessary check in active_record.rb. I'll also beef up the test you mentioned. Thanks.

Contributor

augustj commented Jul 26, 2012

Would you like me to do this cleanup on my branch without rebasing/merging the current version of master?

Owner

wvanbergen commented Jul 26, 2012

The only thing I care about is being able to do a clean merge :)

August Jaenicke added some commits Jul 26, 2012

August Jaenicke Merge branch 'master' of github.com:augustj/state_machine-audit_trail 81a2174
August Jaenicke Added configurable support to log extra attributes in the audit trail…
…. Only supports ActiveRecord at the moment.

Added instructions to README on how to use context logging feature
3c7538e
August Jaenicke Removed comments. Improved test for storing context on transition. 378569f

@wvanbergen wvanbergen added a commit that referenced this pull request Jul 26, 2012

@wvanbergen wvanbergen Merge pull request #6 from augustj/add_message_to_trail
Added configurable support to log extra state machine attributes in the audit trail
771f0da

@wvanbergen wvanbergen merged commit 771f0da into wvanbergen:master Jul 26, 2012

Owner

wvanbergen commented Jul 26, 2012

Merged. Thanks for your patience!

Owner

wvanbergen commented Jul 27, 2012

Not sure why Travis doesn't give the green light; the specs run fine on my local machine.
Seems to be related to the mongoid gem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment