From 0807b70ea16743aaa8b103c127cf12056a27de07 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 30 Nov 2017 08:13:10 -0800
Subject: [PATCH] Add an explicit warning in the Differential transaction log
 when users skip review

Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.

You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.

Test Plan: {F5302351}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10233

Differential Revision: https://secure.phabricator.com/D18808
---
 resources/celerity/map.php                    |  6 +--
 src/__phutil_library_map__.php                |  2 +
 .../DifferentialDiffExtractionEngine.php      | 10 +++++
 ...ferentialRevisionWrongStateTransaction.php | 41 +++++++++++++++++++
 webroot/rsrc/css/phui/phui-timeline-view.css  |  4 ++
 5 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 78ae8bd461..0ea66bec26 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' => '1a4e0c25',
+    'core.pkg.css' => 'fdb27ef9',
     'core.pkg.js' => '4c79d74f',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '45951e9e',
@@ -176,7 +176,7 @@ return array(
     'rsrc/css/phui/phui-spacing.css' => '042804d6',
     'rsrc/css/phui/phui-status.css' => 'd5263e49',
     'rsrc/css/phui/phui-tag-view.css' => 'b4719c50',
-    'rsrc/css/phui/phui-timeline-view.css' => 'f21db7ca',
+    'rsrc/css/phui/phui-timeline-view.css' => 'e2ef62b1',
     'rsrc/css/phui/phui-two-column-view.css' => '44ec4951',
     'rsrc/css/phui/workboards/phui-workboard-color.css' => '783cdff5',
     'rsrc/css/phui/workboards/phui-workboard.css' => '3bc85455',
@@ -871,7 +871,7 @@ return array(
     'phui-status-list-view-css' => 'd5263e49',
     'phui-tag-view-css' => 'b4719c50',
     'phui-theme-css' => '9f261c6b',
-    'phui-timeline-view-css' => 'f21db7ca',
+    'phui-timeline-view-css' => 'e2ef62b1',
     'phui-two-column-view-css' => '44ec4951',
     'phui-workboard-color-css' => '783cdff5',
     'phui-workboard-view-css' => '3bc85455',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 53bbf9a749..11be8a50f0 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -594,6 +594,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php',
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
     'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php',
+    'DifferentialRevisionWrongStateTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php',
     'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php',
     'DifferentialStoredCustomField' => 'applications/differential/customfield/DifferentialStoredCustomField.php',
@@ -5647,6 +5648,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',
     'DifferentialRevisionViewController' => 'DifferentialController',
     'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType',
+    'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod',
     'DifferentialStoredCustomField' => 'DifferentialCustomField',
diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
index 952c76c63f..c426c411e7 100644
--- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
+++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
@@ -268,6 +268,16 @@ final class DifferentialDiffExtractionEngine extends Phobject {
 
     $xactions = array();
 
+    // If the revision isn't closed or "Accepted", write a warning into the
+    // transaction log. This makes it more clear when users bend the rules.
+    if (!$revision->isClosed() && !$revision->isAccepted()) {
+      $wrong_type = DifferentialRevisionWrongStateTransaction::TRANSACTIONTYPE;
+
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($wrong_type)
+        ->setNewValue($revision->getModernRevisionStatus());
+    }
+
     $xactions[] = id(new DifferentialTransaction())
       ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
       ->setIgnoreOnNoEffect(true)
diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php
new file mode 100644
index 0000000000..e19e54199c
--- /dev/null
+++ b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php
@@ -0,0 +1,41 @@
+<?php
+
+final class DifferentialRevisionWrongStateTransaction
+  extends DifferentialRevisionTransactionType {
+
+  const TRANSACTIONTYPE = 'differential.revision.wrong';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function generateNewValue($object, $value) {
+    return $value;
+  }
+
+  public function getIcon() {
+    return 'fa-exclamation';
+  }
+
+  public function getColor() {
+    return 'pink';
+  }
+
+  public function getActionStrength() {
+    return 4;
+  }
+
+  public function getTitle() {
+    $new_value = $this->getNewValue();
+
+    $status = DifferentialRevisionStatus::newForStatus($new_value);
+
+    return pht(
+      'This revision was not accepted when it landed; it landed in state %s.',
+      $this->renderValue($status->getDisplayName()));
+  }
+
+  public function getTitleForFeed() {
+    return null;
+  }
+}
diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css
index 33e0f8ed0c..b0ae9c0044 100644
--- a/webroot/rsrc/css/phui/phui-timeline-view.css
+++ b/webroot/rsrc/css/phui/phui-timeline-view.css
@@ -315,6 +315,10 @@
   background-color: {$violet};
 }
 
+.phui-timeline-icon-fill-pink {
+  background-color: {$pink};
+}
+
 .phui-timeline-icon-fill-grey {
   background-color: #888;
 }