Skip to content

Soft delete for spaces and components#13297

Merged
alecslupu merged 129 commits intodevelopfrom
feature/soft-delete-for-spaces-components
Nov 18, 2024
Merged

Soft delete for spaces and components#13297
alecslupu merged 129 commits intodevelopfrom
feature/soft-delete-for-spaces-components

Conversation

@antopalidi
Copy link
Copy Markdown
Member

@antopalidi antopalidi commented Aug 22, 2024

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

  • Added a deleted_at column to the relevant models.
  • Updated the AddTimestampsToComponents migration to use a temporary class within the migration. This change avoids errors caused by using the Decidim::Component model directly, which includes the SoftDeletable concern that relies on the deleted_at column.
  • Implemented soft delete methods and associated logic to handle the moving of items to the recycle bin:
    • Participatory Processes
    • Assemblies
    • Conferences
    • Components
      Resources:
      • Proposals
      • Accountability
      • Budgets
        • Projects
      • Blogs
      • Debates
      • Meetings
  • Updated the UI to include a trash can icon for delete actions.
  • Modified the global search to exclude deleted items.
  • Added specs to test the new functionality.

✅ Testing

  1. Go to the administration panel.
  2. Attempt to delete a space or component.
  3. Verify that the item moves to the recycle bin.
  4. Check if the item can be restored from the recycle bin.
  5. Ensure that deleted items do not appear in the global search.
  6. Confirm that a notification is sent to resource authors upon deletion.

📷 Screenshots

Please add screenshots of the changes you are proposing

Description

♥️ Thank you!

github-actions[bot]
github-actions bot previously approved these changes Aug 22, 2024
github-actions[bot]
github-actions bot previously approved these changes Aug 23, 2024
@antopalidi antopalidi added the project: 2024-developments Barcelona City Council contract label Aug 23, 2024
github-actions[bot]
github-actions bot previously approved these changes Aug 25, 2024
github-actions[bot]
github-actions bot previously approved these changes Aug 26, 2024
github-actions[bot]
github-actions bot previously approved these changes Aug 28, 2024
github-actions[bot]
github-actions bot previously approved these changes Aug 28, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 12, 2024
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have a look, on the latest changes?

Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unscoped to the queries that are relevant to us, then add the required scopes manually ( something in the line of MyModel.unscoped.not_hidden.not_deleted to 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 occurrences
  • budget. ~ 117 occurrences
  • .budget ~ 93 occurrences
  • Project. ~ 45 occurrences
  • project. ~ 225 occurrences
  • .project ~ 77 occurrences
  • Proposal. ~ 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

@andreslucena
Copy link
Copy Markdown
Member

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>
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
…e_attribute.js.erb

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
…s_participatory_process_components_spec.rb

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
…s_participatory_process_components_spec.rb

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 13, 2024
alecslupu
alecslupu previously approved these changes Nov 13, 2024
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @antopalidi !

Looking good to me in the form it is. Will merge after I have the paranoia configured

@alecslupu alecslupu mentioned this pull request Nov 15, 2024
* 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
@microstudi
Copy link
Copy Markdown
Contributor

@alecslupu nice work with the Paranoia gem, I've merged so you can finish this. Thanks again

Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

<% else %>
<span class="action-space icon"></span>
<% end %>
<% if allowed_to? :copy, :assembly, assembly: assembly, assembly: parent_assembly %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antopalidi which is the correct argument here? The assembly argument is duplicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

Soft delete for spaces and components

8 participants