Skip to content
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

Implement Worldwide Organisation offices as multi-part content #8844

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

brucebolt
Copy link
Member

@brucebolt brucebolt commented Feb 14, 2024

We are switching Worldwide Organisations to include the content of their offices in the main content item for the organisation. This adds that into the presenters for these content types.

In a later PR, we will remove the links and update routes once the rendering code (alphagov/government-frontend#3095) has been updated.

Depends on alphagov/publishing-api#2638.

Trello card

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@brucebolt brucebolt force-pushed the ww-office-multipart branch 3 times, most recently from fe7478e to 0848ad0 Compare February 15, 2024 09:22
This code will be used to embed Worldwide Offices into the multi-part
content item for Worldwide Organisations, so needs to be extracted into
a helper to enable this.
Copy link
Contributor

@jkempster34 jkempster34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

access_and_opening_times: Whitehall::GovspeakRenderer.new.govspeak_to_html(worldwide_org.reload.home_page_offices.first.access_and_opening_times),
contact_content_id: worldwide_org.reload.home_page_offices.first.contact.content_id,
services: [],
slug: worldwide_org.reload.home_page_offices.first.slug,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking is this the full path offices/office-1 or just office-1?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at your Gov Frontend code yet, but in the spike I did the other week, this would have had to be offices/office-1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's offices/office-1. I wasn't really sure whether to add the offices/ here or in the frontend app.

I went with the frontend app in the end, as it isn't really part of the slug.

I wonder whether full base path (e.g. /world/organisation/foo/offices/bar) might actually be better than slug?

Copy link
Contributor

@jkempster34 jkempster34 Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of see it relative to the content item it's part of... if that makes sense? So offices/office-1 makes the most sense to me

Copy link
Member Author

@brucebolt brucebolt Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true. Let's go for office/office-1 here, then I can remove office/ from the Government Frontend code.

worldwide_offices.map do |worldwide_office|
worldwide_office_details(worldwide_office).merge(
contact_content_id: worldwide_office.contact.content_id,
slug: "office/#{worldwide_office.slug}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we could DRY this up to avoid hardcoding it here.

Maybe raising the path as a constant in WorldwideOffice, or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the chance of someone changing the office path is, but if they did, then this would break

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've used the base_path method of the WorldwideOffice, then gsubbed out the base path of the WorldwideOrganisation and a slash.

We are currently assuming all worldwide offices exist under the same
base path, which is not the case for those associated with editionable
worldwide organisations.

Therefore updating the base path method so the correct path is returned
for offices associated with either editionable and non-editionable
worldwide organisations.
This includes the content of the worldwide offices in the worldwide
organisation page, allowing us to render them from a single content
item.

Depends on alphagov/publishing-api#2638.
…ions

This includes the content of the worldwide offices in the editionable
worldwide organisation page, allowing us to render them from a single
content item.

Depends on alphagov/publishing-api#2638.
@brucebolt brucebolt merged commit afe6862 into main Feb 19, 2024
20 checks passed
@brucebolt brucebolt deleted the ww-office-multipart branch February 19, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants