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

feat: soft delete #6576

Merged
merged 25 commits into from
Aug 16, 2024
Merged

feat: soft delete #6576

merged 25 commits into from
Aug 16, 2024

Conversation

magrinj
Copy link
Member

@magrinj magrinj commented Aug 8, 2024

Implement soft delete on standards and custom objects.
This is a temporary solution, when we drop pg_graphql we should rely on the softDelete functions of TypeORM.

@magrinj magrinj marked this pull request as ready for review August 8, 2024 08:41
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

The pull request introduces soft delete functionality across various modules and entities, ensuring records are marked as deleted without being permanently removed from the database.

  • GraphQL Schema Updates: packages/twenty-front/src/generated-metadata/graphql.ts updated to support new mutations and enums for soft delete.
  • Migration Script: packages/twenty-server/src/database/typeorm/metadata/migrations/1723038077987-addSoftDelete.ts adds a softDelete column to the objectMetadata table.
  • Service Modifications: packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts updated to handle softDelete in findMany, findOne, deleteMany, and deleteOne methods.
  • Entity Updates: packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.entity.ts and packages/twenty-server/src/engine/twenty-orm/base.workspace-entity.ts updated to include softDelete and deletedAt fields.
  • Decorator Enhancements: packages/twenty-server/src/engine/twenty-orm/decorators/workspace-custom-object.decorator.ts updated to include a softDelete option.

24 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

icon: 'IconCalendarMinus',
})
@WorkspaceIsNullable()
deletedAt?: Date | null;
Copy link
Member

Choose a reason for hiding this comment

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

When I look at a production workspace it seems this column was created already by I don't understand why!? I'm missing something

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, on custom and standard objects ? I'm not having them on my computer

Copy link
Member

@Weiko Weiko Aug 8, 2024

Choose a reason for hiding this comment

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

@FelixMalfait @magrinj probably because it's defined there as a default column for custom table creation https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/engine/workspace-manager/workspace-migration-runner/utils/custom-table-default-column.util.ts
Originally I even put in the migration-runner in advance so it would have affected standard objects as well (and probably why you see some in prod) but it has been removed after many refactoring of this part I guess.

Copy link
Member

Choose a reason for hiding this comment

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

that was my initial assumption but then why aren't position or name also created on every table?

Copy link
Member

Choose a reason for hiding this comment

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

Even for recent standard objects from v0.23 I see deletedAt column but I don't see position (e.g. NoteTarget)

@Weiko
Copy link
Member

Weiko commented Aug 8, 2024

@magrinj it seems your filter is only on the first level of the queries.
For example if I fetch all the tasks of a company, tasks will be nested and it will probably return deleted tasks at that point.
Can we instead update the query builder (in the recursive functions we have) to inject a filter on each level (checking if the object has isDeletable) with deletedAt: null? Do you think it would be possible?
cc @FelixMalfait

@magrinj magrinj marked this pull request as draft August 12, 2024 16:00
@FelixMalfait
Copy link
Member

Would be nice to be able to show trash icon also on saved views
Screenshot 2024-08-13 at 10 20 14

// Why do we have two types, Filter and ViewFilter?
// Why key defition is already present in the Filter type and added on the fly here with mapViewFiltersToFilters ?
// Also as filter is spread into viewFilter, definition is present
// FixMe: Ugly hack to make it work
Copy link
Member

Choose a reason for hiding this comment

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

@lucasbordeau or @charlesBochet maybe you have context on this?

@FelixMalfait
Copy link
Member

TODO : invalidate cache of trashed items query when an item is deleted? If I go to trash, then delete an item, then go back to trash, that item doesn't show up

@FelixMalfait
Copy link
Member

Still a CSS issue on smaller screens:
Screenshot 2024-08-16 at 10 52 19

@FelixMalfait
Copy link
Member

Also I suspect withSoftDeleted is overly complex.
We want to apply the softDelete filter on every relations, always.
Because the parent record is soft-deleted, doesn't mean that when we display it we should display its soft-deleted childs. We should display the non-soft deleted childs.

It's not a blocker for merge, I think we should merge asap, even with this, and create a follow-up issue.

@lucasbordeau lucasbordeau marked this pull request as ready for review August 16, 2024 18:18
@lucasbordeau
Copy link
Contributor

lucasbordeau commented Aug 16, 2024

Working, I'm merging like this but we should review the following things :

  • Add restore / destroy instead of delete on table selection when we are on trash view
  • Brainstorm with @charlesBochet on Trash view, how should it we implement it in the frontend ?
  • Implement optimistic rendering for delete / restore / destroy
  • Refactor ViewFilter / Filter ?
  • Refactor SubMenuTopBarContainer and around.

@lucasbordeau lucasbordeau merged commit db54469 into main Aug 16, 2024
8 of 14 checks passed
@lucasbordeau lucasbordeau deleted the feat/soft-delete branch August 16, 2024 19:20
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This pull request continues the implementation of soft delete functionality for standard and custom objects in the Twenty application. Key changes include:

  • Added new resolver factories DestroyManyResolverFactory and RestoreManyResolverFactory to support soft delete operations.
  • Implemented InformationBannerDeletedRecord component for displaying and managing deleted records in the UI.
  • Updated query builders and resolvers to handle soft-deleted records, including new withSoftDeleted options.
  • Refactored settings pages for improved navigation and consistency across the application.
  • Removed ExecuteQuickActionOnOneResolverFactory, suggesting a shift away from quick actions in favor of the new soft delete approach.

Key points to note:

  • New destroyMany and restoreMany operations added to support soft delete functionality.
  • UI components updated to handle and display soft-deleted records, improving user experience.
  • Query builders now consider isSoftDeletable flag when constructing queries.
  • Settings pages reorganized for better navigation, with consistent use of icons and breadcrumbs.
  • Removal of quick action related code suggests a significant change in how record actions are handled.

298 file(s) reviewed, 24 comment(s)
Edit PR Review Bot Settings

@FelixMalfait
Copy link
Member

Thanks a lot @lucasbordeau!!!! Great if you can do the followups or at least create issues to make sure we don't forget any

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

Successfully merging this pull request may close these issues.

5 participants