Ensure deleted assets are marked as live #9828
Merged
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
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
2 times, most recently
from
9cc2978 to
e384346
Compare
ryanb-gds
approved these changes
Jan 17, 2025
There was a problem hiding this comment.
Looks good to me, a few comments notwithstanding. I would like a review from @ChrisBAshton on this before we merge it just to see if he likes the direction of travel. Personally though I feel having service listeners and jobs that respond to particular changes in edition state is making things much easier to understand. The old job that tried to work out the changes it needs to make to Asset Manager from the state of the edition was very confusing.
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
2 times, most recently
from
d0155f1 to
806798e
Compare
ChrisBAshton
approved these changes
Jan 17, 2025
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
806798e to
81a13dd
Compare
These tests now explicitly cover how deletion works, namely that, in the case of already published editions, an attachment deletion on a new draft will only ever be sent to Asset Manager at the point of publish. Changes: - Add test to cover replaced assets - to capture a known bug where attachments that are replaced and then deleted, cause the original replaced attachment's assets to remain live, rather than redirect to their replacement. - Fix existing test so that it triggers the edition service listener when publishing.
We want to separate out the catch-all logic we currently have around asset updates. The `AttachmentUpdater` service listener was listening on all possible events, making it hard to understand which specific updates need to happen in which case. Additionally, in the `MetadataWorker` that it enqueues, the update and delete logic fired together. These changes are being made in the context of a bug that causes assets to remain live when a new draft attachment gets replaced and deleted. The issue was that the `deleted?` method on the replaced `AttachmentData` evaluated to true when attempting to update the asset to live after deleting the attachment. By separating the publish logic we are confident in removing the `deleted?` on update. We also removed the deletion call from the attachment controller's destroy method, so that the only delete calls to Asset Manager now happen from the publish listener, at the point of publish.
We want to separate out the catch-all logic we currently have around asset updates. In the `MetadataWorker` the update and delete logic were firing together, making it difficult to understand which specific updates, if any, we need when deleting. Additionally, the same updates would fire on attachment deletion, edition publish and edition discard. We have now made it clear (previous commits) that an attachment deletion should only be sent to Asset Manager at the point of publish. This commit deals with the remaining logic around discard. We used to call the `AttachmentDeleter` from inside the `MetadataWorker`, which suggested updates are being made at discard time. We have now extracted what needs to happen on edition discard into a service listener, making it clear that no updates are firing, only asset deletion.
ryanb-gds
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
e7be563 to
860585e
Compare
This log tracked how many times we attempted to update a deleted asset, with the assumption that that is fault in the system. We now know that we actually need to update deleted draft assets for replacements to work, therefore removing the logging.
ryanb-gds
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
2 times, most recently
from
79317e7 to
dae69db
Compare
…blishing and include parent document urls We want to ensure that the draft value for each asset is set to false when the attachable is published, but Asset Manager does not support updates to assets that are already deleted and not drafts. There's also no real need to update any asset that is not a draft in Asset Manager when publishing. The only time an update might be required is after editing a slug in Whitehall, but that should be handled following the slug editing operation (however it isn't currently). Note that we may still need to delete assets that are not drafts, so we always attempt to delete any assets where attachment_data.deleted? is true. The only way we can think of identifying which attachment data objects need publishing is to check if the attachment data has a single publicly visible attachable, which would imply that the attachment data was created during the editing of the just published edition. The previous test was inaccurate in its setup in that it assumed the PublishAttachmentAssetJob would operate with a draft attachable, but in reality it is only enqueued after the state of the attachable has been set to published in the database. We also include the parent document URL for every asset, to ensure that updating a deleted asset for an attachable that has just been published sets a public Asset Manager URL instead of the draft-origin URL. Asset Manager validates that non-draft assets do not have a draft-origin URL.
ryanb-gds
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
dae69db to
e1e6724
Compare
…outcome attachment assets Until now the delete all attachments methods for these edition types was not capturing the outcome attachments. We fix this by ensuring we destroy attachments for all of the attachables associated with the edition.
ryanb-gds
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
d166657 to
77a59a5
Compare
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
2 times, most recently
from
db17a64 to
d06d09b
Compare
Previously, when deleting a policy group attachment, we called the `AttachmentUpdater` - this enqueued the `MetadataWorker`, which dealt with both deletion and updates. We have since separated the update and delete logic for editions. We are now explicit about the fact that the delete request only gets sent to Asset Manager on publish. This logic is captured in the `AttachmentAssetDeleter`. Nonetheless, since Policy Groups are not editionable, they do not use that workflow - their attachments go live upon creation and should be removed from live immediately upon being destroyed. This commit ensures that assets belonging to a policy group's deleted attachments are deleted from Asset Manager on update.
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
d06d09b to
ab930d7
Compare
lauraghiorghisor-tw
deleted the
ensure-deleted-assets-are-marked-as-live
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.
We want to separate out the catch-all logic we currently have around asset updates. The
AttachmentUpdaterservice listener was listening on all possible events, making it hard to understand which specific updates need to happen in which case. Additionally, in theMetadataWorkerthat it enqueued, the update and delete logic fired together. For deletion, similar updates used to fire on attachment deletion, edition publish and edition discard.We now understand that, due to the limitations of how we can represent draft/live states in Asset Manager, we can only delete a pre-existing live asset at the point of publish. Any earlier, and the live edition's attachment would become unavailable. Thus, we decided to separate out the Asset Manager update and delete calls that run at edition publication time.
These changes are being made in the context of a bug that causes assets to remain live when a previously published attachment gets replaced and deleted. The issue was that the
deleted?method on the replacementAttachmentDataevaluated to true when attempting to update the deleted asset to live (draft: false), causing the update to be skipped. By separating the publish logic we are confident in removing thedeleted?check on update.AttachmentsController'sdestroymethod, which only ever succeeded for first draft editions before.MetadataWorker- deletion is now only triggered from the publish listener, at the point of publish.MetadataWorker), we added jobs inside the listeners.In addition to the edition workflow changes, we've had to make sure asset deletion also works for Policy Groups, as they are not editionable.
Trello