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

Log who deleted products on current bulk edit products screen #12457

Merged

Conversation

abdellani
Copy link
Member

@abdellani abdellani commented May 9, 2024

What? Why?

What should we test?

  • Delete a product from the admin panel
  • Access the logs and check if you can find the following pattern:
"#{self.class} #{id} deleted by #{destroyed_by.id}"

For example

"Product XYZ deleted by USER_ID"

To check the logs, you have two options:

  1. use NewRelic, and run a query. For example: "deleted by" (I don't think that's configured on all the servers)
  2. Access the server directly and use a command like cat PATH_TO_LOGS | grep "deleted by"

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Copy link
Member

@dacook dacook left a 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.

@abdellani abdellani force-pushed the 12452-log-who-deleted-products branch from 25e3207 to 8ccb59a Compare May 17, 2024 14:43
@rioug
Copy link
Collaborator

rioug commented May 20, 2024

@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 ?

Copy link
Member

@dacook dacook left a 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.

@dacook
Copy link
Member

dacook commented May 20, 2024

PS this PR uses log level :info, which makes perfect sense.
I just checked and both staging and production environments are currently configured to :info, so this will ensure it is logged 👍

@dacook
Copy link
Member

dacook commented May 20, 2024

✅ Tested in dev environment:

10:29:43 rails.1   | Spree::Product 13 deleted by 1

I noticed a couple of things:

  1. So far this only logs when the user_id is provided. We could enhance it to always log even when the user_id is unknown (eg just "Spree::Product 13 deleted"`). Then it would be super easy to add the module to many models, and would cover all methods of deleting (it doesn't currently cover deleting from the new products_v3 screen).

  2. The user_id isn't named, so it's not clear what the number is. It might be helpful to also log the user's name or email. Eg: "Spree::Product 13 deleted by user 1 <example@email.com>"

Maybe something to consider in the next round.

@dacook dacook changed the title log who deleted products Log who deleted products on current bulk edit products screen May 20, 2024
@dacook dacook merged commit a9bf252 into openfoodfoundation:master May 20, 2024
50 checks passed
@dacook
Copy link
Member

dacook commented May 20, 2024

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 :)

@rioug
Copy link
Collaborator

rioug commented May 20, 2024

All good, I am happy with the code

@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Log who deleted products (and other records)
4 participants