Closed mariogmz closed 3 years ago
I'm not particularly certain about this approach. I've had bad experiences with the various global soft-delete gems over the years, but I'm open to disucssion with the rest of the group around that. And I'd have to take time to review the gem you chose; I've never heard of it, and we're trying to avoid taking on dependencies that we do not have to.
However, there are a couple of things in the PR I'm going to consider blockers:
Please, do not change configurations, particularly if they are unrelated to the subject of the PR, without discussing it with the project as a whole.
This also goes for the testing structures; we considered using controller testing, but we're following up on the fact that Rails considers it to be obsolete, and increasingly inaccessible. Sticking with request specs is the better way to go. rails-controller-testing is a transition to avoid breaking older projects, not an invitation.
Gems that go through the effort of providing their functionality as a mixin do so specifically so that they can avoid taking up the inheritance slot. Please don't discount that choice by altering the ancestry of every model in the project. If there are things that we want soft-deletable, we can include the module in the record types where we want that behavior, without sticking an obfuscating layer of inheritance in the middle.
As for why I'm not keen on bare soft-delete gems; outside of the historical issues with scoping and associations, the issue I have is that they tend to steer you away from actually thinking about your domain. Being "removed" can mean very different things to different parts of your domain model, and instead of encouraging the exploration of that nuance, they steer you towards a simplistic "removed at this time", which doesn't necessarily capture any information you may later need.
This is partially my fault, because I wrote a very vauge issue looking for this; but I think I'm mostly looking towards making sure that the interface does not present a way to remove records. In cases where a removal does happen, we need to consider what that means, and what the implications are for the rest of the model. A simplistic record like the RestorationActivityLogEntry is probably okay to get a soft-delete like this. A Zone is not.
@cflipse I understand, so, since your concerns are big in this one, please let me know the way you want this to be implemented, if makes you feel maybe less confused, you can check the repo for the discard
gem I added: https://github.com/jhawthorn/discard.
If my approach it's not the correct one, feel free to close this PR and maybe rework based in some other issue with more details about the needs of this project.
Resolves #61
Description
Now many of the app models implement a
soft delete
approach, so there's only a logical delete inside the database and we don't loose any important info.Type of change
Added the
discard
gem forsoft deletes
, which adds convenient methods to query for undiscarded o discarded records.How Has This Been Tested?