Skip to content
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

Show matching filters in search results #7005

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

phansys
Copy link
Member

@phansys phansys commented Apr 1, 2021

Subject

Show matching filters in search results.

I am targeting this branch, because these changes respect BC.

The search results are using the Admin::toString() method to show a representation of the model matching the search criteria, but in cases where the value of the field matching a filter is not present on that string, is hard to understand why that result is presented as a match.
These changes add the name of the filters that produced the model match in each search result.
Here is an example:

image

Changelog

### Added
- Information about the matching filters in the search results.

To do

  • Improve the UI;
  • Check if we can provide the same feature for filters using a deeper property path (IE sub.model.property).

If someone with good CSS skills wants to get rid of the required work to provide a better UI, please be free to suggest any change.

@phansys phansys added the minor label Apr 1, 2021
@phansys phansys force-pushed the block_search_result branch 2 times, most recently from 970840a to 0e0b818 Compare April 2, 2021 22:43
@phansys phansys force-pushed the block_search_result branch 3 times, most recently from 4e9728d to 7104342 Compare April 9, 2021 11:41
@phansys phansys marked this pull request as ready for review April 9, 2021 11:56
@phansys phansys requested a review from a team April 9, 2021 11:56
@VincentLanglet
Copy link
Member

If someone with good CSS skills wants to get rid of the required work to provide a better UI, please be free to suggest any change.

label-primary instead of default in order to have some colors ?

@phansys phansys force-pushed the block_search_result branch from 7104342 to 7f6438c Compare April 9, 2021 13:03
@phansys phansys requested review from core23 and a team April 9, 2021 13:04
@phansys phansys force-pushed the block_search_result branch from 7f6438c to 294e700 Compare April 11, 2021 01:34
@phansys
Copy link
Member Author

phansys commented Apr 11, 2021

@core23, @VincentLanglet; could you please bring a new review?

Comment on lines 68 to 75
{#
When case sensitivity issues (like the one present when using MySQL database) were resolved, remove
the previous condition and uncommment the following one.
See https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1395.
#}
{#{% if match and (((filter.options.case_sensitive or filter.options.case_sensitive is null) and filter.value.value in match)
or filter.options.case_sensitive is same as(false) and filter.value.value|lower in match|lower)
%}#}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me when we can update the code.
The PR: sonata-project/SonataDoctrineORMAdminBundle#1395 is already merged.
And the Admin bundle should be aware about the issues from persistence bundles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus if we rename the option in the persistence bundle, this won't work.
The AdminBundle shouldn't rely on the persistence bundle code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AdminBundle shouldn't rely on the persistence bundle code

This option is set in the SearchHandler:

$filter->setOption('case_sensitive', $this->caseSensitive);

And the Admin bundle should be aware about the issues from persistence bundles.

We already know that isn't properly respected with some DB platforms.
If I use the commented code instead, this template will respect the "case_sensitive" option, which can lead to not matching some results returned by the persistence bundle (since the persistence bundle is not properly respecting the option).
What do you suggest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the right behavior (respecting the "case_sensitive" option), adding this comment in the template:

Some model managers are not respecting the "sonata_admin.global_search.case_sensitive" configuration option.
If you see no matching filters for a search result, it is probably the reason. In that case, you SHOULD consider using false as value for this option.

Is it fine for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok like this

@phansys phansys force-pushed the block_search_result branch 2 times, most recently from 453a137 to 37e5e97 Compare April 11, 2021 13:48
If you see no matching filters for a search result, it is probably the reason. In that case,
you SHOULD consider using `false` as value for this option.
#}
{% if match and (((filter.options.case_sensitive or filter.options.case_sensitive is null) and filter.value.value in match)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% if match and (((filter.options.case_sensitive or filter.options.case_sensitive is null) and filter.value.value in match)
{% if match and ((filter.options.case_sensitive is not same as(false) and filter.value.value in match)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

core23
core23 previously approved these changes Apr 11, 2021
core23
core23 previously approved these changes Apr 12, 2021
If you see no matching filters for a search result, it is probably the reason. In that case,
you SHOULD consider using `false` as value for this option.
#}
{% if match and ((filter.options.case_sensitive is not same as(false) and filter.value.value in match)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These filters are implementing FilterInterface if I'm not mistaken, but there is no getValue method in the interface.

And getValue from Filter is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!
I've replaced these usages with a new variable holding the search term.
Thank you.

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getValue is not in the interface in master :/

@phansys phansys force-pushed the block_search_result branch from 6626ee0 to 6d77bdc Compare April 12, 2021 09:07
@phansys phansys merged commit a2d7df2 into sonata-project:3.x Apr 12, 2021
@phansys phansys deleted the block_search_result branch April 12, 2021 15:38
@core23
Copy link
Member

core23 commented Aug 7, 2021

This highlight does not work if you use cascaded fields, e.g.:

    protected function configureDatagridFilters(DatagridMapper $filter): void
    {
        $filter
            ->add('addresses.city')
        ;
    }

This will result in a RuntimeException:

Neither the property "city" nor one of the methods "city()", "getcity()"/"iscity()"/"hascity()" or "__call()" exist and have public access in class "Doctrine\ORM\PersistentCollection".

@VincentLanglet
Copy link
Member

This highlight does not work if you use cascaded fields, e.g.:

    protected function configureDatagridFilters(DatagridMapper $filter): void
    {
        $filter
            ->add('addresses.city')
        ;
    }

This will result in a RuntimeException:

Neither the property "city" nor one of the methods "city()", "getcity()"/"iscity()"/"hascity()" or "__call()" exist and have public access in class "Doctrine\ORM\PersistentCollection".

This feature was partially removed from Sonata 4. Do you know if it'll be still present ?
#7347

@core23
Copy link
Member

core23 commented Aug 7, 2021

This feature was partially removed from Sonata 4. Do you know if it'll be still present ?
#7347

What do you mean with still present? Tested it just today with the 3.x branch. Never tried the master branch.

@VincentLanglet
Copy link
Member

This feature was partially removed from Sonata 4. Do you know if it'll be still present ?

#7347

What do you mean with still present? Tested it just today with the 3.x branch. Never tried the master branch.

If the bug you describe will be still present in the master branch since the code were modified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants