-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
Log who deleted products on current bulk edit products screen #12457
Log who deleted products on current bulk edit products screen #12457
Conversation
…t log destroy action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mohammed, great to see you! Thanks, yes I think this is good 👍
In my other PR I tried overwriting the destroy
method so that you can pass the user_id as a parameter. It's a little cleaner and seems to be fine, but your approach of using standard rails callbacks is probably safer in the long run.
Just a little rubocop issue to fix.
25e3207
to
8ccb59a
Compare
@dacook the issue mention using a separate log file to record product deletion, the solution uses the usual log file are you happy with that ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's ok with me.
- Of course, it means more messages in the one file to sift through when you're debugging. But it could also be beneficial, giving you a bigger picture.
- Also, more messages could mean logs are rotated quicker. But I propose we focus on that separately.
PS this PR uses log level |
✅ Tested in dev environment:
I noticed a couple of things:
Maybe something to consider in the next round. |
Oops, I just realised @rioug didn't approve yet! Oh well, please comment on the associated issue if you have suggestions for the next step :) |
All good, I am happy with the code |
What? Why?
What should we test?
"#{self.class} #{id} deleted by #{destroyed_by.id}"
For example
To check the logs, you have two options:
cat PATH_TO_LOGS | grep "deleted by"
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates