on undeletion, mark image as archived so it cannot be immediately rereaped#4700
Conversation
andrew-nowak
added
the
maintenance
andrew-nowak
force-pushed
the
an/archive-on-undelete
branch
from
df33f26 to
d834ea8
Compare
andrew-nowak
force-pushed
the
an/archive-on-undelete
branch
from
d834ea8 to
f3548f4
Compare
andrew-nowak
force-pushed
the
an/archive-on-undelete
branch
2 times, most recently
from
f9819bf to
3fb2dd7
Compare
…the Origin header from the original request As we pass along the authentication cookie used in the original request, we also need to pass along the original Origin header so that the subsequent request belongs takes the same cross-origin status and passes through the CSRF filter
andrew-nowak
force-pushed
the
an/archive-on-undelete
branch
from
3fb2dd7 to
e223517
Compare
twrichards
left a comment
twrichards
left a comment
There was a problem hiding this comment.
nice one @andrew-nowak - I'd always planned to do something like this, but never got round to it I guess (as this was the intent of the soft-deletion phase of reaping).
Do we want to check the reapable eligibility before doing this additional archiving - so that regular undeletes (of non-reaped images) aren't unnecessarily archived.
Also, one notable comment...
| ).catch(() => { | ||
| .then(() => pollUndeleted(imageId)) | ||
| .then(() => { | ||
| ctrl.canUndelete = ctrl.isDeleted = false; |
There was a problem hiding this comment.
should this be
| ctrl.canUndelete = ctrl.isDeleted = false; | |
| ctrl.canUndelete = ctrl.isDeleted === false; |
looks like its doing two assignments 🤔
(appreciate that's how it was before, so would warrant some testing to figure out the original intent of that line)
There was a problem hiding this comment.
Yeah, I'm pretty sure this is a rare intentional double assignment (this assignment happens just after we've undeleted the image, and being angular the isDeleted and canUndelete states aren't derived from other variables, so needs explicitly setting back to false.
I'll split it into two separate assignments to make the intention clearer
Personally I'm happy to take an "undelete" press as an indication that a user is very interested in keeping this image (as they have gone through the trouble of finding the soft-deleted image, which is not exactly easy to do), and it should be added to the archive so that it is never again a candidate for reaping, regardless of whether that was why it was originally soft-deleted. |
gu-prout
Bot
added
Pending-on-cropper
Pending-on-leases
Pending-on-media-api
Pending-on-image-loader
Pending-on-metadata-editor
Pending-on-collections
Pending-on-usage
Pending-on-kahuna
Pending-on-auth
Pending-on-thrall
and removed
Pending-on-cropper
Pending-on-leases
Pending-on-media-api
labels
|
Seen on auth, usage, image-loader, leases, cropper, media-api (merged by @andrew-nowak 8 minutes and 56 seconds ago) Please check your changes! |
|
Seen on metadata-editor, collections, thrall, kahuna (merged by @andrew-nowak 9 minutes and 4 seconds ago) Please check your changes! |
What about if the user accidentally deletes a regular image, then immediately un-deletes... do we really want it retained forever |
2 participants
What does this change?
Currently, when an image is reaped (because it has not been saved, edited or used in content in 3 weeks since it arrived on the Grid) it is initially "soft-deleted", and then later fully deleted if it hasn't been rescued for a further 2 weeks. To rescue it, a user must press the "undelete" button. This will immediately make it eligible for the initial reaping stage; if a user is unlucky they might press the button a couple of seconds before the reaper runs, meaning that the image is undeleted, and then almost immediately redeleted.
Removing the soft-deleted metadata does not mark the image in any lasting way, so we cannot update the "reapable eligibility criteria" to account for it. I feel like a user manually asking for the image to be undeleted is a sufficient signal that the image should be kept that we should mark the image as "archived" (remember in Grid that "archived" is not "soft-deleted"; it's "this image is important and we should keep it permanently, as part of the "archive"). This would mean that it is no longer eligible for reaping.
So, when we receive a request to undelete an image, the backend will first mark it as archived (doing this first to avoid any other race conditions with the reaper) before removing the deletion marker.
How should a reviewer test this change?
Use the "delete" button on an image, then find it with a "+is:deleted" search (requires edit_metadata permission). Press "undelete", and the image should then be marked as "archived"
^ an image which has been marked as archived ^
How can success be measured?
Who should look at this?
Tested? Documented?