Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete assets in Asset Manager when attachments are deleted #3853

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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