Deleting a parent Whitehall document should delete underlying assets in Asset Manager

[floehopper]

I think this should focus on reproducing existing Whitehall behaviour, rather than adding missing behaviour or fixing bugs. The only exception to this might be to support potential behaviour provided by models which is not currently available via the UI.

Here are some notes about the current situation:

Editions

• The Admin::EditionsController has a #destroy action, although it's not obvious that this is accessible via the UI.
• This action uses an instance of EditionDeleter which has a #fire_transition! method which, in turn, calls Attachable#delete_all_attachments which seems to mark the attachments as deleted.
• I haven't dug very far into whether this deleted flag is actually used to decide whether an attachment should be visible, but surprisingly there doesn't appear to be a default scope on Attachment.
• Also I can't see any obvious callbacks which delete (or perform any similar action on) the corresponding AttachmentData instance which is what would cause the underlying CarrierWave file to be deleted.

PolicyGroups

• Admin::PolicyGroupsController has a #destroy action, although I haven't checked whether this is accessible via the UI.
• The PolicyGroup model has no custom attachment-related behaviour other than that added by Attachable.
• Attachable doesn't appear to have any callbacks which would remove attachments or their corresponding attachment datas.

(Consultation) Responses

• Admin::ResponsesController has no #destroy action.
• Response nor its subclasses have any significant custom attachment-specific behaviour other than that added by Attachable.

[floehopper]

Further investigation:

1. Attachable#attachments does not include deleted attachments. I think this probably covers all/most of the places where attachments are listed, e.g. on a document page.
2. AttachmentVisibility#edition_ids does not consider deleted attachments and thus I think AttachmentVisibility#visible? will always return false for them. I think this is why a request for a deleted Attachment will result in a 404 Not Found as per this line in AttachmentsController#fail.

I've also verified the above behaviour by deleting an attachment in integration.

Since Attachable is used for all entities which can have associated attachments, I'm confident that point 1 applies to attachments on all entities, i.e. including consultation responses and policy groups.

I'm less confident that point 2 applies to non-edition entities. TODO: Investigate this.

I think all of this implies that when an attachment is deleted in Whitehall, we should "delete" the asset (and any versions) from Asset Manager.

[floehopper]

There doesn't seem to be any way to un-delete a Whitehall attachment, so I don't think we need to worry about that for now. Especially as Asset Manager already supports restoring of deleted assets if we ever need it.

[floehopper]

• Attachment#destroy overrides the standard ActiveRecord::Persistence#destroy method so that it only marks the attachment as deleted, but it still runs :destroy callbacks.
• 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.

So I think we should add an after_destroy (or possibly a before_destroy) callback on AttachmentData which triggers the deletion of the corresponding asset in Asset Manager.

[floehopper]

So I think we should add an after_destroy (or possibly a before_destroy) callback on AttachmentData which triggers the deletion of the corresponding asset in Asset Manager.

When I wrote the above, I had forgotten about the after_commit, on: :destroy callback 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. Thus in the scenario where all attachments referencing a given AttachmentData have been marked as deleted, the underlying file and the Asset Manager asset will be deleted. I've explained this in more detail in this commit.

However, I also noticed some inconsistent behaviour when a draft edition is discarded. In this case all its attachments are marked as deleted, but because Attachment#destroy is not called, the on_destroy callback in FileAttachment is not called and thus the underlying files/assets will never be deleted. This isn't as big a deal as it sounds, because requests for the deleted attachments will still result in a 404 Not Found, but for different reasons. I've explained this in more detail in this commit.

I've spiked on a fix to remove this inconsistency in this commit. All the commits mentioned in this comment are on this branch.

[floehopper]

Thinking about this a bit more, I think we do need to fix the above inconsistency if we want the behaviour to remain the same when we switch over to serve attachments from Asset Manager. This is because requests for attachments will no longer go via AttachmentsController#show and thus we won't be able to rely on the behaviour of AttachmentVisibility#visible? and AttachmentsController#fail to respond with a 404 Not Found. Explicitly removing the asset from Asset Manager will result in a 404 Not Found.

[floehopper]

Note that I've belatedly realised that this issue is specifically about what happens when you delete an attachments parent document; not what happens when you delete the attachment itself. However, as noted in some of the above comments, we have work to do on this front and hence I've captured this in a new issue: #509.

[floehopper]

I've captured the possible issue about deleting a policy group not deleting its associated attachments in this issue.

[floehopper]

• The behaviour when deleting an Edition seems to be correct and has been extended to work with Asset Manager in Delete assets in Asset Manager when attachments are deleted whitehall#3853
• It doesn't currently seem possible to delete a (consultation) Response
• I'm unsure whether the behaviour when deleting a PolicyGroup is correct, but I've captured the issue in Deleting a PolicyGroup does not delete its associated attachments whitehall#3858.

Closing.