Ensure deleted assets are marked as live #9828
Open
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
from
81b8453 to
9cc2978
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.
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
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
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.
lauraghiorghisor-tw
force-pushed
the
ensure-deleted-assets-are-marked-as-live
branch
from
806798e to
81a13dd
Compare
The `attachable_url` for an attachment_data` always evaluates to nil when the `attachment_data` deleted? is true. However, there is no reason to include this in the request to asset manager, when updating published assets. The `AssetUpdater` compares the request it's about to send to the current state of Asset Manager, and cancels the request if it matches the state in Asset Manager. When the `parent_document_url` is nil, the comparison is unequal and the request gets made and subsequently fails - because Asset Manager rejects requests to update assets that are both deleted and non-draft.
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.
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 enqueues, 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 new draft attachment gets replaced and deleted. The issue was that the
deleted?method on the replacedAttachmentDataevaluated to true when attempting to update the asset to live after deleting the attachment. By separating the publish logic we are confident in removing thedeleted?check on update.AttachmentsController'sdestroymethod, which only ever fired 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.