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

allow calling audited multiple times #734

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mohammednasser-32
Copy link

Allow audited to be called multiple times

When audited is called for the first time, setup audit normally

However if audited is called on a class that is already audited we update audit_options and log a warning message

an example of the warning message logged:
2024-10-21_18-22

Fixes #731

@arcreative
Copy link

arcreative commented Oct 22, 2024

@mohammednasser-32 awesome, thanks for the quick turnaround! Just curious—if the updated options work the same as if it were originally called once with those options, maybe the warning is unnecessary (or can be optionally silenced)? My suggestion about the warning was in lieu of a fix so people wouldn’t hit the same pitfall as I did and not know it :-)

@mohammednasser-32
Copy link
Author

@arcreative you'r welcome :) also yes makes sense, adjusted here

@arcreative
Copy link

arcreative commented Oct 22, 2024

@mohammednasser-32 Cool, thanks! So just to confirm my use case, I generally call audited if: :audit? on my abstract ApplicationModel class (with an audit? method that returns true), and then I will call something like audited if: :audit?, except: %i[inconsequential_attribute] on the subclass. It seems in this situation, it would still warn me that audited has already been called because the options are different, but I don't think I would want this. My main concern is that audits scheme is strictly opt-out--I care less about the options than I do about all new models being audited by default, but I would want to be able to do this without it logging to STDOUT.

@mohammednasser-32
Copy link
Author

@arcreative so would it be better to remove the warning?

@arcreative
Copy link

I think so. My original suggestion for the warning was to tell people that it wasn't working as intended. If it's working as intended, then I think the warning is unnecessary.

@mohammednasser-32
Copy link
Author

@arcreative perfect, incorporated here

@mohammednasser-32
Copy link
Author

Hey @danielmorrison , since this already got approved I was just wondering if it is going to be merged soon? 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.

Make audited class method idempotent
3 participants