Soft delete for spaces and components#13297
Conversation
alecslupu
left a comment
There was a problem hiding this comment.
Can you have a look, on the latest changes?
...icipatory_processes/spec/system/admin/admin_manages_participatory_process_components_spec.rb
Outdated
Show resolved
Hide resolved
...icipatory_processes/spec/system/admin/admin_manages_participatory_process_components_spec.rb
Outdated
Show resolved
Hide resolved
decidim-participatory_processes/spec/controllers/admin/components_controller_spec.rb
Outdated
Show resolved
Hide resolved
decidim-participatory_processes/spec/controllers/admin/components_controller_spec.rb
Outdated
Show resolved
Hide resolved
decidim-budgets/spec/controllers/decidim/budgets/admin/budgets_controller_spec.rb
Outdated
Show resolved
Hide resolved
decidim-budgets/app/views/decidim/budgets/admin/projects/update_attribute.js.erb
Show resolved
Hide resolved
alecslupu
left a comment
There was a problem hiding this comment.
While reviewing this PR, i find a lot of places, where a potential bug can be introduced, as we have a lot of "Model.where" that needs to be patched to something like Model.not_trashed.where.
This is not the entire big problem, because this can be found and patched to adopt the new scope. What's more problematic in my view are the associations, or better said the places where we call object.association, and the object found in the association is being soft deleted.
One way i see for fixing this kind of issues is to actually rely on the rails default_scope parameter.
Other way of handling this, could be involving a gem like paranoia that would handle that for us.
In the future, we could have a combination of the 2 ways from above. We could have the soft delete done with paranoia and other behaviors added via default scope.
In a model we could have the following structure :
class MyModel < ActiveRecord::Base
default_scope { published }
default_scope { not_hidden }
default_scope { not_deleted }
end
As per ActiveRecord documentation, If you use multiple default_scope declarations in your model then they will be merged together:
Of course, there are a few downsides to this :
- the resources will always follow the default scopes set behavior ( good )
- we will need to patch all the admin area do add
unscopedto the queries that are relevant to us, then add the required scopes manually ( something in the line ofMyModel.unscoped.not_hidden.not_deletedto remove the published behavior ) - we may need to patch a few frontend actions ( just to make sure that an admin can view an unpublished resource )
I guess that is a very good moment to take a decision whether we start using default scopes or any library. Taking this kind of decision would reduce the back and forth on reviewing this PR, as well is going to keep it as small as possible.
Since we do not want ending up in a situation where we referencing any deleted records to end user, we need to patch or investigate the following occurrences:
Budget.~ 35 occurrencesbudget.~ 117 occurrences.budget~ 93 occurrencesProject.~ 45 occurrencesproject.~ 225 occurrences.project~ 77 occurrencesProposal.~ 1
Considering that we currently have 15 module, if we consider an additional 20 files changed for each module to do the necessary patches, we may need to patch an additional 300 files to have some level of certainty that we don't add the bug.
cc @andreslucena , @microstudi
|
We talked about @alecslupu concern in our weekly Maintainers' meeting, and we agreed that the best solution is to implement the paranoia gem, so we don't need to review the whole application for the scopes change, and just take into account the edge cases where we want to have access to the deleted models. For instance, we may want to have access to the deleted resource in admin's Moderations, so we need to use the unscoped scope. We agreed that for not blocking more this PR, this is something that @decidim/maintainers (@alecslupu in particular) will implement after merging this PR. |
Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
…e_attribute.js.erb Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
…s_participatory_process_components_spec.rb Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
…s_participatory_process_components_spec.rb Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
alecslupu
left a comment
There was a problem hiding this comment.
Great work @antopalidi !
Looking good to me in the form it is. Will merge after I have the paranoia configured
* Install paranoia gem * Patch migrations * Remove old methods * Remove usage of not_trashed * Remove trashed? method * Fix specs * Add specs for behavior * Running linters * Fix specs
|
@alecslupu nice work with the Paranoia gem, I've merged so you can finish this. Thanks again |
| <% else %> | ||
| <span class="action-space icon"></span> | ||
| <% end %> | ||
| <% if allowed_to? :copy, :assembly, assembly: assembly, assembly: parent_assembly %> |
There was a problem hiding this comment.
@antopalidi which is the correct argument here? The assembly argument is duplicated.
🎩 What? Why?
This pull request implements the soft delete functionality for spaces, components, and resources within the administration panel. Instead of permanently deleting these elements, admins now have the option to send them to a recycle bin, allowing for restoration if needed. This feature enhances administrative flexibility.
📌 Related Issues
🔨 How?
deleted_atcolumn to the relevant models.AddTimestampsToComponentsmigration to use a temporary class within the migration. This change avoids errors caused by using theDecidim::Componentmodel directly, which includes the SoftDeletable concern that relies on thedeleted_atcolumn.Resources:
✅ Testing
📷 Screenshots
Please add screenshots of the changes you are proposing