Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Cleanup blogs app #1336
Cleanup blogs app #1336
Conversation
| 'content': rendered_box, | ||
| 'content_markup_type': 'html', | ||
| } | ||
| ) |
jaap3
Sep 25, 2018
Author
Contributor
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?
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?
berkerpeksag
Apr 6, 2019
Member
'supernav-python-blog' is used in static/js/script.js.
Your rewrite is much better than the original code.
'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) | |||
jaap3
Sep 25, 2018
Author
Contributor
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.
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.
berkerpeksag
Apr 6, 2019
Member
I think having more tests wouldn't hurt :) But it can be done in a separate PR.
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 | |||
jaap3
Sep 25, 2018
Author
Contributor
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.
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> | |||
jaap3
Sep 25, 2018
Author
Contributor
There was a serious potential for xss here, locally I could see the images FeedBurner inserts to share a blogpost.
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', | ||
| } | ||
| ) |
berkerpeksag
Apr 6, 2019
Member
'supernav-python-blog' is used in static/js/script.js.
Your rewrite is much better than the original code.
'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) | |||
berkerpeksag
Apr 6, 2019
Member
I think having more tests wouldn't hurt :) But it can be done in a separate PR.
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)