Cleanup blogs app #1336
Cleanup blogs app #1336
Conversation
| 'content': rendered_box, | ||
| 'content_markup_type': 'html', | ||
| } | ||
| ) |
There was a problem hiding this comment.
This change makes it not rely on a hardcoded primary key and it doesn't crash when the feed doesn't exist or there are no BlogEntries at all. Which will happen on a "fresh" installation.
However, I'm not sure if the 'supernav-python-blog' is actually used anymore. I don't see it on the live site. So maybe the entire supernav stuff can be removed?
There was a problem hiding this comment.
'supernav-python-blog' is used in static/js/script.js.
Your rewrite is much better than the original code.
| @@ -33,9 +29,6 @@ def test_get_latest_entries(self): | |||
| entries[0].pub_date.isoformat(), | |||
| '2013-03-04T15:00:00+00:00' | |||
| ) | |||
| b = Box.objects.get(label='supernav-python-blog') | |||
| rendered_box = _render_blog_supernav(BlogEntry.objects.latest()) | |||
| self.assertEqual(b.content.raw, rendered_box) | |||
There was a problem hiding this comment.
I removed this because it didn't test much and relied on a side effect of running the update_blogs and management command and the previously hardcoded primary key in the update_blog_supernav function. All while claiming to be a test for a template tag.
If updating the supernav is still required I can add proper tests.
There was a problem hiding this comment.
I think having more tests wouldn't hurt :) But it can be done in a separate PR.
| @@ -61,14 +54,14 @@ def test_feed_list(self): | |||
| summary='', | |||
| pub_date=now(), | |||
| url='path/to/foo', | |||
| feed=f1 | |||
| feed=f2 | |||
There was a problem hiding this comment.
The second feed never got a blog entry, so this test was only testing that items from f1 were returned. It seems to me the intention was to test that entries of both feeds are returned.
| @@ -2,6 +2,6 @@ | |||
| <p class="date-posted"><small>{{ entry.pub_date|date:"l, F j, Y" }}</small></p> | |||
| <h4>{{ entry.title }}</h4> | |||
| <p class="excerpt"> | |||
| <small>{{ entry.summary|truncatewords_html:50|safe }}<a class="readmore" href="{{ entry.url }}">Read more</a></small> | |||
| <small>{{ entry.summary|truncatewords_html:50|striptags|safe }}<a class="readmore" href="{{ entry.url }}">Read more</a></small> | |||
There was a problem hiding this comment.
There was a serious potential for xss here, locally I could see the images FeedBurner inserts to share a blogpost.
| 'content': rendered_box, | ||
| 'content_markup_type': 'html', | ||
| } | ||
| ) |
There was a problem hiding this comment.
'supernav-python-blog' is used in static/js/script.js.
Your rewrite is much better than the original code.
| @@ -33,9 +29,6 @@ def test_get_latest_entries(self): | |||
| entries[0].pub_date.isoformat(), | |||
| '2013-03-04T15:00:00+00:00' | |||
| ) | |||
| b = Box.objects.get(label='supernav-python-blog') | |||
| rendered_box = _render_blog_supernav(BlogEntry.objects.latest()) | |||
| self.assertEqual(b.content.raw, rendered_box) | |||
There was a problem hiding this comment.
I think having more tests wouldn't hurt :) But it can be done in a separate PR.
It's almost possible to just use update_or_create, but not quite because of the `pub_date` check.
It now handles the cases when there is no Feed, the desired feed has an id other than 1 and when there are no blog entries at all. Before this change this function would raise a `BlogEntry.DoesNotExist` exception.
|
Thanks! |
I might have gotten slightly carried away with cleaning this up, so feel free to reject this wholesale or ask me to create separate PRs for the things that are actually OK.
What I did:
update_or_create, but just not quite)