Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit touch calls #657

Merged
merged 10 commits into from
Feb 14, 2023
Merged

Conversation

mcyoung
Copy link
Contributor

@mcyoung mcyoung commented Feb 13, 2023

Touch is a form of update which generally doesn't call callbacks. It does, however, invoke an after_touch callback that can be leveraged to store away audit information. This can be informative with fields such as locked_at housing a timestamp for when a user account is locked and nil when the user's account is unlocked. Having historical information about when a user was locked/unlocked and auditing comments around the why, is handy.

@@ -324,6 +330,13 @@ def audit_update
end
end

def audit_touch
unless (changes = audited_changes(for_touch: true)).empty?
write_audit(action: "update", audited_changes: changes,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The action could be set to "touch", but I left it as an "update" since the underlying change is effectively updating a timestamp from 1 value to the current time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you made the right call.

Copy link
Member

@danielmorrison danielmorrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Let's get those tests passing.

spec/audited/auditor_spec.rb Outdated Show resolved Hide resolved
lib/audited/auditor.rb Outdated Show resolved Hide resolved
@mcyoung
Copy link
Contributor Author

mcyoung commented Feb 14, 2023

@danielmorrison Looking into the spec failures, it seems that Rails 5 -> 6 had a change under the hood w/ touch that impacted how previous_changes are reported. Do you have feelings/thoughts on adjusting this to be a 6+ feature?

@danielmorrison
Copy link
Member

Do you have feelings/thoughts on adjusting this to be a 6+ feature?

That seems like a fine solution. We already have checks for ActiveRecord::VERSION::MAJOR >= 6.

@danielmorrison danielmorrison merged commit 944f0b8 into collectiveidea:main Feb 14, 2023
@jasonperrone
Copy link

How do I opt out of this on a given model? I have a model with an associated audit like so:

class RolesUsers < ApplicationRecord
 
  audited associated_with: :user, on: [:create, :destroy]
  
  belongs_to :role
  belongs_to :user, touch: true

end

When creating a RolesUsers record it touches :user as you can see. I don't want that to result in an audit record for user. How do I stop this change from happening?

The reason I want that is that my workflow creates a user instance and then add a RolesUsers to a collection on user and then saves user. This normally yields a create audit on user and a create audit on roles_users. But I am subsequently getting an additional update audit on the user I just created. I don't need the create AND the update audit in this situation.

@mcyoung
Copy link
Contributor Author

mcyoung commented Feb 18, 2023

How do I opt out of this on a given model? I have a model with an associated audit like so:

class RolesUsers < ApplicationRecord
 
  audited associated_with: :user, on: [:create, :destroy]
  
  belongs_to :role
  belongs_to :user, touch: true

end

When creating a RolesUsers record it touches :user as you can see. I don't want that to result in an audit record for user. How do I stop this change from happening?

The reason I want that is that my workflow creates a user instance and then add a RolesUsers to a collection on user and then saves user. This normally yields a create audit on user and a create audit on roles_users. But I am subsequently getting an additional update audit on the user I just created. I don't need the create AND the update audit in this situation.

@jasonperrone Great call out, and I've noticed this as well and am working towards a fix. For now, I think you should be able to update your audit call on the user to be audited on: %i[create update destroy] to exclude the new touch audit call.

@jasonperrone
Copy link

Makes sense, thanks @mcyoung

@mcyoung
Copy link
Contributor Author

mcyoung commented Feb 21, 2023

#660 should help to resolve double auditing for touches.

@jasonperrone
Copy link

Cool @mcyoung , thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants