Skip to content

Commit

Permalink
Delete Asset Manager assets when corresponding attachment is deleted
Browse files Browse the repository at this point in the history
This wires up an instance of ServiceListeners::AttachmentDeleter so that
the Asset Manager assets corresponding to an AttachmentData are deleted
via a Sidekiq worker.

This will mark the relevant assets as deleted in Asset Manager. While
there is functionality in the Asset Manager API to restore a deleted
asset, there doesn't appear to be any such functionality in Whitehall.

Note that the Asset Manager assets will only be deleted if no other
non-deleted Attachment instances are still referencing the
AttachmentData, i.e. where AttachmentData#deleted? is `true`.

Also note that it looks as if when a (consultation) Response or a
PolicyGroup is destroyed, the associated attachments are not deleted.
While it's not obvious that it's possible to delete a Response via the
user interface, it does appear to be possible to delete a PolicyGroup.

While it doesn't feel appropriate to address this here, I'd recommend
that for consistency and completeness, both of these problems should be
fixed by calling Attachable#delete_all_attachments from a before/after
destroy callback on Response and PolicyGroup. This would mean a
'destroy' event would be published to the attachment notifier for each
attachment and the relevant assets would be deleted in Asset Manager.
  • Loading branch information
floehopper committed Mar 13, 2018
1 parent 32588e3 commit dfc67a7
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 0 deletions.
5 changes: 5 additions & 0 deletions config/initializers/attachment_notifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,9 @@
.update!
end
end
notifier.subscribe('destroy') do |_event, attachment|
ServiceListeners::AttachmentDeleter
.new(attachment.attachment_data)
.delete!
end
end
10 changes: 10 additions & 0 deletions test/integration/attachment_deletion_integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
get @attachment_url
assert_response :not_found
end

it 'deletes corresponding asset(s) in Asset Manager' do
Services.asset_manager.expects(:delete_asset).with(asset_id)
AssetManagerAttachmentDeleteWorker.drain
end
end

context 'when draft document is discarded' do
Expand All @@ -58,6 +63,11 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
get @attachment_url
assert_response :not_found
end

it 'deletes corresponding asset(s) in Asset Manager' do
Services.asset_manager.expects(:delete_asset).with(asset_id)
AssetManagerAttachmentDeleteWorker.drain
end
end
end

Expand Down

0 comments on commit dfc67a7

Please sign in to comment.