new audit created when only tag is different

[mstate]

Hi Harley,

Great gem!!! Thanks so much for your excellent work.

I've run into a bit of a problem that I'm hoping you can help with. I would like to be able to have auditable evaluate whether or not to create a new audit without taking into consideration the tag's value. Let me explain why I think this may be preferable and why it doesn't negate possible other use cases.

First, a bit about what I'm using auditable for...
I'm using auditable to track changes to orders. When an order has non-blank audited_changes, I tag it with a unique number. The warehouse staff use this unique number to track the version of the order they are picking/packing/shipping/etc. These orders are constantly be saved and re-saved, but not often modified as they are imported/re-imported frequently from our e-commerce provider. If the order changes while it's in-process, a new audit is stored and tagged with a new unique number. If the order's audited attributes have not changed, I don't want to store a new audit. I believe that this is the planned default behaviour of auditable - not to store audits when things have not changed.

However, as I'm using auditable to track the changes, I only know about whether the order has changed (since the tagged audit ), after the order has been saved (updated). I use the after_save and after_touch callbacks to tag the newest audit with a unique number if there have been changes.

The problem I'm having is that, even though my audited_attributes have not changed (neither has the action or user), I still get a new audit every time I try to save because the tag is nil by default and the previous audit had a non-nil tag.

I would like to be able to have auditable evaluate whether or not to save a new audit without taking into consideration the tag's value.

I can see a use case where only updating the tag and saving might be valuable, but it's pretty far-fetched and easy to implement in other ways. The example I was thinking of is something like a regulatory_compliance issue where someone would want to have a record of an object's state every hour or day. They would simply run an update_attributes(:tag=>Time.now) every hour and have a record, even though nothing changed. However, I don't think this was the intent of auditable. Rather, it seems as though your focus was really on tracking whether specific things have changed i.e. the specified attributes and methods. It would be easy enough to implement this regulatory compliance-like use case with a simple method that reported the current time. The user could then just save their object every hour and would have a new audit every hour.

Would you be willing to delete the 'tag' from the list in the relevant_attributes method? Or, alternatively to have a way to override the list in the relevant_attributes method with a custom list?

Sorry for the lengthy email, but I really wanted to be clear. I hope I was.

Cheers and thanks for your consideration.
Mike

[harley]

hi @mstate thanks for the great writeup, and nice to hear you like this simple gem. i'd be more than happy to incorporate this in for you. i don't have much time by the computer today but I will do this some time this week. if you want to discuss it further, feel free to bring it up here. some test cases to make the specs clearer would also help. no pressure, i'll take a look and keep you posted. cheers

[mstate]

Thanks Harley.

Below is the spec blog I changed for the use case. It currently fails on
the code, but passes if you delete "tag" from the list in the
relevant_attributes method.

context "no changes on audited attributes" do
it "should not create new audits" do
survey.save
expect { survey.save }.to_not change { survey.audits.count }
end

it "should not create new audits when only tag changes" do
  survey.update_attributes!(:audit_tag => 'first tag')
  expect { survey.update_attributes(:audit_tag => 'this should not

create a new audit') }.to_not change { survey.audits.count }
end
end

Cheers,
Mike

On Mon, Apr 16, 2012 at 12:47 PM, Harley Trung <
reply@reply.github.com

wrote:

hi @mstate thanks for the great writeup, and nice to hear you like this
simple gem. i'd be more than happy to incorporate this in for you. i don't
have much time by the computer today but I will do this some time this
week. if you want to discuss it further, feel free to bring it up here.
some test cases to make the specs clearer would also help. no pressure,
i'll take a look and keep you posted. cheers

---

Reply to this email directly or view it on GitHub:
#7 (comment)

[seanlinsley]

First off, thanks for making this gem @harleyttd! I'm glad that I don't have to do all the work myself ;-]

The change was actually quite simple, as seen in PR 9. The odd part is that (apparently) you implemented an imperfect fix in Issue #2. The weird thing is that the currently existing spec:

  context "no changes on audited attributes" do
    it "should not create new audits" do
      survey.save
      expect { survey.save }.to_not change { survey.audits.count }
    end
  end

doesn't fail, even though tons of extra audit records are created in practice. Manual testing led me to discover that when running:

u = User.create
u.save
u.save
# etc

The first r.save would not create a new audit record, but every one after it would. No matter, my fix solves that.

The real issue is that my fix broke your specs. I expect that's because certain parts of the specs assume that snap! will be called, while my change prevents that from occurring.

[seanlinsley]

I'd rather not sidestep your code, so I'm searching through the logic for whatever's causing this problem. Here's an easy way to visualize it:

u = User.create
u.audits[-1].changes  # returns empty hash
u.audits[-1].changed? # returns false
u.save                # doesn't save, as nothing's changed
u.audits[-1].changed? # returns true
u.audits[-1].changes  # returns a hash, like below

{"action"=>[nil, "update"], 
"modifications"=>[nil, {"country_id"=>0, "group_id"=>0, "official"=>false}], 
"auditable_id"=>[nil, 24], 
"auditable_type"=>[nil, "Device"]} 

It turns out this is happening because you're using audits.build, which immediately adds itself to the u.audits array, regardless to whether it ends up being saved to the database. Here's a simple fix:

### In /lib/auditable/auditing.rb, near line 73
if !audit.same_audited_content?(last_saved_audit)
  audit.save
else
  audits.delete(audit)
end

I added this change to my Pull Request.

[harley]

finally got down to it (was away for too long). released the new gem version. thanks @daxter and @mstate.

@mstate i don't know if you still use this, but i think tag should still be in relevant_attributes because i think it's better to have the history of the tag names in the database (example) -- there's also audit_tag_with which unfortunately seems to do the reverse of what you want (override and keep the last tag name instead of first name)

if you still want it, feel free to make relevant_attributes configurable if you want and i will merge that in. i'm closing this issue for now