-
-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSS for shelves #3013
base: main
Are you sure you want to change the base?
RSS for shelves #3013
Conversation
Currently can't get the test to succeed - it fails in an unrelated redis error, so pushing this so I can open a draft PR to get advice on a better test.
didn't really need to add the shelf to self and the linter doesn't like seeing it outside of an init or setup.
@mouse-reeve any idea why the test fails? |
@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? |
Fixed. This is ready for review. |
@bookwyrm-social/code-review if someone has time to review this that would be awesome 🙂 |
This sounds so useful! I hope somebody has a chance to review it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for this feature, and apologies the review took so long. I'm sending you my suggestions, minus i18n issues (which I'll do in a second pass).
Most of these suggestions are minor, or self-explanatory. Please let me know what you think.
@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. |
This will make sure that users see books as they are shelved, which is what would be expected. Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
getting context data no longer seems needed, if it ever was.
@dato thanks for the thorough review. I've addressed all comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with the first two suggestions below!
Ah one last thing for a follow-up PR: producing a |
Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
Co-authored-by: Adeodato Simó <73768+dato@users.noreply.github.com>
in book_identifiers.html template we support multiple identifiers in order. This commit adopts the same logic and order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again—
I would like to merge soon so I can see some incremental value
Yes, I support this as well.
I think we can merge, with the tiny adjustments below (needed for a green build; I didn't provide good instructions for blocktrans usage, sorry).
I've spotted a few issues, review coming shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally seems to work, just a couple of things to clean up.
{% blocktrans trimmed with book_title=obj.title book_author=obj.author_text %} | ||
‘{{ book_title }}’ by {{ book_author }} | ||
{% endblocktrans %} | ||
{{obj.description|default:""}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a blocker because it seems to be a bit difficult to manage embedded HTML in the Django syndication framework, but I noticed this just runs on without line breaks so it renders like Title of Book by Jo Bloggs Here is a description of the book
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to do here.
If I could figure out how to trigger a cdata block, we could do something to embed new lines.
This seems like an approach someone got working, if it makes sense to you, I can take a swing at it.
https://nemecek.be/blog/33/how-to-create-rss-feed-with-html-content-in-django
For my purposes, I'm very interested in getting the book ISBN and other identifiers in as raw a format as possible, but I'd like to make sure this feed is useful and looks good for humans in a feed reader.
When I look at how other book tracking sites handle this, they use the cdata block pretty liberally.
They also just introduce custom elements like isbn and asin directly in the feed. Which doesn't seem valid xml but it seems not to blow anything up.
I'm not ready to dedicate myself just yet to implementing a custom feed generator as described in https://docs.djangoproject.com/en/5.0/ref/contrib/syndication/#custom-feed-generators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw @hughrun - this is listed as a blocking change I think - assuming I'm not misreading githubs UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at this shortly.
If there is no author for a book, render just the title of the book.
@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. |
This seems to correctly implement an RSS feed for shelves, closing #2871,