-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add atom feeds to travel advice #257
Conversation
21430ce
to
ba4a046
Compare
get '*path(.:locale)(.:format)' => 'content_items#show', | ||
constraints: { | ||
format: /atom/, | ||
locale: /\w{2}(-[\d\w]{2,3})?/, |
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 regex is gnarly for my simple monkey brain, should we have a few tests around this?
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.
There is a test: https://github.com/alphagov/government-frontend/pull/257/files#diff-7dd9638113bd162736166ccac21fae7cR25
It tests every supported locale against the regex. If we add a new locale that's not supported, the test will fail.
# FIXME: Update when https://trello.com/c/w8HN3M4A/ is ready | ||
get 'foreign-travel-advice/:country/:part' => 'travel_advice#show' | ||
|
||
get '*path(.:locale)(.:format)' => 'content_items#show', |
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.
It looks like the only thing that constrains this route is that it's got an /atom/ and a valid locale.
What happens when you try to render something that is not a travel advice format? Does this work?
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.
It returns a 406: ba4a046 (ie not an accepted format)
If the templates were added, they would work.
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
@@ -1,3 +1,5 @@ | |||
require 'htmlentities' |
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.
Is this necessary? I think bundler
will require it.
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.
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.
Fixed and rebased.
* Update routing to allow atom format * Use a regex to constrain matches on locale and format * Test that all supported locales work with regex * Append the locale when matched to the content_item_path Paired with @andrewgarner
* Output of atom feed identical to multipage frontend (and live) * Port template and tests from multipage frontend * Use html entities gem to encode HTML entities for XML feed Context: alphagov/multipage-frontend#6
Previously, calling a format without an atom feed via: /government/consultations/postgraduate-doctoral-loans.atom Would have 404’d. Now there is atom support this retrieves a content item, but throws a “MissingTemplate” error when it comes to rendering. Catch this error and return a “not acceptable” 406 response. eg The target resource does not have a current representation that would be acceptable to the user agent
* Matches multipage-frontend * Allows supporting browsers to highlight when feeds are available for content * Makes country name method public for use with feed link
https://trello.com/c/mNRwOtHL/139-2-render-travel-advice-atom-feeds-in-government-frontend-m
Render atom feeds for travel advice countries. Output of atom feed is identical to multipage-frontend’s (and live). Template and tests ported from multipage-frontend.
When reviewing please double-check the routing logic and tests
Add support for rendering atom routes:
Paired with @andrewgarner
Example feed:
Matches: https://www.gov.uk/foreign-travel-advice/thailand.atom