From 6dc721009d9e91a46bec34b23b804ca1ff33a1bf Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 7 Sep 2018 12:18:39 -0700
Subject: [PATCH 01/25] Layout Phriction actions without floats, to avoid
 conflicts with floating content

Summary:
Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list.

Instead, use table-cell layout on desktops.

Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: GoogleLegacy

Maniphest Tasks: T13195

Differential Revision: https://secure.phabricator.com/D19649
---
 resources/celerity/map.php                  |  4 ++--
 src/view/phui/PHUIDocumentView.php          | 18 ++++++++++++++----
 webroot/rsrc/css/phui/phui-document-pro.css | 16 +++++++++++++---
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 743abd38ee..d3a9a1a34f 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -146,7 +146,7 @@ return array(
     'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad',
     'rsrc/css/phui/phui-crumbs-view.css' => '10728aaa',
     'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026',
-    'rsrc/css/phui/phui-document-pro.css' => '0e41dd91',
+    'rsrc/css/phui/phui-document-pro.css' => '1a08ef4b',
     'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf',
     'rsrc/css/phui/phui-document.css' => 'c4ac41f9',
     'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9',
@@ -814,7 +814,7 @@ return array(
     'phui-curtain-view-css' => '2bdaf026',
     'phui-document-summary-view-css' => '9ca48bdf',
     'phui-document-view-css' => 'c4ac41f9',
-    'phui-document-view-pro-css' => '0e41dd91',
+    'phui-document-view-pro-css' => '1a08ef4b',
     'phui-feed-story-css' => '44a9c8e9',
     'phui-font-icon-base-css' => '870a7360',
     'phui-fontkit-css' => '1320ed01',
diff --git a/src/view/phui/PHUIDocumentView.php b/src/view/phui/PHUIDocumentView.php
index b29afb5496..f57152833e 100644
--- a/src/view/phui/PHUIDocumentView.php
+++ b/src/view/phui/PHUIDocumentView.php
@@ -171,10 +171,20 @@ final class PHUIDocumentView extends AphrontTagView {
         $table_of_contents,
         $this->header,
         $this->banner,
-        array(
-          $curtain,
-          $main_content,
-        ),
+        phutil_tag(
+          'div',
+          array(
+            'class' => 'phui-document-content-outer',
+          ),
+          phutil_tag(
+            'div',
+            array(
+              'class' => 'phui-document-content-inner',
+            ),
+            array(
+              $main_content,
+              $curtain,
+            ))),
         $foot_content,
       ));
 
diff --git a/webroot/rsrc/css/phui/phui-document-pro.css b/webroot/rsrc/css/phui/phui-document-pro.css
index 29dc3beffa..4d58d17cb1 100644
--- a/webroot/rsrc/css/phui/phui-document-pro.css
+++ b/webroot/rsrc/css/phui/phui-document-pro.css
@@ -4,7 +4,7 @@
 
 .phui-document-view.phui-document-view-pro {
   max-width: 800px;
-  padding: 16px 16px 64px 16px;
+  padding: 16px 16px 32px 16px;
   margin: 0 auto;
 }
 
@@ -21,15 +21,25 @@
 }
 
 .device-desktop .has-curtain .phui-document-content-view {
-  padding-right: 332px;
+  display: table-cell;
 }
 
 .printable .phui-document-content-view {
   padding-right: 0;
 }
 
+.device-desktop .phui-document-content-outer {
+  display: table;
+  width: 100%;
+  layout: fixed;
+}
+
+.device-desktop .phui-document-content-inner {
+  display: table-row;
+}
+
 .device-desktop .phui-document-curtain {
-  float: right;
+  display: table-cell;
   width: 300px;
 }
 

From bae8a951143ab02d3f8d4f3c920784fc4c0f34d1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 7 Sep 2018 09:01:30 -0700
Subject: [PATCH 02/25] Continue replacing Commit/Audit status checks with
 object-based checks

Summary: Ref T13195. See PHI851. Continuing down the path toward replacing these legacy numeric constants with more modern string constants.

Test Plan:
- Raised concern, requested verification, verified.
- Looked at commit hovercard with audit status.
- Viewed header on a commit page.
- (Didn't test the Doorkeeper stuff since it requires linking to Asana and seems unlikely to break.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13195

Differential Revision: https://secure.phabricator.com/D19647
---
 .../PhabricatorAuditCommitStatusConstants.php | 24 ++++++++++++
 .../audit/editor/PhabricatorAuditEditor.php   |  4 +-
 .../controller/DiffusionCommitController.php  | 13 +++----
 ...sionDoorkeeperCommitFeedStoryPublisher.php | 13 +------
 .../DiffusionHovercardEngineExtension.php     | 10 ++---
 ...fusionCommitRequiredActionResultBucket.php | 11 ++----
 .../DiffusionCommitConcernTransaction.php     |  4 +-
 .../DiffusionCommitStateTransaction.php       | 39 ++++++++++---------
 .../DiffusionCommitVerifyTransaction.php      |  3 +-
 .../storage/PhabricatorRepositoryCommit.php   | 20 ++++++++++
 10 files changed, 84 insertions(+), 57 deletions(-)

diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
index e559862c83..50e4180074 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -60,6 +60,30 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key));
   }
 
+  public function isNoAudit() {
+    return ($this->key == self::MODERN_NONE);
+  }
+
+  public function isConcernRaised() {
+    return ($this->key == self::MODERN_CONCERN_RAISED);
+  }
+
+  public function isNeedsVerification() {
+    return ($this->key == self::MODERN_NEEDS_VERIFICATION);
+  }
+
+  public function isPartiallyAudited() {
+    return ($this->key == self::MODERN_PARTIALLY_AUDITED);
+  }
+
+  public function isAudited() {
+    return ($this->key == self::MODERN_AUDITED);
+  }
+
+  public function getIsClosed() {
+    return idx($this->spec, 'closed');
+  }
+
   public static function getStatusNameMap() {
     $map = self::getMap();
     return ipull($map, 'name', 'legacy');
diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index c04bb8185b..d5c22fba6b 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -206,12 +206,10 @@ final class PhabricatorAuditEditor
       $object->writeImportStatusFlag($import_status_flag);
     }
 
-    $partial_status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED;
-
     // If the commit has changed state after this edit, add an informational
     // transaction about the state change.
     if ($old_status != $new_status) {
-      if ($new_status == $partial_status) {
+      if ($object->isAuditStatusPartiallyAudited()) {
         // This state isn't interesting enough to get a transaction. The
         // best way we could lead the user forward is something like "This
         // commit still requires additional audits." but that's redundant and
diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php
index a6743b6557..283278fcf8 100644
--- a/src/applications/diffusion/controller/DiffusionCommitController.php
+++ b/src/applications/diffusion/controller/DiffusionCommitController.php
@@ -171,13 +171,12 @@ final class DiffusionCommitController extends DiffusionController {
         ->setHeaderIcon('fa-code-fork')
         ->addTag($commit_tag);
 
-      if ($commit->getAuditStatus()) {
-        $icon = PhabricatorAuditCommitStatusConstants::getStatusIcon(
-          $commit->getAuditStatus());
-        $color = PhabricatorAuditCommitStatusConstants::getStatusColor(
-          $commit->getAuditStatus());
-        $status = PhabricatorAuditCommitStatusConstants::getStatusName(
-          $commit->getAuditStatus());
+      if (!$commit->isAuditStatusNoAudit()) {
+        $status = $commit->getAuditStatusObject();
+
+        $icon = $status->getIcon();
+        $color = $status->getColor();
+        $status = $status->getName();
 
         $header->setStatus($icon, $color, $status);
       }
diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php
index 088f4b752e..712b7ae150 100644
--- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php
+++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php
@@ -31,8 +31,6 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher
     // After ApplicationTransactions, we could annotate feed stories more
     // explicitly.
 
-    $fully_audited = PhabricatorAuditCommitStatusConstants::FULLY_AUDITED;
-
     $story = $this->getFeedStory();
     $xaction = $story->getPrimaryTransaction();
     switch ($xaction->getTransactionType()) {
@@ -41,7 +39,7 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher
           case PhabricatorAuditActionConstants::CLOSE:
             return true;
           case PhabricatorAuditActionConstants::ACCEPT:
-            if ($object->getAuditStatus() == $fully_audited) {
+            if ($object->isAuditStatusAudited()) {
               return true;
             }
             break;
@@ -165,14 +163,7 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher
   }
 
   public function isObjectClosed($object) {
-    switch ($object->getAuditStatus()) {
-      case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT:
-      case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED:
-      case PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED:
-        return false;
-      default:
-        return true;
-    }
+    return $object->getAuditStatusObject()->getIsClosed();
   }
 
   public function getResponsibilityTitle($object) {
diff --git a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php
index 0711028796..3ce95cb3c3 100644
--- a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php
+++ b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php
@@ -41,12 +41,12 @@ final class DiffusionHovercardEngineExtension
     $hovercard->addField(pht('Date'),
       phabricator_date($commit->getEpoch(), $viewer));
 
-    if ($commit->getAuditStatus() !=
-      PhabricatorAuditCommitStatusConstants::NONE) {
+    if (!$commit->isAuditStatusNoAudit()) {
+      $status = $commit->getAuditStatusObject();
 
-      $hovercard->addField(pht('Audit Status'),
-        PhabricatorAuditCommitStatusConstants::getStatusName(
-          $commit->getAuditStatus()));
+      $hovercard->addField(
+        pht('Audit Status'),
+        $status->getName());
     }
   }
 
diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
index 40bc63025d..70584ae243 100644
--- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
+++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
@@ -69,14 +69,12 @@ final class DiffusionCommitRequiredActionResultBucket
     $results = array();
     $objects = $this->objects;
 
-    $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
-
     foreach ($objects as $key => $object) {
       if (empty($phids[$object->getAuthorPHID()])) {
         continue;
       }
 
-      if ($object->getAuditStatus() != $status_concern) {
+      if (!$object->isAuditStatusConcernRaised()) {
         continue;
       }
 
@@ -91,7 +89,6 @@ final class DiffusionCommitRequiredActionResultBucket
     $results = array();
     $objects = $this->objects;
 
-    $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
     $has_concern = array(
       PhabricatorAuditStatusConstants::CONCERNED,
     );
@@ -102,7 +99,7 @@ final class DiffusionCommitRequiredActionResultBucket
         continue;
       }
 
-      if ($object->getAuditStatus() != $status_verify) {
+      if (!$object->isAuditStatusNeedsVerification()) {
         continue;
       }
 
@@ -147,10 +144,8 @@ final class DiffusionCommitRequiredActionResultBucket
     $results = array();
     $objects = $this->objects;
 
-    $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
-
     foreach ($objects as $key => $object) {
-      if ($object->getAuditStatus() != $status_concern) {
+      if (!$object->isAuditStatusConcernRaised()) {
         continue;
       }
 
diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
index 30ec8d9f6d..07194a2af3 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
@@ -54,10 +54,8 @@ final class DiffusionCommitConcernTransaction
 
     // Even if you've already raised a concern, you can raise again as long
     // as the author requested you verify.
-    $state_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
-
     if ($this->isViewerFullyRejected($object, $viewer)) {
-      if ($object->getAuditStatus() != $state_verify) {
+      if (!$object->isAuditStatusNeedsVerification()) {
         throw new Exception(
           pht(
             'You can not raise a concern with this commit because you have '.
diff --git a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
index a6288923f4..bcf6a9e382 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
@@ -11,29 +11,32 @@ final class DiffusionCommitStateTransaction
     throw new PhutilMethodNotImplementedException();
   }
 
-  public function getIcon() {
+  private function getAuditStatusObject() {
     $new = $this->getNewValue();
-    return PhabricatorAuditCommitStatusConstants::getStatusIcon($new);
+    return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($new);
+  }
+
+  public function getIcon() {
+    return $this->getAuditStatusObject()->getIcon();
   }
 
   public function getColor() {
-    $new = $this->getNewValue();
-    return PhabricatorAuditCommitStatusConstants::getStatusColor($new);
+    return $this->getAuditStatusObject()->getColor();
   }
 
   public function getTitle() {
-    $new = $this->getNewValue();
+    $status = $this->getAuditStatusObject();
 
-    switch ($new) {
-      case PhabricatorAuditCommitStatusConstants::NONE:
+    switch ($status->getKey()) {
+      case PhabricatorAuditCommitStatusConstants::MODERN_NONE:
         return pht('This commit no longer requires audit.');
-      case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT:
+      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT:
         return pht('This commit now requires audit.');
-      case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED:
+      case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED:
         return pht('This commit now has outstanding concerns.');
-      case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION:
+      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION:
         return pht('This commit now requires verification by auditors.');
-      case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED:
+      case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED:
         return pht('All concerns with this commit have now been addressed.');
     }
 
@@ -41,26 +44,26 @@ final class DiffusionCommitStateTransaction
   }
 
   public function getTitleForFeed() {
-    $new = $this->getNewValue();
+    $status = $this->getAuditStatusObject();
 
-    switch ($new) {
-      case PhabricatorAuditCommitStatusConstants::NONE:
+    switch ($status->getKey()) {
+      case PhabricatorAuditCommitStatusConstants::MODERN_NONE:
         return pht(
           '%s no longer requires audit.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT:
+      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT:
         return pht(
           '%s now requires audit.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED:
+      case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED:
         return pht(
           '%s now has outstanding concerns.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION:
+      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION:
         return pht(
           '%s now requires verification by auditors.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED:
+      case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED:
         return pht(
           'All concerns with %s have now been addressed.',
           $this->renderObject());
diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
index a44261f7a4..3e6abb705f 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
@@ -48,8 +48,7 @@ final class DiffusionCommitVerifyTransaction
           'are not the author.'));
     }
 
-    $status = $object->getAuditStatus();
-    if ($status != PhabricatorAuditCommitStatusConstants::CONCERN_RAISED) {
+    if (!$object->isAuditStatusConcernRaised()) {
       throw new Exception(
         pht(
           'You can not request verification of this commit because no '.
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 447db08e6c..f56962d9f8 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -535,6 +535,26 @@ final class PhabricatorRepositoryCommit
     return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($status);
   }
 
+  public function isAuditStatusNoAudit() {
+    return $this->getAuditStatusObject()->isNoAudit();
+  }
+
+  public function isAuditStatusConcernRaised() {
+    return $this->getAuditStatusObject()->isConcernRaised();
+  }
+
+  public function isAuditStatusNeedsVerification() {
+    return $this->getAuditStatusObject()->isNeedsVerification();
+  }
+
+  public function isAuditStatusPartiallyAudited() {
+    return $this->getAuditStatusObject()->isPartiallyAudited();
+  }
+
+  public function isAuditStatusAudited() {
+    return $this->getAuditStatusObject()->isAudited();
+  }
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
   public function getCapabilities() {

From 853a816b3caf7d863d97deda2d3cc4a45f4f2434 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Sep 2018 11:35:11 -0700
Subject: [PATCH 03/25] Continue converting Audit constants, allowing the Query
 to handle either strings or integers

Summary: Ref T13197. We're almost ready to migrate: let the Query accept either older integer values or new string values. Then move some callsites to use strings.

Test Plan: Called `audit.query`, browsed audits, audited commits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19650
---
 .../audit/conduit/AuditQueryConduitAPIMethod.php | 10 +++++-----
 .../PhabricatorAuditCommitStatusConstants.php    | 16 +++++++++++++---
 .../diffusion/query/DiffusionCommitQuery.php     |  9 ++++++++-
 ...DiffusionCommitRequiredActionResultBucket.php | 14 ++++++--------
 .../PhabricatorOwnersDetailController.php        |  3 ++-
 .../storage/PhabricatorRepositoryCommit.php      | 11 ++++++-----
 6 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
index f3858afbf8..6d29d2345d 100644
--- a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
+++ b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
@@ -68,17 +68,17 @@ final class AuditQueryConduitAPIMethod extends AuditConduitAPIMethod {
 
     $status_map = array(
       self::AUDIT_LEGACYSTATUS_OPEN => array(
-        PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT,
-        PhabricatorAuditCommitStatusConstants::CONCERN_RAISED,
+        PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT,
+        PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED,
       ),
       self::AUDIT_LEGACYSTATUS_CONCERN => array(
-        PhabricatorAuditCommitStatusConstants::CONCERN_RAISED,
+        PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED,
       ),
       self::AUDIT_LEGACYSTATUS_ACCEPTED => array(
-        PhabricatorAuditCommitStatusConstants::FULLY_AUDITED,
+        PhabricatorAuditCommitStatusConstants::MODERN_AUDITED,
       ),
       self::AUDIT_LEGACYSTATUS_PARTIAL => array(
-        PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED,
+        PhabricatorAuditCommitStatusConstants::MODERN_PARTIALLY_AUDITED,
       ),
     );
 
diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
index 50e4180074..2196940545 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -22,9 +22,11 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
   public static function newForLegacyStatus($status) {
     $map = self::getMap();
 
-    foreach ($map as $key => $spec) {
-      if (idx($spec, 'legacy') == $status) {
-        return self::newForStatus($key);
+    if (is_int($status) || ctype_digit($status)) {
+      foreach ($map as $key => $spec) {
+        if ((int)idx($spec, 'legacy') === (int)$status) {
+          return self::newForStatus($key);
+        }
       }
     }
 
@@ -56,6 +58,10 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return idx($this->spec, 'color');
   }
 
+  public function getLegacyKey() {
+    return idx($this->spec, 'legacy');
+  }
+
   public function getName() {
     return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key));
   }
@@ -64,6 +70,10 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return ($this->key == self::MODERN_NONE);
   }
 
+  public function isNeedsAudit() {
+    return ($this->key == self::MODERN_NEEDS_AUDIT);
+  }
+
   public function isConcernRaised() {
     return ($this->key == self::MODERN_CONCERN_RAISED);
   }
diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 0ca9cd97d4..9a7e21b0af 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -714,10 +714,17 @@ final class DiffusionCommitQuery
     }
 
     if ($this->statuses !== null) {
+      $statuses = array();
+      foreach ($this->statuses as $status) {
+        $object = PhabricatorAuditCommitStatusConstants::newForLegacyStatus(
+          $status);
+        $statuses[] = $object->getLegacyKey();
+      }
+
       $where[] = qsprintf(
         $conn,
         'commit.auditStatus IN (%Ld)',
-        $this->statuses);
+        $statuses);
     }
 
     if ($this->packagePHIDs !== null) {
diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
index 70584ae243..25984c93e1 100644
--- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
+++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
@@ -164,15 +164,13 @@ final class DiffusionCommitRequiredActionResultBucket
     $results = array();
     $objects = $this->objects;
 
-    $status_waiting = array(
-      PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT,
-      PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION,
-      PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED,
-    );
-    $status_waiting = array_fuse($status_waiting);
-
     foreach ($objects as $key => $object) {
-      if (empty($status_waiting[$object->getAuditStatus()])) {
+      $any_waiting =
+        $object->isAuditStatusNeedsAudit() ||
+        $object->isAuditStatusNeedsVerification() ||
+        $object->isAuditStatusPartiallyAudited();
+
+      if (!$any_waiting) {
         continue;
       }
 
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index 1caf12a82d..bdc4a2e475 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -71,7 +71,8 @@ final class PhabricatorOwnersDetailController
           'package' => $package->getPHID(),
         ));
 
-    $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
+    $status_concern =
+      PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED;
 
     $attention_commits = id(new DiffusionCommitQuery())
       ->setViewer($request->getUser())
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index f56962d9f8..291c895231 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -381,14 +381,11 @@ final class PhabricatorRepositoryCommit
       }
     }
 
-    $current_status = $this->getAuditStatus();
-    $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
-
     if ($any_concern) {
-      if ($current_status == $status_verify) {
+      if ($this->isAuditStatusNeedsVerification()) {
         // If the change is in "Needs Verification", we keep it there as
         // long as any auditors still have concerns.
-        $status = $status_verify;
+        $status = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
       } else {
         $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
       }
@@ -539,6 +536,10 @@ final class PhabricatorRepositoryCommit
     return $this->getAuditStatusObject()->isNoAudit();
   }
 
+  public function isAuditStatusNeedsAudit() {
+    return $this->getAuditStatusObject()->isNeedsAudit();
+  }
+
   public function isAuditStatusConcernRaised() {
     return $this->getAuditStatusObject()->isConcernRaised();
   }

From 8eb8e8e1d86f1eddaf4c4b839910501f9b444a90 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Sep 2018 12:07:36 -0700
Subject: [PATCH 04/25] Make DiffusionCommitSearch accept modern (string)
 constants

Summary:
Depends on D19650. Ref T13197. Allow `SearchCheckboxesField` to have a "deprecated" map of older aliases, then convert them to modern values.

On the API method page, show all the values.

This technically resolves the issue in PHI841, although I still plan to migrate behind this.

Test Plan:
{F5875363}

- Queried audits, fiddled with `?status=1,audited`, etc.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19651
---
 .../PhabricatorAuditCommitStatusConstants.php | 27 ++++++--------
 .../query/PhabricatorCommitSearchEngine.php   |  4 +-
 .../data/ConduitConstantDescription.php       | 10 +++++
 .../PhabricatorSearchEngineAPIMethod.php      | 15 +++++++-
 .../PhabricatorSearchCheckboxesField.php      | 37 ++++++++++++++++++-
 5 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
index 2196940545..eca420ce1b 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -94,15 +94,6 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return idx($this->spec, 'closed');
   }
 
-  public static function getStatusNameMap() {
-    $map = self::getMap();
-    return ipull($map, 'name', 'legacy');
-  }
-
-  public static function getStatusName($code) {
-    return idx(self::getStatusNameMap(), $code, pht('Unknown'));
-  }
-
   public static function getOpenStatusConstants() {
     $constants = array();
     foreach (self::getMap() as $map) {
@@ -113,16 +104,22 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return $constants;
   }
 
-  public static function getStatusColor($code) {
+  public static function newOptions() {
     $map = self::getMap();
-    $map = ipull($map, 'color', 'legacy');
-    return idx($map, $code);
+    return ipull($map, 'name');
   }
 
-  public static function getStatusIcon($code) {
+  public static function newDeprecatedOptions() {
     $map = self::getMap();
-    $map = ipull($map, 'icon', 'legacy');
-    return idx($map, $code);
+
+    $results = array();
+    foreach ($map as $key => $spec) {
+      if (isset($spec['legacy'])) {
+        $results[$spec['legacy']] = $key;
+      }
+    }
+
+    return $results;
   }
 
   private static function getMap() {
diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
index e1b294a9d6..67861b7e7d 100644
--- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php
+++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
@@ -92,7 +92,9 @@ final class PhabricatorCommitSearchEngine
         ->setLabel(pht('Audit Status'))
         ->setKey('statuses')
         ->setAliases(array('status'))
-        ->setOptions(PhabricatorAuditCommitStatusConstants::getStatusNameMap())
+        ->setOptions(PhabricatorAuditCommitStatusConstants::newOptions())
+        ->setDeprecatedOptions(
+          PhabricatorAuditCommitStatusConstants::newDeprecatedOptions())
         ->setDescription(pht('Find commits with given audit statuses.')),
       id(new PhabricatorSearchDatasourceField())
         ->setLabel(pht('Repositories'))
diff --git a/src/applications/conduit/data/ConduitConstantDescription.php b/src/applications/conduit/data/ConduitConstantDescription.php
index fe2af2b716..c9b2d00f6b 100644
--- a/src/applications/conduit/data/ConduitConstantDescription.php
+++ b/src/applications/conduit/data/ConduitConstantDescription.php
@@ -4,6 +4,7 @@ final class ConduitConstantDescription extends Phobject {
 
   private $key;
   private $value;
+  private $isDeprecated;
 
   public function setKey($key) {
     $this->key = $key;
@@ -23,4 +24,13 @@ final class ConduitConstantDescription extends Phobject {
     return $this->value;
   }
 
+  public function setIsDeprecated($is_deprecated) {
+    $this->isDeprecated = $is_deprecated;
+    return $this;
+  }
+
+  public function getIsDeprecated() {
+    return $this->isDeprecated;
+  }
+
 }
diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php
index 335ba58852..235d74d6f3 100644
--- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php
+++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php
@@ -230,9 +230,20 @@ EOTEXT
 
         $constants_rows = array();
         foreach ($constants as $constant) {
+          if ($constant->getIsDeprecated()) {
+            $icon = id(new PHUIIconView())
+              ->setIcon('fa-exclamation-triangle', 'red');
+          } else {
+            $icon = null;
+          }
+
           $constants_rows[] = array(
             $constant->getKey(),
-            $constant->getValue(),
+            array(
+              $icon,
+              ' ',
+              $constant->getValue(),
+            ),
           );
         }
 
@@ -244,7 +255,7 @@ EOTEXT
             ))
           ->setColumnClasses(
             array(
-              'pre',
+              'mono',
               'wide',
             ));
 
diff --git a/src/applications/search/field/PhabricatorSearchCheckboxesField.php b/src/applications/search/field/PhabricatorSearchCheckboxesField.php
index 0985c976ce..ce41f85db3 100644
--- a/src/applications/search/field/PhabricatorSearchCheckboxesField.php
+++ b/src/applications/search/field/PhabricatorSearchCheckboxesField.php
@@ -4,6 +4,7 @@ final class PhabricatorSearchCheckboxesField
   extends PhabricatorSearchField {
 
   private $options;
+  private $deprecatedOptions = array();
 
   public function setOptions(array $options) {
     $this->options = $options;
@@ -14,6 +15,15 @@ final class PhabricatorSearchCheckboxesField
     return $this->options;
   }
 
+  public function setDeprecatedOptions(array $deprecated_options) {
+    $this->deprecatedOptions = $deprecated_options;
+    return $this;
+  }
+
+  public function getDeprecatedOptions() {
+    return $this->deprecatedOptions;
+  }
+
   protected function getDefaultValue() {
     return array();
   }
@@ -23,11 +33,12 @@ final class PhabricatorSearchCheckboxesField
       return array();
     }
 
-    return $value;
+    return $this->getCanonicalValue($value);
   }
 
   protected function getValueFromRequest(AphrontRequest $request, $key) {
-    return $this->getListFromRequest($request, $key);
+    $value = $this->getListFromRequest($request, $key);
+    return $this->getCanonicalValue($value);
   }
 
   protected function newControl() {
@@ -58,7 +69,29 @@ final class PhabricatorSearchCheckboxesField
         ->setValue($option);
     }
 
+    foreach ($this->getDeprecatedOptions() as $key => $value) {
+      $list[] = id(new ConduitConstantDescription())
+        ->setKey($key)
+        ->setIsDeprecated(true)
+        ->setValue(pht('Deprecated alias for "%s".', $value));
+    }
+
     return $list;
   }
 
+  private function getCanonicalValue(array $values) {
+    // Always map the current normal options to themselves.
+    $normal_options = array_fuse(array_keys($this->getOptions()));
+
+    // Map deprecated values to their new values.
+    $deprecated_options = $this->getDeprecatedOptions();
+
+    $map = $normal_options + $deprecated_options;
+    foreach ($values as $key => $value) {
+      $values[$key] = idx($map, $value, $value);
+    }
+
+    return $values;
+  }
+
 }

From 185e72c881cfa9a99af4a259fcfbf1010b4979e5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Sep 2018 14:32:55 -0700
Subject: [PATCH 05/25] Add aural section headers for "Event Timeline", "Add
 Comment", and "Comment Preview"

Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.

Test Plan: {F5875471}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19654
---
 resources/celerity/map.php                    | 22 +++++++++----------
 ...bricatorApplicationTransactionResponse.php |  8 +++++++
 ...catorApplicationTransactionCommentView.php |  7 ++++++
 src/view/phui/PHUITimelineView.php            | 10 ++++++++-
 .../transactions/behavior-comment-actions.js  |  1 +
 5 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index d3a9a1a34f..39fe047f19 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -423,7 +423,7 @@ return array(
     'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072',
     'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08',
     'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f',
-    'rsrc/js/application/transactions/behavior-comment-actions.js' => '54110499',
+    'rsrc/js/application/transactions/behavior-comment-actions.js' => '038bf27f',
     'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243',
     'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96',
     'rsrc/js/application/transactions/behavior-show-older-transactions.js' => '8f29b364',
@@ -575,7 +575,7 @@ return array(
     'javelin-behavior-bulk-job-reload' => 'edf8a145',
     'javelin-behavior-calendar-month-view' => 'fe33e256',
     'javelin-behavior-choose-control' => '327a00d1',
-    'javelin-behavior-comment-actions' => '54110499',
+    'javelin-behavior-comment-actions' => '038bf27f',
     'javelin-behavior-config-reorder-fields' => 'b6993408',
     'javelin-behavior-conpherence-menu' => '4047cd35',
     'javelin-behavior-conpherence-participant-pane' => 'd057e45a',
@@ -905,6 +905,15 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    '038bf27f' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-workflow',
+      'javelin-dom',
+      'phuix-form-control-view',
+      'phuix-icon-view',
+      'javelin-behavior-phabricator-gesture',
+    ),
     '040fce04' => array(
       'javelin-behavior',
       'javelin-request',
@@ -1251,15 +1260,6 @@ return array(
       'javelin-vector',
       'javelin-typeahead-static-source',
     ),
-    54110499 => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-workflow',
-      'javelin-dom',
-      'phuix-form-control-view',
-      'phuix-icon-view',
-      'javelin-behavior-phabricator-gesture',
-    ),
     '549459b8' => array(
       'javelin-behavior',
     ),
diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php
index 08e6ec464b..43a2fbc1b5 100644
--- a/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php
+++ b/src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php
@@ -83,7 +83,15 @@ final class PhabricatorApplicationTransactionResponse
       $xactions[$key] = hsprintf('%s', $xaction);
     }
 
+    $aural = phutil_tag(
+      'h3',
+      array(
+        'class' => 'aural-only',
+      ),
+      pht('Comment Preview'));
+
     $content = array(
+      'header' => hsprintf('%s', $aural),
       'xactions' => $xactions,
       'spacer' => PHUITimelineView::renderSpacer(),
       'previewContent' => hsprintf('%s', $this->getPreviewContent()),
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
index 7469004e1b..fcdc2fcb31 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
@@ -245,6 +245,13 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
       ->setFlush(true)
       ->addClass('phui-comment-form-view')
       ->addSigil('phui-comment-form')
+      ->appendChild(
+        phutil_tag(
+          'h3',
+          array(
+            'class' => 'aural-only',
+          ),
+          pht('Add Comment')))
       ->appendChild($image)
       ->appendChild($badge_view)
       ->appendChild($wedge)
diff --git a/src/view/phui/PHUITimelineView.php b/src/view/phui/PHUITimelineView.php
index 804b060d55..3353a2e2bd 100644
--- a/src/view/phui/PHUITimelineView.php
+++ b/src/view/phui/PHUITimelineView.php
@@ -83,7 +83,15 @@ final class PHUITimelineView extends AphrontView {
         'class' => 'phui-timeline-view',
         'id' => $this->id,
       ),
-      $events);
+      array(
+        phutil_tag(
+          'h3',
+          array(
+            'class' => 'aural-only',
+          ),
+          pht('Event Timeline')),
+        $events,
+      ));
   }
 
   public function buildEvents() {
diff --git a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js
index 3bfcd6c8d5..9cec374f6b 100644
--- a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js
+++ b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js
@@ -105,6 +105,7 @@ JX.behavior('comment-actions', function(config) {
       JX.DOM.setContent(
         preview_root,
         [
+          JX.$H(response.header),
           JX.$H(response.xactions.join('')),
           JX.$H(response.previewContent)
         ]);

From 2379c21fbbd1b4309fe78de98de814bff905b1f2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 08:10:38 -0700
Subject: [PATCH 06/25] Give Phriction documents a normal timeline

Summary:
Ref T13077. See PHI840. Ref T1894. I'm planning to just let you comment on Phriction documents. I think this will create a few problems (e.g., around popular documents which collect long comment threads that are eventually obsolete) but nothing should be too terribly critical (e.g., we handle it gracefully when objects have very large number of comments/transactions) and for most documents this is likely just a net improvement.

"Just enable comments" is probably not the final iteration on this, but I think it's probably a step forward on the balance, not a step sideways or a slippery slope down into a dark hole or anything.

Test Plan: {F5877316}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T1894

Differential Revision: https://secure.phabricator.com/D19659
---
 .../controller/PhrictionDocumentController.php   | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index f2cf974504..3cdcf7230c 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -348,6 +348,10 @@ final class PhrictionDocumentController
       $page_content->setCurtain($curtain);
     }
 
+    $timeline = $this->buildTransactionTimeline(
+      $document,
+      new PhrictionTransactionQuery());
+
     return $this->newPage()
       ->setTitle($page_title)
       ->setCrumbs($crumbs)
@@ -356,7 +360,15 @@ final class PhrictionDocumentController
         array(
           $page_content,
           $prop_list,
-          $children,
+          phutil_tag(
+            'div',
+            array(
+              'class' => 'phui-document-view-pro-box',
+            ),
+            array(
+              $children,
+              $timeline,
+            )),
         ));
 
   }
@@ -600,7 +612,7 @@ final class PhrictionDocumentController
           ),
           $list)));
 
-     return phutil_tag_div('phui-document-view-pro-box', $box);
+    return $box;
   }
 
   private function renderChildDocumentLink(array $info) {

From cc3b6d57907b7dc85a990a75ca5469353ac6fe49 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Sep 2018 12:05:21 -0700
Subject: [PATCH 07/25] Fix a "withHasTransactions()" typo in AuditEditor

Summary: See <https://discourse.phabricator-community.org/t/typo-in-phabricatorauditeditor-php/1910>. This is trivial and reproduces easily, I just missed it in testing.

Test Plan:
  - Left a comment on a commit which I was the author of.
  - Before change: fatal with obvious typo.
  - After change: smooth sailing.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19667
---
 src/applications/audit/editor/PhabricatorAuditEditor.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index d5c22fba6b..cfa84b8bd7 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -275,7 +275,7 @@ final class PhabricatorAuditEditor
 
           if ($actor_is_author) {
             $inlines[] = id(clone $query)
-              ->withHasTransaciton(true)
+              ->withHasTransaction(true)
               ->execute();
           }
 

From 09703938fb44b659fba28c18711ddc0870ba7032 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Sep 2018 12:48:13 -0700
Subject: [PATCH 08/25] Migrate old commit saved queries to new audit status
 constants

Summary: Depends on D19651. Ref T13197. The application now accepts either numeric or string values. However, for consistency and to reduce surprise in the future, migrate existing saved queries to use string values.

Test Plan: Saved some queries on `master` with numeric constants, ran the migration, saw string constants in the database and equivalent behavior in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19652
---
 .../20180910.audit.01.searches.php            | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 resources/sql/autopatches/20180910.audit.01.searches.php

diff --git a/resources/sql/autopatches/20180910.audit.01.searches.php b/resources/sql/autopatches/20180910.audit.01.searches.php
new file mode 100644
index 0000000000..f68e76fe45
--- /dev/null
+++ b/resources/sql/autopatches/20180910.audit.01.searches.php
@@ -0,0 +1,54 @@
+<?php
+
+$table = new PhabricatorSavedQuery();
+$conn = $table->establishConnection('w');
+
+$status_map = array(
+  0 => 'none',
+  1 => 'needs-audit',
+  2 => 'concern-raised',
+  3 => 'partially-audited',
+  4 => 'audited',
+  5 => 'needs-verification',
+);
+
+foreach (new LiskMigrationIterator($table) as $query) {
+  if ($query->getEngineClassName() !== 'PhabricatorCommitSearchEngine') {
+    continue;
+  }
+
+  $parameters = $query->getParameters();
+  $status = idx($parameters, 'statuses');
+
+  if (!$status) {
+    // No saved "status" constraint.
+    continue;
+  }
+
+  if (!is_array($status)) {
+    // Saved constraint isn't a list.
+    continue;
+  }
+
+  // Migrate old integer values to new string values.
+  $old_status = $status;
+  foreach ($status as $key => $value) {
+    if (is_numeric($value)) {
+      $status[$key] = $status_map[$value];
+    }
+  }
+
+  if ($status === $old_status) {
+    // Nothing changed.
+    continue;
+  }
+
+  $parameters['statuses'] = $status;
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET parameters = %s WHERE id = %d',
+    $table->getTableName(),
+    phutil_json_encode($parameters),
+    $query->getID());
+}

From d63281cc549e27978ec8ff6a4e2cbc1a157c93ee Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Sep 2018 13:01:37 -0700
Subject: [PATCH 09/25] Migrate remaining Audit database status constants

Summary: Depends on D19652. Ref T13197. See PHI851. This migrates the actual `auditStatus` on Commits, and older status transactions.

Test Plan:
  - Ran migrations.
  - Spot-checked the database for sanity.
  - Ran some different queries, got unchanged results from before migration.
  - Reviewed historic audit state transactions, and accepted/raised concern on new audits. All state transactions appeared to generate properly.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19655
---
 .../autopatches/20180910.audit.02.string.sql  |  2 +
 .../autopatches/20180910.audit.03.status.php  | 28 +++++++++++
 .../20180910.audit.04.xactions.php            | 48 +++++++++++++++++++
 .../PhabricatorAuditCommitStatusConstants.php | 25 +++++-----
 .../diffusion/query/DiffusionCommitQuery.php  | 10 ++--
 .../DiffusionCommitConcernTransaction.php     |  2 +-
 .../DiffusionCommitStateTransaction.php       |  2 +-
 .../DiffusionCommitVerifyTransaction.php      |  2 +-
 .../storage/PhabricatorRepositoryCommit.php   | 20 ++++----
 9 files changed, 107 insertions(+), 32 deletions(-)
 create mode 100644 resources/sql/autopatches/20180910.audit.02.string.sql
 create mode 100644 resources/sql/autopatches/20180910.audit.03.status.php
 create mode 100644 resources/sql/autopatches/20180910.audit.04.xactions.php

diff --git a/resources/sql/autopatches/20180910.audit.02.string.sql b/resources/sql/autopatches/20180910.audit.02.string.sql
new file mode 100644
index 0000000000..4caa4a1724
--- /dev/null
+++ b/resources/sql/autopatches/20180910.audit.02.string.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_commit
+  CHANGE auditStatus auditStatus VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT};
diff --git a/resources/sql/autopatches/20180910.audit.03.status.php b/resources/sql/autopatches/20180910.audit.03.status.php
new file mode 100644
index 0000000000..b7a0bf9f58
--- /dev/null
+++ b/resources/sql/autopatches/20180910.audit.03.status.php
@@ -0,0 +1,28 @@
+<?php
+
+$table = new PhabricatorRepositoryCommit();
+$conn = $table->establishConnection('w');
+
+$status_map = array(
+  0 => 'none',
+  1 => 'needs-audit',
+  2 => 'concern-raised',
+  3 => 'partially-audited',
+  4 => 'audited',
+  5 => 'needs-verification',
+);
+
+foreach (new LiskMigrationIterator($table) as $commit) {
+  $status = $commit->getAuditStatus();
+
+  if (!isset($status_map[$status])) {
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET auditStatus = %s WHERE id = %d',
+    $table->getTableName(),
+    $status_map[$status],
+    $commit->getID());
+}
diff --git a/resources/sql/autopatches/20180910.audit.04.xactions.php b/resources/sql/autopatches/20180910.audit.04.xactions.php
new file mode 100644
index 0000000000..1ecf9ef320
--- /dev/null
+++ b/resources/sql/autopatches/20180910.audit.04.xactions.php
@@ -0,0 +1,48 @@
+<?php
+
+$table = new PhabricatorAuditTransaction();
+$conn = $table->establishConnection('w');
+
+$status_map = array(
+  0 => 'none',
+  1 => 'needs-audit',
+  2 => 'concern-raised',
+  3 => 'partially-audited',
+  4 => 'audited',
+  5 => 'needs-verification',
+);
+
+$state_type = DiffusionCommitStateTransaction::TRANSACTIONTYPE;
+
+foreach (new LiskMigrationIterator($table) as $xaction) {
+  if ($xaction->getTransactionType() !== $state_type) {
+    continue;
+  }
+
+  $old_value = $xaction->getOldValue();
+  $new_value = $xaction->getNewValue();
+
+  $any_change = false;
+
+  if (isset($status_map[$old_value])) {
+    $old_value = $status_map[$old_value];
+    $any_change = true;
+  }
+
+  if (isset($status_map[$new_value])) {
+    $new_value = $status_map[$new_value];
+    $any_change = true;
+  }
+
+  if (!$any_change) {
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET oldValue = %s, newValue = %s WHERE id = %d',
+    $table->getTableName(),
+    phutil_json_encode($old_value),
+    phutil_json_encode($new_value),
+    $xaction->getID());
+}
diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
index eca420ce1b..23964920b7 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -19,18 +19,21 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
   const MODERN_AUDITED = 'audited';
   const MODERN_NEEDS_VERIFICATION = 'needs-verification';
 
-  public static function newForLegacyStatus($status) {
+  public static function newModernKeys(array $values) {
     $map = self::getMap();
 
-    if (is_int($status) || ctype_digit($status)) {
-      foreach ($map as $key => $spec) {
-        if ((int)idx($spec, 'legacy') === (int)$status) {
-          return self::newForStatus($key);
-        }
+    $modern = array();
+    foreach ($map as $key => $spec) {
+      if (isset($spec['legacy'])) {
+        $modern[$spec['legacy']] = $key;
       }
     }
 
-    return self::newForStatus($status);
+    foreach ($values as $key => $value) {
+      $values[$key] = idx($modern, $value, $value);
+    }
+
+    return $values;
   }
 
   public static function newForStatus($status) {
@@ -58,10 +61,6 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return idx($this->spec, 'color');
   }
 
-  public function getLegacyKey() {
-    return idx($this->spec, 'legacy');
-  }
-
   public function getName() {
     return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key));
   }
@@ -96,9 +95,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
 
   public static function getOpenStatusConstants() {
     $constants = array();
-    foreach (self::getMap() as $map) {
+    foreach (self::getMap() as $key => $map) {
       if (!$map['closed']) {
-        $constants[] = $map['legacy'];
+        $constants[] = $key;
       }
     }
     return $constants;
diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 9a7e21b0af..0a50e834c3 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -714,16 +714,12 @@ final class DiffusionCommitQuery
     }
 
     if ($this->statuses !== null) {
-      $statuses = array();
-      foreach ($this->statuses as $status) {
-        $object = PhabricatorAuditCommitStatusConstants::newForLegacyStatus(
-          $status);
-        $statuses[] = $object->getLegacyKey();
-      }
+      $statuses = PhabricatorAuditCommitStatusConstants::newModernKeys(
+        $this->statuses);
 
       $where[] = qsprintf(
         $conn,
-        'commit.auditStatus IN (%Ld)',
+        'commit.auditStatus IN (%Ls)',
         $statuses);
     }
 
diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
index 07194a2af3..21d688a481 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
@@ -34,7 +34,7 @@ final class DiffusionCommitConcernTransaction
     // NOTE: We force the commit directly into "Concern Raised" so that we
     // override a possible "Needs Verification" state.
     $object->setAuditStatus(
-      PhabricatorAuditCommitStatusConstants::CONCERN_RAISED);
+      PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED);
   }
 
   public function applyExternalEffects($object, $value) {
diff --git a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
index bcf6a9e382..ad8b94f548 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
@@ -13,7 +13,7 @@ final class DiffusionCommitStateTransaction
 
   private function getAuditStatusObject() {
     $new = $this->getNewValue();
-    return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($new);
+    return PhabricatorAuditCommitStatusConstants::newForStatus($new);
   }
 
   public function getIcon() {
diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
index 3e6abb705f..5464df7fa1 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
@@ -37,7 +37,7 @@ final class DiffusionCommitVerifyTransaction
 
   public function applyInternalEffects($object, $value) {
     $object->setAuditStatus(
-      PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION);
+      PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION);
   }
 
   protected function validateAction($object, PhabricatorUser $viewer) {
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 291c895231..7b9f38c388 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -27,7 +27,7 @@ final class PhabricatorRepositoryCommit
   protected $epoch;
   protected $mailKey;
   protected $authorPHID;
-  protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE;
+  protected $auditStatus = PhabricatorAuditCommitStatusConstants::MODERN_NONE;
   protected $summary = '';
   protected $importStatus = 0;
 
@@ -120,7 +120,7 @@ final class PhabricatorRepositoryCommit
         'authorPHID' => 'phid?',
         'authorIdentityPHID' => 'phid?',
         'committerIdentityPHID' => 'phid?',
-        'auditStatus' => 'uint32',
+        'auditStatus' => 'text32',
         'summary' => 'text255',
         'importStatus' => 'uint32',
       ),
@@ -385,20 +385,22 @@ final class PhabricatorRepositoryCommit
       if ($this->isAuditStatusNeedsVerification()) {
         // If the change is in "Needs Verification", we keep it there as
         // long as any auditors still have concerns.
-        $status = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
+        $status =
+          PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION;
       } else {
-        $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
+        $status = PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED;
       }
     } else if ($any_accept) {
       if ($any_need) {
-        $status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED;
+        $status =
+          PhabricatorAuditCommitStatusConstants::MODERN_PARTIALLY_AUDITED;
       } else {
-        $status = PhabricatorAuditCommitStatusConstants::FULLY_AUDITED;
+        $status = PhabricatorAuditCommitStatusConstants::MODERN_AUDITED;
       }
     } else if ($any_need) {
-      $status = PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT;
+      $status = PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT;
     } else {
-      $status = PhabricatorAuditCommitStatusConstants::NONE;
+      $status = PhabricatorAuditCommitStatusConstants::MODERN_NONE;
     }
 
     return $this->setAuditStatus($status);
@@ -529,7 +531,7 @@ final class PhabricatorRepositoryCommit
 
   public function getAuditStatusObject() {
     $status = $this->getAuditStatus();
-    return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($status);
+    return PhabricatorAuditCommitStatusConstants::newForStatus($status);
   }
 
   public function isAuditStatusNoAudit() {

From aaf22695515a3b2496f6763866b7856d32820121 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 07:13:08 -0700
Subject: [PATCH 10/25] Remove legacy numeric Audit status constants

Summary: Depends on D19655. Ref T13197. These no longer have callers.

Test Plan: Grepped for these constants.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19656
---
 .../PhabricatorAuditCommitStatusConstants.php | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
index 23964920b7..98b8324a38 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -5,13 +5,6 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
   private $key;
   private $spec = array();
 
-  const NONE                = 0;
-  const NEEDS_AUDIT         = 1;
-  const CONCERN_RAISED      = 2;
-  const PARTIALLY_AUDITED   = 3;
-  const FULLY_AUDITED       = 4;
-  const NEEDS_VERIFICATION = 5;
-
   const MODERN_NONE = 'none';
   const MODERN_NEEDS_AUDIT = 'needs-audit';
   const MODERN_CONCERN_RAISED = 'concern-raised';
@@ -125,42 +118,42 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return array(
       self::MODERN_NONE => array(
         'name' => pht('No Audits'),
-        'legacy' => self::NONE,
+        'legacy' => 0,
         'icon' => 'fa-check',
         'color' => 'bluegrey',
         'closed' => true,
       ),
       self::MODERN_NEEDS_AUDIT => array(
         'name' => pht('Audit Required'),
-        'legacy' => self::NEEDS_AUDIT,
+        'legacy' => 1,
         'icon' => 'fa-exclamation-circle',
         'color' => 'orange',
         'closed' => false,
       ),
       self::MODERN_CONCERN_RAISED => array(
         'name' => pht('Concern Raised'),
-        'legacy' => self::CONCERN_RAISED,
+        'legacy' => 2,
         'icon' => 'fa-times-circle',
         'color' => 'red',
         'closed' => false,
       ),
       self::MODERN_PARTIALLY_AUDITED => array(
         'name' => pht('Partially Audited'),
-        'legacy' => self::PARTIALLY_AUDITED,
+        'legacy' => 3,
         'icon' => 'fa-check-circle-o',
         'color' => 'yellow',
         'closed' => false,
       ),
       self::MODERN_AUDITED => array(
         'name' => pht('Audited'),
-        'legacy' => self::FULLY_AUDITED,
+        'legacy' => 4,
         'icon' => 'fa-check-circle',
         'color' => 'green',
         'closed' => true,
       ),
       self::MODERN_NEEDS_VERIFICATION => array(
         'name' => pht('Needs Verification'),
-        'legacy' => self::NEEDS_VERIFICATION,
+        'legacy' => 5,
         'icon' => 'fa-refresh',
         'color' => 'indigo',
         'closed' => false,

From c7e7b63f155f22d4e1915f81584338dfb8631733 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 07:19:37 -0700
Subject: [PATCH 11/25] Rename "PhabricatorAuditCommitStatusConstants" to
 "DiffusionCommitAuditStatus"; remove "MODERN_"

Summary:
Depends on D19656. Ref T13197. See PHI851.

  - This class is now a real object, so get rid of the "Constants" part of the name.
  - Rename it for greater consistency with other modern objects.
  - Get rid of the `MODERN_` tag now that the old constants are gone.

Test Plan: Bunch of `grep`, browsed around.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19657
---
 src/__phutil_library_map__.php                |  4 +-
 .../conduit/AuditQueryConduitAPIMethod.php    | 10 ++---
 .../query/PhabricatorCommitSearchEngine.php   |  6 +--
 .../DiffusionCommitAuditStatus.php}           | 38 +++++++++----------
 .../diffusion/query/DiffusionCommitQuery.php  |  2 +-
 .../DiffusionCommitConcernTransaction.php     |  3 +-
 .../DiffusionCommitStateTransaction.php       | 22 +++++------
 .../DiffusionCommitVerifyTransaction.php      |  3 +-
 .../PhabricatorOwnersDetailController.php     |  3 +-
 .../storage/PhabricatorRepositoryCommit.php   | 18 ++++-----
 10 files changed, 52 insertions(+), 57 deletions(-)
 rename src/applications/{audit/constants/PhabricatorAuditCommitStatusConstants.php => diffusion/DiffusionCommitAuditStatus.php} (76%)

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9774741060..ab454e0455 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -704,6 +704,7 @@ phutil_register_library_map(array(
     'DiffusionCommitAcceptTransaction' => 'applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php',
     'DiffusionCommitActionTransaction' => 'applications/diffusion/xaction/DiffusionCommitActionTransaction.php',
     'DiffusionCommitAffectedFilesHeraldField' => 'applications/diffusion/herald/DiffusionCommitAffectedFilesHeraldField.php',
+    'DiffusionCommitAuditStatus' => 'applications/diffusion/DiffusionCommitAuditStatus.php',
     'DiffusionCommitAuditTransaction' => 'applications/diffusion/xaction/DiffusionCommitAuditTransaction.php',
     'DiffusionCommitAuditorsHeraldField' => 'applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php',
     'DiffusionCommitAuditorsTransaction' => 'applications/diffusion/xaction/DiffusionCommitAuditorsTransaction.php',
@@ -2147,7 +2148,6 @@ phutil_register_library_map(array(
     'PhabricatorAuditActionConstants' => 'applications/audit/constants/PhabricatorAuditActionConstants.php',
     'PhabricatorAuditApplication' => 'applications/audit/application/PhabricatorAuditApplication.php',
     'PhabricatorAuditCommentEditor' => 'applications/audit/editor/PhabricatorAuditCommentEditor.php',
-    'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/PhabricatorAuditCommitStatusConstants.php',
     'PhabricatorAuditController' => 'applications/audit/controller/PhabricatorAuditController.php',
     'PhabricatorAuditEditor' => 'applications/audit/editor/PhabricatorAuditEditor.php',
     'PhabricatorAuditInlineComment' => 'applications/audit/storage/PhabricatorAuditInlineComment.php',
@@ -6066,6 +6066,7 @@ phutil_register_library_map(array(
     'DiffusionCommitAcceptTransaction' => 'DiffusionCommitAuditTransaction',
     'DiffusionCommitActionTransaction' => 'DiffusionCommitTransactionType',
     'DiffusionCommitAffectedFilesHeraldField' => 'DiffusionCommitHeraldField',
+    'DiffusionCommitAuditStatus' => 'Phobject',
     'DiffusionCommitAuditTransaction' => 'DiffusionCommitActionTransaction',
     'DiffusionCommitAuditorsHeraldField' => 'DiffusionCommitHeraldField',
     'DiffusionCommitAuditorsTransaction' => 'DiffusionCommitTransactionType',
@@ -7715,7 +7716,6 @@ phutil_register_library_map(array(
     'PhabricatorAuditActionConstants' => 'Phobject',
     'PhabricatorAuditApplication' => 'PhabricatorApplication',
     'PhabricatorAuditCommentEditor' => 'PhabricatorEditor',
-    'PhabricatorAuditCommitStatusConstants' => 'Phobject',
     'PhabricatorAuditController' => 'PhabricatorController',
     'PhabricatorAuditEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorAuditInlineComment' => array(
diff --git a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
index 6d29d2345d..d0d3779c2a 100644
--- a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
+++ b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php
@@ -68,17 +68,17 @@ final class AuditQueryConduitAPIMethod extends AuditConduitAPIMethod {
 
     $status_map = array(
       self::AUDIT_LEGACYSTATUS_OPEN => array(
-        PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT,
-        PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED,
+        DiffusionCommitAuditStatus::NEEDS_AUDIT,
+        DiffusionCommitAuditStatus::CONCERN_RAISED,
       ),
       self::AUDIT_LEGACYSTATUS_CONCERN => array(
-        PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED,
+        DiffusionCommitAuditStatus::CONCERN_RAISED,
       ),
       self::AUDIT_LEGACYSTATUS_ACCEPTED => array(
-        PhabricatorAuditCommitStatusConstants::MODERN_AUDITED,
+        DiffusionCommitAuditStatus::AUDITED,
       ),
       self::AUDIT_LEGACYSTATUS_PARTIAL => array(
-        PhabricatorAuditCommitStatusConstants::MODERN_PARTIALLY_AUDITED,
+        DiffusionCommitAuditStatus::PARTIALLY_AUDITED,
       ),
     );
 
diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
index 67861b7e7d..44413946db 100644
--- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php
+++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
@@ -92,9 +92,9 @@ final class PhabricatorCommitSearchEngine
         ->setLabel(pht('Audit Status'))
         ->setKey('statuses')
         ->setAliases(array('status'))
-        ->setOptions(PhabricatorAuditCommitStatusConstants::newOptions())
+        ->setOptions(DiffusionCommitAuditStatus::newOptions())
         ->setDeprecatedOptions(
-          PhabricatorAuditCommitStatusConstants::newDeprecatedOptions())
+          DiffusionCommitAuditStatus::newDeprecatedOptions())
         ->setDescription(pht('Find commits with given audit statuses.')),
       id(new PhabricatorSearchDatasourceField())
         ->setLabel(pht('Repositories'))
@@ -162,7 +162,7 @@ final class PhabricatorCommitSearchEngine
       case 'active':
         $bucket_key = DiffusionCommitRequiredActionResultBucket::BUCKETKEY;
 
-        $open = PhabricatorAuditCommitStatusConstants::getOpenStatusConstants();
+        $open = DiffusionCommitAuditStatus::getOpenStatusConstants();
 
         $query
           ->setParameter('responsiblePHIDs', array($viewer_phid))
diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/diffusion/DiffusionCommitAuditStatus.php
similarity index 76%
rename from src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
rename to src/applications/diffusion/DiffusionCommitAuditStatus.php
index 98b8324a38..ce8ec09d27 100644
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/diffusion/DiffusionCommitAuditStatus.php
@@ -1,16 +1,16 @@
 <?php
 
-final class PhabricatorAuditCommitStatusConstants extends Phobject {
+final class DiffusionCommitAuditStatus extends Phobject {
 
   private $key;
   private $spec = array();
 
-  const MODERN_NONE = 'none';
-  const MODERN_NEEDS_AUDIT = 'needs-audit';
-  const MODERN_CONCERN_RAISED = 'concern-raised';
-  const MODERN_PARTIALLY_AUDITED = 'partially-audited';
-  const MODERN_AUDITED = 'audited';
-  const MODERN_NEEDS_VERIFICATION = 'needs-verification';
+  const NONE = 'none';
+  const NEEDS_AUDIT = 'needs-audit';
+  const CONCERN_RAISED = 'concern-raised';
+  const PARTIALLY_AUDITED = 'partially-audited';
+  const AUDITED = 'audited';
+  const NEEDS_VERIFICATION = 'needs-verification';
 
   public static function newModernKeys(array $values) {
     $map = self::getMap();
@@ -59,27 +59,27 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
   }
 
   public function isNoAudit() {
-    return ($this->key == self::MODERN_NONE);
+    return ($this->key == self::NONE);
   }
 
   public function isNeedsAudit() {
-    return ($this->key == self::MODERN_NEEDS_AUDIT);
+    return ($this->key == self::NEEDS_AUDIT);
   }
 
   public function isConcernRaised() {
-    return ($this->key == self::MODERN_CONCERN_RAISED);
+    return ($this->key == self::CONCERN_RAISED);
   }
 
   public function isNeedsVerification() {
-    return ($this->key == self::MODERN_NEEDS_VERIFICATION);
+    return ($this->key == self::NEEDS_VERIFICATION);
   }
 
   public function isPartiallyAudited() {
-    return ($this->key == self::MODERN_PARTIALLY_AUDITED);
+    return ($this->key == self::PARTIALLY_AUDITED);
   }
 
   public function isAudited() {
-    return ($this->key == self::MODERN_AUDITED);
+    return ($this->key == self::AUDITED);
   }
 
   public function getIsClosed() {
@@ -116,42 +116,42 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
 
   private static function getMap() {
     return array(
-      self::MODERN_NONE => array(
+      self::NONE => array(
         'name' => pht('No Audits'),
         'legacy' => 0,
         'icon' => 'fa-check',
         'color' => 'bluegrey',
         'closed' => true,
       ),
-      self::MODERN_NEEDS_AUDIT => array(
+      self::NEEDS_AUDIT => array(
         'name' => pht('Audit Required'),
         'legacy' => 1,
         'icon' => 'fa-exclamation-circle',
         'color' => 'orange',
         'closed' => false,
       ),
-      self::MODERN_CONCERN_RAISED => array(
+      self::CONCERN_RAISED => array(
         'name' => pht('Concern Raised'),
         'legacy' => 2,
         'icon' => 'fa-times-circle',
         'color' => 'red',
         'closed' => false,
       ),
-      self::MODERN_PARTIALLY_AUDITED => array(
+      self::PARTIALLY_AUDITED => array(
         'name' => pht('Partially Audited'),
         'legacy' => 3,
         'icon' => 'fa-check-circle-o',
         'color' => 'yellow',
         'closed' => false,
       ),
-      self::MODERN_AUDITED => array(
+      self::AUDITED => array(
         'name' => pht('Audited'),
         'legacy' => 4,
         'icon' => 'fa-check-circle',
         'color' => 'green',
         'closed' => true,
       ),
-      self::MODERN_NEEDS_VERIFICATION => array(
+      self::NEEDS_VERIFICATION => array(
         'name' => pht('Needs Verification'),
         'legacy' => 5,
         'icon' => 'fa-refresh',
diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 0a50e834c3..f2d0920b3a 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -714,7 +714,7 @@ final class DiffusionCommitQuery
     }
 
     if ($this->statuses !== null) {
-      $statuses = PhabricatorAuditCommitStatusConstants::newModernKeys(
+      $statuses = DiffusionCommitAuditStatus::newModernKeys(
         $this->statuses);
 
       $where[] = qsprintf(
diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
index 21d688a481..1f6ae72621 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
@@ -33,8 +33,7 @@ final class DiffusionCommitConcernTransaction
   public function applyInternalEffects($object, $value) {
     // NOTE: We force the commit directly into "Concern Raised" so that we
     // override a possible "Needs Verification" state.
-    $object->setAuditStatus(
-      PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED);
+    $object->setAuditStatus(DiffusionCommitAuditStatus::CONCERN_RAISED);
   }
 
   public function applyExternalEffects($object, $value) {
diff --git a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
index ad8b94f548..6fd2a6977d 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
@@ -13,7 +13,7 @@ final class DiffusionCommitStateTransaction
 
   private function getAuditStatusObject() {
     $new = $this->getNewValue();
-    return PhabricatorAuditCommitStatusConstants::newForStatus($new);
+    return DiffusionCommitAuditStatus::newForStatus($new);
   }
 
   public function getIcon() {
@@ -28,15 +28,15 @@ final class DiffusionCommitStateTransaction
     $status = $this->getAuditStatusObject();
 
     switch ($status->getKey()) {
-      case PhabricatorAuditCommitStatusConstants::MODERN_NONE:
+      case DiffusionCommitAuditStatus::NONE:
         return pht('This commit no longer requires audit.');
-      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT:
+      case DiffusionCommitAuditStatus::NEEDS_AUDIT:
         return pht('This commit now requires audit.');
-      case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED:
+      case DiffusionCommitAuditStatus::CONCERN_RAISED:
         return pht('This commit now has outstanding concerns.');
-      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION:
+      case DiffusionCommitAuditStatus::NEEDS_VERIFICATION:
         return pht('This commit now requires verification by auditors.');
-      case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED:
+      case DiffusionCommitAuditStatus::AUDITED:
         return pht('All concerns with this commit have now been addressed.');
     }
 
@@ -47,23 +47,23 @@ final class DiffusionCommitStateTransaction
     $status = $this->getAuditStatusObject();
 
     switch ($status->getKey()) {
-      case PhabricatorAuditCommitStatusConstants::MODERN_NONE:
+      case DiffusionCommitAuditStatus::NONE:
         return pht(
           '%s no longer requires audit.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT:
+      case DiffusionCommitAuditStatus::NEEDS_AUDIT:
         return pht(
           '%s now requires audit.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED:
+      case DiffusionCommitAuditStatus::CONCERN_RAISED:
         return pht(
           '%s now has outstanding concerns.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION:
+      case DiffusionCommitAuditStatus::NEEDS_VERIFICATION:
         return pht(
           '%s now requires verification by auditors.',
           $this->renderObject());
-      case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED:
+      case DiffusionCommitAuditStatus::AUDITED:
         return pht(
           'All concerns with %s have now been addressed.',
           $this->renderObject());
diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
index 5464df7fa1..f1ec5834b1 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
@@ -36,8 +36,7 @@ final class DiffusionCommitVerifyTransaction
   }
 
   public function applyInternalEffects($object, $value) {
-    $object->setAuditStatus(
-      PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION);
+    $object->setAuditStatus(DiffusionCommitAuditStatus::NEEDS_VERIFICATION);
   }
 
   protected function validateAction($object, PhabricatorUser $viewer) {
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index bdc4a2e475..f71009cf19 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -71,8 +71,7 @@ final class PhabricatorOwnersDetailController
           'package' => $package->getPHID(),
         ));
 
-    $status_concern =
-      PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED;
+    $status_concern = DiffusionCommitAuditStatus::CONCERN_RAISED;
 
     $attention_commits = id(new DiffusionCommitQuery())
       ->setViewer($request->getUser())
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 7b9f38c388..38a6631200 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -27,7 +27,7 @@ final class PhabricatorRepositoryCommit
   protected $epoch;
   protected $mailKey;
   protected $authorPHID;
-  protected $auditStatus = PhabricatorAuditCommitStatusConstants::MODERN_NONE;
+  protected $auditStatus = DiffusionCommitAuditStatus::NONE;
   protected $summary = '';
   protected $importStatus = 0;
 
@@ -385,22 +385,20 @@ final class PhabricatorRepositoryCommit
       if ($this->isAuditStatusNeedsVerification()) {
         // If the change is in "Needs Verification", we keep it there as
         // long as any auditors still have concerns.
-        $status =
-          PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION;
+        $status = DiffusionCommitAuditStatus::NEEDS_VERIFICATION;
       } else {
-        $status = PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED;
+        $status = DiffusionCommitAuditStatus::CONCERN_RAISED;
       }
     } else if ($any_accept) {
       if ($any_need) {
-        $status =
-          PhabricatorAuditCommitStatusConstants::MODERN_PARTIALLY_AUDITED;
+        $status = DiffusionCommitAuditStatus::PARTIALLY_AUDITED;
       } else {
-        $status = PhabricatorAuditCommitStatusConstants::MODERN_AUDITED;
+        $status = DiffusionCommitAuditStatus::AUDITED;
       }
     } else if ($any_need) {
-      $status = PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT;
+      $status = DiffusionCommitAuditStatus::NEEDS_AUDIT;
     } else {
-      $status = PhabricatorAuditCommitStatusConstants::MODERN_NONE;
+      $status = DiffusionCommitAuditStatus::NONE;
     }
 
     return $this->setAuditStatus($status);
@@ -531,7 +529,7 @@ final class PhabricatorRepositoryCommit
 
   public function getAuditStatusObject() {
     $status = $this->getAuditStatus();
-    return PhabricatorAuditCommitStatusConstants::newForStatus($status);
+    return DiffusionCommitAuditStatus::newForStatus($status);
   }
 
   public function isAuditStatusNoAudit() {

From 8268abcb78803c03a4d1f7a7f82a3baf325c9d74 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 07:41:22 -0700
Subject: [PATCH 12/25] Enrich "diffusion.commit.search" with identity, status,
 and message information

Summary:
Depends on D19657. Ref T13197. See PHI841.

This enriches the results from `diffusion.commit.search` with information similar to the information returned by the "commits" attachment from `differential.diff.search`.

Also include unreachable, imported, message, audit status, and repository PHID.

Test Plan: Called `diffusion.commit.search` and reviewed the results, which looked sensible.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19658
---
 .../query/PhabricatorCommitSearchEngine.php   |  1 +
 .../diffusion/DiffusionCommitAuditStatus.php  | 10 ++
 .../storage/PhabricatorRepositoryCommit.php   | 98 ++++++++++++++++++-
 3 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
index 44413946db..ee5105f193 100644
--- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php
+++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php
@@ -15,6 +15,7 @@ final class PhabricatorCommitSearchEngine
     return id(new DiffusionCommitQuery())
       ->needAuditRequests(true)
       ->needCommitData(true)
+      ->needIdentities(true)
       ->needDrafts(true);
   }
 
diff --git a/src/applications/diffusion/DiffusionCommitAuditStatus.php b/src/applications/diffusion/DiffusionCommitAuditStatus.php
index ce8ec09d27..95d0e52213 100644
--- a/src/applications/diffusion/DiffusionCommitAuditStatus.php
+++ b/src/applications/diffusion/DiffusionCommitAuditStatus.php
@@ -54,6 +54,10 @@ final class DiffusionCommitAuditStatus extends Phobject {
     return idx($this->spec, 'color');
   }
 
+  public function getAnsiColor() {
+    return idx($this->spec, 'color.ansi');
+  }
+
   public function getName() {
     return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key));
   }
@@ -122,6 +126,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-check',
         'color' => 'bluegrey',
         'closed' => true,
+        'color.ansi' => null,
       ),
       self::NEEDS_AUDIT => array(
         'name' => pht('Audit Required'),
@@ -129,6 +134,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-exclamation-circle',
         'color' => 'orange',
         'closed' => false,
+        'color.ansi' => 'magenta',
       ),
       self::CONCERN_RAISED => array(
         'name' => pht('Concern Raised'),
@@ -136,6 +142,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-times-circle',
         'color' => 'red',
         'closed' => false,
+        'color.ansi' => 'red',
       ),
       self::PARTIALLY_AUDITED => array(
         'name' => pht('Partially Audited'),
@@ -143,6 +150,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-check-circle-o',
         'color' => 'yellow',
         'closed' => false,
+        'color.ansi' => 'yellow',
       ),
       self::AUDITED => array(
         'name' => pht('Audited'),
@@ -150,6 +158,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-check-circle',
         'color' => 'green',
         'closed' => true,
+        'color.ansi' => 'green',
       ),
       self::NEEDS_VERIFICATION => array(
         'name' => pht('Needs Verification'),
@@ -157,6 +166,7 @@ final class DiffusionCommitAuditStatus extends Phobject {
         'icon' => 'fa-refresh',
         'color' => 'indigo',
         'closed' => false,
+        'color.ansi' => 'magenta',
       ),
     );
   }
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 38a6631200..0362aa778c 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -851,16 +851,110 @@ final class PhabricatorRepositoryCommit
         ->setKey('identifier')
         ->setType('string')
         ->setDescription(pht('The commit identifier.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('repositoryPHID')
+        ->setType('phid')
+        ->setDescription(pht('The repository this commit belongs to.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('author')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Information about the commit author.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('committer')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Information about the committer.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('isImported')
+        ->setType('bool')
+        ->setDescription(pht('True if the commit is fully imported.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('isUnreachable')
+        ->setType('bool')
+        ->setDescription(
+          pht(
+            'True if the commit is not the ancestor of any tag, branch, or '.
+            'ref.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('auditStatus')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Information about the current audit status.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('message')
+        ->setType('string')
+        ->setDescription(pht('The commit message.')),
     );
   }
 
   public function getFieldValuesForConduit() {
+    $data = $this->getCommitData();
 
-    // NOTE: This data should be similar to the information returned about
-    // commmits by "differential.diff.search" with the "commits" attachment.
+    $author_identity = $this->getAuthorIdentity();
+    if ($author_identity) {
+      $author_name = $author_identity->getIdentityDisplayName();
+      $author_email = $author_identity->getIdentityEmailAddress();
+      $author_raw = $author_identity->getIdentityName();
+      $author_identity_phid = $author_identity->getPHID();
+      $author_user_phid = $author_identity->getCurrentEffectiveUserPHID();
+    } else {
+      $author_name = null;
+      $author_email = null;
+      $author_raw = null;
+      $author_identity_phid = null;
+      $author_user_phid = null;
+    }
+
+    $committer_identity = $this->getCommitterIdentity();
+    if ($committer_identity) {
+      $committer_name = $committer_identity->getIdentityDisplayName();
+      $committer_email = $committer_identity->getIdentityEmailAddress();
+      $committer_raw = $committer_identity->getIdentityName();
+      $committer_identity_phid = $committer_identity->getPHID();
+      $committer_user_phid = $committer_identity->getCurrentEffectiveUserPHID();
+    } else {
+      $committer_name = null;
+      $committer_email = null;
+      $committer_raw = null;
+      $committer_identity_phid = null;
+      $committer_user_phid = null;
+    }
+
+    $author_epoch = $data->getCommitDetail('authorEpoch');
+    if ($author_epoch) {
+      $author_epoch = (int)$author_epoch;
+    } else {
+      $author_epoch = null;
+    }
+
+    $audit_status = $this->getAuditStatusObject();
 
     return array(
       'identifier' => $this->getCommitIdentifier(),
+      'repositoryPHID' => $this->getRepository()->getPHID(),
+      'author' => array(
+        'name' => $author_name,
+        'email' => $author_email,
+        'raw' => $author_raw,
+        'epoch' => $author_epoch,
+        'identityPHID' => $author_identity_phid,
+        'userPHID' => $author_user_phid,
+      ),
+      'committer' => array(
+        'name' => $committer_name,
+        'email' => $committer_email,
+        'raw' => $committer_raw,
+        'epoch' => (int)$this->getEpoch(),
+        'identityPHID' => $committer_identity_phid,
+        'userPHID' => $committer_user_phid,
+      ),
+      'isUnreachable' => (bool)$this->isUnreachable(),
+      'isImported' => (bool)$this->isImported(),
+      'auditStatus' => array(
+        'value' => $audit_status->getKey(),
+        'name' => $audit_status->getName(),
+        'closed' => (bool)$audit_status->getIsClosed(),
+        'color.ansi' => $audit_status->getAnsiColor(),
+      ),
+      'message' => $data->getCommitMessage(),
     );
   }
 

From f5e90a363e7a9444e871492bca4fa3b86a9b3cae Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 13:47:15 -0700
Subject: [PATCH 13/25] When a user takes actions while in a high security
 session, note it on the resulting transactions

Summary:
Ref T13197. See PHI873. Record when a user has MFA'd and add a little icon to the transaction, similar to the exiting "Silent" icon.

For now, this just makes this stuff more auditable. Future changes may add ways to require MFA for certain specific transactions, outside of the ones that already always require MFA (like revealing credentials).

Test Plan: {F5877960}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19665
---
 .../engine/PhabricatorAuthSessionEngine.php    |  3 +--
 .../auth/storage/PhabricatorAuthSession.php    | 16 ++++++++++++++++
 .../people/storage/PhabricatorUser.php         |  8 ++++++++
 ...PhabricatorApplicationTransactionEditor.php |  4 ++++
 .../PhabricatorApplicationTransaction.php      | 14 ++++++++++++++
 .../PhabricatorApplicationTransactionView.php  |  3 ++-
 src/view/phui/PHUITimelineEventView.php        | 18 ++++++++++++++++++
 7 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index 82303fff2b..6e40cdde98 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -490,8 +490,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     PhabricatorAuthSession $session,
     $force = false) {
 
-    $until = $session->getHighSecurityUntil();
-    if ($until > time() || $force) {
+    if ($session->isHighSecuritySession() || $force) {
       return new PhabricatorAuthHighSecurityToken();
     }
 
diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php
index b0b4996c93..cf707a053d 100644
--- a/src/applications/auth/storage/PhabricatorAuthSession.php
+++ b/src/applications/auth/storage/PhabricatorAuthSession.php
@@ -74,6 +74,22 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
     }
   }
 
+  public function isHighSecuritySession() {
+    $until = $this->getHighSecurityUntil();
+
+    if (!$until) {
+      return false;
+    }
+
+    $now = PhabricatorTime::getNow();
+    if ($until < $now) {
+      return false;
+    }
+
+    return true;
+  }
+
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 03da9dee04..cd85800ab1 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -306,6 +306,14 @@ final class PhabricatorUser
     return ($this->session !== self::ATTACHABLE);
   }
 
+  public function hasHighSecuritySession() {
+    if (!$this->hasSession()) {
+      return false;
+    }
+
+    return $this->getSession()->isHighSecuritySession();
+  }
+
   private function generateConduitCertificate() {
     return Filesystem::readRandomCharacters(255);
   }
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 1873fcb6e4..b4dd09ed10 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -850,6 +850,10 @@ abstract class PhabricatorApplicationTransactionEditor
       $xaction->setIsSilentTransaction(true);
     }
 
+    if ($actor->hasHighSecuritySession()) {
+      $xaction->setIsMFATransaction(true);
+    }
+
     return $xaction;
   }
 
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index ed88ac98db..c65adcac6b 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -165,6 +165,14 @@ abstract class PhabricatorApplicationTransaction
     return (bool)$this->getMetadataValue('core.silent', false);
   }
 
+  public function setIsMFATransaction($mfa) {
+    return $this->setMetadataValue('core.mfa', $mfa);
+  }
+
+  public function getIsMFATransaction() {
+    return (bool)$this->getMetadataValue('core.mfa', false);
+  }
+
   public function attachComment(
     PhabricatorApplicationTransactionComment $comment) {
     $this->comment = $comment;
@@ -1461,6 +1469,12 @@ abstract class PhabricatorApplicationTransaction
       if ($is_silent != $xaction->getIsSilentTransaction()) {
         return false;
       }
+
+      // Don't group MFA and non-MFA transactions together.
+      $is_mfa = $this->getIsMFATransaction();
+      if ($is_mfa != $xaction->getIsMFATransaction()) {
+        return false;
+      }
     }
 
     return true;
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
index e10f5c008e..9916628edf 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@@ -424,7 +424,8 @@ class PhabricatorApplicationTransactionView extends AphrontView {
       ->setIcon($xaction->getIcon())
       ->setColor($xaction->getColor())
       ->setHideCommentOptions($this->getHideCommentOptions())
-      ->setIsSilent($xaction->getIsSilentTransaction());
+      ->setIsSilent($xaction->getIsSilentTransaction())
+      ->setIsMFA($xaction->getIsMFATransaction());
 
     list($token, $token_removed) = $xaction->getToken();
     if ($token) {
diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php
index e44332c1b5..79aeb454b6 100644
--- a/src/view/phui/PHUITimelineEventView.php
+++ b/src/view/phui/PHUITimelineEventView.php
@@ -30,6 +30,7 @@ final class PHUITimelineEventView extends AphrontView {
   private $badges = array();
   private $pinboardItems = array();
   private $isSilent;
+  private $isMFA;
 
   public function setAuthorPHID($author_phid) {
     $this->authorPHID = $author_phid;
@@ -187,6 +188,15 @@ final class PHUITimelineEventView extends AphrontView {
     return $this->isSilent;
   }
 
+  public function setIsMFA($is_mfa) {
+    $this->isMFA = $is_mfa;
+    return $this;
+  }
+
+  public function getIsMFA() {
+    return $this->isMFA;
+  }
+
   public function setReallyMajorEvent($me) {
     $this->reallyMajorEvent = $me;
     return $this;
@@ -590,6 +600,14 @@ final class PHUITimelineEventView extends AphrontView {
           ->setIcon('fa-bell-slash', 'red')
           ->setTooltip(pht('Silent Edit'));
       }
+
+      // If this edit was applied while the actor was in high-security mode,
+      // provide a hint that it was extra authentic.
+      if ($this->getIsMFA()) {
+        $extra[] = id(new PHUIIconView())
+          ->setIcon('fa-vcard', 'green')
+          ->setTooltip(pht('MFA Authenticated'));
+      }
     }
 
     $extra = javelin_tag(

From e19c555913b1da40e178ce6174cd857d5492cc4d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 08:35:22 -0700
Subject: [PATCH 14/25] Support (basic) commenting on Phriction documents

Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.

  - Add an EditEngine, although it currently supports no fields.
  - Add (basic, top-level-only) commenting (we already had the table in the database).

This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.

This also moves us incrementally toward putting all editing on top of EditEngine.

Test Plan: {F5877347}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T1894

Differential Revision: https://secure.phabricator.com/D19660
---
 resources/celerity/map.php                    |  4 +-
 src/__phutil_library_map__.php                |  4 +
 .../PhabricatorPhrictionApplication.php       |  3 +
 .../PhrictionDocumentController.php           |  8 ++
 .../PhrictionEditEngineController.php         | 14 +++
 .../editor/PhrictionDocumentEditEngine.php    | 85 +++++++++++++++++++
 ...catorApplicationTransactionCommentView.php |  1 +
 src/view/phui/PHUIObjectBoxView.php           | 10 ++-
 webroot/rsrc/css/phui/phui-document-pro.css   |  5 +-
 9 files changed, 130 insertions(+), 4 deletions(-)
 create mode 100644 src/applications/phriction/controller/PhrictionEditEngineController.php
 create mode 100644 src/applications/phriction/editor/PhrictionDocumentEditEngine.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 39fe047f19..1b4fd3517a 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -146,7 +146,7 @@ return array(
     'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad',
     'rsrc/css/phui/phui-crumbs-view.css' => '10728aaa',
     'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026',
-    'rsrc/css/phui/phui-document-pro.css' => '1a08ef4b',
+    'rsrc/css/phui/phui-document-pro.css' => 'd033e8d5',
     'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf',
     'rsrc/css/phui/phui-document.css' => 'c4ac41f9',
     'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9',
@@ -814,7 +814,7 @@ return array(
     'phui-curtain-view-css' => '2bdaf026',
     'phui-document-summary-view-css' => '9ca48bdf',
     'phui-document-view-css' => 'c4ac41f9',
-    'phui-document-view-pro-css' => '1a08ef4b',
+    'phui-document-view-pro-css' => 'd033e8d5',
     'phui-feed-story-css' => '44a9c8e9',
     'phui-font-icon-base-css' => '870a7360',
     'phui-fontkit-css' => '1320ed01',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index ab454e0455..e6146bc9ef 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -5021,6 +5021,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentController' => 'applications/phriction/controller/PhrictionDocumentController.php',
     'PhrictionDocumentDatasource' => 'applications/phriction/typeahead/PhrictionDocumentDatasource.php',
     'PhrictionDocumentDeleteTransaction' => 'applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php',
+    'PhrictionDocumentEditEngine' => 'applications/phriction/editor/PhrictionDocumentEditEngine.php',
     'PhrictionDocumentFerretEngine' => 'applications/phriction/search/PhrictionDocumentFerretEngine.php',
     'PhrictionDocumentFulltextEngine' => 'applications/phriction/search/PhrictionDocumentFulltextEngine.php',
     'PhrictionDocumentHeraldAdapter' => 'applications/phriction/herald/PhrictionDocumentHeraldAdapter.php',
@@ -5042,6 +5043,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentVersionTransaction' => 'applications/phriction/xaction/PhrictionDocumentVersionTransaction.php',
     'PhrictionEditConduitAPIMethod' => 'applications/phriction/conduit/PhrictionEditConduitAPIMethod.php',
     'PhrictionEditController' => 'applications/phriction/controller/PhrictionEditController.php',
+    'PhrictionEditEngineController' => 'applications/phriction/controller/PhrictionEditEngineController.php',
     'PhrictionHistoryConduitAPIMethod' => 'applications/phriction/conduit/PhrictionHistoryConduitAPIMethod.php',
     'PhrictionHistoryController' => 'applications/phriction/controller/PhrictionHistoryController.php',
     'PhrictionInfoConduitAPIMethod' => 'applications/phriction/conduit/PhrictionInfoConduitAPIMethod.php',
@@ -11140,6 +11142,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentController' => 'PhrictionController',
     'PhrictionDocumentDatasource' => 'PhabricatorTypeaheadDatasource',
     'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentVersionTransaction',
+    'PhrictionDocumentEditEngine' => 'PhabricatorEditEngine',
     'PhrictionDocumentFerretEngine' => 'PhabricatorFerretEngine',
     'PhrictionDocumentFulltextEngine' => 'PhabricatorFulltextEngine',
     'PhrictionDocumentHeraldAdapter' => 'HeraldAdapter',
@@ -11161,6 +11164,7 @@ phutil_register_library_map(array(
     'PhrictionDocumentVersionTransaction' => 'PhrictionDocumentTransactionType',
     'PhrictionEditConduitAPIMethod' => 'PhrictionConduitAPIMethod',
     'PhrictionEditController' => 'PhrictionController',
+    'PhrictionEditEngineController' => 'PhrictionController',
     'PhrictionHistoryConduitAPIMethod' => 'PhrictionConduitAPIMethod',
     'PhrictionHistoryController' => 'PhrictionController',
     'PhrictionInfoConduitAPIMethod' => 'PhrictionConduitAPIMethod',
diff --git a/src/applications/phriction/application/PhabricatorPhrictionApplication.php b/src/applications/phriction/application/PhabricatorPhrictionApplication.php
index d44845c4b9..02f285aef3 100644
--- a/src/applications/phriction/application/PhabricatorPhrictionApplication.php
+++ b/src/applications/phriction/application/PhabricatorPhrictionApplication.php
@@ -63,6 +63,9 @@ final class PhabricatorPhrictionApplication extends PhabricatorApplication {
 
         'preview/(?P<slug>.*/)' => 'PhrictionMarkupPreviewController',
         'diff/(?P<id>[1-9]\d*)/' => 'PhrictionDiffController',
+
+        $this->getEditRoutePattern('document/edit/')
+          => 'PhrictionEditEngineController',
       ),
     );
   }
diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index 3cdcf7230c..d4c9d0b263 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -352,6 +352,13 @@ final class PhrictionDocumentController
       $document,
       new PhrictionTransactionQuery());
 
+    $edit_engine = id(new PhrictionDocumentEditEngine())
+      ->setViewer($viewer)
+      ->setTargetObject($document);
+
+    $comment_view = $edit_engine
+      ->buildEditEngineCommentView($document);
+
     return $this->newPage()
       ->setTitle($page_title)
       ->setCrumbs($crumbs)
@@ -368,6 +375,7 @@ final class PhrictionDocumentController
             array(
               $children,
               $timeline,
+              $comment_view,
             )),
         ));
 
diff --git a/src/applications/phriction/controller/PhrictionEditEngineController.php b/src/applications/phriction/controller/PhrictionEditEngineController.php
new file mode 100644
index 0000000000..f2b7722eab
--- /dev/null
+++ b/src/applications/phriction/controller/PhrictionEditEngineController.php
@@ -0,0 +1,14 @@
+<?php
+
+final class PhrictionEditEngineController
+  extends PhrictionController {
+
+  public function handleRequest(AphrontRequest $request) {
+    // NOTE: For now, this controller is only used to handle comments.
+
+    return id(new PhrictionDocumentEditEngine())
+      ->setController($this)
+      ->buildResponse();
+  }
+
+}
diff --git a/src/applications/phriction/editor/PhrictionDocumentEditEngine.php b/src/applications/phriction/editor/PhrictionDocumentEditEngine.php
new file mode 100644
index 0000000000..22f56f2f57
--- /dev/null
+++ b/src/applications/phriction/editor/PhrictionDocumentEditEngine.php
@@ -0,0 +1,85 @@
+<?php
+
+final class PhrictionDocumentEditEngine
+  extends PhabricatorEditEngine {
+
+  const ENGINECONST = 'phriction.document';
+
+  public function isEngineConfigurable() {
+    return false;
+  }
+
+  public function getEngineName() {
+    return pht('Phriction Document');
+  }
+
+  public function getSummaryHeader() {
+    return pht('Edit Phriction Document Configurations');
+  }
+
+  public function getSummaryText() {
+    return pht('This engine is used to edit Phriction documents.');
+  }
+
+  public function getEngineApplicationClass() {
+    return 'PhabricatorPhrictionApplication';
+  }
+
+  protected function newEditableObject() {
+    $viewer = $this->getViewer();
+    return PhrictionDocument::initializeNewDocument(
+      $viewer,
+      '/');
+  }
+
+  protected function newObjectQuery() {
+    return id(new PhrictionDocumentQuery())
+      ->needContent(true);
+  }
+
+  protected function getObjectCreateTitleText($object) {
+    return pht('Create Document');
+  }
+
+  protected function getObjectCreateButtonText($object) {
+    return pht('Create Document');
+  }
+
+  protected function getObjectEditTitleText($object) {
+    return pht('Edit Document: %s', $object->getContent()->getTitle());
+  }
+
+  protected function getObjectEditShortText($object) {
+    return pht('Edit Document');
+  }
+
+  protected function getObjectCreateShortText() {
+    return pht('Create Document');
+  }
+
+  protected function getObjectName() {
+    return pht('Document');
+  }
+
+  protected function getEditorURI() {
+    return '/phriction/document/edit/';
+  }
+
+  protected function getObjectCreateCancelURI($object) {
+    return '/phriction/document/';
+  }
+
+  protected function getObjectViewURI($object) {
+    return $object->getURI();
+  }
+
+  protected function getCreateNewObjectPolicy() {
+    // NOTE: For now, this engine is only to support commenting.
+    return PhabricatorPolicies::POLICY_NOONE;
+  }
+
+  protected function buildCustomEditFields($object) {
+    return array();
+  }
+
+}
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
index fcdc2fcb31..8be45f8cde 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
@@ -243,6 +243,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
 
     $comment_box = id(new PHUIObjectBoxView())
       ->setFlush(true)
+      ->setNoBorder(true)
       ->addClass('phui-comment-form-view')
       ->addSigil('phui-comment-form')
       ->appendChild(
diff --git a/src/view/phui/PHUIObjectBoxView.php b/src/view/phui/PHUIObjectBoxView.php
index f5fdbfffc4..f19c485a36 100644
--- a/src/view/phui/PHUIObjectBoxView.php
+++ b/src/view/phui/PHUIObjectBoxView.php
@@ -25,6 +25,7 @@ final class PHUIObjectBoxView extends AphrontTagView {
   private $showHideHref;
   private $showHideContent;
   private $showHideOpen;
+  private $noBorder;
 
   private $propertyLists = array();
 
@@ -147,6 +148,11 @@ final class PHUIObjectBoxView extends AphrontTagView {
     return $this;
   }
 
+  public function setNoBorder($no_border) {
+    $this->noBorder = $no_border;
+    return $this;
+  }
+
   public function setValidationException(
     PhabricatorApplicationTransactionValidationException $ex = null) {
     $this->validationException = $ex;
@@ -156,7 +162,9 @@ final class PHUIObjectBoxView extends AphrontTagView {
   protected function getTagAttributes() {
     $classes = array();
     $classes[] = 'phui-box';
-    $classes[] = 'phui-box-border';
+    if (!$this->noBorder) {
+      $classes[] = 'phui-box-border';
+    }
     $classes[] = 'phui-object-box';
     $classes[] = 'mlt mll mlr';
 
diff --git a/webroot/rsrc/css/phui/phui-document-pro.css b/webroot/rsrc/css/phui/phui-document-pro.css
index 4d58d17cb1..520ae597cb 100644
--- a/webroot/rsrc/css/phui/phui-document-pro.css
+++ b/webroot/rsrc/css/phui/phui-document-pro.css
@@ -241,10 +241,13 @@ body.printable {
   }
 }
 
+.phui-document-view-pro-box {
+  margin-bottom: 24px;
+}
+
 .phui-document-view-pro-box .phui-timeline-view {
   padding: 16px 0 0 0;
   background: none;
-  border-top: 1px solid rgba({$alphablue}, 0.20);
 }
 
 .phui-document-view-pro-box .phui-timeline-wedge {

From 152e7713eb06453b0dfe4fa89729af6aba4834fa Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 08:35:22 -0700
Subject: [PATCH 15/25] Remove obsolete, nonfunctional draft auto-saving in
 Phriction

Summary:
Depends on D19660. Ref T5811. Ref T13077.

Long ago, if you started editing a Phriction document but didn't save it, we'd save the draft in the background as part of the preview.

D11169 updated the preview to use shared infrastructure and broke this function, since we never save drafts.

Since this doesn't work right now, I want to add another thing called "draft", and the future of this feature should be more integrated with modern drafts and EditEngine (which fixed some bugs related to versioning), just get rid of this code for the moment.

Test Plan: Edited documents. This code doesn't do anything since D11169, so no behavior changed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T5811

Differential Revision: https://secure.phabricator.com/D19661
---
 .../controller/PhrictionEditController.php    | 45 +------------------
 1 file changed, 2 insertions(+), 43 deletions(-)

diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php
index 2c47187007..4c1d69c272 100644
--- a/src/applications/phriction/controller/PhrictionEditController.php
+++ b/src/applications/phriction/controller/PhrictionEditController.php
@@ -72,43 +72,6 @@ final class PhrictionEditController
       }
     }
 
-    if ($request->getBool('nodraft')) {
-      $draft = null;
-      $draft_key = null;
-    } else {
-      if ($document->getPHID()) {
-        $draft_key = $document->getPHID().':'.$content->getVersion();
-      } else {
-        $draft_key = 'phriction:'.$content->getSlug();
-      }
-      $draft = id(new PhabricatorDraft())->loadOneWhere(
-        'authorPHID = %s AND draftKey = %s',
-        $viewer->getPHID(),
-        $draft_key);
-    }
-
-    if ($draft &&
-      strlen($draft->getDraft()) &&
-      ($draft->getDraft() != $content->getContent())) {
-      $content_text = $draft->getDraft();
-
-      $discard = phutil_tag(
-        'a',
-        array(
-          'href' => $request->getRequestURI()->alter('nodraft', true),
-        ),
-        pht('discard this draft'));
-
-      $draft_note = new PHUIInfoView();
-      $draft_note->setSeverity(PHUIInfoView::SEVERITY_NOTICE);
-      $draft_note->setTitle(pht('Recovered Draft'));
-      $draft_note->appendChild(
-        pht('Showing a saved draft of your edits, you can %s.', $discard));
-    } else {
-      $content_text = $content->getContent();
-      $draft_note = null;
-    }
-
     require_celerity_resource('phriction-document-css');
 
     $e_title = true;
@@ -131,6 +94,8 @@ final class PhrictionEditController
 
     $v_space = $document->getSpacePHID();
 
+    $content_text = $content->getContent();
+
     if ($request->isFormPost()) {
 
       $title = $request->getStr('title');
@@ -181,10 +146,6 @@ final class PhrictionEditController
       try {
         $editor->applyTransactions($document, $xactions);
 
-        if ($draft) {
-          $draft->delete();
-        }
-
         $uri = PhrictionDocument::getSlugURI($document->getSlug());
         return id(new AphrontRedirectResponse())->setURI($uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
@@ -239,7 +200,6 @@ final class PhrictionEditController
     $form = id(new AphrontFormView())
       ->setUser($viewer)
       ->addHiddenInput('slug', $document->getSlug())
-      ->addHiddenInput('nodraft', $request->getBool('nodraft'))
       ->addHiddenInput('contentVersion', $max_version)
       ->addHiddenInput('overwrite', $overwrite)
       ->appendChild(
@@ -325,7 +285,6 @@ final class PhrictionEditController
     $view = id(new PHUITwoColumnView())
       ->setFooter(
         array(
-          $draft_note,
           $form_box,
           $preview,
         ));

From 550028a8825a781faaf548c19d1c6b29850baea6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 09:04:44 -0700
Subject: [PATCH 16/25] Allow Phriction document edits to be saved as drafts

Summary:
Depends on D19661. Ref T13077. See PHI840.

When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.

Then there are a few minor rules:

  - If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
  - Highlight "Publish" if it's a likely action that you might want to take.

Internally, there are two types of edits. Both types create a new version with the new content. However:

  - A "content" edit sets the version shown on the live page to the newly-created version.
  - A "draft" edit does not update the version shown on the live page.

Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19662
---
 resources/celerity/map.php                    | 10 +-
 src/__phutil_library_map__.php                |  6 +-
 .../PhrictionDocumentController.php           | 31 +++---
 .../controller/PhrictionEditController.php    | 41 ++++++--
 .../editor/PhrictionTransactionEditor.php     | 15 +++
 .../PhrictionDocumentContentTransaction.php   | 86 +----------------
 .../PhrictionDocumentDraftTransaction.php     | 14 +++
 .../PhrictionDocumentEditTransaction.php      | 95 +++++++++++++++++++
 src/view/phui/PHUIButtonView.php              | 19 +++-
 webroot/rsrc/css/phui/button/phui-button.css  | 13 ++-
 webroot/rsrc/css/phui/phui-form-view.css      | 11 +--
 11 files changed, 218 insertions(+), 123 deletions(-)
 create mode 100644 src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
 create mode 100644 src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 1b4fd3517a..720683c371 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
-    'core.pkg.css' => 'badf3f16',
+    'core.pkg.css' => '2574c199',
     'core.pkg.js' => 'b5a949ca',
     'differential.pkg.css' => '06dc617c',
     'differential.pkg.js' => 'c1cfa143',
@@ -122,7 +122,7 @@ return array(
     'rsrc/css/layout/phabricator-source-code-view.css' => '2ab25dfa',
     'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494',
     'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68',
-    'rsrc/css/phui/button/phui-button.css' => '1863cc6e',
+    'rsrc/css/phui/button/phui-button.css' => '6ccb303c',
     'rsrc/css/phui/calendar/phui-calendar-day.css' => '572b1893',
     'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600',
     'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf',
@@ -151,7 +151,7 @@ return array(
     'rsrc/css/phui/phui-document.css' => 'c4ac41f9',
     'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9',
     'rsrc/css/phui/phui-fontkit.css' => '1320ed01',
-    'rsrc/css/phui/phui-form-view.css' => 'f808e5be',
+    'rsrc/css/phui/phui-form-view.css' => '2f43fae7',
     'rsrc/css/phui/phui-form.css' => '7aaa04e3',
     'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f',
     'rsrc/css/phui/phui-header-view.css' => '1ba8b707',
@@ -800,7 +800,7 @@ return array(
     'phui-box-css' => '4bd6cdb9',
     'phui-bulk-editor-css' => '9a81e5d5',
     'phui-button-bar-css' => 'f1ff5494',
-    'phui-button-css' => '1863cc6e',
+    'phui-button-css' => '6ccb303c',
     'phui-button-simple-css' => '8e1baf68',
     'phui-calendar-css' => 'f1ddf11c',
     'phui-calendar-day-css' => '572b1893',
@@ -819,7 +819,7 @@ return array(
     'phui-font-icon-base-css' => '870a7360',
     'phui-fontkit-css' => '1320ed01',
     'phui-form-css' => '7aaa04e3',
-    'phui-form-view-css' => 'f808e5be',
+    'phui-form-view-css' => '2f43fae7',
     'phui-head-thing-view-css' => 'fd311e5f',
     'phui-header-view-css' => '1ba8b707',
     'phui-hovercard' => '1bd28176',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index e6146bc9ef..c25019b1fb 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -5021,7 +5021,9 @@ phutil_register_library_map(array(
     'PhrictionDocumentController' => 'applications/phriction/controller/PhrictionDocumentController.php',
     'PhrictionDocumentDatasource' => 'applications/phriction/typeahead/PhrictionDocumentDatasource.php',
     'PhrictionDocumentDeleteTransaction' => 'applications/phriction/xaction/PhrictionDocumentDeleteTransaction.php',
+    'PhrictionDocumentDraftTransaction' => 'applications/phriction/xaction/PhrictionDocumentDraftTransaction.php',
     'PhrictionDocumentEditEngine' => 'applications/phriction/editor/PhrictionDocumentEditEngine.php',
+    'PhrictionDocumentEditTransaction' => 'applications/phriction/xaction/PhrictionDocumentEditTransaction.php',
     'PhrictionDocumentFerretEngine' => 'applications/phriction/search/PhrictionDocumentFerretEngine.php',
     'PhrictionDocumentFulltextEngine' => 'applications/phriction/search/PhrictionDocumentFulltextEngine.php',
     'PhrictionDocumentHeraldAdapter' => 'applications/phriction/herald/PhrictionDocumentHeraldAdapter.php',
@@ -11138,11 +11140,13 @@ phutil_register_library_map(array(
     ),
     'PhrictionDocumentAuthorHeraldField' => 'PhrictionDocumentHeraldField',
     'PhrictionDocumentContentHeraldField' => 'PhrictionDocumentHeraldField',
-    'PhrictionDocumentContentTransaction' => 'PhrictionDocumentVersionTransaction',
+    'PhrictionDocumentContentTransaction' => 'PhrictionDocumentEditTransaction',
     'PhrictionDocumentController' => 'PhrictionController',
     'PhrictionDocumentDatasource' => 'PhabricatorTypeaheadDatasource',
     'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentVersionTransaction',
+    'PhrictionDocumentDraftTransaction' => 'PhrictionDocumentEditTransaction',
     'PhrictionDocumentEditEngine' => 'PhabricatorEditEngine',
+    'PhrictionDocumentEditTransaction' => 'PhrictionDocumentVersionTransaction',
     'PhrictionDocumentFerretEngine' => 'PhabricatorFerretEngine',
     'PhrictionDocumentFulltextEngine' => 'PhabricatorFulltextEngine',
     'PhrictionDocumentHeraldAdapter' => 'HeraldAdapter',
diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index d4c9d0b263..e424a7f148 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -450,19 +450,28 @@ final class PhrictionDocumentController
       $publish_name = pht('Publish Revert');
     }
 
+    // If you're looking at the current version; and it's an unpublished
+    // draft; and you can publish it, add a UI hint that this might be an
+    // interesting action to take.
+    $hint_publish = false;
+    if ($is_draft) {
+      if ($can_publish) {
+        if ($document->getMaxVersion() == $content->getVersion()) {
+          $hint_publish = true;
+        }
+      }
+    }
+
     $publish_uri = "/phriction/publish/{$id}/{$content_id}/";
 
-    if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) {
-      $publish_name = pht('Publish (Prototype!)');
-
-      $curtain->addAction(
-        id(new PhabricatorActionView())
-        ->setName($publish_name)
-        ->setIcon('fa-upload')
-        ->setDisabled(!$can_publish)
-        ->setWorkflow(true)
-        ->setHref($publish_uri));
-    }
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+      ->setName($publish_name)
+      ->setIcon('fa-upload')
+      ->setSelected($hint_publish)
+      ->setDisabled(!$can_publish)
+      ->setWorkflow(true)
+      ->setHref($publish_uri));
 
     if ($document->getStatus() == PhrictionDocumentStatus::STATUS_EXISTS) {
       $curtain->addAction(
diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php
index 4c1d69c272..31879632de 100644
--- a/src/applications/phriction/controller/PhrictionEditController.php
+++ b/src/applications/phriction/controller/PhrictionEditController.php
@@ -95,8 +95,10 @@ final class PhrictionEditController
     $v_space = $document->getSpacePHID();
 
     $content_text = $content->getContent();
+    $is_draft_mode = ($document->getContent()->getVersion() != $max_version);
 
     if ($request->isFormPost()) {
+      $save_as_draft = ($is_draft_mode || $request->getExists('draft'));
 
       $title = $request->getStr('title');
       $content_text = $request->getStr('content');
@@ -108,13 +110,18 @@ final class PhrictionEditController
       $v_projects = $request->getArr('projects');
       $v_space = $request->getStr('spacePHID');
 
+      if ($save_as_draft) {
+        $edit_type = PhrictionDocumentDraftTransaction::TRANSACTIONTYPE;
+      } else {
+        $edit_type = PhrictionDocumentContentTransaction::TRANSACTIONTYPE;
+      }
+
       $xactions = array();
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhrictionDocumentTitleTransaction::TRANSACTIONTYPE)
         ->setNewValue($title);
       $xactions[] = id(new PhrictionTransaction())
-        ->setTransactionType(
-          PhrictionDocumentContentTransaction::TRANSACTIONTYPE)
+        ->setTransactionType($edit_type)
         ->setNewValue($content_text);
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
@@ -147,6 +154,14 @@ final class PhrictionEditController
         $editor->applyTransactions($document, $xactions);
 
         $uri = PhrictionDocument::getSlugURI($document->getSlug());
+        $uri = new PhutilURI($uri);
+
+        // If the user clicked "Save as Draft", take them to the draft, not
+        // to the current published page.
+        if ($save_as_draft) {
+          $uri = $uri->alter('v', $document->getMaxVersion());
+        }
+
         return id(new AphrontRedirectResponse())->setURI($uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
         $validation_exception = $ex;
@@ -176,7 +191,7 @@ final class PhrictionEditController
       if ($overwrite) {
         $submit_button = pht('Overwrite Changes');
       } else {
-        $submit_button = pht('Save Changes');
+        $submit_button = pht('Save and Publish');
       }
     } else {
       $submit_button = pht('Create Document');
@@ -196,7 +211,6 @@ final class PhrictionEditController
     $view_capability = PhabricatorPolicyCapability::CAN_VIEW;
     $edit_capability = PhabricatorPolicyCapability::CAN_EDIT;
 
-
     $form = id(new AphrontFormView())
       ->setUser($viewer)
       ->addHiddenInput('slug', $document->getSlug())
@@ -253,11 +267,26 @@ final class PhrictionEditController
           ->setLabel(pht('Edit Notes'))
           ->setValue($notes)
           ->setError(null)
-          ->setName('description'))
-      ->appendChild(
+          ->setName('description'));
+
+    if ($is_draft_mode) {
+      $form->appendControl(
         id(new AphrontFormSubmitControl())
+          ->addCancelButton($cancel_uri)
+          ->setValue(pht('Save Draft')));
+    } else {
+      $draft_button = id(new PHUIButtonView())
+        ->setTag('input')
+        ->setName('draft')
+        ->setText(pht('Save as Draft'))
+        ->setColor(PHUIButtonView::GREEN);
+
+      $form->appendControl(
+        id(new AphrontFormSubmitControl())
+          ->addButton($draft_button)
           ->addCancelButton($cancel_uri)
           ->setValue($submit_button));
+    }
 
     $form_box = id(new PHUIObjectBoxView())
       ->setHeaderText($page_title)
diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php
index 3c045e16ba..6d72fc5b8e 100644
--- a/src/applications/phriction/editor/PhrictionTransactionEditor.php
+++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php
@@ -74,6 +74,21 @@ final class PhrictionTransactionEditor
     return $this;
   }
 
+  public function setShouldPublishContent(
+    PhrictionDocument $object,
+    $publish) {
+
+    if ($publish) {
+      $content_phid = $this->getNewContent()->getPHID();
+    } else {
+      $content_phid = $this->getOldContent()->getPHID();
+    }
+
+    $object->setContentPHID($content_phid);
+
+    return $this;
+  }
+
   public function getEditorApplicationClass() {
     return 'PhabricatorPhrictionApplication';
   }
diff --git a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
index 759121dd65..de6d92aac6 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
@@ -1,94 +1,16 @@
 <?php
 
 final class PhrictionDocumentContentTransaction
-  extends PhrictionDocumentVersionTransaction {
+  extends PhrictionDocumentEditTransaction {
 
   const TRANSACTIONTYPE = 'content';
 
-  public function generateOldValue($object) {
-    if ($this->getEditor()->getIsNewObject()) {
-      return null;
-    }
-    return $object->getContent()->getContent();
-  }
-
-  public function generateNewValue($object, $value) {
-    return $value;
-  }
-
   public function applyInternalEffects($object, $value) {
+    parent::applyInternalEffects($object, $value);
+
     $object->setStatus(PhrictionDocumentStatus::STATUS_EXISTS);
 
-    $content = $this->getNewDocumentContent($object);
-    $content->setContent($value);
-  }
-
-  public function shouldHide() {
-    if ($this->getOldValue() === null) {
-      return true;
-    } else {
-      return false;
-    }
-  }
-
-  public function getActionStrength() {
-    return 1.3;
-  }
-
-  public function getActionName() {
-    return pht('Edited');
-  }
-
-  public function getTitle() {
-    return pht(
-      '%s edited the content of this document.',
-      $this->renderAuthor());
-  }
-
-  public function getTitleForFeed() {
-    return pht(
-      '%s edited the content of %s.',
-      $this->renderAuthor(),
-      $this->renderObject());
-  }
-
-  public function hasChangeDetailView() {
-    return true;
-  }
-
-  public function getMailDiffSectionHeader() {
-    return pht('CHANGES TO DOCUMENT CONTENT');
-  }
-
-  public function newChangeDetailView() {
-    $viewer = $this->getViewer();
-
-    return id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setViewer($viewer)
-      ->setOldText($this->getOldValue())
-      ->setNewText($this->getNewValue());
-  }
-
-  public function newRemarkupChanges() {
-    $changes = array();
-
-    $changes[] = $this->newRemarkupChange()
-      ->setOldValue($this->getOldValue())
-      ->setNewValue($this->getNewValue());
-
-    return $changes;
-  }
-
-  public function validateTransactions($object, array $xactions) {
-    $errors = array();
-
-    $content = $object->getContent()->getContent();
-    if ($this->isEmptyTextTransaction($content, $xactions)) {
-      $errors[] = $this->newRequiredError(
-        pht('Documents must have content.'));
-    }
-
-    return $errors;
+    $this->getEditor()->setShouldPublishContent($object, true);
   }
 
 }
diff --git a/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
new file mode 100644
index 0000000000..91a4b4ade5
--- /dev/null
+++ b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
@@ -0,0 +1,14 @@
+<?php
+
+final class PhrictionDocumentDraftTransaction
+  extends PhrictionDocumentEditTransaction {
+
+  const TRANSACTIONTYPE = 'draft';
+
+  public function applyInternalEffects($object, $value) {
+    parent::applyInternalEffects($object, $value);
+
+    $this->getEditor()->setShouldPublishContent($object, false);
+  }
+
+}
diff --git a/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php
new file mode 100644
index 0000000000..551e8c8c2a
--- /dev/null
+++ b/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php
@@ -0,0 +1,95 @@
+<?php
+
+abstract class PhrictionDocumentEditTransaction
+  extends PhrictionDocumentVersionTransaction {
+
+  public function generateOldValue($object) {
+    if ($this->getEditor()->getIsNewObject()) {
+      return null;
+    }
+
+    // NOTE: We want to get the newest version of the content here, regardless
+    // of whether it's published or not.
+
+    $actor = $this->getActor();
+
+    return id(new PhrictionContentQuery())
+      ->setViewer($actor)
+      ->withDocumentPHIDs(array($object->getPHID()))
+      ->setOrder('newest')
+      ->setLimit(1)
+      ->executeOne()
+      ->getContent();
+  }
+
+  public function generateNewValue($object, $value) {
+    return $value;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $content = $this->getNewDocumentContent($object);
+    $content->setContent($value);
+  }
+
+  public function getActionStrength() {
+    return 1.3;
+  }
+
+  public function getActionName() {
+    return pht('Edited');
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s edited the content of this document.',
+      $this->renderAuthor());
+  }
+
+  public function getTitleForFeed() {
+    return pht(
+      '%s edited the content of %s.',
+      $this->renderAuthor(),
+      $this->renderObject());
+  }
+
+  public function getMailDiffSectionHeader() {
+    return pht('CHANGES TO DOCUMENT CONTENT');
+  }
+
+  public function hasChangeDetailView() {
+    return true;
+  }
+
+  public function newChangeDetailView() {
+    $viewer = $this->getViewer();
+
+    return id(new PhabricatorApplicationTransactionTextDiffDetailView())
+      ->setViewer($viewer)
+      ->setOldText($this->getOldValue())
+      ->setNewText($this->getNewValue());
+  }
+
+  public function newRemarkupChanges() {
+    $changes = array();
+
+    $changes[] = $this->newRemarkupChange()
+      ->setOldValue($this->getOldValue())
+      ->setNewValue($this->getNewValue());
+
+    return $changes;
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $content = $object->getContent()->getContent();
+    if ($this->isEmptyTextTransaction($content, $xactions)) {
+      $errors[] = $this->newRequiredError(
+        pht('Documents must have content.'));
+    }
+
+    return $errors;
+  }
+
+
+}
diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php
index bad24122bc..2231649f12 100644
--- a/src/view/phui/PHUIButtonView.php
+++ b/src/view/phui/PHUIButtonView.php
@@ -233,14 +233,15 @@ final class PHUIButtonView extends AphrontTagView {
       $classes = array();
     }
 
-    // See PHI823. If we aren't rendering a "<button>" tag, give the tag we
-    // are rendering a "button" role as a hint to screen readers.
+    // See PHI823. If we aren't rendering a "<button>" or "<input>" tag,
+    // give the tag we are rendering a "button" role as a hint to screen
+    // readers.
     $role = null;
-    if ($this->tag !== 'button') {
+    if ($this->tag !== 'button' && $this->tag !== 'input') {
       $role = 'button';
     }
 
-    return array(
+    $attrs = array(
       'class'  => $classes,
       'href'   => $this->href,
       'name'   => $this->name,
@@ -249,9 +250,19 @@ final class PHUIButtonView extends AphrontTagView {
       'meta'   => $meta,
       'role' => $role,
     );
+
+    if ($this->tag == 'input') {
+      $attrs['type'] = 'submit';
+      $attrs['value'] = $this->text;
+    }
+
+    return $attrs;
   }
 
   protected function getTagContent() {
+    if ($this->tag === 'input') {
+      return null;
+    }
 
     $icon = $this->icon;
     $text = null;
diff --git a/webroot/rsrc/css/phui/button/phui-button.css b/webroot/rsrc/css/phui/button/phui-button.css
index 14bbdcf46b..22ad96779d 100644
--- a/webroot/rsrc/css/phui/button/phui-button.css
+++ b/webroot/rsrc/css/phui/button/phui-button.css
@@ -4,7 +4,8 @@
 
 
 button,
-a.button {
+a.button,
+input[type="submit"] {
   font: {$basefont};
   -webkit-font-smoothing: antialiased;
   -webkit-user-select: none;
@@ -72,7 +73,8 @@ a.icon:visited {
 
 button.button-green,
 a.button-green.button,
-a.button-green.button:visited {
+a.button-green.button:visited,
+input[type="submit"].button-green {
   background-color: {$green.button.color};
   border-color: {$green.button.color};
   background-image: {$green.button.gradient};
@@ -80,16 +82,17 @@ a.button-green.button:visited {
 
 button.button-red,
 a.button-red.button,
-a.button-red.button:visited {
+a.button-red.button:visited,
+input[type="submit"].button-red {
   background-color: {$red.button.color};
   border-color: {$red.button.color};
   background-image: {$red.button.gradient};
 }
 
 button.button-grey,
-input[type="submit"].button-grey,
 a.button-grey,
-a.button-grey:visited {
+a.button-grey:visited,
+input[type="submit"].button-grey {
   background-color: {$grey.button.color};
   background-image: {$grey.button.gradient};
   border: 1px solid rgba({$alphablue}, 0.3);
diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css
index 9824196882..539eaa2e71 100644
--- a/webroot/rsrc/css/phui/phui-form-view.css
+++ b/webroot/rsrc/css/phui/phui-form-view.css
@@ -133,19 +133,12 @@
 }
 
 .aphront-form-control-submit button,
-.aphront-form-control-submit a.button {
+.aphront-form-control-submit a.button,
+.aphront-form-control-submit input[type="submit"] {
   float: right;
   margin: 4px 0 0 8px;
 }
 
-.phui-form-control-multi-submit input,
-.phui-form-control-multi-submit button,
-.phui-form-control-multi-submit a {
-  float: right;
-  margin: 4px 0 0 8px;
-  width: auto;
-}
-
 .aphront-form-control-textarea textarea.aphront-textarea-very-short {
   height: 44px;
 }

From b6ba75d991df4babeb6e6bdc6cdd9eeabe8741f2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 10:14:56 -0700
Subject: [PATCH 17/25] Add a Phriction hint when a draft exists, and fix some
 draft editing bugs

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.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19663
---
 .../PhrictionDocumentController.php           | 29 +++++++++++++++++
 .../controller/PhrictionEditController.php    | 32 ++++++++++++-------
 .../PhrictionDocumentContentTransaction.php   | 16 ++++++++++
 .../PhrictionDocumentDraftTransaction.php     | 21 ++++++++++++
 .../PhrictionDocumentEditTransaction.php      | 13 --------
 5 files changed, 87 insertions(+), 24 deletions(-)

diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index e424a7f148..1853dd1ee1 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -186,6 +186,35 @@ final class PhrictionDocumentController
         );
       } else {
         $content = $document->getContent();
+
+        if ($content->getVersion() < $document->getMaxVersion()) {
+          $can_edit = PhabricatorPolicyFilter::hasCapability(
+            $viewer,
+            $document,
+            PhabricatorPolicyCapability::CAN_EDIT);
+          if ($can_edit) {
+            $document_uri = new PhutilURI($document->getURI());
+            $draft_uri = $document_uri->alter('v', $document->getMaxVersion());
+
+            $draft_link = phutil_tag(
+              'a',
+              array(
+                'href' => $draft_uri,
+              ),
+              pht('View Draft Version'));
+
+            $draft_link = phutil_tag('strong', array(), $draft_link);
+
+            $version_note = id(new PHUIInfoView())
+              ->setSeverity(PHUIInfoView::SEVERITY_NOTICE)
+              ->appendChild(
+                array(
+                  pht('This document has unpublished draft changes.'),
+                  ' ',
+                  $draft_link,
+                ));
+          }
+        }
       }
 
       $page_title = $content->getTitle();
diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php
index 31879632de..9f59b63121 100644
--- a/src/applications/phriction/controller/PhrictionEditController.php
+++ b/src/applications/phriction/controller/PhrictionEditController.php
@@ -98,7 +98,11 @@ final class PhrictionEditController
     $is_draft_mode = ($document->getContent()->getVersion() != $max_version);
 
     if ($request->isFormPost()) {
-      $save_as_draft = ($is_draft_mode || $request->getExists('draft'));
+      if ($is_new) {
+        $save_as_draft = false;
+      } else {
+        $save_as_draft = ($is_draft_mode || $request->getExists('draft'));
+      }
 
       $title = $request->getStr('title');
       $content_text = $request->getStr('content');
@@ -117,6 +121,7 @@ final class PhrictionEditController
       }
 
       $xactions = array();
+
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhrictionDocumentTitleTransaction::TRANSACTIONTYPE)
         ->setNewValue($title);
@@ -275,17 +280,22 @@ final class PhrictionEditController
           ->addCancelButton($cancel_uri)
           ->setValue(pht('Save Draft')));
     } else {
-      $draft_button = id(new PHUIButtonView())
-        ->setTag('input')
-        ->setName('draft')
-        ->setText(pht('Save as Draft'))
-        ->setColor(PHUIButtonView::GREEN);
+      $submit = id(new AphrontFormSubmitControl());
 
-      $form->appendControl(
-        id(new AphrontFormSubmitControl())
-          ->addButton($draft_button)
-          ->addCancelButton($cancel_uri)
-          ->setValue($submit_button));
+      if (!$is_new) {
+        $draft_button = id(new PHUIButtonView())
+          ->setTag('input')
+          ->setName('draft')
+          ->setText(pht('Save as Draft'))
+          ->setColor(PHUIButtonView::GREEN);
+        $submit->addButton($draft_button);
+      }
+
+      $submit
+        ->addCancelButton($cancel_uri)
+        ->setValue($submit_button);
+
+      $form->appendControl($submit);
     }
 
     $form_box = id(new PHUIObjectBoxView())
diff --git a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
index de6d92aac6..f5f1305279 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentContentTransaction.php
@@ -13,4 +13,20 @@ final class PhrictionDocumentContentTransaction
     $this->getEditor()->setShouldPublishContent($object, true);
   }
 
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    // NOTE: This is slightly different from the draft validation. Here,
+    // we're validating that: you can't edit away a document; and you can't
+    // create an empty document.
+
+    $content = $object->getContent()->getContent();
+    if ($this->isEmptyTextTransaction($content, $xactions)) {
+      $errors[] = $this->newRequiredError(
+        pht('Documents must have content.'));
+    }
+
+    return $errors;
+  }
+
 }
diff --git a/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
index 91a4b4ade5..b2f39485fc 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
@@ -11,4 +11,25 @@ final class PhrictionDocumentDraftTransaction
     $this->getEditor()->setShouldPublishContent($object, false);
   }
 
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    // NOTE: We're only validating that you can't edit a document down to
+    // nothing in a draft transaction. Alone, this doesn't prevent you from
+    // creating a document with no content. The content transaction has
+    // validation for that.
+
+    if (!$xactions) {
+      return $errors;
+    }
+
+    $content = $object->getContent()->getContent();
+    if ($this->isEmptyTextTransaction($content, $xactions)) {
+      $errors[] = $this->newRequiredError(
+        pht('Documents must have content.'));
+    }
+
+    return $errors;
+  }
+
 }
diff --git a/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php
index 551e8c8c2a..3130658311 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentEditTransaction.php
@@ -79,17 +79,4 @@ abstract class PhrictionDocumentEditTransaction
     return $changes;
   }
 
-  public function validateTransactions($object, array $xactions) {
-    $errors = array();
-
-    $content = $object->getContent()->getContent();
-    if ($this->isEmptyTextTransaction($content, $xactions)) {
-      $errors[] = $this->newRequiredError(
-        pht('Documents must have content.'));
-    }
-
-    return $errors;
-  }
-
-
 }

From 0089ef4b60e31514735d9cc275906f481e75537d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Sep 2018 13:38:16 -0700
Subject: [PATCH 18/25] Don't show Phriction draft edit events in feed

Summary: Depends on D19663. Ref T13077. When you edit a Phriction draft, don't publish a feed story. (The eventual "Publish" event gets a story.)

Test Plan: Made draft / non-draft / publish edits, only saw feed stories for non-draft and publish edits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19664
---
 .../phriction/xaction/PhrictionDocumentDraftTransaction.php   | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
index b2f39485fc..3fc9d63ca8 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentDraftTransaction.php
@@ -11,6 +11,10 @@ final class PhrictionDocumentDraftTransaction
     $this->getEditor()->setShouldPublishContent($object, false);
   }
 
+  public function shouldHideForFeed() {
+    return true;
+  }
+
   public function validateTransactions($object, array $xactions) {
     $errors = array();
 

From 0abf29765e058cb410a98d5d0db8c434b9df096b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 13 Sep 2018 08:13:41 -0700
Subject: [PATCH 19/25] Make Phriction diff-of-changes view draft-aware and
 clarify some language

Summary:
Ref T13077. Ref T13197. See PHI840.

  - In the "History > Diff/Compare" view, the button language wasn't draft-aware.
  - Revise language to avoid the word "Revert", since this can be ambiguous.
    - "Edit this page, starting with an older version of the text" is now "Edit Older|Current|Draft Version X".
    - "Mark this older version of the page as the current published version" is now "Publish Older Version".
  - Let the user edit the current published version, too, since this is a reasonable operation if there are drafts.

Test Plan: Navigated the history diff view, saw better button and action text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197, T13077

Differential Revision: https://secure.phabricator.com/D19668
---
 .../controller/PhrictionDiffController.php    | 20 +++++++++----------
 .../PhrictionDocumentController.php           |  2 +-
 .../controller/PhrictionPublishController.php |  2 +-
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/applications/phriction/controller/PhrictionDiffController.php b/src/applications/phriction/controller/PhrictionDiffController.php
index c81a90a3df..537f743eba 100644
--- a/src/applications/phriction/controller/PhrictionDiffController.php
+++ b/src/applications/phriction/controller/PhrictionDiffController.php
@@ -95,7 +95,7 @@ final class PhrictionDiffController extends PhrictionController {
     $navigation_table = null;
     if ($l + 1 == $r) {
       $nav_l = ($l > 1);
-      $nav_r = ($r != $current->getVersion());
+      $nav_r = ($r != $document->getMaxVersion());
 
       $uri = $request->getRequestURI();
 
@@ -191,30 +191,28 @@ final class PhrictionDiffController extends PhrictionController {
       PhrictionChangeType::CHANGE_MOVE_AWAY => true, // Plain silly
       PhrictionChangeType::CHANGE_STUB      => true, // Utterly silly
     );
+
     if (isset($hidden_statuses[$content->getChangeType()])) {
       // Don't show an edit/revert button for changes which deleted, moved or
       // stubbed the content since it's silly.
       return null;
     }
 
-    if ($content->getID() == $current->getID()) {
-      return phutil_tag(
-        'a',
-        array(
-          'href'  => '/phriction/edit/'.$document_id.'/',
-          'class' => 'button button-grey',
-        ),
-        pht('Edit Current Version'));
+    if ($version == $current->getVersion()) {
+      $label = pht('Edit Current Version %s...', new PhutilNumber($version));
+    } else if ($version < $current->getVersion()) {
+      $label = pht('Edit Older Version %s...', new PhutilNumber($version));
+    } else {
+      $label = pht('Edit Draft Version %s...', new PhutilNumber($version));
     }
 
-
     return phutil_tag(
       'a',
       array(
         'href'  => '/phriction/edit/'.$document_id.'/?revert='.$version,
         'class' => 'button button-grey',
       ),
-      pht('Revert to Version %s...', $version));
+      $label);
   }
 
   private function renderComparisonTable(array $content) {
diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index 1853dd1ee1..77f6a3529c 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -476,7 +476,7 @@ final class PhrictionDocumentController
     if ($is_draft) {
       $publish_name = pht('Publish Draft');
     } else {
-      $publish_name = pht('Publish Revert');
+      $publish_name = pht('Publish Older Version');
     }
 
     // If you're looking at the current version; and it's an unpublished
diff --git a/src/applications/phriction/controller/PhrictionPublishController.php b/src/applications/phriction/controller/PhrictionPublishController.php
index 43c90cb1fa..e24b5f9be0 100644
--- a/src/applications/phriction/controller/PhrictionPublishController.php
+++ b/src/applications/phriction/controller/PhrictionPublishController.php
@@ -63,7 +63,7 @@ final class PhrictionPublishController
     }
 
     if ($content->getVersion() < $document->getContent()->getVersion()) {
-      $title = pht('Revert Document?');
+      $title = pht('Publish Older Version?');
       $body = pht(
         'Revert the published version of this document to an older '.
         'version?');

From 2ddc770fd881de7a5bbe613de49d1456e8c978ae Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 13 Sep 2018 09:11:47 -0700
Subject: [PATCH 20/25] Update Phriction documentation for drafts

Summary: Depends on D19668. Ref T13197. See PHI840. This updates the documentation to describe how drafts work in more detail.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19669
---
 src/docs/user/userguide/phriction.diviner | 75 ++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/src/docs/user/userguide/phriction.diviner b/src/docs/user/userguide/phriction.diviner
index 60ad6357fe..1a56dce46b 100644
--- a/src/docs/user/userguide/phriction.diviner
+++ b/src/docs/user/userguide/phriction.diviner
@@ -6,5 +6,78 @@ Construct a detailed written history of your civilization.
 Overview
 ========
 
-Phriction is a simple wiki. You can edit pages, and the text you write will stay
+Phriction is a wiki. You can edit pages, and the text you write will stay
 there. Other people can see it later.
+
+Phriction documents are arranged in a hierarchy, like a filesystem. This can
+make it easier to keep things organized and to apply policy controls to
+groups of documents.
+
+
+Policies
+========
+
+Documents and policies in Phriction are hierarchical, similar to a filesystem.
+For example, a document called "Zebra Information" may be located
+at `/zoo/animals/zebra/`.
+
+To view a document in Phrction, you must first be able to view all of its
+ancestors. So a user can only see {nav Zoo > Animals > Zebra Information} if
+they can see the pages {nav Zoo} and {nav Zoo > Animals}.
+
+This allows sections of the wiki to be restricted by applying a restrictive
+policy to the parent (or grandparent) document. For example, if you apply a
+restrictive view policy to the {nav Zoo} page, it will implicitly apply to
+all sub-pages, including {nav Zoo > Animals > Zebra Information}.
+
+
+Versions and Drafts
+===================
+
+Document content is tracked with linear version numbers: version 1, version 2,
+version 3, and so on. Each time a page is edited, a new version of the page is
+created.
+
+You can {nav View History} to review older versions of a page and see how it
+has changed over time (and who has changed it).
+
+When you visit a particular document, you are normally shown the most recent
+version of that document. For example, if there are 17 versions, you'll see
+version 17.
+
+Likewise, when you edit a document using {nav Edit Document > Save and Publish},
+your changes are published immediately. If there were previously 17 versions,
+your new changes will become version 18 and visitors to the document will begin
+seeing version 18.
+
+If you want to edit a document without publishing the changes right away, you
+can use {nav Edit Document > Save as Draft} instead. This will still create a
+new version 18, but it won't change which version users see when they visit the
+document: they'll still see version 17 (the last published version).
+
+You (and other users) can continue editing the draft by using
+{nav Edit Document}. (Once a document has an unpublished draft, editing will
+stay in draft mode.)
+
+Once you're satisfied with your changes, use {nav Publish Draft} to make your
+changes the current visible version of the document that users see by default
+when they visit it.
+
+If you made a mistake and published something you didn't intend to, you can
+navigate back to an older version of the document and use
+{nav Publish Older Version} to change the current visible version of the
+document to some older version.
+
+Note that draft versions are still normal versions of the document: they are
+not private, they can not be deleted, other users can see them if they can see
+the document, and they will eventually become a standard part of the document
+history. The only private parts of drafts are: editing a draft does not
+generate a feed story; and users won't see draft content by default when
+viewing a document.
+
+Drafts may be a good fit if you are:
+
+  - working on changes over time; or
+  - starting with a rough change and refining it in several iterations; or
+  - collaborating with others on a change; or
+  - sharing changes before they're published to get feedback.

From 40d5d5c9848e0191edf0e4dda68aa72642a49913 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Sep 2018 06:24:32 -0700
Subject: [PATCH 21/25] Remove "mailKey" from "PhabricatorRepositoryCommit"

Summary: Ref T13197. Ref T13065. This continues the gradual purge of dedicated "mailKey" columns in favor of shared infrastructure.

Test Plan:
  - Ran migration.
  - Visually inspected database.
  - Grepped for `mailKey`.
  - Added some comments, saw the daemons generate corresponding mail.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197, T13065

Differential Revision: https://secure.phabricator.com/D19670
---
 .../autopatches/20180914.audit.01.mailkey.php | 26 +++++++++++++++++++
 .../autopatches/20180914.audit.02.rmkey.sql   |  2 ++
 .../storage/PhabricatorRepositoryCommit.php   | 10 -------
 3 files changed, 28 insertions(+), 10 deletions(-)
 create mode 100644 resources/sql/autopatches/20180914.audit.01.mailkey.php
 create mode 100644 resources/sql/autopatches/20180914.audit.02.rmkey.sql

diff --git a/resources/sql/autopatches/20180914.audit.01.mailkey.php b/resources/sql/autopatches/20180914.audit.01.mailkey.php
new file mode 100644
index 0000000000..de8419a3c4
--- /dev/null
+++ b/resources/sql/autopatches/20180914.audit.01.mailkey.php
@@ -0,0 +1,26 @@
+<?php
+
+$commit_table = new PhabricatorRepositoryCommit();
+$commit_conn = $commit_table->establishConnection('w');
+$commit_name = $commit_table->getTableName();
+
+$properties_table = new PhabricatorMetaMTAMailProperties();
+$conn = $properties_table->establishConnection('w');
+
+$iterator = new LiskRawMigrationIterator($commit_conn, $commit_name);
+foreach ($iterator as $commit) {
+  queryfx(
+    $conn,
+    'INSERT IGNORE INTO %T
+        (objectPHID, mailProperties, dateCreated, dateModified)
+      VALUES
+        (%s, %s, %d, %d)',
+    $properties_table->getTableName(),
+    $commit['phid'],
+    phutil_json_encode(
+      array(
+        'mailKey' => $commit['mailKey'],
+      )),
+    PhabricatorTime::getNow(),
+    PhabricatorTime::getNow());
+}
diff --git a/resources/sql/autopatches/20180914.audit.02.rmkey.sql b/resources/sql/autopatches/20180914.audit.02.rmkey.sql
new file mode 100644
index 0000000000..3187da4258
--- /dev/null
+++ b/resources/sql/autopatches/20180914.audit.02.rmkey.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_commit
+  DROP mailKey;
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 0362aa778c..5615b5231f 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -25,7 +25,6 @@ final class PhabricatorRepositoryCommit
   protected $committerIdentityPHID;
   protected $commitIdentifier;
   protected $epoch;
-  protected $mailKey;
   protected $authorPHID;
   protected $auditStatus = DiffusionCommitAuditStatus::NONE;
   protected $summary = '';
@@ -116,7 +115,6 @@ final class PhabricatorRepositoryCommit
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_COLUMN_SCHEMA => array(
         'commitIdentifier' => 'text40',
-        'mailKey' => 'bytes20',
         'authorPHID' => 'phid?',
         'authorIdentityPHID' => 'phid?',
         'committerIdentityPHID' => 'phid?',
@@ -321,13 +319,6 @@ final class PhabricatorRepositoryCommit
     return mpull($audits, 'getAuditorPHID');
   }
 
-  public function save() {
-    if (!$this->mailKey) {
-      $this->mailKey = Filesystem::readRandomCharacters(20);
-    }
-    return parent::save();
-  }
-
   public function delete() {
     $data = $this->loadCommitData();
     $audits = id(new PhabricatorRepositoryAuditRequest())
@@ -605,7 +596,6 @@ final class PhabricatorRepositoryCommit
       'phid' =>  $this->getPHID(),
       'commitIdentifier' =>  $this->getCommitIdentifier(),
       'epoch' => $this->getEpoch(),
-      'mailKey' => $this->getMailKey(),
       'authorPHID' => $this->getAuthorPHID(),
       'auditStatus' => $this->getAuditStatus(),
       'summary' => $this->getSummary(),

From 10f219fb82771669dfaa2405dc11bb2fb0eb6790 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Sep 2018 06:34:17 -0700
Subject: [PATCH 22/25] Allow Drydock logs to be associated with
 RepositoryOperation objects

Summary: Ref T13197. See PHI873. I want to give RepositoryOperation objects access to Drydock logging like leases, resources, and blueprints currently have. This just does the schema/query changes, no actual UI or new logging yet.

Test Plan: Ran storage upgrade, poked around the UI looking for anything broken.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19671
---
 .../20180914.drydock.01.operationphid.sql     |  2 ++
 .../drydock/query/DrydockLogQuery.php         | 34 +++++++++++++++++++
 .../drydock/storage/DrydockLog.php            | 24 +++++++++++--
 3 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20180914.drydock.01.operationphid.sql

diff --git a/resources/sql/autopatches/20180914.drydock.01.operationphid.sql b/resources/sql/autopatches/20180914.drydock.01.operationphid.sql
new file mode 100644
index 0000000000..bdfe02b0df
--- /dev/null
+++ b/resources/sql/autopatches/20180914.drydock.01.operationphid.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_drydock.drydock_log
+  ADD operationPHID VARBINARY(64);
diff --git a/src/applications/drydock/query/DrydockLogQuery.php b/src/applications/drydock/query/DrydockLogQuery.php
index 00980edb4d..b73ad371ae 100644
--- a/src/applications/drydock/query/DrydockLogQuery.php
+++ b/src/applications/drydock/query/DrydockLogQuery.php
@@ -5,6 +5,7 @@ final class DrydockLogQuery extends DrydockQuery {
   private $blueprintPHIDs;
   private $resourcePHIDs;
   private $leasePHIDs;
+  private $operationPHIDs;
 
   public function withBlueprintPHIDs(array $phids) {
     $this->blueprintPHIDs = $phids;
@@ -21,6 +22,11 @@ final class DrydockLogQuery extends DrydockQuery {
     return $this;
   }
 
+  public function withOperationPHIDs(array $phids) {
+    $this->operationPHIDs = $phids;
+    return $this;
+  }
+
   public function newResultObject() {
     return new DrydockLog();
   }
@@ -93,6 +99,27 @@ final class DrydockLogQuery extends DrydockQuery {
       $log->attachLease($lease);
     }
 
+    $operation_phids = array_filter(mpull($logs, 'getOperationPHID'));
+    if ($operation_phids) {
+      $operations = id(new DrydockRepositoryOperationQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withPHIDs($operation_phids)
+        ->execute();
+      $operations = mpull($operations, null, 'getPHID');
+    } else {
+      $operations = array();
+    }
+
+    foreach ($logs as $key => $log) {
+      $operation = null;
+      $operation_phid = $log->getOperationPHID();
+      if ($operation_phid) {
+        $operation = idx($operations, $operation_phid);
+      }
+      $log->attachOperation($operation);
+    }
+
     return $logs;
   }
 
@@ -120,6 +147,13 @@ final class DrydockLogQuery extends DrydockQuery {
         $this->leasePHIDs);
     }
 
+    if ($this->operationPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'operationPHID IN (%Ls)',
+        $this->operationPHIDs);
+    }
+
     return $where;
   }
 
diff --git a/src/applications/drydock/storage/DrydockLog.php b/src/applications/drydock/storage/DrydockLog.php
index 5d75d82c65..81879944a0 100644
--- a/src/applications/drydock/storage/DrydockLog.php
+++ b/src/applications/drydock/storage/DrydockLog.php
@@ -6,6 +6,7 @@ final class DrydockLog extends DrydockDAO
   protected $blueprintPHID;
   protected $resourcePHID;
   protected $leasePHID;
+  protected $operationPHID;
   protected $epoch;
   protected $type;
   protected $data = array();
@@ -13,6 +14,7 @@ final class DrydockLog extends DrydockDAO
   private $blueprint = self::ATTACHABLE;
   private $resource = self::ATTACHABLE;
   private $lease = self::ATTACHABLE;
+  private $operation = self::ATTACHABLE;
 
   protected function getConfiguration() {
     return array(
@@ -24,6 +26,7 @@ final class DrydockLog extends DrydockDAO
         'blueprintPHID' => 'phid?',
         'resourcePHID' => 'phid?',
         'leasePHID' => 'phid?',
+        'operationPHID' => 'phid?',
         'type' => 'text64',
       ),
       self::CONFIG_KEY_SCHEMA => array(
@@ -36,6 +39,9 @@ final class DrydockLog extends DrydockDAO
         'key_lease' => array(
           'columns' => array('leasePHID', 'type'),
         ),
+        'key_operation' => array(
+          'columns' => array('operationPHID', 'type'),
+        ),
         'epoch' => array(
           'columns' => array('epoch'),
         ),
@@ -70,6 +76,16 @@ final class DrydockLog extends DrydockDAO
     return $this->assertAttached($this->lease);
   }
 
+  public function attachOperation(
+    DrydockRepositoryOperation $operation = null) {
+    $this->operation = $operation;
+    return $this;
+  }
+
+  public function getOperation() {
+    return $this->assertAttached($this->operation);
+  }
+
   public function isComplete() {
     if ($this->getBlueprintPHID() && !$this->getBlueprint()) {
       return false;
@@ -83,6 +99,10 @@ final class DrydockLog extends DrydockDAO
       return false;
     }
 
+    if ($this->getOperationPHID() && !$this->getOperation()) {
+      return false;
+    }
+
     return true;
   }
 
@@ -108,8 +128,8 @@ final class DrydockLog extends DrydockDAO
 
   public function describeAutomaticCapability($capability) {
     return pht(
-      'To view log details, you must be able to view the associated '.
-      'blueprint, resource and lease.');
+      'To view log details, you must be able to view all associated '.
+      'blueprints, resources, leases, and repository operations.');
   }
 
 }

From 92bcf859747dcaed3871c0b9ecd7205dc43d0051 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Sep 2018 06:48:06 -0700
Subject: [PATCH 23/25] Add Drydock logs to the RepositoryOperation UI

Summary:
Depends on D19671. Ref T13197. See PHI873.

Expose logs in the RepositoryOperation UI. Nothing writes the logs yet, so these interfaces are currently always empty.

Test Plan:
{F5887102}

{F5887103}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19672
---
 .../PhabricatorDrydockApplication.php         |  2 ++
 .../controller/DrydockLogController.php       | 32 +++++++++++++++++++
 .../controller/DrydockLogListController.php   | 11 +++++++
 ...ydockRepositoryOperationViewController.php | 15 +++++++--
 .../drydock/query/DrydockLogSearchEngine.php  | 23 ++++++++++++-
 5 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php
index fef2c843ff..79bc1cb0d7 100644
--- a/src/applications/drydock/application/PhabricatorDrydockApplication.php
+++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php
@@ -93,6 +93,8 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication {
             '' => 'DrydockRepositoryOperationViewController',
             'status/' => 'DrydockRepositoryOperationStatusController',
             'dismiss/' => 'DrydockRepositoryOperationDismissController',
+            'logs/(?:query/(?P<queryKey>[^/]+)/)?' =>
+              'DrydockLogListController',
           ),
         ),
       ),
diff --git a/src/applications/drydock/controller/DrydockLogController.php b/src/applications/drydock/controller/DrydockLogController.php
index 704cbb53b9..69e0c84804 100644
--- a/src/applications/drydock/controller/DrydockLogController.php
+++ b/src/applications/drydock/controller/DrydockLogController.php
@@ -6,6 +6,7 @@ abstract class DrydockLogController
   private $blueprint;
   private $resource;
   private $lease;
+  private $operation;
 
   public function setBlueprint(DrydockBlueprint $blueprint) {
     $this->blueprint = $blueprint;
@@ -34,6 +35,15 @@ abstract class DrydockLogController
     return $this->lease;
   }
 
+  public function setOperation(DrydockRepositoryOperation $operation) {
+    $this->operation = $operation;
+    return $this;
+  }
+
+  public function getOperation() {
+    return $this->operation;
+  }
+
   public function buildSideNavView() {
     $nav = new AphrontSideNavFilterView();
     $nav->setBaseURI(new PhutilURI($this->getApplicationURI()));
@@ -56,6 +66,11 @@ abstract class DrydockLogController
       $engine->setLease($lease);
     }
 
+    $operation = $this->getOperation();
+    if ($operation) {
+      $engine->setOperation($operation);
+    }
+
     $engine->addNavigationItems($nav->getMenu());
 
     $nav->selectFilter(null);
@@ -66,9 +81,12 @@ abstract class DrydockLogController
   protected function buildApplicationCrumbs() {
     $crumbs = parent::buildApplicationCrumbs();
 
+    $viewer = $this->getViewer();
+
     $blueprint = $this->getBlueprint();
     $resource = $this->getResource();
     $lease = $this->getLease();
+    $operation = $this->getOperation();
     if ($blueprint) {
       $id = $blueprint->getID();
 
@@ -111,6 +129,20 @@ abstract class DrydockLogController
       $crumbs->addTextCrumb(
         pht('Logs'),
         $this->getApplicationURI("lease/{$id}/logs/"));
+    } else if ($operation) {
+      $id = $operation->getID();
+
+      $crumbs->addTextCrumb(
+        pht('Operations'),
+        $this->getApplicationURI('operation/'));
+
+      $crumbs->addTextCrumb(
+        pht('Repository Operation %d', $id),
+        $this->getApplicationURI("operation/{$id}/"));
+
+      $crumbs->addTextCrumb(
+        pht('Logs'),
+        $this->getApplicationURI("operation/{$id}/logs/"));
     }
 
     return $crumbs;
diff --git a/src/applications/drydock/controller/DrydockLogListController.php b/src/applications/drydock/controller/DrydockLogListController.php
index b5e4d4ff0c..26719db39e 100644
--- a/src/applications/drydock/controller/DrydockLogListController.php
+++ b/src/applications/drydock/controller/DrydockLogListController.php
@@ -46,6 +46,17 @@ final class DrydockLogListController extends DrydockLogController {
         $engine->setLease($lease);
         $this->setLease($lease);
         break;
+      case 'operation':
+        $operation = id(new DrydockRepositoryOperationQuery())
+          ->setViewer($viewer)
+          ->withIDs(array($id))
+          ->executeOne();
+        if (!$operation) {
+          return new Aphront404Response();
+        }
+        $engine->setOperation($operation);
+        $this->setOperation($operation);
+        break;
       default:
         return new Aphront404Response();
     }
diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
index e7fbf07b35..23e381a1ee 100644
--- a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
+++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
@@ -47,13 +47,22 @@ final class DrydockRepositoryOperationViewController
       ->setUser($viewer)
       ->setOperation($operation);
 
+    $log_query = id(new DrydockLogQuery())
+      ->withOperationPHIDs(array($operation->getPHID()));
+
+    $logs = $this->buildLogBox(
+      $log_query,
+      $this->getApplicationURI("operation/{$id}/logs/query/all/"));
+
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)
       ->setCurtain($curtain)
       ->addPropertySection(pht('Properties'), $properties)
-      ->setMainColumn(array(
-        $status_view,
-      ));
+      ->setMainColumn(
+        array(
+          $status_view,
+          $logs,
+        ));
 
     return $this->newPage()
       ->setTitle($title)
diff --git a/src/applications/drydock/query/DrydockLogSearchEngine.php b/src/applications/drydock/query/DrydockLogSearchEngine.php
index 43b1511c01..3602714a60 100644
--- a/src/applications/drydock/query/DrydockLogSearchEngine.php
+++ b/src/applications/drydock/query/DrydockLogSearchEngine.php
@@ -5,6 +5,7 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine {
   private $blueprint;
   private $resource;
   private $lease;
+  private $operation;
 
   public function setBlueprint(DrydockBlueprint $blueprint) {
     $this->blueprint = $blueprint;
@@ -33,6 +34,15 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine {
     return $this->lease;
   }
 
+  public function setOperation(DrydockRepositoryOperation $operation) {
+    $this->operation = $operation;
+    return $this;
+  }
+
+  public function getOperation() {
+    return $this->operation;
+  }
+
   public function canUseInPanelContext() {
     // Prevent use on Dashboard panels since all log queries currently need a
     // parent object and these don't seem particularly useful in any case.
@@ -65,6 +75,11 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine {
       $query->withLeasePHIDs(array($lease->getPHID()));
     }
 
+    $operation = $this->getOperation();
+    if ($operation) {
+      $query->withOperationPHIDs(array($operation->getPHID()));
+    }
+
     return $query;
   }
 
@@ -97,9 +112,15 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine {
       return "/drydock/lease/{$id}/logs/{$path}";
     }
 
+    $operation = $this->getOperation();
+    if ($operation) {
+      $id = $operation->getID();
+      return "/drydock/operation/{$id}/logs/{$path}";
+    }
+
     throw new Exception(
       pht(
-        'Search engine has no blueprint, resource, or lease.'));
+        'Search engine has no blueprint, resource, lease, or operation.'));
   }
 
   protected function getBuiltinQueryNames() {

From a7e060f062e0c3c088a604f4ba5d74e0633bad8d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Sep 2018 06:56:11 -0700
Subject: [PATCH 24/25] Write a trivial log when starting a repository
 operation

Summary:
Depends on D19672. Ref T13197. See PHI873. This writes a trivial log when we begin acting on a working copy and makes it look reasonable in the UI.

This is mostly just to prove that logging works properly.

Test Plan: {F5887697}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19673
---
 src/__phutil_library_map__.php                |  2 +
 .../DrydockBlueprintViewController.php        |  5 +-
 .../drydock/controller/DrydockController.php  |  9 ++-
 .../controller/DrydockLeaseViewController.php |  5 +-
 ...ydockRepositoryOperationViewController.php |  5 +-
 .../DrydockResourceViewController.php         | 18 +++---
 .../logtype/DrydockOperationWorkLogType.php   | 19 +++++++
 .../DrydockRepositoryOperationPHIDType.php    |  4 +-
 .../storage/DrydockRepositoryOperation.php    | 37 ++++++++++++
 .../drydock/view/DrydockLogListView.php       | 57 +++++++++++++++++++
 ...DrydockRepositoryOperationUpdateWorker.php |  4 ++
 11 files changed, 150 insertions(+), 15 deletions(-)
 create mode 100644 src/applications/drydock/logtype/DrydockOperationWorkLogType.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index c25019b1fb..ed10d0f9ef 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1168,6 +1168,7 @@ phutil_register_library_map(array(
     'DrydockManagementUpdateResourceWorkflow' => 'applications/drydock/management/DrydockManagementUpdateResourceWorkflow.php',
     'DrydockManagementWorkflow' => 'applications/drydock/management/DrydockManagementWorkflow.php',
     'DrydockObjectAuthorizationView' => 'applications/drydock/view/DrydockObjectAuthorizationView.php',
+    'DrydockOperationWorkLogType' => 'applications/drydock/logtype/DrydockOperationWorkLogType.php',
     'DrydockQuery' => 'applications/drydock/query/DrydockQuery.php',
     'DrydockRepositoryOperation' => 'applications/drydock/storage/DrydockRepositoryOperation.php',
     'DrydockRepositoryOperationController' => 'applications/drydock/controller/DrydockRepositoryOperationController.php',
@@ -6575,6 +6576,7 @@ phutil_register_library_map(array(
     'DrydockManagementUpdateResourceWorkflow' => 'DrydockManagementWorkflow',
     'DrydockManagementWorkflow' => 'PhabricatorManagementWorkflow',
     'DrydockObjectAuthorizationView' => 'AphrontView',
+    'DrydockOperationWorkLogType' => 'DrydockLogType',
     'DrydockQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DrydockRepositoryOperation' => array(
       'DrydockDAO',
diff --git a/src/applications/drydock/controller/DrydockBlueprintViewController.php b/src/applications/drydock/controller/DrydockBlueprintViewController.php
index d3f73bf78c..2c31e4ae8f 100644
--- a/src/applications/drydock/controller/DrydockBlueprintViewController.php
+++ b/src/applications/drydock/controller/DrydockBlueprintViewController.php
@@ -58,8 +58,11 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController {
     $log_query = id(new DrydockLogQuery())
       ->withBlueprintPHIDs(array($blueprint->getPHID()));
 
+    $log_table = $this->buildLogTable($log_query)
+      ->setHideBlueprints(true);
+
     $logs = $this->buildLogBox(
-      $log_query,
+      $log_table,
       $this->getApplicationURI("blueprint/{$id}/logs/query/all/"));
 
     $view = id(new PHUITwoColumnView())
diff --git a/src/applications/drydock/controller/DrydockController.php b/src/applications/drydock/controller/DrydockController.php
index 872a78f66f..f393f210d7 100644
--- a/src/applications/drydock/controller/DrydockController.php
+++ b/src/applications/drydock/controller/DrydockController.php
@@ -79,7 +79,7 @@ abstract class DrydockController extends PhabricatorController {
       ->addRawContent($table);
   }
 
-  protected function buildLogBox(DrydockLogQuery $query, $all_uri) {
+  protected function buildLogTable(DrydockLogQuery $query) {
     $viewer = $this->getViewer();
 
     $logs = $query
@@ -89,9 +89,12 @@ abstract class DrydockController extends PhabricatorController {
 
     $log_table = id(new DrydockLogListView())
       ->setUser($viewer)
-      ->setLogs($logs)
-      ->render();
+      ->setLogs($logs);
 
+    return $log_table;
+  }
+
+  protected function buildLogBox(DrydockLogListView $log_table, $all_uri) {
     $log_header = id(new PHUIHeaderView())
       ->setHeader(pht('Logs'))
       ->addActionLink(
diff --git a/src/applications/drydock/controller/DrydockLeaseViewController.php b/src/applications/drydock/controller/DrydockLeaseViewController.php
index 18e1d0088d..dfffcf8402 100644
--- a/src/applications/drydock/controller/DrydockLeaseViewController.php
+++ b/src/applications/drydock/controller/DrydockLeaseViewController.php
@@ -43,8 +43,11 @@ final class DrydockLeaseViewController extends DrydockLeaseController {
     $log_query = id(new DrydockLogQuery())
       ->withLeasePHIDs(array($lease->getPHID()));
 
+    $log_table = $this->buildLogTable($log_query)
+      ->setHideLeases(true);
+
     $logs = $this->buildLogBox(
-      $log_query,
+      $log_table,
       $this->getApplicationURI("lease/{$id}/logs/query/all/"));
 
     $crumbs = $this->buildApplicationCrumbs();
diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
index 23e381a1ee..e255c63ec3 100644
--- a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
+++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
@@ -50,8 +50,11 @@ final class DrydockRepositoryOperationViewController
     $log_query = id(new DrydockLogQuery())
       ->withOperationPHIDs(array($operation->getPHID()));
 
+    $log_table = $this->buildLogTable($log_query)
+      ->setHideOperations(true);
+
     $logs = $this->buildLogBox(
-      $log_query,
+      $log_table,
       $this->getApplicationURI("operation/{$id}/logs/query/all/"));
 
     $view = id(new PHUITwoColumnView())
diff --git a/src/applications/drydock/controller/DrydockResourceViewController.php b/src/applications/drydock/controller/DrydockResourceViewController.php
index 8718caa9e2..ec0dce53e1 100644
--- a/src/applications/drydock/controller/DrydockResourceViewController.php
+++ b/src/applications/drydock/controller/DrydockResourceViewController.php
@@ -48,8 +48,11 @@ final class DrydockResourceViewController extends DrydockResourceController {
     $log_query = id(new DrydockLogQuery())
       ->withResourcePHIDs(array($resource->getPHID()));
 
-    $log_box = $this->buildLogBox(
-      $log_query,
+    $log_table = $this->buildLogTable($log_query)
+      ->setHideResources(true);
+
+    $logs = $this->buildLogBox(
+      $log_table,
       $this->getApplicationURI("resource/{$id}/logs/query/all/"));
 
     $crumbs = $this->buildApplicationCrumbs();
@@ -86,11 +89,12 @@ final class DrydockResourceViewController extends DrydockResourceController {
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)
       ->setCurtain($curtain)
-      ->setMainColumn(array(
-        $object_box,
-        $lease_box,
-        $log_box,
-      ));
+      ->setMainColumn(
+        array(
+          $object_box,
+          $lease_box,
+          $logs,
+        ));
 
     return $this->newPage()
       ->setTitle($title)
diff --git a/src/applications/drydock/logtype/DrydockOperationWorkLogType.php b/src/applications/drydock/logtype/DrydockOperationWorkLogType.php
new file mode 100644
index 0000000000..deff90c16c
--- /dev/null
+++ b/src/applications/drydock/logtype/DrydockOperationWorkLogType.php
@@ -0,0 +1,19 @@
+<?php
+
+final class DrydockOperationWorkLogType extends DrydockLogType {
+
+  const LOGCONST = 'core.operation.work';
+
+  public function getLogTypeName() {
+    return pht('Started Work');
+  }
+
+  public function getLogTypeIcon(array $data) {
+    return 'fa-check green';
+  }
+
+  public function renderLog(array $data) {
+    return pht('Started this operation in a working copy.');
+  }
+
+}
diff --git a/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php b/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php
index 0926f92388..6222552f84 100644
--- a/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php
+++ b/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php
@@ -5,7 +5,7 @@ final class DrydockRepositoryOperationPHIDType extends PhabricatorPHIDType {
   const TYPECONST = 'DRYO';
 
   public function getTypeName() {
-    return pht('Drydock Repository Operation');
+    return pht('Repository Operation');
   }
 
   public function newObject() {
@@ -33,7 +33,7 @@ final class DrydockRepositoryOperationPHIDType extends PhabricatorPHIDType {
       $operation = $objects[$phid];
       $id = $operation->getID();
 
-      $handle->setName(pht('Drydock Repository Operation %d', $id));
+      $handle->setName(pht('Repository Operation %d', $id));
       $handle->setURI("/drydock/operation/{$id}/");
     }
   }
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 809a10a116..142e14dcf7 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -25,6 +25,7 @@ final class DrydockRepositoryOperation extends DrydockDAO
   private $repository = self::ATTACHABLE;
   private $object = self::ATTACHABLE;
   private $implementation = self::ATTACHABLE;
+  private $workingCopyLease = self::ATTACHABLE;
 
   public static function initializeNewOperation(
     DrydockRepositoryOperationType $op) {
@@ -90,6 +91,19 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return $this->implementation;
   }
 
+  public function getWorkingCopyLease() {
+    return $this->assertAttached($this->workingCopyLease);
+  }
+
+  public function attachWorkingCopyLease(DrydockLease $lease) {
+    $this->workingCopyLease = $lease;
+    return $this;
+  }
+
+  public function hasWorkingCopyLease() {
+    return ($this->workingCopyLease !== self::ATTACHABLE);
+  }
+
   public function getProperty($key, $default = null) {
     return idx($this->properties, $key, $default);
   }
@@ -189,6 +203,29 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return $this->getProperty('exec.workingcopy.error');
   }
 
+  public function logEvent($type, array $data = array()) {
+    $log = id(new DrydockLog())
+      ->setEpoch(PhabricatorTime::getNow())
+      ->setType($type)
+      ->setData($data);
+
+    $log->setOperationPHID($this->getPHID());
+
+    if ($this->hasWorkingCopyLease()) {
+      $lease = $this->getWorkingCopyLease();
+      $log->setLeasePHID($lease->getPHID());
+
+      $resource_phid = $lease->getResourcePHID();
+      if ($resource_phid) {
+        $resource = $lease->getResource();
+
+        $log->setResourcePHID($resource->getPHID());
+        $log->setBlueprintPHID($resource->getBlueprintPHID());
+      }
+    }
+
+    return $log->save();
+  }
 
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
diff --git a/src/applications/drydock/view/DrydockLogListView.php b/src/applications/drydock/view/DrydockLogListView.php
index 61fde7cee7..32f5d0d7ed 100644
--- a/src/applications/drydock/view/DrydockLogListView.php
+++ b/src/applications/drydock/view/DrydockLogListView.php
@@ -3,6 +3,46 @@
 final class DrydockLogListView extends AphrontView {
 
   private $logs;
+  private $hideBlueprints;
+  private $hideResources;
+  private $hideLeases;
+  private $hideOperations;
+
+  public function setHideBlueprints($hide_blueprints) {
+    $this->hideBlueprints = $hide_blueprints;
+    return $this;
+  }
+
+  public function getHideBlueprints() {
+    return $this->hideBlueprints;
+  }
+
+  public function setHideResources($hide_resources) {
+    $this->hideResources = $hide_resources;
+    return $this;
+  }
+
+  public function getHideResources() {
+    return $this->hideResources;
+  }
+
+  public function setHideLeases($hide_leases) {
+    $this->hideLeases = $hide_leases;
+    return $this;
+  }
+
+  public function getHideLeases() {
+    return $this->hideLeases;
+  }
+
+  public function setHideOperations($hide_operations) {
+    $this->hideOperations = $hide_operations;
+    return $this;
+  }
+
+  public function getHideOperations() {
+    return $this->hideOperations;
+  }
 
   public function setLogs(array $logs) {
     assert_instances_of($logs, 'DrydockLog');
@@ -41,6 +81,13 @@ final class DrydockLogListView extends AphrontView {
         $lease = null;
       }
 
+      $operation_phid = $log->getOperationPHID();
+      if ($operation_phid) {
+        $operation = $viewer->renderHandle($operation_phid);
+      } else {
+        $operation = null;
+      }
+
       if ($log->isComplete()) {
         $type_key = $log->getType();
         if (isset($types[$type_key])) {
@@ -72,6 +119,7 @@ final class DrydockLogListView extends AphrontView {
         $blueprint,
         $resource,
         $lease,
+        $operation,
         id(new PHUIIconView())->setIcon($icon),
         $type,
         $data,
@@ -86,16 +134,25 @@ final class DrydockLogListView extends AphrontView {
           pht('Blueprint'),
           pht('Resource'),
           pht('Lease'),
+          pht('Operation'),
           null,
           pht('Type'),
           pht('Data'),
           pht('Date'),
         ))
+      ->setColumnVisibility(
+        array(
+          !$this->getHideBlueprints(),
+          !$this->getHideResources(),
+          !$this->getHideLeases(),
+          !$this->getHideOperations(),
+        ))
       ->setColumnClasses(
         array(
           '',
           '',
           '',
+          '',
           'icon',
           '',
           'wide',
diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
index 2ee9324fb6..c6e6927f22 100644
--- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
@@ -59,6 +59,10 @@ final class DrydockRepositoryOperationUpdateWorker
       // No matter what happens here, destroy the lease away once we're done.
       $lease->setReleaseOnDestruction(true);
 
+      $operation->attachWorkingCopyLease($lease);
+
+      $operation->logEvent(DrydockOperationWorkLogType::LOGCONST);
+
       $operation->applyOperation($interface);
 
     } catch (PhabricatorWorkerYieldException $ex) {

From 0167f357b7233ec23d70985911254681b1c2e6d7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Sep 2018 09:18:54 -0700
Subject: [PATCH 25/25] Provide a convenient way to log arbitrary text in
 Drydock without needing structured log classes

Summary: Depends on D19673. Ref T13197. See PHI873.

Test Plan:
Added some code like this:

```
$operation->logText('Nice convenient text logging.');
```

...then got:

{F5887712}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19674
---
 src/__phutil_library_map__.php                |  2 ++
 .../drydock/logtype/DrydockTextLogType.php    | 27 +++++++++++++++++++
 .../storage/DrydockRepositoryOperation.php    |  8 ++++++
 3 files changed, 37 insertions(+)
 create mode 100644 src/applications/drydock/logtype/DrydockTextLogType.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index ed10d0f9ef..8bcf3d6120 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1206,6 +1206,7 @@ phutil_register_library_map(array(
     'DrydockSlotLockException' => 'applications/drydock/exception/DrydockSlotLockException.php',
     'DrydockSlotLockFailureLogType' => 'applications/drydock/logtype/DrydockSlotLockFailureLogType.php',
     'DrydockTestRepositoryOperation' => 'applications/drydock/operation/DrydockTestRepositoryOperation.php',
+    'DrydockTextLogType' => 'applications/drydock/logtype/DrydockTextLogType.php',
     'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/DrydockWebrootInterface.php',
     'DrydockWorker' => 'applications/drydock/worker/DrydockWorker.php',
     'DrydockWorkingCopyBlueprintImplementation' => 'applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php',
@@ -6620,6 +6621,7 @@ phutil_register_library_map(array(
     'DrydockSlotLockException' => 'Exception',
     'DrydockSlotLockFailureLogType' => 'DrydockLogType',
     'DrydockTestRepositoryOperation' => 'DrydockRepositoryOperationType',
+    'DrydockTextLogType' => 'DrydockLogType',
     'DrydockWebrootInterface' => 'DrydockInterface',
     'DrydockWorker' => 'PhabricatorWorker',
     'DrydockWorkingCopyBlueprintImplementation' => 'DrydockBlueprintImplementation',
diff --git a/src/applications/drydock/logtype/DrydockTextLogType.php b/src/applications/drydock/logtype/DrydockTextLogType.php
new file mode 100644
index 0000000000..8d6fe9264b
--- /dev/null
+++ b/src/applications/drydock/logtype/DrydockTextLogType.php
@@ -0,0 +1,27 @@
+<?php
+
+/**
+ * Simple convenience log type for logging arbitrary text.
+ *
+ * Drydock logs can be given formal types, which allows them to be translated
+ * and filtered. If you don't particularly care about fancy logging features,
+ * you can use this log type to just dump some text into the log. Maybe you
+ * could upgrade to more formal logging later.
+ */
+final class DrydockTextLogType extends DrydockLogType {
+
+  const LOGCONST = 'core.text';
+
+  public function getLogTypeName() {
+    return pht('Text');
+  }
+
+  public function getLogTypeIcon(array $data) {
+    return 'fa-file-text-o grey';
+  }
+
+  public function renderLog(array $data) {
+    return idx($data, 'text');
+  }
+
+}
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 142e14dcf7..195c5de8ec 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -203,6 +203,14 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return $this->getProperty('exec.workingcopy.error');
   }
 
+  public function logText($text) {
+    return $this->logEvent(
+      DrydockTextLogType::LOGCONST,
+      array(
+        'text' => $text,
+      ));
+  }
+
   public function logEvent($type, array $data = array()) {
     $log = id(new DrydockLog())
       ->setEpoch(PhabricatorTime::getNow())