From 91d8b3c8abe65bd7836fb823b20edc0349823ae8 Mon Sep 17 00:00:00 2001 From: vrana Date: Mon, 16 Jul 2012 15:00:32 -0700 Subject: [PATCH] Highlight audited paths in commit detail Summary: This doesn't indicate which path is part of which package - I think it would be too heavy. It just highlights the paths in a similar way as audits are highlighted. Maybe we can use different colors for highlighting different packages and use them also in paths. We can mix the colors if one path is part of more packages :-). Test Plan: Viewed commit with 9 files and 4 packages where I am responsible only for one of them. Verified that the only file in my package is highlighted. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin, haugen Maniphest Tasks: T1226 Differential Revision: https://secure.phabricator.com/D2982 --- .../audit/view/PhabricatorAuditListView.php | 52 ++++++++++++------- .../controller/DiffusionCommitController.php | 16 ++++++ .../view/DiffusionCommitChangeTableView.php | 19 +++++++ 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php index 8df1a80727..67769584b2 100644 --- a/src/applications/audit/view/PhabricatorAuditListView.php +++ b/src/applications/audit/view/PhabricatorAuditListView.php @@ -26,6 +26,8 @@ final class PhabricatorAuditListView extends AphrontView { private $user; private $showDescriptions = true; + private $highlightedAudits; + public function setAudits(array $audits) { assert_instances_of($audits, 'PhabricatorRepositoryAuditRequest'); $this->audits = $audits; @@ -98,11 +100,36 @@ final class PhabricatorAuditListView extends AphrontView { return $commit->getCommitData()->getSummary(); } + public function getHighlightedAudits() { + if ($this->highlightedAudits === null) { + $this->highlightedAudits = array(); + + $user = $this->user; + $authority = array_fill_keys($this->authorityPHIDs, true); + + foreach ($this->audits as $audit) { + $has_authority = !empty($authority[$audit->getAuditorPHID()]); + if ($has_authority) { + $commit_phid = $audit->getCommitPHID(); + $commit_author = $this->commits[$commit_phid]->getAuthorPHID(); + + // You don't have authority over package and project audits on your + // own commits. + + $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); + $user_is_author = ($commit_author == $user->getPHID()); + + if ($auditor_is_user || !$user_is_author) { + $this->highlightedAudits[$audit->getID()] = $audit; + } + } + } + } + + return $this->highlightedAudits; + } + public function render() { - $user = $this->user; - - $authority = array_fill_keys($this->authorityPHIDs, true); - $rowc = array(); $last = null; @@ -137,22 +164,9 @@ final class PhabricatorAuditListView extends AphrontView { ); $row_class = null; - - $has_authority = !empty($authority[$audit->getAuditorPHID()]); - if ($has_authority) { - $commit_author = $this->commits[$commit_phid]->getAuthorPHID(); - - // You don't have authority over package and project audits on your own - // commits. - - $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); - $user_is_author = ($commit_author == $user->getPHID()); - - if ($auditor_is_user || !$user_is_author) { - $row_class = 'highlighted'; - } + if (array_key_exists($audit->getID(), $this->getHighlightedAudits())) { + $row_class = 'highlighted'; } - $rowc[] = $row_class; } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 62d8b7023e..9e4e20c841 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -21,6 +21,7 @@ final class DiffusionCommitController extends DiffusionController { const CHANGES_LIMIT = 100; private $auditAuthorityPHIDs; + private $highlightedAudits; public function willProcessRequest(array $data) { // This controller doesn't use blob/path stuff, just pass the dictionary @@ -119,9 +120,23 @@ final class DiffusionCommitController extends DiffusionController { $changes = array_slice($changes, 0, self::CHANGES_LIMIT); } + $owners_paths = array(); + if ($this->highlightedAudits) { + $packages = id(new PhabricatorOwnersPackage())->loadAllWhere( + 'phid IN (%Ls)', + mpull($this->highlightedAudits, 'getAuditorPHID')); + if ($packages) { + $owners_paths = id(new PhabricatorOwnersPath())->loadAllWhere( + 'repositoryPHID = %s AND packageID IN (%Ld)', + $repository->getPHID(), + mpull($packages, 'getID')); + } + } + $change_table = new DiffusionCommitChangeTableView(); $change_table->setDiffusionRequest($drequest); $change_table->setPathChanges($changes); + $change_table->setOwnersPaths($owners_paths); $count = count($changes); @@ -404,6 +419,7 @@ final class DiffusionCommitController extends DiffusionController { $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); $view->setHandles($handles); $view->setAuthorityPHIDs($this->auditAuthorityPHIDs); + $this->highlightedAudits = $view->getHighlightedAudits(); $panel = new AphrontPanelView(); $panel->setHeader('Audits'); diff --git a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php b/src/applications/diffusion/view/DiffusionCommitChangeTableView.php index 89e170ed02..c688913abe 100644 --- a/src/applications/diffusion/view/DiffusionCommitChangeTableView.php +++ b/src/applications/diffusion/view/DiffusionCommitChangeTableView.php @@ -19,6 +19,7 @@ final class DiffusionCommitChangeTableView extends DiffusionView { private $pathChanges; + private $ownersPaths = array(); public function setPathChanges(array $path_changes) { assert_instances_of($path_changes, 'DiffusionPathChange'); @@ -26,8 +27,15 @@ final class DiffusionCommitChangeTableView extends DiffusionView { return $this; } + public function setOwnersPaths(array $owners_paths) { + assert_instances_of($owners_paths, 'PhabricatorOwnersPath'); + $this->ownersPaths = $owners_paths; + return $this; + } + public function render() { $rows = array(); + $rowc = array(); // TODO: Experiment with path stack rendering. @@ -56,6 +64,16 @@ final class DiffusionCommitChangeTableView extends DiffusionView { $change->getPath()), $path_column, ); + + $row_class = null; + foreach ($this->ownersPaths as $owners_path) { + $owners_path = $owners_path->getPath(); + if (strncmp('/'.$path, $owners_path, strlen($owners_path)) == 0) { + $row_class = 'highlighted'; + break; + } + } + $rowc[] = $row_class; } $view = new AphrontTableView($rows); @@ -73,6 +91,7 @@ final class DiffusionCommitChangeTableView extends DiffusionView { '', 'wide', )); + $view->setRowClasses($rowc); $view->setNoDataString('This change has not been fully parsed yet.'); return $view->render();