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

☑️ Enable "Select All" for delete action #4397

Open
5 of 6 tasks
Bonapara opened this issue Mar 11, 2024 · 11 comments · May be fixed by #5320
Open
5 of 6 tasks

☑️ Enable "Select All" for delete action #4397

Bonapara opened this issue Mar 11, 2024 · 11 comments · May be fixed by #5320
Assignees
Labels
scope: back+front Issues requiring full-stack knowledge size: hours type: bug Something isn't working type: feature

Comments

@Bonapara
Copy link
Member

Bonapara commented Mar 11, 2024

Tasks

  1. good first issue size: hours type: bug
  2. scope: front size: minutes
  3. good first issue scope: front size: minutes type: bug
  4. scope: backend size: hours type: feature
    FelixMalfait
  5. scope: front size: hours
  6. scope: backend size: hours
@Bonapara Bonapara changed the title Select All/Unselect all Fails for Non-visible Records Make Select All/Unselect all work for non-visible records Mar 11, 2024
@Bonapara Bonapara changed the title Make Select All/Unselect all work for non-visible records ☑️ Refacto "Select All/Unselect all" on indexes Mar 11, 2024
@charlesBochet
Copy link
Member

@FelixMalfait Assigning to gitstart FYI

Copy link
Contributor

gitstart-app bot commented Apr 4, 2024

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

@FelixMalfait
Copy link
Member

FelixMalfait commented Apr 4, 2024

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

@gitstart-twenty
Copy link
Contributor

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

@FelixMalfait
Copy link
Member

FelixMalfait commented Apr 29, 2024

@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 FelixMalfait changed the title ☑️ Refacto "Select All/Unselect all" on indexes ☑️ Enable "Select All" for delete action Apr 29, 2024
@gitstart-twenty
Copy link
Contributor

@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
Copy link
Contributor

gitstart-twenty commented Apr 30, 2024

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

@FelixMalfait
Copy link
Member

@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

@charlesBochet
Copy link
Member

charlesBochet commented May 1, 2024

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

@FelixMalfait
Copy link
Member

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

@gitstart-twenty
Copy link
Contributor

Alright

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: back+front Issues requiring full-stack knowledge size: hours type: bug Something isn't working type: feature
Projects
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

5 participants
@FelixMalfait @charlesBochet @Bonapara @gitstart-twenty and others