divio / aldryn-search

Haystack 2.0 search index for django CMS
Other
48 stars 78 forks source link

Extend TitleIndex get_search_data to make a more specific optional se… #38

Closed fmarco closed 9 years ago

fmarco commented 9 years ago

…arch inside specific pages placeholders plugins

This PR add a simple specialization of get_search_data, to handle generic cases or specific cases related to one or more pages.

The idea is to set up this variable inside the project settings

PLACEHOLDERS_SEARCH_LIST = { 'page_title': [ 'placeholder_1', 'placeholder_2', etc. ], }

( or leave it empty to use the generic handling).

czpython commented 9 years ago

Hello @fmarco,

I really like the concept but I don't like coupling a value as dynamic as a title to a setting in the settings file because titles can easily change.

Also you could easily hit an issue with duplicate titles as well as having non-ascii in settings file if you would like to target titles in other pages.

And last, supporting multiple languages would be very repetitive, a placeholder is not bound to title instance which means that it won't change from one title to the next so coupling a title to placeholder slots would result in one of these per title per page.

I see two potential solutions:

My personal opinion is that best approach from code/best practice point of view is point 1 but from a flexibility point of view then point 2.

We could also implement both solutions in a cool setting in order to provide an easy way to target all placeholders or a placeholder under page x.

fmarco commented 9 years ago

Hi @czpython , thanks for your answer!

I agree, page_title value is not so reliable by its nature, this first commit was a sort of proof of concept i thought during a project coding and i needed something similar. I like the first point :), but maybe, as you said, we could leave the user free to use a safer settings as in point 1 or to use a more specific filter on reverse_id.

fmarco commented 9 years ago

@czpython : Refactor of the code based on request and new tests structure

fmarco commented 9 years ago

Fixed and updated

czpython commented 9 years ago

@fmarco Thank you. I will merge this now, but please make another pr for https://github.com/nephila/aldryn-search/blob/feature/improve_titleindex_get_search_data/aldryn_search/tests.py#L306 seems like this test is repeated