[WIP] Add travel advice pages #4225
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
05be61d
to
13d875b
Compare
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
aa8759c
to
182a438
Compare
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
db969d7
to
a9de3d9
Compare
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
abfcfd8
to
6539c30
Compare
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
6539c30
to
dbf66da
Compare
Moves the relevant model methods from the presenter in government-frontend: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/app/presenters/travel_advice_presenter.rb
Moves the alert_status method from the travel_advice_presenter in goverment-frontend. The method has been slightly refactored and also removes the presentation elements, specifically the translation lookup. travel_advice_presenter: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/app/presenters/travel_advice_presenter.rb#L77-L90 travel_advice_presenter_test: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/test/presenters/travel_advice_presenter_test.rb#L189-L197
This calls the presentation code from the original presenter directly in the view rather than adding a new method. travel_advice_presenter: https://github.com/alphagov/government-frontend/blob/a938e4bee78b794e4e5303a1bde1dfd05e94b287/app/presenters/travel_advice_presenter.rb#L87
original code: https://github.com/alphagov/government-frontend/blob/84047412f6f42eaf380a9092aadfe15037de0d35/app/helpers/machine_readable_metadata_helper.rb The other document types call the metadata component directly. It seems to add an unnecessary layer of abstraction to recreate a helper method whose only extra purpose is to pass in the content item, and that can done directly in each view.
leenagupte
force-pushed
the
add-travel-advice-pages
branch
3 times, most recently
from
db2cb1f
to
c901169
Compare
This is rather than porting across the `title_and_context` presenter method: https://github.com/alphagov/government-frontend/blob/16a0e99c52f482f54875eaecf537b31e6e2b75b4/app/presenters/travel_advice_presenter.rb#L34-L39
leenagupte
force-pushed
the
add-travel-advice-pages
branch
2 times, most recently
from
bfc00b8
to
b8a5d26
Compare
This helper creates links to generic things rather than recreating bespoke methods in a travel advice presenter. Audit trail: https://github.com/alphagov/government-frontend/blob/main/app/presenters/travel_advice_presenter.rb#L65-L71
Added as concern as it will be shared by travel advice and guide pages. parts.rb: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/app/presenters/content_item/parts.rb parts_test.rb: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/test/presenters/content_item/parts_test.rb Also Update current_part_body to return an empty string if nil so that the `body_for_metadata` method does not need to be recreated. `body_for_metadata` was created as a bug fix: alphagov/government-frontend@74f6e4e Makes `current_part` a public method so that it can be used in the call to PartsNavigationHelper which will be created in a later commit.
Also checks for the first part rather than recreating the method: https://github.com/alphagov/government-frontend/blob/16a0e99c52f482f54875eaecf537b31e6e2b75b4/app/presenters/travel_advice_presenter.rb#L49-L51
The presentation logic in the government-frontend presenter that wasn't appropriate to include in the model. Some methods have not been included as they are no longer being used in government-frontend: - parts_navigation - parts_navigation_second_list_start - part_links - part_navigation_group_size The render_guide_as_single_page? check has been removed from part_link_elements as it is out of date. The election it refers to was in 2021 See: https://github.com/alphagov/government-frontend/blob/16a0e99c52f482f54875eaecf537b31e6e2b75b4/app/presenters/content_item_presenter.rb#L106 https://github.com/alphagov/government-frontend/blob/16a0e99c52f482f54875eaecf537b31e6e2b75b4/app/presenters/content_item_presenter.rb#L123-L126
It replicates the logic from government-frontend: https://github.com/alphagov/government-frontend/blob/cbbbf70e5f2fd5a41b0208e9c01f9083bb6363d3/app/presenters/content_item_presenter.rb#L74-L80 It's only used in the show template, and passed to the machine_readable_metadata component. The travel advice index template calls `Frontend.govuk_website_root + root_path` directly in the template. Considered using this pattern, but having `Frontend.govuk_website_root` defined seems unnecessary, and calling `Plek.new.website_root` directly in the view feels wrong.
The tests also check that the print variant renders, they don't test that the correct variant is being printed because that would be testing that setting `request.variant` does the right thing.
Travel Advice pages need to known the withdrawn status in able to construct the page title. See: https://github.com/alphagov/government-frontend/blob/main/app/presenters/content_item/withdrawable.rb#L8 and https://github.com/alphagov/government-frontend/blob/main/app/presenters/travel_advice_presenter.rb#L6-L12 However, some of the methods feel like model concerns rather than presentation concerns, so they have been moved the to model. There is only one example of the content item with a withdrawn notice in content schemas, so the shared example is being tested on the generic content item model rather than travel advice. The travel advice page title is a presentation concern and will be added in a later commit.
This presenter is badly named ContentItemModelPresenter because ContentItemPresenter already exists. ContentItemPresenter takes a hash of the results from content store and models them. That work is already being done by the ContentItem model, and shouldn't be repeated in the presenter. Some extra work needs to be done to evaluate the existing routes to see how they can be unstitched from the existing ContentItemPresenter and then this class can be removed. The page_title has been added to a generic presenter because other document types from government-frontend use it as the basis for their own page title methods.
And use it in the views
It's not possible to use `@presenter` as that is being used by the
ApplicationHelper to define the wrapper classes. If you do, and you
don't have a "publication" defined, you get the following error:
NoMethodError:
# private method `format' called for #<TravelAdvicePresenter
./app/helpers/application_helper.rb:15:in `wrapper_class'
It's worth looking into unstitching all of this, but outside of this PR.
This was in the travel advice presenter in government-frontend. However as it is a purely presentation concern, it did not feel appropriate to add it to the model. It'll be needed by some new presenter methods. Add change_description to TravelAdvice model to support this change. travel_advice_presenter: https://github.com/alphagov/government-frontend/blob/c4bea9e232e929995c4fb5539951cadae8c740c8/app/presenters/travel_advice_presenter.rb#L108-L110
Adds methods to the TravelAdvice model. These information is only exists for Travel Advice, so it doesn't need to be added to the generic ContentItem model.
This takes the metadata presenter method from government-frontend: https://github.com/alphagov/government-frontend/blob/main/app/presenters/travel_advice_presenter.rb#L14-L28 but rather than recreating it, builds the params directly in the view. The old presenter method need to use view_context to call the `simple_format` helper method, which feels unnecessary for a method that's only used once in this document type specific partial. This code could probably be refactored into a more elegant helper method in the future.
This is rather than replicating the web_url method from the ContentItemPresenter in government-frontend that is doing the same thing: https://github.com/alphagov/government-frontend/blob/b3cbb8d7b8ff3ff792aa70c58ca8272a384d1773/app/presenters/content_item_presenter.rb#L70
This method only had one line and was unnecessary: https://github.com/alphagov/government-frontend/blob/b3cbb8d7b8ff3ff792aa70c58ca8272a384d1773/app/presenters/travel_advice_presenter.rb#L97C5-L97C21 The public_updated_at field has been added to the ContentItem model as it feels generic enough to be there, and so that the json object doesn't need to be directly accessed from the view.
This is to make space for the "show" tests
TO DO: Fix title and atom link tests
leenagupte
force-pushed
the
add-travel-advice-pages
branch
from
b8a5d26
to
8c8d54d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Why
Trello card
How
Screenshots?