Skip to content

Commit 85d0577

Browse files
ChrisBAshtonpatrickpatrickpatrick
authored andcommitted
Capture same base path error and display to user
Instead of presenting a server error to the user if they try and create an edition that has the base path of another published edition, the error is now presented to the user.
1 parent a2f86f9 commit 85d0577

8 files changed

Lines changed: 95 additions & 3 deletions

File tree

app/controllers/admin/editions_controller.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ def create
9797
@edition.destroy!
9898
build_edition
9999
build_edition_dependencies
100+
@edition.errors.add(:title, "has been used before on GOV.UK, although the page may no longer exist. Please use another title")
100101
render :new
101102
end
102103

app/models/document.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ def first_edition_id
8585

8686
def similar_slug_exists?
8787
scope = Document.where(document_type:)
88-
sequence_separator = friendly_id_config.sequence_separator
8988

9089
# slug is a nullable column, so we can't assume that it exists
9190
return false if slug.nil?
@@ -99,6 +98,8 @@ def similar_slug_exists?
9998
).count > 1
10099
end
101100

101+
delegate :sequence_separator, to: :friendly_id_config
102+
102103
def should_generate_new_friendly_id?
103104
sluggable_string.present?
104105
end

app/models/edition.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,10 @@ def base_path
402402
"/government/generic-editions/#{url_slug}"
403403
end
404404

405+
def base_path_without_sequence
406+
base_path.split(document.sequence_separator).first
407+
end
408+
405409
def public_path(options = {})
406410
return if base_path.nil?
407411

features/standard-edition.feature

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,12 @@ Feature: Standard Editions
5757
And choosing a document type should take me to a preview page summarising the changes
5858
And when I click "Confirm document type change"
5959
Then the document type should have updated
60+
61+
Scenario: Creating a new draft configurable document with the same base path as an already published configurable document
62+
Given I am a writer
63+
And the configurable document types feature flag is enabled
64+
And the test configurable document type is defined
65+
And I have published a configurable document titled "The history of GOV.UK"
66+
When I draft a new "Test configurable document type" configurable document with the same title as a published edition
67+
Then I should see an error of "Title has been used before on GOV.UK, although the page may no longer exist. Please use another title" on the new configurable document page
68+
And the form should retain the user input with the title "The history of GOV.UK" before submit

features/step_definitions/standard_edition_steps.rb

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,29 @@ def default_content_for_locale(locale)
7777
click_button "Save and go to document summary"
7878
end
7979

80+
When(/^I draft a new "([^"]*)" configurable document with the same title as a published edition$/) do |configurable_document_type|
81+
create(:organisation) if Organisation.count.zero?
82+
visit admin_root_path
83+
find("li.app-c-sub-navigation__list-item a", text: "New document").click
84+
page.choose("Standard document")
85+
click_button("Next")
86+
page.choose(configurable_document_type)
87+
click_button("Next")
88+
expect(page).to have_content("New test")
89+
within "form" do
90+
fill_in "edition_title", with: Edition.published.first.title
91+
fill_in "edition_summary", with: "A brief summary of the document."
92+
fill_in "edition_body", with: "## Some govspeak This is the body content"
93+
fill_in "edition[block_content][date_field][3]", with: "01"
94+
fill_in "edition[block_content][date_field][2]", with: "11"
95+
fill_in "edition[block_content][date_field][1]", with: "2011"
96+
end
97+
Whitehall::PublishingApi.unstub(:ensure_base_path_is_associated_with_this_content_id!)
98+
Services.publishing_api.stubs(:lookup_content_id).returns("not a real content id")
99+
click_button "Save and go to document summary"
100+
Whitehall::PublishingApi.stubs(:ensure_base_path_is_associated_with_this_content_id!).returns(nil)
101+
end
102+
80103
Then("when I switch to the Images tab to fill in the other configurable fields") do
81104
# Pretend we've uploaded an image already
82105
edition = @standard_edition || StandardEdition.last
@@ -308,6 +331,22 @@ def default_content_for_locale(locale)
308331
end
309332
end
310333

334+
Given(/^I have published a configurable document titled "([^"]*)"$/) do |title|
335+
en_content = default_content_for_locale("en")
336+
@standard_edition = create(
337+
:published_standard_edition,
338+
{
339+
configurable_document_type: "test",
340+
title: title || en_content[:title],
341+
summary: en_content[:summary],
342+
block_content: {
343+
"body" => en_content[:body],
344+
"date_field" => en_content[:date_field],
345+
},
346+
},
347+
)
348+
end
349+
311350
When(/^I create a new draft and visit the Welsh translation$/) do
312351
edition = @standard_edition || StandardEdition.last
313352
visit admin_standard_edition_path(edition)
@@ -380,3 +419,17 @@ def default_content_for_locale(locale)
380419

381420
expect(page).to have_content("Test configurable document type two")
382421
end
422+
423+
Then(/^I should see an error of "(.*)" on the new configurable document page$/) do |error|
424+
expect(page).to have_selector(".gem-c-error-summary__list-item")
425+
expect(page).to have_content(error.to_s)
426+
end
427+
428+
And(/^the form should retain the user input with the title "(.*)" before submit$/) do |title|
429+
expect(page).to have_field("Title", with: title)
430+
expect(page).to have_field("Summary", with: "A brief summary of the document.")
431+
expect(page).to have_field("Body", with: "## Some govspeak This is the body content")
432+
expect(page).to have_field("Day", with: "01")
433+
expect(page).to have_field("Month", with: "11")
434+
expect(page).to have_field("Year", with: "2011")
435+
end

features/support/document_helper.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ def begin_drafting_document(options)
2121
page.choose(options[:type].humanize)
2222
click_button("Next")
2323

24+
if options[:subtype]
25+
page.choose(options[:subtype].humanize)
26+
click_button("Next")
27+
end
28+
2429
if options[:locale]
2530
check "Create a foreign language only"
2631
select options[:locale], from: "Document language"

lib/whitehall/publishing_api.rb

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ def self.save_draft_translation(
6969

7070
content.merge!(bulk_publishing: true) if bulk_publishing
7171

72-
ensure_base_path_is_associated_with_this_content_id!(content[:base_path], model_instance.content_id)
72+
ensure_base_path_is_associated_with_this_content_id!(model_instance, content)
7373

7474
Services.publishing_api.put_content(presenter.content_id, content)
7575
end
@@ -181,7 +181,18 @@ def self.assert_public_edition!(instance)
181181
end
182182
end
183183

184-
def self.ensure_base_path_is_associated_with_this_content_id!(base_path, content_id)
184+
def self.ensure_base_path_is_associated_with_this_content_id!(instance, content)
185+
content_id = instance.content_id
186+
187+
base_path = if instance.respond_to?(:base_path_without_sequence)
188+
# `base_path_without_sequence` required because if an edition with the same
189+
# title exists in Whitehall then `base_path` of `instance` will have
190+
# a unique sequence identifier and the check will not work as intended
191+
instance.base_path_without_sequence
192+
else
193+
content[:base_path]
194+
end
195+
185196
existing_content_id = Services.publishing_api.lookup_content_id(base_path:)
186197
return if existing_content_id.nil? || existing_content_id == content_id
187198

test/unit/app/models/edition_test.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -945,6 +945,14 @@ class EditionTest < ActiveSupport::TestCase
945945
assert edition.reload.revalidated_at
946946
end
947947

948+
test "#base_path_without_sequence should return base path without unique identifier " do
949+
existing = create(:edition, title: "Latest news")
950+
draft = create(:edition, title: "Latest news")
951+
952+
assert_not_equal draft.base_path, existing.base_path
953+
assert_equal draft.base_path_without_sequence, existing.base_path
954+
end
955+
948956
def decoded_token_payload(token)
949957
payload, _header = JWT.decode(
950958
token,

0 commit comments

Comments
 (0)