Save deleted_by user when deleting products

[dacook]

Work in progress, but I'd like someone to validate if it's a good idea to proceed

What? Why?

This came up when trying to investigate why products got deleted.
https://openfoodnetwork.slack.com/archives/CDLKH9MM0/p1714272151621489

It seemed like a good idea to log the user that did the delete, so I thought I'd try and prove the concept. I was surprised to find it wasn't already a common pattern, and I think I found out why...

I wasn't sure the best way to set the user, while ensuring it got saved during deletion (and undone if delete fails). Overriding destroy seems like a bad idea but it seems to work too..

Perhaps another way would be to add it to an instance variable from the controller, then copy it during deletion:

# controller
product.to_be_deleted_by = current_user
product.destroy

# model (around_destroy)
destruction do
  transaction do
    # other stuff
    deleted_by = to_be_deleted_by
    yield
  end
end

What should we test?

• Visit ... page.

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

[dacook]

TODO: fix up discrepancies here

[rioug]

I am thinking we shouldn't have a default for deleted_by to enforce recording a user when deleting a product. But I am pretty sure it would blow up in various unexpected places.

[rioug]

I think the logic should be in a service, something like ProductDestroyer, then you don't need to override the destroy method.
The service can have all the logic to set/unset deleted_by , it should just be matter of wrapping the logic you used in a transaction block.

[mkllnk]

I have a different perspective:

• The database is holding the current state but we want to look at a log of actions.
• We are adding database constraints that could haunt us later. What if we wanted to delete an admin user who deleted products in the past?

I think that we should think about a logger instead. Spree is logging some stuff in database tables but I think that files are easier to manage. We could just add to the Rails log and make sure it's preserved for longer. Or we could start a separate logger for important actions like this.

The log is usually only written and never read. Only in rare circumstances when something goes wrong, we want to look at the log file.

[dacook]

I think that we should think about a logger instead.

Thanks, good point, this fits well into the category of logging.
We already log requests like DELETE /products/123, but if it got deleted via an association, it wouldn't be so easy to track (i forget if that's possible). So we could add additional logging for any records (soft-)deleted. This would easily apply to any type of record.
Should it be in the general log file? That's probably fine, because I think deletes don't happen that frequently. Otherwise it might be worth having a separate log file for deletes. It might be helpful logging other actions too, like edits.

The downside of file logs is that we can't easily expose it in the user interface, to let users solve their own questions. Hmm, but it shouldn't be too hard surely.. (eg click a button on the product, Rails greps the log file for that ID and returns the matches).

Ok so I'm sold on file logging.

[dacook]

I raised #12452 as an alternative