[Bug] The ListOperation reset button does not work properly when the route is defined by route name (not path)

[gbachev]

Bug report

What I did

Clicking the reset (DataTables) button on the ListOperation

What I expected to happen

Clear the localStorage item, therefore reset the sort & pagination

What happened

Filter items in the localStorage seems to be cleared but the 'DataTables_crudTable_/admin/[PATH]' item responsible for the sort and pagination is not.

What I've already tried to fix it

After quick analysis noticed that the DataTables sets the localStorage item using the 'location.pathname'. This on line 11122 in the UNminified jquery.DataTables.js file that comes in public/packages/datatables.net/js/jquery.dataTables.js

Here is the JS snippet:

"fnStateSaveCallback": function ( settings, data ) {
    try {
       (settings.iStateDuration === -1 ? sessionStorage : localStorage).setItem(. 
           'DataTables_'+settings.sInstance+'_'+location.pathname,
           JSON.stringify( data )
	);
    } catch (e) {}
},

However the logic that clears the localStorage in 'vendor/backpack/crud/src/resources/views/crud/inc/datatables_logic.blade.php' tries to clear the localStorage item based on the current route. This is on lines 247 - 249. Here is the snippet:

//clear the table sorting/ordering/visibility
if(localStorage.getItem('DataTables_crudTable_/{{ $crud->getRoute() }}')) {
    localStorage.removeItem('DataTables_crudTable_/{{ $crud->getRoute() }}');
}

After overriding the view template and making the following change, the issue seems to be fixed:

//clear the table sorting/ordering/visibility
if(localStorage.getItem('DataTables_crudTable_' + location.pathname )) {
    localStorage.removeItem('DataTables_crudTable_' + location.pathname);
}

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP VERSION:

PHP 7.4.7 (cli) (built: Jun 11 2020 18:33:07) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
with Zend OPcache v7.4.7, Copyright (c), by Zend Technologies

LARAVEL VERSION:

v7.21.0@3ccdb116524de408fdc00715b6f06a1031ddace9

BACKPACK VERSION:

4.1.15@6c751de946a9c8511dd32eb7bfa3ca6a568849f5

I may be missing something but it`s a fresh install based on the docs so probably good idea to take a look.

Best,
Georgi

[welcome]

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

• Bug Reports, Feature Requests - Github Issues (here);
• Quick help (How do I do X) - Gitter Chatroom;
• Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
• Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

[tabacitu]

Thanks a lot for submitting the bug, and especially for the solution @gbachev !

Would you like to submit a PR with this change, so that you show up as a Backpack contributor? If not, that's no problem, we'll take a look at this and put together a PR ASAP.

Cheers!

[tabacitu]

Hmm actually @gbachev - this is weird, my tests show that the sorting and pagination are reset by the Reset button, even without this change. Testing with the latest Brave/Chrome here:

Did I misunderstand your issue then? I'm torn on what the problem is here... Or why we'd get different experiences doing the same thing.

If this is indeed the problem:
(1) Does it happen to you on demo.backpackforlaravel.com, for the News CRUD?
(2) What browser are you using?

[gbachev]

Hi @tabacitu,

Interesting indeed, but I think I have tracked it down, my thoughts:

1. After quick check - it does not happen on demo.backpackforlaravel.com (works like charm there). The BackPack version is the same, the DataTables version is the same therefore it's not a diff browser issue.
2. If it's not the browser must a config or code difference - looking at the original code of the datatables_logic.blade.php it uses $crud->getRoute() which resolves to $this->route (the controller's route). This made me check how I have defined the route in the particular CrudController and I noticed I have set the route by name:

CRUD::setRoute(route('[ROUTE-NAME].index'));

While in the demo and docs you use using:

CRUD::setRoute(config('backpack.base.route_prefix')."/[URL-PATH]");

Both are fine in terms of general workings, the applications behaves fine, however the getRoute returns different things. In any case this isn't a critical issue and is more of an edge-case but probably good idea to make it work based on location.pathname as this is what DataTables uses to set the localStorage item :)

Best,
Georgi

[pxpm]

Thanks for the clarification @gbachev .

Indeed it works for getRoute() when we define the full path url. But I agree with you that using location.pathname as Datables uses it should also work independant of the way we define the route.

If you don't mind submit a PR with the change so you show up as a contributor and speed up the merging process ?

If you don't have time to do it, eventually me or @tabacitu will get to this.

Best,
Pedro

[tabacitu]

OOOOHH!! Now I understand. Thank your digging deep into this issue and finding the culprit @gbachev ! Indeed it's a small issue but an annoying one - we should fix it. Like @pxpm said, let us know if you want to submit a PR to fix this and show up as a Contributor, if you can't then one of us will. I've prioritised the issue, it's marked as a TODO.

Thanks for submitting this, and especially for finding the root cause.
Cheers!

[pxpm]

Hello @gbachev

Sorry for taking so much time to get back into this.

Now that I took a second look this sound more like a feature request to support a new way of adding the route in crud, because at the moment we already have the ability to setup the route by name:

public function setRouteName($route, $parameters = [])
    {
        $complete_route = $route.'.index';

        if (! \Route::has($complete_route)) {
            throw new \Exception('There are no routes for this route name.', 404);
        }

        $this->route = route($complete_route, $parameters);
    }

So in your scenario you shoud just:

CRUD::setRouteName('name_you_write_inside_route()_helper');

//eg in backpack demo project

CRUD::setRouteName('monster');

//instead of

$this->crud->setRoute(config('backpack.base.route_prefix').'/monster');

Don't need to add the .'index' because we will pick it up as you can see from code.

I am going to close this because I think we already provide an easier way than the one you are trying to achieve. That said if you think I am wrong please re-open.

Wish you the best,
Pedro