bookwyrm
bookwyrm copied to clipboard
bookwyrm-social
RSS for shelves
@jaschaurbach good news is that it looks to be a very similar error from the failed test on github vs what I saw locally. Somehow I'm not mocking something out well enough and the test is trying to publish things to redis etc - none of that is set up in the test environment so everything goes boom. I hope to revisit this sometime when I can spend time figuring out why my test explodes.
It's frustrating to get the logic working locally but not the test, but for a project like this where it's just... some people ... working on it, I sympathize that there isn't time to triage the work of everyone that drops by with a pull request. If I can fix it, I will. If mouse or someone else competent sees what I'm doing wrong, that's great.
If I just need to pull an RSS feed of everything for my purposes and then filter out to just the to-read items, I can do that on my side.
@hughrun - all checks passed - hopefully the commits are not controversial.
@mattkatz the title of this is still 'DRAFT' - is this ready for review or are you still working on it?
@mattkatz the title of this is still 'DRAFT' - is this ready for review or are you still working on it?
Fixed. This is ready for review.
@bookwyrm-social/code-review if someone has time to review this that would be awesome 🙂
@dato - I'll take a look. And thank YOU for volunteering time to do this. I know this is real work and effort and I appreciate it. I'm checking it out now.
@dato thanks for the thorough review. I've addressed all comments. I haven't added new tests around ordering - I don't know that it's worth the test load. If it's needed I'd update the test rss to add 2 books, figure out how to add a published date to each, then add the them to the shelf and expect that ordering is maintained.
Ah one last thing for a follow-up PR: producing a <link rel="alternate" type="application/rss+xml" href="…" title="…" /> element in the template for public shelves (unlisted too?).
@mattkatz I've made a PR in your repository with a fix for the template issue, and added a link the head to enable auto-discovery of the feeds.
Sorry, I kind of lead you in the wrong direction a bit with adding logic for missing authors. We can't put logic inside block tags. But when I thought about it more I realised we shouldn't be translating titles and descriptions anyway: e.g. if it's a book in French the title should be in French, not in whatever language I have my device set to display.
If you're happy with my changes just merge them in to your branch and they will flow through to this PR.