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
Add optional search #565
Add optional search #565
Conversation
@@ -29,12 +30,16 @@ django-selectable==0.9.0 | |||
django-storages==1.5.1 | |||
django-versatileimagefield==1.6 | |||
django-webpack-loader==0.3.3 | |||
Django==1.10.1 # via django-absolute, django-emailit, django-model-utils, django-payments, django-prices, jsonfield | |||
Django==1.9.10 # via django-absolute, django-emailit, django-haystack, django-model-utils, django-payments, django-prices, jsonfield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1.9?
@@ -290,3 +292,12 @@ | |||
'IGNORE': [ | |||
r'.+\.hot-update\.js', | |||
r'.+\.map']}} | |||
|
|||
|
|||
HAYSTACK_CONNECTIONS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this configurable using env and default to using whoosh?
|
||
|
||
class ProductVariantIndex(indexes.SearchIndex, indexes.Indexable): | ||
text = indexes.CharField(document=True, use_template=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please not use a single document field for search? It does not what which field is which so it'll treat the product name, variant name and description as a single value. None of the metrics like term/field factor or term position make any sense in that setup.
@patrys This is |
I know it's in progress but I'm afraid requiring 1.9 may become a blocker. |
Current coverage is 58.26% (diff: 37.51%)@@ master #565 diff @@
==========================================
Files 91 105 +14
Lines 4156 5219 +1063
Methods 0 0
Messages 0 0
Branches 440 615 +175
==========================================
+ Hits 2642 3041 +399
- Misses 1424 2063 +639
- Partials 90 115 +25
|
24f58dd
to
1cba4a3
Compare
Implementation is finished and ready for review |
3d00d65
to
d97f7cb
Compare
We'll be able to merge this as soon as Haystack will support Django 1.10 (django-haystack/django-haystack#1445) |
65e5971
to
480c850
Compare
@@ -48,3 +48,24 @@ Environment variables | |||
|
|||
``SECRET_KEY`` | |||
Controls `Django's secret key <https://docs.djangoproject.com/en/1.10/ref/settings/#secret-key>`_ setting. | |||
|
|||
``ENABLE_ELASTICSEARCH`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a separate variable for that? Why would I set ELASTICSEARCH_URL
and ELASTICSEARCH_INDEX_NAME
if I did not intend to use Elasticsearch?
|
||
.. code-block:: bash | ||
|
||
$ pip install elasticsearch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add this to our requirements. It needs to run out of the box on Heroku.
HAYSTACK_CONNECTIONS = { | ||
'default': { | ||
'ENGINE': 'haystack.backends.elasticsearch_backend.ElasticsearchSearchEngine', | ||
'URL': os.environ.get('ELASTICSEARCH_URL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to also handle BONSAI_URL
and SEARCHBOX_URL
(these are Heroku add-ons that provide Elasticsearch).
var $menuToggle = $('.menu-toggle') | ||
var $closeMenu = $('#close-menu') | ||
var $openMenu = $('#open-menu') | ||
function openMenu(a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can come up with a more descriptive name than a
😉
$openMenu.addClass('hide') | ||
$closeMenu.removeClass('hide') | ||
$menuToggle.addClass('fixed') | ||
if ( $windowWidth > 990 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The space inside the parentheses is not needed.
$closeMenu.click(function() { | ||
closeMenu() | ||
}) | ||
if ($windowWidth < 991) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of hardcoding the pixel values all around the place. Could we define some constants for break points and use those later in code?
Also how do we handle resizing the window?
@@ -37,3 +37,6 @@ requests>=1.2.0 | |||
satchless>=1.1.2,<1.2a0 | |||
unidecode | |||
uwsgi>=2.0.0 | |||
django-haystack==2.5.0 | |||
Whoosh==2.7.4 | |||
elasticsearch==2.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is meant to be sorted :)
return Product | ||
|
||
def index_queryset(self, using=None): | ||
qs = self.get_model().objects.get_available_products() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to index all products and reimplement availability checks at search time (but only when searching the site and not the dashboard).
logger = logging.getLogger(__name__) | ||
|
||
|
||
def update_object(instance, using='default'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we gain anything from having a "generic" solution like this? Why not have explicit update_product
, update_user
etc.?
from ..userprofile.models import User | ||
|
||
|
||
def search_for_model(request, models): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name suggests it would search for a single model.
raise Http404("No such page!") | ||
else: | ||
page = form.no_query_found() | ||
return {'page': page, 'query': form.cleaned_data['q']} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider returning a tuple or a namedtuple?
'ENGINE': 'haystack.backends.elasticsearch_backend.ElasticsearchSearchEngine', | ||
'URL': os.environ.get('ELASTICSEARCH_URL', | ||
'http://127.0.0.1:9200/'), | ||
'INDEX_NAME': os.environ.get('ELASTICSEARCH_INDEX_NAME', 'saleor') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think storefront
would be a more reasonable default index name. I'd take descriptive database names over branded ones.
'ENGINE': 'haystack.backends.whoosh_backend.WhooshEngine', | ||
'PATH': os.path.join(os.path.dirname(__file__), 'whoosh_index'), | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting
``ELASTICSEARCH_URL`` | ||
Contains URL address to the elasticsearch cluster. Defaults to ``http://127.0.0.1:9200/``. | ||
|
||
``BONSAI_URL`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data indexing | ||
------------- | ||
|
||
Saleor uses `django-haystack <http://haystacksearch.org/>`_ to provide search engine, please refer to haystack documentation to get familiar with implementation details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"[...] to provide search, [...]" and "Haystack" instead of "django-haystack" and "haystack"
@@ -1,6 +1,8 @@ | |||
from __future__ import unicode_literals | |||
|
|||
import ast | |||
from urlparse import urlparse | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path
is also a platform import.
from urllib.parse import urlparse | ||
except ImportError: | ||
from urlparse import urlparse | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please isort
this.
Is it possible to make sidebar start below the navbar? Can we replace |
|
||
|
||
@staff_member_required | ||
def dashboard_search(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this live in dashboard/search/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep all search related views inside one app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thus is inconsistent with the rest of Saleor.
bac3e02
to
32d9101
Compare
@patrys We no longer depend on Django 1.9, django-haystack 2.5.1 supports new Django versions. |
5e628f6
to
189b46a
Compare
@patrys Box shadow is added to the logo part on navbar for long pages, when side nav stay fixed and content is scrolling: |
a104446
to
cce3f69
Compare
@patrys Could you review this branch? |
a21e9a7
to
696b550
Compare
Reopened on fresh branch in #661 |
Closes #291