From c3206476a303556f19e8dc4c058030005172ca9b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 07:44:23 -0800
Subject: [PATCH 01/31] Give "Autoclose Only" repository detail proper
 getters/setters

Summary:
Ref T13222. See D19829. We're inconsistent about using `getDetail()/setDetail()` to do some ad-hoc reads. Put this stuff in proper accessor methods.

Also a couple of text fixes from D19850.

Test Plan: Set, edited, and removed autoclose branches from a repository. Got sensible persistence and rendering behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19856
---
 .../editor/DiffusionRepositoryEditEngine.php          |  3 +--
 .../DiffusionRepositoryBranchesManagementPanel.php    |  6 ++++--
 .../PhabricatorRepositoryManagementThawWorkflow.php   |  6 +++---
 .../repository/storage/PhabricatorRepository.php      | 11 ++++++++++-
 .../PhabricatorRepositoryAutocloseOnlyTransaction.php |  4 ++--
 5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
index 29dc588b45..e570b6debe 100644
--- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
+++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
@@ -215,8 +215,7 @@ final class DiffusionRepositoryEditEngine
     $track_value = $object->getDetail('branch-filter', array());
     $track_value = array_keys($track_value);
 
-    $autoclose_value = $object->getDetail('close-commits-filter', array());
-    $autoclose_value = array_keys($autoclose_value);
+    $autoclose_value = $object->getAutocloseOnlyRules();
 
     $automation_instructions = pht(
       "Configure **Repository Automation** to allow Phabricator to ".
diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
index e3ace3a5a5..77a13c07a1 100644
--- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
+++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
@@ -24,7 +24,7 @@ final class DiffusionRepositoryBranchesManagementPanel
     $has_any =
       $repository->getDetail('default-branch') ||
       $repository->getDetail('branch-filter') ||
-      $repository->getDetail('close-commits-filter');
+      $repository->getAutocloseOnlyRules();
 
     if ($has_any) {
       return 'fa-code-fork';
@@ -83,8 +83,10 @@ final class DiffusionRepositoryBranchesManagementPanel
       phutil_tag('em', array(), pht('Track All Branches')));
     $view->addProperty(pht('Track Only'), $track_only);
 
+    $autoclose_rules = $repository->getAutocloseOnlyRules();
+    $autoclose_rules = implode(', ', $autoclose_rules);
     $autoclose_only = nonempty(
-      $repository->getHumanReadableDetail('close-commits-filter', array()),
+      $autoclose_rules,
       phutil_tag('em', array(), pht('Autoclose On All Branches')));
 
     $autoclose_disabled = false;
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
index 3743045c34..7ca725de81 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementThawWorkflow.php
@@ -15,11 +15,11 @@ final class PhabricatorRepositoryManagementThawWorkflow
         array(
           array(
             'name' => 'demote',
-            'param' => 'device/service',
+            'param' => 'device|service',
             'help' => pht(
               'Demote a device (or all devices in a service) discarding '.
-              'local changes. Clears stuck write locks and recovers from '.
-              'lost leaders.'),
+              'unsynchronized changes. Clears stuck write locks and recovers '.
+              'from lost leaders.'),
           ),
           array(
             'name' => 'promote',
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index c961e06f27..302ef82a48 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -244,7 +244,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
     switch ($key) {
       case 'branch-filter':
-      case 'close-commits-filter':
         $value = array_keys($value);
         $value = implode(', ', $value);
         break;
@@ -1202,6 +1201,16 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return null;
   }
 
+  public function getAutocloseOnlyRules() {
+    return array_keys($this->getDetail('close-commits-filter', array()));
+  }
+
+  public function setAutocloseOnlyRules(array $rules) {
+    $rules = array_fill_keys($rules, true);
+    $this->setDetail('close-commits-filter', $rules);
+    return $this;
+  }
+
 
 /* -(  Repository URI Management  )------------------------------------------ */
 
diff --git a/src/applications/repository/xaction/PhabricatorRepositoryAutocloseOnlyTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryAutocloseOnlyTransaction.php
index feacc73c44..5ff17c4b05 100644
--- a/src/applications/repository/xaction/PhabricatorRepositoryAutocloseOnlyTransaction.php
+++ b/src/applications/repository/xaction/PhabricatorRepositoryAutocloseOnlyTransaction.php
@@ -6,11 +6,11 @@ final class PhabricatorRepositoryAutocloseOnlyTransaction
   const TRANSACTIONTYPE = 'repo:autoclose-only';
 
   public function generateOldValue($object) {
-    return array_keys($object->getDetail('close-commits-filter', array()));
+    return $object->getAutocloseOnlyRules();
   }
 
   public function applyInternalEffects($object, $value) {
-    $object->setDetail('close-commits-filter', array_fill_keys($value, true));
+    $object->setAutocloseOnlyRules($value);
   }
 
   public function getTitle() {

From bf6c534b567a5defa33119de7b2c501d17fc0624 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 08:05:13 -0800
Subject: [PATCH 02/31] Give "Track Only" repository detail proper
 getters/setters

Summary: Depends on D19856. Ref T13222. See D19829. Make access to "Track Only" slightly cleaner and more consistent..

Test Plan: Set, edited, and removed "Track Only" settings for a repository. Saw sensible persistence and display behaviors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19857
---
 .../editor/DiffusionRepositoryEditEngine.php  |  4 +---
 ...usionRepositoryBranchesManagementPanel.php |  8 ++++---
 ...ionRepositorySubversionManagementPanel.php |  2 +-
 .../storage/PhabricatorRepository.php         | 23 ++++++++-----------
 .../PhabricatorRepositoryTestCase.php         |  6 +----
 ...bricatorRepositoryTrackOnlyTransaction.php |  4 ++--
 6 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
index e570b6debe..8907a22ce5 100644
--- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
+++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php
@@ -212,9 +212,7 @@ final class DiffusionRepositoryEditEngine
       ->setObject($object)
       ->execute();
 
-    $track_value = $object->getDetail('branch-filter', array());
-    $track_value = array_keys($track_value);
-
+    $track_value = $object->getTrackOnlyRules();
     $autoclose_value = $object->getAutocloseOnlyRules();
 
     $automation_instructions = pht(
diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
index 77a13c07a1..bdd0a3f71d 100644
--- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
+++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php
@@ -23,7 +23,7 @@ final class DiffusionRepositoryBranchesManagementPanel
 
     $has_any =
       $repository->getDetail('default-branch') ||
-      $repository->getDetail('branch-filter') ||
+      $repository->getTrackOnlyRules() ||
       $repository->getAutocloseOnlyRules();
 
     if ($has_any) {
@@ -74,12 +74,14 @@ final class DiffusionRepositoryBranchesManagementPanel
       ->setViewer($viewer);
 
     $default_branch = nonempty(
-      $repository->getHumanReadableDetail('default-branch'),
+      $repository->getDetail('default-branch'),
       phutil_tag('em', array(), $repository->getDefaultBranch()));
     $view->addProperty(pht('Default Branch'), $default_branch);
 
+    $track_only_rules = $repository->getTrackOnlyRules();
+    $track_only_rules = implode(', ', $track_only_rules);
     $track_only = nonempty(
-      $repository->getHumanReadableDetail('branch-filter', array()),
+      $track_only_rules,
       phutil_tag('em', array(), pht('Track All Branches')));
     $view->addProperty(pht('Track Only'), $track_only);
 
diff --git a/src/applications/diffusion/management/DiffusionRepositorySubversionManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositorySubversionManagementPanel.php
index f87a5d9b85..2d95bc2292 100644
--- a/src/applications/diffusion/management/DiffusionRepositorySubversionManagementPanel.php
+++ b/src/applications/diffusion/management/DiffusionRepositorySubversionManagementPanel.php
@@ -68,7 +68,7 @@ final class DiffusionRepositorySubversionManagementPanel
       ->setViewer($viewer);
 
     $default_branch = nonempty(
-      $repository->getHumanReadableDetail('svn-subpath'),
+      $repository->getDetail('svn-subpath'),
       phutil_tag('em', array(), pht('Import Entire Repository')));
     $view->addProperty(pht('Import Only'), $default_branch);
 
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index 302ef82a48..12c2b9e8a3 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -239,19 +239,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return idx($this->details, $key, $default);
   }
 
-  public function getHumanReadableDetail($key, $default = null) {
-    $value = $this->getDetail($key, $default);
-
-    switch ($key) {
-      case 'branch-filter':
-        $value = array_keys($value);
-        $value = implode(', ', $value);
-        break;
-    }
-
-    return $value;
-  }
-
   public function setDetail($key, $value) {
     $this->details[$key] = $value;
     return $this;
@@ -1211,6 +1198,16 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $this;
   }
 
+  public function getTrackOnlyRules() {
+    return array_keys($this->getDetail('branch-filter', array()));
+  }
+
+  public function setTrackOnlyRules(array $rules) {
+    $rules = array_fill_keys($rules, true);
+    $this->setDetail('branch-filter', $rules);
+    return $this;
+  }
+
 
 /* -(  Repository URI Management  )------------------------------------------ */
 
diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
index f78c3374ed..ea83aa9d56 100644
--- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
+++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
@@ -37,11 +37,7 @@ final class PhabricatorRepositoryTestCase
       $repo->shouldTrackBranch('imaginary'),
       pht('Track all branches by default.'));
 
-    $repo->setDetail(
-      'branch-filter',
-      array(
-        'master' => true,
-      ));
+    $repo->setTrackOnlyRules(array('master'));
 
     $this->assertTrue(
       $repo->shouldTrackBranch('master'),
diff --git a/src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php
index 20ff1baf08..5fdc26e792 100644
--- a/src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php
+++ b/src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php
@@ -6,11 +6,11 @@ final class PhabricatorRepositoryTrackOnlyTransaction
   const TRANSACTIONTYPE = 'repo:track-only';
 
   public function generateOldValue($object) {
-    return array_keys($object->getDetail('branch-filter', array()));
+    return $object->getTrackOnlyRules();
   }
 
   public function applyInternalEffects($object, $value) {
-    $object->setDetail('branch-filter', array_fill_keys($value, true));
+    $object->setTrackOnlyRules($value);
   }
 
   public function getTitle() {

From 00a7071e2d1d3f25ec445430708b7a426298b2a1 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Mon, 10 Dec 2018 12:47:54 -0800
Subject: [PATCH 03/31] Fix handling of Phriction conduit edits

Summary: See https://discourse.phabricator-community.org/t/conduit-method-phriction-edit-requires-title-while-the-docs-say-its-optional/2176. Make code consistent with documentation by not requiring either `content` or `title`.

Test Plan: Hit the method via the UI and no longer got an error on missing `content` or `title` fields.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

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

diff --git a/src/applications/phriction/conduit/PhrictionEditConduitAPIMethod.php b/src/applications/phriction/conduit/PhrictionEditConduitAPIMethod.php
index 9d97ed7f8c..d1e400a485 100644
--- a/src/applications/phriction/conduit/PhrictionEditConduitAPIMethod.php
+++ b/src/applications/phriction/conduit/PhrictionEditConduitAPIMethod.php
@@ -41,12 +41,19 @@ final class PhrictionEditConduitAPIMethod extends PhrictionConduitAPIMethod {
     }
 
     $xactions = array();
-    $xactions[] = id(new PhrictionTransaction())
-      ->setTransactionType(PhrictionDocumentTitleTransaction::TRANSACTIONTYPE)
-      ->setNewValue($request->getValue('title'));
-    $xactions[] = id(new PhrictionTransaction())
-      ->setTransactionType(PhrictionDocumentContentTransaction::TRANSACTIONTYPE)
-      ->setNewValue($request->getValue('content'));
+    if ($request->getValue('title')) {
+      $xactions[] = id(new PhrictionTransaction())
+        ->setTransactionType(
+          PhrictionDocumentTitleTransaction::TRANSACTIONTYPE)
+        ->setNewValue($request->getValue('title'));
+    }
+
+    if ($request->getValue('content')) {
+      $xactions[] = id(new PhrictionTransaction())
+        ->setTransactionType(
+          PhrictionDocumentContentTransaction::TRANSACTIONTYPE)
+        ->setNewValue($request->getValue('content'));
+    }
 
     $editor = id(new PhrictionTransactionEditor())
       ->setActor($request->getUser())

From da4341cf8be5551832597c5e6611112984998088 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Mon, 10 Dec 2018 13:40:37 -0800
Subject: [PATCH 04/31] Make it less confusing to create root-level Phriction
 doc

Summary: Without an existing root document, Phriction shows a nice little "fake" document as the landing page, which has its own nice "Edit this document" button. When showing that page, don't also render the standard "New Document" breadcrumb in the top right. That button always prompts first for a slug name, which is silly when the root document doesn't exist (because the slug name is required to be '').

Test Plan: Loaded Phriction with and without a root document.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19863
---
 .../controller/PhrictionController.php        | 22 ++++++++++++++-----
 .../PhrictionDocumentController.php           |  5 +++--
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/applications/phriction/controller/PhrictionController.php b/src/applications/phriction/controller/PhrictionController.php
index 51ffbcab1f..06f0102526 100644
--- a/src/applications/phriction/controller/PhrictionController.php
+++ b/src/applications/phriction/controller/PhrictionController.php
@@ -2,6 +2,14 @@
 
 abstract class PhrictionController extends PhabricatorController {
 
+  private $showingWelcomeDocument = false;
+
+  public function setShowingWelcomeDocument($show_welcome) {
+    $this->showingWelcomeDocument = $show_welcome;
+    return $this;
+
+  }
+
   public function buildSideNavView($for_app = false) {
     $user = $this->getRequest()->getUser();
 
@@ -37,12 +45,14 @@ abstract class PhrictionController extends PhabricatorController {
           ->setIcon('fa-home'));
     }
 
-    $crumbs->addAction(
-      id(new PHUIListItemView())
-        ->setName(pht('New Document'))
-        ->setHref('/phriction/new/?slug='.$this->getDocumentSlug())
-        ->setWorkflow(true)
-        ->setIcon('fa-plus-square'));
+    if (!$this->showingWelcomeDocument) {
+      $crumbs->addAction(
+        id(new PHUIListItemView())
+          ->setName(pht('New Document'))
+          ->setHref('/phriction/new/?slug='.$this->getDocumentSlug())
+          ->setWorkflow(true)
+          ->setIcon('fa-plus-square'));
+    }
 
     return $crumbs;
   }
diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php
index 8da9807064..bf7282b35a 100644
--- a/src/applications/phriction/controller/PhrictionDocumentController.php
+++ b/src/applications/phriction/controller/PhrictionDocumentController.php
@@ -9,6 +9,7 @@ final class PhrictionDocumentController
     return true;
   }
 
+
   public function handleRequest(AphrontRequest $request) {
     $viewer = $request->getViewer();
     $this->slug = $request->getURIData('slug');
@@ -35,15 +36,15 @@ final class PhrictionDocumentController
       ->needContent(true)
       ->executeOne();
     if (!$document) {
-
       $document = PhrictionDocument::initializeNewDocument($viewer, $slug);
-
       if ($slug == '/') {
         $title = pht('Welcome to Phriction');
         $subtitle = pht('Phriction is a simple and easy to use wiki for '.
           'keeping track of documents and their changes.');
         $page_title = pht('Welcome');
         $create_text = pht('Edit this Document');
+        $this->setShowingWelcomeDocument(true);
+
 
       } else {
         $title = pht('No Document Here');

From d1bcdaeda467570066a7e9e7ddd565eee51bc50b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Dec 2018 08:25:25 -0800
Subject: [PATCH 05/31] Allow the "Create Subtask" workflow to prompt for a
 subtype selection, and prepare for customizable options

Summary:
Ref T13222. Ref T12588. See PHI683. Currently, "Create Subtask" always uses the first edit form that the user has access to for the same task subtype. (For example, if you "Create Subtask" from a "Bug", you get the first edit form for "Bugs".)

I didn't want to go too crazy with the initial subtype implementation, but it seems like we're generally on firm ground and it's working fairly well: user requests are for more flexibility in using the system as implemented, not changes to the system or confusion/difficulty with any of the tradeoffs. Thus, I'm generally comfortable continuing to build it out in the same direction. To improve flexibility, I want to make the options from "Create Subtask" more flexible/configurable.

I plan to let you specify that a given subtype (say, "Quest") prompts you with creation options for a set of other subtypes (say, "Objective"), or prompts you with a particular set of forms.

If we end up with a single option, we just go into the current flow (directly to the edit form). If we end up with more than one option, we prompt the user to choose between them.

This change is a first step toward this:

  - When building "Create Subtask", query for multiple forms.
  - The default behavior is now "prompt user to choose among create forms of the same subtype". Previously, it was "use the first edit form of the same subtype". This is a behavioral change.
  - The next change will make the selected forms configurable.
  - (I also plan to make the dialog itself less rough.)

Test Plan: {F6051067}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T12588

Differential Revision: https://secure.phabricator.com/D19853
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorManiphestApplication.php       |  1 +
 .../ManiphestTaskDetailController.php         | 38 ++++++----
 .../ManiphestTaskSubtaskController.php        | 76 +++++++++++++++++++
 .../editengine/PhabricatorEditEngine.php      |  2 +-
 .../PhabricatorEditEngineSubtypeMap.php       | 43 +++++++++++
 6 files changed, 147 insertions(+), 15 deletions(-)
 create mode 100644 src/applications/maniphest/controller/ManiphestTaskSubtaskController.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index cd260f497d..184c2a3d7c 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1760,6 +1760,7 @@ phutil_register_library_map(array(
     'ManiphestTaskStatusTestCase' => 'applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php',
     'ManiphestTaskStatusTransaction' => 'applications/maniphest/xaction/ManiphestTaskStatusTransaction.php',
     'ManiphestTaskSubpriorityTransaction' => 'applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php',
+    'ManiphestTaskSubtaskController' => 'applications/maniphest/controller/ManiphestTaskSubtaskController.php',
     'ManiphestTaskSubtypeDatasource' => 'applications/maniphest/typeahead/ManiphestTaskSubtypeDatasource.php',
     'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php',
     'ManiphestTaskTitleHeraldField' => 'applications/maniphest/herald/ManiphestTaskTitleHeraldField.php',
@@ -7347,6 +7348,7 @@ phutil_register_library_map(array(
     'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
     'ManiphestTaskStatusTransaction' => 'ManiphestTaskTransactionType',
     'ManiphestTaskSubpriorityTransaction' => 'ManiphestTaskTransactionType',
+    'ManiphestTaskSubtaskController' => 'ManiphestController',
     'ManiphestTaskSubtypeDatasource' => 'PhabricatorTypeaheadDatasource',
     'ManiphestTaskTestCase' => 'PhabricatorTestCase',
     'ManiphestTaskTitleHeraldField' => 'ManiphestTaskHeraldField',
diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php
index 1ed4096660..35c1efb6e8 100644
--- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php
+++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php
@@ -52,6 +52,7 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication {
         'task/' => array(
           $this->getEditRoutePattern('edit/')
             => 'ManiphestTaskEditController',
+          'subtask/(?P<id>[1-9]\d*)/' => 'ManiphestTaskSubtaskController',
         ),
         'subpriority/' => 'ManiphestSubpriorityController',
       ),
diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
index a11cc4d85f..0f96d76b91 100644
--- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
@@ -281,29 +281,39 @@ final class ManiphestTaskDetailController extends ManiphestController {
         ->setDisabled(!$can_edit)
         ->setWorkflow($workflow_edit));
 
-    $edit_config = $edit_engine->loadDefaultEditConfiguration($task);
-    $can_create = (bool)$edit_config;
+    $subtype_map = $task->newEditEngineSubtypeMap();
+    $subtask_options = $subtype_map->getCreateFormsForSubtype(
+      $edit_engine,
+      $task);
 
-    if ($can_create) {
-      $form_key = $edit_config->getIdentifier();
-      $edit_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
+    // If no forms are available, we want to show the user an error.
+    // If one form is available, we take them user directly to the form.
+    // If two or more forms are available, we give the user a choice.
+
+    // The "subtask" controller handles the first case (no forms) and the
+    // third case (more than one form). In the case of one form, we link
+    // directly to the form.
+    $subtask_uri = "/task/subtask/{$id}/";
+    $subtask_workflow = true;
+
+    if (count($subtask_options) == 1) {
+      $subtask_form = head($subtask_options);
+      $form_key = $subtask_form->getIdentifier();
+      $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
         ->setQueryParam('parent', $id)
         ->setQueryParam('template', $id)
         ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
-      $edit_uri = $this->getApplicationURI($edit_uri);
-    } else {
-      // TODO: This will usually give us a somewhat-reasonable error page, but
-      // could be a bit cleaner.
-      $edit_uri = "/task/edit/{$id}/";
-      $edit_uri = $this->getApplicationURI($edit_uri);
+      $subtask_workflow = false;
     }
 
+    $subtask_uri = $this->getApplicationURI($subtask_uri);
+
     $subtask_item = id(new PhabricatorActionView())
       ->setName(pht('Create Subtask'))
-      ->setHref($edit_uri)
+      ->setHref($subtask_uri)
       ->setIcon('fa-level-down')
-      ->setDisabled(!$can_create)
-      ->setWorkflow(!$can_create);
+      ->setDisabled(!$subtask_options)
+      ->setWorkflow($subtask_workflow);
 
     $relationship_list = PhabricatorObjectRelationshipList::newForObject(
       $viewer,
diff --git a/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
new file mode 100644
index 0000000000..9da0ed4206
--- /dev/null
+++ b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
@@ -0,0 +1,76 @@
+<?php
+
+final class ManiphestTaskSubtaskController
+  extends ManiphestController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');
+
+    $task = id(new ManiphestTaskQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->executeOne();
+    if (!$task) {
+      return new Aphront404Response();
+    }
+
+    $cancel_uri = $task->getURI();
+
+    $edit_engine = id(new ManiphestEditEngine())
+      ->setViewer($viewer)
+      ->setTargetObject($task);
+
+    $subtype_map = $task->newEditEngineSubtypeMap();
+
+    $subtype_options = $subtype_map->getCreateFormsForSubtype(
+      $edit_engine,
+      $task);
+
+    if (!$subtype_options) {
+      return $this->newDialog()
+        ->setTitle(pht('No Forms'))
+        ->appendParagraph(
+          pht(
+            'You do not have access to any forms which can be used to '.
+            'create a subtask.'))
+        ->addCancelButton($cancel_uri, pht('Close'));
+    }
+
+    if ($request->isFormPost()) {
+      $form_key = $request->getStr('formKey');
+      if (isset($subtype_options[$form_key])) {
+        $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
+          ->setQueryParam('parent', $id)
+          ->setQueryParam('template', $id)
+          ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
+        $subtask_uri = $this->getApplicationURI($subtask_uri);
+
+        return id(new AphrontRedirectResponse())
+          ->setURI($subtask_uri);
+      }
+    }
+
+    $control = id(new AphrontFormRadioButtonControl())
+      ->setName('formKey')
+      ->setLabel(pht('Subtype'));
+
+    foreach ($subtype_options as $key => $subtype_form) {
+      $control->addButton(
+        $key,
+        $subtype_form->getDisplayName(),
+        null);
+    }
+
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer)
+      ->appendControl($control);
+
+    return $this->newDialog()
+      ->setTitle(pht('Choose Subtype'))
+      ->appendForm($form)
+      ->addSubmitButton(pht('Continue'))
+      ->addCancelButton($cancel_uri);
+  }
+
+}
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
index 4e189df164..86cc014102 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -358,7 +358,7 @@ abstract class PhabricatorEditEngine
     return $this->editEngineConfiguration;
   }
 
-  private function newConfigurationQuery() {
+  public function newConfigurationQuery() {
     return id(new PhabricatorEditEngineConfigurationQuery())
       ->setViewer($this->getViewer())
       ->withEngineKeys(array($this->getEngineKey()));
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
index cd8e0674ae..c76911dbc3 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
@@ -39,4 +39,47 @@ final class PhabricatorEditEngineSubtypeMap
     return $this->subtypes[$subtype_key];
   }
 
+  public function getCreateFormsForSubtype(
+    PhabricatorEditEngine $edit_engine,
+    PhabricatorEditEngineSubtypeInterface $object) {
+
+    $subtype_key = $object->getEditEngineSubtype();
+    $subtype = $this->getSubtype($subtype_key);
+
+    // TODO: Allow subtype configuration to specify that children should be
+    // created from particular forms or subtypes.
+    $select_ids = array();
+    $select_subtypes = array();
+
+    $query = $edit_engine->newConfigurationQuery()
+      ->withIsDisabled(false);
+
+    if ($select_ids) {
+      $query->withIDs($select_ids);
+    } else {
+      // If we're selecting by subtype rather than selecting specific forms,
+      // only select create forms.
+      $query->withIsDefault(true);
+
+      if ($select_subtypes) {
+        $query->withSubtypes($select_subtypes);
+      } else {
+        $query->withSubtypes(array($subtype_key));
+      }
+    }
+
+    $forms = $query->execute();
+    $forms = mpull($forms, null, 'getIdentifier');
+
+    // If we're selecting by ID, respect the order specified in the
+    // constraint. Otherwise, use the create form sort order.
+    if ($select_ids) {
+      $forms = array_select_keys($forms, $select_ids) + $forms;
+    } else {
+      $forms = msort($forms, 'getCreateSortKey');
+    }
+
+    return $forms;
+  }
+
 }

From a6632f8c188a0b0b460b35003ccc0e41a45bb90e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Dec 2018 11:42:53 -0800
Subject: [PATCH 06/31] Allow "maniphest.subtypes" to configure which options
 are presented by "Create Subtask"

Summary:
Ref T13222. Ref T12588. See PHI683. After D19853, "Create Subtask" may pop a dialog to let you choose between multiple forms.

Allow users to configure which forms are available by using `maniphest.subtypes` to choose available children for each subtype. Users may either specify particular subtypes or specific forms.

Test Plan: Configured "Quest" tasks to have "Objective" children, got appropriate prompt behavior. Used "subtypes" and "forms" to select forms; used "forms" to reorder forms.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T12588

Differential Revision: https://secure.phabricator.com/D19854
---
 .../PhabricatorManiphestConfigOptions.php     | 50 +++++++++++++++++
 .../PhabricatorEditEngineSubtype.php          | 54 +++++++++++++++++++
 .../PhabricatorEditEngineSubtypeMap.php       | 14 +++--
 ...torEditEngineConfigurationSearchEngine.php | 33 +++++++++---
 4 files changed, 135 insertions(+), 16 deletions(-)

diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
index 4458dc3636..609db0d1b6 100644
--- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
+++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php
@@ -338,6 +338,8 @@ dictionary with these keys:
   - `tag` //Optional string.// Tag text for this subtype.
   - `color` //Optional string.// Display color for this subtype.
   - `icon` //Optional string.// Icon for the subtype.
+  - `children` //Optional map.// Configure options shown to the user when
+     they "Create Subtask". See below.
 
 Each subtype must have a unique key, and you must define a subtype with
 the key "%s", which is used as a default subtype.
@@ -345,6 +347,54 @@ the key "%s", which is used as a default subtype.
 The tag text (`tag`) is used to set the text shown in the subtype tag on list
 views and workboards. If you do not configure it, the default subtype will have
 no subtype tag and other subtypes will use their name as tag text.
+
+The `children` key allows you to configure which options are presented to the
+user when they "Create Subtask" from a task of this subtype. You can specify
+these keys:
+
+  - `subtypes`: //Optional list<string>.// Show users creation forms for these
+    task subtypes.
+  - `forms`: //Optional list<string|int>.// Show users these specific forms,
+    in order.
+
+If you don't specify either constraint, users will be shown creation forms
+for the same subtype.
+
+For example, if you have a "quest" subtype and do not configure `children`,
+users who click "Create Subtask" will be presented with all create forms for
+"quest" tasks.
+
+If you want to present them with forms for a different task subtype or set of
+subtypes instead, use `subtypes`:
+
+```
+  {
+    ...
+    "children": {
+      "subtypes": ["objective", "boss", "reward"]
+    }
+    ...
+  }
+```
+
+If you want to present them with specific forms, use `forms` and specify form
+IDs:
+
+```
+  {
+    ...
+    "children": {
+      "forms": [12, 16]
+    }
+    ...
+  }
+```
+
+When specifying forms by ID explicitly, the order you specify the forms in will
+be used when presenting options to the user.
+
+If only one option would be presented, the user will be taken directly to the
+appropriate form instead of being prompted to choose a form.
 EOTEXT
       ,
       $subtype_default_key));
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
index 23fac106d7..7aad1c9509 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
@@ -11,6 +11,8 @@ final class PhabricatorEditEngineSubtype
   private $icon;
   private $tagText;
   private $color;
+  private $childSubtypes = array();
+  private $childIdentifiers = array();
 
   public function setKey($key) {
     $this->key = $key;
@@ -57,6 +59,24 @@ final class PhabricatorEditEngineSubtype
     return $this->color;
   }
 
+  public function setChildSubtypes(array $child_subtypes) {
+    $this->childSubtypes = $child_subtypes;
+    return $this;
+  }
+
+  public function getChildSubtypes() {
+    return $this->childSubtypes;
+  }
+
+  public function setChildFormIdentifiers(array $child_identifiers) {
+    $this->childIdentifiers = $child_identifiers;
+    return $this;
+  }
+
+  public function getChildFormIdentifiers() {
+    return $this->childIdentifiers;
+  }
+
   public function hasTagView() {
     return (bool)strlen($this->getTagText());
   }
@@ -118,6 +138,7 @@ final class PhabricatorEditEngineSubtype
           'tag' => 'optional string',
           'color' => 'optional string',
           'icon' => 'optional string',
+          'children' => 'optional map<string, wild>',
         ));
 
       $key = $value['key'];
@@ -141,6 +162,27 @@ final class PhabricatorEditEngineSubtype
             'no name. Subtypes must have a name.',
             $key));
       }
+
+      $children = idx($value, 'children');
+      if ($children) {
+        PhutilTypeSpec::checkMap(
+          $children,
+          array(
+            'subtypes' => 'optional list<string>',
+            'forms' => 'optional list<string|int>',
+          ));
+
+        $child_subtypes = idx($children, 'subtypes');
+        $child_forms = idx($children, 'forms');
+
+        if ($child_subtypes && $child_forms) {
+          throw new Exception(
+            pht(
+              'Subtype configuration is invalid: subtype with key "%s" '.
+              'specifies both child subtypes and child forms. Specify one '.
+              'or the other, but not both.'));
+        }
+      }
     }
 
     if (!isset($map[self::SUBTYPE_DEFAULT])) {
@@ -179,6 +221,18 @@ final class PhabricatorEditEngineSubtype
         $subtype->setColor($color);
       }
 
+      $children = idx($entry, 'children', array());
+      $child_subtypes = idx($children, 'subtypes');
+      $child_forms = idx($children, 'forms');
+
+      if ($child_subtypes) {
+        $subtype->setChildSubtypes($child_subtypes);
+      }
+
+      if ($child_forms) {
+        $subtype->setChildFormIdentifiers($child_forms);
+      }
+
       $map[$key] = $subtype;
     }
 
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
index c76911dbc3..638b665184 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtypeMap.php
@@ -46,16 +46,14 @@ final class PhabricatorEditEngineSubtypeMap
     $subtype_key = $object->getEditEngineSubtype();
     $subtype = $this->getSubtype($subtype_key);
 
-    // TODO: Allow subtype configuration to specify that children should be
-    // created from particular forms or subtypes.
-    $select_ids = array();
-    $select_subtypes = array();
+    $select_identifiers = $subtype->getChildFormIdentifiers();
+    $select_subtypes = $subtype->getChildSubtypes();
 
     $query = $edit_engine->newConfigurationQuery()
       ->withIsDisabled(false);
 
-    if ($select_ids) {
-      $query->withIDs($select_ids);
+    if ($select_identifiers) {
+      $query->withIdentifiers($select_identifiers);
     } else {
       // If we're selecting by subtype rather than selecting specific forms,
       // only select create forms.
@@ -73,8 +71,8 @@ final class PhabricatorEditEngineSubtypeMap
 
     // If we're selecting by ID, respect the order specified in the
     // constraint. Otherwise, use the create form sort order.
-    if ($select_ids) {
-      $forms = array_select_keys($forms, $select_ids) + $forms;
+    if ($select_identifiers) {
+      $forms = array_select_keys($forms, $select_identifiers) + $forms;
     } else {
       $forms = msort($forms, 'getCreateSortKey');
     }
diff --git a/src/applications/transactions/query/PhabricatorEditEngineConfigurationSearchEngine.php b/src/applications/transactions/query/PhabricatorEditEngineConfigurationSearchEngine.php
index 52d48f528b..694b4c48b6 100644
--- a/src/applications/transactions/query/PhabricatorEditEngineConfigurationSearchEngine.php
+++ b/src/applications/transactions/query/PhabricatorEditEngineConfigurationSearchEngine.php
@@ -115,14 +115,6 @@ final class PhabricatorEditEngineConfigurationSearchEngine
         ->setHeader($config->getDisplayName());
 
       $id = $config->getID();
-      if ($id) {
-        $item->addIcon('fa-file-text-o bluegrey', pht('Form %d', $id));
-        $key = $id;
-      } else {
-        $item->addIcon('fa-file-text bluegrey', pht('Builtin'));
-        $key = $config->getBuiltinKey();
-      }
-      $item->setHref("/transactions/editengine/{$engine_key}/view/{$key}/");
 
       if ($config->getIsDefault()) {
         $item->addAttribute(pht('Default Create Form'));
@@ -139,6 +131,31 @@ final class PhabricatorEditEngineConfigurationSearchEngine
         $item->setStatusIcon('fa-file-text-o green', pht('Enabled'));
       }
 
+      $subtype_key = $config->getSubtype();
+      if ($subtype_key !== PhabricatorEditEngineSubtype::SUBTYPE_DEFAULT) {
+        $engine = $config->getEngine();
+        if ($engine->supportsSubtypes()) {
+          $map = $engine->newSubtypeMap();
+          if ($map->isValidSubtype($subtype_key)) {
+            $subtype = $map->getSubtype($subtype_key);
+
+            $icon = $subtype->getIcon();
+            $color = $subtype->getColor();
+
+            $item->addIcon("{$icon} {$color}", $subtype->getName());
+          }
+        }
+      }
+
+      if ($id) {
+        $item->setObjectName(pht('Form %d', $id));
+        $key = $id;
+      } else {
+        $item->addIcon('fa-file-text bluegrey', pht('Builtin'));
+        $key = $config->getBuiltinKey();
+      }
+      $item->setHref("/transactions/editengine/{$engine_key}/view/{$key}/");
+
       $list->addItem($item);
     }
 

From 68b1dee1390d59fbfce55988f8e4d3810a7da2a7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 7 Dec 2018 06:04:07 -0800
Subject: [PATCH 07/31] Replace the "Choose Subtype" radio buttons dialog with
 a simpler "big stuff you click" sort of UI

Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.

Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.

In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:

  - It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
  - We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
    - It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
    - We can't have any other links inside the element (e.g., details or documentation).
  - We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
    - You can't right-click to interact with a button in the same way you can with a link.
    - Also not great for screenreaders.

Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".

This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.

If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).

Test Plan:
{F6053035}

  - Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222, T12588

Differential Revision: https://secure.phabricator.com/D19855
---
 resources/celerity/map.php                    | 18 ++++---
 .../controller/DrydockConsoleController.php   |  4 ++
 .../ManiphestTaskSubtaskController.php        | 51 +++++++++----------
 .../PhabricatorEditEngineSubtype.php          |  5 ++
 src/view/AphrontDialogView.php                |  2 +-
 src/view/phui/PHUIObjectItemView.php          | 28 ++++++----
 .../css/phui/object-item/phui-oi-big-ui.css   | 13 +++++
 .../rsrc/js/core/behavior-linked-container.js | 47 +++++++++++++++++
 8 files changed, 123 insertions(+), 45 deletions(-)
 create mode 100644 webroot/rsrc/js/core/behavior-linked-container.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index e5eb50bb58..4223d94095 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' => 'cff4ff6f',
+    'core.pkg.css' => '9d1148a4',
     'core.pkg.js' => '4bde473b',
     'differential.pkg.css' => '06dc617c',
     'differential.pkg.js' => 'ef0b989b',
@@ -127,7 +127,7 @@ return array(
     'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600',
     'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf',
     'rsrc/css/phui/calendar/phui-calendar.css' => 'f1ddf11c',
-    'rsrc/css/phui/object-item/phui-oi-big-ui.css' => '628f59de',
+    'rsrc/css/phui/object-item/phui-oi-big-ui.css' => '7a7c22af',
     'rsrc/css/phui/object-item/phui-oi-color.css' => 'cd2b9b77',
     'rsrc/css/phui/object-item/phui-oi-drag-ui.css' => '08f4ccc3',
     'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '9d9685d6',
@@ -469,6 +469,7 @@ return array(
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0',
     'rsrc/js/core/behavior-lightbox-attachments.js' => '6b31879a',
     'rsrc/js/core/behavior-line-linker.js' => '66a62306',
+    'rsrc/js/core/behavior-linked-container.js' => '291da458',
     'rsrc/js/core/behavior-more.js' => 'a80d0378',
     'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0',
     'rsrc/js/core/behavior-oncopy.js' => '2926fff2',
@@ -616,6 +617,7 @@ return array(
     'javelin-behavior-launch-icon-composer' => '48086888',
     'javelin-behavior-lightbox-attachments' => '6b31879a',
     'javelin-behavior-line-chart' => 'e4232876',
+    'javelin-behavior-linked-container' => '291da458',
     'javelin-behavior-maniphest-batch-selector' => 'ad54037e',
     'javelin-behavior-maniphest-list-editor' => 'a9f88de2',
     'javelin-behavior-maniphest-subpriority-editor' => '71237763',
@@ -832,7 +834,7 @@ return array(
     'phui-lightbox-css' => '0a035e40',
     'phui-list-view-css' => '38f8c9bd',
     'phui-object-box-css' => '9cff003c',
-    'phui-oi-big-ui-css' => '628f59de',
+    'phui-oi-big-ui-css' => '7a7c22af',
     'phui-oi-color-css' => 'cd2b9b77',
     'phui-oi-drag-ui-css' => '08f4ccc3',
     'phui-oi-flush-ui-css' => '9d9685d6',
@@ -1027,6 +1029,10 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    '291da458' => array(
+      'javelin-behavior',
+      'javelin-dom',
+    ),
     '2926fff2' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1348,9 +1354,6 @@ return array(
       'javelin-magical-init',
       'javelin-util',
     ),
-    '628f59de' => array(
-      'phui-oi-list-view-css',
-    ),
     '62dfea03' => array(
       'javelin-install',
       'javelin-util',
@@ -1508,6 +1511,9 @@ return array(
       'owners-path-editor',
       'javelin-behavior',
     ),
+    '7a7c22af' => array(
+      'phui-oi-list-view-css',
+    ),
     '7cbe244b' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/drydock/controller/DrydockConsoleController.php b/src/applications/drydock/controller/DrydockConsoleController.php
index 54adcf5ca5..d0ae358768 100644
--- a/src/applications/drydock/controller/DrydockConsoleController.php
+++ b/src/applications/drydock/controller/DrydockConsoleController.php
@@ -34,6 +34,7 @@ final class DrydockConsoleController extends DrydockController {
         ->setHeader(pht('Blueprints'))
         ->setImageIcon('fa-map-o')
         ->setHref($this->getApplicationURI('blueprint/'))
+        ->setClickable(true)
         ->addAttribute(
           pht(
             'Configure blueprints so Drydock can build resources, like '.
@@ -44,6 +45,7 @@ final class DrydockConsoleController extends DrydockController {
         ->setHeader(pht('Resources'))
         ->setImageIcon('fa-map')
         ->setHref($this->getApplicationURI('resource/'))
+        ->setClickable(true)
         ->addAttribute(
           pht('View and manage resources Drydock has built, like hosts.')));
 
@@ -52,6 +54,7 @@ final class DrydockConsoleController extends DrydockController {
         ->setHeader(pht('Leases'))
         ->setImageIcon('fa-link')
         ->setHref($this->getApplicationURI('lease/'))
+        ->setClickable(true)
         ->addAttribute(pht('Manage leases on resources.')));
 
     $menu->addItem(
@@ -59,6 +62,7 @@ final class DrydockConsoleController extends DrydockController {
         ->setHeader(pht('Repository Operations'))
         ->setImageIcon('fa-fighter-jet')
         ->setHref($this->getApplicationURI('operation/'))
+        ->setClickable(true)
         ->addAttribute(pht('Review the repository operation queue.')));
 
     $crumbs = $this->buildApplicationCrumbs();
diff --git a/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
index 9da0ed4206..5256c1bd26 100644
--- a/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
@@ -37,39 +37,34 @@ final class ManiphestTaskSubtaskController
         ->addCancelButton($cancel_uri, pht('Close'));
     }
 
-    if ($request->isFormPost()) {
-      $form_key = $request->getStr('formKey');
-      if (isset($subtype_options[$form_key])) {
-        $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
-          ->setQueryParam('parent', $id)
-          ->setQueryParam('template', $id)
-          ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
-        $subtask_uri = $this->getApplicationURI($subtask_uri);
+    $menu = id(new PHUIObjectItemListView())
+      ->setUser($viewer)
+      ->setBig(true)
+      ->setFlush(true);
 
-        return id(new AphrontRedirectResponse())
-          ->setURI($subtask_uri);
-      }
+    foreach ($subtype_options as $form_key => $subtype_form) {
+      $subtype_key = $subtype_form->getSubtype();
+      $subtype = $subtype_map->getSubtype($subtype_key);
+
+      $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
+        ->setQueryParam('parent', $id)
+        ->setQueryParam('template', $id)
+        ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
+      $subtask_uri = $this->getApplicationURI($subtask_uri);
+
+      $item = id(new PHUIObjectItemView())
+        ->setHeader($subtype_form->getDisplayName())
+        ->setHref($subtask_uri)
+        ->setClickable(true)
+        ->setImageIcon($subtype->newIconView())
+        ->addAttribute($subtype->getName());
+
+      $menu->addItem($item);
     }
 
-    $control = id(new AphrontFormRadioButtonControl())
-      ->setName('formKey')
-      ->setLabel(pht('Subtype'));
-
-    foreach ($subtype_options as $key => $subtype_form) {
-      $control->addButton(
-        $key,
-        $subtype_form->getDisplayName(),
-        null);
-    }
-
-    $form = id(new AphrontFormView())
-      ->setViewer($viewer)
-      ->appendControl($control);
-
     return $this->newDialog()
       ->setTitle(pht('Choose Subtype'))
-      ->appendForm($form)
-      ->addSubmitButton(pht('Continue'))
+      ->appendChild($menu)
       ->addCancelButton($cancel_uri);
   }
 
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
index 7aad1c9509..9e754a3ca8 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php
@@ -239,4 +239,9 @@ final class PhabricatorEditEngineSubtype
     return new PhabricatorEditEngineSubtypeMap($map);
   }
 
+  public function newIconView() {
+    return id(new PHUIIconView())
+      ->setIcon($this->getIcon(), $this->getColor());
+  }
+
 }
diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php
index 12bec92843..52681845d2 100644
--- a/src/view/AphrontDialogView.php
+++ b/src/view/AphrontDialogView.php
@@ -404,7 +404,7 @@ final class AphrontDialogView
         $header),
       phutil_tag('div',
         array(
-          'class' => 'aphront-dialog-body phabricator-remarkup grouped',
+          'class' => 'aphront-dialog-body grouped',
         ),
         $children),
       $tail,
diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php
index 0657086c49..565b9c2165 100644
--- a/src/view/phui/PHUIObjectItemView.php
+++ b/src/view/phui/PHUIObjectItemView.php
@@ -28,6 +28,7 @@ final class PHUIObjectItemView extends AphrontTagView {
   private $sideColumn;
   private $coverImage;
   private $description;
+  private $clickable;
 
   private $selectableName;
   private $selectableValue;
@@ -179,6 +180,15 @@ final class PHUIObjectItemView extends AphrontTagView {
     return $this;
   }
 
+  public function setClickable($clickable) {
+    $this->clickable = $clickable;
+    return $this;
+  }
+
+  public function getClickable() {
+    return $this->clickable;
+  }
+
   public function setEpoch($epoch) {
     $date = phabricator_datetime($epoch, $this->getUser());
     $this->addIcon('none', $date);
@@ -332,6 +342,13 @@ final class PHUIObjectItemView extends AphrontTagView {
       $item_classes[] = 'phui-oi-with-image-icon';
     }
 
+    if ($this->getClickable()) {
+      Javelin::initBehavior('linked-container');
+
+      $item_classes[] = 'phui-oi-linked-container';
+      $sigils[] = 'linked-container';
+    }
+
     return array(
       'class' => $item_classes,
       'sigil' => $sigils,
@@ -443,15 +460,6 @@ final class PHUIObjectItemView extends AphrontTagView {
           ),
           $spec['label']);
 
-        if (isset($spec['attributes']['href'])) {
-          $icon_href = phutil_tag(
-            'a',
-            array('href' => $spec['attributes']['href']),
-            array($icon, $label));
-        } else {
-          $icon_href = array($icon, $label);
-        }
-
         $classes = array();
         $classes[] = 'phui-oi-icon';
         if (isset($spec['attributes']['class'])) {
@@ -463,7 +471,7 @@ final class PHUIObjectItemView extends AphrontTagView {
           array(
             'class' => implode(' ', $classes),
           ),
-          $icon_href);
+          $icon);
       }
 
       $icons[] = phutil_tag(
diff --git a/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css b/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css
index 67ce566733..6f60560a2e 100644
--- a/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css
+++ b/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css
@@ -59,3 +59,16 @@
   background-color: {$hoverblue};
   border-radius: 3px;
 }
+
+.device-desktop .phui-oi-linked-container {
+  cursor: pointer;
+}
+
+.device-desktop .phui-oi-linked-container:hover {
+  background-color: {$hoverblue};
+  border-radius: 3px;
+}
+
+.device-desktop .phui-oi-linked-container a:hover {
+  text-decoration: none;
+}
diff --git a/webroot/rsrc/js/core/behavior-linked-container.js b/webroot/rsrc/js/core/behavior-linked-container.js
new file mode 100644
index 0000000000..9721f91b22
--- /dev/null
+++ b/webroot/rsrc/js/core/behavior-linked-container.js
@@ -0,0 +1,47 @@
+/**
+ * @provides javelin-behavior-linked-container
+ * @requires javelin-behavior javelin-dom
+ */
+
+JX.behavior('linked-container', function() {
+
+  JX.Stratcom.listen(
+    'click',
+    'linked-container',
+    function(e) {
+
+      // If the user clicked some link inside the container, bail out and just
+      // click the link.
+      if (e.getNode('tag:a')) {
+        return;
+      }
+
+      // If this is some sort of unusual click, bail out. Note that we'll
+      // handle "Left Click" and "Command + Left Click" differently, below.
+      if (!e.isLeftButton()) {
+        return;
+      }
+
+      var container = e.getNode('linked-container');
+
+      // Find the first link in the container. We're going to pretend the user
+      // clicked it.
+      var link = JX.DOM.scry(container, 'a')[0];
+      if (!link) {
+        return;
+      }
+
+      // If the click is a "Command + Left Click", change the target of the
+      // link so we open it in a new tab.
+      var is_command = !!e.getRawEvent().metaKey;
+      if (is_command) {
+        var old_target = link.target;
+        link.target = '_blank';
+        link.click();
+        link.target = old_target;
+      } else {
+        link.click();
+      }
+    });
+
+});

From 46feccdfcf1963d7d8d38b2c282b1e702f059a76 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 08:20:22 -0800
Subject: [PATCH 08/31] Share more inline "Done" code between Differential and
 Diffusion

Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.

Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.

(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)

Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19858
---
 .../audit/editor/PhabricatorAuditEditor.php   | 45 ++----------
 .../editor/DifferentialRevisionEditEngine.php | 37 +++-------
 .../editor/DifferentialTransactionEditor.php  | 48 ++-----------
 .../editor/DiffusionCommitEditEngine.php      | 37 +++-------
 ...habricatorApplicationTransactionEditor.php | 71 +++++++++++++++++++
 5 files changed, 103 insertions(+), 135 deletions(-)

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index 3f65cc80f8..165f8ef84f 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -251,47 +251,16 @@ final class PhabricatorAuditEditor
         case PhabricatorTransactions::TYPE_COMMENT:
           $this->didExpandInlineState = true;
 
-          $actor_phid = $this->getActingAsPHID();
-          $author_phid = $object->getAuthorPHID();
-          $actor_is_author = ($actor_phid == $author_phid);
+          $query_template = id(new DiffusionDiffInlineCommentQuery())
+            ->withCommitPHIDs(array($object->getPHID()));
 
-          $state_map = PhabricatorTransactions::getInlineStateMap();
+          $state_xaction = $this->newInlineStateTransaction(
+            $object,
+            $query_template);
 
-          $query = id(new DiffusionDiffInlineCommentQuery())
-            ->setViewer($this->getActor())
-            ->withCommitPHIDs(array($object->getPHID()))
-            ->withFixedStates(array_keys($state_map));
-
-          $inlines = array();
-
-          $inlines[] = id(clone $query)
-            ->withAuthorPHIDs(array($actor_phid))
-            ->withHasTransaction(false)
-            ->execute();
-
-          if ($actor_is_author) {
-            $inlines[] = id(clone $query)
-              ->withHasTransaction(true)
-              ->execute();
+          if ($state_xaction) {
+            $xactions[] = $state_xaction;
           }
-
-          $inlines = array_mergev($inlines);
-
-          if (!$inlines) {
-            break;
-          }
-
-          $old_value = mpull($inlines, 'getFixedState', 'getPHID');
-          $new_value = array();
-          foreach ($old_value as $key => $state) {
-            $new_value[$key] = $state_map[$state];
-          }
-
-          $xactions[] = id(new PhabricatorAuditTransaction())
-            ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
-            ->setIgnoreOnNoEffect(true)
-            ->setOldValue($old_value)
-            ->setNewValue($new_value);
           break;
       }
     }
diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index ffae7fab16..07b693044f 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -279,36 +279,17 @@ final class DifferentialRevisionEditEngine
       $object);
     $inlines = msort($inlines, 'getID');
 
-    foreach ($inlines as $inline) {
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType(DifferentialTransaction::TYPE_INLINE)
-        ->attachComment($inline);
-    }
+    $editor = $object->getApplicationTransactionEditor()
+      ->setActor($viewer);
 
-    $viewer_phid = $viewer->getPHID();
-    $viewer_is_author = ($object->getAuthorPHID() == $viewer_phid);
-    if ($viewer_is_author) {
-      $state_map = PhabricatorTransactions::getInlineStateMap();
+    $query_template = id(new DifferentialDiffInlineCommentQuery())
+      ->withRevisionPHIDs(array($object->getPHID()));
 
-      $inlines = id(new DifferentialDiffInlineCommentQuery())
-        ->setViewer($viewer)
-        ->withRevisionPHIDs(array($object->getPHID()))
-        ->withFixedStates(array_keys($state_map))
-        ->execute();
-      if ($inlines) {
-        $old_value = mpull($inlines, 'getFixedState', 'getPHID');
-        $new_value = array();
-        foreach ($old_value as $key => $state) {
-          $new_value[$key] = $state_map[$state];
-        }
-
-        $xactions[] = id(new DifferentialTransaction())
-          ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
-          ->setIgnoreOnNoEffect(true)
-          ->setOldValue($old_value)
-          ->setNewValue($new_value);
-      }
-    }
+    $xactions = $editor->newAutomaticInlineTransactions(
+      $object,
+      $inlines,
+      DifferentialTransaction::TYPE_INLINE,
+      $query_template);
 
     return $xactions;
   }
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 853b024e27..8072c19e9a 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -247,50 +247,16 @@ final class DifferentialTransactionEditor
         case DifferentialTransaction::TYPE_INLINE:
           $this->didExpandInlineState = true;
 
-          $actor_phid = $this->getActingAsPHID();
-          $author_phid = $object->getAuthorPHID();
-          $actor_is_author = ($actor_phid == $author_phid);
+          $query_template = id(new DifferentialDiffInlineCommentQuery())
+            ->withRevisionPHIDs(array($object->getPHID()));
 
-          $state_map = PhabricatorTransactions::getInlineStateMap();
+          $state_xaction = $this->newInlineStateTransaction(
+            $object,
+            $query_template);
 
-          $query = id(new DifferentialDiffInlineCommentQuery())
-            ->setViewer($this->getActor())
-            ->withRevisionPHIDs(array($object->getPHID()))
-            ->withFixedStates(array_keys($state_map));
-
-          $inlines = array();
-
-          // We're going to undraft any "done" marks on your own inlines.
-          $inlines[] = id(clone $query)
-            ->withAuthorPHIDs(array($actor_phid))
-            ->withHasTransaction(false)
-            ->execute();
-
-          // If you're the author, we also undraft any "done" marks on other
-          // inlines.
-          if ($actor_is_author) {
-            $inlines[] = id(clone $query)
-              ->withHasTransaction(true)
-              ->execute();
+          if ($state_xaction) {
+            $results[] = $state_xaction;
           }
-
-          $inlines = array_mergev($inlines);
-
-          if (!$inlines) {
-            break;
-          }
-
-          $old_value = mpull($inlines, 'getFixedState', 'getPHID');
-          $new_value = array();
-          foreach ($old_value as $key => $state) {
-            $new_value[$key] = $state_map[$state];
-          }
-
-          $results[] = id(new DifferentialTransaction())
-            ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
-            ->setIgnoreOnNoEffect(true)
-            ->setOldValue($old_value)
-            ->setNewValue($new_value);
           break;
       }
     }
diff --git a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
index 8392504def..667af46550 100644
--- a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
+++ b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php
@@ -170,36 +170,17 @@ final class DiffusionCommitEditEngine
       $raw = true);
     $inlines = msort($inlines, 'getID');
 
-    foreach ($inlines as $inline) {
-      $xactions[] = $object->getApplicationTransactionTemplate()
-        ->setTransactionType(PhabricatorAuditActionConstants::INLINE)
-        ->attachComment($inline);
-    }
+    $editor = $object->getApplicationTransactionEditor()
+      ->setActor($viewer);
 
-    $viewer_phid = $viewer->getPHID();
-    $viewer_is_author = ($object->getAuthorPHID() == $viewer_phid);
-    if ($viewer_is_author) {
-      $state_map = PhabricatorTransactions::getInlineStateMap();
+    $query_template = id(new DiffusionDiffInlineCommentQuery())
+      ->withCommitPHIDs(array($object->getPHID()));
 
-      $inlines = id(new DiffusionDiffInlineCommentQuery())
-        ->setViewer($viewer)
-        ->withCommitPHIDs(array($object->getPHID()))
-        ->withFixedStates(array_keys($state_map))
-        ->execute();
-      if ($inlines) {
-        $old_value = mpull($inlines, 'getFixedState', 'getPHID');
-        $new_value = array();
-        foreach ($old_value as $key => $state) {
-          $new_value[$key] = $state_map[$state];
-        }
-
-        $xactions[] = $object->getApplicationTransactionTemplate()
-          ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
-          ->setIgnoreOnNoEffect(true)
-          ->setOldValue($old_value)
-          ->setNewValue($new_value);
-      }
-    }
+    $xactions = $editor->newAutomaticInlineTransactions(
+      $object,
+      $inlines,
+      PhabricatorAuditActionConstants::INLINE,
+      $query_template);
 
     return $xactions;
   }
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index f354de6a1a..634b4b92db 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -4594,4 +4594,75 @@ abstract class PhabricatorApplicationTransactionEditor
     return $mail;
   }
 
+  public function newAutomaticInlineTransactions(
+    PhabricatorLiskDAO $object,
+    array $inlines,
+    $transaction_type,
+    PhabricatorCursorPagedPolicyAwareQuery $query_template) {
+
+    $xactions = array();
+
+    foreach ($inlines as $inline) {
+      $xactions[] = $object->getApplicationTransactionTemplate()
+        ->setTransactionType($transaction_type)
+        ->attachComment($inline);
+    }
+
+    $state_xaction = $this->newInlineStateTransaction(
+      $object,
+      $query_template);
+
+    if ($state_xaction) {
+      $xactions[] = $state_xaction;
+    }
+
+    return $xactions;
+  }
+
+  protected function newInlineStateTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorCursorPagedPolicyAwareQuery $query_template) {
+
+    $actor_phid = $this->getActingAsPHID();
+    $author_phid = $object->getAuthorPHID();
+    $actor_is_author = ($actor_phid == $author_phid);
+
+    $state_map = PhabricatorTransactions::getInlineStateMap();
+
+    $query = id(clone $query_template)
+      ->setViewer($this->getActor())
+      ->withFixedStates(array_keys($state_map));
+
+    $inlines = array();
+
+    $inlines[] = id(clone $query)
+      ->withAuthorPHIDs(array($actor_phid))
+      ->withHasTransaction(false)
+      ->execute();
+
+    if ($actor_is_author) {
+      $inlines[] = id(clone $query)
+        ->withHasTransaction(true)
+        ->execute();
+    }
+
+    $inlines = array_mergev($inlines);
+
+    if (!$inlines) {
+      return null;
+    }
+
+    $old_value = mpull($inlines, 'getFixedState', 'getPHID');
+    $new_value = array();
+    foreach ($old_value as $key => $state) {
+      $new_value[$key] = $state_map[$state];
+    }
+
+    return $object->getApplicationTransactionTemplate()
+      ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
+      ->setIgnoreOnNoEffect(true)
+      ->setOldValue($old_value)
+      ->setNewValue($new_value);
+  }
+
 }

From 508df60a621715778ee9d1f8064874da6dd770cd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 10:58:51 -0800
Subject: [PATCH 09/31] When users mark their own inline comments as "Done",
 suppress the timeline/mail stories

Summary:
Depends on D19858. Ref T13222. See PHI995. In D19635 and related revisions, inline behavior changed to allow you to pre-mark your own inlines as done (as a reviewer) and to pre-mark your inlines for you (as an author).

These actions generate low-value stories in the timeline, like "alice marked 3 comments done." when an author adds some notes to their own revision. These aren't helpful and can be a little misleading.

Instead, just don't count it when someone marks their own inlines as "done". If we throw away all the marks after throwing away the self-marks, hide the whole story.

This happens in three cases:

  # You comment on your own revision, and don't uncheck the "Done" checkbox.
  # You comment on someone else's revision, and check the "Done" checkbox before submitting.
  # You leave a not-"Done" inline on your own revision, then "Done" it later.

Cases (1) and (2) seem unambiguously good/clear. Case (3) is a little more questionable, but I think this still isn't very useful for reviewers.

If there's still a clarity issue around case (3), we could change the story text to "alice marked 3 inline comments by other users as done.", but I think this is probably needlessly verbose and that no one will be confused by the behavior as written here.

(Also note that this story is never shown in feed.)

Test Plan: Created and marked a bunch of inlines as "Done" in Differential and Diffusion, as the author and reviewer/auditor. My own marks didn't generate timeline stories; marking others' comments still does.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19859
---
 ...habricatorApplicationTransactionEditor.php | 12 ++++
 .../PhabricatorApplicationTransaction.php     | 63 ++++++++++++++++---
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 634b4b92db..378b96adb4 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -4658,9 +4658,21 @@ abstract class PhabricatorApplicationTransactionEditor
       $new_value[$key] = $state_map[$state];
     }
 
+    // See PHI995. Copy some information about the inlines into the transaction
+    // so we can tailor rendering behavior. In particular, we don't want to
+    // render transactions about users marking their own inlines as "Done".
+
+    $inline_details = array();
+    foreach ($inlines as $inline) {
+      $inline_details[$inline->getPHID()] = array(
+        'authorPHID' => $inline->getAuthorPHID(),
+      );
+    }
+
     return $object->getApplicationTransactionTemplate()
       ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE)
       ->setIgnoreOnNoEffect(true)
+      ->setMetadataValue('inline.details', $inline_details)
       ->setOldValue($old_value)
       ->setNewValue($new_value);
   }
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index c65adcac6b..5fc3385d27 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -664,6 +664,16 @@ abstract class PhabricatorApplicationTransaction
             break;
         }
         break;
+
+      case PhabricatorTransactions::TYPE_INLINESTATE:
+        list($done, $undone) = $this->getInterestingInlineStateChangeCounts();
+
+        if (!$done && !$undone) {
+          return true;
+        }
+
+        break;
+
     }
 
     return false;
@@ -1007,15 +1017,7 @@ abstract class PhabricatorApplicationTransaction
         }
 
       case PhabricatorTransactions::TYPE_INLINESTATE:
-        $done = 0;
-        $undone = 0;
-        foreach ($new as $phid => $state) {
-          if ($state == PhabricatorInlineCommentInterface::STATE_DONE) {
-            $done++;
-          } else {
-            $undone++;
-          }
-        }
+        list($done, $undone) = $this->getInterestingInlineStateChangeCounts();
         if ($done && $undone) {
           return pht(
             '%s marked %s inline comment(s) as done and %s inline comment(s) '.
@@ -1582,6 +1584,49 @@ abstract class PhabricatorApplicationTransaction
     return $moves;
   }
 
+  private function getInterestingInlineStateChangeCounts() {
+    // See PHI995. Newer inline state transactions have additional details
+    // which we use to tailor the rendering behavior. These details are not
+    // present on older transactions.
+    $details = $this->getMetadataValue('inline.details', array());
+
+    $new = $this->getNewValue();
+
+    $done = 0;
+    $undone = 0;
+    foreach ($new as $phid => $state) {
+      $is_done = ($state == PhabricatorInlineCommentInterface::STATE_DONE);
+
+      // See PHI995. If you're marking your own inline comments as "Done",
+      // don't count them when rendering a timeline story. In the case where
+      // you're only affecting your own comments, this will hide the
+      // "alice marked X comments as done" story entirely.
+
+      // Usually, this happens when you pre-mark inlines as "done" and submit
+      // them yourself. We'll still generate an "alice added inline comments"
+      // story (in most cases/contexts), but the state change story is largely
+      // just clutter and slightly confusing/misleading.
+
+      $inline_details = idx($details, $phid, array());
+      $inline_author_phid = idx($inline_details, 'authorPHID');
+      if ($inline_author_phid) {
+        if ($inline_author_phid == $this->getAuthorPHID()) {
+          if ($is_done) {
+            continue;
+          }
+        }
+      }
+
+      if ($is_done) {
+        $done++;
+      } else {
+        $undone++;
+      }
+    }
+
+    return array($done, $undone);
+  }
+
 
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 

From 55a1ef339f4534ab1a8e1a0f82818f78f3408fab Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 11:39:43 -0800
Subject: [PATCH 10/31] Fix a bad method call signature throwing exceptions in
 newer Node

Summary:
Ref T13222. See PHI996. Ref T10743. For context, perhaps see T12171.

Node changed some signatures, behaviors, and error handling here in recent versions. As far as I can tell:

  - The `script.runInNewContext(...)` method has never taken a `path` parameter, and passing the path has always been wrong.
  - The `script.runInNewContext(...)` method started taking an `[options]` parameter at some point, and validating it, so the bad `path` parameter now throws.
  - `vm.createScript(...)` is "soft deprecated" but basically fine, and keeping it looks more compatible.

This seems like the smallest and most compatible correct change.

Test Plan: Under Node 10, started Aphlict. Before: fatal error on bad `options` parameter to `runInNewContext()` (expected dictionary). After: notification server starts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T10743

Differential Revision: https://secure.phabricator.com/D19860
---
 resources/celerity/map.php                       | 2 +-
 webroot/rsrc/externals/javelin/core/init_node.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 4223d94095..3b86ebeeb4 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -214,7 +214,7 @@ return array(
     'rsrc/externals/javelin/core/__tests__/stratcom.js' => '88bf7313',
     'rsrc/externals/javelin/core/__tests__/util.js' => 'e251703d',
     'rsrc/externals/javelin/core/init.js' => '8d83d2a1',
-    'rsrc/externals/javelin/core/init_node.js' => 'c234aded',
+    'rsrc/externals/javelin/core/init_node.js' => 'f7732951',
     'rsrc/externals/javelin/core/install.js' => '05270951',
     'rsrc/externals/javelin/core/util.js' => '93cc50d6',
     'rsrc/externals/javelin/docs/Base.js' => '74676256',
diff --git a/webroot/rsrc/externals/javelin/core/init_node.js b/webroot/rsrc/externals/javelin/core/init_node.js
index 19834664b4..1a7999e253 100644
--- a/webroot/rsrc/externals/javelin/core/init_node.js
+++ b/webroot/rsrc/externals/javelin/core/init_node.js
@@ -49,7 +49,7 @@ JX.require = function(thing) {
   }
 
   vm.createScript(content, path)
-    .runInNewContext(sandbox, path);
+    .runInNewContext(sandbox);
 };
 
 exports.JX = JX;

From ba833805656e724aea17463618874e75bcd8612f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 11:32:13 -0800
Subject: [PATCH 11/31] Update the "Notification Test" workflow to use more
 modern mechanisms

Summary:
Depends on D19860. Ref T13222. Ref T10743. See PHI996.

Long ago, there were different types of feed stories. Over time, there was less and less need for this, and nowadays basically everything is a "transaction" feed story. Each story renders differently, but they're fundamentally all about transactions.

The Notification test controller still uses a custom type of feed story to send notifications. Move away from this, and apply a transaction against the user instead. This has the same ultimate effect, but involves less weird custom code from ages long forgotten.

This doesn't fix the actual problem with these things showing up in feed. Currently, stories always use the same rendering for feed and notifications, and there need to be some additional changes to fix this. So no behavioral change yet, just slightly more reasonable code.

Test Plan: Clicked the button and got some test notifications, with Aphlict running.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T10743

Differential Revision: https://secure.phabricator.com/D19861
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorNotificationTestController.php | 51 +++++++++----------
 ...abricatorPeopleProfileManageController.php |  6 +++
 .../PhabricatorUserTransactionEditor.php      | 14 +++++
 .../PhabricatorUserDisableTransaction.php     | 17 ++-----
 .../PhabricatorUserNotifyTransaction.php      | 34 +++++++++++++
 ...habricatorApplicationTransactionEditor.php | 25 +++++++--
 .../PhabricatorApplicationTransaction.php     |  9 ++++
 8 files changed, 115 insertions(+), 43 deletions(-)
 create mode 100644 src/applications/people/xaction/PhabricatorUserNotifyTransaction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 184c2a3d7c..e2586354e8 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4623,6 +4623,7 @@ phutil_register_library_map(array(
     'PhabricatorUserLogView' => 'applications/people/view/PhabricatorUserLogView.php',
     'PhabricatorUserMessageCountCacheType' => 'applications/people/cache/PhabricatorUserMessageCountCacheType.php',
     'PhabricatorUserNotificationCountCacheType' => 'applications/people/cache/PhabricatorUserNotificationCountCacheType.php',
+    'PhabricatorUserNotifyTransaction' => 'applications/people/xaction/PhabricatorUserNotifyTransaction.php',
     'PhabricatorUserPHIDResolver' => 'applications/phid/resolver/PhabricatorUserPHIDResolver.php',
     'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php',
     'PhabricatorUserPreferencesCacheType' => 'applications/people/cache/PhabricatorUserPreferencesCacheType.php',
@@ -10676,6 +10677,7 @@ phutil_register_library_map(array(
     'PhabricatorUserLogView' => 'AphrontView',
     'PhabricatorUserMessageCountCacheType' => 'PhabricatorUserCacheType',
     'PhabricatorUserNotificationCountCacheType' => 'PhabricatorUserCacheType',
+    'PhabricatorUserNotifyTransaction' => 'PhabricatorUserTransactionType',
     'PhabricatorUserPHIDResolver' => 'PhabricatorPHIDResolver',
     'PhabricatorUserPreferences' => array(
       'PhabricatorUserDAO',
diff --git a/src/applications/notification/controller/PhabricatorNotificationTestController.php b/src/applications/notification/controller/PhabricatorNotificationTestController.php
index 5c76e53357..f49f9d31a1 100644
--- a/src/applications/notification/controller/PhabricatorNotificationTestController.php
+++ b/src/applications/notification/controller/PhabricatorNotificationTestController.php
@@ -6,34 +6,31 @@ final class PhabricatorNotificationTestController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $request->getViewer();
 
-    $story_type = 'PhabricatorNotificationTestFeedStory';
-    $story_data = array(
-      'title' => pht(
-        'This is a test notification, sent at %s.',
-        phabricator_datetime(time(), $viewer)),
-    );
-
-    $viewer_phid = $viewer->getPHID();
-
-    // NOTE: Because we don't currently show you your own notifications, make
-    // sure this comes from a different PHID.
-    $application_phid = id(new PhabricatorNotificationsApplication())
-      ->getPHID();
-
-    // TODO: When it's easier to get these buttons to render as forms, this
-    // would be slightly nicer as a more standard isFormPost() check.
-
     if ($request->validateCSRF()) {
-      id(new PhabricatorFeedStoryPublisher())
-        ->setStoryType($story_type)
-        ->setStoryData($story_data)
-        ->setStoryTime(time())
-        ->setStoryAuthorPHID($application_phid)
-        ->setRelatedPHIDs(array($viewer_phid))
-        ->setPrimaryObjectPHID($viewer_phid)
-        ->setSubscribedPHIDs(array($viewer_phid))
-        ->setNotifyAuthor(true)
-        ->publish();
+      $message_text = pht(
+        'This is a test notification, sent at %s.',
+        phabricator_datetime(time(), $viewer));
+
+      // NOTE: Currently, the FeedStoryPublisher explicitly filters out
+      // notifications about your own actions. Send this notification from
+      // a different actor to get around this.
+      $application_phid = id(new PhabricatorNotificationsApplication())
+        ->getPHID();
+
+      $xactions = array();
+
+      $xactions[] = id(new PhabricatorUserTransaction())
+        ->setTransactionType(
+          PhabricatorUserNotifyTransaction::TRANSACTIONTYPE)
+        ->setNewValue($message_text)
+        ->setForceNotifyPHIDs(array($viewer->getPHID()));
+
+      $editor = id(new PhabricatorUserTransactionEditor())
+        ->setActor($viewer)
+        ->setActingAsPHID($application_phid)
+        ->setContentSourceFromRequest($request);
+
+      $editor->applyTransactions($viewer, $xactions);
     }
 
     return id(new AphrontAjaxResponse());
diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
index 9759a375c7..55f9311ada 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
@@ -43,6 +43,11 @@ final class PhabricatorPeopleProfileManageController
       ->setCurtain($curtain)
       ->addPropertySection(pht('Details'), $properties);
 
+    $timeline = $this->buildTransactionTimeline(
+      $user,
+      new PhabricatorPeopleTransactionQuery());
+    $timeline->setShouldTerminate(true);
+
     return $this->newPage()
       ->setTitle(
         array(
@@ -54,6 +59,7 @@ final class PhabricatorPeopleProfileManageController
       ->appendChild(
         array(
           $manage,
+          $timeline,
         ));
   }
 
diff --git a/src/applications/people/editor/PhabricatorUserTransactionEditor.php b/src/applications/people/editor/PhabricatorUserTransactionEditor.php
index c0dfa941af..929b2e224c 100644
--- a/src/applications/people/editor/PhabricatorUserTransactionEditor.php
+++ b/src/applications/people/editor/PhabricatorUserTransactionEditor.php
@@ -11,4 +11,18 @@ final class PhabricatorUserTransactionEditor
     return pht('Users');
   }
 
+  protected function shouldPublishFeedStory(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+    return true;
+  }
+
+  protected function getMailTo(PhabricatorLiskDAO $object) {
+    return array();
+  }
+
+  protected function getMailCC(PhabricatorLiskDAO $object) {
+    return array();
+  }
+
 }
diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
index ab399a28e0..629d538dee 100644
--- a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
+++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
@@ -35,19 +35,10 @@ final class PhabricatorUserDisableTransaction
     }
   }
 
-  public function getTitleForFeed() {
-    $new = $this->getNewValue();
-    if ($new) {
-      return pht(
-        '%s disabled %s.',
-        $this->renderAuthor(),
-        $this->renderObject());
-    } else {
-      return pht(
-        '%s enabled %s.',
-        $this->renderAuthor(),
-        $this->renderObject());
-    }
+  public function shouldHideForFeed() {
+    // Don't publish feed stories about disabling users, since this can be
+    // a sensitive action.
+    return true;
   }
 
   public function validateTransactions($object, array $xactions) {
diff --git a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php
new file mode 100644
index 0000000000..bc5657d22e
--- /dev/null
+++ b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php
@@ -0,0 +1,34 @@
+<?php
+
+final class PhabricatorUserNotifyTransaction
+  extends PhabricatorUserTransactionType {
+
+  const TRANSACTIONTYPE = 'notify';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function generateNewValue($object, $value) {
+    return $value;
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s sent this user a test notification.',
+      $this->renderAuthor());
+  }
+
+  public function getTitleForFeed() {
+    return $this->getNewValue();
+  }
+
+  public function shouldHideForFeed() {
+    return false;
+  }
+
+  public function shouldHideForMail() {
+    return true;
+  }
+
+}
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 378b96adb4..9e946a249e 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -3378,9 +3378,28 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    return array_unique(array_merge(
-      $this->getMailTo($object),
-      $this->getMailCC($object)));
+    // If some transactions are forcing notification delivery, add the forced
+    // recipients to the notify list.
+    $force_list = array();
+    foreach ($xactions as $xaction) {
+      $force_phids = $xaction->getForceNotifyPHIDs();
+
+      if (!$force_phids) {
+        continue;
+      }
+
+      foreach ($force_phids as $force_phid) {
+        $force_list[] = $force_phid;
+      }
+    }
+
+    $to_list = $this->getMailTo($object);
+    $cc_list = $this->getMailCC($object);
+
+    $full_list = array_merge($force_list, $to_list, $cc_list);
+    $full_list = array_fuse($full_list);
+
+    return array_keys($full_list);
   }
 
 
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 5fc3385d27..9e024b43aa 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -1662,6 +1662,15 @@ abstract class PhabricatorApplicationTransaction
     return null;
   }
 
+  public function setForceNotifyPHIDs(array $phids) {
+    $this->setMetadataValue('notify.force', $phids);
+    return $this;
+  }
+
+  public function getForceNotifyPHIDs() {
+    return $this->getMetadataValue('notify.force', array());
+  }
+
 
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 

From 773b4eaa9ea0999fd86fb9d883b186d867f60a09 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 12:40:24 -0800
Subject: [PATCH 12/31] Separate "feed" and "notifications" better, allow
 stories to appear in notifications only

Summary:
Depends on D19861. Ref T13222. See PHI996. Fixes T10743. Currently, notifications only work if a story also has a feed rendering.

Separate "visible in feed" and "visible in notifications", and make notifications query only notifications and vice versa.

Then, set the test notification stories to be visible in notifications only, not feed.

This could be refined a bit (there's no way to have the two views render different values today, for example) but since the only actual use case we have right now is test notifications I don't want to go //too// crazy future-proofing it. I could imagine doing some more of this kind of stuff in Conpherence eventually, though, perhaps.

Test Plan: Sent myself test notifications, saw them appear on my profile timeline and in the JS popup, and in my notifications menu, but not in feed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T10743

Differential Revision: https://secure.phabricator.com/D19864
---
 .../feed/query/PhabricatorFeedQuery.php       | 10 ++++-
 .../feed/story/PhabricatorFeedStory.php       |  8 ++++
 .../query/PhabricatorNotificationQuery.php    | 20 ++++++---
 .../PhabricatorUserNotifyTransaction.php      |  6 ++-
 .../PhabricatorNotificationsSettingsPanel.php |  2 +-
 ...habricatorApplicationTransactionEditor.php | 14 +++++-
 ...ricatorApplicationTransactionFeedStory.php | 44 ++++++++++++++++---
 .../PhabricatorApplicationTransaction.php     |  4 ++
 .../storage/PhabricatorModularTransaction.php | 11 +++++
 .../PhabricatorModularTransactionType.php     |  4 ++
 10 files changed, 107 insertions(+), 16 deletions(-)

diff --git a/src/applications/feed/query/PhabricatorFeedQuery.php b/src/applications/feed/query/PhabricatorFeedQuery.php
index ce472a6026..a35f14da57 100644
--- a/src/applications/feed/query/PhabricatorFeedQuery.php
+++ b/src/applications/feed/query/PhabricatorFeedQuery.php
@@ -34,7 +34,15 @@ final class PhabricatorFeedQuery
   }
 
   protected function willFilterPage(array $data) {
-    return PhabricatorFeedStory::loadAllFromRows($data, $this->getViewer());
+    $stories = PhabricatorFeedStory::loadAllFromRows($data, $this->getViewer());
+
+    foreach ($stories as $key => $story) {
+      if (!$story->isVisibleInFeed()) {
+        unset($stories[$key]);
+      }
+    }
+
+    return $stories;
   }
 
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php
index c3984573c7..e0c65c7dc2 100644
--- a/src/applications/feed/story/PhabricatorFeedStory.php
+++ b/src/applications/feed/story/PhabricatorFeedStory.php
@@ -418,6 +418,14 @@ abstract class PhabricatorFeedStory
     return array();
   }
 
+  public function isVisibleInFeed() {
+    return true;
+  }
+
+  public function isVisibleInNotifications() {
+    return true;
+  }
+
 
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 
diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php
index b40bdee066..0063c06e45 100644
--- a/src/applications/notification/query/PhabricatorNotificationQuery.php
+++ b/src/applications/notification/query/PhabricatorNotificationQuery.php
@@ -53,13 +53,13 @@ final class PhabricatorNotificationQuery
 
     $data = queryfx_all(
       $conn,
-      'SELECT story.*, notif.hasViewed FROM %T notif
-         JOIN %T story ON notif.chronologicalKey = story.chronologicalKey
+      'SELECT story.*, notif.hasViewed FROM %R notif
+         JOIN %R story ON notif.chronologicalKey = story.chronologicalKey
          %Q
          ORDER BY notif.chronologicalKey DESC
          %Q',
-      $notification_table->getTableName(),
-      $story_table->getTableName(),
+      $notification_table,
+      $story_table,
       $this->buildWhereClause($conn),
       $this->buildLimitClause($conn));
 
@@ -93,7 +93,7 @@ final class PhabricatorNotificationQuery
         (int)!$this->unread);
     }
 
-    if ($this->keys) {
+    if ($this->keys !== null) {
       $where[] = qsprintf(
         $conn,
         'notif.chronologicalKey IN (%Ls)',
@@ -103,6 +103,16 @@ final class PhabricatorNotificationQuery
     return $where;
   }
 
+  protected function willFilterPage(array $stories) {
+    foreach ($stories as $key => $story) {
+      if (!$story->isVisibleInNotifications()) {
+        unset($stories[$key]);
+      }
+    }
+
+    return $stories;
+  }
+
   protected function getResultCursor($item) {
     return $item->getChronologicalKey();
   }
diff --git a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php
index bc5657d22e..4a527f8265 100644
--- a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php
+++ b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php
@@ -23,10 +23,14 @@ final class PhabricatorUserNotifyTransaction
     return $this->getNewValue();
   }
 
-  public function shouldHideForFeed() {
+  public function shouldHideForNotifications() {
     return false;
   }
 
+  public function shouldHideForFeed() {
+    return true;
+  }
+
   public function shouldHideForMail() {
     return true;
   }
diff --git a/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php
index e75c2b99ee..797bcafcb3 100644
--- a/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php
@@ -143,7 +143,7 @@ final class PhabricatorNotificationsSettingsPanel
         ->setCaption(
           pht(
             'Phabricator can send real-time notifications to your web browser '.
-            'or to your desktop. Select where you\'d want to receive these '.
+            'or to your desktop. Select where you want to receive these '.
             'real-time updates.'))
         ->initBehavior(
           'desktop-notifications-control',
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 9e946a249e..738fcf098b 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -3428,7 +3428,19 @@ abstract class PhabricatorApplicationTransactionEditor
     array $xactions,
     array $mailed_phids) {
 
-    $xactions = mfilter($xactions, 'shouldHideForFeed', true);
+    // Remove transactions which don't publish feed stories or notifications.
+    // These never show up anywhere, so we don't need to do anything with them.
+    foreach ($xactions as $key => $xaction) {
+      if (!$xaction->shouldHideForFeed()) {
+        continue;
+      }
+
+      if (!$xaction->shouldHideForNotifications()) {
+        continue;
+      }
+
+      unset($xactions[$key]);
+    }
 
     if (!$xactions) {
       return;
diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
index 305e38ab09..0e8531a22d 100644
--- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
+++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
@@ -36,12 +36,7 @@ class PhabricatorApplicationTransactionFeedStory
       // time because it's cheap and gets us better results when things change
       // by letting the changes apply retroactively.
 
-      $xaction_phids = $this->getValue('transactionPHIDs');
-
-      $xactions = array();
-      foreach ($xaction_phids as $xaction_phid) {
-        $xactions[] = $this->getObject($xaction_phid);
-      }
+      $xactions = $this->getTransactions();
 
       foreach ($xactions as $key => $xaction) {
         if ($xaction->shouldHideForFeed()) {
@@ -52,7 +47,7 @@ class PhabricatorApplicationTransactionFeedStory
       if ($xactions) {
         $primary_phid = head($xactions)->getPHID();
       } else {
-        $primary_phid = head($xaction_phids);
+        $primary_phid = head($this->getValue('transactionPHIDs'));
       }
 
       $this->primaryTransactionPHID = $primary_phid;
@@ -61,6 +56,41 @@ class PhabricatorApplicationTransactionFeedStory
     return $this->primaryTransactionPHID;
   }
 
+  public function isVisibleInNotifications() {
+    $xactions = $this->getTransactions();
+
+    foreach ($xactions as $key => $xaction) {
+      if (!$xaction->shouldHideForNotifications()) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  public function isVisibleInFeed() {
+    $xactions = $this->getTransactions();
+
+    foreach ($xactions as $key => $xaction) {
+      if (!$xaction->shouldHideForFeed()) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  private function getTransactions() {
+    $xaction_phids = $this->getValue('transactionPHIDs');
+
+    $xactions = array();
+    foreach ($xaction_phids as $xaction_phid) {
+      $xactions[] = $this->getObject($xaction_phid);
+    }
+
+    return $xactions;
+  }
+
   public function getPrimaryTransaction() {
     return $this->getObject($this->getPrimaryTransactionPHID());
   }
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 9e024b43aa..f38c56acb3 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -759,6 +759,10 @@ abstract class PhabricatorApplicationTransaction
     return $this->shouldHide();
   }
 
+  public function shouldHideForNotifications() {
+    return $this->shouldHideForFeed();
+  }
+
   public function getTitleForMail() {
     return id(clone $this)->setRenderingTarget('text')->getTitle();
   }
diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php
index eef4b8e5d9..f6aece2a7f 100644
--- a/src/applications/transactions/storage/PhabricatorModularTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php
@@ -108,6 +108,17 @@ abstract class PhabricatorModularTransaction
     return parent::shouldHideForMail($xactions);
   }
 
+  final public function shouldHideForNotifications() {
+    $hide = $this->getTransactionImplementation()->shouldHideForNotifications();
+
+    // Returning "null" means "use the default behavior".
+    if ($hide === null) {
+      return parent::shouldHideForNotifications();
+    }
+
+    return $hide;
+  }
+
   /* final */ public function getIcon() {
     $icon = $this->getTransactionImplementation()->getIcon();
     if ($icon !== null) {
diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
index ca82af1c66..35dd3ac197 100644
--- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
@@ -59,6 +59,10 @@ abstract class PhabricatorModularTransactionType
     return false;
   }
 
+  public function shouldHideForNotifications() {
+    return null;
+  }
+
   public function getIcon() {
     return null;
   }

From e43f9124f8d4c2b0bc7950b8faea5f9058c5df86 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 14:24:30 -0800
Subject: [PATCH 13/31] Remove obsolete "NotifyTest" feed story

Summary: Depends on D19864. Ref T13222. See PHI996. This is no longer used by anything, so get rid of it.

Test Plan: Grepped; viewed a feed with these stories in it to make sure nothing crashed/exploded.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19865
---
 src/__phutil_library_map__.php                |  2 --
 .../PhabricatorNotificationBuilder.php        |  9 -------
 .../PhabricatorNotificationTestFeedStory.php  | 27 -------------------
 3 files changed, 38 deletions(-)
 delete mode 100644 src/applications/notification/feed/PhabricatorNotificationTestFeedStory.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index e2586354e8..9d7b783091 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3504,7 +3504,6 @@ phutil_register_library_map(array(
     'PhabricatorNotificationServersConfigType' => 'applications/notification/config/PhabricatorNotificationServersConfigType.php',
     'PhabricatorNotificationStatusView' => 'applications/notification/view/PhabricatorNotificationStatusView.php',
     'PhabricatorNotificationTestController' => 'applications/notification/controller/PhabricatorNotificationTestController.php',
-    'PhabricatorNotificationTestFeedStory' => 'applications/notification/feed/PhabricatorNotificationTestFeedStory.php',
     'PhabricatorNotificationUIExample' => 'applications/uiexample/examples/PhabricatorNotificationUIExample.php',
     'PhabricatorNotificationsApplication' => 'applications/notification/application/PhabricatorNotificationsApplication.php',
     'PhabricatorNotificationsSetting' => 'applications/settings/setting/PhabricatorNotificationsSetting.php',
@@ -9323,7 +9322,6 @@ phutil_register_library_map(array(
     'PhabricatorNotificationServersConfigType' => 'PhabricatorJSONConfigType',
     'PhabricatorNotificationStatusView' => 'AphrontTagView',
     'PhabricatorNotificationTestController' => 'PhabricatorNotificationController',
-    'PhabricatorNotificationTestFeedStory' => 'PhabricatorFeedStory',
     'PhabricatorNotificationUIExample' => 'PhabricatorUIExample',
     'PhabricatorNotificationsApplication' => 'PhabricatorApplication',
     'PhabricatorNotificationsSetting' => 'PhabricatorInternalSetting',
diff --git a/src/applications/notification/builder/PhabricatorNotificationBuilder.php b/src/applications/notification/builder/PhabricatorNotificationBuilder.php
index 026f55149c..b9a79be3f3 100644
--- a/src/applications/notification/builder/PhabricatorNotificationBuilder.php
+++ b/src/applications/notification/builder/PhabricatorNotificationBuilder.php
@@ -160,15 +160,6 @@ final class PhabricatorNotificationBuilder extends Phobject {
           'href'         => $story->getURI(),
           'icon'         => $story->getImageURI(),
         );
-      } else if ($story instanceof PhabricatorNotificationTestFeedStory) {
-        $dict[] = array(
-          'showAnyNotification' => $web_ready,
-          'showDesktopNotification' => $desktop_ready,
-          'title'        => pht('Test Notification'),
-          'body'         => $story->renderText(),
-          'href'         => null,
-          'icon'         => PhabricatorUser::getDefaultProfileImageURI(),
-        );
       } else {
         $dict[] = array(
           'showWebNotification' => false,
diff --git a/src/applications/notification/feed/PhabricatorNotificationTestFeedStory.php b/src/applications/notification/feed/PhabricatorNotificationTestFeedStory.php
deleted file mode 100644
index ad984431ba..0000000000
--- a/src/applications/notification/feed/PhabricatorNotificationTestFeedStory.php
+++ /dev/null
@@ -1,27 +0,0 @@
-<?php
-
-final class PhabricatorNotificationTestFeedStory extends PhabricatorFeedStory {
-
-  public function getPrimaryObjectPHID() {
-    return $this->getAuthorPHID();
-  }
-
-  public function renderView() {
-    $data = $this->getStoryData();
-
-    $author_phid = $data->getAuthorPHID();
-
-    $view = $this->newStoryView();
-
-    $view->setTitle($data->getValue('title'));
-    $view->setImage($this->getHandle($author_phid)->getImageURI());
-
-    return $view;
-  }
-
-  public function renderText() {
-    $data = $this->getStoryData();
-    return $data->getValue('title');
-  }
-
-}

From 05900a4cc990b0fc06b5b9c55dda99ef68b21d32 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 14:33:23 -0800
Subject: [PATCH 14/31] Add a CLI workflow for testing that notifications are
 being delivered

Summary: Depends on D19865. Ref T13222. See PHI996. Provide a `bin/aphlict notify --user ... --message ...` workflow for sending test notifications from the CLI.

Test Plan: {F6058287}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19866
---
 src/__phutil_library_map__.php                |  2 +
 ...ricatorAphlictManagementNotifyWorkflow.php | 81 +++++++++++++++++++
 2 files changed, 83 insertions(+)
 create mode 100644 src/applications/aphlict/management/PhabricatorAphlictManagementNotifyWorkflow.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9d7b783091..8bb0b89d0e 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2085,6 +2085,7 @@ phutil_register_library_map(array(
     'PhabricatorAmazonAuthProvider' => 'applications/auth/provider/PhabricatorAmazonAuthProvider.php',
     'PhabricatorAnchorView' => 'view/layout/PhabricatorAnchorView.php',
     'PhabricatorAphlictManagementDebugWorkflow' => 'applications/aphlict/management/PhabricatorAphlictManagementDebugWorkflow.php',
+    'PhabricatorAphlictManagementNotifyWorkflow' => 'applications/aphlict/management/PhabricatorAphlictManagementNotifyWorkflow.php',
     'PhabricatorAphlictManagementRestartWorkflow' => 'applications/aphlict/management/PhabricatorAphlictManagementRestartWorkflow.php',
     'PhabricatorAphlictManagementStartWorkflow' => 'applications/aphlict/management/PhabricatorAphlictManagementStartWorkflow.php',
     'PhabricatorAphlictManagementStatusWorkflow' => 'applications/aphlict/management/PhabricatorAphlictManagementStatusWorkflow.php',
@@ -7702,6 +7703,7 @@ phutil_register_library_map(array(
     'PhabricatorAmazonAuthProvider' => 'PhabricatorOAuth2AuthProvider',
     'PhabricatorAnchorView' => 'AphrontView',
     'PhabricatorAphlictManagementDebugWorkflow' => 'PhabricatorAphlictManagementWorkflow',
+    'PhabricatorAphlictManagementNotifyWorkflow' => 'PhabricatorAphlictManagementWorkflow',
     'PhabricatorAphlictManagementRestartWorkflow' => 'PhabricatorAphlictManagementWorkflow',
     'PhabricatorAphlictManagementStartWorkflow' => 'PhabricatorAphlictManagementWorkflow',
     'PhabricatorAphlictManagementStatusWorkflow' => 'PhabricatorAphlictManagementWorkflow',
diff --git a/src/applications/aphlict/management/PhabricatorAphlictManagementNotifyWorkflow.php b/src/applications/aphlict/management/PhabricatorAphlictManagementNotifyWorkflow.php
new file mode 100644
index 0000000000..a7653e74b5
--- /dev/null
+++ b/src/applications/aphlict/management/PhabricatorAphlictManagementNotifyWorkflow.php
@@ -0,0 +1,81 @@
+<?php
+
+final class PhabricatorAphlictManagementNotifyWorkflow
+  extends PhabricatorAphlictManagementWorkflow {
+
+  protected function didConstruct() {
+    $this
+      ->setName('notify')
+      ->setSynopsis(pht('Send a notification to a user.'))
+      ->setArguments(
+        array(
+          array(
+            'name' => 'user',
+            'param' => 'username',
+            'help' => pht('User to notify.'),
+          ),
+          array(
+            'name' => 'message',
+            'param' => 'text',
+            'help' => pht('Message to send.'),
+          ),
+        ));
+  }
+
+  public function execute(PhutilArgumentParser $args) {
+    $viewer = $this->getViewer();
+
+    $username = $args->getArg('user');
+    if (!strlen($username)) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Specify a user to notify with "--user".'));
+    }
+
+    $user = id(new PhabricatorPeopleQuery())
+      ->setViewer($viewer)
+      ->withUsernames(array($username))
+      ->executeOne();
+
+    if (!$user) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'No user with username "%s" exists.',
+          $username));
+    }
+
+    $message = $args->getArg('message');
+    if (!strlen($message)) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Specify a message to send with "--message".'));
+    }
+
+    $application_phid = id(new PhabricatorNotificationsApplication())
+      ->getPHID();
+
+    $content_source = $this->newContentSource();
+
+    $xactions = array();
+
+    $xactions[] = id(new PhabricatorUserTransaction())
+      ->setTransactionType(
+        PhabricatorUserNotifyTransaction::TRANSACTIONTYPE)
+      ->setNewValue($message)
+      ->setForceNotifyPHIDs(array($user->getPHID()));
+
+    $editor = id(new PhabricatorUserTransactionEditor())
+      ->setActor($viewer)
+      ->setActingAsPHID($application_phid)
+      ->setContentSource($content_source);
+
+    $editor->applyTransactions($user, $xactions);
+
+    echo tsprintf(
+      "%s\n",
+      pht('Sent notification.'));
+
+    return 0;
+  }
+
+}

From 0e067213fbb17e5f06d78fcb2081e04f3907e94c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Dec 2018 16:13:54 -0800
Subject: [PATCH 15/31] Make viewing a user's profile page clear notifications
 about that user

Summary: Ref T13222. See PHI996. This is a general correctness improvement, but also allows you to clear test notifications by clicking on them (since their default destination is the recipient's profile page).

Test Plan: Clicked a test notification, got taken to my profile page, saw notification marked as read.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19867
---
 .../controller/PhabricatorPeopleProfileViewController.php     | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/people/controller/PhabricatorPeopleProfileViewController.php b/src/applications/people/controller/PhabricatorPeopleProfileViewController.php
index 5dce872c4b..0c32075932 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileViewController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileViewController.php
@@ -74,6 +74,10 @@ final class PhabricatorPeopleProfileViewController
       ->setTitle($user->getUsername())
       ->setNavigation($nav)
       ->setCrumbs($crumbs)
+      ->setPageObjectPHIDs(
+        array(
+          $user->getPHID(),
+        ))
       ->appendChild(
         array(
           $home,

From d8e2bb9f0f51c73b1d13427eb494a1d2a3c583ba Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 05:20:36 -0800
Subject: [PATCH 16/31] Fix some straggling qsprintf() warnings in repository
 import

Summary:
Ref T13217. See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warnings-while-importing-git-commits/2191>.

Hunt down and fix two more `qsprintf()` things.

I just converted the "performance optimization" into a normal, safe call since we're dealing with far less SVN stuff nowadays and the actual issue has been lost in the mists of time. If it resurfaces, we can take another look.

Test Plan: Imported some commits, no longer saw these warnings in the daemon logs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19869
---
 .../pathchange/DiffusionPathChangeQuery.php   |  3 ++-
 ...atorRepositoryCommitChangeParserWorker.php | 24 ++++++++-----------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php b/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
index 8ae9b3d203..850c06f105 100644
--- a/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
+++ b/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
@@ -43,12 +43,13 @@ final class DiffusionPathChangeQuery extends Phobject {
 
     $conn_r = $repository->establishConnection('r');
 
-    $limit = '';
     if ($this->limit) {
       $limit = qsprintf(
         $conn_r,
         'LIMIT %d',
         $this->limit + 1);
+    } else {
+      $limit = qsprintf($conn_r, '');
     }
 
     $raw_changes = queryfx_all(
diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
index b544228561..f5d3af7fd8 100644
--- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
+++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
@@ -114,40 +114,36 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
     PhabricatorRepositoryCommit $commit,
     array $changes) {
 
+    $conn = $repository->establishConnection('w');
+
     $repository_id = (int)$repository->getID();
     $commit_id = (int)$commit->getID();
 
-    // NOTE: This SQL is being built manually instead of with qsprintf()
-    // because some SVN changes affect an enormous number of paths (millions)
-    // and this showed up as significantly slow on a profile at some point.
-
     $changes_sql = array();
     foreach ($changes as $change) {
-      $values = array(
+      $changes_sql[] = qsprintf(
+        $conn,
+        '(%d, %d, %d, %nd, %nd, %d, %d, %d, %d)',
         $repository_id,
         (int)$change->getPathID(),
         $commit_id,
-        nonempty((int)$change->getTargetPathID(), 'null'),
-        nonempty((int)$change->getTargetCommitID(), 'null'),
+        nonempty((int)$change->getTargetPathID(), null),
+        nonempty((int)$change->getTargetCommitID(), null),
         (int)$change->getChangeType(),
         (int)$change->getFileType(),
         (int)$change->getIsDirect(),
-        (int)$change->getCommitSequence(),
-      );
-      $changes_sql[] = '('.implode(', ', $values).')';
+        (int)$change->getCommitSequence());
     }
 
-    $conn_w = $repository->establishConnection('w');
-
     queryfx(
-      $conn_w,
+      $conn,
       'DELETE FROM %T WHERE commitID = %d',
       PhabricatorRepository::TABLE_PATHCHANGE,
       $commit_id);
 
     foreach (PhabricatorLiskDAO::chunkSQL($changes_sql) as $chunk) {
       queryfx(
-        $conn_w,
+        $conn,
         'INSERT INTO %T
           (repositoryID, pathID, commitID, targetPathID, targetCommitID,
             changeType, fileType, isDirect, commitSequence)

From 66ff6d4a2ca00b7d4fd15eda140c140735205159 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 08:09:13 -0800
Subject: [PATCH 17/31] Fix an issue with creating tasks directly into
 workboard columns

Summary:
See <https://discourse.phabricator-community.org/t/tasks-created-via-workboard-column-menu-are-moved-to-wrong-column/2200>. The recent `setIsConduitOnly()` / `setIsFormField()` change (in D19842) disrupted creating tasks directly into a column from the workboard UI.

This field //is// a form field, it just doesn't render a visible control.

Test Plan:
  - Created a task directly into a workboard column. Before: column selection ignored. After: appeared in correct column.
  - Used "move on workboard" comment action.
  - Edited tasks; edited forms for tasks. Didn't observe any collateral damage (weird "Column" fields being present).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19870
---
 src/applications/maniphest/editor/ManiphestEditEngine.php | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php
index 0b0b4d6758..dc9c56f840 100644
--- a/src/applications/maniphest/editor/ManiphestEditEngine.php
+++ b/src/applications/maniphest/editor/ManiphestEditEngine.php
@@ -169,7 +169,9 @@ EODOCS
         ->setConduitDocumentation($column_documentation)
         ->setAliases(array('columnPHID', 'columns', 'columnPHIDs'))
         ->setTransactionType(PhabricatorTransactions::TYPE_COLUMNS)
-        ->setIsFormField(false)
+        ->setIsReorderable(false)
+        ->setIsDefaultable(false)
+        ->setIsLockable(false)
         ->setCommentActionLabel(pht('Move on Workboard'))
         ->setCommentActionOrder(2000)
         ->setColumnMap($column_map),

From 265f1f9c4d8a73e26ed20693a364d3940420c55a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 09:29:27 -0800
Subject: [PATCH 18/31] Fix an issue with item list view icon labels (including
 Differential date updated times) not appearing in the UI

Summary: In D19855, I removed a no-longer-necessary link around icons in some cases, but incorrectly discarded labels in other cases. Restore labels.

Test Plan: Viewed Differential revision list, saw date stamps again.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19871
---
 src/view/phui/PHUIObjectItemView.php | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php
index 565b9c2165..e1c67c7f32 100644
--- a/src/view/phui/PHUIObjectItemView.php
+++ b/src/view/phui/PHUIObjectItemView.php
@@ -471,7 +471,10 @@ final class PHUIObjectItemView extends AphrontTagView {
           array(
             'class' => implode(' ', $classes),
           ),
-          $icon);
+          array(
+            $icon,
+            $label,
+          ));
       }
 
       $icons[] = phutil_tag(

From 2814d340367c4f369aa56e1a51480206adb44702 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 10:57:21 -0800
Subject: [PATCH 19/31] Fix a stray qsprintf() in the Herald rules engine when
 recording rule application to objects

Summary: Ref T13217. See PHI1006.

Test Plan: Touched an object with associated Herald rules.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19872
---
 src/applications/herald/engine/HeraldEngine.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php
index 739e83e4e8..d853e3eb9a 100644
--- a/src/applications/herald/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/HeraldEngine.php
@@ -243,9 +243,9 @@ final class HeraldEngine extends Phobject {
         }
         queryfx(
           $conn_w,
-          'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q',
+          'INSERT IGNORE INTO %T (phid, ruleID) VALUES %LQ',
           HeraldRule::TABLE_RULE_APPLIED,
-          implode(', ', $sql));
+          $sql);
       }
     }
   }

From 5cb462d511c7d938af8bfc2f6da70e7f72c0bdc7 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Wed, 12 Dec 2018 13:02:52 -0800
Subject: [PATCH 20/31] Show more of UTC offset when user's TZ is not an
 integer number of hours offset

Summary: See https://discourse.phabricator-community.org/t/personal-timezone-setting-mismatch-cleared-and-more-specific-cases/1680. The code has always worked correctly, but the resulting timezone mismatch warning messsage wasn't specific enough when the mismatch is by a non-integer number of hours.

Test Plan: Set timezone locally to Asia/Vladivostok and in Phabricator to Australia/Adelaide (which as of today's date are 30 minutes apart) and observed a more precise error message: F6061330

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19873
---
 .../PhabricatorSettingsTimezoneController.php       | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
index 2e2e174e0f..51f1747b9f 100644
--- a/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
+++ b/src/applications/settings/controller/PhabricatorSettingsTimezoneController.php
@@ -63,7 +63,7 @@ final class PhabricatorSettingsTimezoneController
 
     $server_offset = $viewer->getTimeZoneOffset();
 
-    if ($client_offset == $server_offset || $did_calibrate) {
+    if (($client_offset == $server_offset) || $did_calibrate) {
       return $this->newDialog()
         ->setTitle(pht('Timezone Calibrated'))
         ->appendParagraph(
@@ -113,12 +113,13 @@ final class PhabricatorSettingsTimezoneController
   }
 
   private function formatOffset($offset) {
-    $offset = $offset / 60;
-
-    if ($offset >= 0) {
-      return pht('UTC-%d', $offset);
+    $hours = $offset / 60;
+    // Non-integer number of hours off UTC?
+    if ($offset % 60) {
+      $minutes = abs($offset % 60);
+      return pht('UTC%+d:%02d', $hours, $minutes);
     } else {
-      return pht('UTC+%d', -$offset);
+      return pht('UTC%+d', $hours);
     }
   }
 

From aba99459238fcb04e095c7ddead2271f95cacbdc Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Wed, 12 Dec 2018 15:38:06 -0800
Subject: [PATCH 21/31] Move user approval to modular transactions

Summary: See https://discourse.phabricator-community.org/t/how-to-approve-user-via-conduit-api/2189. This particular use case doesn't seem very compelling, but moving this logic out of `PhabricatorUserEditor` is a win anyway.

Test Plan: Registered a new user, approved/unapproved them conduit, approved from the UI.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19877
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorPeopleApproveController.php    | 34 +++----
 .../editor/PhabricatorUserEditEngine.php      | 10 ++
 .../people/editor/PhabricatorUserEditor.php   | 39 --------
 .../PhabricatorUserApproveTransaction.php     | 96 +++++++++++++++++++
 .../PhabricatorUserDisableTransaction.php     |  2 +
 6 files changed, 121 insertions(+), 62 deletions(-)
 create mode 100644 src/applications/people/xaction/PhabricatorUserApproveTransaction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 8bb0b89d0e..f4c6faa4f1 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4597,6 +4597,7 @@ phutil_register_library_map(array(
     'PhabricatorUnknownContentSource' => 'infrastructure/contentsource/PhabricatorUnknownContentSource.php',
     'PhabricatorUnsubscribedFromObjectEdgeType' => 'applications/transactions/edges/PhabricatorUnsubscribedFromObjectEdgeType.php',
     'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php',
+    'PhabricatorUserApproveTransaction' => 'applications/people/xaction/PhabricatorUserApproveTransaction.php',
     'PhabricatorUserBadgesCacheType' => 'applications/people/cache/PhabricatorUserBadgesCacheType.php',
     'PhabricatorUserBlurbField' => 'applications/people/customfield/PhabricatorUserBlurbField.php',
     'PhabricatorUserCache' => 'applications/people/storage/PhabricatorUserCache.php',
@@ -10645,6 +10646,7 @@ phutil_register_library_map(array(
       'PhabricatorConduitResultInterface',
       'PhabricatorAuthPasswordHashInterface',
     ),
+    'PhabricatorUserApproveTransaction' => 'PhabricatorUserTransactionType',
     'PhabricatorUserBadgesCacheType' => 'PhabricatorUserCacheType',
     'PhabricatorUserBlurbField' => 'PhabricatorUserCustomField',
     'PhabricatorUserCache' => 'PhabricatorUserDAO',
diff --git a/src/applications/people/controller/PhabricatorPeopleApproveController.php b/src/applications/people/controller/PhabricatorPeopleApproveController.php
index 0e97ad6ee6..013f4371f6 100644
--- a/src/applications/people/controller/PhabricatorPeopleApproveController.php
+++ b/src/applications/people/controller/PhabricatorPeopleApproveController.php
@@ -24,30 +24,18 @@ final class PhabricatorPeopleApproveController
     }
 
     if ($request->isFormPost()) {
-      id(new PhabricatorUserEditor())
+      $xactions = array();
+
+      $xactions[] = id(new PhabricatorUserTransaction())
+        ->setTransactionType(PhabricatorUserApproveTransaction::TRANSACTIONTYPE)
+        ->setNewValue(true);
+
+      id(new PhabricatorUserTransactionEditor())
         ->setActor($viewer)
-        ->approveUser($user, true);
-
-      $title = pht(
-        'Phabricator Account "%s" Approved',
-        $user->getUsername());
-
-      $body = sprintf(
-        "%s\n\n  %s\n\n",
-        pht(
-          'Your Phabricator account (%s) has been approved by %s. You can '.
-          'login here:',
-          $user->getUsername(),
-          $viewer->getUsername()),
-        PhabricatorEnv::getProductionURI('/'));
-
-      $mail = id(new PhabricatorMetaMTAMail())
-        ->addTos(array($user->getPHID()))
-        ->addCCs(array($viewer->getPHID()))
-        ->setSubject('[Phabricator] '.$title)
-        ->setForceDelivery(true)
-        ->setBody($body)
-        ->saveAndSend();
+        ->setContentSourceFromRequest($request)
+        ->setContinueOnMissingFields(true)
+        ->setContinueOnNoEffect(true)
+        ->applyTransactions($user, $xactions);
 
       return id(new AphrontRedirectResponse())->setURI($done_uri);
     }
diff --git a/src/applications/people/editor/PhabricatorUserEditEngine.php b/src/applications/people/editor/PhabricatorUserEditEngine.php
index c4c1abf1e3..4b3a60a2fc 100644
--- a/src/applications/people/editor/PhabricatorUserEditEngine.php
+++ b/src/applications/people/editor/PhabricatorUserEditEngine.php
@@ -75,6 +75,16 @@ final class PhabricatorUserEditEngine
         ->setConduitDescription(pht('Disable or enable the user.'))
         ->setConduitTypeDescription(pht('True to disable the user.'))
         ->setValue($object->getIsDisabled()),
+      id(new PhabricatorBoolEditField())
+        ->setKey('approved')
+        ->setOptions(pht('Approved'), pht('Unapproved'))
+        ->setLabel(pht('Approved'))
+        ->setDescription(pht('Approve the user.'))
+        ->setTransactionType(PhabricatorUserApproveTransaction::TRANSACTIONTYPE)
+        ->setIsFormField(false)
+        ->setConduitDescription(pht('Approve or reject the user.'))
+        ->setConduitTypeDescription(pht('True to approve the user.'))
+        ->setValue($object->getIsApproved()),
     );
   }
 
diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php
index 58c2ba1ff1..a64c1cc2a9 100644
--- a/src/applications/people/editor/PhabricatorUserEditor.php
+++ b/src/applications/people/editor/PhabricatorUserEditor.php
@@ -293,45 +293,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
     return $this;
   }
 
-  /**
-   * @task role
-   */
-  public function approveUser(PhabricatorUser $user, $approve) {
-    $actor = $this->requireActor();
-
-    if (!$user->getID()) {
-      throw new Exception(pht('User has not been created yet!'));
-    }
-
-    $user->openTransaction();
-      $user->beginWriteLocking();
-
-        $user->reload();
-        if ($user->getIsApproved() == $approve) {
-          $user->endWriteLocking();
-          $user->killTransaction();
-          return $this;
-        }
-
-        $log = PhabricatorUserLog::initializeNewLog(
-          $actor,
-          $user->getPHID(),
-          PhabricatorUserLog::ACTION_APPROVE);
-        $log->setOldValue($user->getIsApproved());
-        $log->setNewValue($approve);
-
-        $user->setIsApproved($approve);
-        $user->save();
-
-        $log->save();
-
-      $user->endWriteLocking();
-    $user->saveTransaction();
-
-    return $this;
-  }
-
-
 /* -(  Adding, Removing and Changing Email  )-------------------------------- */
 
 
diff --git a/src/applications/people/xaction/PhabricatorUserApproveTransaction.php b/src/applications/people/xaction/PhabricatorUserApproveTransaction.php
new file mode 100644
index 0000000000..e458c5822c
--- /dev/null
+++ b/src/applications/people/xaction/PhabricatorUserApproveTransaction.php
@@ -0,0 +1,96 @@
+<?php
+
+final class PhabricatorUserApproveTransaction
+  extends PhabricatorUserTransactionType {
+
+  const TRANSACTIONTYPE = 'user.approve';
+
+  public function generateOldValue($object) {
+    return (bool)$object->getIsApproved();
+  }
+
+  public function generateNewValue($object, $value) {
+    return (bool)$value;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setIsApproved((int)$value);
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $user = $object;
+    $this->newUserLog(PhabricatorUserLog::ACTION_APPROVE)
+      ->setOldValue((bool)$user->getIsApproved())
+      ->setNewValue((bool)$value)
+      ->save();
+
+    $actor = $this->getActor();
+    $title = pht(
+      'Phabricator Account "%s" Approved',
+      $user->getUsername());
+
+    $body = sprintf(
+      "%s\n\n  %s\n\n",
+      pht(
+        'Your Phabricator account (%s) has been approved by %s. You can '.
+        'login here:',
+        $user->getUsername(),
+        $actor->getUsername()),
+      PhabricatorEnv::getProductionURI('/'));
+
+    $mail = id(new PhabricatorMetaMTAMail())
+      ->addTos(array($user->getPHID()))
+      ->addCCs(array($actor->getPHID()))
+      ->setSubject('[Phabricator] '.$title)
+      ->setForceDelivery(true)
+      ->setBody($body)
+      ->saveAndSend();
+  }
+
+  public function getTitle() {
+    $new = $this->getNewValue();
+    if ($new) {
+      return pht(
+        '%s approved this user.',
+        $this->renderAuthor());
+    } else {
+      return pht(
+        '%s rejected this user.',
+        $this->renderAuthor());
+    }
+  }
+
+  public function shouldHideForFeed() {
+    return true;
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $actor = $this->getActor();
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $is_approved = (bool)$object->getIsApproved();
+
+      if ((bool)$xaction->getNewValue() === $is_approved) {
+        continue;
+      }
+
+      if (!$actor->getIsAdmin()) {
+        $errors[] = $this->newInvalidError(
+          pht('You must be an administrator to approve users.'));
+      }
+    }
+
+    return $errors;
+  }
+
+  public function getRequiredCapabilities(
+    $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    // Unlike normal user edits, approvals require admin permissions, which
+    // is enforced by validateTransactions().
+
+    return null;
+  }
+}
diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
index 629d538dee..7a8a1c7966 100644
--- a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
+++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php
@@ -15,7 +15,9 @@ final class PhabricatorUserDisableTransaction
 
   public function applyInternalEffects($object, $value) {
     $object->setIsDisabled((int)$value);
+  }
 
+  public function applyExternalEffects($object, $value) {
     $this->newUserLog(PhabricatorUserLog::ACTION_DISABLE)
       ->setOldValue((bool)$object->getIsDisabled())
       ->setNewValue((bool)$value)

From 5c99163b7c8050e1ec62cd57b25573a3fa4812b4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 15:37:51 -0800
Subject: [PATCH 22/31] Remove application callers to
 "LiskDAO->loadRelatives()"

Summary: Ref T13218. See that task for some discussion. `loadRelatives()` is like `loadAllWhere(...)` except that it does enormous amounts of weird magic which we've moved away from.

Test Plan: Did not test whatsoever since these changes are in Releeph.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13218

Differential Revision: https://secure.phabricator.com/D19874
---
 .../conduit/ReleephGetBranchesConduitAPIMethod.php  |  8 +++-----
 .../ReleephDiffSizeFieldSpecification.php           | 13 +++++++------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php b/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
index 6e225ec8e3..469713ce39 100644
--- a/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
+++ b/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
@@ -30,11 +30,9 @@ final class ReleephGetBranchesConduitAPIMethod extends ReleephConduitAPIMethod {
     foreach ($projects as $project) {
       $repository = $project->getRepository();
 
-      $branches = $project->loadRelatives(
-        id(new ReleephBranch()),
-        'releephProjectID',
-        'getID',
-        'isActive = 1');
+      $branches = id(new ReleephBranch())->loadAllWhere(
+        'releephProjectID = %d AND isActive = 1',
+        $project->getID());
 
       foreach ($branches as $branch) {
         $full_branch_name = $branch->getName();
diff --git a/src/applications/releeph/field/specification/ReleephDiffSizeFieldSpecification.php b/src/applications/releeph/field/specification/ReleephDiffSizeFieldSpecification.php
index 5975f208e6..20c8d5e226 100644
--- a/src/applications/releeph/field/specification/ReleephDiffSizeFieldSpecification.php
+++ b/src/applications/releeph/field/specification/ReleephDiffSizeFieldSpecification.php
@@ -22,16 +22,17 @@ final class ReleephDiffSizeFieldSpecification
     }
     $diff_rev = $requested_object;
 
-    $diffs = $diff_rev->loadRelatives(
-      new DifferentialDiff(),
-      'revisionID',
-      'getID',
-      'creationMethod <> "commit"');
+    $diffs = id(new DifferentialDiff())->loadAllWhere(
+      'revisionID = %d AND creationMethod != %s',
+      $diff_rev->getID(),
+      'commit');
 
     $all_changesets = array();
     $most_recent_changesets = null;
     foreach ($diffs as $diff) {
-      $changesets = $diff->loadRelatives(new DifferentialChangeset(), 'diffID');
+      $changesets = id(new DifferentialChangeset())->loadAllWhere(
+        'diffID = %d',
+        $diff->getID());
       $all_changesets += $changesets;
       $most_recent_changesets = $changesets;
     }

From 793f185d292400e8b9ab713cd8c96ffe6b130e28 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 15:47:17 -0800
Subject: [PATCH 23/31] Remove application callsites to
 "LiskDAO->loadOneRelative()"

Summary: Ref T13218. This is like `loadOneWhere(...)` but with more dark magic. Get rid of it.

Test Plan:
- Forced `20130219.commitsummarymig.php` to hit this code and ran it with `bin/storage upgrade --force --apply ...`.
- Ran `20130409.commitdrev.php` with `bin/storage upgrade --force --apply ...`.
- Called `user.search` to indirectly get primary email information.
- Did not test Releeph at all.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13218

Differential Revision: https://secure.phabricator.com/D19876
---
 resources/sql/patches/20130219.commitsummarymig.php |  6 +++---
 resources/sql/patches/20130409.commitdrev.php       |  4 +++-
 src/applications/people/storage/PhabricatorUser.php | 11 +++--------
 .../conduit/ReleephGetBranchesConduitAPIMethod.php  |  7 +++----
 src/applications/releeph/storage/ReleephRequest.php | 13 ++++++-------
 5 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/resources/sql/patches/20130219.commitsummarymig.php b/resources/sql/patches/20130219.commitsummarymig.php
index 60bdd1542c..f47016804d 100644
--- a/resources/sql/patches/20130219.commitsummarymig.php
+++ b/resources/sql/patches/20130219.commitsummarymig.php
@@ -12,9 +12,9 @@ foreach ($commits as $commit) {
     continue;
   }
 
-  $data = $commit->loadOneRelative(
-    new PhabricatorRepositoryCommitData(),
-    'commitID');
+  $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+    'commitID = %d',
+    $commit->getID());
 
   if (!$data) {
     continue;
diff --git a/resources/sql/patches/20130409.commitdrev.php b/resources/sql/patches/20130409.commitdrev.php
index fb556f1846..a264e8edeb 100644
--- a/resources/sql/patches/20130409.commitdrev.php
+++ b/resources/sql/patches/20130409.commitdrev.php
@@ -8,7 +8,9 @@ $commit_table->establishConnection('w');
 $edges = 0;
 
 foreach (new LiskMigrationIterator($commit_table) as $commit) {
-  $data = $commit->loadOneRelative($data_table, 'commitID');
+  $data = $data_table->loadOneWhere(
+    'commitID = %d',
+    $commit->getID());
   if (!$data) {
     continue;
   }
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 7717fbf18c..5d310378e0 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -458,14 +458,9 @@ final class PhabricatorUser
   }
 
   public function loadPrimaryEmail() {
-    $email = new PhabricatorUserEmail();
-    $conn = $email->establishConnection('r');
-
-    return $this->loadOneRelative(
-      $email,
-      'userPHID',
-      'getPHID',
-      qsprintf($conn, '(isPrimary = 1)'));
+    return id(new PhabricatorUserEmail())->loadOneWhere(
+      'userPHID = %s AND isPrimary = 1',
+      $this->getPHID());
   }
 
 
diff --git a/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php b/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
index 469713ce39..5516b4edb6 100644
--- a/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
+++ b/src/applications/releeph/conduit/ReleephGetBranchesConduitAPIMethod.php
@@ -37,10 +37,9 @@ final class ReleephGetBranchesConduitAPIMethod extends ReleephConduitAPIMethod {
       foreach ($branches as $branch) {
         $full_branch_name = $branch->getName();
 
-        $cut_point_commit = $branch->loadOneRelative(
-          id(new PhabricatorRepositoryCommit()),
-          'phid',
-          'getCutPointCommitPHID');
+        $cut_point_commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
+          'phid = %s',
+          $branch->getCutPointCommitPHID());
 
         $results[] = array(
           'project'         => $project->getName(),
diff --git a/src/applications/releeph/storage/ReleephRequest.php b/src/applications/releeph/storage/ReleephRequest.php
index c071600ec7..a877192107 100644
--- a/src/applications/releeph/storage/ReleephRequest.php
+++ b/src/applications/releeph/storage/ReleephRequest.php
@@ -257,18 +257,17 @@ final class ReleephRequest extends ReleephDAO
 /* -(  Loading external objects  )------------------------------------------- */
 
   public function loadPhabricatorRepositoryCommit() {
-    return $this->loadOneRelative(
-      new PhabricatorRepositoryCommit(),
-      'phid',
-      'getRequestCommitPHID');
+    return id(new PhabricatorRepositoryCommit())->loadOneWhere(
+      'phid = %s',
+      $this->getRequestCommitPHID());
   }
 
   public function loadPhabricatorRepositoryCommitData() {
     $commit = $this->loadPhabricatorRepositoryCommit();
     if ($commit) {
-      return $commit->loadOneRelative(
-        new PhabricatorRepositoryCommitData(),
-        'commitID');
+      return id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+        'commitID = %d',
+        $commit->getID());
     }
   }
 

From 02933acbd5ae8c843020600c2125590cded1897e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 16:12:11 -0800
Subject: [PATCH 24/31] Remove all application callers to "putInSet()"

Summary: Ref T13218. This is the last public-facing API call for `loadRelatives/loadOneRelative`. This just "primed" objects to make the other calls work and had no direct effects.

Test Plan:
- Ran `bin/fact analyze`.
- Used `bin/storage upgrade -f --apply` to apply `20181031.board.01.queryreset.php`, which uses `LiskMigrationIterator`.
- Browsed user list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13218

Differential Revision: https://secure.phabricator.com/D19878
---
 .../fact/extract/PhabricatorFactUpdateIterator.php       | 7 +------
 src/applications/people/query/PhabricatorPeopleQuery.php | 9 +--------
 .../storage/lisk/LiskMigrationIterator.php               | 8 +++-----
 3 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/src/applications/fact/extract/PhabricatorFactUpdateIterator.php b/src/applications/fact/extract/PhabricatorFactUpdateIterator.php
index df77f4dd33..c0d229084f 100644
--- a/src/applications/fact/extract/PhabricatorFactUpdateIterator.php
+++ b/src/applications/fact/extract/PhabricatorFactUpdateIterator.php
@@ -12,11 +12,8 @@ final class PhabricatorFactUpdateIterator extends PhutilBufferedIterator {
   private $position;
   private $ignoreUpdatesDuration = 15;
 
-  private $set;
-
   public function __construct(LiskDAO $object) {
-    $this->set = new LiskDAOSet();
-    $this->object = $object->putInSet($this->set);
+    $this->object = $object;
   }
 
   public function setPosition($position) {
@@ -41,8 +38,6 @@ final class PhabricatorFactUpdateIterator extends PhutilBufferedIterator {
   }
 
   protected function loadPage() {
-    $this->set->clearSet();
-
     if ($this->object->hasProperty('dateModified')) {
       if ($this->cursor) {
         list($after_epoch, $after_id) = explode(':', $this->cursor);
diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php
index 131dc7c588..542b685e29 100644
--- a/src/applications/people/query/PhabricatorPeopleQuery.php
+++ b/src/applications/people/query/PhabricatorPeopleQuery.php
@@ -163,14 +163,7 @@ final class PhabricatorPeopleQuery
   }
 
   protected function loadPage() {
-    $table = new PhabricatorUser();
-    $data = $this->loadStandardPageRows($table);
-
-    if ($this->needPrimaryEmail) {
-      $table->putInSet(new LiskDAOSet());
-    }
-
-    return $table->loadAllFromArray($data);
+    return $this->loadStandardPage($this->newResultObject());
   }
 
   protected function didFilterPage(array $users) {
diff --git a/src/infrastructure/storage/lisk/LiskMigrationIterator.php b/src/infrastructure/storage/lisk/LiskMigrationIterator.php
index a46512f866..edd31c8123 100644
--- a/src/infrastructure/storage/lisk/LiskMigrationIterator.php
+++ b/src/infrastructure/storage/lisk/LiskMigrationIterator.php
@@ -17,11 +17,9 @@ final class LiskMigrationIterator extends PhutilBufferedIterator {
 
   private $object;
   private $cursor;
-  private $set;
 
   public function __construct(LiskDAO $object) {
-    $this->set = new LiskDAOSet();
-    $this->object = $object->putInSet($this->set);
+    $this->object = $object;
   }
 
   protected function didRewind() {
@@ -33,15 +31,15 @@ final class LiskMigrationIterator extends PhutilBufferedIterator {
   }
 
   protected function loadPage() {
-    $this->set->clearSet();
-
     $results = $this->object->loadAllWhere(
       'id > %d ORDER BY id ASC LIMIT %d',
       $this->cursor,
       $this->getPageSize());
+
     if ($results) {
       $this->cursor = last($results)->getID();
     }
+
     return $results;
   }
 

From 9aa5a52fbd1b4265633df88b337eb95acee9bd85 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 16:15:19 -0800
Subject: [PATCH 25/31] Completely remove "LiskDAOSet" and
 "loadRelatives/loadOneRelative"

Summary: Fixes T13218. We have no more callers to any of this and can get rid of it forever.

Test Plan: Grepped for all four API methods, `LiskDAOSet`, and `inSet`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13218

Differential Revision: https://secure.phabricator.com/D19879
---
 src/__phutil_library_map__.php                |   2 -
 src/infrastructure/storage/lisk/LiskDAO.php   | 170 ------------------
 .../storage/lisk/LiskDAOSet.php               | 101 -----------
 3 files changed, 273 deletions(-)
 delete mode 100644 src/infrastructure/storage/lisk/LiskDAOSet.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index f4c6faa4f1..09430367d9 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1633,7 +1633,6 @@ phutil_register_library_map(array(
     'LegalpadTransactionView' => 'applications/legalpad/view/LegalpadTransactionView.php',
     'LiskChunkTestCase' => 'infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php',
     'LiskDAO' => 'infrastructure/storage/lisk/LiskDAO.php',
-    'LiskDAOSet' => 'infrastructure/storage/lisk/LiskDAOSet.php',
     'LiskDAOTestCase' => 'infrastructure/storage/lisk/__tests__/LiskDAOTestCase.php',
     'LiskEphemeralObjectException' => 'infrastructure/storage/lisk/LiskEphemeralObjectException.php',
     'LiskFixtureTestCase' => 'infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php',
@@ -7200,7 +7199,6 @@ phutil_register_library_map(array(
       'Phobject',
       'AphrontDatabaseTableRefInterface',
     ),
-    'LiskDAOSet' => 'Phobject',
     'LiskDAOTestCase' => 'PhabricatorTestCase',
     'LiskEphemeralObjectException' => 'Exception',
     'LiskFixtureTestCase' => 'PhabricatorTestCase',
diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php
index aa12f8e614..81005ab30d 100644
--- a/src/infrastructure/storage/lisk/LiskDAO.php
+++ b/src/infrastructure/storage/lisk/LiskDAO.php
@@ -193,8 +193,6 @@ abstract class LiskDAO extends Phobject
 
   private static $connections       = array();
 
-  private $inSet = null;
-
   protected $id;
   protected $phid;
   protected $dateCreated;
@@ -659,179 +657,11 @@ abstract class LiskDAO extends Phobject
       } else {
         $result[] = $obj->loadFromArray($row);
       }
-      if ($this->inSet) {
-        $this->inSet->addToSet($obj);
-      }
     }
 
     return $result;
   }
 
-  /**
-   * This method helps to prevent the 1+N queries problem. It happens when you
-   * execute a query for each row in a result set. Like in this code:
-   *
-   *   COUNTEREXAMPLE, name=Easy to write but expensive to execute
-   *   $diffs = id(new DifferentialDiff())->loadAllWhere(
-   *     'revisionID = %d',
-   *     $revision->getID());
-   *   foreach ($diffs as $diff) {
-   *     $changesets = id(new DifferentialChangeset())->loadAllWhere(
-   *       'diffID = %d',
-   *       $diff->getID());
-   *     // Do something with $changesets.
-   *   }
-   *
-   * One can solve this problem by reading all the dependent objects at once and
-   * assigning them later:
-   *
-   *   COUNTEREXAMPLE, name=Cheaper to execute but harder to write and maintain
-   *   $diffs = id(new DifferentialDiff())->loadAllWhere(
-   *     'revisionID = %d',
-   *     $revision->getID());
-   *   $all_changesets = id(new DifferentialChangeset())->loadAllWhere(
-   *     'diffID IN (%Ld)',
-   *     mpull($diffs, 'getID'));
-   *   $all_changesets = mgroup($all_changesets, 'getDiffID');
-   *   foreach ($diffs as $diff) {
-   *     $changesets = idx($all_changesets, $diff->getID(), array());
-   *     // Do something with $changesets.
-   *   }
-   *
-   * The method @{method:loadRelatives} abstracts this approach which allows
-   * writing a code which is simple and efficient at the same time:
-   *
-   *   name=Easy to write and cheap to execute
-   *   $diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
-   *   foreach ($diffs as $diff) {
-   *     $changesets = $diff->loadRelatives(
-   *       new DifferentialChangeset(),
-   *       'diffID');
-   *     // Do something with $changesets.
-   *   }
-   *
-   * This will load dependent objects for all diffs in the first call of
-   * @{method:loadRelatives} and use this result for all following calls.
-   *
-   * The method supports working with set of sets, like in this code:
-   *
-   *   $diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
-   *   foreach ($diffs as $diff) {
-   *     $changesets = $diff->loadRelatives(
-   *       new DifferentialChangeset(),
-   *       'diffID');
-   *     foreach ($changesets as $changeset) {
-   *       $hunks = $changeset->loadRelatives(
-   *         new DifferentialHunk(),
-   *         'changesetID');
-   *       // Do something with hunks.
-   *     }
-   *   }
-   *
-   * This code will execute just three queries - one to load all diffs, one to
-   * load all their related changesets and one to load all their related hunks.
-   * You can try to write an equivalent code without using this method as
-   * a homework.
-   *
-   * The method also supports retrieving referenced objects, for example authors
-   * of all diffs (using shortcut @{method:loadOneRelative}):
-   *
-   *   foreach ($diffs as $diff) {
-   *     $author = $diff->loadOneRelative(
-   *       new PhabricatorUser(),
-   *       'phid',
-   *       'getAuthorPHID');
-   *     // Do something with author.
-   *   }
-   *
-   * It is also possible to specify additional conditions for the `WHERE`
-   * clause. Similarly to @{method:loadAllWhere}, you can specify everything
-   * after `WHERE` (except `LIMIT`). Contrary to @{method:loadAllWhere}, it is
-   * allowed to pass only a constant string (`%` doesn't have a special
-   * meaning). This is intentional to avoid mistakes with using data from one
-   * row in retrieving other rows. Example of a correct usage:
-   *
-   *   $status = $author->loadOneRelative(
-   *     new PhabricatorCalendarEvent(),
-   *     'userPHID',
-   *     'getPHID',
-   *     '(UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo)');
-   *
-   * @param  LiskDAO  Type of objects to load.
-   * @param  string   Name of the column in target table.
-   * @param  string   Method name in this table.
-   * @param  string   Additional constraints on returned rows. It supports no
-   *                  placeholders and requires putting the WHERE part into
-   *                  parentheses. It's not possible to use LIMIT.
-   * @return list     Objects of type $object.
-   *
-   * @task   load
-   */
-  public function loadRelatives(
-    LiskDAO $object,
-    $foreign_column,
-    $key_method = 'getID',
-    $where = '') {
-
-    if (!$this->inSet) {
-      id(new LiskDAOSet())->addToSet($this);
-    }
-    $relatives = $this->inSet->loadRelatives(
-      $object,
-      $foreign_column,
-      $key_method,
-      $where);
-    return idx($relatives, $this->$key_method(), array());
-  }
-
-  /**
-   * Load referenced row. See @{method:loadRelatives} for details.
-   *
-   * @param  LiskDAO  Type of objects to load.
-   * @param  string   Name of the column in target table.
-   * @param  string   Method name in this table.
-   * @param  string   Additional constraints on returned rows. It supports no
-   *                  placeholders and requires putting the WHERE part into
-   *                  parentheses. It's not possible to use LIMIT.
-   * @return LiskDAO  Object of type $object or null if there's no such object.
-   *
-   * @task   load
-   */
-  final public function loadOneRelative(
-    LiskDAO $object,
-    $foreign_column,
-    $key_method = 'getID',
-    $where = '') {
-
-    $relatives = $this->loadRelatives(
-      $object,
-      $foreign_column,
-      $key_method,
-      $where);
-
-    if (!$relatives) {
-      return null;
-    }
-
-    if (count($relatives) > 1) {
-      throw new AphrontCountQueryException(
-        pht(
-          'More than one result from %s!',
-          __FUNCTION__.'()'));
-    }
-
-    return reset($relatives);
-  }
-
-  final public function putInSet(LiskDAOSet $set) {
-    $this->inSet = $set;
-    return $this;
-  }
-
-  final protected function getInSet() {
-    return $this->inSet;
-  }
-
 
 /* -(  Examining Objects  )-------------------------------------------------- */
 
diff --git a/src/infrastructure/storage/lisk/LiskDAOSet.php b/src/infrastructure/storage/lisk/LiskDAOSet.php
deleted file mode 100644
index 90eba708ea..0000000000
--- a/src/infrastructure/storage/lisk/LiskDAOSet.php
+++ /dev/null
@@ -1,101 +0,0 @@
-<?php
-
-/**
- * You usually don't need to use this class directly as it is controlled by
- * @{class:LiskDAO}. You can create it if you want to work with objects of same
- * type from different sources as with one set. Let's say you want to get
- * e-mails of all users involved in a revision:
- *
- *   $users = new LiskDAOSet();
- *   $users->addToSet($author);
- *   foreach ($reviewers as $reviewer) {
- *     $users->addToSet($reviewer);
- *   }
- *   foreach ($ccs as $cc) {
- *     $users->addToSet($cc);
- *   }
- *   // Preload e-mails of all involved users and return e-mails of author.
- *   $author_emails = $author->loadRelatives(
- *     new PhabricatorUserEmail(),
- *     'userPHID',
- *     'getPHID');
- */
-final class LiskDAOSet extends Phobject {
-  private $daos = array();
-  private $relatives = array();
-  private $subsets = array();
-
-  public function addToSet(LiskDAO $dao) {
-    if ($this->relatives) {
-      throw new Exception(
-        pht(
-          "Don't call %s after loading data!",
-          __FUNCTION__.'()'));
-    }
-    $this->daos[] = $dao;
-    $dao->putInSet($this);
-    return $this;
-  }
-
-  /**
-   * The main purpose of this method is to break cyclic dependency.
-   * It removes all objects from this set and all subsets created by it.
-   */
-  public function clearSet() {
-    $this->daos = array();
-    $this->relatives = array();
-    foreach ($this->subsets as $set) {
-      $set->clearSet();
-    }
-    $this->subsets = array();
-    return $this;
-  }
-
-
-  /**
-   * See @{method:LiskDAO::loadRelatives}.
-   */
-  public function loadRelatives(
-    LiskDAO $object,
-    $foreign_column,
-    $key_method = 'getID',
-    $where = '') {
-
-    $relatives = &$this->relatives[
-      get_class($object)."-{$foreign_column}-{$key_method}-{$where}"];
-
-    if ($relatives === null) {
-      $ids = array();
-      foreach ($this->daos as $dao) {
-        $id = $dao->$key_method();
-        if ($id !== null) {
-          $ids[$id] = $id;
-        }
-      }
-      if (!$ids) {
-        $relatives = array();
-      } else {
-        $set = new LiskDAOSet();
-        $this->subsets[] = $set;
-
-        $conn = $object->establishConnection('r');
-
-        if (strlen($where)) {
-          $where_clause = qsprintf($conn, 'AND %Q', $where);
-        } else {
-          $where_clause = qsprintf($conn, '');
-        }
-
-        $relatives = $object->putInSet($set)->loadAllWhere(
-          '%C IN (%Ls) %Q',
-          $foreign_column,
-          $ids,
-          $where_clause);
-        $relatives = mgroup($relatives, 'get'.$foreign_column);
-      }
-    }
-
-    return $relatives;
-  }
-
-}

From ecae936d97011293737f39a44feaefaf4583fa77 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 16:55:32 -0800
Subject: [PATCH 26/31] Fix another qsprintf() straggler in "Has Open Subtasks"

Summary: See <https://discourse.phabricator-community.org/t/error-message-is-not-being-logged-when-unable-to-connect-to-the-database/2201/>.

Test Plan: Queried for "With Open Subtasks" and "With No Open Subtasks".

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Subscribers: joshuaspence

Differential Revision: https://secure.phabricator.com/D19880
---
 src/applications/maniphest/query/ManiphestTaskQuery.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php
index d1932a7597..fc5097f4d3 100644
--- a/src/applications/maniphest/query/ManiphestTaskQuery.php
+++ b/src/applications/maniphest/query/ManiphestTaskQuery.php
@@ -641,9 +641,9 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
     if ($this->hasOpenSubtasks !== null) {
       if ($this->hasOpenSubtasks) {
-        $join_type = 'JOIN';
+        $join_type = qsprintf($conn, 'JOIN');
       } else {
-        $join_type = 'LEFT JOIN';
+        $join_type = qsprintf($conn, 'LEFT JOIN');
       }
 
       $joins[] = qsprintf(

From c58506aeaace694534a13b40c558a670aa8230c5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 13 Dec 2018 10:13:56 -0800
Subject: [PATCH 27/31] Give sessions real PHIDs and slightly modernize session
 queries

Summary:
Ref T13222. See PHI873. I'm preparing to introduce a new MFA "Challenge" table which stores state about challenges we've issued (to bind challenges to sessions and prevent most challenge reuse).

This table will reference sessions (since each challenge will be bound to a particular session) but sessions currently don't have PHIDs. Give them PHIDs and slightly modernize some related code.

Test Plan:
  - Ran migrations.
  - Verified table got PHIDs.
  - Used `var_dump()` to dump an organic user session.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19881
---
 .../20181213.auth.01.sessionphid.sql          |  2 +
 .../20181213.auth.02.populatephid.php         | 18 +++++++++
 .../autopatches/20181213.auth.03.phidkey.sql  |  2 +
 src/__phutil_library_map__.php                |  2 +
 .../engine/PhabricatorAuthSessionEngine.php   |  1 +
 .../phid/PhabricatorAuthSessionPHIDType.php   | 34 +++++++++++++++++
 .../query/PhabricatorAuthSessionQuery.php     | 38 ++++++++++---------
 .../auth/storage/PhabricatorAuthSession.php   |  5 +++
 8 files changed, 85 insertions(+), 17 deletions(-)
 create mode 100644 resources/sql/autopatches/20181213.auth.01.sessionphid.sql
 create mode 100644 resources/sql/autopatches/20181213.auth.02.populatephid.php
 create mode 100644 resources/sql/autopatches/20181213.auth.03.phidkey.sql
 create mode 100644 src/applications/auth/phid/PhabricatorAuthSessionPHIDType.php

diff --git a/resources/sql/autopatches/20181213.auth.01.sessionphid.sql b/resources/sql/autopatches/20181213.auth.01.sessionphid.sql
new file mode 100644
index 0000000000..34b5aa5bf6
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.01.sessionphid.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.phabricator_session
+  ADD phid VARBINARY(64) NOT NULL;
diff --git a/resources/sql/autopatches/20181213.auth.02.populatephid.php b/resources/sql/autopatches/20181213.auth.02.populatephid.php
new file mode 100644
index 0000000000..314eaf87a3
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.02.populatephid.php
@@ -0,0 +1,18 @@
+<?php
+
+$table = new PhabricatorAuthSession();
+$iterator = new LiskMigrationIterator($table);
+$conn = $table->establishConnection('w');
+
+foreach ($iterator as $session) {
+  if (strlen($session->getPHID())) {
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'UPDATE %R SET phid = %s WHERE id = %d',
+    $table,
+    $session->generatePHID(),
+    $session->getID());
+}
diff --git a/resources/sql/autopatches/20181213.auth.03.phidkey.sql b/resources/sql/autopatches/20181213.auth.03.phidkey.sql
new file mode 100644
index 0000000000..6bc11b3e55
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.03.phidkey.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.phabricator_session
+  ADD UNIQUE KEY `key_phid` (phid);
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 09430367d9..420457f525 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2296,6 +2296,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionEngineExtensionModule' => 'applications/auth/engine/PhabricatorAuthSessionEngineExtensionModule.php',
     'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php',
     'PhabricatorAuthSessionInfo' => 'applications/auth/data/PhabricatorAuthSessionInfo.php',
+    'PhabricatorAuthSessionPHIDType' => 'applications/auth/phid/PhabricatorAuthSessionPHIDType.php',
     'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
     'PhabricatorAuthSessionRevoker' => 'applications/auth/revoker/PhabricatorAuthSessionRevoker.php',
     'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php',
@@ -7948,6 +7949,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionEngineExtensionModule' => 'PhabricatorConfigModule',
     'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector',
     'PhabricatorAuthSessionInfo' => 'Phobject',
+    'PhabricatorAuthSessionPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthSessionRevoker' => 'PhabricatorAuthRevoker',
     'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController',
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index 4ce86e8f97..ada4162067 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -119,6 +119,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       $conn_r,
       'SELECT
           s.id AS s_id,
+          s.phid AS s_phid,
           s.sessionExpires AS s_sessionExpires,
           s.sessionStart AS s_sessionStart,
           s.highSecurityUntil AS s_highSecurityUntil,
diff --git a/src/applications/auth/phid/PhabricatorAuthSessionPHIDType.php b/src/applications/auth/phid/PhabricatorAuthSessionPHIDType.php
new file mode 100644
index 0000000000..e031c4ae88
--- /dev/null
+++ b/src/applications/auth/phid/PhabricatorAuthSessionPHIDType.php
@@ -0,0 +1,34 @@
+<?php
+
+final class PhabricatorAuthSessionPHIDType
+  extends PhabricatorPHIDType {
+
+  const TYPECONST = 'SSSN';
+
+  public function getTypeName() {
+    return pht('Session');
+  }
+
+  public function newObject() {
+    return new PhabricatorAuthSession();
+  }
+
+  public function getPHIDTypeApplicationClass() {
+    return 'PhabricatorAuthApplication';
+  }
+
+  protected function buildQueryForObjects(
+    PhabricatorObjectQuery $query,
+    array $phids) {
+    return id(new PhabricatorAuthSessionQuery())
+      ->withPHIDs($phids);
+  }
+
+  public function loadHandles(
+    PhabricatorHandleQuery $query,
+    array $handles,
+    array $objects) {
+    return;
+  }
+
+}
diff --git a/src/applications/auth/query/PhabricatorAuthSessionQuery.php b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
index 25928e72c1..a2101201e2 100644
--- a/src/applications/auth/query/PhabricatorAuthSessionQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
@@ -4,6 +4,7 @@ final class PhabricatorAuthSessionQuery
   extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $ids;
+  private $phids;
   private $identityPHIDs;
   private $sessionKeys;
   private $sessionTypes;
@@ -28,19 +29,17 @@ final class PhabricatorAuthSessionQuery
     return $this;
   }
 
+  public function withPHIDs(array $phids) {
+    $this->phids = $phids;
+    return $this;
+  }
+
+  public function newResultObject() {
+    return new PhabricatorAuthSession();
+  }
+
   protected function loadPage() {
-    $table = new PhabricatorAuthSession();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+    return $this->loadStandardPage($this->newResultObject());
   }
 
   protected function willFilterPage(array $sessions) {
@@ -65,8 +64,8 @@ final class PhabricatorAuthSessionQuery
     return $sessions;
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn) {
-    $where = array();
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
 
     if ($this->ids !== null) {
       $where[] = qsprintf(
@@ -75,6 +74,13 @@ final class PhabricatorAuthSessionQuery
         $this->ids);
     }
 
+    if ($this->phids !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'phid IN (%Ls)',
+        $this->phids);
+    }
+
     if ($this->identityPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
@@ -100,9 +106,7 @@ final class PhabricatorAuthSessionQuery
         $this->sessionTypes);
     }
 
-    $where[] = $this->buildPagingClause($conn);
-
-    return $this->formatWhereClause($conn, $where);
+    return $where;
   }
 
   public function getQueryApplicationClass() {
diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php
index cf707a053d..c4731b669b 100644
--- a/src/applications/auth/storage/PhabricatorAuthSession.php
+++ b/src/applications/auth/storage/PhabricatorAuthSession.php
@@ -20,6 +20,7 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
   protected function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_AUX_PHID => true,
       self::CONFIG_COLUMN_SCHEMA => array(
         'type' => 'text32',
         'sessionKey' => 'bytes40',
@@ -74,6 +75,10 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
     }
   }
 
+  public function getPHIDType() {
+    return PhabricatorAuthSessionPHIDType::TYPECONST;
+  }
+
   public function isHighSecuritySession() {
     $until = $this->getHighSecurityUntil();
 

From 1d34238dc94555466e15039ff6991b371ae294ef Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 13 Dec 2018 10:52:54 -0800
Subject: [PATCH 28/31] Upgrade sessions digests to HMAC256, retaining
 compatibility with old digests

Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.

Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.

We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:

  - Always write new digests.
  - We still match sessions with either digest.
  - When we read a session with an old digest, upgrade it to a new digest.

In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.

We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.

Test Plan:
  - Hit a page with an old session, got a session upgrade.
  - Reviewed sessions in Settings.
  - Reviewed user logs.
  - Logged out.
  - Logged in.
  - Terminated other sessions individually.
  - Terminated all other sessions.
  - Spot checked session table for general sanity.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13225, T13222

Differential Revision: https://secure.phabricator.com/D19883
---
 .../20181213.auth.04.longerhashes.sql         |  2 +
 .../20181213.auth.05.longerloghashes.sql      |  2 +
 .../sql/patches/20130530.sessionhash.php      | 25 ++--------
 ...bricatorAuthTerminateSessionController.php |  5 +-
 .../PhabricatorAuthUnlinkController.php       |  3 +-
 .../engine/PhabricatorAuthSessionEngine.php   | 49 ++++++++++++++-----
 .../query/PhabricatorAuthSessionQuery.php     |  3 +-
 .../auth/storage/PhabricatorAuthSession.php   | 10 +++-
 .../people/storage/PhabricatorUserLog.php     |  2 +-
 .../PhabricatorMultiFactorSettingsPanel.php   |  3 +-
 .../PhabricatorPasswordSettingsPanel.php      |  3 +-
 .../PhabricatorSessionsSettingsPanel.php      |  5 +-
 src/infrastructure/util/PhabricatorHash.php   | 14 +++++-
 13 files changed, 84 insertions(+), 42 deletions(-)
 create mode 100644 resources/sql/autopatches/20181213.auth.04.longerhashes.sql
 create mode 100644 resources/sql/autopatches/20181213.auth.05.longerloghashes.sql

diff --git a/resources/sql/autopatches/20181213.auth.04.longerhashes.sql b/resources/sql/autopatches/20181213.auth.04.longerhashes.sql
new file mode 100644
index 0000000000..2bffb4c4a8
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.04.longerhashes.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.phabricator_session
+  CHANGE sessionKey sessionKey VARBINARY(64) NOT NULL;
diff --git a/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql b/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql
new file mode 100644
index 0000000000..dc8638d91c
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.user_log
+  CHANGE session session VARBINARY(64);
diff --git a/resources/sql/patches/20130530.sessionhash.php b/resources/sql/patches/20130530.sessionhash.php
index 771dac61e3..1e09ee32fd 100644
--- a/resources/sql/patches/20130530.sessionhash.php
+++ b/resources/sql/patches/20130530.sessionhash.php
@@ -1,22 +1,7 @@
 <?php
 
-$table = new PhabricatorUser();
-$table->openTransaction();
-$conn = $table->establishConnection('w');
-
-$sessions = queryfx_all(
-  $conn,
-  'SELECT userPHID, type, sessionKey FROM %T FOR UPDATE',
-  PhabricatorUser::SESSION_TABLE);
-
-foreach ($sessions as $session) {
-  queryfx(
-    $conn,
-    'UPDATE %T SET sessionKey = %s WHERE userPHID = %s AND type = %s',
-    PhabricatorUser::SESSION_TABLE,
-    PhabricatorHash::weakDigest($session['sessionKey']),
-    $session['userPHID'],
-    $session['type']);
-}
-
-$table->saveTransaction();
+// See T13225. Long ago, this upgraded session key storage from unhashed to
+// HMAC-SHA1 here. We later upgraded storage to HMAC-SHA256, so this is initial
+// upgrade is now fairly pointless. Dropping this migration entirely only logs
+// users out of installs that waited more than 5 years to upgrade, which seems
+// like a reasonable behavior.
diff --git a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
index fa58977c90..413d0306b9 100644
--- a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
+++ b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
@@ -16,8 +16,9 @@ final class PhabricatorAuthTerminateSessionController
       $query->withIDs(array($id));
     }
 
-    $current_key = PhabricatorHash::weakDigest(
-      $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+    $current_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope(
+        $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
     $sessions = $query->execute();
     foreach ($sessions as $key => $session) {
diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
index 6211e78110..e6e1493e5a 100644
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -56,7 +56,8 @@ final class PhabricatorAuthUnlinkController
 
       id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
         $viewer,
-        $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+        new PhutilOpaqueEnvelope(
+          $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
       return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
     }
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index ada4162067..2020e4a542 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -109,14 +109,19 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
     $session_table = new PhabricatorAuthSession();
     $user_table = new PhabricatorUser();
-    $conn_r = $session_table->establishConnection('r');
-    $session_key = PhabricatorHash::weakDigest($session_token);
+    $conn = $session_table->establishConnection('r');
 
-    $cache_parts = $this->getUserCacheQueryParts($conn_r);
+    // TODO: See T13225. We're moving sessions to a more modern digest
+    // algorithm, but still accept older cookies for compatibility.
+    $session_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope($session_token));
+    $weak_key = PhabricatorHash::weakDigest($session_token);
+
+    $cache_parts = $this->getUserCacheQueryParts($conn);
     list($cache_selects, $cache_joins, $cache_map, $types_map) = $cache_parts;
 
     $info = queryfx_one(
-      $conn_r,
+      $conn,
       'SELECT
           s.id AS s_id,
           s.phid AS s_phid,
@@ -125,21 +130,28 @@ final class PhabricatorAuthSessionEngine extends Phobject {
           s.highSecurityUntil AS s_highSecurityUntil,
           s.isPartial AS s_isPartial,
           s.signedLegalpadDocuments as s_signedLegalpadDocuments,
+          IF(s.sessionKey = %P, 1, 0) as s_weak,
           u.*
           %Q
-        FROM %T u JOIN %T s ON u.phid = s.userPHID
-        AND s.type = %s AND s.sessionKey = %P %Q',
+        FROM %R u JOIN %R s ON u.phid = s.userPHID
+        AND s.type = %s AND s.sessionKey IN (%P, %P) %Q',
+      new PhutilOpaqueEnvelope($weak_key),
       $cache_selects,
-      $user_table->getTableName(),
-      $session_table->getTableName(),
+      $user_table,
+      $session_table,
       $session_type,
       new PhutilOpaqueEnvelope($session_key),
+      new PhutilOpaqueEnvelope($weak_key),
       $cache_joins);
 
     if (!$info) {
       return null;
     }
 
+    // TODO: Remove this, see T13225.
+    $is_weak = (bool)$info['s_weak'];
+    unset($info['s_weak']);
+
     $session_dict = array(
       'userPHID' => $info['phid'],
       'sessionKey' => $session_key,
@@ -202,6 +214,19 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       unset($unguarded);
     }
 
+    // TODO: Remove this, see T13225.
+    if ($is_weak) {
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $conn_w = $session_table->establishConnection('w');
+        queryfx(
+          $conn_w,
+          'UPDATE %T SET sessionKey = %P WHERE id = %d',
+          $session->getTableName(),
+          new PhutilOpaqueEnvelope($session_key),
+          $session->getID());
+      unset($unguarded);
+    }
+
     $user->attachSession($session);
     return $user;
   }
@@ -241,7 +266,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     // This has a side effect of validating the session type.
     $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
 
-    $digest_key = PhabricatorHash::weakDigest($session_key);
+    $digest_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope($session_key));
 
     // Logging-in users don't have CSRF stuff yet, so we have to unguard this
     // write.
@@ -299,7 +325,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
    */
   public function terminateLoginSessions(
     PhabricatorUser $user,
-    $except_session = null) {
+    PhutilOpaqueEnvelope $except_session = null) {
 
     $sessions = id(new PhabricatorAuthSessionQuery())
       ->setViewer($user)
@@ -307,7 +333,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       ->execute();
 
     if ($except_session !== null) {
-      $except_session = PhabricatorHash::weakDigest($except_session);
+      $except_session = PhabricatorAuthSession::newSessionDigest(
+        $except_session);
     }
 
     foreach ($sessions as $key => $session) {
diff --git a/src/applications/auth/query/PhabricatorAuthSessionQuery.php b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
index a2101201e2..00a663e964 100644
--- a/src/applications/auth/query/PhabricatorAuthSessionQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
@@ -91,7 +91,8 @@ final class PhabricatorAuthSessionQuery
     if ($this->sessionKeys !== null) {
       $hashes = array();
       foreach ($this->sessionKeys as $session_key) {
-        $hashes[] = PhabricatorHash::weakDigest($session_key);
+        $hashes[] = PhabricatorAuthSession::newSessionDigest(
+          new PhutilOpaqueEnvelope($session_key));
       }
       $where[] = qsprintf(
         $conn,
diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php
index c4731b669b..6d54dda781 100644
--- a/src/applications/auth/storage/PhabricatorAuthSession.php
+++ b/src/applications/auth/storage/PhabricatorAuthSession.php
@@ -6,6 +6,8 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
   const TYPE_WEB      = 'web';
   const TYPE_CONDUIT  = 'conduit';
 
+  const SESSION_DIGEST_KEY = 'session.digest';
+
   protected $userPHID;
   protected $type;
   protected $sessionKey;
@@ -17,13 +19,19 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
 
   private $identityObject = self::ATTACHABLE;
 
+  public static function newSessionDigest(PhutilOpaqueEnvelope $session_token) {
+    return PhabricatorHash::digestWithNamedKey(
+      $session_token->openEnvelope(),
+      self::SESSION_DIGEST_KEY);
+  }
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_COLUMN_SCHEMA => array(
         'type' => 'text32',
-        'sessionKey' => 'bytes40',
+        'sessionKey' => 'text64',
         'sessionStart' => 'epoch',
         'sessionExpires' => 'epoch',
         'highSecurityUntil' => 'epoch?',
diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php
index 82819f8f5b..12cb4cb626 100644
--- a/src/applications/people/storage/PhabricatorUserLog.php
+++ b/src/applications/people/storage/PhabricatorUserLog.php
@@ -150,7 +150,7 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
         'actorPHID' => 'phid?',
         'action' => 'text64',
         'remoteAddr' => 'text64',
-        'session' => 'bytes40?',
+        'session' => 'text64?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'actorPHID' => array(
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
index ae653e0f70..5fada0bbed 100644
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -193,7 +193,8 @@ final class PhabricatorMultiFactorSettingsPanel
 
         id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
           $user,
-          $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+          new PhutilOpaqueEnvelope(
+            $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
         return id(new AphrontRedirectResponse())
           ->setURI($this->getPanelURI('?id='.$config->getID()));
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
index 9fb8838cf7..79d7610f2f 100644
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -121,7 +121,8 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
 
         id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
           $user,
-          $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+          new PhutilOpaqueEnvelope(
+            $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
         return id(new AphrontRedirectResponse())->setURI($next);
       }
diff --git a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
index eab18002a1..314d68f69d 100644
--- a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
@@ -44,8 +44,9 @@ final class PhabricatorSessionsSettingsPanel extends PhabricatorSettingsPanel {
       ->withPHIDs($identity_phids)
       ->execute();
 
-    $current_key = PhabricatorHash::weakDigest(
-      $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+    $current_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope(
+        $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
     $rows = array();
     $rowc = array();
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index ce48b0966b..d717778eb3 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -187,6 +187,16 @@ final class PhabricatorHash extends Phobject {
   }
 
   public static function digestHMACSHA256($message, $key) {
+    if (!is_string($message)) {
+      throw new Exception(
+        pht('HMAC-SHA256 can only digest strings.'));
+    }
+
+    if (!is_string($key)) {
+      throw new Exception(
+        pht('HMAC-SHA256 keys must be strings.'));
+    }
+
     if (!strlen($key)) {
       throw new Exception(
         pht('HMAC-SHA256 requires a nonempty key.'));
@@ -194,7 +204,9 @@ final class PhabricatorHash extends Phobject {
 
     $result = hash_hmac('sha256', $message, $key, $raw_output = false);
 
-    if ($result === false) {
+    // Although "hash_hmac()" is documented as returning `false` when it fails,
+    // it can also return `null` if you pass an object as the "$message".
+    if ($result === false || $result === null) {
       throw new Exception(
         pht('Unable to compute HMAC-SHA256 digest of message.'));
     }

From 080fb1985f29909bb2710fd25f9918253b1a5c04 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 13 Dec 2018 12:14:33 -0800
Subject: [PATCH 29/31] Upgrade an old "weakDigest()" inside TOTP
 synchronization code

Summary:
Ref T13222. Ref T12509. When you add a new MFA TOTP authenticator, we generate a temporary token to make sure you're actually adding the key we generated and not picking your own key.

That is, if we just put inputs in the form like `key=123, response=456`, users could pick their own keys by changing the value of `key` and then generating the correct `response`. That's probably fine, but maybe attackers could somehow force users to pick known keys in combination with other unknown vulnerabilities that might exist in the future. Instead, we generate a random key and keep track of it to make sure nothing funny is afoot.

As an additional barrier, we do the standard "store the digest, not the real key" sort of thing so you can't force a known value even if you can read the database (although this is mostly pointless since you can just read TOTP secrets directly if you can read the database). But it's pretty standard and doesn't hurt anything.

Update this from SHA1 to SHA256. This will break any TOTP factors which someone was in the middle of adding during a Phabricator upgrade, but that seems reasonable. They'll get a sensible failure mode.

Test Plan: Added a new TOTP factor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T12509

Differential Revision: https://secure.phabricator.com/D19884
---
 .../auth/factor/PhabricatorTOTPAuthFactor.php      | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
index 7d7aec921f..ae3608d525 100644
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -2,6 +2,8 @@
 
 final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
 
+  const DIGEST_TEMPORARY_KEY = 'mfa.totp.sync';
+
   public function getFactorKey() {
     return 'totp';
   }
@@ -34,12 +36,16 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       // (We store and verify the hash of the key, not the key itself, to limit
       // how useful the data in the table is to an attacker.)
 
+      $token_code = PhabricatorHash::digestWithNamedKey(
+        $key,
+        self::DIGEST_TEMPORARY_KEY);
+
       $temporary_token = id(new PhabricatorAuthTemporaryTokenQuery())
         ->setViewer($user)
         ->withTokenResources(array($user->getPHID()))
         ->withTokenTypes(array($totp_token_type))
         ->withExpired(false)
-        ->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
+        ->withTokenCodes(array($token_code))
         ->executeOne();
       if (!$temporary_token) {
         // If we don't have a matching token, regenerate the key below.
@@ -53,12 +59,16 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       // Mark this key as one we generated, so the user is allowed to submit
       // a response for it.
 
+      $token_code = PhabricatorHash::digestWithNamedKey(
+        $key,
+        self::DIGEST_TEMPORARY_KEY);
+
       $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
         id(new PhabricatorAuthTemporaryToken())
           ->setTokenResource($user->getPHID())
           ->setTokenType($totp_token_type)
           ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-          ->setTokenCode(PhabricatorHash::weakDigest($key))
+          ->setTokenCode($token_code)
           ->save();
       unset($unguarded);
     }

From d23cc4b862aac68caf6fc68835884b2d2dae735b Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Thu, 13 Dec 2018 16:13:07 -0800
Subject: [PATCH 30/31] Move user renames to modular transactions

Summary: Cleaning up more super-old code from `PhabricatorUserEditor`. Also fix user logging in approve transactions. I'm not sure how it worked at all previously.

Test Plan: Created new users, renamed them, checked DB for sanity. Entered invalid names, duplicate names, and empty names, got appropriate error messages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19887
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorPeopleRenameController.php     | 56 +++++------
 .../people/editor/PhabricatorUserEditor.php   | 47 ----------
 .../PhabricatorUserUsernameTransaction.php    | 92 +++++++++++++++++++
 4 files changed, 116 insertions(+), 81 deletions(-)
 create mode 100644 src/applications/people/xaction/PhabricatorUserUsernameTransaction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 420457f525..b0408e127d 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4646,6 +4646,7 @@ phutil_register_library_map(array(
     'PhabricatorUserTransaction' => 'applications/people/storage/PhabricatorUserTransaction.php',
     'PhabricatorUserTransactionEditor' => 'applications/people/editor/PhabricatorUserTransactionEditor.php',
     'PhabricatorUserTransactionType' => 'applications/people/xaction/PhabricatorUserTransactionType.php',
+    'PhabricatorUserUsernameTransaction' => 'applications/people/xaction/PhabricatorUserUsernameTransaction.php',
     'PhabricatorUsersEditField' => 'applications/transactions/editfield/PhabricatorUsersEditField.php',
     'PhabricatorUsersPolicyRule' => 'applications/people/policyrule/PhabricatorUsersPolicyRule.php',
     'PhabricatorUsersSearchField' => 'applications/people/searchfield/PhabricatorUsersSearchField.php',
@@ -10706,6 +10707,7 @@ phutil_register_library_map(array(
     'PhabricatorUserTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorUserTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorUserTransactionType' => 'PhabricatorModularTransactionType',
+    'PhabricatorUserUsernameTransaction' => 'PhabricatorUserTransactionType',
     'PhabricatorUsersEditField' => 'PhabricatorTokenizerEditField',
     'PhabricatorUsersPolicyRule' => 'PhabricatorPolicyRule',
     'PhabricatorUsersSearchField' => 'PhabricatorSearchTokenizerField',
diff --git a/src/applications/people/controller/PhabricatorPeopleRenameController.php b/src/applications/people/controller/PhabricatorPeopleRenameController.php
index cd552717e0..42ff2e7988 100644
--- a/src/applications/people/controller/PhabricatorPeopleRenameController.php
+++ b/src/applications/people/controller/PhabricatorPeopleRenameController.php
@@ -22,36 +22,29 @@ final class PhabricatorPeopleRenameController
       $request,
       $done_uri);
 
-    $errors = array();
-
-    $v_username = $user->getUsername();
-    $e_username = true;
+    $validation_exception = null;
+    $username = $user->getUsername();
     if ($request->isFormPost()) {
-      $v_username = $request->getStr('username');
+      $username = $request->getStr('username');
+      $xactions = array();
 
-      if (!strlen($v_username)) {
-        $e_username = pht('Required');
-        $errors[] = pht('New username is required.');
-      } else if ($v_username == $user->getUsername()) {
-        $e_username = pht('Invalid');
-        $errors[] = pht('New username must be different from old username.');
-      } else if (!PhabricatorUser::validateUsername($v_username)) {
-        $e_username = pht('Invalid');
-        $errors[] = PhabricatorUser::describeValidUsername();
+      $xactions[] = id(new PhabricatorUserTransaction())
+        ->setTransactionType(
+          PhabricatorUserUsernameTransaction::TRANSACTIONTYPE)
+        ->setNewValue($username);
+
+      $editor = id(new PhabricatorUserTransactionEditor())
+        ->setActor($viewer)
+        ->setContentSourceFromRequest($request)
+        ->setContinueOnMissingFields(true);
+
+      try {
+        $editor->applyTransactions($user, $xactions);
+        return id(new AphrontRedirectResponse())->setURI($done_uri);
+      } catch (PhabricatorApplicationTransactionValidationException $ex) {
+        $validation_exception = $ex;
       }
 
-      if (!$errors) {
-        try {
-          id(new PhabricatorUserEditor())
-            ->setActor($viewer)
-            ->changeUsername($user, $v_username);
-
-          return id(new AphrontRedirectResponse())->setURI($done_uri);
-        } catch (AphrontDuplicateKeyQueryException $ex) {
-          $e_username = pht('Not Unique');
-          $errors[] = pht('Another user already has that username.');
-        }
-      }
     }
 
     $inst1 = pht(
@@ -87,18 +80,13 @@ final class PhabricatorPeopleRenameController
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setLabel(pht('New Username'))
-          ->setValue($v_username)
-          ->setName('username')
-          ->setError($e_username));
-
-    if ($errors) {
-      $errors = id(new PHUIInfoView())->setErrors($errors);
-    }
+          ->setValue($username)
+          ->setName('username'));
 
     return $this->newDialog()
       ->setWidth(AphrontDialogView::WIDTH_FORM)
       ->setTitle(pht('Change Username'))
-      ->appendChild($errors)
+      ->setValidationException($validation_exception)
       ->appendParagraph($inst1)
       ->appendParagraph($inst2)
       ->appendParagraph($inst3)
diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php
index a64c1cc2a9..8092824a0c 100644
--- a/src/applications/people/editor/PhabricatorUserEditor.php
+++ b/src/applications/people/editor/PhabricatorUserEditor.php
@@ -129,53 +129,6 @@ final class PhabricatorUserEditor extends PhabricatorEditor {
   }
 
 
-  /**
-   * @task edit
-   */
-  public function changeUsername(PhabricatorUser $user, $username) {
-    $actor = $this->requireActor();
-
-    if (!$user->getID()) {
-      throw new Exception(pht('User has not been created yet!'));
-    }
-
-    if (!PhabricatorUser::validateUsername($username)) {
-      $valid = PhabricatorUser::describeValidUsername();
-      throw new Exception(pht('Username is invalid! %s', $valid));
-    }
-
-    $old_username = $user->getUsername();
-
-    $user->openTransaction();
-      $user->reload();
-      $user->setUsername($username);
-
-      try {
-        $user->save();
-      } catch (AphrontDuplicateKeyQueryException $ex) {
-        $user->setUsername($old_username);
-        $user->killTransaction();
-        throw $ex;
-      }
-
-      $log = PhabricatorUserLog::initializeNewLog(
-        $actor,
-        $user->getPHID(),
-        PhabricatorUserLog::ACTION_CHANGE_USERNAME);
-      $log->setOldValue($old_username);
-      $log->setNewValue($username);
-      $log->save();
-
-    $user->saveTransaction();
-
-    // The SSH key cache currently includes usernames, so dirty it. See T12554
-    // for discussion.
-    PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache();
-
-    $user->sendUsernameChangeEmail($actor, $old_username);
-  }
-
-
 /* -(  Editing Roles  )------------------------------------------------------ */
 
 
diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
new file mode 100644
index 0000000000..db134a5c78
--- /dev/null
+++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
@@ -0,0 +1,92 @@
+<?php
+
+final class PhabricatorUserUsernameTransaction
+  extends PhabricatorUserTransactionType {
+
+  const TRANSACTIONTYPE = 'user.rename';
+
+  public function generateOldValue($object) {
+    return $object->getUsername();
+  }
+
+  public function generateNewValue($object, $value) {
+    return $value;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setUsername($value);
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $user = $object;
+
+    $this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME)
+      ->setOldValue($this->getOldValue())
+      ->setNewValue($value)
+      ->save();
+
+    // The SSH key cache currently includes usernames, so dirty it. See T12554
+    // for discussion.
+    PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache();
+
+    $user->sendUsernameChangeEmail($this->getActor(), $this->getOldValue());
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s renamed this user from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $actor = $this->getActor();
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+      $old = $xaction->getOldValue();
+
+      if ($old === $new) {
+        continue;
+      }
+
+      if (!$actor->getIsAdmin()) {
+        $errors[] = $this->newInvalidError(
+          pht('You must be an administrator to rename users.'));
+      }
+
+      if (!strlen($new)) {
+        $errors[] = $this->newRequiredError(
+          pht('New username is required.'),  $xaction);
+      } else if (!PhabricatorUser::validateUsername($new)) {
+        $errors[] = $this->newInvalidError(
+          PhabricatorUser::describeValidUsername(), $xaction);
+      }
+
+      $user = id(new PhabricatorPeopleQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withUsernames(array($new))
+        ->executeOne();
+
+      if ($user) {
+        $errors[] = $this->newInvalidError(
+          pht('Another user already has that username.'), $xaction);
+      }
+
+    }
+
+    return $errors;
+  }
+
+  public function getRequiredCapabilities(
+    $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    // Unlike normal user edits, renames require admin permissions, which
+    // is enforced by validateTransactions().
+
+    return null;
+  }
+}

From 54b952df5d1483a61ceabaf44f92a35df4e0e983 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 14 Dec 2018 15:18:11 -0800
Subject: [PATCH 31/31] Fix weird gap/spacing on user "Manage" page

Summary: I added this recently for debugging test notifications, but goofed up the markup, thought it was just some weird layout issue, and never got back to it.

Test Plan: {F6063455}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19892
---
 ...abricatorPeopleProfileManageController.php | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
index 55f9311ada..046726f39e 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
@@ -36,17 +36,18 @@ final class PhabricatorPeopleProfileManageController
     $crumbs->addTextCrumb(pht('Manage'));
     $crumbs->setBorder(true);
 
+    $timeline = $this->buildTransactionTimeline(
+      $user,
+      new PhabricatorPeopleTransactionQuery());
+    $timeline->setShouldTerminate(true);
+
     $manage = id(new PHUITwoColumnView())
       ->setHeader($header)
       ->addClass('project-view-home')
       ->addClass('project-view-people-home')
       ->setCurtain($curtain)
-      ->addPropertySection(pht('Details'), $properties);
-
-    $timeline = $this->buildTransactionTimeline(
-      $user,
-      new PhabricatorPeopleTransactionQuery());
-    $timeline->setShouldTerminate(true);
+      ->addPropertySection(pht('Details'), $properties)
+      ->setMainColumn($timeline);
 
     return $this->newPage()
       ->setTitle(
@@ -56,11 +57,7 @@ final class PhabricatorPeopleProfileManageController
         ))
       ->setNavigation($nav)
       ->setCrumbs($crumbs)
-      ->appendChild(
-        array(
-          $manage,
-          $timeline,
-        ));
+      ->appendChild($manage);
   }
 
   private function buildPropertyView(PhabricatorUser $user) {