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

Make audited class method idempotent #731

Open
arcreative opened this issue Sep 11, 2024 · 3 comments · May be fixed by #734
Open

Make audited class method idempotent #731

arcreative opened this issue Sep 11, 2024 · 3 comments · May be fixed by #734

Comments

@arcreative
Copy link

arcreative commented Sep 11, 2024

This is an issue that's caused major headaches for me in many of my projects, because audited can only be called once. Calling it a second time does not function as intended, and worse, it doesn't error out if it's been called prior.

The use case is simple--I want all my models to be audited unless I opt out of the process. Whether its my careless mistake or a team member that's unaware of it, it's quite easy to miss calling audited when you create a new model. To solve this problem, I've created an ApplicationRecord class that calls audited if: :audit?, from which all other classes inherit, the default class' audit? method returns true, and I can opt out on a per-model basis. Cool.

Problem 1

Options can't be amended for models that need to use them. If I want to use only:/except:/associated_with:, these options are totally ignored on the second invocation. So now I have several years' worth of audit history all clogged up with ephemeral attributes that I didn't want to trigger an audit record.

Problem 2

self.primary_key = :name is only respected if called before audited is called. So now I have several years' worth of audit history with NULL auditable_id attributes, and have no idea what records they actually belong to.

Solution

Make the audited call idempotent so we can call it more than once. The options from the latest invocation should be respected, or at the very least, trigger an error to warn of unintended consequences. Many of the calls in the audited class method are already idempotent, but I would like the options to be respected or merged so we can call this on an abstract model, and call it again on the concrete model if needed.

@mohammednasser-32
Copy link

Hey @danielmorrison, just checking before jumping in..can I create a PR for this?
I can see in the code here that it explicitly mentions don't allow multiple calls, so maybe it is intentional
However I believe updating the options attribute on multiple calls -while logging a warning- would be useful, so if it is okay I can add the change.
Thanks!

@danielmorrison
Copy link
Member

I don't think it was intentional, but probably what was easy at the time. I'd love to see a PR for it.

@mohammednasser-32 mohammednasser-32 linked a pull request Oct 21, 2024 that will close this issue
@mohammednasser-32
Copy link

Done, here is the PR 🙏

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 a pull request may close this issue.

3 participants