Skip to content

Allow Welsh-language mainstream URLs [WHIT-3436]#11442

Open
GDSNewt wants to merge 1 commit into
mainfrom
allow-mainstream-welsh-links
Open

Allow Welsh-language mainstream URLs [WHIT-3436]#11442
GDSNewt wants to merge 1 commit into
mainfrom
allow-mainstream-welsh-links

Conversation

@GDSNewt
Copy link
Copy Markdown
Contributor

@GDSNewt GDSNewt commented May 12, 2026

Welsh-only GOV.UK pages (e.g. /talu-treth-twe) were failing validation with "Url must reference a GOV.UK page" when added to document collection groups. The root cause was that get_content in the Publishing API defaults to locale: en, so Welsh-only content — which only exists in locale: cy — returned a 404.

This PR replaces get_content with the Content Store for fetching content items. The Content Store serves content regardless of locale, so Welsh URLs now resolve correctly.

The guide sub-page check (which validates that a two-segment path like /guidance/subpage belongs to a guide) continues to work because the Content Store returns the parent guide's content item for sub-page paths, making document_type available directly from that response.

BEFORE

Screen.Recording.2026-05-13.at.17.24.09.mov

AFTER

Screen.Recording.2026-05-13.at.17.23.03.mov

Jira

@GDSNewt GDSNewt force-pushed the allow-mainstream-welsh-links branch from 80db2fe to 7bc045a Compare May 12, 2026 16:02
Comment thread app/models/document_collection_non_whitehall_link/govuk_url.rb Outdated
Copy link
Copy Markdown
Contributor

@jamiestamp jamiestamp left a comment

Choose a reason for hiding this comment

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

LGTM

@GDSNewt GDSNewt force-pushed the allow-mainstream-welsh-links branch 3 times, most recently from f368fb0 to 1cabc1c Compare May 13, 2026 16:33
@GDSNewt GDSNewt requested a review from jamiestamp May 13, 2026 16:39
Comment thread app/models/document_collection_non_whitehall_link/govuk_url.rb Outdated
@GDSNewt GDSNewt force-pushed the allow-mainstream-welsh-links branch 2 times, most recently from b72739f to adc8991 Compare May 14, 2026 15:22
Copy link
Copy Markdown
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Looks broadly fine - document collections need a bit more love in general anyway - but we do have an opportunity to simplify with Content Store calls and dropping Publishing API calls 🤞

Comment thread app/models/document_collection_non_whitehall_link/govuk_url.rb Outdated
Comment thread app/models/document_collection_non_whitehall_link/govuk_url.rb Outdated
@TonyGDS
Copy link
Copy Markdown
Contributor

TonyGDS commented May 15, 2026

I assume we're willing to live with any lag between publishing-api pushing the latest content to the content store? Worst that would happen is the user would see the same error as now.

…on groups

The Publishing API's lookup_content_id endpoint does not index translated
(Welsh) base paths, causing a false "must reference a GOV.UK page" error
when users tried to add Welsh mainstream URLs to a document collection.

Fall back to the Content Store when both Publishing API lookups return no
content ID, as the Content Store resolves content items by any translated
base path. If the Content Store also returns a 404, the original error is
still raised.

Adds Services.content_store using GdsApi::ContentStore, and updates tests
to cover the Welsh URL case and to stub the Content Store in the existing
"not found" test.
@GDSNewt GDSNewt force-pushed the allow-mainstream-welsh-links branch from cd24aef to 29d5029 Compare May 20, 2026 10:10
Copy link
Copy Markdown
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Hi Alex - the code broadly looks good, just some tidying up to do before merge. Thanks


test "should be invalid when Publishing API is down" do
stub_publishing_api_isnt_available
Services.content_store.stubs(:content_item).raises(GdsApi::HTTPIntermittentServerError.new(503))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test name needs updating

Comment thread .gitignore
/app/assets/builds/*
!/app/assets/builds/.keep

console_history.txt
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This appears to have slipped in?

Services.content_store.stubs(:content_item).with("/foo/subpage").returns(
"content_id" => content_id,
"title" => "Foo Bar",
"base_path" => "/foo",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this not be /foo/subpage?


test "should be invalid when Publishing API returns a 404" do
stub_any_publishing_api_call_to_return_not_found
Services.content_store.stubs(:content_item).raises(GdsApi::ContentStore::ItemNotFound.new(404))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test name needs updating

)
end

test "should be valid without a GOV.UK url that Publishing API knows" do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test name needs updating

setup do
@content_id = SecureRandom.uuid
stub_publishing_api_has_lookups("/test" => @content_id)
stub_publishing_api_has_item(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do these Publishing API stubs need to exist any more?

assert url.errors.full_messages.include?("Url must reference a GOV.UK page")
end

test "should be valid when a Welsh-language GOV.UK URL is used that is not in the Publishing API path reservations" do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test name needs updating

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also I don't really understand the relevance of the second half of the test name 🤔

assert url.valid?
end

test "should be valid when a Welsh-language GOV.UK URL is in the Publishing API but only has a Welsh locale" do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not clear what this test is trying to exercise differently to the one above it?

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.

4 participants