Update calendar code #4223
Update calendar code #4223
Conversation
sairamya93
changed the title
sairamya93
changed the title
KludgeKML
force-pushed
the
update-clocks-change
branch
from
1ca589d
to
76bd074
Compare
KludgeKML
force-pushed
the
update-clocks-change
branch
from
76bd074
to
aaa86a7
Compare
KludgeKML
force-pushed
the
update-clocks-change
branch
from
aaa86a7
to
2918d82
Compare
- This hack captured missing translations and replaced them
with the original key, and was used to support code from
calendars which needed to handle either a translation key
or hard-coded text, but this lead to surprising behaviour
sairamya93
force-pushed
the
update-clocks-change
branch
from
2918d82
to
2ad554e
Compare
KludgeKML
force-pushed
the
update-clocks-change
branch
from
2ad554e
to
febb749
Compare
There was a problem hiding this comment.
@KludgeKML, @sairamya93 This looks like a worthwhile change, and thanks for making a start with it.
There's a lot going on in the second commit, so it would be great it could be broken up so that I can follow what's changing. Could you also add the "why" to the commit messages?
| @@ -13,9 +13,9 @@ class InvalidCalendarScope < StandardError; end | |||
| def show_calendar | |||
| respond_to do |format| | |||
| format.html do | |||
| @faq_presenter = FaqPresenter.new(scope, calendar, content_item_hash, view_context) | |||
| @faq_presenter = FaqPresenter.new(Calendar.file_name(params[:scope]), calendar, content_item_hash, view_context) | |||
There was a problem hiding this comment.
Why is FaqPresenter being instantiated with Calendar.file_name(params[:scope]) now rather than calendar? Are they different?
There was a problem hiding this comment.
Fixed to return calendar.scope
| @@ -14,21 +14,34 @@ def self.find(slug) | |||
| end | |||
| end | |||
|
|
|||
| attr_reader :slug, :title, :description | |||
| def self.file_name(slug) | |||
| slug == "gwyliau-banc" ? "bank-holidays" : slug | |||
There was a problem hiding this comment.
It feels like this should return the file name rather than the slug.
There was a problem hiding this comment.
Have reengineered to return "scope", which is how the bulk of the code refers to "bank-holidays" vs "when-the-clocks-change" regardless of translation.
- Remove translation of slugs in to_param - Remove translation of division slug in view Co-authored-by: Ramya Vidapanakal <ramya.vidapanakal@digital.cabinet-office.gov.uk>
- We always pass the slug into the division, and it's translated at that point if necessary, so stop it being overwritten by the slug in the data Co-authored-by: Ramya Vidapanakal <ramya.vidapanakal@digital.cabinet-office.gov.uk>
- There's one datafile shared between Welsh and English paths, which in the original code is refered to as the scope (so all translations of the bank holidays data have the same scope, but different slugs). - Allow the Calendar instance to retain the desired slug without having it translated away in the controller. - Provide a class method that will translate the scope to the relevant scope, and a reader attribute of same on the instance. Co-authored-by: Ramya Vidapanakal <ramya.vidapanakal@digital.cabinet-office.gov.uk>
KludgeKML
force-pushed
the
update-clocks-change
branch
from
febb749
to
68e23c5
Compare
KludgeKML
force-pushed
the
update-clocks-change
branch
from
68e23c5
to
9e77f5c
Compare
KludgeKML
changed the title
There was a problem hiding this comment.
Thanks for breaking up the commits and adding the test. That made these changes much easier to follow.
That was a lot to unpick, so well done for figuring it all out. The use of "scope" is a bit confusing, but you've managed to make it easier to understand 🎉
There's one really minor comment that's not a blocker, otherwise it all looks really good 👍
| @@ -41,7 +41,7 @@ def division | |||
| helper_method :calendar | |||
|
|
|||
| def content_item | |||
| @content_item ||= GdsApi.content_store.content_item("/#{scope}") | |||
| @content_item ||= GdsApi.content_store.content_item("/#{params[:scope]}") | |||
There was a problem hiding this comment.
Ah, ok, so you can call the Welsh version directly now? That's nicer.
| @@ -31,7 +31,7 @@ def division | |||
|
|
|||
| respond_to do |format| | |||
| format.json { render json: div } | |||
| format.ics { render plain: IcsRenderer.new(div.events, request.path).render } | |||
| format.ics { render plain: IcsRenderer.new(div.events, request.path, I18n.locale).render } | |||
There was a problem hiding this comment.
Does having to specific the locale here mean that calling set_locale on line 28 has no effect here? I don't think it affects the json, so perhaps line 28 isn't needed?
There was a problem hiding this comment.
set_locale is what makes I18n.locale have the right values, so it's needed (and it does affect the json)
- Now that the Calendar model handles translating scope slug to a valid scope, we remove the scope method here and just refer to the parameter to make it easier to differentiate. Co-authored-by: Ramya Vidapanakal <ramya.vidapanakal@digital.cabinet-office.gov.uk>
- locale is usually only set for HTML, but for the json and ICS files here it should be set so that they can be returned in the requested language Co-authored-by: Ramya Vidapanakal <ramya.vidapanakal@digital.cabinet-office.gov.uk>
KludgeKML
force-pushed
the
update-clocks-change
branch
from
3e6f788
to
4175fcf
Compare
- create translation keys for when-do-the-clocks-change content
- update division slugs in both .json files to be translation keys
- remove body (body was not used, and bank holidays body
was empty, so no point it being there for either calendar type)
- simplify description to remove years range, so that it doesn't
have to be updated (important because it will be in two places,
here and in the special-route-publisher data for this route).
- Fixtures moved to use translation keys rather than hard-coded values
- Tests now use clocks go forward rather than clocks advance to match content
- Tests with arbitrary division / event names now use valid names
- This is out of date with how the bank holiday files work now (and how they've worked in the past), now we're given lists of bank holidays by the content people.
- ICS files now return Welsh content when accessed through the Welsh pages, so we should indicate the correct locale in the file header.
- As of https://github.com/alphagov/special-route-publisher/pull/351, responsibility for publishing the calendars is now moved to special-route-publisher.
- without the publishing task, this is only used to support a test, so refactor the test and delete this class and its test - FAQ Presenter only uses the title and description from the content item
KludgeKML
force-pushed
the
update-clocks-change
branch
from
4175fcf
to
6047c1f
Compare
What
Adds locale values for When do the clock change file, moving them from the JSON files, and remove the hack file that handled returning either a translated key (for keys in the locale files), or the original file (for hardcoded text)
We also remove the task for publishing the calendars from frontend and add the relevant data into special route publisher (here: https://github.com/alphagov/special-route-publisher/pull/351). In order to do that, we make the description of when-do-the-clocks-change more generic (removing the list of years), and remove the body (since the body is not present for the bank holiday pages, and appears not to be used in search).
Why
[Trello card?] https://trello.com/c/dx543EiX/322-tidy-when-do-the-clocks-change-files
How
We should handle the when do the clocks change page the same way as the bank holidays one, which would allow us to translate it accordingly in the config file and lets us remove the hack file