Skip to content
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

Implement RSS/Atom feeds #188

Merged
merged 7 commits into from
Nov 19, 2021
Merged

Implement RSS/Atom feeds #188

merged 7 commits into from
Nov 19, 2021

Conversation

JMAlego
Copy link
Member

@JMAlego JMAlego commented Nov 2, 2021

Implementation for RSS/Atom feeds for the website.

  • RSS feed
  • Atom feed
  • Work out correct way to get absolute URL from flask, context.yaml possible?
  • Validate generated XML as RSS/Atom

Fixes #159

@JMAlego JMAlego added backend Relating to the code which runs on the server and generates the site enhancement low priority an issue that doesn't need to be addressed soon labels Nov 2, 2021
@JMAlego
Copy link
Member Author

JMAlego commented Nov 2, 2021

@LukeMoll do you know the correct way to generate an absolute URL from flask? During development or when running dynamically you can use url_for with the _external=True parameter (which feels wrong as it is). When freezing we probably need to store the absolute URL in somewhere like context.yaml right?

@JMAlego JMAlego changed the title Feature/rss Implement RSS/Atom feeds Nov 2, 2021
@LukeMoll
Copy link
Member

LukeMoll commented Nov 2, 2021

What feels wrong about _external? This seems like the most useful way to do it in Flask.

Given the GitHub repository is hard-coded in filters.py, I'd be comfortable hard-coding the website URL.

As per https://flask.palletsprojects.com/en/2.0.x/api/#flask.url_for, we can set the URL root with SERVER_NAME. The tricky bit would be handling the difference between development and production environments when freezing; the former probably wants it set to localhost:8000 (icr the port number), but the latter would use the actual webroot. We could set it to always use the actual webroot, but this could hinder development with absolute URLs (when looking at a locally-frozen copy). In fact, as long as this difference is documented, I think I'd be happy having the dynamic site use the default behaviour (from HTTP Host header) and all frozen sites use the website URL root (hard-coded or similar). Additionally, absolute URL use (that includes a host/domain, rather than being implicitly on the same domain) should be minimised.

The note in the docs that

Configuration values APPLICATION_ROOT and SERVER_NAME are only used when generating URLs outside of a request context.

is concerning, as AIUI we'd always be generating in a request context.

@JMAlego JMAlego marked this pull request as ready for review November 3, 2021 19:12
@JMAlego
Copy link
Member Author

JMAlego commented Nov 3, 2021

This is now ready for review with two open questions:

Where should we keep the files? I've gone with rss.xml and atom.xml but we could equally use feed.rss and feed.atom, there isn't really a standard.

Does all the information generated seem correct? It looks it to me and I've validated the generated RSS/Atom using the W3C validator (it is valid).

I've used FREEZER_BASE_URL which turns out to do exactly what I wanted, the default is hardcoded to https://hacksoc.org but that can be changed in production/development by Flask config.

Apart from that normal review things apply.

@LukeMoll
Copy link
Member

LukeMoll commented Nov 12, 2021

Thanks for working on this, I'll try to give it a good look over on Monday. One quick comment based on the discussion here is that the cannonical URL for the website is www.HackSoc.org, so the base URL should probably reflect that. nginx is configured to redirect anyway so it shouldn't matter too much.

Copy link
Member

@LukeMoll LukeMoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the minor documentation suggestion, looks good to me, thank you! Have tested RSS and Atom in Akregator and behave as expected.

@JMAlego JMAlego requested a review from LukeMoll November 19, 2021 17:53
@LukeMoll LukeMoll merged commit d02dd23 into HackSoc:main Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Relating to the code which runs on the server and generates the site enhancement low priority an issue that doesn't need to be addressed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RSS feed for news articles
2 participants