Add a formal "Draft" / "Not Yet Ready for Review" state to Differential
Open, NormalPublic

Description

This is a use case I just ran into, and I'm looking for feedback, but maybe it could lead to a feature request.

Sometimes, I will be working on a feature and I reach a point where I'd like to review where I'm at before continuing further. I can't finish the feature, because I need to verify that what I've done so far is OK with the rest of the team.

Just now, I submitted the revision using arc diff --plan-changes, which is ok -- but it doesn't show up in the reviewer's "Blocking Others" list; which it is, because I need a review before I can move forward with coding.

The alternative is to submit it for review without pending changes, but this runs the risk of accepting and landing an incomplete feature if the reviewer is not especially careful about reading through my comments.

I'm wondering if it would be worth having a sort of "Plan changes pending review" state? Or if there's a better workflow for this type of scenario.

Details

Commits
D18819 / rP6d3baa92f908: Execute Herald again after promoting revisions out of the "Draft" state
D18802 / rP1b76250f8995: Disallow "Accept" on drafts, allow "Resign"
D18801 / rP54f131dc4baa: Make "first broadcast" rules for Differential drafts more general
D18771 / rPc098aec66931: (stable) Restore "Summary" and "Test Plan" to initial mail for non-draft…
D18771 / rP314e7266c3e5: Restore "Summary" and "Test Plan" to initial mail for non-draft configurations
D18765 / rP759c757264c8: Include "Draft" revisions in Differential legacy status queries
D18748 / rPa4b934cad2b5: Clean up Differential draft mail behaviors
D18745 / rP7fa0d066bc45: Don't run Herald build rules when Differential revisions are updated…
D18743 / rARCf1ba27ffedd5: Add "arc diff --draft"
D18737 / rP28cec2f8a2bf: Allow revisions to be held as drafts, even after builds finish
D18740 / rARC52992b9550e0: Rename "arc diff --preview" to "arc diff --only"
D18739 / rARC4c4f2e6674e6: Remove "arc diff --only"
D18738 / rARCccd648b4c408: Remove "arc diff --plan-changes"
D18731 / rPf7f3dd5b2084: Don't run Herald build and mail rules when they don't make sense
D18730 / rPbeaf0ad9a636: Attribute revision promotion from "Draft" to "Needs Review" to the author
D18721 / rP63e6b2553e0e: Simply how Differential drafts ignore Harbormaster autobuilds
D18714 / rP1755ec242989: Show more detailed hints about draft revisions in the UI
D18713 / rPbfabe49c5ae8: Start revisions in "Draft" if prototypes are enabled
D18628 / rP36df39761ea5: Create revisions into "Draft", publish them when builds finish
D18627 / rPfca553f142a2: Prepare revision mail for the "Draft" status
D18626 / rPc7af663523e5: Align most revision actions to the new "Draft" state
D18625 / rP23867c148798: Add a "Draft" state for revisions, and action bucket support
D18493 / rP70ebdd269455: (stable) Make legacy revision statuses from "differential.query" have type…
D18493 / rPb4cbea901845: Make legacy revision statuses from "differential.query" have type "string" again
D18490 / rP017125598631: (stable) Fix an issue where "Close Revision" did not appear in the UI
D18490 / rPf49d103af5d3: Fix an issue where "Close Revision" did not appear in the UI
D18419 / rP48a74de0b649: Move all revision status transactions to modern values and mechanics
D18418 / rP7b695aa43bd1: Migrate revision storage to modern status constants ("accepted") instead of…
D18417 / rP5348f34c9eec: Make all revision status readers explicitly read modern or legacy status
D18416 / rP0b1d6a3f6ec6: Convert straggling Herald rules to modern revision status constants
D18415 / rPcd15c2d545ad: Swap transactions and initialization over to modern status constants
D18414 / rP895f0cde1f0a: Use modern revision statuses when bucketing revisions on the Differential…
D18413 / rP7f743c14d532: Remove remaining ArcanistDifferentialRevisionStatus references in revision…
D18412 / rP2b9838b482af: Modularize remaining TYPE_ACTION transactions in Differential, reducing calls…
D18410 / rP19bc91fd208d: Modularize the Differential "status" transaction and move away from…
D18408 / rP77bf24563704: Continue reducing callsites to ArcanistDifferentialRevisionStatus in…
D18400 / rP36197bf783e4: Provide revision status information via API all "differential.revision.search"
D18399 / rPea4e33261e49: (stable) Fix an inverted condition for the "Reopen Revision" action
D18399 / rPef8d4e2126af: Fix an inverted condition for the "Reopen Revision" action
D18398 / rP153e4d8a383c: Remove old reviewer double writes to legacy edge table in Differential
D18397 / rP42020e135767: Completely remove "differential.find" Conduit API method
D18396 / rP13ddb15bbcfb: Remove legacy `withStatus()` method from RevisionQuery
D18395 / rP50dfdb8d0355: Replace legacy Differential queries for "open" revisions with a modern mechanism
D18394 / rP212d4d0dc753: Migrate Differential Revision SavedQueries to the new "statuses" tokenizer
D18393 / rP53516093aed0: Replace Differential hard-coded status "<select />" with tokenizer
D18392 / rP8160baec2a6b: Add a Differential revision status tokenizer datasource
D18343 / rP46d1596bf797: Pull legacy revision query status filters out of the main Query class
D18341 / rP03ab7224bb4e: Reduce STATUS_CLOSED (now internally "Published") revision status callsites
D18340 / rP70088f7eec8f: Continue reducing callsites to ArcanistDifferentialRevisionStatus
D18339 / rP2e36653965a0: Reduce callsites to "ArcanistDifferentialRevisionStatus" in Phabricator
D8400 / rARC7b6b24154b71: Add "Changes Planned" and "In Preparation" states for revisions

Related Objects

There are a very large number of changes, so older changes are hidden. Show Older Changes
bradley removed a subscriber: bradley.Aug 11 2017, 9:43 PM

I've landed the two scary migration changes (D18418, D18419) and deployed them here without any obvious issues arising. The actual effects here are:

  • Phabricator now stores revision status string constants (like "accepted") everywhere, and no longer stores older numeric constants (like "2").
  • The differential.revision.search Conduit API method now allows querying by status.
  • The differential.revision.search Conduit API method now returns revision status information.
  • Phabricator no longer depends on the ArcanistDifferentialRevisionStatus class, defined in Arcanist.
  • The differential.find Conduit API method, which has had no upstream callers for about five years, has been removed.

This roughly corresponds to unearthing most of the technical debt here and leaving us free to pursue new server-defined statuses.

alexmv added a subscriber: alexmv.Sep 14 2017, 11:05 PM

An unusual case here is Herald rules which take the "Send me an email" action, and act "only the first time". Their current behavior will be to activate when the draft is created, have their mail killed later in the pipeline when the Editor declines to generate any, and then never fire again.

I think I'm maybe going to add a special case to Herald to deal with this, an exception like "RetryThisActionLaterException". If an action raises this exception, it will act like the entire rule failed.

This will still create a little bit of odd behavior if you have a rule which takes multiple actions, like "send me an email" and "add project tags" or something -- the tag part could act on a draft, but won't, and the whole rule will act after the revision publishes.

This also means we need to fire Herald more often -- essentially on every update, not just every major change (see T10109). If we don't, these "email me" rules won't activate when a revision leaves the "draft" state.

But I think we can do that, and just make the "Run Build Plan" rules also throw a "RetryThisActionLaterException" if the revision wasn't just updated. Kind of a lot of rules behind the scenes but I think the actual behavior will mostly be intuitive, and the edge cases (a rule which both sends email and runs a build plan, e.g.) can be resolved by splitting rules apart if they do arise. This should also let us resolve T10109.

A very rough version of this is now in master, but only active if phabricator.show-prototypes is on. These are some of the (substantial) remaining issues I'm aware of, and there are probably others:

  • Trivially, email doesn't really respect drafts yet, so the whole thing is semi-pointless, but likely will soon.
  • We currently (well, after the change above) mute the initial email entirely if a revision is a draft, but I think I want to continue sending it, just only to the author. Maybe? The email would be rephrased to say "This is a draft, and waiting on builds X, Y, Z."
  • When a revision gets stuck in a draft state because of a failed build, I suspect the notifications and cues are currently insufficient. For example: the /differential/ UI won't distinguish between "building draft", "static draft", and "failed draft", and I think the email notifications will be limited and/or misleading.
  • The "Send me an email only the first time" interaction described above won't work. I plan to implement a ThisCondition/ActionDoesNotWorkInThisState exception, per above.
  • The client has no awareness of this state yet, so you can't arc diff into it explicitly (e.g., with --hold) and stuff like arc branch won't color revisions in this state properly (in fact, it may not even know that this state exists).
  • We don't track if a revision has returned to a draft state. I plan to discard the "draftiness" of a revision permanently the first time it leaves the draft state so reviewers are immediately notified of later updates, but there's currently no tracking of this state.
hskiba added a subscriber: hskiba.Oct 22 2017, 2:46 AM
OCram added a subscriber: OCram.Oct 23 2017, 5:35 AM

This is technically accurate but real awkward for humans:

After all that stuff:

  • Herald should now interact properly with drafts.
  • Email (and other build-related cues) are still a bit weird and need some more refinement.
  • With a sufficiently up-to-date server and arc on the experimental branch, arc diff --draft should work (once everything here lands). See T11911 for other experimental changes and a rundown of new arc diff --flag behaviors. --preview, --only, --plan-changes, and --draft have changed. These changes ONLY affect the experimental branch.
  • See T13010 for a more concise summary of the status of this feature.

From T10109, we shouldn't make revisions "Buildable" to Herald if the update is an automatic update in response to a commit being discovered.

edibiase removed a subscriber: edibiase.Oct 31 2017, 6:55 PM

The harbormaster builds that we have running on revision creation can sometimes be rather slow; they're builds in our primary CI system, but with less of a priority than jobs in the submit queue. This will result in 10-30 minute delays for every revision in our monorepo. I understand the preference for only adding configuration settings when absolutely necessary, but since this may result in a significant time sink for any installs with expensive builds, is it worth keeping something around like differential.initial-state?

Maybe. Another possibility is to mark builds as "nonblocking" in some sense. There are some other open requests for defining "advisory" or "noncritical" builds where build failure doesn't cause the Buildable to report a failed result overall. If we end up adding this state to builds anyway, it probably makes sense to include a "Does Not Block Review" flag to the "Does Not Fail Buildable" / "Does Not Block Dependent Builds" / "Failure Is Totally Meaningless" / Whatever flags.

If this is a pain in the short term, you can tweak DifferentialRevision::initializeNewRevision() as a workaround until we figure out the plan in the longer term.

(Another possibility is to add behavior to arc to control this.)

@epriestley Are there any more concrete plans for this "nonblocking" builds? That is something we would like to utilise for one of our projects. Actually quite important as waiting for builds there is slowing us down quite a bit now.

GHC recently upgraded its Phabricator installation and has been rather unpleasantly affected by this feature. We have multi-hour CI jobs, which often queue to a day or so of builds. Making contributors wait this long before receiving feedback is turning out to be rather problematic.

Am I correct in thinking that this patch should be sufficient to revert to the previous behavior?

diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 6813c12..f1b789c 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -72,11 +72,7 @@ final class DifferentialRevision extends DifferentialDAO
     $view_policy = $app->getPolicy(
       DifferentialDefaultViewCapability::CAPABILITY);
 
-    if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) {
-      $initial_state = DifferentialRevisionStatus::DRAFT;
-    } else {
-      $initial_state = DifferentialRevisionStatus::NEEDS_REVIEW;
-    }
+    $initial_state = DifferentialRevisionStatus::NEEDS_REVIEW;
 
     return id(new DifferentialRevision())
       ->setViewPolicy($view_policy)

@epriestley, can you confirm that the above patch should disable the draft state? I have applied it to GHC's Phabricator deployment yet we are still seeing Differentials opened in draft state.