Add optional search #565

artursmet · 2016-10-06T14:58:18Z

Closes #291

patrys · 2016-10-06T15:14:29Z

requirements.txt

@@ -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


patrys · 2016-10-06T15:15:28Z

saleor/settings.py

@@ -290,3 +292,12 @@
        'IGNORE': [
            r'.+\.hot-update\.js',
            r'.+\.map']}}
+
+
+HAYSTACK_CONNECTIONS = {


Can we make this configurable using env and default to using whoosh?

patrys · 2016-10-06T15:21:39Z

saleor/search/search_indexes.py

+
+
+class ProductVariantIndex(indexes.SearchIndex, indexes.Indexable):
+    text = indexes.CharField(document=True, use_template=True)


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.

artursmet · 2016-10-07T06:39:08Z

@patrys This is In progress pull request, all these problems, you've mentioned will be fixed. Django 1.9 because django-haystack now doesn't support 1.10. I only wanted to open it to have CI builds running :)

patrys · 2016-10-07T07:51:30Z

I know it's in progress but I'm afraid requiring 1.9 may become a blocker.

codecov-io · 2016-10-07T13:01:31Z

Current coverage is 58.26% (diff: 37.51%)

Merging #565 into master will decrease coverage by 5.30%

@@             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

Powered by Codecov. Last update 4408761...dbc6aa2

artursmet · 2016-10-12T09:28:55Z

Implementation is finished and ready for review

artursmet · 2016-10-19T10:03:41Z

We'll be able to merge this as soon as Haystack will support Django 1.10 (django-haystack/django-haystack#1445)

patrys · 2016-10-26T12:45:58Z

docs/configuration.rst

@@ -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``


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?

patrys · 2016-10-26T12:46:43Z

docs/elasticsearch.rst

+
+.. code-block:: bash
+
+    $ pip install elasticsearch


I'd add this to our requirements. It needs to run out of the box on Heroku.

patrys · 2016-10-26T12:51:16Z

saleor/settings.py

+    HAYSTACK_CONNECTIONS = {
+        'default': {
+            'ENGINE': 'haystack.backends.elasticsearch_backend.ElasticsearchSearchEngine',
+            'URL': os.environ.get('ELASTICSEARCH_URL',


We need to also handle BONSAI_URL and SEARCHBOX_URL (these are Heroku add-ons that provide Elasticsearch).

patrys · 2016-10-26T12:52:43Z

saleor/static/js/dashboard.js

+  var $menuToggle = $('.menu-toggle')
+  var $closeMenu = $('#close-menu')
+  var $openMenu = $('#open-menu')
+  function openMenu(a) {


I think we can come up with a more descriptive name than a 😉

patrys · 2016-10-26T12:53:52Z

saleor/static/js/dashboard.js

+    $openMenu.addClass('hide')
+    $closeMenu.removeClass('hide')
+    $menuToggle.addClass('fixed')
+    if ( $windowWidth > 990 ) {


The space inside the parentheses is not needed.

patrys · 2016-10-26T12:55:05Z

saleor/static/js/dashboard.js

+  $closeMenu.click(function() {
+    closeMenu()
+  })
+  if ($windowWidth < 991) {


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?

weronka · 2016-10-26T14:16:21Z

before

after

patrys · 2016-10-26T14:49:17Z

requirements.in

@@ -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


This file is meant to be sorted :)

patrys · 2016-10-26T14:50:59Z

saleor/search/search_indexes.py

+        return Product
+
+    def index_queryset(self, using=None):
+        qs = self.get_model().objects.get_available_products()


We need to index all products and reimplement availability checks at search time (but only when searching the site and not the dashboard).

patrys · 2016-10-26T14:52:57Z

saleor/search/utils.py

+logger = logging.getLogger(__name__)
+
+
+def update_object(instance, using='default'):


Do we gain anything from having a "generic" solution like this? Why not have explicit update_product, update_user etc.?

patrys · 2016-10-26T14:54:01Z

saleor/search/views.py

+from ..userprofile.models import User
+
+
+def search_for_model(request, models):


The name suggests it would search for a single model.

patrys · 2016-10-26T14:54:41Z

saleor/search/views.py

+            raise Http404("No such page!")
+    else:
+        page = form.no_query_found()
+    return {'page': page, 'query': form.cleaned_data['q']}


Consider returning a tuple or a namedtuple?

patrys · 2016-10-26T14:56:59Z

saleor/settings.py

+            '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')


I think storefront would be a more reasonable default index name. I'd take descriptive database names over branded ones.

patrys · 2016-10-26T14:57:16Z

saleor/settings.py

+            'ENGINE': 'haystack.backends.whoosh_backend.WhooshEngine',
+            'PATH': os.path.join(os.path.dirname(__file__), 'whoosh_index'),
+        },
+    }


patrys · 2016-10-27T10:01:59Z

docs/elasticsearch.rst

+``ELASTICSEARCH_URL``
+  Contains URL address to the elasticsearch cluster. Defaults to ``http://127.0.0.1:9200/``.
+
+``BONSAI_URL``


See http://saleor.readthedocs.io/en/latest/configuration.html#environment-variables (CACHE_URL)

patrys · 2016-10-27T10:03:06Z

docs/elasticsearch.rst

+Data indexing
+-------------
+
+Saleor uses `django-haystack <http://haystacksearch.org/>`_ to provide search engine, please refer to haystack documentation to get familiar with implementation details.


"[...] to provide search, [...]" and "Haystack" instead of "django-haystack" and "haystack"

patrys · 2016-10-27T10:12:37Z

saleor/settings.py

@@ -1,6 +1,8 @@
 from __future__ import unicode_literals

 import ast
+from urlparse import urlparse
+


os.path is also a platform import.

patrys · 2016-10-27T10:15:40Z

saleor/settings.py

+    from urllib.parse import urlparse
+except ImportError:
+    from urlparse import urlparse
+


Please isort this.

patrys · 2016-10-27T10:27:00Z

@weronka

Is it possible to make sidebar start below the navbar?

Can we replace |< icon with a full breadcrumb trail for now? (ie. "Products ⟩ Kirby, Clark and Green ▼")

patrys · 2016-10-27T11:59:36Z

saleor/search/views.py

+
+
+@staff_member_required
+def dashboard_search(request):


Shouldn't this live in dashboard/search/?

I wanted to keep all search related views inside one app

Thus is inconsistent with the rest of Saleor.

artursmet · 2016-10-31T08:59:08Z

@patrys We no longer depend on Django 1.9, django-haystack 2.5.1 supports new Django versions.

weronka · 2016-11-04T15:54:13Z

@patrys
Sidebar already starts below the navbar. Attaching screenshot with closed sidebar:

Box shadow is added to the logo part on navbar for long pages, when side nav stay fixed and content is scrolling:

artursmet · 2016-11-15T13:50:04Z

@patrys Could you review this branch?

artursmet · 2016-12-15T10:30:16Z

Reopened on fresh branch in #661

artursmet added the in progress label Oct 6, 2016

patrys requested changes Oct 6, 2016

View reviewed changes

artursmet force-pushed the add-optional-search branch from 24f58dd to 1cba4a3 Compare October 10, 2016 13:59

artursmet removed the in progress label Oct 12, 2016

artursmet force-pushed the add-optional-search branch from 3d00d65 to d97f7cb Compare October 12, 2016 09:54

weronka force-pushed the add-optional-search branch 4 times, most recently from 65e5971 to 480c850 Compare October 26, 2016 08:56

patrys requested changes Oct 26, 2016

View reviewed changes

patrys requested changes Oct 27, 2016

View reviewed changes

patrys reviewed Oct 27, 2016

View reviewed changes

artursmet force-pushed the add-optional-search branch 2 times, most recently from bac3e02 to 32d9101 Compare October 27, 2016 13:56

artursmet assigned patrys Oct 31, 2016

artursmet force-pushed the add-optional-search branch 2 times, most recently from 5e628f6 to 189b46a Compare October 31, 2016 09:36

artursmet force-pushed the add-optional-search branch 2 times, most recently from a104446 to cce3f69 Compare November 15, 2016 13:49

patrys and others added 23 commits December 14, 2016 11:28

Use elasticsearch when dockerized

680e4aa

Move search form to header block

67e8445

Improve documentation

dd5c9cf

Drop unused module

7ffa3f0

Don't track whoosh files

f1efc2a

Move results filtering to a helper

1a9b072

Update search index after successful order placement

d23845e

Improve code style

e12ecdc

Index more useful data

35ffd5e

Use more verbose label

5a03487

Index order number as well

ace65f0

Fix header template

583062d

Add/edit stock location view

bf6ebb7

Update search index after updating object in dashboard

30da5f3

Drop generic update_object method and use small model specific ones

9278c32

Use more verbose helper names

3bb2dc4

Don't require haystack

1d0fa73

Adapt search from wagtailsearch app

558cf13

WIP wagtail based multi-model search

917aaf2

Improve multi model search

2fba220

Use different backends on storefront and dashboard

dc6516f

Allow to override default queryset

bda7d64

Improve dashboard results template

696b550

artursmet force-pushed the add-optional-search branch from a21e9a7 to 696b550 Compare December 14, 2016 10:28

artursmet added 4 commits December 14, 2016 11:49

Implement search auto update

9e7b277

Index more details

88d6f25

Drop outdated test

7415959

Index users as well

dbc6aa2

artursmet closed this Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional search #565

Add optional search #565

artursmet commented Oct 6, 2016

patrys Oct 6, 2016

patrys Oct 6, 2016

patrys Oct 6, 2016

artursmet commented Oct 7, 2016 •

edited

patrys commented Oct 7, 2016

codecov-io commented Oct 7, 2016 •

edited

artursmet commented Oct 12, 2016

artursmet commented Oct 19, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

weronka commented Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 26, 2016

patrys Oct 27, 2016

patrys Oct 27, 2016

patrys Oct 27, 2016

patrys Oct 27, 2016

patrys commented Oct 27, 2016

patrys Oct 27, 2016

artursmet Oct 27, 2016

patrys Oct 31, 2016

artursmet commented Oct 31, 2016

weronka commented Nov 4, 2016

artursmet commented Nov 15, 2016

artursmet commented Dec 15, 2016



		class ProductVariantIndex(indexes.SearchIndex, indexes.Indexable):
		text = indexes.CharField(document=True, use_template=True)

		logger = logging.getLogger(__name__)


		def update_object(instance, using='default'):

		from ..userprofile.models import User


		def search_for_model(request, models):

Add optional search #565

Add optional search #565

Conversation

artursmet commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artursmet commented Oct 7, 2016 • edited

patrys commented Oct 7, 2016

codecov-io commented Oct 7, 2016 • edited

Current coverage is 58.26% (diff: 37.51%)

artursmet commented Oct 12, 2016

artursmet commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weronka commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrys commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artursmet commented Oct 31, 2016

weronka commented Nov 4, 2016

artursmet commented Nov 15, 2016

artursmet commented Dec 15, 2016

artursmet commented Oct 7, 2016 •

edited

codecov-io commented Oct 7, 2016 •

edited