From 84f21c8a20ca1510cf58a45f656224394e82d678 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 18 May 2013 10:49:37 -0700 Subject: [PATCH] Modernize Audit Summary: Adds mobile support to Audit, converts tables to object item views. I also colored 'concerns' and 'audit required' in the list, but nothing else. We can add more if needed but I'm assuming these are the two most important cases. Test Plan: Tested as much as I could, a little unsure of a few things since my local repo isn't super filled. Will let epriestley run through. Reviewers: epriestley, btrahan Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D5962 --- .../PhabricatorAuditActionConstants.php | 16 ++-- .../PhabricatorAuditCommitStatusConstants.php | 12 +-- .../PhabricatorAuditStatusConstants.php | 37 ++++++--- .../controller/PhabricatorAuditController.php | 30 +++++++ .../PhabricatorAuditListController.php | 32 ++----- .../view/PhabricatorAuditCommitListView.php | 83 ++++++++++--------- .../audit/view/PhabricatorAuditListView.php | 67 ++++++--------- .../controller/DiffusionCommitController.php | 13 ++- .../phabricator-object-item-list-view.css | 5 ++ 9 files changed, 153 insertions(+), 142 deletions(-) diff --git a/src/applications/audit/constants/PhabricatorAuditActionConstants.php b/src/applications/audit/constants/PhabricatorAuditActionConstants.php index c59a56374e..8f25cfb8d4 100644 --- a/src/applications/audit/constants/PhabricatorAuditActionConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditActionConstants.php @@ -11,14 +11,14 @@ final class PhabricatorAuditActionConstants { const ADD_AUDITORS = 'add_auditors'; public static function getActionNameMap() { - static $map = array( - self::COMMENT => 'Comment', - self::CONCERN => "Raise Concern \xE2\x9C\x98", - self::ACCEPT => "Accept Commit \xE2\x9C\x94", - self::RESIGN => 'Resign from Audit', - self::CLOSE => 'Close Audit', - self::ADD_CCS => 'Add CCs', - self::ADD_AUDITORS => 'Add Auditors', + $map = array( + self::COMMENT => pht('Comment'), + self::CONCERN => pht("Raise Concern \xE2\x9C\x98"), + self::ACCEPT => pht("Accept Commit \xE2\x9C\x94"), + self::RESIGN => pht('Resign from Audit'), + self::CLOSE => pht('Close Audit'), + self::ADD_CCS => pht('Add CCs'), + self::ADD_AUDITORS => pht('Add Auditors'), ); return $map; diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index a4c0e96ca8..1f6164733a 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -9,12 +9,12 @@ final class PhabricatorAuditCommitStatusConstants { const FULLY_AUDITED = 4; public static function getStatusNameMap() { - static $map = array( - self::NONE => 'None', - self::NEEDS_AUDIT => 'Audit Required', - self::CONCERN_RAISED => 'Concern Raised', - self::PARTIALLY_AUDITED => 'Partially Audited', - self::FULLY_AUDITED => 'Audited', + $map = array( + self::NONE => pht('None'), + self::NEEDS_AUDIT => pht('Audit Required'), + self::CONCERN_RAISED => pht('Concern Raised'), + self::PARTIALLY_AUDITED => pht('Partially Audited'), + self::FULLY_AUDITED => pht('Audited'), ); return $map; diff --git a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php index 152d257dd0..70a244a9de 100644 --- a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php @@ -13,23 +13,38 @@ final class PhabricatorAuditStatusConstants { const CC = 'cc'; public static function getStatusNameMap() { - static $map = array( - self::NONE => 'Not Applicable', - self::AUDIT_NOT_REQUIRED => 'Audit Not Required', - self::AUDIT_REQUIRED => 'Audit Required', - self::CONCERNED => 'Concern Raised', - self::ACCEPTED => 'Accepted', - self::AUDIT_REQUESTED => 'Audit Requested', - self::RESIGNED => 'Resigned', - self::CLOSED => 'Closed', - self::CC => "Was CC'd", + $map = array( + self::NONE => pht('Not Applicable'), + self::AUDIT_NOT_REQUIRED => pht('Audit Not Required'), + self::AUDIT_REQUIRED => pht('Audit Required'), + self::CONCERNED => pht('Concern Raised'), + self::ACCEPTED => pht('Accepted'), + self::AUDIT_REQUESTED => pht('Audit Requested'), + self::RESIGNED => pht('Resigned'), + self::CLOSED => pht('Closed'), + self::CC => pht("Was CC'd"), ); return $map; } public static function getStatusName($code) { - return idx(self::getStatusNameMap(), $code, 'Unknown'); + return idx(self::getStatusNameMap(), $code, pht('Unknown')); + } + + public static function getStatusColor($code) { + switch ($code) { + case self::CONCERNED: + $color = 'orange'; + break; + case self::AUDIT_REQUIRED: + $color = 'red'; + break; + default: + $color = null; + break; + } + return $color; } public static function getOpenStatusConstants() { diff --git a/src/applications/audit/controller/PhabricatorAuditController.php b/src/applications/audit/controller/PhabricatorAuditController.php index 0c9f4ef318..b920afe58b 100644 --- a/src/applications/audit/controller/PhabricatorAuditController.php +++ b/src/applications/audit/controller/PhabricatorAuditController.php @@ -2,6 +2,36 @@ abstract class PhabricatorAuditController extends PhabricatorController { + public $filter; + + public function buildSideNavView() { + + $nav = new AphrontSideNavFilterView(); + $nav->setBaseURI(new PhutilURI('/audit/view/')); + $nav->addLabel(pht('Active')); + $nav->addFilter('active', pht('Need Attention')); + + $nav->addLabel(pht('Audits')); + $nav->addFilter('audits', pht('All')); + $nav->addFilter('user', pht('By User')); + $nav->addFilter('project', pht('By Project')); + $nav->addFilter('package', pht('By Package')); + $nav->addFilter('repository', pht('By Repository')); + + $nav->addLabel(pht('Commits')); + $nav->addFilter('commits', pht('All')); + $nav->addFilter('author', pht('By Author')); + $nav->addFilter('packagecommits', pht('By Package')); + + $this->filter = $nav->selectFilter($this->filter, 'active'); + + return $nav; + } + + public function buildApplicationMenu() { + return $this->buildSideNavView()->getMenu(); + } + public function buildStandardPageResponse($view, array $data) { $page = $this->buildStandardPageView(); diff --git a/src/applications/audit/controller/PhabricatorAuditListController.php b/src/applications/audit/controller/PhabricatorAuditListController.php index 717545e3a1..681d78fb9b 100644 --- a/src/applications/audit/controller/PhabricatorAuditListController.php +++ b/src/applications/audit/controller/PhabricatorAuditListController.php @@ -2,7 +2,6 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { - private $filter; private $name; private $filterStatus; @@ -13,8 +12,7 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { public function processRequest() { $request = $this->getRequest(); - - $nav = $this->buildNavAndSelectFilter(); + $nav = $this->buildSideNavView(); if ($request->isFormPost()) { // If the list filter is POST'ed, redirect to GET so the page can be @@ -75,42 +73,22 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { $nav->appendChild($panel); } - return $this->buildStandardPageResponse( + return $this->buildApplicationPage( $nav, array( 'title' => pht('Audits'), + 'device' => true, + 'dust' => true, )); } - private function buildNavAndSelectFilter() { - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/audit/view/')); - $nav->addLabel(pht('Active')); - $nav->addFilter('active', pht('Need Attention')); - - $nav->addLabel(pht('Audits')); - $nav->addFilter('audits', pht('All')); - $nav->addFilter('user', pht('By User')); - $nav->addFilter('project', pht('By Project')); - $nav->addFilter('package', pht('By Package')); - $nav->addFilter('repository', pht('By Repository')); - - $nav->addLabel(pht('Commits')); - $nav->addFilter('commits', pht('All')); - $nav->addFilter('author', pht('By Author')); - $nav->addFilter('packagecommits', pht('By Package')); - - $this->filter = $nav->selectFilter($this->filter, 'active'); - - return $nav; - } - private function buildListFilters(PhabricatorObjectHandle $handle = null) { $request = $this->getRequest(); $user = $request->getUser(); $form = new AphrontFormView(); $form->setUser($user); + $form->setNoShading(true); $show_status = false; $show_user = false; diff --git a/src/applications/audit/view/PhabricatorAuditCommitListView.php b/src/applications/audit/view/PhabricatorAuditCommitListView.php index d7ab0cb671..ffa62f4482 100644 --- a/src/applications/audit/view/PhabricatorAuditCommitListView.php +++ b/src/applications/audit/view/PhabricatorAuditCommitListView.php @@ -13,7 +13,7 @@ final class PhabricatorAuditCommitListView extends AphrontView { public function setCommits(array $commits) { assert_instances_of($commits, 'PhabricatorRepositoryCommit'); - $this->commits = $commits; + $this->commits = mpull($commits, null, 'getPHID'); return $this; } @@ -52,10 +52,29 @@ final class PhabricatorAuditCommitListView 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() { - $rows = array(); + $list = new PhabricatorObjectItemListView(); + $list->setCards(true); + $list->setFlush(true); foreach ($this->commits as $commit) { - $commit_name = $this->getHandle($commit->getPHID())->renderLink(); + $commit_phid = $commit->getPHID(); + $commit_name = $this->getHandle($commit_phid)->getName(); + $commit_link = $this->getHandle($commit_phid)->getURI(); + $commit_desc = $this->getCommitDescription($commit_phid); + $author_name = null; if ($commit->getAuthorPHID()) { $author_name = $this->getHandle($commit->getAuthorPHID())->renderLink(); @@ -66,51 +85,35 @@ final class PhabricatorAuditCommitListView extends AphrontView { $actor_phid = $audit->getActorPHID(); $auditors[$actor_phid] = $this->getHandle($actor_phid)->renderLink(); } + $auditors = phutil_implode_html(', ', $auditors); } - $rows[] = array( - $commit_name, - $author_name, - PhabricatorAuditCommitStatusConstants::getStatusName( - $commit->getAuditStatus()), - phutil_implode_html(', ', $auditors), - phabricator_datetime($commit->getEpoch(), $this->user), - ); - } + $committed = phabricator_datetime($commit->getEpoch(), $this->user); + $commit_status = PhabricatorAuditCommitStatusConstants::getStatusName( + $commit->getAuditStatus()); - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - 'Commit', - 'Author', - 'Audit Status', - 'Auditors', - 'Date', - )); - $table->setColumnClasses( - array( - 'wide', - '', - '', - '', - '', - )); + $item = id(new PhabricatorObjectItemView()) + ->setObjectName($commit_name) + ->setHeader($commit_desc) + ->setHref($commit_link) + ->addAttribute($commit_status) + ->addIcon('none', $committed); - if ($this->commits && reset($this->commits)->getAudits() === null) { - $table->setColumnVisibility( - array( - true, - true, - true, - false, - true, - )); + if (!empty($auditors)) { + $item->addAttribute(pht('Auditors: %s', $auditors)); + } + + if ($author_name) { + $item->addByline(pht('Author: %s', $author_name)); + } + + $list->addItem($item); } if ($this->noDataString) { - $table->setNoDataString($this->noDataString); + $list->setNoDataString($this->noDataString); } - return $table->render(); + return $list->render(); } } diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php index 42487dae5b..b11f705c2f 100644 --- a/src/applications/audit/view/PhabricatorAuditListView.php +++ b/src/applications/audit/view/PhabricatorAuditListView.php @@ -110,12 +110,15 @@ final class PhabricatorAuditListView extends AphrontView { public function render() { $rowc = array(); - $rows = array(); + $list = new PhabricatorObjectItemListView(); + $list->setCards(true); + $list->setFlush(true); foreach ($this->audits as $audit) { $commit_phid = $audit->getCommitPHID(); $committed = null; - $commit_name = $this->getHandle($commit_phid)->renderLink(); + $commit_name = $this->getHandle($commit_phid)->getName(); + $commit_link = $this->getHandle($commit_phid)->getURI(); $commit_desc = $this->getCommitDescription($commit_phid); $commit = idx($this->commits, $commit_phid); if ($commit && $this->user) { @@ -123,59 +126,37 @@ final class PhabricatorAuditListView extends AphrontView { } $reasons = $audit->getAuditReasons(); - $reasons = phutil_implode_html(phutil_tag('br'), $reasons); + $reasons = phutil_implode_html(', ', $reasons); $status_code = $audit->getAuditStatus(); - $status = PhabricatorAuditStatusConstants::getStatusName($status_code); + $status_text = + PhabricatorAuditStatusConstants::getStatusName($status_code); + $status_color = + PhabricatorAuditStatusConstants::getStatusColor($status_code); $auditor_handle = $this->getHandle($audit->getAuditorPHID()); - $rows[] = array( - $commit_name, - $committed, - $auditor_handle->renderLink(), - $status, - $reasons, - ); + $item = id(new PhabricatorObjectItemView()) + ->setObjectName($commit_name) + ->setHeader($commit_desc) + ->setHref($commit_link) + ->setBarColor($status_color) + ->addAttribute($status_text) + ->addAttribute($reasons) + ->addIcon('none', $committed) + ->addByline(pht('Auditor: %s', $auditor_handle->renderLink())); - $row_class = null; if (array_key_exists($audit->getID(), $this->getHighlightedAudits())) { - $row_class = 'highlighted'; + $item->setBarColor('yellow'); } - $rowc[] = $row_class; - } - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - 'Commit', - 'Committed', - 'Auditor', - 'Status', - 'Details', - )); - $table->setColumnClasses( - array( - 'pri', - '', - '', - '', - ($this->showCommits ? '' : 'wide'), - )); - $table->setRowClasses($rowc); - $table->setColumnVisibility( - array( - $this->showCommits, - $this->showCommits, - true, - true, - true, - )); + $list->addItem($item); + } if ($this->noDataString) { - $table->setNoDataString($this->noDataString); + $list->setNoDataString($this->noDataString); } - return $table->render(); + return $list->render(); } } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index f11eceecba..792adc8640 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -359,6 +359,7 @@ final class DiffusionCommitController extends DiffusionController { $content, array( 'title' => $commit_id, + 'dust' => true, )); } @@ -649,12 +650,9 @@ final class DiffusionCommitController extends DiffusionController { id(new AphrontFormSubmitControl()) ->setValue($is_serious ? pht('Submit') : pht('Cook the Books'))); - $panel = new AphrontPanelView(); - $panel->setHeader( + $header = new PhabricatorHeaderView(); + $header->setHeader( $is_serious ? pht('Audit Commit') : pht('Creative Accounting')); - $panel->appendChild($form); - $panel->addClass('aphront-panel-accent'); - $panel->addClass('aphront-panel-flush'); require_celerity_resource('phabricator-transaction-view-css'); @@ -714,12 +712,13 @@ final class DiffusionCommitController extends DiffusionController { 'id' => $pane_id, ), hsprintf( - '
%s%s%s
', + '
%s%s%s%s
', id(new PhabricatorAnchorView()) ->setAnchorName('comment') ->setNavigationMarker(true) ->render(), - $panel->render(), + $header, + $form, $preview_panel)); } diff --git a/webroot/rsrc/css/layout/phabricator-object-item-list-view.css b/webroot/rsrc/css/layout/phabricator-object-item-list-view.css index 9467495803..e5f845931e 100644 --- a/webroot/rsrc/css/layout/phabricator-object-item-list-view.css +++ b/webroot/rsrc/css/layout/phabricator-object-item-list-view.css @@ -346,6 +346,11 @@ background: #bfdcff; } +.phabricator-object-list-flush .aphront-error-view { + margin: 0; + background: #fff; +} + /* - Foot Icons ----------------------------------------------------------------