From c13b7da2907e932fb13f40db3206a1d173adfc7d Mon Sep 17 00:00:00 2001 From: jungejason Date: Wed, 14 Dec 2011 00:53:25 -0800 Subject: [PATCH] Add Related Commits for Owners Summary: For each commit, find the affected packages, and provide a way to search by package. Test Plan: create commits that touch and don't touch two packages, and verify that they display correctly in all the UI pages. Reviewers: epriestley, blair, nh, tuomaspelkonen Reviewed By: epriestley CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi Maniphest Tasks: T83 Differential Revision: 1208 --- .../patches/085.packagecommitrelationship.sql | 8 + scripts/repository/reparse.php | 37 ++- src/__phutil_library_map__.php | 7 + ...AphrontDefaultApplicationConfiguration.php | 4 + .../adapter/commit/HeraldCommitAdapter.php | 23 +- .../herald/adapter/commit/__init__.php | 4 +- .../base/PhabricatorOwnersController.php | 4 + .../PhabricatorOwnersDetailController.php | 11 + .../list/PhabricatorOwnersListController.php | 8 + .../PhabricatorOwnerRelatedListController.php | 229 ++++++++++++++++++ .../controller/relatedlist/__init__.php | 30 +++ .../query/path/PhabricatorOwnerPathQuery.php | 52 ++++ .../owners/query/path/__init__.php | 14 ++ ...ricatorOwnersPackageCommitRelationship.php | 30 +++ .../packagecommitrelationship/__init__.php | 12 + ...atorRepositoryCommitChangeParserWorker.php | 16 +- ...habricatorRepositoryCommitOwnersWorker.php | 50 ++++ .../repository/worker/owner/__init__.php | 17 ++ 18 files changed, 524 insertions(+), 32 deletions(-) create mode 100644 resources/sql/patches/085.packagecommitrelationship.sql create mode 100644 src/applications/owners/controller/relatedlist/PhabricatorOwnerRelatedListController.php create mode 100644 src/applications/owners/controller/relatedlist/__init__.php create mode 100644 src/applications/owners/query/path/PhabricatorOwnerPathQuery.php create mode 100644 src/applications/owners/query/path/__init__.php create mode 100644 src/applications/owners/storage/packagecommitrelationship/PhabricatorOwnersPackageCommitRelationship.php create mode 100644 src/applications/owners/storage/packagecommitrelationship/__init__.php create mode 100644 src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php create mode 100644 src/applications/repository/worker/owner/__init__.php diff --git a/resources/sql/patches/085.packagecommitrelationship.sql b/resources/sql/patches/085.packagecommitrelationship.sql new file mode 100644 index 0000000000..ed6222f17e --- /dev/null +++ b/resources/sql/patches/085.packagecommitrelationship.sql @@ -0,0 +1,8 @@ +CREATE TABLE IF NOT EXISTS phabricator_owners.owners_packagecommitrelationship ( + `id` int(10) unsigned NOT NULL AUTO_INCREMENT, + `packagePHID` varchar(64) CHARACTER SET latin1 COLLATE latin1_bin NOT NULL, + `commitPHID` varchar(64) CHARACTER SET latin1 COLLATE latin1_bin NOT NULL, + PRIMARY KEY (`id`), + KEY `packagePHID` (`packagePHID`), + KEY `commitPHID` (`commitPHID`) +) ENGINE=InnoDB DEFAULT CHARSET=latin1; diff --git a/scripts/repository/reparse.php b/scripts/repository/reparse.php index f6e0fff060..c6849f7c79 100755 --- a/scripts/repository/reparse.php +++ b/scripts/repository/reparse.php @@ -26,7 +26,9 @@ $is_all = false; $reparse_message = false; $reparse_change = false; $reparse_herald = false; +$reparse_owners = false; $reparse_what = false; +$force = false; $args = array_slice($argv, 1); foreach ($args as $arg) { @@ -47,6 +49,12 @@ foreach ($args as $arg) { case 'herald': $reparse_herald = true; break; + case 'owners': + $reparse_owners = true; + break; + case 'force': + $force = true; + break; case 'trace': PhutilServiceProfiler::installEchoListener(); break; @@ -67,9 +75,21 @@ foreach ($args as $arg) { if (!$reparse_what) { usage("Specify a commit or repository to reparse."); } -if (!$reparse_message && !$reparse_change && !$reparse_herald) { - usage("Specify what information to reparse with --message, --change, and/or ". - "--herald."); +if (!$reparse_message && !$reparse_change && !$reparse_herald && + !$reparse_owners) { + usage("Specify what information to reparse with --message, --change, ". + "--herald, and/or --owners"); +} +if ($reparse_owners && !$force) { + echo phutil_console_wrap( + "You are about to recreate the relationship entries between the commits ". + "and the packages they touch. This might delete some existing ". + "relationship entries for some old commits."); + + if (!phutil_console_confirm('Are you ready to continue?')) { + echo "Cancelled.\n"; + exit(1); + } } $commits = array(); @@ -157,6 +177,10 @@ foreach ($commits as $commit) { $classes[] = 'PhabricatorRepositoryCommitHeraldWorker'; } + if ($reparse_owners) { + $classes[] = 'PhabricatorRepositoryCommitOwnersWorker'; + } + $spec = array( 'commitID' => $commit->getID(), 'only' => true, @@ -194,7 +218,7 @@ function help() { $help = << 'applications/phid/handle/data', 'PhabricatorObjectHandleStatus' => 'applications/phid/handle/const/status', 'PhabricatorObjectSelectorDialog' => 'view/control/objectselector', + 'PhabricatorOwnerPathQuery' => 'applications/owners/query/path', + 'PhabricatorOwnerRelatedListController' => 'applications/owners/controller/relatedlist', 'PhabricatorOwnersController' => 'applications/owners/controller/base', 'PhabricatorOwnersDAO' => 'applications/owners/storage/base', 'PhabricatorOwnersDeleteController' => 'applications/owners/controller/delete', @@ -534,6 +536,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersListController' => 'applications/owners/controller/list', 'PhabricatorOwnersOwner' => 'applications/owners/storage/owner', 'PhabricatorOwnersPackage' => 'applications/owners/storage/package', + 'PhabricatorOwnersPackageCommitRelationship' => 'applications/owners/storage/packagecommitrelationship', 'PhabricatorOwnersPath' => 'applications/owners/storage/path', 'PhabricatorPHID' => 'applications/phid/storage/phid', 'PhabricatorPHIDConstants' => 'applications/phid/constants', @@ -590,6 +593,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryCommitHeraldWorker' => 'applications/repository/worker/herald', 'PhabricatorRepositoryCommitMessageDetailParser' => 'applications/repository/parser/base', 'PhabricatorRepositoryCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/base', + 'PhabricatorRepositoryCommitOwnersWorker' => 'applications/repository/worker/owner', 'PhabricatorRepositoryCommitParserWorker' => 'applications/repository/worker/base', 'PhabricatorRepositoryCommitTaskDaemon' => 'applications/repository/daemon/committask', 'PhabricatorRepositoryController' => 'applications/repository/controller/base', @@ -1157,6 +1161,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController', 'PhabricatorObjectGraph' => 'AbstractDirectedGraph', 'PhabricatorObjectHandleStatus' => 'PhabricatorObjectHandleConstants', + 'PhabricatorOwnerRelatedListController' => 'PhabricatorOwnersController', 'PhabricatorOwnersController' => 'PhabricatorController', 'PhabricatorOwnersDAO' => 'PhabricatorLiskDAO', 'PhabricatorOwnersDeleteController' => 'PhabricatorOwnersController', @@ -1165,6 +1170,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersListController' => 'PhabricatorOwnersController', 'PhabricatorOwnersOwner' => 'PhabricatorOwnersDAO', 'PhabricatorOwnersPackage' => 'PhabricatorOwnersDAO', + 'PhabricatorOwnersPackageCommitRelationship' => 'PhabricatorOwnersDAO', 'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO', 'PhabricatorPHID' => 'PhabricatorPHIDDAO', 'PhabricatorPHIDController' => 'PhabricatorController', @@ -1218,6 +1224,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryCommitDiscoveryDaemon' => 'PhabricatorRepositoryDaemon', 'PhabricatorRepositoryCommitHeraldWorker' => 'PhabricatorRepositoryCommitParserWorker', 'PhabricatorRepositoryCommitMessageParserWorker' => 'PhabricatorRepositoryCommitParserWorker', + 'PhabricatorRepositoryCommitOwnersWorker' => 'PhabricatorRepositoryCommitParserWorker', 'PhabricatorRepositoryCommitParserWorker' => 'PhabricatorWorker', 'PhabricatorRepositoryCommitTaskDaemon' => 'PhabricatorRepositoryDaemon', 'PhabricatorRepositoryController' => 'PhabricatorController', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index ce26384b35..5e8e85b115 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -296,6 +296,10 @@ class AphrontDefaultApplicationConfiguration 'new/$' => 'PhabricatorOwnersEditController', 'package/(?P\d+)/$' => 'PhabricatorOwnersDetailController', 'delete/(?P\d+)/$' => 'PhabricatorOwnersDeleteController', + 'related/' => array( + '$' => 'PhabricatorOwnerRelatedListController', + 'view/(?P[^/]+)/$' => 'PhabricatorOwnerRelatedListController', + ), ), '/xhpast/' => array( diff --git a/src/applications/herald/adapter/commit/HeraldCommitAdapter.php b/src/applications/herald/adapter/commit/HeraldCommitAdapter.php index a897e3f6c3..31c6537a6c 100644 --- a/src/applications/herald/adapter/commit/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/commit/HeraldCommitAdapter.php @@ -62,19 +62,8 @@ class HeraldCommitAdapter extends HeraldObjectAdapter { public function loadAffectedPaths() { if ($this->affectedPaths === null) { - $drequest = $this->buildDiffusionRequest(); - $path_query = DiffusionPathChangeQuery::newFromDiffusionRequest( - $drequest); - $paths = $path_query->loadChanges(); - - $result = array(); - foreach ($paths as $path) { - $basic_path = '/'.$path->getPath(); - if ($path->getFileType() == DifferentialChangeType::FILE_DIRECTORY) { - $basic_path = rtrim($basic_path, '/').'/'; - } - $result[] = $basic_path; - } + $result = PhabricatorOwnerPathQuery::loadAffectedPaths( + $this->repository, $this->commit); $this->affectedPaths = $result; } return $this->affectedPaths; @@ -106,14 +95,6 @@ class HeraldCommitAdapter extends HeraldObjectAdapter { return $this->affectedRevision; } - private function buildDiffusionRequest() { - return DiffusionRequest::newFromAphrontRequestDictionary( - array( - 'callsign' => $this->repository->getCallsign(), - 'commit' => $this->commit->getCommitIdentifier(), - )); - } - public function getHeraldField($field) { $data = $this->commitData; switch ($field) { diff --git a/src/applications/herald/adapter/commit/__init__.php b/src/applications/herald/adapter/commit/__init__.php index 01385ec1d0..24262a55f0 100644 --- a/src/applications/herald/adapter/commit/__init__.php +++ b/src/applications/herald/adapter/commit/__init__.php @@ -6,15 +6,13 @@ -phutil_require_module('phabricator', 'applications/differential/constants/changetype'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); -phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base'); -phutil_require_module('phabricator', 'applications/diffusion/request/base'); phutil_require_module('phabricator', 'applications/herald/adapter/base'); phutil_require_module('phabricator', 'applications/herald/config/action'); phutil_require_module('phabricator', 'applications/herald/config/contenttype'); phutil_require_module('phabricator', 'applications/herald/config/field'); phutil_require_module('phabricator', 'applications/herald/storage/transcript/apply'); +phutil_require_module('phabricator', 'applications/owners/query/path'); phutil_require_module('phabricator', 'applications/owners/storage/owner'); phutil_require_module('phabricator', 'applications/owners/storage/package'); diff --git a/src/applications/owners/controller/base/PhabricatorOwnersController.php b/src/applications/owners/controller/base/PhabricatorOwnersController.php index a18d3fcba2..32f36922fd 100644 --- a/src/applications/owners/controller/base/PhabricatorOwnersController.php +++ b/src/applications/owners/controller/base/PhabricatorOwnersController.php @@ -31,6 +31,10 @@ abstract class PhabricatorOwnersController extends PhabricatorController { 'href' => '/owners/', 'name' => 'Package Index', ), + 'related' => array( + 'href' => '/owners/related/', + 'name' => 'Related Commits', + ), ), idx($data, 'tab')); $page->setGlyph("\xE2\x98\x81"); diff --git a/src/applications/owners/controller/detail/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/detail/PhabricatorOwnersDetailController.php index 9709472e1a..58a64ec6c8 100644 --- a/src/applications/owners/controller/detail/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/detail/PhabricatorOwnersDetailController.php @@ -76,6 +76,17 @@ class PhabricatorOwnersDetailController extends PhabricatorOwnersController { 'Owners', $owner_links); + $rows[] = array( + 'Related Commits', + phutil_render_tag( + 'a', + array( + 'href' => '/owners/related/view/all/?phid='.$package->getPHID(), + ), + phutil_escape_html('Related Commits')) + ); + + $path_links = array(); foreach ($paths as $path) { $callsign = $handles[$path->getRepositoryPHID()]->getName(); diff --git a/src/applications/owners/controller/list/PhabricatorOwnersListController.php b/src/applications/owners/controller/list/PhabricatorOwnersListController.php index 76810b448c..8e154bf217 100644 --- a/src/applications/owners/controller/list/PhabricatorOwnersListController.php +++ b/src/applications/owners/controller/list/PhabricatorOwnersListController.php @@ -257,6 +257,12 @@ class PhabricatorOwnersListController extends PhabricatorOwnersController { phutil_escape_html($package->getName())), $pkg_owners, $pkg_paths, + phutil_render_tag( + 'a', + array( + 'href' => '/owners/related/view/all/?phid='.$package->getPHID(), + ), + phutil_escape_html('Related Commits')) ); } @@ -266,12 +272,14 @@ class PhabricatorOwnersListController extends PhabricatorOwnersController { 'Name', 'Owners', 'Paths', + 'Related Commits', )); $table->setColumnClasses( array( 'pri', '', 'wide wrap', + 'narrow', )); $panel = new AphrontPanelView(); diff --git a/src/applications/owners/controller/relatedlist/PhabricatorOwnerRelatedListController.php b/src/applications/owners/controller/relatedlist/PhabricatorOwnerRelatedListController.php new file mode 100644 index 0000000000..d646f5b65f --- /dev/null +++ b/src/applications/owners/controller/relatedlist/PhabricatorOwnerRelatedListController.php @@ -0,0 +1,229 @@ +view = idx($data, 'view', 'all'); + } + + public function processRequest() { + $this->request = $this->getRequest(); + + if ($this->request->isFormPost()) { + $package_phids = $this->request->getArr('search_packages'); + $package_phid = head($package_phids); + return id(new AphrontRedirectResponse()) + ->setURI( + $this->request + ->getRequestURI() + ->alter('phid', $package_phid)); + } + + $this->user = $this->request->getUser(); + $this->packagePHID = nonempty($this->request->getStr('phid'), null); + + $search_view = $this->renderSearchView(); + $list_panel = $this->renderListPanel(); + $nav = $this->renderSideNav(); + + $nav->appendChild($search_view); + $nav->appendChild($list_panel); + + return $this->buildStandardPageResponse( + $nav, + array( + 'title' => 'Related Commits', + 'tab' => 'related', + )); + } + + private function renderListPanel() { + if (!$this->packagePHID) { + return id(new AphrontErrorView()) + ->setSeverity(AphrontErrorView::SEVERITY_NOTICE) + ->setTitle('No package seleted. Please select one from above.'); + } + + $package = id(new PhabricatorOwnersPackage())->loadOneWhere( + "phid = %s", + $this->packagePHID); + + $offset = $this->request->getInt('offset', 0); + $pager = new AphrontPagerView(); + $pager->setPageSize(50); + $pager->setOffset($offset); + $pager->setURI($this->request->getRequestURI(), 'offset'); + + $conn_r = id(new PhabricatorOwnersPackageCommitRelationship()) + ->establishConnection('r'); + + switch ($this->view) { + case 'all': + $data = queryfx_all( + $conn_r, + 'SELECT commitPHID FROM %T + WHERE packagePHID = %s + ORDER BY id DESC + LIMIT %d, %d', + id(new PhabricatorOwnersPackageCommitRelationship())->getTableName(), + $package->getPHID(), + $pager->getOffset(), + $pager->getPageSize() + 1); + break; + + default: + throw new Exception("view {$this->view} not recognized"); + } + + $data = $pager->sliceResults($data); + $data = ipull($data, null, 'commitPHID'); + + $list_panel = $this->renderCommitTable($data, $package); + $list_panel->appendChild($pager); + + return $list_panel; + } + + private function renderSideNav() { + $views = array( + 'all' => 'Related to Package', + ); + + $query = null; + if ($this->packagePHID) { + $query = '?phid=' . $this->packagePHID; + } + + $nav = new AphrontSideNavView(); + foreach ($views as $key => $name) { + $nav->addNavItem( + phutil_render_tag( + 'a', + array( + 'href' => '/owners/related/view/'.$key.'/'.$query, + 'class' => ($this->view === $key + ? 'aphront-side-nav-selected' + : null), + ), + phutil_escape_html($name))); + } + return $nav; + } + + private function renderSearchView() { + if ($this->packagePHID) { + $loader = new PhabricatorObjectHandleData(array($this->packagePHID)); + $handles = $loader->loadHandles(); + $package_handle = $handles[$this->packagePHID]; + + $view_packages = array( + $this->packagePHID => $package_handle->getFullName(), + ); + } else { + $view_packages = array(); + } + + $search_form = id(new AphrontFormView()) + ->setUser($this->user) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setDatasource('/typeahead/common/packages/') + ->setLabel('Package') + ->setName('search_packages') + ->setValue($view_packages) + ->setLimit(1)); + + $search_form->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue('Search')); + + $search_view = new AphrontListFilterView(); + $search_view->appendChild($search_form); + return $search_view; + } + + private function renderCommitTable($data, PhabricatorOwnersPackage $package) { + $commit_phids = array_keys($data); + $loader = new PhabricatorObjectHandleData($commit_phids); + $handles = $loader->loadHandles(); + $objects = $loader->loadObjects(); + + $rows = array(); + foreach ($commit_phids as $commit_phid) { + $handle = $handles[$commit_phid]; + $object = $objects[$commit_phid]; + $commit_data = $object->getCommitData(); + $epoch = $handle->getTimeStamp(); + $date = phabricator_date($epoch, $this->user); + $time = phabricator_time($epoch, $this->user); + $link = phutil_render_tag( + 'a', + array( + 'href' => $handle->getURI(), + ), + phutil_escape_html($handle->getName())); + $row = array( + $link, + $date, + $time, + phutil_escape_html($commit_data->getSummary()), + ); + + $rows[] = $row; + } + + $commit_table = new AphrontTableView($rows); + + $headers = array( + 'Commit', + 'Date', + 'Time', + 'Summary', + ); + $commit_table->setHeaders($headers); + + $column_classes = + array( + '', + '', + 'right', + 'wide', + ); + $commit_table->setColumnClasses($column_classes); + + $list_panel = new AphrontPanelView(); + $list_panel->setHeader('Commits Related to package "'. + phutil_render_tag( + 'a', + array( + 'href' => '/owners/package/'.$package->getID().'/', + ), + phutil_escape_html($package->getName())). + '"'); + $list_panel->appendChild($commit_table); + + return $list_panel; + } +} diff --git a/src/applications/owners/controller/relatedlist/__init__.php b/src/applications/owners/controller/relatedlist/__init__.php new file mode 100644 index 0000000000..e89d529402 --- /dev/null +++ b/src/applications/owners/controller/relatedlist/__init__.php @@ -0,0 +1,30 @@ +loadChanges(); + + $result = array(); + foreach ($paths as $path) { + $basic_path = '/' . $path->getPath(); + if ($path->getFileType() == DifferentialChangeType::FILE_DIRECTORY) { + $basic_path = rtrim($basic_path, '/') . '/'; + } + $result[] = $basic_path; + } + return $result; + } + + private static function buildDiffusionRequest( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + + return DiffusionRequest::newFromAphrontRequestDictionary( + array( + 'callsign' => $repository->getCallsign(), + 'commit' => $commit->getCommitIdentifier(), + )); + } + +} diff --git a/src/applications/owners/query/path/__init__.php b/src/applications/owners/query/path/__init__.php new file mode 100644 index 0000000000..ec91a2b6bd --- /dev/null +++ b/src/applications/owners/query/path/__init__.php @@ -0,0 +1,14 @@ + false, + ) + parent::getConfiguration(); + } + +} diff --git a/src/applications/owners/storage/packagecommitrelationship/__init__.php b/src/applications/owners/storage/packagecommitrelationship/__init__.php new file mode 100644 index 0000000000..97415f18d0 --- /dev/null +++ b/src/applications/owners/storage/packagecommitrelationship/__init__.php @@ -0,0 +1,12 @@ +shouldQueueFollowupTasks()) { - $task = new PhabricatorWorkerTask(); - $task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker'); - $task->setData( + $herald_task = new PhabricatorWorkerTask(); + $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker'); + $herald_task->setData( array( 'commitID' => $commit->getID(), )); - $task->save(); + $herald_task->save(); + + $owner_task = new PhabricatorWorkerTask(); + $owner_task->setTaskClass('PhabricatorRepositoryCommitOwnersWorker'); + $owner_task->setData( + array( + 'commitID' => $commit->getID(), + )); + $owner_task->save(); } } diff --git a/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php new file mode 100644 index 0000000000..b48bde9219 --- /dev/null +++ b/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php @@ -0,0 +1,50 @@ +loadOneWhere('packagePHID=%s AND commitPHID=%s', + $package->getPHID(), + $commit->getPHID()); + + if (!$relationship) { + $relationship = new PhabricatorOwnersPackageCommitRelationship(); + $relationship->setPackagePHID($package->getPHID()); + $relationship->setCommitPHID($commit->getPHID()); + $relationship->save(); + } + } + } +} diff --git a/src/applications/repository/worker/owner/__init__.php b/src/applications/repository/worker/owner/__init__.php new file mode 100644 index 0000000000..4a7581511b --- /dev/null +++ b/src/applications/repository/worker/owner/__init__.php @@ -0,0 +1,17 @@ +