Page MenuHomePhabricator

Phriction - consolidate edit business logic into Editor
ClosedPublic

Authored by btrahan on Nov 6 2014, 8:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 8:22 AM
Unknown Object (File)
Wed, Apr 3, 9:18 PM
Unknown Object (File)
Wed, Apr 3, 10:15 AM
Unknown Object (File)
Fri, Mar 29, 6:14 AM
Unknown Object (File)
Thu, Mar 28, 7:37 PM
Unknown Object (File)
Thu, Mar 28, 5:09 PM
Unknown Object (File)
Thu, Mar 28, 4:35 PM
Unknown Object (File)
Thu, Mar 28, 3:47 PM
Subscribers

Details

Summary

Ref T4029. Some business logic lives outside the editor. This revision moves that logic from the edit controller into the editor proper. This makes re-using that business logic across other endpoints - say like a conduit end point - possible. This is also part of the general modernization quest for phriction I am on.

This diff also restores the functionality where you can delete a document by wiping out the content and saving.

Test Plan

tried to make a document with no title or content and saw errors. opened a document for edit with user 1, then made edits with user 2, then saw an error when i made the edit with user 1. clicking "overwrite changes" then worked. deleted a document by wiping out the body and clicking save.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Phriction - consolidate edit business logic into Editor.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

Oh, my inline misunderstood the behavior. The API seems a little confusing to me but not unreasonable. Maybe clarify it a bit.

src/applications/phriction/editor/PhrictionTransactionEditor.php
12

This seems a little oddly named since it's not an error (right?) but more like hasUserSeenContentVersionError? -- maybe more like forceOverwriteContent or ignoreVersionCheck or something?

This revision is now accepted and ready to land.Nov 6 2014, 8:29 PM

Specifically, when I was reading through, it would have been clearest if there were separate flags for the caller intent (like $ignoreEditConflicts) and generation of the conflict warning (like $rasiedVersionError).

btrahan edited edge metadata.

use a slighter better name - get/set ProcessContentVersionError - and then update negation as necessary.

re-run tests - they still pass

This revision was automatically updated to reflect the committed changes.