Skip to content

Commit

Permalink
Merge pull request #3853 from alphagov/delete-assets-in-asset-manager…
Browse files Browse the repository at this point in the history
…-when-attachments-are-deleted

Delete assets in Asset Manager when attachments are deleted
  • Loading branch information
floehopper authored Mar 13, 2018
2 parents bda339b + dfc67a7 commit 071b9de
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 0 deletions.
5 changes: 5 additions & 0 deletions app/models/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class Attachment < ApplicationRecord
before_save :set_ordering, if: -> { ordering.blank? }
before_save :nilify_locale_if_blank
before_save :prevent_saving_of_abstract_base_class
after_destroy :publish_destroy_event

VALID_COMMAND_PAPER_NUMBER_PREFIXES = ['C.', 'Cd.', 'Cmd.', 'Cmnd.', 'Cm.'].freeze

Expand Down Expand Up @@ -149,4 +150,8 @@ def prevent_saving_of_abstract_base_class
raise RuntimeError, "Attempted to save abstract base class Attachment"
end
end

def publish_destroy_event
Whitehall.attachment_notifier.publish('destroy', self)
end
end
15 changes: 15 additions & 0 deletions app/services/service_listeners/attachment_deleter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module ServiceListeners
class AttachmentDeleter
attr_reader :attachment_data

def initialize(attachment_data)
@attachment_data = attachment_data
end

def delete!
return unless attachment_data.present?

AssetManagerAttachmentDeleteWorker.perform_async(attachment_data.id)
end
end
end
16 changes: 16 additions & 0 deletions app/workers/asset_manager_attachment_delete_worker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class AssetManagerAttachmentDeleteWorker < WorkerBase
def perform(attachment_data_id)
attachment_data = AttachmentData.find_by(id: attachment_data_id)
return unless attachment_data.present?
return unless attachment_data.deleted?

delete_asset_for(attachment_data.file)
if attachment_data.pdf?
delete_asset_for(attachment_data.file.thumbnail)
end
end

def delete_asset_for(uploader)
AssetManagerDeleteAssetWorker.new.perform(uploader.asset_manager_path)
end
end
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
11 changes: 11 additions & 0 deletions test/unit/attachment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
class AttachmentTest < ActiveSupport::TestCase
include ActionDispatch::TestProcess

setup do
Whitehall.attachment_notifier.stubs(:publish)
end

test 'should be invalid without an attachable' do
refute build(:file_attachment, attachable: nil).valid?
end
Expand Down Expand Up @@ -224,4 +228,11 @@ class AttachmentTest < ActiveSupport::TestCase
attachment.destroy
assert attachment.deleted?
end

test 'destroy publishes destroy event to attachment notifier' do
attachment = create(:file_attachment)
Whitehall.attachment_notifier.expects(:publish).with('destroy', attachment)

attachment.destroy
end
end
30 changes: 30 additions & 0 deletions test/unit/services/service_listeners/attachment_deleter_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
require 'test_helper'

module ServiceListeners
class AttachmentDeleterTest < ActiveSupport::TestCase
extend Minitest::Spec::DSL

let(:deleter) { AttachmentDeleter.new(attachment_data) }

context 'when attachment data is not nil' do
let(:id) { 123 }
let(:attachment_data) { AttachmentData.new(id: id) }

it 'deletes related assets in Asset Manager' do
AssetManagerAttachmentDeleteWorker.expects(:perform_async).with(id)

deleter.delete!
end
end

context 'when attachment data is nil' do
let(:attachment_data) { nil }

it 'does not delete any assets in Asset Manager' do
AssetManagerAttachmentDeleteWorker.expects(:perform_async).never

deleter.delete!
end
end
end
end
75 changes: 75 additions & 0 deletions test/unit/workers/asset_manager_attachment_delete_worker_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
require 'test_helper'

class AssetManagerAttachmentDeleteWorkerTest < ActiveSupport::TestCase
extend Minitest::Spec::DSL

let(:worker) { AssetManagerAttachmentDeleteWorker.new }
let(:attachment_data) { nil }
let(:delete_worker) { mock('delete-worker') }

before do
AttachmentData.stubs(:find_by).with(id: id).returns(attachment_data)
AssetManagerDeleteAssetWorker.stubs(:new).returns(delete_worker)
end

context 'when attachment data does not exist' do
let(:id) { 'no-such-id' }

it 'does not delete any assets from Asset Manager' do
delete_worker.expects(:perform).never

worker.perform(id)
end
end

context 'when attachment data exists' do
let(:attachment_data) { create(:attachment_data, file: file) }
let(:id) { attachment_data.id }

before do
attachment_data.stubs(:deleted?).returns(deleted)
end

context 'when attachment data is not a PDF' do
let(:file) { File.open(fixture_path.join('sample.rtf')) }

context 'and attachment data is deleted' do
let(:deleted) { true }

it 'deletes corresponding asset in Asset Manager' do
delete_worker.expects(:perform)
.with(attachment_data.file.asset_manager_path)

worker.perform(id)
end
end

context 'and attachment data is not deleted' do
let(:deleted) { false }

it 'does not delete any assets from Asset Manager' do
delete_worker.expects(:perform).never

assert AssetManagerDeleteAssetWorker.jobs.empty?
end
end
end

context 'when attachment data is a PDF' do
let(:file) { File.open(fixture_path.join('simple.pdf')) }

context 'and attachment data is deleted' do
let(:deleted) { true }

it 'deletes attachment & thumbnail asset in Asset Manager' do
delete_worker.expects(:perform)
.with(attachment_data.file.asset_manager_path)
delete_worker.expects(:perform)
.with(attachment_data.file.thumbnail.asset_manager_path)

worker.perform(id)
end
end
end
end
end

0 comments on commit 071b9de

Please sign in to comment.