Various minor Audit UI improvements

Summary:
  - Order audits by id desc so they tend to be in descending time order, like other content.
  - In audit tables without commit context (audit tool, home page) show commit descriptions.
  - Correctly hide pagers on "active" audit filter.
  - Make pagers work correctly on commit / audit views.

Test Plan: Looked at homepage, audit, owners, differential. Paginated relevant interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1875

src/applications/audit/controller/list/PhabricatorAuditListController.php

@@ -288,12 +288,15 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
   private function buildAuditView(PhabricatorObjectHandle $handle = null) {
     $request = $this->getRequest();
 
-    $pager = new AphrontPagerView();
-    $pager->setURI($request->getRequestURI(), 'offset');
-
     $query = new PhabricatorAuditQuery();
 
-    if ($this->filter != 'active') {
+    $use_pager = ($this->filter != 'active');
+
+    if ($use_pager) {
+      $pager = new AphrontPagerView();
+      $pager->setURI($request->getRequestURI(), 'offset');
+      $pager->setOffset($request->getInt('offset'));
+
       $query->setOffset($pager->getOffset());
       $query->setLimit($pager->getPageSize() + 1);
     }
@@ -369,11 +372,16 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
         break;
     }
 
+    $query->needCommitData(true);
+
     $audits = $query->execute();
-    $audits = $pager->sliceResults($audits);
+    if ($use_pager) {
+      $audits = $pager->sliceResults($audits);
+    }
 
     $view = new PhabricatorAuditListView();
     $view->setAudits($audits);
+    $view->setCommits($query->getCommits());
     $view->setNoDataString($nodata);
 
     $phids = $view->getRequiredHandlePHIDs();
@@ -383,7 +391,10 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
     $panel = new AphrontPanelView();
     $panel->setHeader($header);
     $panel->appendChild($view);
-    $panel->appendChild($pager);
+
+    if ($use_pager) {
+      $panel->appendChild($pager);
+    }
 
     return $panel;
   }
@@ -391,13 +402,16 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
   private function buildCommitView(PhabricatorObjectHandle $handle = null) {
     $request = $this->getRequest();
 
-    $pager = new AphrontPagerView();
-    $pager->setURI($request->getRequestURI(), 'offset');
-
     $query = new PhabricatorAuditCommitQuery();
     $query->needCommitData(true);
 
-    if ($this->filter != 'active') {
+    $use_pager = ($this->filter != 'active');
+
+    if ($use_pager) {
+      $pager = new AphrontPagerView();
+      $pager->setURI($request->getRequestURI(), 'offset');
+      $pager->setOffset($request->getInt('offset'));
+
       $query->setOffset($pager->getOffset());
       $query->setLimit($pager->getPageSize() + 1);
     }
@@ -452,7 +466,10 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
     }
 
     $commits = $query->execute();
-    $commits = $pager->sliceResults($commits);
+
+    if ($use_pager) {
+      $commits = $pager->sliceResults($commits);
+    }
 
     $view = new PhabricatorAuditCommitListView();
     $view->setUser($request->getUser());
@@ -466,7 +483,10 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
     $panel = new AphrontPanelView();
     $panel->setHeader($header);
     $panel->appendChild($view);
-    $panel->appendChild($pager);
+
+    if ($use_pager) {
+      $panel->appendChild($pager);
+    }
 
     return $panel;
   }

src/applications/audit/query/audit/PhabricatorAuditQuery.php

@@ -24,10 +24,15 @@ final class PhabricatorAuditQuery {
   private $auditorPHIDs;
   private $commitPHIDs;
 
+  private $needCommits;
+  private $needCommitData;
+
   private $status     = 'status-any';
   const STATUS_ANY    = 'status-any';
   const STATUS_OPEN   = 'status-open';
 
+  private $commits;
+
   public function withCommitPHIDs(array $commit_phids) {
     $this->commitPHIDs = $commit_phids;
     return $this;
@@ -53,23 +58,58 @@ final class PhabricatorAuditQuery {
     return $this;
   }
 
+  public function needCommits($need) {
+    $this->needCommits = $need;
+    return $this;
+  }
+
+  public function needCommitData($need) {
+    $this->needCommitData = $need;
+    return $this;
+  }
+
   public function execute() {
     $table = new PhabricatorRepositoryAuditRequest();
     $conn_r = $table->establishConnection('r');
 
     $where = $this->buildWhereClause($conn_r);
+    $order = $this->buildOrderClause($conn_r);
     $limit = $this->buildLimitClause($conn_r);
 
     $data = queryfx_all(
       $conn_r,
-      'SELECT * FROM %T %Q %Q',
+      'SELECT * FROM %T %Q %Q %Q',
       $table->getTableName(),
       $where,
+      $order,
       $limit);
 
     $audits = $table->loadAllFromArray($data);
-    return $audits;
 
+    if ($this->needCommits || $this->needCommitData) {
+      $phids = mpull($audits, 'getCommitPHID', 'getCommitPHID');
+      if ($phids) {
+        $cquery = new PhabricatorAuditCommitQuery();
+        $cquery->needCommitData($this->needCommitData);
+        $cquery->withCommitPHIDs(array_keys($phids));
+        $commits = $cquery->execute();
+      } else {
+        $commits = array();
+      }
+      $this->commits = $commits;
+    }
+
+    return $audits;
+  }
+
+  public function getCommits() {
+    if ($this->commits === null) {
+      throw new Exception(
+        "Call needCommits() or needCommitData() and then execute() the query ".
+        "before calling getCommits()!");
+    }
+
+    return $this->commits;
   }
 
   private function buildWhereClause($conn_r) {
@@ -128,4 +168,8 @@ final class PhabricatorAuditQuery {
     }
   }
 
+  private function buildOrderClause($conn_r) {
+    return 'ORDER BY id DESC';
+  }
+
 }

src/applications/audit/query/audit/__init__.php

@@ -7,9 +7,12 @@
 
 
 phutil_require_module('phabricator', 'applications/audit/constants/status');
+phutil_require_module('phabricator', 'applications/audit/query/commit');
 phutil_require_module('phabricator', 'applications/repository/storage/auditrequest');
 phutil_require_module('phabricator', 'storage/qsprintf');
 phutil_require_module('phabricator', 'storage/queryfx');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('PhabricatorAuditQuery.php');

src/applications/audit/query/commit/PhabricatorAuditCommitQuery.php

@@ -21,6 +21,7 @@ final class PhabricatorAuditCommitQuery {
   private $offset;
   private $limit;
 
+  private $commitPHIDs;
   private $authorPHIDs;
   private $packagePHIDs;
 
@@ -40,6 +41,11 @@ final class PhabricatorAuditCommitQuery {
     return $this;
   }
 
+  public function withCommitPHIDs(array $phids) {
+    $this->commitPHIDs = $phids;
+    return $this;
+  }
+
   public function withStatus($status) {
     $this->status = $status;
     return $this;
@@ -125,6 +131,13 @@ final class PhabricatorAuditCommitQuery {
   private function buildWhereClause($conn_r) {
     $where = array();
 
+    if ($this->commitPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'c.phid IN (%Ls)',
+        $this->commitPHIDs);
+    }
+
     if ($this->authorPHIDs) {
       $where[] = qsprintf(
         $conn_r,

src/applications/audit/view/list/PhabricatorAuditListView.php

@@ -22,6 +22,7 @@ final class PhabricatorAuditListView extends AphrontView {
   private $handles;
   private $authorityPHIDs = array();
   private $noDataString;
+  private $commits;
 
   public function setAudits(array $audits) {
     $this->audits = $audits;
@@ -47,6 +48,11 @@ final class PhabricatorAuditListView extends AphrontView {
     return $this->noDataString;
   }
 
+  public function setCommits(array $commits) {
+    $this->commits = mpull($commits, null, 'getPHID');
+    return $this;
+  }
+
   public function getRequiredHandlePHIDs() {
     $phids = array();
     foreach ($this->audits as $audit) {
@@ -64,6 +70,19 @@ final class PhabricatorAuditListView extends AphrontView {
     return $handle;
   }
 
+  private function getCommitDescription($phid) {
+    if ($this->commits === null) {
+      return null;
+    }
+
+    $commit = idx($this->commits, $phid);
+    if (!$commit) {
+      return null;
+    }
+
+    return $commit->getCommitData()->getSummary();
+  }
+
   public function render() {
 
     $authority = array_fill_keys($this->authorityPHIDs, true);
@@ -76,8 +95,10 @@ final class PhabricatorAuditListView extends AphrontView {
       $commit_phid = $audit->getCommitPHID();
       if ($last == $commit_phid) {
         $commit_name = null;
+        $commit_desc = null;
       } else {
         $commit_name = $this->getHandle($commit_phid)->renderLink();
+        $commit_desc = $this->getCommitDescription($commit_phid);
         $last = $commit_phid;
       }
 
@@ -93,6 +114,7 @@ final class PhabricatorAuditListView extends AphrontView {
       $auditor_handle = $this->getHandle($audit->getAuditorPHID());
       $rows[] = array(
         $commit_name,
+        phutil_escape_html($commit_desc),
         $auditor_handle->renderLink(),
         phutil_escape_html($status),
         $reasons,
@@ -109,6 +131,7 @@ final class PhabricatorAuditListView extends AphrontView {
     $table->setHeaders(
       array(
         'Commit',
+        'Description',
         'Auditor',
         'Status',
         'Details',
@@ -116,11 +139,20 @@ final class PhabricatorAuditListView extends AphrontView {
     $table->setColumnClasses(
       array(
         'pri',
+        (($this->commits === null) ? '' : 'wide'),
         '',
         '',
-        'wide',
+        (($this->commits === null) ? 'wide' : ''),
       ));
     $table->setRowClasses($rowc);
+    $table->setColumnVisibility(
+      array(
+        true,
+        ($this->commits !== null),
+        true,
+        true,
+        true,
+      ));
 
     if ($this->noDataString) {
       $table->setNoDataString($this->noDataString);

src/applications/directory/controller/main/PhabricatorDirectoryMainController.php

@@ -576,9 +576,11 @@ final class PhabricatorDirectoryMainController
     $query = new PhabricatorAuditQuery();
     $query->withAuditorPHIDs($phids);
     $query->withStatus(PhabricatorAuditQuery::STATUS_OPEN);
+    $query->needCommitData(true);
     $query->setLimit(10);
 
     $audits = $query->execute();
+    $commits = $query->getCommits();
 
     if (!$audits) {
       return $this->renderMinipanel(
@@ -588,6 +590,7 @@ final class PhabricatorDirectoryMainController
 
     $view = new PhabricatorAuditListView();
     $view->setAudits($audits);
+    $view->setCommits($commits);
 
     $phids = $view->getRequiredHandlePHIDs();
     $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles();