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

Touch Audit - Ensure previous auditing #662

Merged

Conversation

mcyoung
Copy link
Contributor

@mcyoung mcyoung commented Feb 21, 2023

If records are looking to audit for touch, but have yet to be audited, or are specifically auditing just touch events, ensure we're not failing due to a bad previous audit check.

@mcyoung mcyoung marked this pull request as ready for review February 21, 2023 21:10
@mcyoung mcyoung closed this Feb 21, 2023
@mcyoung mcyoung reopened this Feb 21, 2023
Check for audits
@mcyoung mcyoung force-pushed the mcy/feature/safely-touch-audit branch from 241f8a0 to 829bdb8 Compare February 21, 2023 22:23
@mcyoung
Copy link
Contributor Author

mcyoung commented Feb 21, 2023

@danielmorrison Who woulda known touch events were such touchy things...

@danielmorrison danielmorrison merged commit 874f291 into collectiveidea:main Feb 22, 2023
Comment on lines 250 to +252
if for_touch
filtered_changes.reject! do |k, v|
next unless audits.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be

if for_touch && audits.present?
  ...
end

What is the point in iterating over the changes?

@@ -249,6 +249,8 @@ def audited_changes(for_touch: false)

if for_touch
filtered_changes.reject! do |k, v|
next unless audits.present?

audits.last.audited_changes[k].to_json == v.to_json ||
audits.last.audited_changes[k].to_json == v[1].to_json
Copy link
Contributor

Choose a reason for hiding this comment

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

also it makes sense here to get audits.last.audited_changes once instead of twice per iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, updated in #668

@akostadinov
Copy link
Contributor

I would actually suggest to enable touch callback only if requested. It would usually not make sense to have it. Also it adds database queries to the audit which will add even more latency to responses.

@JDrizzy
Copy link
Contributor

JDrizzy commented Jul 17, 2023

Also it adds database queries to the audit which will add even more latency to responses

This scenario actually bit me when pushing the upgrade to production. Locally, the audits table only had a thousand records whereas production has millions 💥

If we could disable this callback in the config that would be awesome, as this would avoid the same scenario occurring if touch was forgotten from the options callback list. I can add this change if others agree it's needed? I was thinking of adding something that extends to all callbacks as well as it might be beneficial for others.

For example:

# audited.rb

Audited.config do |config|
  config.ignored_callbacks = [:create, :touch]
end

#####

# auditor.rb

after_create :audit_create if audit_callback?(:create)
...
after_touch :audit_touch if ::ActiveRecord::VERSION::MAJOR >= 6 && audit_callback?(:touch)

def audit_callback?(callback)
  return false if Audited.ignored_callbacks.include?(callback)

  audited_options[:on].include?(callback)
end

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.

4 participants