Skip to content

Commit 287bb4e

Browse files
Do not validate slug for drafts of existing published editions with sequenced slugs
If there are already published editions with slugs ending in `--2` and the like, we want to continue publishing at that slug. Raising an error means users are basically unable to create new drafts without renaming at the moment. There are around 12k documents with sequenced slugs, and fixing them manually may no be desirable to users who potentially depend on those slugs not changing for whatever reasons. Cases in the wild are such that regenerating the slug "correctly" based on titles might end up redirecting to unexpected documents, as we cannot account for all the possible permutations of human error in the data. ``` whitehall(prod)> Document.where("slug LIKE ?", "%--%").count => 12246 ```
1 parent 20129a4 commit 287bb4e

2 files changed

Lines changed: 42 additions & 18 deletions

File tree

lib/whitehall/publishing_api.rb

Lines changed: 14 additions & 13 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!(model_instance, content)
72+
ensure_base_path_is_associated_with_this_content_id!(presenter.content_id, content[:base_path], model_instance)
7373

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

184-
def self.ensure_base_path_is_associated_with_this_content_id!(instance, content)
185-
content_id = instance.content_id
184+
def self.ensure_base_path_is_associated_with_this_content_id!(content_id, base_path, instance)
185+
# Not all models that use this module respond to `base_path_without_sequence`.
186+
base_path_without_sequence = instance.try(:base_path_without_sequence)
186187

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
188+
# If the edition is a first draft, then we should check that no existing content uses the base_path_without_sequence.
189+
# Drafts with already taken slugs will have sequences appended to their base_paths, like '--2'.
190+
# To provide the correct error message, we need to find the existing content at the base_path_without_sequence.
191+
existing_content_id = if instance.published_major_version.nil?
192+
Services.publishing_api.lookup_content_id(base_path: base_path_without_sequence || base_path)
193+
else
194+
# If already published, then check only the published base_path (may or may not have sequence).
195+
Services.publishing_api.lookup_content_id(base_path: base_path)
196+
end
195197

196-
existing_content_id = Services.publishing_api.lookup_content_id(base_path:)
197198
return if existing_content_id.nil? || existing_content_id == content_id
198199

199-
raise UnpublishableInstanceError, "Cannot save draft (content_id #{content_id}). There is existing content at the '#{base_path}' route, under a different content_id (#{existing_content_id}). Try changing your title to resolve the conflict."
200+
raise UnpublishableInstanceError, "Cannot save draft (content_id #{content_id}). There is existing content at the '#{base_path_without_sequence || base_path}' route, under a different content_id (#{existing_content_id}). Try changing your title to resolve the conflict."
200201
end
201202
end
202203
end

test/unit/lib/whitehall/publishing_api_test.rb

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -298,22 +298,45 @@ class Whitehall::PublishingApiTest < ActiveSupport::TestCase
298298
assert_requested request
299299
end
300300

301-
test ".save_draft raises exception if there is a live content item with the same base path but different content ID" do
301+
test ".save_draft raises exception for first new draft, if there is a live content item with the same base path but different content ID" do
302302
ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type"))
303-
edition = create(:draft_standard_edition)
303+
published_edition = create(:published_standard_edition, title: "Original Title")
304+
new_draft_edition_with_same_base_path = create(:draft_standard_edition, title: "Original Title")
305+
306+
assert new_draft_edition_with_same_base_path.reload.base_path.starts_with?(published_edition.base_path)
307+
assert new_draft_edition_with_same_base_path.reload.base_path.ends_with?("--2")
308+
304309
Whitehall::PublishingApi.unstub(:ensure_base_path_is_associated_with_this_content_id!)
305-
Services.publishing_api.expects(:lookup_content_id).with(base_path: edition.public_path).returns("different-content-id")
310+
Services.publishing_api.expects(:lookup_content_id).with(base_path: published_edition.base_path).returns(published_edition.content_id)
306311

307312
error = assert_raises Whitehall::UnpublishableInstanceError do
308-
Whitehall::PublishingApi.save_draft(edition)
313+
Whitehall::PublishingApi.save_draft(new_draft_edition_with_same_base_path)
309314
end
310315

311316
assert_equal(
312-
"Cannot save draft (content_id #{edition.content_id}). There is existing content at the '#{edition.public_path}' route, under a different content_id (different-content-id). Try changing your title to resolve the conflict.",
317+
"Cannot save draft (content_id #{new_draft_edition_with_same_base_path.content_id}). There is existing content at the '#{published_edition.base_path}' route, under a different content_id (#{published_edition.content_id}). Try changing your title to resolve the conflict.",
313318
error.message,
314319
)
315320
end
316321

322+
test ".save_draft does no raise an exception if the published path is already sequenced" do
323+
ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type"))
324+
slug = "example-document--2"
325+
edition = create(:published_standard_edition)
326+
edition.reload.document.update!(slug: slug)
327+
draft = edition.create_draft(create(:user))
328+
329+
assert edition.reload.base_path.ends_with?(slug)
330+
assert draft.reload.base_path.ends_with?(slug)
331+
332+
Whitehall::PublishingApi.unstub(:ensure_base_path_is_associated_with_this_content_id!)
333+
Services.publishing_api.expects(:lookup_content_id).with(base_path: draft.base_path).returns(draft.content_id)
334+
335+
assert_nothing_raised do
336+
Whitehall::PublishingApi.save_draft(draft)
337+
end
338+
end
339+
317340
test ".publish_redirect_async publishes a redirect to the Publishing API" do
318341
document = create(:document)
319342
destination = "/government/people/milli-vanilli"

0 commit comments

Comments
 (0)