-
Notifications
You must be signed in to change notification settings - Fork 20
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
Move case studies routes #4372
base: main
Are you sure you want to change the base?
Move case studies routes #4372
Conversation
4b79f76
to
5736b90
Compare
dbc0da6
to
f4d9f5d
Compare
aee5af8
to
6d4592f
Compare
6d4592f
to
40db71b
Compare
40db71b
to
a368c2c
Compare
e8d36b1
to
0d5579a
Compare
0d5579a
to
dc0a31b
Compare
734330f
to
2a2bb7b
Compare
2a2bb7b
to
578bdf2
Compare
578bdf2
to
2b2548d
Compare
2b2548d
to
3dbe69a
Compare
3dbe69a
to
cd1dceb
Compare
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.
Thank for making the changes requested in the previous reviews.
There are only 3 inline comments this time, all of them related to tests. The most important one is that there are no tests for the contributors
method in the case study the model.
@@ -0,0 +1,8 @@ | |||
RSpec.describe CaseStudy do | |||
it_behaves_like "it has updates", "case_study", "has-updates" | |||
it_behaves_like "it has updates", "case_study", "only-has-first-published-at" |
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.
The "only-has-first-published-at" example is bogus because the "first_published_at" is later than the "public_updated_at". It shouldn't be possible for that scenario to occur.
include WorldwideOrganisations | ||
include Organisations | ||
|
||
def contributors |
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 should be tests for this method.
cd1dceb
to
ea1daab
Compare
- https://github.com/alphagov/government-frontend/blob/2e602630db112fe0594653cebbeb313396e67a02/app/presenters/content_item/linkable.rb As part of this commit I needed to change the govuk_styled_link method as it was causing tests to fail in CI. This was due to some of the link titles containing an &. This change fixes this issue and all tests pass This is creating a helper method to allow us to move some of the presentation logic into a helper to avoid us from either needing to recreate the presenter, or creating a model concern.
Due to the complexity of the Linkable presenter, we have decided to split the Organisations and WorldwideOrganisations collections into their own concerns this makes it easier to manage as the logic in the Linkable presenter in government-frontend was very hard to understand and unpick. In addition to this the Metadata presenter from government-frontend was consuming the organisations array from the Linkable presenter, which seemed odd as a child concern should not be consuming another child concerns methods. This change is in preparation for allowing the ContentItem model to have a parent method to return just organisations, we will then add an overriding method in the document type model to override this, in this case the Case Study model will consume from both the Organisations and WorldwideOrganisations concern. - https://github.com/alphagov/government-frontend/blob/2e602630db112fe0594653cebbeb313396e67a02/app/presenters/content_item/linkable.rb - https://github.com/alphagov/government-frontend/blob/2e602630db112fe0594653cebbeb313396e67a02/app/presenters/content_item/metadata.rb
- To expose first_public_at and first_published_at attributes - Add Organisations concern - Add contributors method which calls the organisations function from Organisations concern to allow us to have a default value for all document_types We need both first_public_at and first_published_at as these both need to be checked otherwise updates won't be picked up for the other document types that use this concern. We have added a contributors function which returns the list of contributors, these are then rendered in the "from" section of the publisher_metadata partial. The contributors function will default to return just organisations for all document_types however this function will be overriden depending on what each document_type needs.
Commit audit trail: - https://github.com/alphagov/government-frontend/blob/a938e4bee78b794e4e5303a1bde1dfd05e94b287/app/presenters/content_item/withdrawable.rb - https://github.com/alphagov/government-frontend/blob/a938e4bee78b794e4e5303a1bde1dfd05e94b287/test/presenters/content_item/withdrawable_test.rb
Commit audit trail: - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/app/views/shared/_publisher_metadata_with_logo.html.erb - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/app/assets/stylesheets/helpers/_publisher-metadata-with-logo.scss As we dont need the logo part of the partial for case studies, I have removed it and renamed the partial
Add case studies model, view, controller, route and tests as well as content_item_model_presenter and related concerns Commit audit trail: - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/app/presenters/case_study_presenter.rb - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/app/views/content_items/case_study.html.erb - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/test/integration/case_study_test.rb - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/test/presenters/case_study_presenter_test.rb - https://github.com/alphagov/government-frontend/blob/0aee348d51cebacd5344115f2ca02bdb17095249/config/routes.rb Update app/views/components/_published_dates.html.erb to render dates correctly
Using govuk-docker-run rake "consolidation:copy_translation[content_item.schema_name.case_study]" we port over the case_study translations and change it to be under our new yaml structure Audit trail: - https://github.com/alphagov/government-frontend/tree/0aee348d51cebacd5344115f2ca02bdb17095249/config/locales
ea1daab
to
ea0b07a
Compare
This PR #4379 needs to be merged in and this will need to be rebased to fix the case studies which have translations e.g https://www.gov.uk/government/case-studies/doing-business-in-spain
What
Handle Case Studies documents in Frontend, in preparation for them being taken out of Government Frontend
Why
Ticket: https://trello.com/c/ybpXwp34/368-move-document-type-casestudy-from-government-frontend-to-frontend
How
By moving the routes and necessary code to get the pages rendering in the frontend application
Screenshots?
Frontend
Gov-Frontend