Page MenuHomePhabricator

Switch File deletion to use ModularTransactions
ClosedPublic

Authored by amckinley on Apr 18 2017, 7:23 PM.
Tags
None
Referenced Files
F13278305: D17723.id42628.diff
Fri, May 31, 7:51 PM
F13269376: D17723.diff
Wed, May 29, 6:15 AM
F13266668: D17723.id.diff
Tue, May 28, 5:17 PM
F13254776: D17723.diff
Sat, May 25, 3:44 AM
F13235090: D17723.diff
Tue, May 21, 4:15 AM
F13218917: D17723.id.diff
Sat, May 18, 1:40 PM
F13215026: D17723.id42627.diff
Fri, May 17, 2:26 PM
F13189445: D17723.diff
Sat, May 11, 5:57 AM

Details

Summary

Fixes T12587. Adds a new PhabricatorFileDeleteTransaction that enqueues File delete tasks.

Test Plan
  • hack PhabricatorFileQuery to ignore isDeleted state
  • stop daemons
  • upload a file, delete it from the UI
  • check that the DB has updated isDeleted = 1
  • check timeline rendering in File detail view
  • start daemons
  • confirm rows are deleted from DB

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley added inline comments.
src/applications/files/controller/PhabricatorFileDeleteController.php
33

Maybe just use true to make the API a little more obvious later, unless you can come up with states we might have in the future other than "deleted" or "not deleted"?

src/applications/files/xaction/PhabricatorFileDeleteTransaction.php
17

You can omit save() here since the Editor calls it after calling all the applyInternalEffects() resolve -- basically it's doing:

foreach ($transactions as $xaction) {
  $xaction->applyInternalEffects($object, ...);
}
$object->save();
This revision is now accepted and ready to land.Apr 18 2017, 7:28 PM
amckinley added inline comments.
src/applications/files/controller/PhabricatorFileDeleteController.php
33

"Delete in progress"? in re: discussion in T12587

src/applications/files/controller/PhabricatorFileDeleteController.php
33

We might store a flag for that internally, but any API client would only ever say "delete this" or "cancel deleting this", I think -- there aren't three different values they could meaningfully pass that I can come up with, at least.

This revision was automatically updated to reflect the committed changes.