From 1b1297ec4ff23758d6feb3eaea596ef496fa9f24 Mon Sep 17 00:00:00 2001 From: Minno Dang Date: Wed, 4 Dec 2024 15:51:17 +0000 Subject: [PATCH] Fix invalid state and reset parent document url when updating assets - Add a clause to remove parent document url when fixing draft states when deleting superseded assets Some assets are incorrectly staying in the draft state. But when we try and fix the draft state, it cannot be linking to a draft parent_document_url. We are removing the parent document url link since it is difficult to figure out what the parent_document_url should be, and is largely irrelevant for a superseded asset. - Fix correct state for assets that have gone in a weird invalid state over the years. "deleted" should no longer be a valid state. patching any data that we come across that has a wrong state. - Log when we've patched an invalid state or parent document URL --- lib/tasks/assets.rake | 19 +++++++-- spec/lib/tasks/assets_spec.rb | 72 ++++++++++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/lib/tasks/assets.rake b/lib/tasks/assets.rake index 93b571d5..5ab53f6c 100644 --- a/lib/tasks/assets.rake +++ b/lib/tasks/assets.rake @@ -80,7 +80,7 @@ namespace :assets do if is_replacement && replacement_asset.nil? && original_asset.draft? begin delete_and_update_draft(original_asset) - puts "Asset ID: #{original_asset_id} - is a replacement. Asset deleted and updated to false." + puts "Asset ID: #{original_asset_id} - OK. Asset is a replacement. Asset deleted and updated to false." rescue StandardError puts "Asset ID: #{original_asset_id} - ERROR. Asset failed to save. Error: #{original_asset.errors.full_messages}." end @@ -93,7 +93,7 @@ namespace :assets do end begin - original_asset.destroy! + delete_and_update_draft(original_asset, should_update_draft: false) puts "Asset ID: #{original_asset_id} - OK. Asset has been deleted." rescue StandardError puts "Asset ID: #{original_asset_id} - ERROR. Asset failed to save. Error: #{original_asset.errors.full_messages}." @@ -103,8 +103,21 @@ namespace :assets do end end -def delete_and_update_draft(asset) +def delete_and_update_draft(asset, should_update_draft: true) + if asset.state.to_s == "deleted" + asset.state = "uploaded" + puts "Patched state: #{asset.id}" + end + + if asset.parent_document_url&.include?("draft-origin") + asset.parent_document_url = nil + puts "Patched Parent URL: #{asset.id}" + end + asset.destroy! unless asset.deleted? + + return unless should_update_draft + asset.draft = false asset.save! end diff --git a/spec/lib/tasks/assets_spec.rb b/spec/lib/tasks/assets_spec.rb index 956399cd..b5c8aae6 100644 --- a/spec/lib/tasks/assets_spec.rb +++ b/spec/lib/tasks/assets_spec.rb @@ -66,16 +66,47 @@ expect(replacement.reload.draft).to be false end - it "deletes the asset replacement and updates the draft state if the replacement is not deleted" do - replacement = FactoryBot.create(:asset, draft: true, deleted_at: nil, replacement_id: nil) + it "deletes the asset replacement, updates the draft state, and turn draft parent_url to nil if the replacement is not deleted" do + replacement = FactoryBot.create(:asset, draft: true, deleted_at: nil, replacement_id: nil, parent_document_url: "https://draft-origin.publishing.service.gov.uk/example") FactoryBot.create(:asset, id: asset_id, replacement_id: replacement.id) expected_output = <<~OUTPUT + Patched Parent URL: #{replacement.id} Asset ID: #{asset_id} - OK. Draft replacement #{replacement.id} deleted and updated to false. OUTPUT expect { task.invoke(filepath) }.to output(expected_output).to_stdout expect(replacement.reload.deleted_at).not_to be_nil expect(replacement.reload.draft).to be false + expect(replacement.reload.parent_document_url).to be_nil + end + + it "deletes the asset replacement, updates the draft state, and leave live parent_url alone if the replacement is not deleted" do + replacement = FactoryBot.create(:asset, draft: true, deleted_at: nil, replacement_id: nil, parent_document_url: nil) + FactoryBot.create(:asset, id: asset_id, replacement_id: replacement.id) + + expected_output = <<~OUTPUT + Asset ID: #{asset_id} - OK. Draft replacement #{replacement.id} deleted and updated to false. + OUTPUT + expect { task.invoke(filepath) }.to output(expected_output).to_stdout + expect(replacement.reload.deleted_at).not_to be_nil + expect(replacement.reload.draft).to be false + expect(replacement.reload.parent_document_url).to be_nil + end + + it "deletes the asset replacement and fixes invalid upload state if the replacement is not deleted" do + replacement = FactoryBot.create(:asset, draft: true, deleted_at: nil) + FactoryBot.create(:asset, id: asset_id, replacement_id: replacement.id) + replacement.state = "deleted" + replacement.save!(validate: false) + + expected_output = <<~OUTPUT + Patched state: #{replacement.id} + Asset ID: #{asset_id} - OK. Draft replacement #{replacement.id} deleted and updated to false. + OUTPUT + expect { task.invoke(filepath) }.to output(expected_output).to_stdout + expect(replacement.reload.deleted_at).not_to be_nil + expect(replacement.reload.draft).to be false + expect(replacement.reload.state).to eq "uploaded" end it "only updates the draft state if the asset is already deleted (asset is a replacement)" do @@ -83,23 +114,54 @@ FactoryBot.create(:asset, replacement_id: asset.id) expected_output = <<~OUTPUT - Asset ID: #{asset_id} - is a replacement. Asset deleted and updated to false. + Asset ID: #{asset_id} - OK. Asset is a replacement. Asset deleted and updated to false. + OUTPUT + expect { task.invoke(filepath) }.to output(expected_output).to_stdout + expect(asset.reload.deleted_at).not_to be_nil + expect(asset.reload.draft).to be false + end + + it "deletes the asset, updates the draft state, and turn draft parent_url into nil if the asset is not deleted (asset is a replacement)" do + asset = FactoryBot.create(:asset, id: asset_id, draft: true, deleted_at: nil, parent_document_url: "https://draft-origin.publishing.service.gov.uk/example") + FactoryBot.create(:asset, replacement_id: asset.id) + + expected_output = <<~OUTPUT + Patched Parent URL: #{asset_id} + Asset ID: #{asset_id} - OK. Asset is a replacement. Asset deleted and updated to false. + OUTPUT + expect { task.invoke(filepath) }.to output(expected_output).to_stdout + expect(asset.reload.deleted_at).not_to be_nil + expect(asset.reload.draft).to be false + expect(asset.reload.parent_document_url).to be_nil + end + + it "deletes the asset, updates the draft state, and leaves live parent_url alone if the asset is not deleted (asset is a replacement)" do + asset = FactoryBot.create(:asset, id: asset_id, draft: true, deleted_at: nil, parent_document_url: nil) + FactoryBot.create(:asset, replacement_id: asset.id) + + expected_output = <<~OUTPUT + Asset ID: #{asset_id} - OK. Asset is a replacement. Asset deleted and updated to false. OUTPUT expect { task.invoke(filepath) }.to output(expected_output).to_stdout expect(asset.reload.deleted_at).not_to be_nil expect(asset.reload.draft).to be false + expect(asset.reload.parent_document_url).to be_nil end - it "deletes the asset and updates the draft state if the asset is not deleted (asset is a replacement)" do + it "deletes the asset and fixes invalid upload state if the asset is not deleted (asset is a replacement)" do asset = FactoryBot.create(:asset, id: asset_id, draft: true, deleted_at: nil) FactoryBot.create(:asset, replacement_id: asset.id) + asset.state = "deleted" + asset.save!(validate: false) expected_output = <<~OUTPUT - Asset ID: #{asset_id} - is a replacement. Asset deleted and updated to false. + Patched state: #{asset_id} + Asset ID: #{asset_id} - OK. Asset is a replacement. Asset deleted and updated to false. OUTPUT expect { task.invoke(filepath) }.to output(expected_output).to_stdout expect(asset.reload.deleted_at).not_to be_nil expect(asset.reload.draft).to be false + expect(asset.reload.state).to eq "uploaded" end it "skips the asset if asset is itself redirected (and not replaced by draft or itself a replacement)" do