Delete assets in Asset Manager when attachments are deleted #3853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I want to delete the Asset Manager assets corresponding to Whitehall attachments when they are deleted. This is a first step towards making this possible. I chose to use the attachment notifier rather than the attachment data notifier, because the deleted state that is definitely changing is on the Attachment - the deleted state of the AttachmentData may or may not be changing, depending on the state of other Attachments which reference it.
I'm planning to wire this up to delete assets in Asset Manager for Whitehall attachments which are deleted. Given the ID of an AttachmentData which is considered as deleted, this worker will look up the AttachmentData and mark the corresponding Asset Manager asset as deleted, as well as the asset corresponding to the attachment's thumbnail image if appropriate.
I'm planning to wire this up to delete assets in Asset Manager for Whitehall attachments which are deleted. We've discussed inlining all the attachment-related service listener classes, but I'm going to leave that for now to keep the code more consistent.
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.
floehopper
changed the title
chrislo
approved these changes
Mar 13, 2018
There was a problem hiding this comment.
This looks good to me - good work!
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.
Perhaps we should add an issue against Whitehall for this?
|
Thanks for reviewing.
I think it would be good to capture it somewhere, but I'm not sure the repo issues are monitored by the GOV.UK team. I'll investigate and try to capture the issue somewhere, but in the meantime I'm going to merge this. |
floehopper
deleted the
delete-assets-in-asset-manager-when-attachments-are-deleted
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This wires up an instance of
ServiceListeners::AttachmentDeleterso that the Asset Manager assets corresponding to anAttachmentDataare 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. whereAttachmentData#deleted?istrue.Also note that it looks as if when a (consultation)
Responseor aPolicyGroupis destroyed, the associated attachments are not destroyed. While it's not obvious that it's possible to delete aResponsevia the user interface, it does appear to be possible to delete aPolicyGroup.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_attachmentsfrom a before/after destroy callback onResponseandPolicyGroup. 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.