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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This aren't used anymore, we use
`readthedocs/templates/search/elastic_search.html`
@stsewd
Copy link
Member Author

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

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

Looks good. Wee!

@ericholscher ericholscher merged commit a7aadbd into readthedocs:master May 1, 2018
@stsewd stsewd deleted the remove-haystack branch May 1, 2018 00:16
@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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants