From ecfb72071372b3abc9205e7b725c08a4d7ec42de Mon Sep 17 00:00:00 2001 From: Anh Nhan Nguyen Date: Fri, 22 Mar 2013 09:46:21 -0700 Subject: [PATCH] Adding some filters and queries to Macro application Summary: Fixes T2778 Introduces `PhabricatorMacroQuery`, which should consolidate all queries regarding macros Adds `PolicyInterface` to `PhabricatorImageMacro`, as else the query would fail (we should consider adding it to the ApplicationTransaction instead, if that was ever planned) Adds `Active Macros` filter, making it the default Adds `My Macros` filter. You may ask why it overwrites `$authors`. Well, I did not want the page jump to the conclusion that it is a search result. It //is// one more or less, but the filter would jump to `seach` instead of `my`. If you want `My Macros` removed, tell me. It is useful only to heavy-macro-uploaders-and-users though. Five or six people in `http://secure.phabricator.(org|com)`, and an estimated dozen and a half at bigger installs. Test Plan: created multiple macros from multiple authors, disabled them at will. browsed around, verified that Macros only appeared in the right filters and that nothing else broke. Reviewers: epriestley, chad, btrahan CC: aran, Korvin Maniphest Tasks: T2778 Differential Revision: https://secure.phabricator.com/D5409 --- src/__phutil_library_map__.php | 3 + .../PhabricatorApplicationMacro.php | 2 +- .../controller/PhabricatorMacroController.php | 4 +- .../PhabricatorMacroListController.php | 90 ++++++----- .../macro/query/PhabricatorMacroQuery.php | 143 ++++++++++++++++++ .../storage/PhabricatorFileImageMacro.php | 33 +++- 6 files changed, 225 insertions(+), 50 deletions(-) create mode 100644 src/applications/macro/query/PhabricatorMacroQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index be2b2af57c..773a6cb8ef 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1032,6 +1032,7 @@ phutil_register_library_map(array( 'PhabricatorMacroListController' => 'applications/macro/controller/PhabricatorMacroListController.php', 'PhabricatorMacroMemeController' => 'applications/macro/controller/PhabricatorMacroMemeController.php', 'PhabricatorMacroMemeDialogController' => 'applications/macro/controller/PhabricatorMacroMemeDialogController.php', + 'PhabricatorMacroQuery' => 'applications/macro/query/PhabricatorMacroQuery.php', 'PhabricatorMacroReplyHandler' => 'applications/macro/mail/PhabricatorMacroReplyHandler.php', 'PhabricatorMacroTransaction' => 'applications/macro/storage/PhabricatorMacroTransaction.php', 'PhabricatorMacroTransactionComment' => 'applications/macro/storage/PhabricatorMacroTransactionComment.php', @@ -2588,6 +2589,7 @@ phutil_register_library_map(array( 0 => 'PhabricatorFileDAO', 1 => 'PhabricatorSubscribableInterface', 2 => 'PhabricatorApplicationTransactionInterface', + 3 => 'PhabricatorPolicyInterface', ), 'PhabricatorFileInfoController' => 'PhabricatorFileController', 'PhabricatorFileLinkListView' => 'AphrontView', @@ -2658,6 +2660,7 @@ phutil_register_library_map(array( 'PhabricatorMacroListController' => 'PhabricatorMacroController', 'PhabricatorMacroMemeController' => 'PhabricatorMacroController', 'PhabricatorMacroMemeDialogController' => 'PhabricatorMacroController', + 'PhabricatorMacroQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorMacroReplyHandler' => 'PhabricatorMailReplyHandler', 'PhabricatorMacroTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorMacroTransactionComment' => 'PhabricatorApplicationTransactionComment', diff --git a/src/applications/macro/application/PhabricatorApplicationMacro.php b/src/applications/macro/application/PhabricatorApplicationMacro.php index 1dc24aae8a..eb6a23ccc4 100644 --- a/src/applications/macro/application/PhabricatorApplicationMacro.php +++ b/src/applications/macro/application/PhabricatorApplicationMacro.php @@ -25,7 +25,7 @@ final class PhabricatorApplicationMacro extends PhabricatorApplication { public function getRoutes() { return array( '/macro/' => array( - '' => 'PhabricatorMacroListController', + '((?Pall|active|my)/)?' => 'PhabricatorMacroListController', 'create/' => 'PhabricatorMacroEditController', 'view/(?P[1-9]\d*)/' => 'PhabricatorMacroViewController', 'comment/(?P[1-9]\d*)/' => 'PhabricatorMacroCommentController', diff --git a/src/applications/macro/controller/PhabricatorMacroController.php b/src/applications/macro/controller/PhabricatorMacroController.php index 7df1e98b4d..0a4ac3ccf4 100644 --- a/src/applications/macro/controller/PhabricatorMacroController.php +++ b/src/applications/macro/controller/PhabricatorMacroController.php @@ -15,7 +15,9 @@ abstract class PhabricatorMacroController } $nav->addLabel(pht('Macros')); - $nav->addFilter('/', pht('All Macros')); + $nav->addFilter('active', pht('Active Macros')); + $nav->addFilter('all', pht('All Macros')); + $nav->addFilter('my', pht('My Macros')); if ($has_search) { $nav->addFilter('search', pht('Search'), diff --git a/src/applications/macro/controller/PhabricatorMacroListController.php b/src/applications/macro/controller/PhabricatorMacroListController.php index 5ac2a388dd..ef3dd75d33 100644 --- a/src/applications/macro/controller/PhabricatorMacroListController.php +++ b/src/applications/macro/controller/PhabricatorMacroListController.php @@ -3,6 +3,12 @@ final class PhabricatorMacroListController extends PhabricatorMacroController { + private $filter; + + public function willProcessRequest(array $data) { + $this->filter = idx($data, 'filter', 'active'); + } + public function processRequest() { $request = $this->getRequest(); @@ -12,55 +18,39 @@ final class PhabricatorMacroListController $file_table = new PhabricatorFile(); $conn = $macro_table->establishConnection('r'); - $where = array(); + $pager = id(new AphrontCursorPagerView()) + ->readFromRequest($request); - $join = array(); - $join[] = qsprintf($conn, '%T m', $macro_table->getTableName()); + $query = new PhabricatorMacroQuery(); + $query->setViewer($viewer); $filter = $request->getStr('name'); if (strlen($filter)) { - $where[] = qsprintf($conn, 'm.name LIKE %~', $filter); + $query->withNameLike($filter); } $authors = $request->getArr('authors'); + if ($authors) { - $join[] = qsprintf( - $conn, - '%T f ON m.filePHID = f.phid', - $file_table->getTableName()); - $where[] = qsprintf($conn, 'f.authorPHID IN (%Ls)', $authors); + $query->withAuthorPHIDs($authors); } - $has_search = $where; + $has_search = $filter || $authors; + if ($this->filter == 'my') { + $query->withAuthorPHIDs(array($viewer->getPHID())); + // For pre-filling the tokenizer + $authors = array($viewer->getPHID()); + } + + if ($this->filter == 'active') { + $query->withStatus(PhabricatorMacroQuery::STATUS_ACTIVE); + } + + $macros = $query->executeWithCursorPager($pager); if ($has_search) { - $macros = queryfx_all( - $conn, - 'SELECT m.* FROM %Q WHERE %Q', - implode(' JOIN ', $join), - implode(' AND ', $where)); - $macros = $macro_table->loadAllFromArray($macros); $nodata = pht('There are no macros matching the filter.'); } else { - $pager = new AphrontPagerView(); - $pager->setOffset($request->getInt('page')); - - $macros = $macro_table->loadAllWhere( - '1 = 1 ORDER BY id DESC LIMIT %d, %d', - $pager->getOffset(), - $pager->getPageSize()); - - // Get an exact count since the size here is reasonably going to be a few - // thousand at most in any reasonable case. - $count = queryfx_one( - $conn, - 'SELECT COUNT(*) N FROM %T', - $macro_table->getTableName()); - $count = $count['N']; - - $pager->setCount($count); - $pager->setURI($request->getRequestURI(), 'page'); - $nodata = pht('There are no image macros yet.'); } @@ -70,16 +60,10 @@ final class PhabricatorMacroListController $author_phids = array(); } - $file_phids = mpull($macros, 'getFilePHID'); - - $files = array(); - if ($file_phids) { - $files = $file_table->loadAllWhere( - "phid IN (%Ls)", - $file_phids); + $files = mpull($macros, 'getFile'); + if ($files) { $author_phids += mpull($files, 'getAuthorPHID', 'getAuthorPHID'); } - $files_map = mpull($files, null, 'getPHID'); $this->loadHandles($author_phids); $author_handles = array_select_keys($this->getLoadedHandles(), $authors); @@ -108,15 +92,14 @@ final class PhabricatorMacroListController $nav = $this->buildSideNavView( $for_app = false, $has_search); - $nav->selectFilter($has_search ? 'search' : '/'); + $nav->selectFilter($has_search ? 'search' : $this->filter); $nav->appendChild($filter_view); $pinboard = new PhabricatorPinboardView(); $pinboard->setNoDataString($nodata); foreach ($macros as $macro) { - $file_phid = $macro->getFilePHID(); - $file = idx($files_map, $file_phid); + $file = $macro->getFile(); $item = new PhabricatorPinboardItemView(); if ($file) { @@ -143,7 +126,20 @@ final class PhabricatorMacroListController if (!$has_search) { $nav->appendChild($pager); - $name = pht('All Macros'); + switch ($this->filter) { + case 'all': + $name = pht('All Macros'); + break; + case 'my': + $name = pht('My Macros'); + break; + case 'active': + $name = pht('Active Macros'); + break; + default: + throw new Exception("Unknown filter $this->filter"); + break; + } } else { $name = pht('Search'); } diff --git a/src/applications/macro/query/PhabricatorMacroQuery.php b/src/applications/macro/query/PhabricatorMacroQuery.php new file mode 100644 index 0000000000..45d24afbd7 --- /dev/null +++ b/src/applications/macro/query/PhabricatorMacroQuery.php @@ -0,0 +1,143 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withAuthorPHIDs(array $authors) { + $this->authors = $authors; + return $this; + } + + public function withNameLike($name) { + $this->name = $name; + return $this; + } + + public function withStatus($status) { + $this->status = $status; + return $this; + } + + protected function loadPage() { + $macro_table = new PhabricatorFileImageMacro(); + $conn = $macro_table->establishConnection('r'); + + $rows = queryfx_all( + $conn, + 'SELECT * FROM %T m %Q %Q %Q %Q', + $macro_table->getTableName(), + $this->buildJoinClause($conn), + $this->buildWhereClause($conn), + $this->buildOrderClause($conn), + $this->buildLimitClause($conn)); + + return $macro_table->loadAllFromArray($rows); + } + + protected function buildJoinClause(AphrontDatabaseConnection $conn) { + $joins = array(); + + if ($this->authors) { + $file_table = new PhabricatorFile(); + $joins[] = qsprintf( + $conn, + 'JOIN %T f ON m.filePHID = f.phid', + $file_table->getTableName()); + } + + return implode(' ', $joins); + } + + protected function buildWhereClause(AphrontDatabaseConnection $conn) { + $where = array(); + + if ($this->ids) { + $where[] = qsprintf( + $conn, + 'm.id IN (%Ld)', + $this->ids); + } + + if ($this->phids) { + $where[] = qsprintf( + $conn, + 'm.phid IN (%Ld)', + $this->phids); + } + + if ($this->authors) { + $where[] = qsprintf( + $conn, + 'f.authorPHID IN (%Ls)', + $this->authors); + } + + if ($this->name) { + $where[] = qsprintf( + $conn, + 'm.name LIKE %~', + $this->name); + } + + if ($this->status == self::STATUS_ACTIVE) { + $where[] = qsprintf( + $conn, + 'm.isDisabled = 0'); + } + + $where[] = $this->buildPagingClause($conn); + + return $this->formatWhereClause($where); + } + + protected function willFilterPage(array $macros) { + if (!$macros) { + return array(); + } + + $file_phids = mpull($macros, 'getFilePHID'); + $files = id(new PhabricatorFileQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($file_phids) + ->execute(); + $files = mpull($files, null, 'getPHID'); + + foreach ($macros as $key => $macro) { + $file = idx($files, $macro->getFilePHID()); + if (!$file) { + unset($macros[$key]); + continue; + } + $macro->attachFile($file); + } + + return $macros; + } + + protected function getPagingColumn() { + return 'm.id'; + } + +} diff --git a/src/applications/macro/storage/PhabricatorFileImageMacro.php b/src/applications/macro/storage/PhabricatorFileImageMacro.php index 470d1a7ab4..9a46b838d0 100644 --- a/src/applications/macro/storage/PhabricatorFileImageMacro.php +++ b/src/applications/macro/storage/PhabricatorFileImageMacro.php @@ -3,13 +3,29 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO implements PhabricatorSubscribableInterface, - PhabricatorApplicationTransactionInterface { + PhabricatorApplicationTransactionInterface, + PhabricatorPolicyInterface { protected $filePHID; protected $phid; protected $name; protected $isDisabled = 0; + private $file; + + public function attachFile(PhabricatorFile $file) { + $this->file = $file; + return $this; + } + + public function getFile() { + if (!$this->file) { + throw new Exception("Attach a file with attachFile() first!"); + } + + return $this->file; + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -33,5 +49,20 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO return new PhabricatorMacroTransaction(); } + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + public function getPolicy($capability) { + return PhabricatorPolicies::POLICY_USER; + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + }