From 3ea14d7eb427394b736e37e423b7326eaab1f993 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 10:29:14 +0100 Subject: [PATCH 01/12] Delete current implementation of StandardEditionMigrator(Job) We'll rebuild the StandardEditionMigrator over the next few commits. Having spiked the approach in PR 11530, the changes are so numerous that attempting to do them commit by commit makes little sense - we're better off killing what we have and starting from the ground up, as so much of the behaviour will have changed. StandardEditionMigrator started life as an Editionable-only migrator for News Articles. It then underwent some modest modification for CaseStudies, before then having some more significant prep work done on it in advance of Topical Event migration. That prep work, to support non-Editionable source documents (topical events), is untested, and what we've ended up with is a migrator service that is quite difficult to reason about. Additional methods were added for defining titles and summaries (because implementations differ across content types), but worse than that, there is an attempt to add generic support for translations where 1) all editionable content types support translations, 2) some non-editionable content types support translations (e.g. Organisations) and 3) some non-editionable content types don't support translations (e.g. Topical Events). The resulting API made it difficult to build and iterate a recipe. Similarly, there was a half-baked 'preview' method which had little use. There was a 're-present to Publishing API' option which was ultimately not used for news articles due to race conditions. There was an awkward coupling of logic between the Job and the Migrator itself. I'm hoping the new StandardEditionMigrator will be simpler to work with, and will scale to all content types. The intention is to use it to migrate topical events shortly. --- app/services/standard_edition_migrator.rb | 39 -- app/sidekiq/standard_edition_migrator_job.rb | 150 ----- .../standard_edition_migrator_test.rb | 141 ---- .../standard_edition_migrator_job_test.rb | 622 ------------------ 4 files changed, 952 deletions(-) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index 44d5bf7e863..da0f67f65fc 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -1,41 +1,2 @@ class StandardEditionMigrator - def initialize(scope:) - @scope = scope - end - - def preview - if document_scope? - total_editions = @scope.sum { |doc| Edition.unscoped.where(document: doc).count } - { unique_documents: @scope.count, total_editions: total_editions } - else - { unique_records: @scope.count } - end - end - - def migrate!(republish: false, compare_payloads: true) - @scope.each do |record| - StandardEditionMigratorJob.perform_async( - record.id, - { "republish" => republish, "compare_payloads" => compare_payloads, "model_class" => model_class_name }, - ) - end - end - - def self.recipe_for(model) - if model.is_a?(Edition) - raise "No migration recipe defined for Edition type #{model.type}" - end - - raise "No migration recipe defined for #{model.class.name}" - end - -private - - def model_class_name - @scope.model.name - end - - def document_scope? - model_class_name == "Document" - end end diff --git a/app/sidekiq/standard_edition_migrator_job.rb b/app/sidekiq/standard_edition_migrator_job.rb index 77798d5fe00..545c4b574ab 100644 --- a/app/sidekiq/standard_edition_migrator_job.rb +++ b/app/sidekiq/standard_edition_migrator_job.rb @@ -1,156 +1,6 @@ -require "diffy" -require "pp" - class StandardEditionMigratorJob < JobBase # Don't retry this job if it fails, because it's typically all # ‘internal’ – so it won’t fail because a third-party API is down, # and any failure is unlikely to resolve itself on a retry. sidekiq_options queue: "standard_edition_migration", retry: 0 - - def perform(record_id, args) - republish = args["republish"] - compare_payloads = args["compare_payloads"] - model_class_name = args["model_class"] - - if model_class_name != "Document" - perform_for_non_editionable(record_id, model_class_name, republish:, compare_payloads:) - else - perform_for_document(record_id, republish:, compare_payloads:) - end - end - -private - - def perform_for_document(document_id, republish:, compare_payloads:) - ActiveRecord::Base.transaction do - document = Document.find(document_id) - migrate_editions!(document, compare_payloads) - document.update_column(:document_type, "StandardEdition") - end - PublishingApiDocumentRepublishingJob.new.perform(document_id, true) if republish - end - - def perform_for_non_editionable(record_id, model_class_name, republish:, compare_payloads:) - record = model_class_name.constantize.find(record_id) - recipe = StandardEditionMigrator.recipe_for(record) - - new_document_id = ActiveRecord::Base.transaction do - migrate_non_editionable!(record, recipe, compare_payloads) - end - - PublishingApiDocumentRepublishingJob.new.perform(new_document_id, true) if republish - end - - def migrate_editions!(document, compare_payloads) - editions_to_migrate = Edition.unscoped.where(document: document) - - editions_to_migrate.each do |edition| - recipe = StandardEditionMigrator.recipe_for(edition) - - # Skip the payload comparison for superseded or deleted editions - if compare_payloads && edition.state != "superseded" && !edition.deleted? - ensure_payloads_remain_identical(edition, recipe) { migrate_edition!(edition, recipe) } - else - migrate_edition!(edition, recipe) - end - end - end - - def migrate_non_editionable!(record, recipe, compare_payloads) - document = Document.create!(document_type: "StandardEdition", content_id: record.content_id) - edition = StandardEdition.new( - document:, - configurable_document_type: recipe.configurable_document_type, - state: "published", - ) - edition.save!(validate: false) - - record.translations.each do |record_translation| - edition.translations.find_or_create_by!(locale: record_translation.locale).update_columns( - title: recipe.title(record_translation), - summary: recipe.summary(record_translation), - block_content: recipe.map_legacy_fields_to_block_content(record, record_translation), - ) - end - - if compare_payloads - old_presenter = recipe.presenter.new(record) - new_presenter = PublishingApi::StandardEditionPresenter.new(edition) - compare_presenter_payloads( - old_presenter.content, new_presenter.content, - old_presenter.links, new_presenter.links, - recipe, subject: "#{record.class.name} ID #{record.id}" - ) - end - - document.id - end - - def migrate_edition!(edition, recipe) - edition.update_columns( - type: "StandardEdition", - configurable_document_type: recipe.configurable_document_type, - ) - edition.translations.each do |translation| - translation.update_columns( - block_content: recipe.map_legacy_fields_to_block_content(translation), - body: nil, # only applicable to legacy document types - ) - end - end - - def ensure_payloads_remain_identical(edition, recipe) - edition_id = edition.id # capture ID because we can't call `.reload` after updating type - - old_presenter = recipe.presenter.new(edition) - old_content = old_presenter.content - old_links = old_presenter.links - - yield - - new_presenter = PublishingApi::StandardEditionPresenter.new(Edition.unscoped.find(edition_id)) - new_content = new_presenter.content - new_links = new_presenter.links - - compare_presenter_payloads( - old_content, new_content, old_links, new_links, - recipe, subject: "Edition ID #{edition.id}" - ) - end - - def compare_presenter_payloads(old_content, new_content, old_links, new_links, recipe, subject:) - diff = diff_values( - recipe.ignore_legacy_content_fields(old_content), - recipe.ignore_new_content_fields(new_content), - ) - unless diff.to_s.empty? - raise "Presenter content mismatch after migration for #{subject}: #{diff}" - end - - diff = diff_values( - recipe.ignore_legacy_links(old_links), - recipe.ignore_new_links(new_links), - ) - unless diff.to_s.empty? - raise "Presenter links mismatch after migration for #{subject}: #{diff}" - end - end - - def diff_values(left_val, right_val) - left = PP.pp(deep_sort(left_val), +"") # pretty-print to string for cleaner diff output - right = PP.pp(deep_sort(right_val), +"") - Diffy::Diff.new(left, right, context: 5, color: true) - end - - def deep_sort(obj) - case obj - when Hash - obj.keys.sort.index_with { |k| deep_sort(obj[k]) } - when Array - a = obj.map { |v| deep_sort(v) } - a.sort_by { |v| JSON.generate(v) } - else - obj - end - end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index df35cc19a9c..ec3edbfeae9 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -2,145 +2,4 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase extend Minitest::Spec::DSL - - describe "#initialize" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - end - - test "takes a scope" do - assert_nothing_raised do - StandardEditionMigrator.new(scope: Document.all) - end - end - - test "raises exception if no scope provided" do - assert_raises(ArgumentError) do - StandardEditionMigrator.new - end - end - end - - describe "#preview" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - end - - test "summarises how many documents and editions will be migrated" do - editor = create(:departmental_editor) - some_doc = build(:standard_edition) - some_doc.save! - some_doc.first_published_at = Time.zone.now - some_doc.major_change_published_at = Time.zone.now - force_publish(some_doc) - some_doc.create_draft(editor) - - migrator = StandardEditionMigrator.new(scope: Document.where(id: some_doc.document.id)) - summary = { - unique_documents: 1, - total_editions: 2, - } - - assert_equal summary, migrator.preview - end - - test "includes superseded editions in the scope" do - editor = create(:departmental_editor) - some_doc = build(:standard_edition) - some_doc.save! - some_doc.first_published_at = Time.zone.now - some_doc.major_change_published_at = Time.zone.now - force_publish(some_doc) - draft = some_doc.create_draft(editor) - draft.change_note = "Superseding edition" - draft.save! - force_publish(draft) - - migrator = StandardEditionMigrator.new(scope: Document.where(id: some_doc.document.id)) - summary = { - unique_documents: 1, - total_editions: 2, - } - - assert_equal summary, migrator.preview - end - - test "includes deleted editions in the scope" do - editor = create(:departmental_editor) - some_doc = create(:published_standard_edition) - draft = some_doc.create_draft(editor) - draft.change_note = "Superseding edition" - draft.save! - draft.delete!(editor) - - migrator = StandardEditionMigrator.new(scope: Document.where(id: some_doc.document.id)) - summary = { - unique_documents: 1, - total_editions: 2, - } - - assert_equal summary, migrator.preview - end - - test "summarises how many non-editionable records will be migrated" do - org1 = create(:organisation) - org2 = create(:organisation) - - migrator = StandardEditionMigrator.new(scope: Organisation.where(id: [org1.id, org2.id])) - - assert_equal({ unique_records: 2 }, migrator.preview) - end - end - - describe "#migrate!" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - end - - test "enqueues a migration job for each unique document in the scope" do - editor = create(:departmental_editor) - some_doc_1 = build(:standard_edition) - some_doc_1.save! - some_doc_1.first_published_at = Time.zone.now - some_doc_1.major_change_published_at = Time.zone.now - force_publish(some_doc_1) - some_doc_1.create_draft(editor) - - some_doc_2 = build(:standard_edition) - some_doc_2.save! - some_doc_2.first_published_at = Time.zone.now - some_doc_2.major_change_published_at = Time.zone.now - force_publish(some_doc_2) - - migrator = StandardEditionMigrator.new(scope: Document.all) - - StandardEditionMigratorJob.expects(:perform_async).with(some_doc_1.document.id, { "republish" => false, "compare_payloads" => true, "model_class" => "Document" }).once - StandardEditionMigratorJob.expects(:perform_async).with(some_doc_2.document.id, { "republish" => false, "compare_payloads" => true, "model_class" => "Document" }).once - - migrator.migrate! - end - - test "allows republish and compare_payloads options to be passed to the job" do - some_doc = create(:standard_edition) - - migrator = StandardEditionMigrator.new(scope: Document.all) - - StandardEditionMigratorJob.expects(:perform_async).with(some_doc.document.id, { "republish" => true, "compare_payloads" => false, "model_class" => "Document" }).once - migrator.migrate!(republish: true, compare_payloads: false) - end - - test "enqueues a migration job for each non-editionable record, passing the model class" do - org1 = create(:organisation) - org2 = create(:organisation) - - migrator = StandardEditionMigrator.new(scope: Organisation.where(id: [org1.id, org2.id])) - - StandardEditionMigratorJob.expects(:perform_async) - .with(org1.id, { "republish" => false, "compare_payloads" => true, "model_class" => "Organisation" }).once - StandardEditionMigratorJob.expects(:perform_async) - .with(org2.id, { "republish" => false, "compare_payloads" => true, "model_class" => "Organisation" }).once - - migrator.migrate! - end - end end diff --git a/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb b/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb index c83d2847739..0d2bd070123 100644 --- a/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb +++ b/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb @@ -8,626 +8,4 @@ class StandardEditionMigratorJobTest < ActiveSupport::TestCase assert_equal({ "retry" => 0, "queue" => "standard_edition_migration" }, StandardEditionMigratorJob.sidekiq_options) end end - - describe "#perform" do - it "finds the Document by ID" do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - document = create(:document) - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipe.new) - job = StandardEditionMigratorJob.new - job.stubs(:migrate_editions!) - assert_nothing_raised do - job.perform(document.id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") - end - end - - it "raises exception if Document is not found" do - invalid_document_id = 0 - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipe.new) - - job = StandardEditionMigratorJob.new - - assert_raises(ActiveRecord::RecordNotFound) do - job.perform(invalid_document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") - end - end - - describe "#migrate_editions!" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipe.new) - - # stub the presenters - we'll test those separately - TestPresenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content" }) - TestPresenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links" }) - - editor = create(:departmental_editor) - # 1. create a legacy document... ([Edition: draft]) - # TODO: it would be good to genericise this test so we don't have to update it when the - # document type in question gets deleted from Whitehall. But a few attempts at this were - # unsuccessful, so I've picked a document type that is likely to exist for a while yet. - organisation = create(:organisation, :with_alternative_format_contact_email) - superseded_edition = build(:publication, alternative_format_provider: organisation, body: "This is my legacy document type body") - superseded_edition.save! - superseded_edition.first_published_at = Time.zone.now - superseded_edition.major_change_published_at = Time.zone.now - # 2. publish it and then create a new draft ([Edition: published, Edition: draft]) - force_publish(superseded_edition) - published_edition = superseded_edition.create_draft(editor) - published_edition.body = "This is my updated body" - published_edition.change_note = "Superseding edition" - published_edition.save! - # 3. create a new draft which we'll then delete - force_publish(published_edition) - deleted_edition = published_edition.create_draft(editor) - deleted_edition.body = "This is my body to be deleted" - deleted_edition.change_note = "Deleting draft edition" - deleted_edition.save! - deleted_edition.delete! - # 4. publish it and then create a new draft ([Edition: superseded, Edition: published, Edition: draft]) - draft_edition = published_edition.create_draft(editor) - draft_edition.body = "This is my updated body for the draft" - draft_edition.change_note = "Superseding edition" - draft_edition.save! - - # Calling `.reload` won't work now we've changed the type - so we'll have to re-fetch by Edition ID - @superseded_edition_id = superseded_edition.id - @published_edition_id = published_edition.id - @deleted_edition_id = deleted_edition.id - @draft_edition_id = draft_edition.id - @document_id = draft_edition.document.id - end - - test "doesn't change the document history" do - change_history = Document.find(@document_id).change_history.to_json - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - - assert_equal change_history, Document.find(@document_id).change_history.to_json - end - - test "migrates all editions in the scope to the configurable_document_type variant of StandardEdition" do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - superseded_edition = Edition.find(@superseded_edition_id) - published_edition = Edition.find(@published_edition_id) - draft_edition = Edition.find(@draft_edition_id) - deleted_edition = Edition.unscoped.find(@deleted_edition_id) - - # It's a bit of a smell that we're having to set the document type at - # both the edition and document level - a symptom of denormalisation. - # TODO: write up an issue to fix this up. - assert_equal "StandardEdition", superseded_edition.document.document_type - assert_equal "StandardEdition", superseded_edition.type - assert_equal "StandardEdition", published_edition.type - assert_equal "StandardEdition", draft_edition.type - assert_equal "StandardEdition", deleted_edition.type - assert_equal "test_type", superseded_edition.configurable_document_type - assert_equal "test_type", published_edition.configurable_document_type - assert_equal "test_type", draft_edition.configurable_document_type - assert_equal "test_type", deleted_edition.configurable_document_type - end - - test "migrates all editions in the scope and retains their original states" do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - superseded_edition = Edition.find(@superseded_edition_id) - published_edition = Edition.find(@published_edition_id) - draft_edition = Edition.find(@draft_edition_id) - deleted_edition = Edition.unscoped.find(@deleted_edition_id) - - assert_equal "superseded", superseded_edition.state - assert_equal "published", published_edition.state - assert_equal "draft", draft_edition.state - assert_equal "deleted", deleted_edition.state - end - - test "defers to `map_legacy_fields_to_block_content` to set the block_content= field" do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - superseded_edition = Edition.find(@superseded_edition_id) - published_edition = Edition.find(@published_edition_id) - deleted_edition = Edition.unscoped.find(@deleted_edition_id) - draft_edition = Edition.find(@draft_edition_id) - superseded_block_content = { "field_attribute" => "MODIFIED This is my legacy document type body" } - published_block_content = { "field_attribute" => "MODIFIED This is my updated body" } - deleted_block_content = { "field_attribute" => "MODIFIED This is my body to be deleted" } - draft_block_content = { "field_attribute" => "MODIFIED This is my updated body for the draft" } - - assert_equal superseded_block_content, superseded_edition.block_content.to_h - assert_equal published_block_content, published_edition.block_content.to_h - assert_equal deleted_block_content, deleted_edition.block_content.to_h - assert_equal draft_block_content, draft_edition.block_content.to_h - end - - test "clears the legacy body field" do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - superseded_edition = Edition.find(@superseded_edition_id) - published_edition = Edition.find(@published_edition_id) - deleted_edition = Edition.unscoped.find(@deleted_edition_id) - draft_edition = Edition.find(@draft_edition_id) - - assert_nil superseded_edition.body - assert_nil published_edition.body - assert_nil deleted_edition.body - assert_nil draft_edition.body - end - - test "associated images and attachments are retained after migration" do - draft_edition = Edition.find(@draft_edition_id) - image = build(:image) - attachment = build(:file_attachment, attachable: draft_edition) - draft_edition.images << image - draft_edition.attachments = [attachment] - draft_edition.save! - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - - draft_edition = Edition.find(@draft_edition_id) - assert_equal draft_edition.images, [image] - assert_equal draft_edition.attachments, [attachment] - end - - test "migrates all translations of each edition" do - draft_edition = Edition.find(@draft_edition_id) - with_locale(:fr) do - draft_edition.title = "french-title" - draft_edition.summary = "french-summary" - draft_edition.body = "french-body" - end - draft_edition.save! - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - - draft_edition = Edition.find(@draft_edition_id) - with_locale(:fr) do - assert_equal "french-title", draft_edition.title - assert_equal "french-summary", draft_edition.summary - assert_equal "MODIFIED french-body", draft_edition.block_content.to_h["field_attribute"] - end - end - - test "rolls back the transaction if it encounters an exception" do - # We update the Document towards the end of the migration process, so this should - # be a reasonable simulation of a failure part way through the migration. - Document.any_instance.stubs(:update_column).raises(StandardError.new("Simulated failure")) - assert_raises(StandardError) do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - superseded_edition = Edition.find(@superseded_edition_id) - published_edition = Edition.find(@published_edition_id) - deleted_edition = Edition.unscoped.find(@deleted_edition_id) - draft_edition = Edition.find(@draft_edition_id) - assert_equal "Publication", superseded_edition.type - assert_equal "Publication", published_edition.type - assert_equal "Publication", deleted_edition.type - assert_equal "Publication", draft_edition.type - end - - test "re-presents document to Publishing API (on bulk publishing queue) when `republish => true` is passed" do - PublishingApiDocumentRepublishingJob.any_instance.expects(:perform).with(Edition.find(@draft_edition_id).document.id, true).once - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - - test "doesn't re-present document to Publishing API if `republish => true` isn't passed" do - PublishingApiDocumentRepublishingJob.any_instance.expects(:perform).with(Edition.find(@draft_edition_id).document.id, true).never - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => false, "compare_payloads" => true, "model_class" => "Document") } - end - - test "doesn't re-present document to Publishing API if exception encountered during migration" do - PublishingApiDocumentRepublishingJob.any_instance.expects(:perform).with(Edition.find(@draft_edition_id).document.id, true).never - - Document.any_instance.stubs(:update_column).raises(StandardError.new("Simulated failure")) - assert_raises(StandardError) do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - end - - test "Calls ensure_payloads_remain_identical when `compare_payloads => true`" do - calls = [] - StandardEditionMigratorJob.any_instance.stubs(:migrate_edition!) - StandardEditionMigratorJob.any_instance.stubs(:ensure_payloads_remain_identical).with do |edition, _recipe| - calls << edition.id - true - end - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - assert_equal [@published_edition_id, @draft_edition_id], calls - end - - test "doesn't call ensure_payloads_remain_identical when `compare_payloads => true` isn't passed" do - StandardEditionMigratorJob.any_instance.stubs(:migrate_edition!) - StandardEditionMigratorJob.any_instance.expects(:ensure_payloads_remain_identical).never - - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => false, "model_class" => "Document") } - end - end - - describe "ensure_payloads_remain_identical logic" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipe.new) - document = create(:document) - create(:standard_edition, document: document) #  we need an edition of any type to attach to the document - @document_id = document.id - end - - test "compares the presenter outputs on non-superseded and deleted editions, before and after migration, and passes if they're identical" do - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content" }) - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links" }) - - assert_nothing_raised do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - end - - test "does no presenter comparison on superseded or deleted editions" do - editor = create(:departmental_editor) - # 1. create a draft edition of something... ([Edition: draft]) - document = create(:document) - superseded_edition = create(:standard_edition, document: document) #  we need an edition of any type to attach to the document - superseded_edition.save! - superseded_edition.first_published_at = Time.zone.now - superseded_edition.major_change_published_at = Time.zone.now - # 2. publish it - force_publish(superseded_edition) - # 3. create a new draft which we'll then delete - deleted_edition = superseded_edition.create_draft(editor) - deleted_edition.body = "Edition to be deleted" - deleted_edition.change_note = "Edition to be deleted" - deleted_edition.save! - deleted_edition.delete! - # 4. create a new draft which will cause the original edition to be superseded ([Edition: published, Edition: draft]) - published_edition = superseded_edition.create_draft(editor) - published_edition.body = "This is my updated body" - published_edition.change_note = "Superseding edition" - published_edition.save! - # 5. publish it and then create a new draft ([Edition: superseded, Edition: published, Edition: draft]) - force_publish(published_edition) - draft_edition = published_edition.create_draft(editor) - draft_edition.body = "This is my updated body for the draft" - draft_edition.change_note = "Superseding edition" - draft_edition.save! - - published_edition_id = published_edition.id - draft_edition_id = draft_edition.id - document_id = draft_edition.document.id - - calls = [] - StandardEditionMigratorJob.any_instance.stubs(:ensure_payloads_remain_identical).with do |edition| - calls << edition.id - true - end - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - assert_equal [published_edition_id, draft_edition_id], calls - end - - test "payload comparison passes even if the ordering is different" do - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:content).returns({ some: "content", other: "stuff" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ other: "stuff", some: "content" }) - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:links).returns({ some: "links", nested: { foo: "bar", baz: "bax" } }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links", nested: { baz: "bax", foo: "bar" } }) - - assert_nothing_raised do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - end - - test "raises exception if 'content' payload differs" do - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "something else" }) - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links" }) - - error = assert_raises(RuntimeError) do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - assert_match(/Presenter content mismatch after migration for Edition ID/, error.message) - end - - test "raises exception if 'links' payload differs" do - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content" }) - StandardEditionMigratorJobTest::TestPresenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "something else" }) - - error = assert_raises(RuntimeError) do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - assert_match(/Presenter links mismatch after migration for Edition ID/, error.message) - end - - test "uses ignore_legacy_content_fields and ignore_new_content_fields hooks to filter out expected differences" do - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipeForIgnoreContentFields.new) - # stub content to be identical except for one legacy field, and one new field - StandardEditionMigratorJobTest::TestRecipeForIgnoreContentFields.new.presenter.any_instance.stubs(:content).returns({ some: "content", ignore_legacy: "old_value" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content", ignore_new: "new_value" }) - - # stub links to be identical - StandardEditionMigratorJobTest::TestRecipeForIgnoreContentFields.new.presenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links" }) - - assert_nothing_raised do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - end - - test "uses ignore_legacy_links and ignore_new_links hooks to filter out expected differences" do - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestRecipeForIgnoreLinksFields.new) - - # stub content to be identical - StandardEditionMigratorJobTest::TestRecipeForIgnoreLinksFields.new.presenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content" }) - - # stub links to be identical except for one legacy field, and one new field - StandardEditionMigratorJobTest::TestRecipeForIgnoreLinksFields.new.presenter.any_instance.stubs(:links).returns({ some: "links", ignore_legacy: "old_value" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links", ignore_new: "new_value" }) - - assert_nothing_raised do - Sidekiq::Testing.inline! { StandardEditionMigratorJob.new.perform(@document_id, "republish" => true, "compare_payloads" => true, "model_class" => "Document") } - end - end - end - end - - describe "#perform with non-editionable records" do - setup do - ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) - StandardEditionMigrator.stubs(:recipe_for).returns(StandardEditionMigratorJobTest::TestNonEditionableRecipe.new) - - TestNonEditionablePresenter.any_instance.stubs(:content).returns({ some: "content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "content" }) - TestNonEditionablePresenter.any_instance.stubs(:links).returns({ some: "links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "links" }) - - @test_record = StandardEditionMigratorJobTest::TestNonEditionableRecord.new( - id: 1, - name: "My Event", - summary: "Event summary", - description: "Event description", - content_id: SecureRandom.uuid, - ) - TestNonEditionableRecord.register(@test_record) - end - - teardown do - TestNonEditionableRecord.clear - end - - test "creates a new Document and StandardEdition, mapping fields from the recipe" do - assert_difference("Document.count", 1) do - assert_difference("Edition.count", 1) do - perform_non_editionable_migration - end - end - - new_document = Document.last - assert_equal "StandardEdition", new_document.document_type - assert_equal @test_record.content_id, new_document.content_id - - new_edition = Edition.last - assert_equal "published", new_edition.state - assert_equal TestNonEditionableRecipe.new.configurable_document_type, new_edition.configurable_document_type - assert_equal "My Event", new_edition.title - assert_equal "Event summary", new_edition.summary - assert_equal({ "description" => "Event description" }, new_edition[:block_content]) - end - - test "carries over translations from the non-editionable record to the new edition" do - bilingual_record = TestNonEditionableRecord.new( - id: 2, - name: "", - summary: "", - description: "", - content_id: SecureRandom.uuid, - translations: [ - TestNonEditionableRecord::Translation.new(locale: :en, name: "english name", summary: "english summary", description: "english description"), - TestNonEditionableRecord::Translation.new(locale: :cy, name: "welsh name", summary: "welsh summary", description: "welsh description"), - ], - ) - TestNonEditionableRecord.register(bilingual_record) - - Sidekiq::Testing.inline! do - StandardEditionMigratorJob.new.perform( - bilingual_record.id, - "republish" => false, - "compare_payloads" => false, - "model_class" => "StandardEditionMigratorJobTest::TestNonEditionableRecord", - ) - end - - new_edition = Edition.last - en_translation = new_edition.translations.find_by(locale: "en") - assert_not_nil en_translation - assert_equal "english name", en_translation.title - assert_equal "english summary", en_translation.summary - - cy_translation = new_edition.translations.find_by(locale: "cy") - assert_not_nil cy_translation - assert_equal "welsh name", cy_translation.title - assert_equal "welsh summary", cy_translation.summary - end - - test "the original non-editionable record is not deleted" do - perform_non_editionable_migration - - assert TestNonEditionableRecord.find(@test_record.id).present?, - "Expected original TestNonEditionableRecord to still exist after migration" - end - - test "re-presents document to Publishing API when republish => true" do - PublishingApiDocumentRepublishingJob.any_instance.expects(:perform).once - - perform_non_editionable_migration(republish: true) - end - - test "republishes the newly created document (not some other document) when republish => true" do - republished_document_id = nil - PublishingApiDocumentRepublishingJob.any_instance.expects(:perform).with do |document_id, _bulk| - republished_document_id = document_id - true - end - - perform_non_editionable_migration(republish: true) - - assert_equal Document.last.id, republished_document_id - end - - test "runs payload comparison when compare_payloads => true and raises if content differs" do - TestNonEditionablePresenter.any_instance.stubs(:content).returns({ some: "legacy content" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:content).returns({ some: "different content" }) - - error = assert_raises(RuntimeError) do - perform_non_editionable_migration(compare_payloads: true) - end - - assert_match(/Presenter content mismatch after migration for StandardEditionMigratorJobTest::TestNonEditionableRecord ID/, error.message) - end - - test "runs payload comparison when compare_payloads => true and raises if links differ" do - TestNonEditionablePresenter.any_instance.stubs(:links).returns({ some: "legacy links" }) - PublishingApi::StandardEditionPresenter.any_instance.stubs(:links).returns({ some: "different links" }) - - error = assert_raises(RuntimeError) do - perform_non_editionable_migration(compare_payloads: true) - end - - assert_match(/Presenter links mismatch after migration for StandardEditionMigratorJobTest::TestNonEditionableRecord ID/, error.message) - end - - private - - def perform_non_editionable_migration(republish: false, compare_payloads: false) - Sidekiq::Testing.inline! do - StandardEditionMigratorJob.new.perform( - @test_record.id, - "republish" => republish, - "compare_payloads" => compare_payloads, - "model_class" => "StandardEditionMigratorJobTest::TestNonEditionableRecord", - ) - end - end - end - - class TestPresenter - def initialize(_edition); end - def content; end - def links; end - end - - class TestNonEditionablePresenter - def initialize(_record); end - def content; end - def links; end - end - - class TestRecipe - def configurable_document_type - "test_type" - end - - def presenter - StandardEditionMigratorJobTest::TestPresenter - end - - def map_legacy_fields_to_block_content(translation) - { "field_attribute" => "MODIFIED #{translation.body}" } - end - - def ignore_legacy_content_fields(content) - content - end - - def ignore_new_content_fields(content) - content - end - - def ignore_legacy_links(links) - links - end - - def ignore_new_links(links) - links - end - end - - class TestRecipeForIgnoreContentFields < TestRecipe - def ignore_legacy_content_fields(content) - content.delete(:ignore_legacy) - content - end - - def ignore_new_content_fields(content) - content.delete(:ignore_new) - content - end - end - - class TestRecipeForIgnoreLinksFields < TestRecipe - def ignore_legacy_links(links) - links.delete(:ignore_legacy) - links - end - - def ignore_new_links(links) - links.delete(:ignore_new) - links - end - end - - class TestNonEditionableRecipe < TestRecipe - def presenter - StandardEditionMigratorJobTest::TestNonEditionablePresenter - end - - def title(record_translation) - record_translation.name - end - - def summary(record_translation) - record_translation.summary - end - - def map_legacy_fields_to_block_content(_record, record_translation) - { "description" => record_translation.description } - end - end - - class TestNonEditionableRecord - Translation = Struct.new(:locale, :name, :summary, :description, keyword_init: true) - - attr_reader :id, :name, :summary, :description, :content_id - - def initialize(id:, name:, summary:, description:, content_id:, translations: nil) - @id = id - @name = name - @summary = summary - @description = description - @content_id = content_id - @translations = translations - end - - def translations - @translations || [Translation.new(locale: :en, name: @name, summary: @summary, description: @description)] - end - - def self.find(id) - all_instances.find { |r| r.id == id } || raise(ActiveRecord::RecordNotFound) - end - - def self.register(record) - all_instances << record - end - - def self.clear - @all_instances = [] - end - - def self.all_instances - @all_instances ||= [] - end - end end From 8aa469e79e9c196bb2d133704bf7c1b9faef6832 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 11:05:27 +0100 Subject: [PATCH 02/12] Implement `StandardEditionMigrator.preview_migration` This is a much more straightforward API than before. We will now be able to pass the legacy record - of any type - and a given recipe (uninitialized), and let the StandardEditionMigrator service figure out how to compare the before-and-after state of the given record. I'll implement the `compare_payloads` method in the next commit. --- app/services/standard_edition_migrator.rb | 22 +++++ .../recipe_for_legacy_editionable_document.rb | 2 + .../recipe_for_non_editionable_record.rb | 2 + .../standard_edition_migrator_test.rb | 89 +++++++++++++++++++ 4 files changed, 115 insertions(+) create mode 100644 test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb create mode 100644 test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index da0f67f65fc..b7c459e0917 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -1,2 +1,24 @@ class StandardEditionMigrator + def self.preview_migration(...) + new.preview_migration(...) + end + + def preview_migration(legacy_record, recipe) + if legacy_record.is_a?(Edition) + raise "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)" + end + + # If passed an Editionable legacy model, let's preview migrating only the latest edition. + if legacy_record.is_a?(Document) + legacy_record = legacy_record.editions.last + end + + compare_payloads(legacy_record, recipe) + end + +private + + def compare_payloads(_legacy_record, _recipe) + "TODO: Implement payload comparison logic here" + end end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb new file mode 100644 index 00000000000..25a19cefecf --- /dev/null +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -0,0 +1,2 @@ +class StandardEditionMigrator::RecipeForLegacyEditionableDocument +end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb new file mode 100644 index 00000000000..d00b751f691 --- /dev/null +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb @@ -0,0 +1,2 @@ +class StandardEditionMigrator::RecipeForNonEditionableRecord +end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index ec3edbfeae9..f726ea5abf2 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -1,5 +1,94 @@ require "test_helper" +require_relative "./standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document" +require_relative "./standard_edition_migrator/fixtures/recipe_for_non_editionable_record" class StandardEditionMigratorTest < ActiveSupport::TestCase extend Minitest::Spec::DSL + + setup do + ConfigurableDocumentType.setup_test_types(build_configurable_document_type("test_type")) + @legacy_non_editionable_record = create(:organisation, name: "Title") + @legacy_editionable_document = create(:document, document_type: "DetailedGuide") + create(:deleted_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") + create(:superseded_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") + create(:published_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") + create(:draft_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") + end + + describe "#preview_migration" do + it "raises exception if an Edition is passed" do + edition = build(:edition) + assert_raises(RuntimeError, "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)") do + StandardEditionMigrator.preview_migration(edition, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + end + end + + context "when passing a non-editionable record" do + let(:preview_migration) do + StandardEditionMigrator.preview_migration( + @legacy_non_editionable_record, + StandardEditionMigrator::RecipeForNonEditionableRecord, + ) + end + + it "doesn't create any StandardEdition or Document, and doesn't persist any changes to the legacy record" do + before_attributes = @legacy_non_editionable_record.attributes + + assert_no_difference [StandardEdition.method(:count), Document.method(:count)] do + preview_migration + end + + assert_equal( + before_attributes, + @legacy_non_editionable_record.reload.attributes, + ) + end + + it "passes the record to `compare_payloads`" do + StandardEditionMigrator.any_instance.expects(:compare_payloads).with( + @legacy_non_editionable_record, + StandardEditionMigrator::RecipeForNonEditionableRecord, + ) + StandardEditionMigrator.preview_migration( + @legacy_non_editionable_record, + StandardEditionMigrator::RecipeForNonEditionableRecord, + ) + end + end + + context "when passing an editionable record" do + let(:preview_migration) do + StandardEditionMigrator.preview_migration(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + end + + it "doesn't create any StandardEdition or Document, and doesn't persist any changes to the legacy record" do + before_attributes = @legacy_editionable_document.attributes + before_attributes_on_edition = @legacy_editionable_document.editions.last.attributes + + assert_no_difference [StandardEdition.method(:count), Document.method(:count)] do + preview_migration + end + + assert_equal( + before_attributes, + @legacy_editionable_document.reload.attributes, + ) + assert_equal( + before_attributes_on_edition, + @legacy_editionable_document.editions.last.reload.attributes, + ) + end + + it "passes the latest edition to `compare_payloads`" do + StandardEditionMigrator.any_instance.expects(:compare_payloads).with( + @legacy_editionable_document.editions.last, + StandardEditionMigrator::RecipeForLegacyEditionableDocument, + ) + StandardEditionMigrator.preview_migration( + @legacy_editionable_document, + StandardEditionMigrator::RecipeForLegacyEditionableDocument, + ) + end + end + end end From 2ab4b1e9fcd6e1a6a6b8a5e6401a6ea43a1588ed Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 11:50:50 +0100 Subject: [PATCH 03/12] Implement `compare_payloads` private method logic We call the recipe's `build_edition` method, which should build a StandardEdition instance in memory, and we then compare the content/links payloads with those of the legacy presenter on the legacy record. The actual 'diff' generation will happen in the next commit, as it is complex enough to require testing in isolation. --- app/services/standard_edition_migrator.rb | 27 ++++++++++- .../fixtures/hardcoded_presenter.rb | 16 +++++++ .../recipe_for_legacy_editionable_document.rb | 26 ++++++++++ .../recipe_for_non_editionable_record.rb | 27 +++++++++++ .../standard_edition_migrator_test.rb | 47 +++++++++++++++++++ 5 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 test/unit/app/services/standard_edition_migrator/fixtures/hardcoded_presenter.rb diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index b7c459e0917..c29978b8a09 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -18,7 +18,30 @@ def preview_migration(legacy_record, recipe) private - def compare_payloads(_legacy_record, _recipe) - "TODO: Implement payload comparison logic here" + def compare_payloads(legacy_record, recipe) + # Grab the payloads from the old presenter _before_ we do any mutation, to ensure we're comparing against the original payload + old_presenter = recipe.new.legacy_presenter.new(legacy_record) + old_content = old_presenter.content + old_links = old_presenter.links + + standard_edition = recipe.new.build_edition(legacy_record) + new_presenter = PublishingApi::StandardEditionPresenter.new(standard_edition) + + <<~OUTPUT + OLD PAYLOAD + ===CONTENT + #{JSON.pretty_generate(old_content)} + ===LINKS + #{JSON.pretty_generate(old_links)} + + NEW PAYLOAD + ===CONTENT + #{JSON.pretty_generate(new_presenter.content)} + ===LINKS + #{JSON.pretty_generate(new_presenter.links)} + + NORMALISED DIFF + ===TODO: fill this in + OUTPUT end end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/hardcoded_presenter.rb b/test/unit/app/services/standard_edition_migrator/fixtures/hardcoded_presenter.rb new file mode 100644 index 00000000000..c24c5951f96 --- /dev/null +++ b/test/unit/app/services/standard_edition_migrator/fixtures/hardcoded_presenter.rb @@ -0,0 +1,16 @@ +class StandardEditionMigrator::HardcodedPresenter + def initialize(record); end + + def content + { + "body": "OLD PAYLOAD", + "some_old_field": "some old value", + } + end + + def links + { + "old_link": "old link", + } + end +end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb index 25a19cefecf..10ce568162f 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -1,2 +1,28 @@ class StandardEditionMigrator::RecipeForLegacyEditionableDocument + def legacy_presenter + StandardEditionMigrator::HardcodedPresenter + end + + def build_edition(legacy_record) + edition_attrs = { + configurable_document_type: "test_type", + updated_at: legacy_record.updated_at.rfc3339, + creator: User.last, + document: legacy_record.document, + } + edition = StandardEdition.new(edition_attrs) + + legacy_record.translations.each do |translation| + # Still operating on the newly initialized Edition in memory - careful use of `find_or_initialize_by` + edition.translations.find_or_initialize_by(locale: translation.locale).update( + title: "Title", + summary: "Summary", + block_content: { + "field_attribute" => translation.body.to_s, + }, + ) + end + + edition + end end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb index d00b751f691..f537b003ceb 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb @@ -1,2 +1,29 @@ class StandardEditionMigrator::RecipeForNonEditionableRecord + def legacy_presenter + StandardEditionMigrator::HardcodedPresenter + end + + def build_edition(legacy_record) + edition_attrs = { + configurable_document_type: "test_type", + updated_at: legacy_record.updated_at.rfc3339, + creator: User.last, + } + edition = StandardEdition.new(edition_attrs) + + # NOTE: implementation will vary depending on non-editionable model. + # E.g. Organisation has `translations` we can iterate over, but TopicalEvent does not. + legacy_record.translations.each do |translation| + edition.translations.find_or_initialize_by(locale: translation.locale).update( + title: translation.name, + summary: "Summary", + block_content: { + # Hardcoded for simplicity, so we can check the payload comes out the same + # for both the editionable and non-editionable test cases. + "field_attribute" => "Old body", + }, + ) + end + edition + end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index f726ea5abf2..6d5a347cdd2 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -1,4 +1,5 @@ require "test_helper" +require_relative "./standard_edition_migrator/fixtures/hardcoded_presenter" require_relative "./standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document" require_relative "./standard_edition_migrator/fixtures/recipe_for_non_editionable_record" @@ -54,6 +55,29 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase StandardEditionMigrator::RecipeForNonEditionableRecord, ) end + + it "returns string summary of content and links payloads before-and-after of the record itself" do + legacy_presenter = StandardEditionMigrator::RecipeForNonEditionableRecord.new.legacy_presenter.new(@legacy_non_editionable_record) + new_presenter = PublishingApi::StandardEditionPresenter.new( + StandardEditionMigrator::RecipeForNonEditionableRecord.new.build_edition(@legacy_non_editionable_record), + ) + expected_output = <<~OUTPUT + OLD PAYLOAD + ===CONTENT + #{JSON.pretty_generate(legacy_presenter.content)} + ===LINKS + #{JSON.pretty_generate(legacy_presenter.links)} + + NEW PAYLOAD + ===CONTENT + #{JSON.pretty_generate(new_presenter.content)} + ===LINKS + #{JSON.pretty_generate(new_presenter.links)} + OUTPUT + + summary = StandardEditionMigrator.preview_migration(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + assert summary.start_with?(expected_output), "Expected output to start with:\n#{expected_output}\n\nActual output:\n#{summary}" + end end context "when passing an editionable record" do @@ -89,6 +113,29 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase StandardEditionMigrator::RecipeForLegacyEditionableDocument, ) end + + it "returns string summary of content and links payloads before-and-after of the latest edition" do + legacy_presenter = StandardEditionMigrator::RecipeForLegacyEditionableDocument.new.legacy_presenter.new(@legacy_editionable_document) + new_presenter = PublishingApi::StandardEditionPresenter.new( + StandardEditionMigrator::RecipeForLegacyEditionableDocument.new.build_edition(@legacy_editionable_document.latest_edition), + ) + expected_output = <<~OUTPUT + OLD PAYLOAD + ===CONTENT + #{JSON.pretty_generate(legacy_presenter.content)} + ===LINKS + #{JSON.pretty_generate(legacy_presenter.links)} + + NEW PAYLOAD + ===CONTENT + #{JSON.pretty_generate(new_presenter.content)} + ===LINKS + #{JSON.pretty_generate(new_presenter.links)} + OUTPUT + + summary = StandardEditionMigrator.preview_migration(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + assert summary.start_with?(expected_output), "Expected output to start with:\n#{expected_output}\n\nActual output:\n#{summary}" + end end end end From eac3a562569e9d5895cdf7755ca05fa2bda07954 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 18:03:58 +0100 Subject: [PATCH 04/12] Add 'BaseRecipe' The recipes are about to get more complex (when it comes to saving artefacts associated with the edition built in memory). The test recipes would also end up duplicating more and more definitions. Extracting the common logic into one base recipe that future recipes can inherit from should be a useful starting point. --- .../standard_edition_migrator/base_recipe.rb | 9 +++++++++ .../base_recipe_test.rb | 17 +++++++++++++++++ .../recipe_for_legacy_editionable_document.rb | 2 +- .../recipe_for_non_editionable_record.rb | 2 +- 4 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 app/services/standard_edition_migrator/base_recipe.rb create mode 100644 test/unit/app/services/standard_edition_migrator/base_recipe_test.rb diff --git a/app/services/standard_edition_migrator/base_recipe.rb b/app/services/standard_edition_migrator/base_recipe.rb new file mode 100644 index 00000000000..f12e7389c4c --- /dev/null +++ b/app/services/standard_edition_migrator/base_recipe.rb @@ -0,0 +1,9 @@ +class StandardEditionMigrator::BaseRecipe + def legacy_presenter + raise NotImplementedError, "Subclasses must implement legacy_presenter!" + end + + def build_edition(record) + raise NotImplementedError, "Subclasses must implement build_edition!" + end +end diff --git a/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb b/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb new file mode 100644 index 00000000000..4c12e16124b --- /dev/null +++ b/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb @@ -0,0 +1,17 @@ +require "test_helper" + +class BaseRecipeTest < ActiveSupport::TestCase + test "raises exception if legacy_presenter method is called" do + error = assert_raises(NotImplementedError) do + StandardEditionMigrator::BaseRecipe.new.legacy_presenter + end + assert_equal "Subclasses must implement legacy_presenter!", error.message + end + + test "raises exception if build_edition method is called" do + error = assert_raises(NotImplementedError) do + StandardEditionMigrator::BaseRecipe.new.build_edition(nil) + end + assert_equal "Subclasses must implement build_edition!", error.message + end +end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb index 10ce568162f..b11c188416a 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -1,4 +1,4 @@ -class StandardEditionMigrator::RecipeForLegacyEditionableDocument +class StandardEditionMigrator::RecipeForLegacyEditionableDocument < StandardEditionMigrator::BaseRecipe def legacy_presenter StandardEditionMigrator::HardcodedPresenter end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb index f537b003ceb..1b820292e4f 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb @@ -1,4 +1,4 @@ -class StandardEditionMigrator::RecipeForNonEditionableRecord +class StandardEditionMigrator::RecipeForNonEditionableRecord < StandardEditionMigrator::BaseRecipe def legacy_presenter StandardEditionMigrator::HardcodedPresenter end From e9bbe7b0ddc4c828c8be02bab239dd1b846a4c18 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 14:48:18 +0100 Subject: [PATCH 05/12] Add recipe methods for normalising diffs before comparing them Hypothetically speaking, we want the payload of a content item to be the same after migration as before. But in reality, there are often new content properties and links on StandardEditionPresenter that did not exist on the legacy content item. Likewise, there are often content properties or links on the legacy content item that are no longer relevant and should not be carried over to their StandardEdition equivalent. We therefore have the following methods defined for normalising the diff: - ignore_legacy_content_fields - ignore_new_content_fields - ignore_legacy_links - ignore_new_links --- app/services/standard_edition_migrator.rb | 54 +++++++- .../standard_edition_migrator/base_recipe.rb | 24 ++++ .../base_recipe_test.rb | 12 ++ .../standard_edition_migrator_test.rb | 120 ++++++++++++++++++ 4 files changed, 209 insertions(+), 1 deletion(-) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index c29978b8a09..cf0d914e9dd 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -3,6 +3,14 @@ def self.preview_migration(...) new.preview_migration(...) end + def self.diff_content_payloads(...) + new.diff_content_payloads(...) + end + + def self.diff_links_payloads(...) + new.diff_links_payloads(...) + end + def preview_migration(legacy_record, recipe) if legacy_record.is_a?(Edition) raise "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)" @@ -16,6 +24,20 @@ def preview_migration(legacy_record, recipe) compare_payloads(legacy_record, recipe) end + def diff_content_payloads(recipe:, old_content: nil, new_content: nil) + diff_values( + recipe.ignore_legacy_content_fields(old_content.deep_dup), + recipe.ignore_new_content_fields(new_content.deep_dup), + ).to_s + end + + def diff_links_payloads(recipe:, old_links: nil, new_links: nil) + diff_values( + recipe.ignore_legacy_links(old_links.deep_dup), + recipe.ignore_new_links(new_links.deep_dup), + ).to_s + end + private def compare_payloads(legacy_record, recipe) @@ -41,7 +63,37 @@ def compare_payloads(legacy_record, recipe) #{JSON.pretty_generate(new_presenter.links)} NORMALISED DIFF - ===TODO: fill this in + ===CONTENT + #{diff_content_payloads( + old_content: old_content, + new_content: new_presenter.content, + recipe: recipe.new, + )} + ===LINKS + #{diff_links_payloads( + old_links: old_links, + new_links: new_presenter.links, + recipe: recipe.new, + )} OUTPUT end + + def diff_values(left_val, right_val) + # Newlines required otherwise Diffy appends "\\ No newline at end of file" to the output + left = "#{JSON.pretty_generate(deep_sort(left_val))}\n" + right = "#{JSON.pretty_generate(deep_sort(right_val))}\n" + Diffy::Diff.new(left, right, context: 5, color: true) + end + + def deep_sort(obj) + case obj + when Hash + obj.keys.sort.index_with { |k| deep_sort(obj[k]) } + when Array + a = obj.map { |v| deep_sort(v) } + a.sort_by { |v| JSON.generate(v) } + else + obj + end + end end diff --git a/app/services/standard_edition_migrator/base_recipe.rb b/app/services/standard_edition_migrator/base_recipe.rb index f12e7389c4c..ec789462795 100644 --- a/app/services/standard_edition_migrator/base_recipe.rb +++ b/app/services/standard_edition_migrator/base_recipe.rb @@ -6,4 +6,28 @@ def legacy_presenter def build_edition(record) raise NotImplementedError, "Subclasses must implement build_edition!" end + + ### + # The below methods aren't used in Edition creation - they're used only for payload normalisation for comparison purposes + ### + + def ignore_legacy_content_fields(content) + # Noop + content + end + + def ignore_new_content_fields(content) + # Noop + content + end + + def ignore_legacy_links(links) + # Noop + links + end + + def ignore_new_links(links) + # Noop + links + end end diff --git a/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb b/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb index 4c12e16124b..e74e8c608c2 100644 --- a/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb +++ b/test/unit/app/services/standard_edition_migrator/base_recipe_test.rb @@ -14,4 +14,16 @@ class BaseRecipeTest < ActiveSupport::TestCase end assert_equal "Subclasses must implement build_edition!", error.message end + + test "defines noop methods for ignore_legacy_content_fields, ignore_new_content_fields, ignore_legacy_links, and ignore_new_links" do + recipe = StandardEditionMigrator::BaseRecipe.new + + content = { "field" => "value" } + links = { "link" => "value" } + + assert_equal content, recipe.ignore_legacy_content_fields(content) + assert_equal content, recipe.ignore_new_content_fields(content) + assert_equal links, recipe.ignore_legacy_links(links) + assert_equal links, recipe.ignore_new_links(links) + end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index 6d5a347cdd2..ec654dca134 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -78,6 +78,24 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase summary = StandardEditionMigrator.preview_migration(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) assert summary.start_with?(expected_output), "Expected output to start with:\n#{expected_output}\n\nActual output:\n#{summary}" end + + it "makes a call to `diff_payloads` for content and links" do + StandardEditionMigrator.any_instance.expects(:diff_content_payloads).with( + has_entries( + recipe: instance_of(StandardEditionMigrator::RecipeForNonEditionableRecord), + old_content: anything, + new_content: anything, + ), + ) + StandardEditionMigrator.any_instance.expects(:diff_links_payloads).with( + has_entries( + recipe: instance_of(StandardEditionMigrator::RecipeForNonEditionableRecord), + old_links: anything, + new_links: anything, + ), + ) + StandardEditionMigrator.preview_migration(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + end end context "when passing an editionable record" do @@ -136,6 +154,108 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase summary = StandardEditionMigrator.preview_migration(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) assert summary.start_with?(expected_output), "Expected output to start with:\n#{expected_output}\n\nActual output:\n#{summary}" end + + it "makes a call to `diff_payloads` for content and links" do + StandardEditionMigrator.any_instance.expects(:diff_content_payloads).with( + has_entries( + recipe: instance_of(StandardEditionMigrator::RecipeForLegacyEditionableDocument), + old_content: anything, + new_content: anything, + ), + ) + StandardEditionMigrator.any_instance.expects(:diff_links_payloads).with( + has_entries( + recipe: instance_of(StandardEditionMigrator::RecipeForLegacyEditionableDocument), + old_links: anything, + new_links: anything, + ), + ) + StandardEditionMigrator.preview_migration(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + end + end + end + + describe "#diff_content_payloads (using `ignore_legacy_content_fields` and `ignore_new_content_fields`)" do + it "returns a diff of the two payloads, ignoring any fields specified in the recipe" do + old_content = { + field_we_are_persisting_and_changing: "value that changed", + field_we_are_persisting_unchanged: "foo", + field_we_dont_care_about: "some old value we don't care about", + } + new_content = { + field_we_are_persisting_and_changing: "new value", + field_we_are_persisting_unchanged: "foo", + new_field_that_has_no_legacy_equivalent: "hello", + } + recipe = Class.new { + def ignore_legacy_content_fields(content) + content.delete(:field_we_dont_care_about) + content + end + + def ignore_new_content_fields(content) + content.delete(:new_field_that_has_no_legacy_equivalent) + content + end + }.new + + diff = StandardEditionMigrator.diff_content_payloads( + recipe: recipe, + old_content: old_content, + new_content: new_content, + ) + + expected_diff = <<~DIFF + { + - "field_we_are_persisting_and_changing": "value that changed", + + "field_we_are_persisting_and_changing": "new value", + "field_we_are_persisting_unchanged": "foo" + } + DIFF + + assert_equal expected_diff, diff + end + end + + describe "#diff_links_payloads (using `ignore_legacy_links` and `ignore_new_links`)" do + it "returns a diff of the two payloads, ignoring any fields specified in the recipe" do + old_links = { + link_we_are_persisting_and_changing: "value that changed", + link_we_are_persisting_unchanged: "foo", + link_we_dont_care_about: "some old value we don't care about", + } + new_links = { + link_we_are_persisting_and_changing: "new value", + link_we_are_persisting_unchanged: "foo", + new_link_that_has_no_legacy_equivalent: "hello", + } + recipe = Class.new { + def ignore_legacy_links(links) + links.delete(:link_we_dont_care_about) + links + end + + def ignore_new_links(links) + links.delete(:new_link_that_has_no_legacy_equivalent) + links + end + }.new + + diff = StandardEditionMigrator.diff_links_payloads( + recipe: recipe, + old_links: old_links, + new_links: new_links, + ) + + expected_diff = <<~DIFF + { + - "link_we_are_persisting_and_changing": "value that changed", + + "link_we_are_persisting_and_changing": "new value", + "link_we_are_persisting_unchanged": "foo" + } + DIFF + + assert_equal expected_diff, diff end end end From 6e91c3aa880ecc9ada5da14c57e9b32c0a5c3496 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Thu, 4 Jun 2026 09:32:05 +0100 Subject: [PATCH 06/12] Add WhitehallError base class As introduced in draft PR #11431. Sometimes we just want a generic Whitehall error, or a way of catching multiple Whitehall errors, without accidentally swallowing unexpected errors like NoMethodError etc. --- app/models/whitehall_error.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 app/models/whitehall_error.rb diff --git a/app/models/whitehall_error.rb b/app/models/whitehall_error.rb new file mode 100644 index 00000000000..6e7642a5bee --- /dev/null +++ b/app/models/whitehall_error.rb @@ -0,0 +1,14 @@ +# Base class for Whitehall-specific domain errors. +# +# We rescue WhitehallError in places where we want to handle +# expected business/application failures gracefully, without also +# swallowing unexpected framework or programming errors. +# +# Avoid rescuing StandardError directly unless we genuinely intend +# to catch any application exception, including Rails, ActiveRecord, +# or Ruby runtime errors. +# +# Any error inheriting from WhitehallError is considered part of +# Whitehall's intentional public error surface. +class WhitehallError < StandardError +end From 2518a41af33f5aef3e00979ea1609e3eedd8189d Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 15:49:10 +0100 Subject: [PATCH 07/12] Implement `StandardEditionMigrator.create_new_document` This method takes a legacy record and a recipe, and creates a new Document (and a 'published' Edition, though this is down to the recipe - in theory the Edition could be persisted in a different state if the dev wishes), with the same content ID as before (allowing us to overwrite the old content in-place in Publishing API). This is where a `save_artefacts!` method becomes necessary on the recipe; whilst the StandardEditionMigrator can handle the saving of the Document and the Edition, it has no knowledge of any related records created within the recipe (e.g. associations with organisations, featurings, images etc). It is up to the recipe to store a reference to these in-memory associations under an array called `@artefacts_to_save`, and the StandardEditionMigrator iterates over said artefacts to save them. We also have a two-pass save system, where all records are saved without validation initially, as some may be transiently invalid whilst waiting for dependent records to be saved. Once all records are persisted, we expect that all records should be considered valid, so we test this by re-saving with `validate: true`. --- app/services/standard_edition_migrator.rb | 27 +++++++++++ .../standard_edition_migrator/base_recipe.rb | 11 +++++ .../recipe_for_legacy_editionable_document.rb | 2 +- .../recipe_for_non_editionable_record.rb | 1 + .../standard_edition_migrator_test.rb | 47 +++++++++++++++++++ 5 files changed, 87 insertions(+), 1 deletion(-) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index cf0d914e9dd..f8aff473a3d 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -11,6 +11,10 @@ def self.diff_links_payloads(...) new.diff_links_payloads(...) end + def self.create_new_document(...) + new.create_new_document(...) + end + def preview_migration(legacy_record, recipe) if legacy_record.is_a?(Edition) raise "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)" @@ -38,6 +42,29 @@ def diff_links_payloads(recipe:, old_links: nil, new_links: nil) ).to_s end + def create_new_document(legacy_record, recipe) + document = Document.new(document_type: "StandardEdition", content_id: legacy_record.content_id) + + ActiveRecord::Base.transaction do + recipe_instance = recipe.new + raise "Cannot pass a Document to create_new_document" if legacy_record.is_a?(Document) + + edition = recipe_instance.build_edition(legacy_record) + edition.document = document + + # Save without validation first, to get all interdependent artefacts + # persisted. Then re-save with validation applied - rolling back if + # a validation error is raised. + [false, true].each do |validate| + edition.save!(validate:) + recipe_instance.save_artefacts!(edition:, validate:) + document.save!(validate:) + end + end + + document + end + private def compare_payloads(legacy_record, recipe) diff --git a/app/services/standard_edition_migrator/base_recipe.rb b/app/services/standard_edition_migrator/base_recipe.rb index ec789462795..bd8f92dc7bb 100644 --- a/app/services/standard_edition_migrator/base_recipe.rb +++ b/app/services/standard_edition_migrator/base_recipe.rb @@ -7,6 +7,17 @@ def build_edition(record) raise NotImplementedError, "Subclasses must implement build_edition!" end + def save_artefacts!(validate:, edition:) + # This is where the Recipe can handle saving any associated artefacts (e.g. Features, Organisations, etc.). + (@artefacts_to_save || []).each do |artefact| + # Translations etc need to be associated with the edition before they can be saved + if artefact.respond_to?(:edition_id=) + artefact.edition_id = edition.id + end + artefact.save!(validate: validate) + end + end + ### # The below methods aren't used in Edition creation - they're used only for payload normalisation for comparison purposes ### diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb index b11c188416a..01e731c1cca 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -22,7 +22,7 @@ def build_edition(legacy_record) }, ) end - + @artefacts_to_save = edition.translations edition end end diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb index 1b820292e4f..24a60c3fe40 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_non_editionable_record.rb @@ -24,6 +24,7 @@ def build_edition(legacy_record) }, ) end + @artefacts_to_save = edition.translations edition end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index ec654dca134..ad1ddae065d 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -258,4 +258,51 @@ def ignore_new_links(links) assert_equal expected_diff, diff end end + + describe "#create_new_document" do + it "performs the migration and saves a new document and edition, when given a legacy non-editionable record" do + document = StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + edition = StandardEdition.last + + assert_equal document.content_id, @legacy_non_editionable_record.content_id + assert edition.persisted? + assert_equal edition, document.editions.last + assert_equal "test_type", edition.configurable_document_type + assert_equal "Title", edition.translations.first.title + assert_equal({ "field_attribute" => "Old body" }, edition.translations.first.block_content) + end + + it "saves the edition, artefacts and document first without validation, then with validation" do + StandardEdition.any_instance.expects(:save!).with(validate: false) + StandardEditionMigrator::RecipeForNonEditionableRecord.any_instance.expects(:save_artefacts!).with(validate: false, edition: instance_of(StandardEdition)) + Document.any_instance.expects(:save!).with(validate: false) + + StandardEdition.any_instance.expects(:save!).with(validate: true) + StandardEditionMigrator::RecipeForNonEditionableRecord.any_instance.expects(:save_artefacts!).with(validate: true, edition: instance_of(StandardEdition)) + Document.any_instance.expects(:save!).with(validate: true) + + StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + end + + it "rolls back the transaction if a validation issue is encountered" do + Document.any_instance.stubs(:save!).returns(true) + Document.any_instance.expects(:save!).with(validate: true).raises(WhitehallError.new("Validation failed: Title can't be blank")) + + assert_no_difference [StandardEdition.method(:count), Document.method(:count)] do + error = assert_raises(WhitehallError) do + StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + end + assert_equal "Validation failed: Title can't be blank", error.message + end + end + + it "raises exception if a Document is passed (we could handle this in theory, but for now, for simplicity we expect only non-Document records)" do + document = build(:document) + error = assert_raises(RuntimeError) do + StandardEditionMigrator.create_new_document(document, StandardEditionMigrator::RecipeForNonEditionableRecord) + end + + assert_equal "Cannot pass a Document to create_new_document", error.message + end + end end From 2d4009ac3472557f39d58340603a72a3b310d354 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 16:50:49 +0100 Subject: [PATCH 08/12] Implement `StandardEditionMigrator.migrate_existing_document` This takes an existing Document and all of its Editions (including superseded and deleted ones) and migrates all of them in-place to StandardEdition. --- app/services/standard_edition_migrator.rb | 36 +++++++++++++++++ .../recipe_for_legacy_editionable_document.rb | 1 + .../standard_edition_migrator_test.rb | 40 +++++++++++++++++++ 3 files changed, 77 insertions(+) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index f8aff473a3d..a99b7af89c0 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -15,6 +15,10 @@ def self.create_new_document(...) new.create_new_document(...) end + def self.migrate_existing_document(...) + new.migrate_existing_document(...) + end + def preview_migration(legacy_record, recipe) if legacy_record.is_a?(Edition) raise "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)" @@ -65,6 +69,38 @@ def create_new_document(legacy_record, recipe) document end + def migrate_existing_document(document, recipe) + ActiveRecord::Base.transaction do + recipe_instance = recipe.new + raise "Cannot pass a non-Document to migrate_existing_document" unless document.is_a?(Document) + + editions_to_update = Edition.unscoped.where(document: document) + document.update_column(:document_type, "StandardEdition") + + # Update each edition in-place + editions_to_update.each do |legacy_edition| + edition = recipe_instance.build_edition(legacy_edition) + + # Convert the legacy edition instance type in-memory to StandardEdition + legacy_edition = legacy_edition.becomes!(StandardEdition) + # Overwrite the legacy edition's attributes with the new edition's attributes + # (except for id and document_id - we want to retain the original IDs, not the + # new ones generated by build_edition) + legacy_edition.assign_attributes(edition.attributes.except("id", "document_id")) + legacy_edition.auth_bypass_id ||= edition.auth_bypass_id || [] #  can't be null + + # Save without validation first, to get all interdependent artefacts + # persisted. Then re-save with validation applied - rolling back if + # a validation error is raised. + [false, true].each do |validate| + legacy_edition.save!(validate:) + recipe_instance.save_artefacts!(edition:, validate:) + end + end + end + document + end + private def compare_payloads(legacy_record, recipe) diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb index 01e731c1cca..880e696b9ab 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -9,6 +9,7 @@ def build_edition(legacy_record) updated_at: legacy_record.updated_at.rfc3339, creator: User.last, document: legacy_record.document, + change_note: legacy_record.change_note, } edition = StandardEdition.new(edition_attrs) diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index ad1ddae065d..fa796b82eaa 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -305,4 +305,44 @@ def ignore_new_links(links) assert_equal "Cannot pass a Document to create_new_document", error.message end end + + describe "#migrate_existing_document" do + it "migrates all of a given Document's Editions (including deleted and superseded ones)" do + StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + + assert_equal "StandardEdition", @legacy_editionable_document.document_type + + Edition.unscoped.where(document_id: @legacy_editionable_document.id).find_each do |edition| + assert_equal "StandardEdition", edition.type + assert_equal "test_type", edition.configurable_document_type + assert_equal "Title", edition.translations.first.title + assert_equal "Summary", edition.translations.first.summary + assert_equal({ "field_attribute" => "Old body" }, edition.translations.first.block_content) + end + end + + test "rolls back the transaction if a validation issue is encountered" do + document = create(:document, document_type: "DetailedGuide") + create(:draft_detailed_guide, document: document, title: "Detailed Guide", summary: "Summary", body: "Old body") + Edition.any_instance.stubs(:save!).returns(true) + Edition.any_instance.expects(:save!).with(validate: true).raises(WhitehallError.new("Validation failed: Title can't be blank")) + + error = assert_raises(WhitehallError) do + StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + end + assert_equal "Validation failed: Title can't be blank", error.message + assert_equal "DetailedGuide", document.reload.document_type + Edition.unscoped.where(document_id: document.id).find_each do |edition| + assert_not_equal "StandardEdition", edition.type + end + end + + it "raises exception if a non-Document is passed (non-Documents should always use `create_new_document`)" do + error = assert_raises(RuntimeError) do + StandardEditionMigrator.migrate_existing_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + end + + assert_equal "Cannot pass a non-Document to migrate_existing_document", error.message + end + end end From 4f58049c1e4adcfa2bf4a2a830d8064d4d1709be Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 17:24:57 +0100 Subject: [PATCH 09/12] Add `raise_if_payloads_differ` parameter We want the security of being able to dynamically compare our normalised payloads across all editions being migrated, BEFORE committing to saving those changes. If there are any unexpected differences in the payloads, we want to know about them and raise an exception so that we can investigate and iterate the recipe. --- app/services/standard_edition_migrator.rb | 33 ++++++++++++++++--- .../standard_edition_migrator_test.rb | 30 +++++++++++++---- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index a99b7af89c0..d78089e2feb 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -46,14 +46,14 @@ def diff_links_payloads(recipe:, old_links: nil, new_links: nil) ).to_s end - def create_new_document(legacy_record, recipe) + def create_new_document(legacy_record, recipe, raise_if_payloads_differ: true) document = Document.new(document_type: "StandardEdition", content_id: legacy_record.content_id) ActiveRecord::Base.transaction do recipe_instance = recipe.new raise "Cannot pass a Document to create_new_document" if legacy_record.is_a?(Document) - edition = recipe_instance.build_edition(legacy_record) + edition = build_edition(recipe_instance, legacy_record, raise_if_payloads_differ) edition.document = document # Save without validation first, to get all interdependent artefacts @@ -69,7 +69,7 @@ def create_new_document(legacy_record, recipe) document end - def migrate_existing_document(document, recipe) + def migrate_existing_document(document, recipe, raise_if_payloads_differ: true) ActiveRecord::Base.transaction do recipe_instance = recipe.new raise "Cannot pass a non-Document to migrate_existing_document" unless document.is_a?(Document) @@ -79,7 +79,7 @@ def migrate_existing_document(document, recipe) # Update each edition in-place editions_to_update.each do |legacy_edition| - edition = recipe_instance.build_edition(legacy_edition) + edition = build_edition(recipe_instance, legacy_edition, raise_if_payloads_differ) # Convert the legacy edition instance type in-memory to StandardEdition legacy_edition = legacy_edition.becomes!(StandardEdition) @@ -103,6 +103,31 @@ def migrate_existing_document(document, recipe) private + def build_edition(recipe_instance, legacy_edition, raise_if_payloads_differ) + return recipe_instance.build_edition(legacy_edition) unless raise_if_payloads_differ + + old_presenter = recipe_instance.legacy_presenter.new(legacy_edition) + edition = recipe_instance.build_edition(legacy_edition) + diff = diff_content_payloads( + recipe: recipe_instance, + old_content: old_presenter.content, + new_content: PublishingApi::StandardEditionPresenter.new(edition).content, + ) + diff_links_payloads( + recipe: recipe_instance, + old_links: old_presenter.links, + new_links: PublishingApi::StandardEditionPresenter.new(edition).links, + ) + if diff.present? + raise <<~ERROR + The legacy and new edition payloads differ after normalisation. + + #{diff} + ERROR + end + + edition + end + def compare_payloads(legacy_record, recipe) # Grab the payloads from the old presenter _before_ we do any mutation, to ensure we're comparing against the original payload old_presenter = recipe.new.legacy_presenter.new(legacy_record) diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index fa796b82eaa..db4b7a6fd31 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -261,7 +261,7 @@ def ignore_new_links(links) describe "#create_new_document" do it "performs the migration and saves a new document and edition, when given a legacy non-editionable record" do - document = StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + document = StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) edition = StandardEdition.last assert_equal document.content_id, @legacy_non_editionable_record.content_id @@ -281,7 +281,7 @@ def ignore_new_links(links) StandardEditionMigrator::RecipeForNonEditionableRecord.any_instance.expects(:save_artefacts!).with(validate: true, edition: instance_of(StandardEdition)) Document.any_instance.expects(:save!).with(validate: true) - StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) end it "rolls back the transaction if a validation issue is encountered" do @@ -290,7 +290,7 @@ def ignore_new_links(links) assert_no_difference [StandardEdition.method(:count), Document.method(:count)] do error = assert_raises(WhitehallError) do - StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord) + StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) end assert_equal "Validation failed: Title can't be blank", error.message end @@ -299,16 +299,24 @@ def ignore_new_links(links) it "raises exception if a Document is passed (we could handle this in theory, but for now, for simplicity we expect only non-Document records)" do document = build(:document) error = assert_raises(RuntimeError) do - StandardEditionMigrator.create_new_document(document, StandardEditionMigrator::RecipeForNonEditionableRecord) + StandardEditionMigrator.create_new_document(document, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) end assert_equal "Cannot pass a Document to create_new_document", error.message end + + it "raises exception if the normalised payloads differ between the legacy record and new edition, if `raise_if_payloads_differ` is true" do + error = assert_raises(RuntimeError) do + StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: true) + end + + assert error.message.start_with?("The legacy and new edition payloads differ after normalisation."), "Expected output to start with:\n#{error.message}\n\nActual output:\n#{error.message}" + end end describe "#migrate_existing_document" do it "migrates all of a given Document's Editions (including deleted and superseded ones)" do - StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument, raise_if_payloads_differ: false) assert_equal "StandardEdition", @legacy_editionable_document.document_type @@ -328,7 +336,7 @@ def ignore_new_links(links) Edition.any_instance.expects(:save!).with(validate: true).raises(WhitehallError.new("Validation failed: Title can't be blank")) error = assert_raises(WhitehallError) do - StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument, raise_if_payloads_differ: false) end assert_equal "Validation failed: Title can't be blank", error.message assert_equal "DetailedGuide", document.reload.document_type @@ -339,10 +347,18 @@ def ignore_new_links(links) it "raises exception if a non-Document is passed (non-Documents should always use `create_new_document`)" do error = assert_raises(RuntimeError) do - StandardEditionMigrator.migrate_existing_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForLegacyEditionableDocument) + StandardEditionMigrator.migrate_existing_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForLegacyEditionableDocument, raise_if_payloads_differ: false) end assert_equal "Cannot pass a non-Document to migrate_existing_document", error.message end + + it "raises exception if the normalised payloads differ between the legacy and new edition, if `raise_if_payloads_differ` is true" do + error = assert_raises(RuntimeError) do + StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument, raise_if_payloads_differ: true) + end + + assert error.message.start_with?("The legacy and new edition payloads differ after normalisation."), "Expected output to start with:\n#{error.message}\n\nActual output:\n#{error.message}" + end end end From c4ba19b2ad96da3b57c11b548defbd7733266426 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 17:49:04 +0100 Subject: [PATCH 10/12] Add EditorialRemark and AuditTrail on successful migration We want to leave some sort of note to the publisher, and to our future selves, that the migrated document originated from a legacy document. --- app/services/standard_edition_migrator.rb | 80 ++++++++++++------- .../standard_edition_migrator/base_recipe.rb | 4 + .../recipe_for_legacy_editionable_document.rb | 4 + .../standard_edition_migrator_test.rb | 20 +++++ 4 files changed, 80 insertions(+), 28 deletions(-) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index d78089e2feb..313cbb2dcce 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -53,16 +53,26 @@ def create_new_document(legacy_record, recipe, raise_if_payloads_differ: true) recipe_instance = recipe.new raise "Cannot pass a Document to create_new_document" if legacy_record.is_a?(Document) - edition = build_edition(recipe_instance, legacy_record, raise_if_payloads_differ) - edition.document = document - - # Save without validation first, to get all interdependent artefacts - # persisted. Then re-save with validation applied - rolling back if - # a validation error is raised. - [false, true].each do |validate| - edition.save!(validate:) - recipe_instance.save_artefacts!(edition:, validate:) - document.save!(validate:) + # Whitehall's in-house 'AuditTrail' is used to populate the timeline in the sidebar + robot_user = User.find_by(name: "Scheduled Publishing Robot") + AuditTrail.acting_as(robot_user) do + edition = build_edition(recipe_instance, legacy_record, raise_if_payloads_differ) + edition.document = document + + # Save without validation first, to get all interdependent artefacts + # persisted. Then re-save with validation applied - rolling back if + # a validation error is raised. + [false, true].each do |validate| + edition.save!(validate:) + recipe_instance.save_artefacts!(edition:, validate:) + document.save!(validate:) + end + + EditorialRemark.create!( + edition: edition, + body: recipe_instance.editorial_remark, + author: robot_user, + ) end end @@ -77,24 +87,38 @@ def migrate_existing_document(document, recipe, raise_if_payloads_differ: true) editions_to_update = Edition.unscoped.where(document: document) document.update_column(:document_type, "StandardEdition") - # Update each edition in-place - editions_to_update.each do |legacy_edition| - edition = build_edition(recipe_instance, legacy_edition, raise_if_payloads_differ) - - # Convert the legacy edition instance type in-memory to StandardEdition - legacy_edition = legacy_edition.becomes!(StandardEdition) - # Overwrite the legacy edition's attributes with the new edition's attributes - # (except for id and document_id - we want to retain the original IDs, not the - # new ones generated by build_edition) - legacy_edition.assign_attributes(edition.attributes.except("id", "document_id")) - legacy_edition.auth_bypass_id ||= edition.auth_bypass_id || [] #  can't be null - - # Save without validation first, to get all interdependent artefacts - # persisted. Then re-save with validation applied - rolling back if - # a validation error is raised. - [false, true].each do |validate| - legacy_edition.save!(validate:) - recipe_instance.save_artefacts!(edition:, validate:) + # Whitehall's in-house 'AuditTrail' is used to populate the timeline in the sidebar + robot_user = User.find_by(name: "Scheduled Publishing Robot") + + AuditTrail.acting_as(robot_user) do + # Update each edition in-place + editions_to_update.each_with_index do |legacy_edition, index| + edition = build_edition(recipe_instance, legacy_edition, raise_if_payloads_differ) + + # Convert the legacy edition instance type in-memory to StandardEdition + legacy_edition = legacy_edition.becomes!(StandardEdition) + # Overwrite the legacy edition's attributes with the new edition's attributes + # (except for id and document_id - we want to retain the original IDs, not the + # new ones generated by build_edition) + legacy_edition.assign_attributes(edition.attributes.except("id", "document_id")) + legacy_edition.auth_bypass_id ||= edition.auth_bypass_id || [] #  can't be null + + # Save without validation first, to get all interdependent artefacts + # persisted. Then re-save with validation applied - rolling back if + # a validation error is raised. + [false, true].each do |validate| + legacy_edition.save!(validate:) + recipe_instance.save_artefacts!(edition: legacy_edition, validate:) + end + + # Add an editorial remark to the last edition only + next unless index == editions_to_update.size - 1 + + EditorialRemark.create!( + edition: legacy_edition, + body: recipe_instance.editorial_remark, + author: robot_user, + ) end end end diff --git a/app/services/standard_edition_migrator/base_recipe.rb b/app/services/standard_edition_migrator/base_recipe.rb index bd8f92dc7bb..7576377a4e1 100644 --- a/app/services/standard_edition_migrator/base_recipe.rb +++ b/app/services/standard_edition_migrator/base_recipe.rb @@ -18,6 +18,10 @@ def save_artefacts!(validate:, edition:) end end + def editorial_remark + "Migrated to StandardEdition" + end + ### # The below methods aren't used in Edition creation - they're used only for payload normalisation for comparison purposes ### diff --git a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb index 880e696b9ab..919da4cd728 100644 --- a/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb +++ b/test/unit/app/services/standard_edition_migrator/fixtures/recipe_for_legacy_editionable_document.rb @@ -26,4 +26,8 @@ def build_edition(legacy_record) @artefacts_to_save = edition.translations edition end + + def editorial_remark + "Migrated legacy editionable document to StandardEdition" + end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index db4b7a6fd31..41d89cf7235 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -14,6 +14,7 @@ class StandardEditionMigratorTest < ActiveSupport::TestCase create(:superseded_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") create(:published_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") create(:draft_detailed_guide, document: @legacy_editionable_document, title: "Detailed Guide", summary: "Summary", body: "Old body") + @robot_user = create(:user, name: "Scheduled Publishing Robot") end describe "#preview_migration" do @@ -284,6 +285,14 @@ def ignore_new_links(links) StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) end + it "creates an EditorialRemark associated with the robot user" do + document = StandardEditionMigrator.create_new_document(@legacy_non_editionable_record, StandardEditionMigrator::RecipeForNonEditionableRecord, raise_if_payloads_differ: false) + edition = document.editions.last + + assert_equal "Migrated to StandardEdition", edition.editorial_remarks.last.body + assert_equal @robot_user, edition.editorial_remarks.last.author + end + it "rolls back the transaction if a validation issue is encountered" do Document.any_instance.stubs(:save!).returns(true) Document.any_instance.expects(:save!).with(validate: true).raises(WhitehallError.new("Validation failed: Title can't be blank")) @@ -329,6 +338,17 @@ def ignore_new_links(links) end end + it "creates an EditorialRemark associated with the robot user, on the last edition only" do + document = StandardEditionMigrator.migrate_existing_document(@legacy_editionable_document, StandardEditionMigrator::RecipeForLegacyEditionableDocument, raise_if_payloads_differ: false) + edition = document.editions.last + + assert_equal "Migrated legacy editionable document to StandardEdition", edition.editorial_remarks.last.body + assert_equal @robot_user, edition.editorial_remarks.last.author + Edition.unscoped.where(document_id: document.id).where.not(id: edition.id).find_each do |other_edition| + assert_empty other_edition.editorial_remarks + end + end + test "rolls back the transaction if a validation issue is encountered" do document = create(:document, document_type: "DetailedGuide") create(:draft_detailed_guide, document: document, title: "Detailed Guide", summary: "Summary", body: "Old body") From 099f49ce003d708a0f2b41f8b00a7fd52fb4d98e Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Mon, 15 Jun 2026 17:52:47 +0100 Subject: [PATCH 11/12] Implement `StandardEdition.bulk_enqueue_migration` for jobs This method queues up StandardEditionMigratorJobs for every legacy record in scope. --- app/services/standard_edition_migrator.rb | 18 ++++++++++ app/sidekiq/standard_edition_migrator_job.rb | 12 +++++++ .../standard_edition_migrator_test.rb | 35 +++++++++++++++++++ .../standard_edition_migrator_job_test.rb | 19 ++++++++++ 4 files changed, 84 insertions(+) diff --git a/app/services/standard_edition_migrator.rb b/app/services/standard_edition_migrator.rb index 313cbb2dcce..e4b5b1b44fd 100644 --- a/app/services/standard_edition_migrator.rb +++ b/app/services/standard_edition_migrator.rb @@ -19,6 +19,10 @@ def self.migrate_existing_document(...) new.migrate_existing_document(...) end + def self.bulk_enqueue_migration(...) + new.bulk_enqueue_migration(...) + end + def preview_migration(legacy_record, recipe) if legacy_record.is_a?(Edition) raise "An Edition was passed. You must pass the Document instead (so that we can migrate all of its Editions)" @@ -125,6 +129,20 @@ def migrate_existing_document(document, recipe, raise_if_payloads_differ: true) document end + def bulk_enqueue_migration(legacy_records, recipe_class, migration_method:, raise_if_payloads_differ: true) + legacy_records.each do |legacy_record| + StandardEditionMigratorJob.perform_async( + legacy_record.id, + { + "model_class" => legacy_record.class.name, + "recipe_class" => recipe_class.name, + "migration_method" => migration_method, + "raise_if_payloads_differ" => raise_if_payloads_differ, + }, + ) + end + end + private def build_edition(recipe_instance, legacy_edition, raise_if_payloads_differ) diff --git a/app/sidekiq/standard_edition_migrator_job.rb b/app/sidekiq/standard_edition_migrator_job.rb index 545c4b574ab..662cfbb3a5e 100644 --- a/app/sidekiq/standard_edition_migrator_job.rb +++ b/app/sidekiq/standard_edition_migrator_job.rb @@ -3,4 +3,16 @@ class StandardEditionMigratorJob < JobBase # ‘internal’ – so it won’t fail because a third-party API is down, # and any failure is unlikely to resolve itself on a retry. sidekiq_options queue: "standard_edition_migration", retry: 0 + + def perform(record_id, args) + model_class_name = args["model_class"] + recipe_class_name = args["recipe_class"] + migration_method = args["migration_method"] + raise_if_payloads_differ = args["raise_if_payloads_differ"] + + legacy_record = model_class_name.constantize.find(record_id) + recipe = recipe_class_name.constantize + + StandardEditionMigrator.send(migration_method, legacy_record, recipe, raise_if_payloads_differ: raise_if_payloads_differ) + end end diff --git a/test/unit/app/services/standard_edition_migrator_test.rb b/test/unit/app/services/standard_edition_migrator_test.rb index 41d89cf7235..91e98b47ea0 100644 --- a/test/unit/app/services/standard_edition_migrator_test.rb +++ b/test/unit/app/services/standard_edition_migrator_test.rb @@ -381,4 +381,39 @@ def ignore_new_links(links) assert error.message.start_with?("The legacy and new edition payloads differ after normalisation."), "Expected output to start with:\n#{error.message}\n\nActual output:\n#{error.message}" end end + + describe "#bulk_enqueue_migration" do + test "enqueues a migration job for each legacy record, with the correct recipe and migration method" do + legacy_non_editionable_records = [ + create(:organisation, name: "My first org"), + create(:organisation, name: "My second org"), + ] + StandardEditionMigrator.new.bulk_enqueue_migration( + legacy_non_editionable_records, + StandardEditionMigrator::RecipeForNonEditionableRecord, + migration_method: "create_new_document", + raise_if_payloads_differ: true, + ) + + assert_equal 2, StandardEditionMigratorJob.jobs.size + + first_job_args = StandardEditionMigratorJob.jobs.first["args"] + record_id = first_job_args.first + keyword_args = first_job_args.second + assert_equal legacy_non_editionable_records.first.id, record_id + assert_equal "Organisation", keyword_args["model_class"] + assert_equal "StandardEditionMigrator::RecipeForNonEditionableRecord", keyword_args["recipe_class"] + assert_equal "create_new_document", keyword_args["migration_method"] + assert_equal true, keyword_args["raise_if_payloads_differ"] + + second_job_args = StandardEditionMigratorJob.jobs.second["args"] + record_id = second_job_args.first + keyword_args = second_job_args.second + assert_equal legacy_non_editionable_records.second.id, record_id + assert_equal "Organisation", keyword_args["model_class"] + assert_equal "StandardEditionMigrator::RecipeForNonEditionableRecord", keyword_args["recipe_class"] + assert_equal "create_new_document", keyword_args["migration_method"] + assert_equal true, keyword_args["raise_if_payloads_differ"] + end + end end diff --git a/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb b/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb index 0d2bd070123..7793a13e0cc 100644 --- a/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb +++ b/test/unit/app/sidekiq/standard_edition_migrator_job_test.rb @@ -8,4 +8,23 @@ class StandardEditionMigratorJobTest < ActiveSupport::TestCase assert_equal({ "retry" => 0, "queue" => "standard_edition_migration" }, StandardEditionMigratorJob.sidekiq_options) end end + + describe "#perform" do + it "calls the migration method on StandardEditionMigrator with the correct arguments" do + legacy_record = create(:organisation) + recipe_class = StandardEditionMigrator::BaseRecipe + + StandardEditionMigrator.expects(:create_new_document).with(legacy_record, recipe_class, raise_if_payloads_differ: true) + + StandardEditionMigratorJob.new.perform( + legacy_record.id, + { + "model_class" => "Organisation", + "recipe_class" => "StandardEditionMigrator::BaseRecipe", + "migration_method" => "create_new_document", + "raise_if_payloads_differ" => true, + }, + ) + end + end end From a3a8cc2e0d17133f3c984d7a03d4b29b0e4df7b2 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Thu, 11 Jun 2026 07:00:11 +0100 Subject: [PATCH 12/12] Document new StandardEditionMigrator workflow / API --- docs/migrating_to_standard_edition.md | 103 +++++++------------------- 1 file changed, 27 insertions(+), 76 deletions(-) diff --git a/docs/migrating_to_standard_edition.md b/docs/migrating_to_standard_edition.md index a2e482af215..0c56922b37a 100644 --- a/docs/migrating_to_standard_edition.md +++ b/docs/migrating_to_standard_edition.md @@ -16,105 +16,56 @@ We're gradually migrating all document types to use the StandardEdition model, a ## Steps to migrate a legacy content type -These steps assume you have already recreated the legacy content type as a StandardEdition (above). +Use the [StandardEditionMigrator](https://github.com/alphagov/whitehall/blob/main/app/services/standard_edition_migrator.rb) to migrate both editionable and non-editionable content types. -### The legacy content type already uses the Edition model +The StandardEditionMigrator takes a 'recipe' that tells the migrator service how to build a StandardEdition from the legacy record. It also has built-in payload comparison, so that it can take the Publishing API payload of the legacy content item and 'diff' it against the StandardEdition Publishing API payload of the _converted_ content item, aborting the migration if the diff isn't as expected. -If the legacy content type already uses the Edition model, the migration is made much simpler by use of the [StandardEditionMigrator](https://github.com/alphagov/whitehall/blob/main/app/services/standard_edition_migrator.rb) class. +Conceptually, the diff should be empty: converting legacy content types to being config-driven is a straight up refactor that shouldn't affect the end user experience. In practice, the diffs end up being subtly different, so we have to configure the recipe to state which bits of the diff are 'expected'. -The StandardEditionMigrator takes a 'recipe' ([example](https://github.com/alphagov/whitehall/pull/10814/changes#diff-08a7cc438a61f2e81368a00c4b5753e52a072e2e4ddc30563eb531d4d041f139R1)) that tells the migrator service how to map the legacy fields to the config-driven `block_content` structure (`map_legacy_fields_to_block_content`). It also has built-in payload comparison, so that it can take the Publishing API payload of the legacy content item and 'diff' it against the StandardEdition Publishing API payload of the _converted_ content item, aborting the migration if the diff isn't as expected. +Your recipe will need to define the following methods: -Conceptually, the diff should be empty: converting legacy content types to being config-driven is a straight up refactor that shouldn't affect the end user experience. In practice, the diffs end up being subtly different, so we have to configure the recipe to state which bits of the diff are 'expected'. +- `legacy_presenter` - the presenter for the legacy content type, e.g. `PublishingApi::NewsArticlePresenter` +- `build_edition(legacy_record)` - create a StandardEdition, and assign its attributes based on what is in the legacy_record. This is the main part of the recipe. *Important*: be careful to no persist anything here as this method is also called by the preview_migration method. It is cleaner to build all of this in memory, than to make the actual changes and then rely on rolling back (which can have unexpected side effects such as updating Publishing API). + +Then the following methods are all about normalisation of the payloads, for comparison purposes: -The API is as follows: -- `configurable_document_type` - the config-driven 'configurable_document_type', e.g. `news_story` -- `presenter` - the presenter for the legacy content type, e.g. `PublishingApi::NewsArticlePresenter` -- `map_legacy_fields_to_block_content(edition, translation)` - defines which top level 'fields' of the legacy edition should be injected into the `block_content` of the config-driven edition. E.g. `{ "body" => translation.body }`. - `ignore_legacy_content_fields(content)` - defines which fields in the `details` hash of the legacy content item will not be carried over to the `details` hash of the config-driven content item. (Sometimes we consciously drop fields if they're no longer required). E.g. `content[:details].delete(:first_public_at)` (which is a legacy field no longer needed - see [this PR description](https://github.com/alphagov/whitehall/pull/10670)). - `ignore_new_content_fields(content)` - defines which fields in the `details` hash of the config-driven content item were not present in the legacy content item. E.g. `content[:details].delete(:image)` - `ignore_legacy_links(links)` - defines which links in the legacy content item will not be carried over to the links of the config-driven content item. E.g. `links.delete(:worldwide_organisations)` - `ignore_new_links(links)` - defines which links in the config-driven content item were not present on the legacy content item. If no changes, just return unchanged, i.e. `links`. The same applies to all of the above arguments. -The migrator itself offers two methods: +The migrator itself offers a few methods: -- `preview`: to see how many unique documents and editions are in scope for migration -- `migrate!`: to perform the actual migration. It takes two arguments: - - `compare_payloads` (default `true`): runs the legacy content item through its Publishing API presenter and compares it with the converted content item through the StandardEdition presenter, raising an exception if the diff contains any differences not accounted for by the recipe. - It is highly recommended to keep this on, but it does slow down the migration somewhat. - - `republish` (default `false`): whether or not to republish the document after it has been migrated. - This is a nice idea in theory, but in practice can cause [race conditions with document slugs being changed](https://gov-uk.atlassian.net/browse/WHIT-2766?focusedCommentId=165942), so use with caution. +- `preview_migration(legacy_record, recipe)` - given an old record and a recipe, this will build the StandardEdition in memory and summarise its before-and-after payloads, including any diff. +- `migrate_existing_document(legacy_record, recipe, raise_if_payloads_differ: boolean)` - overwrites an existing (legacy) Document and all of its Editions. It is intended to be called for legacy edition types such as Publication, Speech, etc. It is not compatible with other content types. Passing `raise_if_payloads_differ: true` aborts the migration if the normalised payloads don't match. +- `create_new_document(legacy_record, recipe, raise_if_payloads_differ: boolean)` - creates a new Document instance and one 'published' Edition. It is intended to be called for legacy content types that don't follow the Document/Edition model, e.g. TopicalEvent, Organisation etc. It could in theory support legacy edition types too, so that we have an alternative to overwriting-in-place (could instead duplicate a given Document and then delete the old one). +- `bulk_enqueue_migration(scope, recipe, migration_method: string, raise_if_payloads_differ: boolean)` - enqueues the migration of an array of legacy records, using the migration method provided ("migrate_existing_document" or "create_new_document"), using StandardEditionJob. With that in mind, the steps for migrating a legacy content type to being config-driven is roughly as follows: -1. Write a recipe and [associated tests](https://github.com/alphagov/whitehall/pull/10814/changes#diff-54659f36bda92219acd8b13bc0a48192707fcd668c591e4651ddbe5b58981a4bR1). - -2. See how many unique documents and editions are in scope for migration: - `StandardEditionMigrator.new(scope: Document.where(document_type: "NewsArticle")).preview` - -3. Try converting a legacy content item (or several) locally. Example: - - ```ruby - array_of_one_test_document = Document.where(id: NewsArticle.where(id: 1733267).map(&:document_id)) - StandardEditionMigrator.new(scope: array_of_one_test_document).migrate! - ``` +1. Choose a guinea pig legacy document, e.g. `TopicalEvent.find(123)` and use it as the basis of writing your recipe. + `StandardEditionMigrator.preview_migration(TopicalEvent.find(123), StandardEditionMigrator::TopicalEventRecipe)`. + Gradually build up your recipe (and associated tests) until you have a recipe that fully carries over all the relevant fields and the normalised payloads match. - If you want a clearer view of what's going on, you could call the job directly, synchronously: +2. Try migrating that one document locally (using `migrate_existing_document` or `create_new_document`). + Manually test that the resulting StandardEdition is as you expect. If not, iterate the recipe. - ```ruby - StandardEditionMigratorJob.new.perform(document.id, { "republish" => true, "compare_payloads" => true }) - ``` +3. Try the same on Integration so that you can also check the converted content item looks as it should on the frontend (you'll need to edit and republish the document to have it present to Publishing API). Compare the resulting page with how it looks on Production. -4. Test the converted content item to make sure it still works as expected. +4. Try a few more examples locally, especially where you know they differ (e.g. some have translations, or 'features', or no associated organisations, etc). Keep iterating the recipe. -5. Do the same on Integration so that you can also check the converted content item looks as it should on the frontend. - Compare it with how it looks on Production. +5. When you think the recipe is complete, try a bulk migration of everything in scope (or take it more slowly to begin with, e.g. `.limit(10)`). -6. Repeat the steps locally for more content items. - You will likely run into edge cases you haven't factored in to your recipe, which you'll want to tweak accordingly. - - ```ruby - StandardEditionMigrator.new(scope: Document.where(document_type: "NewsArticle").first(5000)).migrate!(republish: false, compare_payloads: true) - ``` - -7. Come up with a plan for the handful of stragglers. +6. Come up with a plan for the handful of stragglers. You'll likely run into a few instances that, for whatever reason, cannot be easily auto-migrated. See [examples of issues we ran into with NewsArticles](https://gov-uk.atlassian.net/browse/WHIT-2487?focusedCommentId=165943). - Typically a little manual intervention is required, e.g. to patch up some data, before you can then invoke the `StandardEditionMigratorJob` to complete the migration of the document. + Typically a little manual intervention is required, e.g. to patch up some data first. 8. Test running the full migration (and any manual patches) on Integration. + If you've used `create_new_document`, you'll need to delete the old legacy records, e.g. `TopicalEvent.destroy_all`, so that you don't have two competing items per document. + When you're down to one item per document, [bulk republish by type](https://whitehall-admin.integration.publishing.service.gov.uk/government/admin/republishing/bulk/by-type/new) to present them all downstream. + Then keep an eye out for Sentry errors, and manually check a handful of documents to see if they've been updated as expected. -9. Run the full migration (and any manual patches) on Production. +9. Repeat for Production. 10. Celebrate! 🥳 Then delete the recipe - you won't need it again. - -### The legacy content type does not use the Edition model - -If the legacy content type doesn't use the Edition model (e.g. `TopicalEvent`), you can still use the `StandardEditionMigrator` — just pass a scope of the legacy records directly rather than a `Document` scope. - -For each record in scope, the migrator creates a brand new `Document` and `StandardEdition`, preserving the original record's `content_id` on the new document. Note that for the base path overwrite to work in Publishing API you'll need to do the groundwork described in [#11210](https://github.com/alphagov/whitehall/pull/11210) — this lets you interchangeably publish either the legacy content item or the new `StandardEdition` at the same path for testing purposes. The original record is left untouched, so you can remove it manually afterwards (or leave the two co-existing for a while if that's useful). - -The recipe works the same way as for editionable content types, with two differences: - -- Use the same `StandardEditionMigrator.recipe_for` method (it accepts any model, not just editions) -- Add `title(record_translation)` and `summary(record_translation)` methods — these are called once per source record translation, so if the record has multiple locales, each gets its own edition translation. The argument is the Globalize translation object, so you can read locale-specific attributes directly from it (e.g. `record_translation.name`). - -`map_legacy_fields_to_block_content` takes `(record, record_translation)` — the first argument is your record (for any non-translated data) and the second is the source translation being processed. Everything else — `presenter`, `configurable_document_type`, and the `ignore_*` methods — works identically to the editionable recipe. - -The steps from there are the same as above. To preview: - -```ruby -StandardEditionMigrator.new(scope: TopicalEvent.all).preview -``` - -To migrate a handful locally first: - -```ruby -StandardEditionMigrator.new(scope: TopicalEvent.first(5)).migrate!(compare_payloads: true, republish: false) -``` - -Or to call the job directly for a single record: - -```ruby -StandardEditionMigratorJob.new.perform(topical_event.id, { "model_class" => "TopicalEvent", "republish" => false, "compare_payloads" => true }) -``` -