Conversation
- defer StreamFields - fall back to the search description field if the listing summary is not set - add author names - fetch related feed image in blog post queryset
helenb
left a comment
helenb
left a comment
There was a problem hiding this comment.
Thanks @zerolab - I have reviewed and tested. Added a couple of comments and queries (with help from Claude) for you to check. Apologies for the code suggestions not replacing the right lines of code but hopefully you get the idea....
tbx/blog/feeds.py
Outdated
|
|
||
| class BlogFeed(Feed): | ||
| title = "The Torchbox Blog" | ||
| link = "/blog/" |
There was a problem hiding this comment.
Can you update this while you are making changes? The link generated is https://torchbox.com/blog/ which immediately redirects to https://torchbox.com/news - we might as well directly link to /news
There was a problem hiding this comment.
(I don't know if there's an option in the RSS spec to have multiple links - if there is that would be ideal as the feed combines our multiple blog pages)
| def item_link(self, item): | ||
| return item.get_full_url() | ||
| def item_link(self, item: BlogPage) -> str: | ||
| return item.get_full_url(request=self.request) |
There was a problem hiding this comment.
Can you explain this change a bit more? I get the exact same result for the urls before and after the change.
There was a problem hiding this comment.
tbx/blog/feeds.py
Outdated
| def item_enclosure_mime_type(self, item): | ||
| def item_enclosure_mime_type(self, item: BlogPage) -> str | None: | ||
| if item.feed_image: | ||
| image_format = filetype.guess_extension(item.feed_image.file) |
There was a problem hiding this comment.
Although this isn't a change, claude flagged a possible issue here that we could fix. filetype.guess_extension() can return None if the format can't be determined which would result in a mime type of image/None as the MIME type.
| image_format = filetype.guess_extension(item.feed_image.file) | |
| def item_enclosure_mime_type(self, item: BlogPage) -> str | None: | |
| if item.feed_image: | |
| try: | |
| image_format = filetype.guess_extension(item.feed_image.file) | |
| if image_format: | |
| return f"image/{image_format}" | |
| except (OSError, AttributeError): | |
| pass | |
| return None |
The above was it's suggested change as 'RSS feed readers might reject enclosures with invalid MIME types like "image/None"'. I don't think it is a big issue as I don't think we'd ever have images with an invalid mime type, but could be defensive programming to update it.
| return f"image/{image_format}" | ||
|
|
||
| def item_enclosure_length(self, item): | ||
| def item_enclosure_length(self, item: BlogPage) -> str | None: |
There was a problem hiding this comment.
Another code review issue that claude raised - will leave you to judge if it is valid (I don't know if it's correct about what file.size returns but seems plausible that it is an int):
Issue: The return type is str | None, but file.size returns an int. This creates a type mismatch.
| def item_enclosure_length(self, item: BlogPage) -> str | None: | |
| def item_enclosure_length(self, item: BlogPage) -> str | None: | |
| if item.feed_image: | |
| return str(item.feed_image.file.size) | |
| return None |
Impact:
- RSS spec requires enclosure length to be a string
- Current code might work due to Python's implicit conversion, but explicit is better
2 participants
Description of Changes Made
This PR makes the following changes to the feed:
How to Test
Go to /blog/feed, download the resulting file and open with an editor, check the outputs match
Screenshots
Expand to see more
Metadata
Checklists
MR Checklist
Unit tests
Documentation
Browser testing
Data protection
Light and dark mode
Accessibility
Sustainability
Pattern library