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

Optionally pass audited action to conditionals #754

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sebastianludwig
Copy link

I only want to record destructions under some circumstances. The proposed implementation allows users to optionally accept an action parameter in their condition method or Proc. I've implemented tests and updated the documentation. Looking forward to your feedback!

@@ -33,7 +33,7 @@ module ClassMethods
# audited except: :password
# end
#
# * +require_comment+ - Ensures that audit_comment is supplied before
# * +comment_required+ - Ensures that audit_comment is supplied before
Copy link
Author

Choose a reason for hiding this comment

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

Kind of related documentation fix: The private method is called require_comment, the option is called comment_required.

return true if condition.blank?
return condition.call(self) == matching if condition.respond_to?(:call)
return send(condition) == matching if respond_to?(condition.to_sym, true)
result = case condition
Copy link
Author

Choose a reason for hiding this comment

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

Rewritten in the style of evaluate_max_audits above

return condition.call(self) == matching if condition.respond_to?(:call)
return send(condition) == matching if respond_to?(condition.to_sym, true)
result = case condition
when Proc then condition.call(self, action)
Copy link
Author

Choose a reason for hiding this comment

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

I thought about passing action as Symbol but Audit.action is also a String so it's more consistent this way.

@sebastianludwig
Copy link
Author

sebastianludwig commented Mar 6, 2025

It would be easy to also incorporate #697 into this PR, but I'm not sure it's worth it. Using proc { MyApp::CONFIG[:audit_my_model] } as workaround seems simple enough to avoid the complexity. If you think otherwise, please let me know. I'm happy to also add that functionality (and tests).

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.

1 participant