Added generic class based search views #1130

troygrosfield · 2015-01-07T17:36:11Z

This addresses the need for django-haystack's django style class based views. This is based on the discussion from the following PR:

https://github.com/toastdriven/django-haystack/pull/826

This work was originally done by @bennylope with only minor changes from his original file:

https://github.com/bennylope/elasticstack/blob/master/elasticstack/views.py

This PR addresses the following issues:

Fixes SearchView should have as_view() method #348
Fixes Align with 1.3 Class-based Views? #350
Fixes allow search views to accept url args and url kwargs #873
Resolves Generic Views #826
Resolves Include py34-django1.7 in .travis.yml #1131
Fixes many tests that were previously broken prior to this PR

troygrosfield · 2015-01-07T18:11:05Z

The only test that's failing looks to be from a configuration change from elasticsearch which is causing all test runs to fail:

======================================================================
ERROR: test_kwargs_are_passed_on (test_haystack.elasticsearch_tests.test_elasticsearch_backend.TestSettings)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/toastdriven/django-haystack/test_haystack/elasticsearch_tests/test_elasticsearch_backend.py", line 204, in test_kwargs_are_passed_on
    backend = ElasticsearchSearchBackend('alias', **{'URL': {}, 'INDEX_NAME': 'testing', 'KWARGS': {'max_retries': 42}})
  File "/home/travis/build/toastdriven/django-haystack/haystack/backends/elasticsearch_backend.py", line 104, in __init__
    self.conn = elasticsearch.Elasticsearch(connection_options['URL'], timeout=self.timeout, **connection_options.get('KWARGS', {}))
  File "/home/travis/build/toastdriven/django-haystack/.eggs/elasticsearch-1.3.0-py2.6.egg/elasticsearch/client/__init__.py", line 150, in __init__
    self.transport = transport_class(_normalize_hosts(hosts), **kwargs)
  File "/home/travis/build/toastdriven/django-haystack/.eggs/elasticsearch-1.3.0-py2.6.egg/elasticsearch/transport.py", line 113, in __init__
    self.set_connections(hosts)
  File "/home/travis/build/toastdriven/django-haystack/.eggs/elasticsearch-1.3.0-py2.6.egg/elasticsearch/transport.py", line 173, in set_connections
    self.connection_pool = self.connection_pool_class(connections, **self.kwargs)
  File "/home/travis/build/toastdriven/django-haystack/.eggs/elasticsearch-1.3.0-py2.6.egg/elasticsearch/connection_pool.py", line 106, in __init__
    specify at least one host.")
ImproperlyConfigured: No defined connections, you need to specify at least one host.

The failure doesn't have anything to do with this PR change.

I'm assuming the elasticsearch change is referring to:

http://elasticsearch-py.readthedocs.org/en/master/Changelog.html#id2

ConnectionPool now checks if any connections were defined

troygrosfield · 2015-01-07T18:50:31Z

Added a fix for the elasticsearch tests which got some of the previously broken tests to successfully run again.

troygrosfield · 2015-01-07T20:46:54Z

haystack/models.py

+                # this changed in change 1.7 to throw an error instead of
+                # returning None when the model isn't found. So catch the
+                # lookup error and keep self._model == None.
+                pass


get_model raises LookupError instead of returning None when no model is found.

https://docs.djangoproject.com/en/1.7/releases/1.7/#introspecting-applications

This is also the first part of the fix to #1069.

troygrosfield · 2015-01-07T21:24:18Z

The only tests now failing I would question if they should be failing. It has to do with django 1.7 and finding models. The remaining django 1.7 errors are as follows:

======================================================================
FAIL: test_missing_object (test_haystack.test_models.SearchResultTestCase)
----------------------------------------------------------------------
_UnexpectedSuccess: 
-------------------- >> begin captured logging << --------------------
haystack: ERROR: Object could not be found in database for SearchResult '<SearchResult: core.mockmodel (pk='1000000')>'.
haystack: ERROR: Model could not be found for SearchResult '<SearchResult: core.yetanothermockmodel (pk='1000000')>'.
haystack: ERROR: Model could not be found for SearchResult '<SearchResult: core.yetanothermockmodel (pk='1000000')>'.
haystack: ERROR: Model could not be found for SearchResult '<SearchResult: core.yetanothermockmodel (pk='1000000')>'.
--------------------- >> end captured logging << ---------------------

The tests are all passing, but since there are items added to the error log, the tests fail. The first one:

haystack: ERROR: Object could not be found in database for SearchResult '<SearchResult: core.mockmodel (pk='1000000')>'.

Is simply because the item was never saved to the database. It makes sense that this would show up in the logs so it could be cleaned up.

The 2nd, 3rd and 4th errors are:

haystack: ERROR: Model could not be found for SearchResult '<SearchResult: core.yetanothermockmodel (pk='1000000')>'.

which also make sense to see in the logs since yetanothermockmodel doesn't actually exist and the only way this case would happen in real life is if you:

had a model
indexed search results, then
removed the model

In that case, I would expect to see an error in the error log as well. So all these errors seem legit so why do we want this test to fail in these cases?

troygrosfield · 2015-01-08T17:05:01Z

@acdha, @lehins, @bennylope any feedback?

acdha · 2015-01-08T20:36:31Z

I think the Django 1.7 errors should be deferred to another ticket - we already have one related to the various failures which are caused by the mock models. The last time @bigjust and I were talking about that, I was strongly in favor of discarding our legacy mock models entirely in favor normal models on the test app since the performance wins aren't worth the maintenance costs.

troygrosfield · 2015-01-09T09:47:45Z

I would think these Django 1.7 changes would be welcome additions seeing how that part of the code was never updated in the first place to correctly reflect expected behavior in Django 1.7.

These fixes get the test suite closer to all passing which is a win all the way around if you ask me. I don't see the issue with those changes in this pr.

troygrosfield · 2015-01-09T09:51:22Z

however, I would be happy to open up a new ticket after this pr is merged based on the comment I made about the remaining portion of the Django 1.7 tests not passing at:

https://github.com/toastdriven/django-haystack/pull/1130#issuecomment-69094597

Thoughts?

acdha · 2015-01-09T15:15:58Z

@troygrosfield Sorry, that wasn't clearly phrased: I have no objection to including those fixes in this ticket – I just figured it'd be good to defer anything which isn't necessary to making the new CBVs work so you don't feel like you have to fix unrelated test failures to get this merged.

troygrosfield · 2015-01-09T16:45:02Z

Happy to help the open source community! This is a good project and I want to see it succeed.

With that said, are there any objections to getting this merged?

troygrosfield · 2015-01-13T06:44:03Z

@acdha, do you have privs to merge? Are we waiting on anything here?

acdha · 2015-01-13T13:45:22Z

The biggest thing I'd like to see would be some tests for the new views to avoid any future regressions.

troygrosfield · 2015-01-13T17:47:37Z

Tests have been added.

acdha · 2015-01-13T19:48:21Z

This is looking good – also like we can remove this expected failure?

https://travis-ci.org/toastdriven/django-haystack/jobs/46879996#L1367

troygrosfield · 2015-01-13T19:55:45Z

Are you referring to the following lines in test_haystack.test_models.SearchResultTestCase:

if django.VERSION >= (1, 7, 0):
        # FIXME: https://github.com/toastdriven/django-haystack/issues/1069
        test_missing_object = unittest.expectedFailure(test_missing_object)

?

acdha · 2015-01-13T22:24:59Z

Yes - the Travis logs made it look like that passed:

======================================================================

FAIL: test_missing_object (test_haystack.test_models.SearchResultTestCase)

----------------------------------------------------------------------

nose.proxy._UnexpectedSuccess:

troygrosfield · 2015-01-13T22:53:40Z

That was already there. This PR didn't change that behavior. However, I'll go ahead and remove it because I don't think it should be there either.

troygrosfield · 2015-01-13T23:21:34Z

Done. The following job needs to be rerun in travis:

https://travis-ci.org/toastdriven/django-haystack/jobs/46920662

After that, all tests should be successfully passing now.

@troygrosfield

Added generic class based search views (thanks @troygrosfield)

acdha · 2015-01-14T15:02:33Z

Next we need to mark the legacy view documentation as deprecated and update it for the new CBVs.

troygrosfield · 2015-01-14T15:07:12Z

👍

troygrosfield · 2015-01-14T18:01:42Z

@acdha, is there a new release planned to push out the new changes anytime soon?

acdha · 2015-01-14T18:43:44Z

There's a fair amount of stuff stacked up for 2.4 which is probably due for another triage round:

https://github.com/toastdriven/django-haystack/milestones/v2.4.0

SalahAdDin · 2015-01-14T20:49:37Z

You guys are working very hard for update haystack, thank you very much :D

dekoza · 2015-01-27T19:56:23Z

@troygrosfield THANK YOU for this! These are the changes I was looking forward to for quite some time.

troygrosfield · 2015-01-27T20:08:58Z

No problem. Happy to help!

acdha · 2015-01-27T21:30:30Z

@dekoza We still have an open ticket to update the documentation – #1133. Feel free to comment there if you have any advice for other users

troygrosfield · 2015-01-29T01:22:17Z

PR is out for the doc to resolve #1133:

https://github.com/toastdriven/django-haystack/pull/1139

@troygrosfield

Thanks to @troygrosfield for the pull-request Closes django-haystack#1139 Closes django-haystack#1133 See django-haystack#1130

SalahAdDin · 2015-02-09T04:04:45Z

Whats about FacetedSearch Views and Forms in 2.4?

troygrosfield · 2015-04-18T21:53:24Z

@acdha, any word on when this (2.4) will be released?

SalahAdDin · 2015-04-20T02:24:34Z

No idea 👍

acdha · 2015-06-05T16:04:28Z

There's a release candidate on PyPI now:

pip install django-haystack==2.4.0rc1

SalahAdDin · 2015-06-06T02:05:17Z

Wow, but, i prefers wait for the oficial release :D.
Thank you. 👍

troygrosfield added 2 commits January 7, 2015 10:29

added generic class based search views.

da7cc0b

Added unicode_literals import for py 2/3 compat.

fc027ba

troygrosfield mentioned this pull request Jan 7, 2015

Generic Views #826

Closed

test for the elasticsearch client.

81b845a

Adding py34-django1.7 to travis.

4869be0

troygrosfield mentioned this pull request Jan 7, 2015

Include py34-django1.7 in .travis.yml #1131

Closed

troygrosfield added 3 commits January 7, 2015 13:25

Adding fix for previously test for django 1.7.

99fb662

fix for django version check.

4a200db

Hopefully last fix for django version checking.

721b942

troygrosfield mentioned this pull request Jan 7, 2015

Update test mocks to deal with Django 1.7's registry #1069

Open

troygrosfield reviewed Jan 7, 2015
View reviewed changes

acdha self-assigned this Jan 12, 2015

troygrosfield added 2 commits January 13, 2015 10:27

Added tests for the generic search view.

b12d5b4

minor update.

85ff8f3

removed "expectedFailure".

22f6437

acdha added a commit that referenced this pull request Jan 14, 2015

Merge pull request #1130 from troygrosfield/master

74b9eba

Added generic class based search views (thanks @troygrosfield)

acdha merged commit 74b9eba into django-haystack:master Jan 14, 2015

troygrosfield mentioned this pull request Jan 14, 2015

Add Documentation for the New Generic Class Based View Added in PR #1130 #1133

Closed

acdha added this to the v2.4.0 milestone Jan 14, 2015

troygrosfield mentioned this pull request Jan 29, 2015

Doc for the new class based views #1139

Closed

troygrosfield unassigned acdha Feb 3, 2015

acdha mentioned this pull request Feb 3, 2015

form_kwargs on basic_search #667

Closed

acdha added a commit to acdha/django-haystack that referenced this pull request Feb 3, 2015

Update documentation for new class-based views

2ae5609

Thanks to @troygrosfield for the pull-request Closes django-haystack#1139 Closes django-haystack#1133 See django-haystack#1130

This was referenced May 29, 2015

SeachView inherits TemplateResponseMixin. #880

Closed

Align with 1.3 Class-based Views? #350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added generic class based search views #1130

Added generic class based search views #1130

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 7, 2015

troygrosfield Jan 7, 2015

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 8, 2015

acdha commented Jan 8, 2015

troygrosfield commented Jan 9, 2015

troygrosfield commented Jan 9, 2015

acdha commented Jan 9, 2015

troygrosfield commented Jan 9, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 14, 2015

troygrosfield commented Jan 14, 2015

troygrosfield commented Jan 14, 2015

acdha commented Jan 14, 2015

SalahAdDin commented Jan 14, 2015

dekoza commented Jan 27, 2015

troygrosfield commented Jan 27, 2015

acdha commented Jan 27, 2015

troygrosfield commented Jan 29, 2015

SalahAdDin commented Feb 9, 2015

troygrosfield commented Apr 18, 2015

SalahAdDin commented Apr 20, 2015

acdha commented Jun 5, 2015

SalahAdDin commented Jun 6, 2015

Added generic class based search views #1130

Added generic class based search views #1130

Conversation

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 7, 2015

troygrosfield Jan 7, 2015

Choose a reason for hiding this comment

troygrosfield commented Jan 7, 2015

troygrosfield commented Jan 8, 2015

acdha commented Jan 8, 2015

troygrosfield commented Jan 9, 2015

troygrosfield commented Jan 9, 2015

acdha commented Jan 9, 2015

troygrosfield commented Jan 9, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

troygrosfield commented Jan 13, 2015

acdha commented Jan 14, 2015

troygrosfield commented Jan 14, 2015

troygrosfield commented Jan 14, 2015

acdha commented Jan 14, 2015

SalahAdDin commented Jan 14, 2015

dekoza commented Jan 27, 2015

troygrosfield commented Jan 27, 2015

acdha commented Jan 27, 2015

troygrosfield commented Jan 29, 2015

SalahAdDin commented Feb 9, 2015

troygrosfield commented Apr 18, 2015

SalahAdDin commented Apr 20, 2015

acdha commented Jun 5, 2015

SalahAdDin commented Jun 6, 2015