fix Post.get_keywords to handle empty keywords, cleanup search_indexes #205

skirsdeda · 2016-02-15T16:09:32Z

Tried using search_indexes got some exceptions (like None returned from get_keywords). Ended up cleaning up the entire PostIndex.

skirsdeda · 2016-02-15T16:10:15Z

djangocms_blog/search_indexes.py

@@ -47,32 +47,32 @@ def get_model(self):
        return Post

    def get_search_data(self, post, language, request):
-        optional_attributes = []


this optional_attributes thing was never used :)

It seems a zombie code, yeah. The idea behind, if i remember correctly, was to provide optional attribute to extend the search. Maybe we could just remove it and eventually develop the idea in a separated branch.

skirsdeda · 2016-02-15T16:46:10Z

Tests have to be rewritten. I will try to finish this up tomorrow.

skirsdeda · 2016-02-16T13:37:46Z

djangocms_blog/search_indexes.py

@@ -28,7 +25,7 @@ def get_description(self, post):
        return post.get_description()

    def prepare_pub_date(self, post):
-        return post.date_published.strftime("%Y-%m-%d %H:%M:%S")


Haystack handles datetime objects, strftime is really unnecessary.

skirsdeda · 2016-02-16T14:52:40Z

This is pretty much finished. But there is an interesting thing going on with date_published. For some reason the prepared datetime differs a few seconds from the one saved. Not sure what to blame yet :)

yakky · 2016-02-16T14:59:01Z

@skirsdeda let's ping @fmarco who worked on the first implementation of the haystack indexes IIRC he had time skewes issues as well

skirsdeda · 2016-02-16T15:25:22Z

@yakky I would say that the logic in test_blog_post_is_indexed_using_update_object is wrong. The other one (test_blog_post_is_indexed_using_prepare) always passes.

skirsdeda · 2016-02-16T15:45:17Z

@yakky Actually test_blog_post_is_indexed_using_update_object calls index.update_object which calls backend.update (inside Haystack) which is noop (since HAYSTACK_CONECTIONS is empty hence backend is the default - SimpleSearchBackend). So it should return the date from a previously prepared index and therefore the difference. Now I thought that test function call order is non-deterministic but I guess I was wrong?

skirsdeda · 2016-02-16T15:46:36Z

Also note that _get_post calls Post.objects.create which makes sure we get a new instance (and a new date_published therefore).

skirsdeda · 2016-02-16T18:58:54Z

So I removed the offending test which did nothing more than the one that is left now.

skirsdeda · 2016-02-17T08:24:45Z

Also removed get_title from get_search_data because index_title = True. No need to repeat it twice.

fmarco · 2016-02-17T16:31:09Z

I'll have a deep look into ASAP @skirsdeda

fmarco · 2016-02-19T11:04:15Z

djangocms_blog/search_indexes.py

@@ -47,32 +44,36 @@ def get_model(self):
        return Post

    def get_search_data(self, post, language, request):
-        optional_attributes = []
-        abstract = post.safe_translation_getter('abstract')
-        text_bits = [post.get_title()]


I'll keep this one, because it returns the meta_title or the title as a fallback, so it's not a real repetition.
See: https://github.com/nephila/djangocms-blog/blob/develop/djangocms_blog/models.py#L228

Btw, we can discuss about it :)!

skirsdeda · 2016-02-19T11:33:13Z

@fmarco Since index_title=True, it gets included anyway. I changed get_title to use post.get_title so that fallback is used when needed.

fmarco · 2016-02-19T11:39:09Z

djangocms_blog/search_indexes.py

@@ -15,20 +15,17 @@ class PostIndex(get_index_base()):

    author = indexes.CharField(indexed=True, model_attr='get_author')
    keywords = indexes.CharField(null=True)
-    tags = indexes.CharField(null=True)
+    tags = indexes.CharField(null=True, model_attr='get_tags')


fmarco · 2016-02-19T11:41:30Z

@skirsdeda Ok, it's an idea. By the way i think we could keep as a separated default search attribute the title, in the case we want to search for the title or the meta_title. @yakky what do you think?

skirsdeda · 2016-02-19T11:45:40Z

@fmarco Regarding title, I personally use index_title=False and when searching I search both in title and text. And that's how it usually is because title gets bigger boost and documents matching query in title get bigger score.

fmarco · 2016-02-19T12:29:06Z

djangocms_blog/models.py

@@ -232,7 +232,7 @@ def get_title(self):
        return title.strip()

    def get_keywords(self):
-        return self.safe_translation_getter('meta_keywords').strip().split(',')
+        return self.safe_translation_getter('meta_keywords', default='').strip().split(',')


Ok, we could move it directly here: https://github.com/nephila/djangocms-blog/blob/develop/djangocms_blog/models.py#L235

ps: btw, how can the error be reproduced? Because here: https://github.com/nephila/djangocms-blog/blob/develop/djangocms_blog/models.py#L133 "meta_keywords" have the '' default.
Just to have an idea of the flow error. Anyway, it's a sane check.

@fmarco Now that is interesting because I have all migrations applied and yet meta_keywords is not null with no default in database. Actually all fields in djangocms_blog_post_translation are not null with no default... So either there were some migration changes after they were first created or it has to do with parler. I have to investigate this a bit.

@fmarco So Django doesn't save default values in DB schema. Means that all is good with migrations. I bet I am hitting some django-parler bug here. Maybe it doesn't consider field's default value in safe_translation_getter for some reason.

Argh! So that's the problem that causes the issue!
So, the stronger check have even more sense.

@fmarco Yup, it's django-parler's fault. Filed an issue. But for now it's better to have default='' to make safe_translation_getter really safe :)

Yes, I agree :)! 👍

ps: so, as said above let's make the check on the Post model method get_keywords to have a default '' fallback 👍 .

fmarco · 2016-02-19T13:47:26Z

@skirsdeda About test_blog_post_is_indexed_using_update_object test, do you any suggestion to make test using a different backend? Otherwise we keep it removed as you did :).

skirsdeda · 2016-02-19T13:57:46Z

@fmarco If you use a different backend you will need to pull in some dependencies for tests. For Solr and elasticsearch you would have to install and run an actual service which is a bit of an overkill for one test. Also it would make Travis builds considerably longer. The only option is using Whoosh backend which just runs as a python lib.

fmarco · 2016-02-19T14:03:39Z

@skirsdeda I see, infact i read that someone try to use services (as even Docker containers to run those services), but i agree with you that's an overkill for the moment. Maybe we could integrate a whoosh based test next.

fmarco · 2016-02-19T15:38:37Z

@yakky LGTM

yakky · 2016-02-23T20:36:54Z

@skirsdeda thanks for your work
@fmarco thanks for the review

fix Post.get_keywords to handle empty keywords, cleanup search_indexes

skirsdeda reviewed Feb 15, 2016
View reviewed changes

skirsdeda reviewed Feb 16, 2016
View reviewed changes

skirsdeda force-pushed the fix_search_indexes branch from 85a17db to 977b0ef Compare February 16, 2016 13:47

yakky self-assigned this Feb 16, 2016

skirsdeda added 2 commits February 16, 2016 20:26

fix Post.get_keywords to handle empty keywords, cleanup search_indexes

55bf324

remove defunct test

a85cc16

skirsdeda force-pushed the fix_search_indexes branch from 2285ce3 to a85cc16 Compare February 16, 2016 18:26

yakky assigned fmarco and unassigned yakky Feb 17, 2016

don't repeat blog title in search index twice

f28a2cd

fmarco reviewed Feb 19, 2016
View reviewed changes

use post.get_title() to get fallback title from meta

d3f64cc

fmarco reviewed Feb 19, 2016
View reviewed changes

yakky assigned yakky and unassigned fmarco Feb 19, 2016

yakky added a commit that referenced this pull request Feb 23, 2016

Merge pull request #205 from skirsdeda/fix_search_indexes

1941cc5

fix Post.get_keywords to handle empty keywords, cleanup search_indexes

yakky merged commit 1941cc5 into nephila:develop Feb 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Post.get_keywords to handle empty keywords, cleanup search_indexes #205

fix Post.get_keywords to handle empty keywords, cleanup search_indexes #205

skirsdeda commented Feb 15, 2016

skirsdeda Feb 15, 2016

fmarco Feb 19, 2016

skirsdeda commented Feb 15, 2016

skirsdeda Feb 16, 2016

fmarco Feb 19, 2016

skirsdeda commented Feb 16, 2016

yakky commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 17, 2016

fmarco commented Feb 17, 2016

fmarco Feb 19, 2016

skirsdeda commented Feb 19, 2016

fmarco Feb 19, 2016

fmarco commented Feb 19, 2016

skirsdeda commented Feb 19, 2016

fmarco Feb 19, 2016

fmarco Feb 19, 2016

skirsdeda Feb 19, 2016

skirsdeda Feb 19, 2016

fmarco Feb 19, 2016

skirsdeda Feb 19, 2016

fmarco Feb 19, 2016

fmarco Feb 19, 2016

fmarco commented Feb 19, 2016

skirsdeda commented Feb 19, 2016

fmarco commented Feb 19, 2016

fmarco commented Feb 19, 2016

yakky commented Feb 23, 2016

fix Post.get_keywords to handle empty keywords, cleanup search_indexes #205

fix Post.get_keywords to handle empty keywords, cleanup search_indexes #205

Conversation

skirsdeda commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirsdeda commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirsdeda commented Feb 16, 2016

yakky commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 16, 2016

skirsdeda commented Feb 17, 2016

fmarco commented Feb 17, 2016

Choose a reason for hiding this comment

skirsdeda commented Feb 19, 2016

Choose a reason for hiding this comment

fmarco commented Feb 19, 2016

skirsdeda commented Feb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco commented Feb 19, 2016

skirsdeda commented Feb 19, 2016

fmarco commented Feb 19, 2016

fmarco commented Feb 19, 2016

yakky commented Feb 23, 2016