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

Add temporary soft delete bulk rake task #1543

Merged

Conversation

lauraghiorghisor-tw
Copy link
Contributor

@lauraghiorghisor-tw lauraghiorghisor-tw commented Nov 20, 2024

Context

Multiple Zendesk tickets related to whitehall assets being live when they should not be, triggered an analysis of the problematic data - see PR. We have now investigated asset deletion states further, and we've produced a set of assets that should be deleted, according to whitehall - see extraction rake task.

This PR is just for the data patch. Future work will deal with fixing the underlying issues in the code.

The rake task

This PR defines a rake task that soft deletes each asset ID passed through in a csv file. Since Attachments and AttachmentDatas are already dealt with on the whitehall side, we can just soft delete the corresponding assets in asset-manager.

We discovered a set of assets that have an invalid deleted state field, most likely from older code logic. These are giving a "State is invalid" validation error. We're resetting them to "uploaded" state in order to pass validation.

NB: There are around 600 assets in this invalid state, but they were not flagged up for deletion on our side. Work to fix these is captured in this card here.

The data extraction

We now know that there are a few broken flows in whitehall that are continuously producing incorrect asset states, specifically unintentionally live assets. The two main ones we've identified are:

  • An attachment replacement followed by a delete on a draft edition (off a previously published one) - fails to set draft to false on publish for the deleted asset, causing the previous live asset to remain live. Asset manager does not redirect a live asset to draft. This is intentional.
  • A replacement done on a new draft off a published edition, without saving the draft first - causes the replacement to not be set, thus leaving the original asset live.

We've extracted all the AttachmentDatas where the deleted? method is true and relevant database fields to help guide our understanding of the data. The fact that the deleted? method is true is a consequence of a previous discard event and has no bearing on the bugs. If the reported asset has a missing link or a replacement that is in draft, it is likely to have been the result of one of the known bugs.

We had a choice to "fix" the data in more meaningful ways, but since whitehall has a deletion set, i.e. the deleted? method is true, we chose to mass delete these assets to keep the data "in sync". We've ensured that the data is not serving any live editions and it is not a risk to delete it.

Splitting the data extraction based on the deleted? method evaluation was a way to reason about deletion state in whitehall, since there is no obvious way in which that state is represented in the database. Whilst a soft delete is applied to the Attachment model, it is the AttachmentData model that actually manages the Assets. It deduces the deletion state based on the Attachment state and the Edition visibility. This logic is captured in this deleted? method, which is evaluated whenever we try to update asset-manager. It must be true for a delete value to be sent, and false for the other updates to be sent through - replacement, draft, redirect_url.

NB: The current data extracted to be fixed is based on an older integration dump (end of October 2024), meaning there may be wrong Assets that got created between then and the time an upcoming code fix goes live. Nonetheless, this data patch should eliminate a great deal of risk from past data.

Next steps

The other half of the data, where AttachmentData's deleted? evaluates to false will be dealt with next. We will attempt a more comprehensive data "fix" in that case, rather than deletion. We'll attempt to fix the logic that causes the ongoing issues in an upcoming card.

Fixing this data removes a big part of the risk, as does fixing the code. Nonetheless, there are batches of assets we've looked at, whose states we cannot account for. We've started ideating around some remodelling work of the attachments/assets workflow, to make it simpler, easier to understand and maintain, and bug-free.

Links

Trello
Original Platform card
Assets investigation document

Co-authored-by @minhngocd

@lauraghiorghisor-tw lauraghiorghisor-tw force-pushed the add-temporary-rake-task-for-asset-deletion branch from 0bdf3c5 to 358bfaa Compare November 20, 2024 09:55
This is part of running a clean-up on assets that should be deleted, according to whitehall.
We discovered a set of assets that have an invalid deleted state, so we're resetting them to "uploaded".
@lauraghiorghisor-tw lauraghiorghisor-tw force-pushed the add-temporary-rake-task-for-asset-deletion branch from 358bfaa to 4359340 Compare November 20, 2024 10:34
@lauraghiorghisor-tw lauraghiorghisor-tw merged commit 709a544 into main Nov 20, 2024
9 checks passed
@lauraghiorghisor-tw lauraghiorghisor-tw deleted the add-temporary-rake-task-for-asset-deletion branch November 20, 2024 11:14
@lauraghiorghisor-tw lauraghiorghisor-tw changed the title Add temproary soft delete bulk rake task Add temporary soft delete bulk rake task Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants