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
Updated frontend to send a single bulk delete request instead of one request for each record #2985
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this pr, @thesujai. It mostly looks great. There's one more thing I'd like to handle though...
It's possible that attempting to delete records will fail at the DB layer. For example, you might be trying to delete a record which is referenced by other records. We don't yet support deleting via CASCADE, so the related records will need to be deleted first.
-
Before this PR:
Failure to delete such records had decent error behavior, allowing the user to understand what happened and understand the steps needed to fix it.
-
After this PR:
When a single record is being deleted, the UI handles errors nicely. Good!
But when multiple records are being deleted, the UI does not handle errors very well.
Changes I'd like to see to improve the problematic error scenario noted above
- All loading spinners should clear when the request resolves.
- The error message from the API should display in the toast message. (No need to display it in the row header cells, because the API doesn't give us per-row error messages.)
Hey @seancolsen, Thank you for the review Earlier when multiple rows were selected containing ones with foreign references, some were deleted leaving the ones with foreign reference untouched. But with the new changes if the selection contain any foreign reference row, it will not delete any of the rows(Which i think would be more safe) |
- Use `pluralize` function. - Small readability improvements. - Organize imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4bee347
Fixes #2701
Technical details
mathesar_ui/src/api/utils/requestUtils/DeleteAPI
with optional argument for handling bulk delete request.mathesar_ui/src/stores/table-data/records/deleteSelected
for sending single bulk delete request instead of one request for each record.Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin