fix datatable persistency when manually filtering

[pxpm]

WHY

BEFORE - What was wrong? What was happening before this PR?

Two things fixed here: [1] datatable persistency was not getting the filters re-applied if you manually filter the table by using a custom url http://backpack.com/admin/entity?filter=1 as reported in https://github.com/Laravel-Backpack/CRUD/issues/4360 and very well explained. [2] a bug with persistent attribute that would stick in the url (apparently without issues, but ugly anyway). https://recordit.co/SCalb9hMvf Notice the persistent-table attribute in the url bar as I move from page to page. At the end we endup with ?persistent-table=true&persistent-table=true and it would continue.

AFTER - What is happening after this PR?

Both errors are fixed.

HOW

How did you achieve that, in technical terms?

Re-implemented the updateUrl script to strip the persistent-table attribute and save only the correct url.

Is it a breaking change?

I don't think so.

How can we test the before & after?

Described in the examples.

[jcastroa87]

Hi @pxpm

I look deeply in this bug, as you describe works for the main problem, but i get a new bug didnt happen in main branch.

Here i go...

Probe on main, set filter from object

Go to update

Came back, filter persist and add "persistent-table=true"

Add filter by url

Go to update

Came back, lost filter

Now with "fix-datatable-persistency-when-manually-filtering" branch, set filter by URL, because i trust your fix...

Go to update, came back with back link, filter persist, not add "persistent-table=true"

Happy world...but i try new scenario. I update a record for real, and then save send me back to list view...

Everything work

I get the update message, then i go to update any other record and click browser back button along URL, and i always get the update message, even if i just back and of course dont save anything, i try this in "main" branch and didnt happen.

I add this video if help.

screencast-backpack.test-2022.08.04-20_56_44.webm

Cheers.

[pxpm]

@jorgetwgroup

I've been testing this for a while. Please can you confirm my findings?

• If I have developer console open, clearing the browser cache on refresh I don't get the alert when clicking go back in browser.

• If I close the console, I get the same behaviour as you.

I haven't found the solution yet, I just want to confirm this with you.

Cheers

[jcastroa87]

Hello @pxpm i confirm, only happen with cache enabled, when disable and clean everything dont happen.

[pxpm]

Hey guys.

What @jorgetwgroup found out is also happening in the main branch, and I am not sure there is an easy way to fix it.

We can reproduce this exactly same behaviour on previous versions, but I think it's a browser thing with cache because from my tests we are doing the things correctly. We display the alerts and then we delete them from localStorage. But when going back with the press of the back button, browser re-creates the localstorage etc from the "previous initialized state", that means that we will have the alert again because the page was "initially loaded with the alert" and only after displaying it, we do the delete process.

I've searched alot and the only solution I found is moving the alerts from localStorage to the server session (not browser session storage), so that browser is not able to re-create the "server session" as it does with browser based stuff.

@tabacitu before going on for it, do you foresee any issue using server session instead of localstorage to deal with the alerts ? If I recall correctly we used localStorage only to make it work similar as datatables ... but well, in this scenario that's not desirable.

[tabacitu]

Changing from localstorage to server session sounds like a change with unforeseen impact. People might have done customizations on top of our localstorage implementation, so changing it might break their shit 😅

I'm somehow skeptical that it's worth doing, because I think it would be a breaking change. But if you guys think it is, let's do a separate PR for that please. Use case seems valid, and I also saw notifications when going back at times and thought "wtf", so we might want to fix that, yes. It's a COULD/SHOULD if you ask me. But its seems unrelated to this PR, if you ask me.

In the meantime, let's focus this PR on what this PR is trying to solve. Is the main problem here tested and ready for me to merge @jorgetwgroup ?

[tabacitu]

😅 I need @jorgetwgroup 's approval to merge, @pxpm . If he's the reviewer, best to leave him final say on this, he's the one that should say "Tabacitu, merge it".

[jcastroa87]

Hi there @pxpm main problem is fixed. Thanks. @tabacitu this look ready to merge for me.