ENHANCEMENT: Search both Title and Filename in AssetAdmin (#7013) #71

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@ajoneil
Contributor

ajoneil commented Mar 21, 2012

No description provided.

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Mar 21, 2012

Member

Hm, the removal of SearchContext means that you can't simply add to File::$searchable_fields (or decorate File with additional properties) - you now have to subclass AssetAdmin. Can't you just remove the SearchFilter responsible for querying the name, add it manually, but leave the SearchContext in place?

Member

chillu commented Mar 21, 2012

Hm, the removal of SearchContext means that you can't simply add to File::$searchable_fields (or decorate File with additional properties) - you now have to subclass AssetAdmin. Can't you just remove the SearchFilter responsible for querying the name, add it manually, but leave the SearchContext in place?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Mar 24, 2012

Member

...or modify File::$searchable_fields to search across both Filename and Title?

Member

sminnee commented Mar 24, 2012

...or modify File::$searchable_fields to search across both Filename and Title?

@ajoneil

This comment has been minimized.

Show comment
Hide comment
@ajoneil

ajoneil Mar 25, 2012

Contributor

@chillu Title was the only thing still being searched by the SearchContext. I didn't see any point keeping the SearchContext around when it wasn't being used. I can add it back if you feel it makes sense.

@sminnee It didn't look to me like it was possible to have a single form field that searches across two database fields, and I didn't see an immediately obvious way to extend it to do so. Ideas?

Contributor

ajoneil commented Mar 25, 2012

@chillu Title was the only thing still being searched by the SearchContext. I didn't see any point keeping the SearchContext around when it wasn't being used. I can add it back if you feel it makes sense.

@sminnee It didn't look to me like it was possible to have a single form field that searches across two database fields, and I didn't see an immediately obvious way to extend it to do so. Ideas?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Mar 25, 2012

Member

It's more work but I'd probably suggest that we make the ORM support this. The API that we had previously discussed was to allow the map key to be a string containing a comma separated list of field names. I'm not sure if it got into implementation or not.

The best place to do this would be in the DataList::filter() method, and then to refactor SearchContext to push most of the hard work to filter (or refactor modeladmin to use filter directly).

Member

sminnee commented Mar 25, 2012

It's more work but I'd probably suggest that we make the ORM support this. The API that we had previously discussed was to allow the map key to be a string containing a comma separated list of field names. I'm not sure if it got into implementation or not.

The best place to do this would be in the DataList::filter() method, and then to refactor SearchContext to push most of the hard work to filter (or refactor modeladmin to use filter directly).

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Mar 26, 2012

Member

@ajoneil For customization purposes, I'd prefer to keep SearchContext in there, yes.
I'm not quite sure how @sminnee has planned to deprecate SearchContext though,
so this might be a non-starter. Sam, is there a ticket describing the required work already?
Which class would be responsible for scaffolding the search form fields according to model definitions in that new API?

BTW, there's a ticket/patch about multiple field support in trac that's just about as old as SearchContext itself ;)
http://open.silverstripe.org/ticket/4734

Member

chillu commented Mar 26, 2012

@ajoneil For customization purposes, I'd prefer to keep SearchContext in there, yes.
I'm not quite sure how @sminnee has planned to deprecate SearchContext though,
so this might be a non-starter. Sam, is there a ticket describing the required work already?
Which class would be responsible for scaffolding the search form fields according to model definitions in that new API?

BTW, there's a ticket/patch about multiple field support in trac that's just about as old as SearchContext itself ;)
http://open.silverstripe.org/ticket/4734

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Apr 11, 2012

Member

Fixed the issue on the ticket with 363dc9a, lets tackle the composite searchfilter problem in the ticket Sam referenced.

Member

chillu commented Apr 11, 2012

Fixed the issue on the ticket with 363dc9a, lets tackle the composite searchfilter problem in the ticket Sam referenced.

@chillu chillu closed this Apr 11, 2012

chillu added a commit that referenced this pull request Apr 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment