Refactor delete #3379

martijnb92 · 2020-12-04T20:25:26Z

This PR fixes the following:

[minor bug] When an item was deleted, Showing x to x of x entries was not updated. Fixed that by using the DataTables API to redraw the table
~~Removed (old?) call to close a modal. (I can't think of any modal that should be closed in the list view)~~
Refactor the delete button code by using some Sweetalert defaults. Which also makes the cancel button the default selected button (for data safety this is better).

tabacitu · 2020-12-08T19:52:34Z

Totally agree with all the decisions you've made here @martijnb92 - good work! It's definitely something we should do.

I'll let @promatik or @pxpm pick this up to test it & review it, afterwards I'll hit merge. This new process where everything needs to be reviewed by one of them takes a little longer, but it lets fewer mistakes slip, so it's better in the long run 😄

Thanks again, cheers!

pxpm

Left comments in code.

Thanks again @martijnb92

pxpm · 2020-12-09T13:17:45Z

src/resources/views/crud/buttons/delete.blade.php

 			              // Remove the row from the datatable
-			              row.remove();
+						  crud.table.row($("#crudTable a[data-route='"+route+"']").parents('tr')).remove().draw();


Hello @martijnb92

Thanks again for one more contribution. I was testing this out, and I am afraid that this is a change we cannot aford to do.

Redrawing the table is a very expensive operation, if you want to have a better understanding of what I mean load 200 entries in the table and delete one, using both versions of code. (plus the cost of drawing the table goes up as the number of columns go up too not only the number of entries).

Let me know @martijnb92 :)

I did some testing with a table with 350 entries and 10 columns, never did it take more than a second to redraw the table, even on my phone.
The old version of the code is a bit faster/instant, since it only removes the table row. But that means the totale entries line at the top is incorrect, since it’s not updated on delete.
That’s why in my opinion the re-draw option in datatable is a good idea, since it actually removes the data instead of just removing the rendered table row.

Thanks for clarification on this.

Unfortunatelly we are aware of some issues in Datatables that make some tables take some amount of time to load. It's a combination of the number of columns, type of columns, number of entries etc, so I must sustain what I said before:

the redraw option is only good in already fast tables, if you happen to have a table that takes 4 seconds to load, it will take 4 seconds to redraw (4 seconds it's not an HUGE issue in real big data cruds), but it will be 4 times faster to do it like previously than redrawing the table.

Let's keep in mind what you mentioned: the totale entries line at the top is incorrect, since it’s not updated on delete, are you aware of someway we could update this value upon deletion or is redrawing the only option ?

Thanks for your effort in solving this, I am pretty sure we will end this with a better solution than what we have in current backpack code 👍

Hmmm good thinking... both of you 😂

re-drawing the table is indeed more expensive;

that number is wrong, so... this is a bug; a small one, but a bug nonetheless;

Tricky one...

Ok so... here's how I see it:

Personally I don't see how we'd be able to redraw just that number, instead of redrawing the whole table. Not without breaking changes which are definitely not worth it.

I don't think it's worth trying to just change the number at the top, because in some cases, the count() operation is the most expensive one (think tables with millions upon millions of entries - over there getting 10 entries is fast, counting 99 million entries is what's expensive).

Decrementing that number with JS... yeah... that feels like a hack so I'd rather we not do that either.

Redrawing the datatable also prevents this one situation, that was annoying to me in the past: deleting a bunch of stuff one by one leads you to look at an empty table;

Redrawing the datatable also fixes an inconsistency we currently have:

Delete doesn't do a table redraw

BulkDelete does do a datatable redraw;
(so if you use both Delete and BulkDelete, the not-redrawing thing looks like a bug, not a feature)

To me, it looks like the redraw is more expensive indeed, but it's a reasonable drawback for fixing this bug (and a few other minor bugs in the process).

I think the ones considerably affected by the redraw will be CRUDs paginated by 100+ items per page and/or with a large number of columns. But if redrawing is a problem there, it means it was also a problem at the first page load. And upon search. And upon filtering. And upon reordering. So it's a problem worth fixing.

I say... let's redraw 🥳 I'm still open to being convinced otherwise @pxpm if you feel strongly about this.

That being said, I did find a bug with redrawing the table. And I've noticed it's also a bug inside the BulkDelete operation, so we could fix it there too.

If you're on page 33/100, and you delete an item, you'd expect the item to be deleted but stay on page 33/100. But no, the way this PR redraws the table, the user is sent to page 1. Which I think will make it super-frustrating to users.

I've checked filters, they are kept.
I've checked search, that is kept.
But the page number is not, that is reset upon datatable redraw.

src/resources/views/crud/buttons/delete.blade.php

promatik · 2020-12-09T15:13:12Z

This is an awesome change, because this delete button content is merged once for every crud table entry. So as much we can shorten this file, the better.

By my calculations, this small change can save around 14Kb/request (uncompressed) on a crud table with 20 entries/page.

Thank you for this @martijnb92 🙌

src/resources/views/crud/buttons/clone.blade.php

tabacitu · 2020-12-09T16:41:56Z

src/resources/views/crud/buttons/delete.blade.php

 			              // Remove the row from the datatable
-			              row.remove();
+						  crud.table.row($("#crudTable a[data-route='"+route+"']").parents('tr')).remove().draw();


That being said, I did find a bug with redrawing the table. And I've noticed it's also a bug inside the BulkDelete operation, so we could fix it there too.

If you're on page 33/100, and you delete an item, you'd expect the item to be deleted but stay on page 33/100. But no, the way this PR redraws the table, the user is sent to page 1. Which I think will make it super-frustrating to users.

I've checked filters, they are kept.
I've checked search, that is kept.
But the page number is not, that is reset upon datatable redraw.

martijnb92 · 2020-12-09T17:34:34Z

New commit to return the modal closing, totally missed it..
Also page will now be kept on redraw, datatables has a parameter for that

tabacitu · 2020-12-09T20:13:15Z

Excellent! Just tested this and it seems to me like all problems are fixed and it works great! Excellent job @martijnb92 .

I'll let @promatik or @pxpm give it one more test, maybe they find something I didn't. But it looks like this is super-close to merging 🥳

src/resources/views/crud/buttons/delete.blade.php

scrutinizer-notifier · 2020-12-10T09:57:58Z

The inspection completed: No new issues

pxpm

It seems good to me.

Thanks @martijnb92

promatik

Tested with non responsive tables, and some edge cases, like having only one entry on the last page, and everything works as expected.

Thank you for another great improvement @martijnb92 🙌

soufiene-slimi · 2020-12-17T10:36:47Z

Hi @tabacitu, @promatik, re-drawing the datatable in the show view will fire an error.

martijnb92 · 2020-12-17T10:50:21Z

Ooch.. I missed the show view..

Indeed crud does not exist in the show view.
We either need to update the script check if crud exists or revert this commit.

…ation - ref #3379

tabacitu · 2020-12-17T12:29:20Z

Ugh! Thanks a lot for the heads-up @soufiene-slimi ! Fixed in #3404, merged and tagged.
A composer update should have it fixed now.

Refactor delete and remove modal call

daf13f2

tabacitu added non breaking change Priority: SHOULD refactoring labels Dec 8, 2020

tabacitu requested a review from promatik December 8, 2020 19:53

pxpm self-assigned this Dec 9, 2020

pxpm requested changes Dec 9, 2020

View reviewed changes

tabacitu added Priority: COULD and removed Priority: SHOULD labels Dec 9, 2020

tabacitu requested changes Dec 9, 2020

View reviewed changes

Return modal and hold current page

03de647

martijnb92 changed the title ~~Refactor delete and remove modal call~~ Refactor delete Dec 9, 2020

tabacitu requested a review from pxpm December 9, 2020 20:13

tabacitu approved these changes Dec 9, 2020

View reviewed changes

promatik suggested changes Dec 10, 2020

View reviewed changes

src/resources/views/crud/buttons/delete.blade.php Outdated Show resolved Hide resolved

Remove 'remove', change order

114fd40

pxpm approved these changes Dec 10, 2020

View reviewed changes

promatik approved these changes Dec 10, 2020

View reviewed changes

tabacitu merged commit 4314558 into Laravel-Backpack:master Dec 14, 2020

tabacitu added a commit that referenced this pull request Dec 17, 2020

fix the JS error when clicking the Delete button inside the Show oper…

b6288c9

…ation - ref #3379

tabacitu mentioned this pull request Dec 17, 2020

fix the JS error when clicking the Delete button inside the Show oper… #3404

Merged

backpack-ai-bot bot mentioned this pull request Nov 19, 2024

[Bug] Not hide and not redirect after delete record #5718

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor delete #3379

Refactor delete #3379

martijnb92 commented Dec 4, 2020 •

edited

Loading

tabacitu commented Dec 8, 2020

pxpm left a comment

pxpm Dec 9, 2020

martijnb92 Dec 9, 2020

pxpm Dec 9, 2020

tabacitu Dec 9, 2020

tabacitu Dec 9, 2020

tabacitu Dec 9, 2020

promatik commented Dec 9, 2020

tabacitu Dec 9, 2020

martijnb92 commented Dec 9, 2020

tabacitu commented Dec 9, 2020

scrutinizer-notifier commented Dec 10, 2020

pxpm left a comment

promatik left a comment •

edited

Loading

soufiene-slimi commented Dec 17, 2020

martijnb92 commented Dec 17, 2020

tabacitu commented Dec 17, 2020 •

edited

Loading

Refactor delete #3379

Refactor delete #3379

Conversation

martijnb92 commented Dec 4, 2020 • edited Loading

tabacitu commented Dec 8, 2020

pxpm left a comment

Choose a reason for hiding this comment

pxpm Dec 9, 2020

Choose a reason for hiding this comment

martijnb92 Dec 9, 2020

Choose a reason for hiding this comment

pxpm Dec 9, 2020

Choose a reason for hiding this comment

tabacitu Dec 9, 2020

Choose a reason for hiding this comment

tabacitu Dec 9, 2020

Choose a reason for hiding this comment

tabacitu Dec 9, 2020

Choose a reason for hiding this comment

promatik commented Dec 9, 2020

tabacitu Dec 9, 2020

Choose a reason for hiding this comment

martijnb92 commented Dec 9, 2020

tabacitu commented Dec 9, 2020

scrutinizer-notifier commented Dec 10, 2020

pxpm left a comment

Choose a reason for hiding this comment

promatik left a comment • edited Loading

Choose a reason for hiding this comment

soufiene-slimi commented Dec 17, 2020

martijnb92 commented Dec 17, 2020

tabacitu commented Dec 17, 2020 • edited Loading

martijnb92 commented Dec 4, 2020 •

edited

Loading

promatik left a comment •

edited

Loading

tabacitu commented Dec 17, 2020 •

edited

Loading