...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- Restricted Diffusion Commit
rPdcd7a316d239: Differential - add DifferentialDraft to track whether revisions have draft…
made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- T3496
- Lint
Lint Passed - Unit
Tests Passed
Event Timeline
Comment Actions
I spent too much time on this because I was never too happy with the result.
Storage wise, i think this is the best way to not make things too complicated given "has draft" can come from a main comment box or inline, thus needing draft key and N rows to cleanly manage adding / deleting, though clearly only one row is needed to say "has draft"
Feature wise, I think this is valuable and likely to be valuable generally in other applications. I could imagine Conpherence (for example) showing off something "has a draft" in the UI. All that said, the existing DIfferential users probably keep around tons of diffs which is not exactly how I think the tool is best used.
| src/applications/differential/controller/DifferentialCommentPreviewController.php | ||
|---|---|---|
| 129 | this is maybe not a good idea - once replaceOrDelete() is called once isDeleted() is always true and other oddities - but I don't think we delete() then save() stuff...? maybe delete() needs to be handled in PhabricatorDraft though too? | |
| src/applications/differential/storage/DifferentialDraft.php | ||
| 4 | feel free to pick on the class name here and the method names | |
Comment Actions
this is so beautiful
it brings tears to my eyes ;_;
Totally agreed about not migrating. We might be able to do a fake INSERT IGNORE ... CLEVER SELECT BLAH "migration" that kind of approximates the right end state if anyone complains but I don't think this feature sees a ton of use.
| src/applications/differential/storage/DifferentialDraft.php | ||
|---|---|---|
| 48 | stray phlog | |
| src/applications/differential/controller/DifferentialCommentPreviewController.php | ||
|---|---|---|
| 129 | Checking for strlen($request->getStr('content')) is a little more redundant but might be less perilous. | |
Comment Actions
- implement didDelete() hook to make this a little tighter
- also, if it was deleted, then delete the pertinent DifferentialDraft row (fixes a bug where just deleting your main comment didn't clear the notion of has draft)