Skip to content
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

Remove haystack from code base #4039

Merged
merged 6 commits into from May 1, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 28, 2018

Ok, this is a big remove (I'm more worried more about the apiv1 and some templates).

This closes #4006

RTD doesn't use haystack on production anymore. Also, I try some endpoints

Both give a 500 error code.

The only part that haystack was really being used was on some templates to use the highlight https://django-haystack.readthedocs.io/en/master/templatetags.html#highlight tag, so, I'm not sure if it's good to remove this from here since it gives a nice visual hint for the user, maybe we can search another lib that gives the same functionality or maybe this isn't so relevant and we are ok removing it.

Note: I wasn't able to full tests this on the search page with results because I don't have elasticsearch, I will try to setup elasticsearch to test this later.

@@ -84,9 +84,6 @@ def INSTALLED_APPS(self): # noqa
'annoying',
'django_extensions',
'messages_extends',

# daniellindsleyrocksdahouse
Copy link
Member Author

@stsewd stsewd Apr 28, 2018

Weird comment here 😁

@@ -15,7 +14,7 @@
<a href="{% if result.absolute_url %}{{ result.absolute_url }}{% else %}{{ result.object.get_absolute_url }}{% endif %}?highlight={{ query }}">{{ result.project }} - {{ result.title }}</a>
</p>
<p>
{% highlight result.text with query %}
{% result.text %}
Copy link
Member Author

@stsewd stsewd Apr 28, 2018

We are losing some visual hints here

@@ -55,7 +55,7 @@ <h5>{% trans "Filter by Project" %}</h5>
<a href="{% if result.absolute_url %}{{ result.absolute_url }}{% else %}{{ result.object.get_absolute_url }}{% endif %}?highlight={{ query }}">{{ result.project }} {% if result.version %} ({{ result.version }}) {% endif %} - {{ result.title }}</a>
</p>
<p>
{% highlight result.text with query %}
{% result.text %}
Copy link
Member Author

@stsewd stsewd Apr 28, 2018

...and here

@@ -23,8 +23,6 @@ readthedocs-build<2.1
# django-tastypie 0.13.x and 0.14.0 are not compatible with our code
django-tastypie==0.13.0

django-haystack==2.8.1
celery-haystack==0.10
Copy link
Member Author

@stsewd stsewd Apr 28, 2018

I'm not sure where the celery-haystack was used, I assume it was some dependency from django-haystack

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 28, 2018

Travis is all green, that's a good sign 🍏

...Or we don't have enough tests for apiv1 😨

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 29, 2018

haystack is not used for search results. The entire set of search templates referenced here can also be deleted, as our search results use elastic_search.html now: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/search/views.py#L94

@stsewd stsewd force-pushed the remove-haystack branch from 04d2818 to ae48838 Apr 30, 2018
stsewd added 2 commits Apr 30, 2018
This aren't used anymore, we use
`readthedocs/templates/search/elastic_search.html`
@stsewd
Copy link
Member Author

@stsewd stsewd commented May 1, 2018

I deleted the templates that were used previously, I checked the search pages and those work correctly and also I checked some endpoints and those are good too.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 1, 2018

Looks good. I'll give this a spin locally as well, but I don't forsee any issues since haystack hasn't been used in years.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 1, 2018

Looks good. Wee!

@ericholscher ericholscher merged commit a7aadbd into readthedocs:master May 1, 2018
1 check passed
@stsewd stsewd deleted the remove-haystack branch May 1, 2018
@stsewd stsewd mentioned this pull request Jun 14, 2018
@stsewd stsewd mentioned this pull request Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants