Skip to content

Commit

Permalink
Fix invalid state and reset parent document url when updating assets
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
minhngocd authored and lauraghiorghisor-tw committed Dec 4, 2024
1 parent 387eb35 commit 58c4006
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 3 deletions.
3 changes: 3 additions & 0 deletions lib/tasks/assets.rake
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ namespace :assets do
end

def delete_and_update_draft asset

Check failure on line 106 in lib/tasks/assets.rake

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Style/MethodDefParentheses: Use def with parentheses when there are parameters. (https://rubystyle.guide#method-parens)
(asset.state = "uploaded"; puts "Patched state: #{asset.id}") if asset.state.to_s == "deleted"

Check failure on line 107 in lib/tasks/assets.rake

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Style/Semicolon: Do not use semicolons to terminate expressions. (https://rubystyle.guide#no-semicolon)
asset.destroy! unless asset.deleted?

(asset.parent_document_url = nil; puts "Patched Parent URL: #{asset.id}") if asset.parent_document_url&.include?("draft-origin")

Check failure on line 110 in lib/tasks/assets.rake

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Style/Semicolon: Do not use semicolons to terminate expressions. (https://rubystyle.guide#no-semicolon)
asset.draft = false
asset.save!
end
Expand Down
68 changes: 65 additions & 3 deletions spec/lib/tasks/assets_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 eq nil

Check failure on line 93 in spec/lib/tasks/assets_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

RSpec/BeEq: Prefer `be` over `eq`. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/BeEq)
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)

Check failure on line 100 in spec/lib/tasks/assets_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Rails/SaveBang: Use `save!` instead of `save` if the return value is not checked. (https://rails.rubystyle.guide#save-bang)

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
Expand All @@ -90,16 +121,47 @@
expect(asset.reload.draft).to be false
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, 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} - 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} - 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 eq nil

Check failure on line 148 in spec/lib/tasks/assets_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

RSpec/BeEq: Prefer `be` over `eq`. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/BeEq)
end

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)

Check failure on line 155 in spec/lib/tasks/assets_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Rails/SaveBang: Use `save!` instead of `save` if the return value is not checked. (https://rails.rubystyle.guide#save-bang)

expected_output = <<~OUTPUT
Patched state: #{asset_id}
Asset ID: #{asset_id} - 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
Expand Down

0 comments on commit 58c4006

Please sign in to comment.