Move Corporate information pages from government-frontend #4608
+1,194
−29
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
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
dc0c87f to
2a2ae59
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
2a2ae59 to
a326913
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
a326913 to
ccbfc08
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
ccbfc08 to
27b3b32
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
27b3b32 to
c68d580
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
12 times, most recently
from
12527c8 to
3e7ddfd
Compare
deborahchua
requested review from
KludgeKML,
leenagupte,
georges1996 and
sairamya93
KludgeKML
reviewed
Feb 4, 2025
KludgeKML
reviewed
Feb 4, 2025
|
|
||
| it "yields the block without contents data" do | ||
| render_component({}) { block } | ||
| expect(rendered).to include(block) |
This was referenced Feb 11, 2025
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
fd4484d to
8f719f0
Compare
leenagupte
reviewed
Feb 14, 2025
There was a problem hiding this comment.
We've already chatted about modelling Organisations in person, but as promised I've reviewed the ContentsList concern.
It's very confusing! So I can understand why you've kept most of the code as it was in government-frontend.
Part of me wonders whether it's worth trying to deconstruct the ContentsList a bit in to it's model and presentational parts.
For example, the decision to show_contents_list? is quite presentational, not something the model cares about. Could the model return a structured object of headers? The code in the ContentsList is confusing because as I understand it, much of it won't be reached unless contents_items is overwritten.
I'm in two minds about whether it's worth the effort to simplify it now, or leave it for a firebreak. I think perhaps if the system tests are thorough, then the refactoring could be left for "later".
app/models/content_item.rb
Outdated
| @@ -63,10 +63,6 @@ def meta_section | |||
| )&.downcase | |||
| end | |||
|
|
|||
| def contributors | |||
There was a problem hiding this comment.
Why has this been removed? I'm hoping it's in error!
There was a problem hiding this comment.
Perhaps it's being moved to the presenter? Ideally the contributors method would pluck out the title and base_path for each, so that they can be styled as link in the view.
spec/models/content_item_spec.rb
Outdated
| @@ -104,39 +104,6 @@ | |||
| end | |||
| end | |||
|
|
|||
| describe "#contributors" do | |||
There was a problem hiding this comment.
The removal of the test makes me think that you intended to remove contributors, but I don't understand why. Is it being replaced with something else?
app/models/concerns/organisations.rb
Outdated
| (content_store_hash.dig("links", "organisations") || []).map do |organisation| | ||
| { "title" => organisation["title"], "base_path" => organisation["base_path"], "content_id" => organisation["content_id"] } | ||
| end | ||
| content_store_hash.dig("links", "organisations") || [] |
There was a problem hiding this comment.
I'm wondering if there needs to be an Organisation class and then each organisation here could be modelled. I think that would make adding a logo in the later commit easier, and conceptually it would make more sense.
| @@ -0,0 +1,182 @@ | |||
| RSpec.shared_examples "it can have a contents list" do |document_type, example_name| | |||
There was a problem hiding this comment.
Thanks for adding these tests ⭐
|
|
||
| describe "#contents_items" do | ||
| it "includes the corporate information heading in the contents list if corporate informations groups are present" do | ||
| expect(corporate_information_page.content_store_response["details"]["corporate_information_groups"].present?).to be(true) |
There was a problem hiding this comment.
Should this be checking for the presence of corporate_information_page.corporate_information_groups instead?
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
3 times, most recently
from
7bcf360 to
c30d8e6
Compare
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
c30d8e6 to
ff63a5d
Compare
The view has been directly copied over from government-frontend. It will be updated to match the new data structure in a later commit. Commit audit trail: - https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/app/views/content_items/corporate_information_page.html.erb > app/views/corporate_information_page/show.html.erb
This keeps it consistent with the approach in other document types, which call the metadata component directly. It does not include the description tag which needs to be added manually for now. It has also been updated to use `extra_headers`, which ensures that the open graph meta tags are added to the page source correctly.
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
3 times, most recently
from
3b22c75 to
32b4394
Compare
This was originally added #4673 to accomodate including the `base_path` as the `url` for the `organisation_logo` publishing component which is not always required for a logo. However, we can transform the data required for the component in the view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13 , so having this additional model is not necessary, and we can directly access the `base_path` from the `content_item`. The view will be updated in a separate commit.
The title contains html such as line breaks, which we want to render safely on the page. Audit trail: - https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L9
The organisation_branding presenter is not used by any of the other document types, so it seems more appropriate to move these methods into the model. The `default_organisation` method is no longer private as it will be used in the view as part of the page title. Audit trail; - app/models/corporate_information_page.rb > https://github.com/alphagov/government-frontend/blob/main/app/presenters/content_item/organisation_branding.rb - spec/models/corporate_information_page_spec.rb > https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/test/presenters/content_item/organisation_branding_test.rb
These are displayed in the "Corporate information" section at the bottom of the page if those pages are available. Ran rake "consolidation:copy_translation[corporate_information_page, formats.corporate_information_page]" Audit trail: https://github.com/alphagov/government-frontend/tree/e7b9db33536f487991ba907502080e0625a066cc/config/locales
Ran rake "consolidation:copy_translation[content_item.schema_name.corporate_information_page, formats.corporate_information_page]"
…odel Similar to the organisation branding methods, these methods are in a separate module in government-frontend but is not used by any other document types. Audit trail: - https://github.com/alphagov/government-frontend/blob/a91838affca0c907ff94beba7c3bd021ddbd1809/app/presenters/content_item/corporate_information_groups.rb - https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/test/presenters/corporate_information_page_presenter_test.rb#L42-L79
This is also included in numerous document types. In government-frontend, a lot of complexity was introduced in alphagov/government-frontend#719 to deal with content that has only two H2 headings and criteria on their following content lengths to determine whether or not the contents lists were shown. This has caused a lot of confusion since, and some recent changes were made so that there is no longer a minimum number of headings required to show the contents list. As long as there are H2 headings with IDs present, the contents list will be shown. There are some document types (including corporate information pages) where some additional headings are required which will be handled in the document type model itself. Audit trail: - app/models/concerns/contents_list.rb > https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/app/presenters/content_item/contents_list.rb - spec/support/concerns/contents_list.rb > https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/test/presenters/content_item/contents_list_test.rb
It is possible to have extra H2 headings in the contents list to anchor to other sections that are included at the bottom of the page. Overriding the `contents_items` method allows us to include the "Corporate information" heading if there are `corporate_information_groups` available in the `details` hash. As the corporate information groups methods now live in the CorporateInformationPagePresenter it means the methods `extra_headings` use is no longer accessible within the model. Audit trail: - https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/app/presenters/corporate_information_page_presenter.rb#L21-L31 - https://github.com/alphagov/government-frontend/blob/90cf5386b165b3a342689744feece7c83449b153/app/presenters/content_item/corporate_information_groups.rb#L50-L57
- include title - all occurences of `@content_item` has been updated to `content_item` - use new Withdrawable methods introduced in 4b1d2d4 Add title to view
deborahchua
force-pushed
the
move-corporate-information-pages-from-government-frontend
branch
from
32b4394 to
575c734
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
We want to handle the rendering of
corporate_information_pagedocuments from government-frontend to frontend.Why
Trello card
How
By moving the routes and necessary code to get the pages rendering in Frontend.
Page examples
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/forensic-science-regulator/about
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/forensic-science-regulator/about/accessible-documents-policy
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/forensic-science-regulator/about/personal-information-charter
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/department-for-education/about/modern-slavery-statement
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/department-for-education/about/equality-and-diversity (includes a table)
https://[HEROKU-APP-ID].herokuapp.com/government/organisations/companies-house/about (has Welsh translation)
Screenshots