Unchanged filters params #6700

kirya-dev · 2020-12-11T23:01:37Z

Subject

I am targeting this branch, because created new feature for SonataAdminBundle: Only changed filters will be submitted. Made admin`s urls shorter as possible.

Closes #6642.

Changelog

### Added
- Added `AbstractAdmin::getDefaultFilterParameters()` to get default filter parameters.

### Changed
- Only changed (non default, non empty) filters will be submitted.

VincentLanglet

Could you also provide a screenshot Before/After ? :)

src/Resources/public/Admin.js

src/Resources/views/CRUD/base_list.html.twig

kirya-dev · 2020-12-11T23:19:10Z

Im suggest only idea now. Its works how expects in ISSUE.

VincentLanglet · 2020-12-11T23:29:13Z

Im suggest only idea now. Its works how expects in ISSUE.

Yes, but I never worked with the js of Sonata, so I can't say if it works or not.
For instance, you're using includes (https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Array/includes) which is ECMA 2016 ; I would say we cannot use such a recent JS syntax (I see no let/const/arrow functions/...).

A screenshot was a great way to prove it works as expected, and comments could help futur developers.

src/Admin/AdminInterface.php

src/Resources/public/Admin.js

kirya-dev · 2020-12-12T14:04:29Z

BEFORE:

AFTER:

AFTER (with decode URI):

franmomu · 2020-12-12T15:41:17Z

src/Resources/public/Admin.js

+            window.location = this.action + (diffValues.length ? ('?' + diffValues.join('&')) : '');
+
+            return false;


I'm not updated with Javascript, but last time I checked (many years ago), return false; was not a best practice. IMO we should keep the "normal" flow of the form and do not use window.location.

Im dont view something bad in it.
You can find other usages in your project(also Admin.js) with window.location.href

return false only works for jQuery and does not express clearly what it does, it is not explicit. IMO we should use Vanilla JS as much as we can.

By not using window.location I meant that some other listener else could listen to the form submit.

https://github.com/sonata-project/SonataAdminBundle/search?q=window.location this are the usages in mythis project, I see just one and I guess it is used when selecting how many results per page.

Hmm. Only read access. Ok. What is your suggeston to refactor it?

Im will see:

Disable no need to submit values.

Create fake form with this url and submit it.

franmomu · 2020-12-12T15:46:16Z

I don't think we should add another method to AdminInterface just for hiding some parameters in the url when filters are submitted. There should be another way, in any case I guess it should go in something related to datagrid.

If it is just for the "view", maybe just in twig.

VincentLanglet · 2020-12-12T15:59:10Z

How did you get the with decode URI example ? I would say it's better to keep the URI encoded.

What about advanced filters ? It seems like you're only working on values for simple filters.

getDefaultFilterParameters returns an array:

['filterName' => ['type' => $type, 'value' => $value]]

Where $type is optional and $value is mixed (and can be an array).

When I used an advance filter I get

?filter[label][type]=1&filter[label][value]=

When I use a multiple filter I get

?filter[endDate][type]=&filter[endDate][value][start]=&filter[endDate][value][end]=

I'm not sure you're currently supporting these situation.

Also, since #6402, filters should be submitted as soon as they have not-empty value or a not-empty type. Here you're filtering every empty values.

kirya-dev · 2020-12-12T16:23:10Z

Here you're filtering every empty values.

It main purpose for this PR

VincentLanglet · 2020-12-12T16:26:34Z

Here you're filtering every empty values.

It main purpose for this PR

But like I said, if a type is submitted, it should stay submitted and displayed.

kirya-dev · 2020-12-12T16:29:43Z

But like I said, if a type is submitted, it should stay submitted and displayed.

It problem don`t solves in this PR.

kirya-dev · 2020-12-12T16:48:53Z

You can clone my branch and check it yourself so as not to make assumptions.

Its tested on 3.x branch:

kirya-dev · 2020-12-12T16:56:08Z

I'm not sure you're currently supporting these situation.

My code just compute difference between two 1d arrays. Comparing worked on keyValueString.
For ex: filter[endDate][type]= or filter[endDate][value][end]=.
My code absolutely worked on any structure

kirya-dev · 2020-12-12T17:01:26Z

Here you're filtering every empty values.

Its no so. Im keep non empty AND non default. Becouse not present filter in defaults has empty value after submit. Im prevent it.

VincentLanglet · 2020-12-12T18:26:31Z

You can clone my branch and check it yourself so as not to make assumptions.

Just tried, and it seems to cover every case but one.

When there is some persistent parameters (?incomingCall=0), the url become

?incomingCall=0?filter%5Busagename%5D%5Bvalue%5D=CHARVOLIN&filter%5B_page%5D=1&filter%5B_sort_by%5D=id&filter%5B_sort_order%5D=DESC&filter%5B_per_page%5D=32

kirya-dev · 2020-12-12T18:32:34Z

When there is some persistent parameters (?incomingCall=0), the url become

Its problem on 3.x too

VincentLanglet · 2020-12-12T18:41:07Z

When there is some persistent parameters (?incomingCall=0), the url become

Its problem on 3.x too

No, in 3.x the url with the persistent parameters and the filters is

?filter%5Busagename%5D%5Btype%5D=&filter%5Busagename%5D%5Bvalue%5D=er&filter%5B_page%5D=1&filter%5B_sort_by%5D=id&filter%5B_sort_order%5D=DESC&filter%5B_per_page%5D=32&incomingCall=

with the persistent parameter incomingCall= at the end.
With your branch the persistent parameter is in the beginning and the url is

?incomingCall=0?filter%5Busagename%5D%5Bvalue%5D=toto&filter%5B_page%5D=1&filter%5B_sort_by%5D=id&filter%5B_sort_order%5D=DESC&filter%5B_per_page%5D=32

And I lose all my filters because of the two ?.

this.action already has query params, you didn't take this case in account.

kirya-dev · 2020-12-12T18:53:35Z

Im agree in one point. If action already has query parameter. But its impossible.

VincentLanglet · 2020-12-12T19:09:44Z

But its impossible.

How can you say it's impossible since I just said to you that I tried it on my project.

Action is

action="{{ admin.generateUrl('list') }}"

Which is

public function generateUrl($name, array $parameters = [], $referenceType = RoutingUrlGeneratorInterface::ABSOLUTE_PATH)
    {
        return $this->routeGenerator->generateUrl($this, $name, $parameters, $referenceType);
    }

and DefaultRouteGenerator::generateUrl use DefaultRouteGenerator::generateMenuUrl which has the code

// allows to define persistent parameters
if ($admin->hasRequest()) {
    $parameters = array_merge($admin->getPersistentParameters(), $parameters);
}

So YES, it's possible.

And if you look at the code, there is others way to have query params in the action variable.

You have to support both case where there is already query params and case where there is no query params.
If there is already a ? in the url, we add &, if not, we add ?.

And since this code has a lot of logic, some comments could help.

kirya-dev · 2020-12-12T20:44:44Z

Current version without window.location:

haivala · 2020-12-12T23:06:50Z

just asking: does this have anything to do with null filter values? they should be valid too

kirya-dev · 2020-12-13T07:02:11Z

just asking: does this have anything to do with null filter values? they should be valid too

Im think no. Null filter value (in 3.x branch too) equal inactive filter

franmomu · 2020-12-14T18:13:59Z

IMO we should not add a method in AdminInterface for this: #6700 (comment)

VincentLanglet · 2020-12-14T18:50:17Z

IMO we should not add a method in AdminInterface for this: #6700 (comment)

What would be the alternative ? Only the admin can determine the default filters values.

src/Resources/views/CRUD/base_list.html.twig

src/Resources/public/Admin.js

franmomu · 2020-12-14T21:53:02Z

What would be the alternative ? Only the admin can determine the default filters values.

I have no idea, but I had supposed that datagrid would have some info or at least sounds like a better place

SonataCI · 2020-12-15T13:43:16Z

Could you please rebase your PR and fix merge conflicts?

src/Resources/public/Admin.js

Co-authored-by: Javier Spagnoletti <[email protected]>

haivala · 2020-12-15T18:56:38Z

I was just pointing out that we should be able to filter by empty value NULL in db. Haven't had time to check how this works..

Use case: I have post entity and it has comments. I would like to find posts without comments.

kirya-dev · 2020-12-15T20:22:43Z

Use case: I have post entity and it has comments. I would like to find posts without comments.

Please use Callback filter with full control query builder

haivala · 2020-12-15T22:06:13Z

Please use Callback filter with full control query builder

Probably relation was not the best example. I meant basic text field

VincentLanglet · 2020-12-15T22:13:55Z

I was just pointing out that we should be able to filter by empty value NULL in db. Haven't had time to check how this works..

You should just try this branch in your composer.json and then check if your filter still work.

But for what I read, this has nothing to do with the form data, just the url.
The filter has apply with $form->getData(). Since he's only removing from the url the defaults values, the $data stay the same.

src/Admin/AdminInterface.php

kirya-dev · 2020-12-18T20:15:43Z

@phansys you are approve this PR or not? wait new conflicts on merge?)

phansys · 2020-12-19T12:09:13Z

@phansys you are approve this PR or not? wait new conflicts on merge?)

First of all, I want to thank you very much for your contribution and for sharing your thoughts and concerns.
Also, I must remind you that this is an open source project and we are contributors. We cannot guarantee the availability of time or other things like SLA.
That said, I'll try to review again today.

phansys

I think these changes and its goal are fine. My only concern is that we are delegating such a core feature to the frontend, where we have no tests.

VincentLanglet · 2021-01-02T20:58:12Z

Is it ok for you @franmomu ? :)

kappaj · 2021-02-16T12:10:25Z

I have still problems with this feature. Tested with sonata-admin verion 3.89. Have to step back to <3.86 to get old filter behavior.

My Scenario

Having a default filter of choice type with activated advanced filters

protected function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
        $datagridMapper
            ->add('chatState',
                'doctrine_orm_choice', [
                    'global_search' => true,
                    'field_type'    => ChoiceType::class,
                    'field_options' => [
                        'choices' => ChatStates::getChoices()
                    ]
                ]
            );
    }

protected function configureDefaultFilterValues(array &$filterValues)
    {
        $filterValues['chatState'] = [
            'type'  => EqualOperatorType::TYPE_NOT_EQUAL,
            'value' => ChatStates::ARCHIVED,
        ];
    }

When I open the list and change the type to "equal" and press "Filter" the page reloads and the list is empty. The filter value is missing in the get param list. Thats the list:

list?filter[chatState][type]=1

But it has to be:

list?filter[chatState][type]=1&filter[chatState][value]=archived

VincentLanglet · 2021-02-16T12:11:44Z

@kappaj Can you open an issue ? It will be better to discuss/solve it

kappaj · 2021-02-16T12:25:00Z

Added issue: #6865

VincentLanglet reviewed Dec 11, 2020

View reviewed changes

src/Resources/public/Admin.js Outdated Show resolved Hide resolved

src/Resources/views/CRUD/base_list.html.twig Outdated Show resolved Hide resolved

VincentLanglet requested changes Dec 12, 2020

View reviewed changes

src/Admin/AdminInterface.php Outdated Show resolved Hide resolved

VincentLanglet reviewed Dec 12, 2020

View reviewed changes

src/Resources/public/Admin.js Outdated Show resolved Hide resolved

franmomu reviewed Dec 12, 2020

View reviewed changes

VincentLanglet previously approved these changes Dec 14, 2020

View reviewed changes

VincentLanglet requested review from a team and franmomu December 14, 2020 15:41

phansys reviewed Dec 14, 2020

View reviewed changes

src/Resources/views/CRUD/base_list.html.twig Show resolved Hide resolved

src/Resources/public/Admin.js Outdated Show resolved Hide resolved

SonataCI added the pending author label Dec 15, 2020

Merge branch '3.x' into feature-unchanged-filters

a378f67

SonataCI removed the pending author label Dec 15, 2020

fix tests

28fcc37

kirya-dev requested a review from phansys December 15, 2020 16:59

phansys reviewed Dec 15, 2020

View reviewed changes

src/Resources/public/Admin.js Outdated Show resolved Hide resolved

Update src/Resources/public/Admin.js

ab4c3c4

Co-authored-by: Javier Spagnoletti <[email protected]>

VincentLanglet requested review from phansys and a team December 15, 2020 22:14

core23 reviewed Dec 16, 2020

View reviewed changes

src/Admin/AdminInterface.php Outdated Show resolved Hide resolved

Remove comment

e6bcc99

core23 approved these changes Dec 17, 2020

View reviewed changes

phansys approved these changes Dec 21, 2020

View reviewed changes

VincentLanglet approved these changes Dec 24, 2020

View reviewed changes

VincentLanglet requested a review from franmomu December 24, 2020 15:28

VincentLanglet merged commit baaea97 into sonata-project:3.x Jan 9, 2021

kirya-dev deleted the feature-unchanged-filters branch January 9, 2021 16:17

VincentLanglet added the minor label Jan 9, 2021

phansys mentioned this pull request Feb 27, 2021

ChoiceFilter is not working properly when 'multiple' => true option is used #6894

Closed

		window.location = this.action + (diffValues.length ? ('?' + diffValues.join('&')) : '');

		return false;

Unchanged filters params #6700

Unchanged filters params #6700

Conversation

kirya-dev commented Dec 11, 2020

Subject

Changelog

VincentLanglet left a comment

Choose a reason for hiding this comment

kirya-dev commented Dec 11, 2020

VincentLanglet commented Dec 11, 2020 • edited Loading

kirya-dev commented Dec 12, 2020 • edited Loading

franmomu Dec 12, 2020

Choose a reason for hiding this comment

kirya-dev Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

franmomu Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

kirya-dev Dec 12, 2020

Choose a reason for hiding this comment

franmomu commented Dec 12, 2020

VincentLanglet commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

VincentLanglet commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

kirya-dev commented Dec 12, 2020 • edited Loading

VincentLanglet commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

VincentLanglet commented Dec 12, 2020

kirya-dev commented Dec 12, 2020

VincentLanglet commented Dec 12, 2020 • edited Loading

kirya-dev commented Dec 12, 2020

haivala commented Dec 12, 2020 • edited Loading

kirya-dev commented Dec 13, 2020

franmomu commented Dec 14, 2020

VincentLanglet commented Dec 14, 2020

franmomu commented Dec 14, 2020

SonataCI commented Dec 15, 2020

haivala commented Dec 15, 2020

kirya-dev commented Dec 15, 2020

haivala commented Dec 15, 2020 • edited Loading

VincentLanglet commented Dec 15, 2020

kirya-dev commented Dec 18, 2020

phansys commented Dec 19, 2020

phansys left a comment

Choose a reason for hiding this comment

VincentLanglet commented Jan 2, 2021

kappaj commented Feb 16, 2021 • edited Loading

VincentLanglet commented Feb 16, 2021

kappaj commented Feb 16, 2021

VincentLanglet commented Dec 11, 2020 •

edited

Loading

kirya-dev commented Dec 12, 2020 •

edited

Loading

kirya-dev Dec 12, 2020 •

edited

Loading

franmomu Dec 12, 2020 •

edited

Loading

kirya-dev commented Dec 12, 2020 •

edited

Loading

VincentLanglet commented Dec 12, 2020 •

edited

Loading

haivala commented Dec 12, 2020 •

edited

Loading

haivala commented Dec 15, 2020 •

edited

Loading

kappaj commented Feb 16, 2021 •

edited

Loading