twenty icon indicating copy to clipboard operation
twenty copied to clipboard

☑️ Enable "Select All" for delete action

Open Bonapara opened this issue 1 year ago • 11 comments

### Tasks
- [ ] https://github.com/twentyhq/twenty/issues/4406
- [ ] https://github.com/twentyhq/twenty/issues/4409
- [ ] https://github.com/twentyhq/twenty/issues/4408
- [ ] https://github.com/twentyhq/twenty/issues/4407
- [ ] https://github.com/twentyhq/twenty/issues/4412
- [ ] https://github.com/twentyhq/twenty/issues/5169

Bonapara avatar Mar 11 '24 10:03 Bonapara

@FelixMalfait Assigning to gitstart FYI

charlesBochet avatar Apr 04 '24 12:04 charlesBochet

Here is the GitStart Ticket for this issue: https://clients.gitstart.com/twenty/5449/tickets/TWNTY-4397

gitstart-app[bot] avatar Apr 04 '24 12:04 gitstart-app[bot]

Ok perfect! To recap:

  • The current design "xxx Select : Delete / Export" is the right one, the count only refects the box selected on the frontend for now
  • So when we select all, then this count needs to come from the backend (already loaded and displayed on the view switcher)
  • The "export all feature" has already been implemented and it's in the options menu, so it's just about moving it. We decided to do it on the frontend with many calls to avoid large payloads coming from the backend
  • The "delete all" call was not implemented yet on the frontend yet. We should adopt a similar method than with the export, calling deleteMany and showing progress. Try to avoid duplicating too much code

FelixMalfait avatar Apr 04 '24 12:04 FelixMalfait

Hey @charlesBochet The tickets within this ticket have already been worked on, I guess it should be unassigned

gitstart-twenty avatar Apr 29 '24 09:04 gitstart-twenty

@gitstart-twenty no it's all yours! see my last message above.

I edited my message above though. Initially I told you that for createMany we could pass a large number of items and therefore there was no need to implement a looping behavior on the frontend. But @Bonapara pointed out that we did in fact have a limitation on the number of records on the backend, so we should actually adopt a similar behavior.

I renamed the issue for greater clarity on what's left to be done.

Let us know if you have any question!

FelixMalfait avatar Apr 29 '24 13:04 FelixMalfait

@gitstart-twenty no it's all yours! see my last message above.

I edited my message above though. Initially I told you that for createMany we could pass a large number of items and therefore there was no need to implement a looping behavior on the frontend. But @Bonapara pointed out that we did in fact have a limitation on the number of records on the backend, so we should actually adopt a similar behavior.

I renamed the issue for greater clarity on what's left to be done.

Let us know if you have any question!

Hey @FelixMalfait,

Thanks for the clarification. So, the actual ticket to work on is #5169?

gitstart-twenty avatar Apr 29 '24 13:04 gitstart-twenty

Hey @charlesBochet @FellipeMTX ,

Logic Breakdown

  • We need to know all the rowId's beforehand in order to delete all
  • In case user clicks select all button, update selectedRowIds to all the rowIds of the table
  • In case user unselects anything, update selectedRowIds to the visible rowIds

Questions

  1. Does the above logic make sense?
  2. Is there a mechanism to know all the rowIds of a table in case not all the records can fit in one page?

Screenshot 2024-04-30 at 14 37 10

gitstart-twenty avatar Apr 30 '24 11:04 gitstart-twenty

@gitstart-twenty

Is there a mechanism to know all the rowIds of a table in case not all the records can fit in one page?

For the total count, it's already retrieved and displayed besides the view. As for the looping mechanism, as mentioned I now think you should have a common implementation with "Export". We already loop and show percentage progress, let's try to share the code

FelixMalfait avatar Apr 30 '24 16:04 FelixMalfait

We also need to refactor the Export code that is doing a useFindManyRecords (reactive) to move to a sync trigger (lazyQuery or apollo.query) to avoid making an unnecessary call on page load btw :) See todo in useExportTableData.ts

charlesBochet avatar May 01 '24 08:05 charlesBochet

Also, if you don't "Select All" but just select a lot of records, then frontend should be smart enough to figure out it should go through the loop path (doing several requests) instead of throwing this: Screenshot 2024-05-02 at 13 43 53

FelixMalfait avatar May 02 '24 11:05 FelixMalfait

Alright

gitstart-twenty avatar May 03 '24 08:05 gitstart-twenty