Skip to content

Commit

Permalink
Added integration test for attachment deletion
Browse files Browse the repository at this point in the history
This demonstrates the current behaviour when an attachment is deleted
from a draft edition which has never been published.

* `Admin::AttachmentsController#destroy` calls `Attachment#destroy`.

* `Attachment#destroy` overrides the standard
`ActiveRecord::Persistence#destroy` method so that it only marks the
attachment as deleted, but it still runs `:destroy` callbacks [1].

* `FileAttachment`, a subclass of `Attachment` has an `after_destroy`
callback which will destroy the associated `AttachmentData` only if
no other non-deleted attachments are using it [2].

* In this case, because there's only a single edition of the document,
there is only one instance of `Attachment` for the relevant
`AttachmentData` and thus the latter is destroyed.

* This in turn fires an `after_commit, on: :destroy` callback which is
added by CarrierWave which eventually results in a call to
`Whitehall::AssetManagerStorage::File#delete` which calls
`AssetManagerDeleteAssetWorker.perform_async` to queue a job to delete
the corresponding asset in Asset Manager.

* The fact that the `AttachmentData` has actually been deleted results
in an `ActiveRecord::RecordNotFound` exception being raised in
`AttachmentsController#attachment_data`. In production this will result
in a `404 Not Found` response [3].

* In the case where an `AttachmentData` is associated with more than one
edition, the deletion of an `Attachment` will only result in the
`Attachment` being marked as deleted; the `AttachmentData` will not be
destroyed`. However, requests to the attachment URL will still result in
a `404 Not Found`, because `AttachmentVisibility#edition_ids` [4] does
not consider deleted attachments and thus
`AttachmentVisibility#visible?` will always return `false` and this line
[5] in `AttachmentsController#fail` will be invoked.

[1]:
https://github.com/alphagov/whitehall/blob/32bb13cf3ae3ae9ca71fa14bd4c620c631660419/app/models/attachment.rb#L130-L132
[2]:
https://github.com/alphagov/whitehall/blob/32bb13cf3ae3ae9ca71fa14bd4c620c631660419/app/models/file_attachment.rb#L33-L38
[3]:
http://guides.rubyonrails.org/v5.0.6/action_controller_overview.html#rescue
[4]:
https://github.com/alphagov/whitehall/blob/32bb13cf3ae3ae9ca71fa14bd4c620c631660419/app/models/attachment_visibility.rb#L116-L118
[5]:
https://github.com/alphagov/whitehall/blob/32bb13cf3ae3ae9ca71fa14bd4c620c631660419/app/controllers/attachments_controller.rb#L31
  • Loading branch information
floehopper committed Mar 3, 2018
1 parent 32bb13c commit 7efe624
Showing 1 changed file with 82 additions and 0 deletions.
82 changes: 82 additions & 0 deletions test/integration/attachment_deletion_integration_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
require 'test_helper'
require 'capybara/rails'

class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
extend Minitest::Spec::DSL
include Capybara::DSL
include Rails.application.routes.url_helpers

context 'given a draft document with a file attachment' do
let(:managing_editor) { create(:managing_editor) }

let(:filename) { 'sample.docx' }
let(:file) { File.open(path_to_attachment(filename)) }
let(:attachment) { build(:file_attachment, attachable: edition, file: file) }
let(:asset_id) { 'asset-id' }

let(:replacement_filename) { 'sample.rtf' }
let(:replacement_asset_id) { 'replacement-asset-id' }

let(:edition) { create(:news_article) }

before do
login_as(managing_editor)
edition.attachments << attachment
setup_publishing_api_for(edition)
stub_whitehall_asset(filename, id: asset_id)
VirusScanHelpers.simulate_virus_scan

visit admin_news_article_path(edition)
click_link 'Modify attachments'
@attachment_url = find('.existing-attachments a', text: filename)[:href]
end

context 'when attachment is deleted' do
before do
visit admin_news_article_path(edition)
click_link 'Modify attachments'
within '.existing-attachments' do
click_link 'Delete'
end
end

it 'responds with 404 Not Found for attachment URL' do
logout

assert_raises(ActiveRecord::RecordNotFound) do
get @attachment_url
end
end

it 'deletes attachment in Asset Manager' do
Services.asset_manager.expects(:delete_asset)
.with(asset_id)
AssetManagerDeleteAssetWorker.drain
end
end
end

private

def ends_with(expected)
->(actual) { actual.end_with?(expected) }
end

def setup_publishing_api_for(edition)
publishing_api_has_links(
content_id: edition.document.content_id,
links: {}
)
end

def path_to_attachment(filename)
fixture_path.join(filename)
end

def stub_whitehall_asset(filename, attributes = {})
url_id = "http://asset-manager/assets/#{attributes[:id]}"
Services.asset_manager.stubs(:whitehall_asset)
.with(&ends_with(filename))
.returns(attributes.merge(id: url_id).stringify_keys)
end
end

0 comments on commit 7efe624

Please sign in to comment.