Page MenuHomePhabricator

Add a Phriction hint when a draft exists, and fix some draft editing bugs
ClosedPublic

Authored by epriestley on Sep 11 2018, 5:28 PM.

Details

Summary

Depends on D19662. Ref T13077. See PHI840.

  • If you're looking at the published version of a document, but a draft version exists and you can edit it, add a hint/link.
  • Fix an issue where the "draft" transaction would complain when you created a document since the initial content is empty and no "draft" transaction is adding any content.
Test Plan

Created new documents, viewed documents with current published versions and unpublished drafts.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Sep 11 2018, 5:28 PM
epriestley requested review of this revision.Sep 11 2018, 5:30 PM
amckinley accepted this revision.Sep 12 2018, 7:21 PM
amckinley added inline comments.
src/applications/phriction/controller/PhrictionDocumentController.php
190

I've seen this line of code a few times now; maybe this should be an isDraft() method on PhrictionDocument?

src/applications/phriction/controller/PhrictionEditController.php
100–106

I read about this in T13077#240720. Is the problem that saving a new document as a draft makes it ambiguous what the "live version" of the page should look like? (Like, you'd have to make the Phriction "Page Not Found" UI somehow reflect that there's a draft hanging out at this URL so you can't create a document here, even though there isn't anything actually here yet).

This revision is now accepted and ready to land.Sep 12 2018, 7:21 PM

Is the problem that saving a new document as a draft makes it ambiguous what the "live version" of the page should look like?

Largely, yes. I think there's one technical problem and one product problem:

The technical problem is that if we implement hasUnpublishedDraft() on Document like this:

public function hasUnpublishedDraft() {
  return ($this->getContent()->getVersion() < $this->getMaximumVerison());
}

...then there's no easy/obvious way to set things up in the database to make this return true for a "new + draft" document, since there's only going to be one content object and thus only one version, and 1 (the version of that first content object) is not less than 1 (the maximum version of any content object on the document). This is tractable (we could write a fake content object, we could have some special flag, whatever) but I think we need to add some amount of complexity to handle it.

The other problem is as you suggest:

  • What does the "live page" show when you look at it and there's no published version?
  • What does it show in the document hierarchy?

I think we could find reasonable answers to these questions, but I'm generally not completely sure that this draft feature is actually good / non-confusing / reasonable / useful as currently written, so I want to kind of float out a minimum take on it and see what feedback looks like. Most of the other changes in this series are probably good things whether "draft" sticks around in this form or not (database fixes, more modern transaction handling, better UI navigation between versions, etc) but I'd like a little more confirmation that I'm on the right track with "Draft" (even if that's just using it for a bit myself and having a reasonable experience with it).

One related issue is that there's no way to find documents which return true from (hypothetical) hasUnpublishedDraft() via a DocumentQuery, so we can't add features like "show drafts I'm working on" easily. If we added something like that, it might make it easier to answer "what happens in the hierarchy?" -- if the answer to "What shows in the hierarchy for new + draft documents?" is "nothing", we need some other way for users to get back to them, finish editing them, and eventually publish them.

One related issue is that there's no way to find documents which return true from (hypothetical) hasUnpublishedDraft() via a DocumentQuery...

Oh, that makes sense, because drafts aren't a first-class entity; they're just documents that have had a "don't actually bump the version" transaction applied to them at some point. Eventually (if drafts survive at all), I guess we'll end up with a column on phriction_document like is_draft? (I'm just wargaming this out loud to understand how you imagine this feature would evolve).

Yeah -- some possible approaches I haven't thought about too much:

  • Just an is_draft thing on Document.
  • There's an existing status on Document (which stores moved, deleted) and maybe that gets a "draft" value? (Can you have a "draft" deleted document? Today, the answer is "no".)
  • When you apply a "draft", write a "UserWorkingOnDraft" edge. When a document is published, remove it. Then we can query for "drafts you're working on" even if you aren't the most recent editor. But this maybe gets a little weird with "publish-revert" operations, long-lived drafts, drafts you want to disavow, etc.
  • Maybe is_draft on the Content? That's a slightly simpler version of "UserWorkingOnDraft" but it might not be quite flexible enough.
  • We can probably make Query find "has draft" without any schema changes with multiple clever JOINs, although I'd guess the query plan for this will be garbage.

I suppose we could cheat like this if we want to explore draft queries:

SELECT d.* FROM phriction_document d JOIN phriction_content draft
  ON draft.documentPHID = d.phid
  AND draft.version = d.maxVersion
  AND d.contentPHID != draft.phid

The query plan probably isn't great but I don't think it's explosively bad and Phriction document tables are probably not huge in reasonable cases. That's not good enough to get "drafts you've contributed to", but AND draft.authorPHID = <viewerPHID> could get "drafts you've personally written".

epriestley marked an inline comment as done.Sep 12 2018, 8:38 PM
epriestley added inline comments.
src/applications/phriction/controller/PhrictionDocumentController.php
190

This seems reasonable but I couldn't actually find any other checks against this particular condition, or any checks where we're asking the same question in numerous places. We do:

  • "Does this document have one or more unpublished drafts?", here.
  • "Does this document have a different max version now than it did when you started editing it?", in the Editor.
  • "Is this particular content object the current maximum object?", later in this controller.

I'll keep an eye out for opportunities here. This might be worth doing even if we have only one callsite today just for clarity, but I'd guess we'll pick up additional callsites for at least some of these questions when, e.g., phriction.document.search gets expanded to handle drafts.

This revision was automatically updated to reflect the committed changes.