From 01d0fc443afea39e573226c42fbf81f10f4f0f37 Mon Sep 17 00:00:00 2001 From: Ariel Yang Date: Sun, 24 Feb 2019 13:37:14 +0000 Subject: [PATCH 01/43] Fix a typo Summary: Fix a type Test Plan: No need. Reviewers: #blessed_reviewers, amckinley Reviewed By: #blessed_reviewers, amckinley Subscribers: amckinley, epriestley Differential Revision: https://secure.phabricator.com/D20203 --- src/applications/auth/engine/PhabricatorAuthInviteEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/auth/engine/PhabricatorAuthInviteEngine.php b/src/applications/auth/engine/PhabricatorAuthInviteEngine.php index f1cb45483e..70fc03345c 100644 --- a/src/applications/auth/engine/PhabricatorAuthInviteEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthInviteEngine.php @@ -147,7 +147,7 @@ final class PhabricatorAuthInviteEngine extends Phobject { // no address. Users can use password recovery to access the other // account if they really control the address. throw id(new PhabricatorAuthInviteAccountException( - pht('Wrong Acount'), + pht('Wrong Account'), pht( 'You are logged in as %s, but the email address you just '. 'clicked a link from is already the primary email address '. From 66161feb13c47fa93f5e9d812a66c676e497177c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 04:50:37 -0800 Subject: [PATCH 02/43] Fix a URI construction exception when filtering the Maniphest Burnup chart by project Summary: See . Test Plan: Viewed Maniphest burnup chart, filtered by project: no more URI construction exception. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20207 --- .../controller/ManiphestReportController.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index 77bd6c0d59..40498e6e40 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -13,10 +13,19 @@ final class ManiphestReportController extends ManiphestController { $project = head($request->getArr('set_project')); $project = nonempty($project, null); - $uri = $uri->alter('project', $project); + + if ($project !== null) { + $uri->replaceQueryParam('project', $project); + } else { + $uri->removeQueryParam('project'); + } $window = $request->getStr('set_window'); - $uri = $uri->alter('window', $window); + if ($window !== null) { + $uri->replaceQueryParam('window', $window); + } else { + $uri->removeQueryParam('window'); + } return id(new AphrontRedirectResponse())->setURI($uri); } From 767afd1780fd92829073754cc334ebbcc156515d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 04:58:57 -0800 Subject: [PATCH 03/43] Support an "authorPHIDs" constraint for "transaction.search" Summary: Ref T13255. The "transaction.search" API method currently does not support author constraints, but this is a reasonable thing to support. Test Plan: Queried transactions by author, hit the error cases. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13255 Differential Revision: https://secure.phabricator.com/D20208 --- .../TransactionSearchConduitAPIMethod.php | 59 +++++++++++++------ 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 0edc0b3f5a..0614f1d4ed 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -73,24 +73,8 @@ final class TransactionSearchConduitAPIMethod ->setViewer($viewer); $constraints = $request->getValue('constraints', array()); - PhutilTypeSpec::checkMap( - $constraints, - array( - 'phids' => 'optional list', - )); - $with_phids = idx($constraints, 'phids'); - - if ($with_phids === array()) { - throw new Exception( - pht( - 'Constraint "phids" to "transaction.search" requires nonempty list, '. - 'empty list provided.')); - } - - if ($with_phids) { - $xaction_query->withPHIDs($with_phids); - } + $xaction_query = $this->applyConstraints($constraints, $xaction_query); $xactions = $xaction_query->executeWithCursorPager($pager); @@ -240,4 +224,45 @@ final class TransactionSearchConduitAPIMethod return $this->addPagerResults($results, $pager); } + + private function applyConstraints( + array $constraints, + PhabricatorApplicationTransactionQuery $query) { + + PhutilTypeSpec::checkMap( + $constraints, + array( + 'phids' => 'optional list', + 'authorPHIDs' => 'optional list', + )); + + $with_phids = idx($constraints, 'phids'); + + if ($with_phids === array()) { + throw new Exception( + pht( + 'Constraint "phids" to "transaction.search" requires nonempty list, '. + 'empty list provided.')); + } + + if ($with_phids) { + $query->withPHIDs($with_phids); + } + + $with_authors = idx($constraints, 'authorPHIDs'); + if ($with_authors === array()) { + throw new Exception( + pht( + 'Constraint "authorPHIDs" to "transaction.search" requires '. + 'nonempty list, empty list provided.')); + } + + if ($with_authors) { + $query->withAuthorPHIDs($with_authors); + } + + return $query; + } + + } From f61e825905c1b30a105e74b1c22f290cc3fca0e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 06:18:53 -0800 Subject: [PATCH 04/43] Make the Diffusion warning about "svnlook" and PATH more clear Summary: See for discussion. The UI currently shows a misleading warning that looks like "found svnlook; can't find svnlook". It actually means "found svnlook, but when Subversion wipes PATH before executing commit hooks, we will no longer be able to find it". Test Plan: {F6240967} Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20210 --- .../DiffusionRepositoryBasicsManagementPanel.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php index bcf3ad6d74..d585c5774d 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php @@ -444,13 +444,15 @@ final class DiffusionRepositoryBasicsManagementPanel id(new PHUIStatusItemView()) ->setIcon(PHUIStatusItemView::ICON_WARNING, 'red') ->setTarget( - pht('Missing Binary %s', phutil_tag('tt', array(), $binary))) - ->setNote(pht( - 'Unable to find this binary in `%s`. '. - 'You need to configure %s and include %s.', - 'environment.append-paths', - $this->getEnvConfigLink(), - $path))); + pht('Commit Hooks: %s', phutil_tag('tt', array(), $binary))) + ->setNote( + pht( + 'The directory containing the "svnlook" binary is not '. + 'listed in "environment.append-paths", so commit hooks '. + '(which execute with an empty "PATH") will not be able to '. + 'find "svnlook". Add `%s` to %s.', + $path, + $this->getEnvConfigLink()))); } } } From 83aba7b01cb155d483c3566aa4997c4c996b6745 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 05:32:21 -0800 Subject: [PATCH 05/43] Enrich the "change project tags" transaction in "transaction.search" Summary: Depends on D20208. Ref T13255. See that task for some long-winded discussion and rationale. Short version: - This is a list of operations instead of a list of old/new PHIDs because of scalability issues for large lists (T13056). - This is a fairly verbose list (instead of, for example, the more concise internal map we sometimes use with "+" and "-" as keys) to try to make the structure obvious and extensible in the future. - The "add" and "remove" echo the `*.edit` operations. Test Plan: Called `transaction.search` on an object with project tag changes, saw them in the results. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13255 Differential Revision: https://secure.phabricator.com/D20209 --- .../TransactionSearchConduitAPIMethod.php | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 0614f1d4ed..362b490a42 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -202,6 +202,14 @@ final class TransactionSearchConduitAPIMethod case PhabricatorTransactions::TYPE_CREATE: $type = 'create'; break; + case PhabricatorTransactions::TYPE_EDGE: + switch ($xaction->getMetadataValue('edge:type')) { + case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: + $type = 'projects'; + $fields = $this->newEdgeTransactionFields($xaction); + break; + } + break; } } @@ -264,5 +272,29 @@ final class TransactionSearchConduitAPIMethod return $query; } + private function newEdgeTransactionFields( + PhabricatorApplicationTransaction $xaction) { + + $record = PhabricatorEdgeChangeRecord::newFromTransaction($xaction); + + $operations = array(); + foreach ($record->getAddedPHIDs() as $phid) { + $operations[] = array( + 'operation' => 'add', + 'phid' => $phid, + ); + } + + foreach ($record->getRemovedPHIDs() as $phid) { + $operations[] = array( + 'operation' => 'remove', + 'phid' => $phid, + ); + } + + return array( + 'operations' => $operations, + ); + } } From d1546209c53613163baa72912ad02fa92a683626 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 07:10:29 -0800 Subject: [PATCH 06/43] Expand documentation for "transaction.search" Summary: Depends on D20209. Ref T13255. It would probably be nice to make this into a "real" `*.search` API method some day, but at least document the features for now. Test Plan: Read documentation. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13255 Differential Revision: https://secure.phabricator.com/D20211 --- .../conduit/method/ConduitAPIMethod.php | 15 +++++ .../PhabricatorSearchEngineAPIMethod.php | 30 +++------- .../TransactionSearchConduitAPIMethod.php | 55 +++++++++++++++++-- 3 files changed, 72 insertions(+), 28 deletions(-) diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php index 05831a782d..0fbfaa2fc3 100644 --- a/src/applications/conduit/method/ConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitAPIMethod.php @@ -409,4 +409,19 @@ abstract class ConduitAPIMethod $capability); } + final protected function newRemarkupDocumentationView($remarkup) { + $viewer = $this->getViewer(); + + $view = new PHUIRemarkupView($viewer, $remarkup); + + $view->setRemarkupOptions( + array( + PHUIRemarkupView::OPTION_PRESERVE_LINEBREAKS => false, + )); + + return id(new PHUIBoxView()) + ->appendChild($view) + ->addPadding(PHUI::PADDING_LARGE); + } + } diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 235d74d6f3..510ad91864 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -144,7 +144,7 @@ EOTEXT ->setHeaderText(pht('Builtin and Saved Queries')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($info)) + ->appendChild($this->newRemarkupDocumentationView($info)) ->appendChild($table); } @@ -223,7 +223,7 @@ EOTEXT ); if ($constants) { - $constant_lists[] = $this->buildRemarkup( + $constant_lists[] = $this->newRemarkupDocumentationView( pht( 'Constants supported by the `%s` constraint:', 'statuses')); @@ -283,7 +283,7 @@ EOTEXT ->setHeaderText(pht('Custom Query Constraints')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($info)) + ->appendChild($this->newRemarkupDocumentationView($info)) ->appendChild($table) ->appendChild($constant_lists); } @@ -391,9 +391,9 @@ EOTEXT ->setHeaderText(pht('Result Ordering')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($orders_info)) + ->appendChild($this->newRemarkupDocumentationView($orders_info)) ->appendChild($orders_table) - ->appendChild($this->buildRemarkup($columns_info)) + ->appendChild($this->newRemarkupDocumentationView($columns_info)) ->appendChild($columns_table); } @@ -472,7 +472,7 @@ EOTEXT ->setHeaderText(pht('Object Fields')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($info)) + ->appendChild($this->newRemarkupDocumentationView($info)) ->appendChild($table); } @@ -562,7 +562,7 @@ EOTEXT ->setHeaderText(pht('Attachments')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($info)) + ->appendChild($this->newRemarkupDocumentationView($info)) ->appendChild($table); } @@ -633,21 +633,7 @@ EOTEXT ->setHeaderText(pht('Paging and Limits')) ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($this->buildRemarkup($info)); + ->appendChild($this->newRemarkupDocumentationView($info)); } - private function buildRemarkup($remarkup) { - $viewer = $this->getViewer(); - - $view = new PHUIRemarkupView($viewer, $remarkup); - - $view->setRemarkupOptions( - array( - PHUIRemarkupView::OPTION_PRESERVE_LINEBREAKS => false, - )); - - return id(new PHUIBoxView()) - ->appendChild($view) - ->addPadding(PHUI::PADDING_LARGE); - } } diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 362b490a42..4ab5de519e 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -8,15 +8,58 @@ final class TransactionSearchConduitAPIMethod } public function getMethodDescription() { - return pht('Read transactions for an object.'); + return pht('Read transactions and comments for an object.'); } - public function getMethodStatus() { - return self::METHOD_STATUS_UNSTABLE; - } + public function getMethodDocumentation() { + $markup = pht(<<.// Find specific transactions by PHID. This + is most likely to be useful if you're responding to a webhook notification + and want to inspect only the related events. + - `authorPHIDs` //Optional list.// Find transactions with particular + authors. + +Transaction Format +================== + +Each transaction has custom data describing what the transaction did. The +format varies from transaction to transaction. The easiest way to figure out +exactly what a particular transaction looks like is to make the associated kind +of edit to a test object, then query that object. + +Not all transactions have data: by default, transactions have a `null` "type" +and no additional data. This API does not expose raw transaction data because +some of it is internal, oddly named, misspelled, confusing, not useful, or +could create security or policy problems to expose directly. + +New transactions are exposed (with correctly spelled, comprehensible types and +useful, reasonable fields) as we become aware of use cases for them. + +EOREMARKUP + ); + + $markup = $this->newRemarkupDocumentationView($markup); + + return id(new PHUIObjectBoxView()) + ->setCollapsed(true) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setHeaderText(pht('Method Details')) + ->appendChild($markup); } protected function defineParamTypes() { From dc9aaa0fc2bf41ed01e4beeeca98c442c486fe8e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 12:41:57 -0800 Subject: [PATCH 07/43] Fix a stray "%Q" warning when hiding/showing inline comments Summary: See PHI1095. Test Plan: Viewed a revision, toggled hide/show on inline comments. Before: warning in logs; after: smooth sailing. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20212 --- .../controller/DifferentialInlineCommentEditController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 9741cc93ee..1de156a9b7 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -204,9 +204,9 @@ final class DifferentialInlineCommentEditController queryfx( $conn_w, - 'INSERT IGNORE INTO %T (userPHID, commentID) VALUES %Q', + 'INSERT IGNORE INTO %T (userPHID, commentID) VALUES %LQ', $table->getTableName(), - implode(', ', $sql)); + $sql); } protected function showComments(array $ids) { From 814e6d2de927a7b2137d7198e9377382ee8d497e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 19:13:55 -0800 Subject: [PATCH 08/43] Add more type checking to transactions queued by Herald Summary: See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code. Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of. Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20214 --- .../editor/DifferentialDiffEditor.php | 9 -------- .../herald/adapter/HeraldAdapter.php | 21 +++++++++++++++---- ...habricatorApplicationTransactionEditor.php | 9 ++++++-- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php index 261f6f1598..e78e08d808 100644 --- a/src/applications/differential/editor/DifferentialDiffEditor.php +++ b/src/applications/differential/editor/DifferentialDiffEditor.php @@ -208,15 +208,6 @@ final class DifferentialDiffEditor return $adapter; } - protected function didApplyHeraldRules( - PhabricatorLiskDAO $object, - HeraldAdapter $adapter, - HeraldTranscript $transcript) { - - $xactions = array(); - return $xactions; - } - private function updateDiffFromDict(DifferentialDiff $diff, $dict) { $diff ->setSourcePath(idx($dict, 'sourcePath')) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index a266e21f39..69f538afcc 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -186,15 +186,16 @@ abstract class HeraldAdapter extends Phobject { return $this->appliedTransactions; } - public function queueTransaction($transaction) { + final public function queueTransaction( + PhabricatorApplicationTransaction $transaction) { $this->queuedTransactions[] = $transaction; } - public function getQueuedTransactions() { + final public function getQueuedTransactions() { return $this->queuedTransactions; } - public function newTransaction() { + final public function newTransaction() { $object = $this->newObject(); if (!($object instanceof PhabricatorApplicationTransactionInterface)) { @@ -205,7 +206,19 @@ abstract class HeraldAdapter extends Phobject { 'PhabricatorApplicationTransactionInterface')); } - return $object->getApplicationTransactionTemplate(); + $xaction = $object->getApplicationTransactionTemplate(); + + if (!($xaction instanceof PhabricatorApplicationTransaction)) { + throw new Exception( + pht( + 'Expected object (of class "%s") to return a transaction template '. + '(of class "%s"), but it returned something else ("%s").', + get_class($object), + 'PhabricatorApplicationTransaction', + phutil_describe_type($xaction))); + } + + return $xaction; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 9460dd3030..3a46784a33 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3779,9 +3779,14 @@ abstract class PhabricatorApplicationTransactionEditor $this->mustEncrypt = $adapter->getMustEncryptReasons(); + $apply_xactions = $this->didApplyHeraldRules($object, $adapter, $xscript); + assert_instances_of($apply_xactions, 'PhabricatorApplicationTransaction'); + + $queue_xactions = $adapter->getQueuedTransactions(); + return array_merge( - $this->didApplyHeraldRules($object, $adapter, $xscript), - $adapter->getQueuedTransactions()); + array_values($apply_xactions), + array_values($queue_xactions)); } protected function didApplyHeraldRules( From b28b05342bccb9ecb3fbbfff9a29b78e4393554e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Feb 2019 20:02:10 -0800 Subject: [PATCH 09/43] Simplify one "array_keys/range" -> "phutil_is_natural_list()" in "phabricator/" Summary: Depends on D20213. Simplify this idiom. Test Plan: Squinted hard; `grep array_keys | grep range`. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20215 --- src/applications/config/type/PhabricatorSetConfigType.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/config/type/PhabricatorSetConfigType.php b/src/applications/config/type/PhabricatorSetConfigType.php index 805ae50468..553ee614b8 100644 --- a/src/applications/config/type/PhabricatorSetConfigType.php +++ b/src/applications/config/type/PhabricatorSetConfigType.php @@ -43,7 +43,7 @@ final class PhabricatorSetConfigType } if ($value) { - if (array_keys($value) !== range(0, count($value) - 1)) { + if (!phutil_is_natural_list($value)) { throw $this->newException( pht( 'Option "%s" is of type "%s", and should be specified on the '. From 41c03bab39575cae180bf3f5cd3da80c3a02ff58 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Feb 2019 04:32:23 -0800 Subject: [PATCH 10/43] Remove unusual "Created" element from Build Plan curtain UI Summary: Ref T13088. Build Plans currently have a "Created" date in the right-hand "Curtain" UI, but this is unusual and the creation date is evident from the timeline. It's also not obvious why anyone would care. Remove it for simplicity/consistency. I think this may have just been a placeholder during initial implementation. Test Plan: Viewed a build plan, no more "Created" element. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D20216 --- .../controller/HarbormasterPlanViewController.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 6ebadf7a62..f91c18c5b6 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -263,11 +263,6 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setDisabled(!$can_run) ->setIcon('fa-play-circle')); - $curtain->addPanel( - id(new PHUICurtainPanelView()) - ->setHeaderText(pht('Created')) - ->appendChild(phabricator_datetime($plan->getDateCreated(), $viewer))); - return $curtain; } From f6ed873f1728269dc0613b02237c81f3ce18c0e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Feb 2019 05:11:46 -0800 Subject: [PATCH 11/43] Move Harbormaster Build Plans to modular transactions Summary: Depends on D20216. Ref T13258. Bland infrastructure update to prepare for bigger things. Test Plan: Created and edited a build plan. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20217 --- src/__phutil_library_map__.php | 8 +- .../HarbormasterPlanDisableController.php | 4 +- .../HarbormasterBuildPlanEditEngine.php | 3 +- .../editor/HarbormasterBuildPlanEditor.php | 93 ++----------------- .../configuration/HarbormasterBuildPlan.php | 10 ++ .../HarbormasterBuildPlanTransaction.php | 68 +------------- .../HarbormasterBuildPlanNameTransaction.php | 46 +++++++++ ...HarbormasterBuildPlanStatusTransaction.php | 67 +++++++++++++ .../HarbormasterBuildPlanTransactionType.php | 4 + 9 files changed, 149 insertions(+), 154 deletions(-) create mode 100644 src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php create mode 100644 src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanStatusTransaction.php create mode 100644 src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f4ee380cc0..d4696bde60 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1334,12 +1334,15 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanEditEngine' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php', 'HarbormasterBuildPlanEditor' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditor.php', 'HarbormasterBuildPlanNameNgrams' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanNameNgrams.php', + 'HarbormasterBuildPlanNameTransaction' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php', 'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php', 'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php', 'HarbormasterBuildPlanSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php', 'HarbormasterBuildPlanSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php', + 'HarbormasterBuildPlanStatusTransaction' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanStatusTransaction.php', 'HarbormasterBuildPlanTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php', 'HarbormasterBuildPlanTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanTransactionQuery.php', + 'HarbormasterBuildPlanTransactionType' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanTransactionType.php', 'HarbormasterBuildQuery' => 'applications/harbormaster/query/HarbormasterBuildQuery.php', 'HarbormasterBuildRequest' => 'applications/harbormaster/engine/HarbormasterBuildRequest.php', 'HarbormasterBuildSearchConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildSearchConduitAPIMethod.php', @@ -6943,12 +6946,15 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanEditEngine' => 'PhabricatorEditEngine', 'HarbormasterBuildPlanEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildPlanNameNgrams' => 'PhabricatorSearchNgrams', + 'HarbormasterBuildPlanNameTransaction' => 'HarbormasterBuildPlanTransactionType', 'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildPlanSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'HarbormasterBuildPlanSearchEngine' => 'PhabricatorApplicationSearchEngine', - 'HarbormasterBuildPlanTransaction' => 'PhabricatorApplicationTransaction', + 'HarbormasterBuildPlanStatusTransaction' => 'HarbormasterBuildPlanTransactionType', + 'HarbormasterBuildPlanTransaction' => 'PhabricatorModularTransaction', 'HarbormasterBuildPlanTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'HarbormasterBuildPlanTransactionType' => 'PhabricatorModularTransactionType', 'HarbormasterBuildQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildRequest' => 'Phobject', 'HarbormasterBuildSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', diff --git a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php index ccf6b8986f..65a993396d 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php @@ -19,11 +19,11 @@ final class HarbormasterPlanDisableController return new Aphront404Response(); } - $plan_uri = $this->getApplicationURI('plan/'.$plan->getID().'/'); + $plan_uri = $plan->getURI(); if ($request->isFormPost()) { - $type_status = HarbormasterBuildPlanTransaction::TYPE_STATUS; + $type_status = HarbormasterBuildPlanStatusTransaction::TRANSACTIONTYPE; $v_status = $plan->isDisabled() ? HarbormasterBuildPlan::STATUS_ACTIVE diff --git a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php index 11837051c3..35a417d9d4 100644 --- a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php +++ b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php @@ -82,7 +82,8 @@ final class HarbormasterBuildPlanEditEngine ->setKey('name') ->setLabel(pht('Name')) ->setIsRequired(true) - ->setTransactionType(HarbormasterBuildPlanTransaction::TYPE_NAME) + ->setTransactionType( + HarbormasterBuildPlanNameTransaction::TRANSACTIONTYPE) ->setDescription(pht('The build plan name.')) ->setConduitDescription(pht('Rename the plan.')) ->setConduitTypeDescription(pht('New plan name.')) diff --git a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php index 71c9283ade..1b340b6524 100644 --- a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php +++ b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php @@ -11,100 +11,23 @@ final class HarbormasterBuildPlanEditor return pht('Harbormaster Build Plans'); } + public function getCreateObjectTitle($author, $object) { + return pht('%s created this build plan.', $author); + } + + public function getCreateObjectTitleForFeed($author, $object) { + return pht('%s created %s.', $author, $object); + } + protected function supportsSearch() { return true; } public function getTransactionTypes() { $types = parent::getTransactionTypes(); - $types[] = HarbormasterBuildPlanTransaction::TYPE_NAME; - $types[] = HarbormasterBuildPlanTransaction::TYPE_STATUS; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; return $types; } - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HarbormasterBuildPlanTransaction::TYPE_NAME: - if ($this->getIsNewObject()) { - return null; - } - return $object->getName(); - case HarbormasterBuildPlanTransaction::TYPE_STATUS: - return $object->getPlanStatus(); - } - - return parent::getCustomTransactionOldValue($object, $xaction); - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HarbormasterBuildPlanTransaction::TYPE_NAME: - return $xaction->getNewValue(); - case HarbormasterBuildPlanTransaction::TYPE_STATUS: - return $xaction->getNewValue(); - } - return parent::getCustomTransactionNewValue($object, $xaction); - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HarbormasterBuildPlanTransaction::TYPE_NAME: - $object->setName($xaction->getNewValue()); - return; - case HarbormasterBuildPlanTransaction::TYPE_STATUS: - $object->setPlanStatus($xaction->getNewValue()); - return; - } - return parent::applyCustomInternalTransaction($object, $xaction); - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HarbormasterBuildPlanTransaction::TYPE_NAME: - case HarbormasterBuildPlanTransaction::TYPE_STATUS: - return; - } - return parent::applyCustomExternalTransaction($object, $xaction); - } - - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - switch ($type) { - case HarbormasterBuildPlanTransaction::TYPE_NAME: - $missing = $this->validateIsEmptyTextField( - $object->getName(), - $xactions); - - if ($missing) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('You must choose a name for your build plan.'), - last($xactions)); - - $error->setIsMissingFieldError(true); - $errors[] = $error; - } - break; - } - - return $errors; - } - - } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index 2e379aab23..c177c2e7b7 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -84,6 +84,16 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return ($this->getPlanStatus() == self::STATUS_DISABLED); } + public function getURI() { + return urisprintf( + '/harbormaster/plan/%s/', + $this->getID()); + } + + public function getObjectName() { + return pht('Build Plan %d', $this->getID()); + } + /* -( Autoplans )---------------------------------------------------------- */ diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php index 130471e21b..6cd286343a 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php @@ -1,10 +1,7 @@ getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - if ($old === null) { - return 'fa-plus'; - } - break; - } - - return parent::getIcon(); - } - - public function getColor() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - if ($old === null) { - return 'green'; - } - break; - } - - return parent::getIcon(); - } - - public function getTitle() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - $author_handle = $this->renderHandleLink($this->getAuthorPHID()); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - if ($old === null) { - return pht( - '%s created this build plan.', - $author_handle); - } else { - return pht( - '%s renamed this build plan from "%s" to "%s".', - $author_handle, - $old, - $new); - } - case self::TYPE_STATUS: - if ($new == HarbormasterBuildPlan::STATUS_DISABLED) { - return pht( - '%s disabled this build plan.', - $author_handle); - } else { - return pht( - '%s enabled this build plan.', - $author_handle); - } - } - - return parent::getTitle(); + public function getBaseTransactionClass() { + return 'HarbormasterBuildPlanTransactionType'; } } diff --git a/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php new file mode 100644 index 0000000000..30fdbe72ca --- /dev/null +++ b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php @@ -0,0 +1,46 @@ +getName(); + } + + public function applyInternalEffects($object, $value) { + $object->setName($value); + } + + public function getTitle() { + return pht( + '%s renamed this build plan from "%s" to "%s".', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must choose a name for your build plan.')); + } + + return $errors; + } + + public function getTransactionTypeForConduit($xaction) { + return 'name'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + +} diff --git a/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanStatusTransaction.php b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanStatusTransaction.php new file mode 100644 index 0000000000..e1c72b4183 --- /dev/null +++ b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanStatusTransaction.php @@ -0,0 +1,67 @@ +getPlanStatus(); + } + + public function applyInternalEffects($object, $value) { + $object->setPlanStatus($value); + } + + public function getTitle() { + $new = $this->getNewValue(); + if ($new === HarbormasterBuildPlan::STATUS_DISABLED) { + return pht( + '%s disabled this build plan.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this build plan.', + $this->renderAuthor()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $options = array( + HarbormasterBuildPlan::STATUS_DISABLED, + HarbormasterBuildPlan::STATUS_ACTIVE, + ); + $options = array_fuse($options); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (!isset($options[$new])) { + $errors[] = $this->newInvalidError( + pht( + 'Status "%s" is not a valid build plan status. Valid '. + 'statuses are: %s.', + $new, + implode(', ', $options))); + continue; + } + + } + + return $errors; + } + + public function getTransactionTypeForConduit($xaction) { + return 'status'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + +} diff --git a/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanTransactionType.php b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanTransactionType.php new file mode 100644 index 0000000000..5545d1de38 --- /dev/null +++ b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanTransactionType.php @@ -0,0 +1,4 @@ + Date: Tue, 26 Feb 2019 05:21:35 -0800 Subject: [PATCH 12/43] Provide "harbormaster.buildplan.edit" in the API Summary: Depends on D20217. Ref T13258. Mostly for completeness. You can't edit build steps so this may not be terribly useful, but you can do bulk policy edits or whatever? Test Plan: Edited a build plan via API. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20218 --- src/__phutil_library_map__.php | 2 ++ .../HarbormasterBuildPlanEditAPIMethod.php | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 src/applications/harbormaster/conduit/HarbormasterBuildPlanEditAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d4696bde60..076344ed3e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1331,6 +1331,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanDatasource' => 'applications/harbormaster/typeahead/HarbormasterBuildPlanDatasource.php', 'HarbormasterBuildPlanDefaultEditCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php', 'HarbormasterBuildPlanDefaultViewCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php', + 'HarbormasterBuildPlanEditAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildPlanEditAPIMethod.php', 'HarbormasterBuildPlanEditEngine' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php', 'HarbormasterBuildPlanEditor' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditor.php', 'HarbormasterBuildPlanNameNgrams' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanNameNgrams.php', @@ -6943,6 +6944,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanDatasource' => 'PhabricatorTypeaheadDatasource', 'HarbormasterBuildPlanDefaultEditCapability' => 'PhabricatorPolicyCapability', 'HarbormasterBuildPlanDefaultViewCapability' => 'PhabricatorPolicyCapability', + 'HarbormasterBuildPlanEditAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'HarbormasterBuildPlanEditEngine' => 'PhabricatorEditEngine', 'HarbormasterBuildPlanEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildPlanNameNgrams' => 'PhabricatorSearchNgrams', diff --git a/src/applications/harbormaster/conduit/HarbormasterBuildPlanEditAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterBuildPlanEditAPIMethod.php new file mode 100644 index 0000000000..5509cf189e --- /dev/null +++ b/src/applications/harbormaster/conduit/HarbormasterBuildPlanEditAPIMethod.php @@ -0,0 +1,20 @@ + Date: Tue, 26 Feb 2019 05:37:51 -0800 Subject: [PATCH 13/43] Add a "Recent Builds" element to the Build Plan UI and tighten up a few odds and ends Summary: Depends on D20218. Ref T13258. It's somewhat cumbersome to get from build plans to related builds but this is a reasonable thing to want to do, so make it a little easier. Also clean up / standardize / hint a few things a little better. Test Plan: {F6244116} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20219 --- src/__phutil_library_map__.php | 2 + .../HarbormasterPlanViewController.php | 92 ++++++++++++++----- .../query/HarbormasterBuildSearchEngine.php | 49 ++-------- .../storage/build/HarbormasterBuild.php | 4 + .../configuration/HarbormasterBuildPlan.php | 2 +- .../view/HarbormasterBuildView.php | 67 ++++++++++++++ 6 files changed, 151 insertions(+), 65 deletions(-) create mode 100644 src/applications/harbormaster/view/HarbormasterBuildView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 076344ed3e..b80940a25c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1370,6 +1370,7 @@ phutil_register_library_map(array( 'HarbormasterBuildTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildTransactionQuery.php', 'HarbormasterBuildUnitMessage' => 'applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php', 'HarbormasterBuildUnitMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php', + 'HarbormasterBuildView' => 'applications/harbormaster/view/HarbormasterBuildView.php', 'HarbormasterBuildViewController' => 'applications/harbormaster/controller/HarbormasterBuildViewController.php', 'HarbormasterBuildWorker' => 'applications/harbormaster/worker/HarbormasterBuildWorker.php', 'HarbormasterBuildable' => 'applications/harbormaster/storage/HarbormasterBuildable.php', @@ -6999,6 +7000,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', ), 'HarbormasterBuildUnitMessageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildView' => 'AphrontView', 'HarbormasterBuildViewController' => 'HarbormasterController', 'HarbormasterBuildWorker' => 'HarbormasterWorker', 'HarbormasterBuildable' => array( diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index f91c18c5b6..f2ead2c3db 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -18,11 +18,6 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { return new Aphront404Response(); } - $timeline = $this->buildTransactionTimeline( - $plan, - new HarbormasterBuildPlanTransactionQuery()); - $timeline->setShouldTerminate(true); - $title = $plan->getName(); $header = id(new PHUIHeaderView()) @@ -33,24 +28,30 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $curtain = $this->buildCurtainView($plan); - $crumbs = $this->buildApplicationCrumbs(); - $crumbs->addTextCrumb(pht('Plan %d', $id)); - $crumbs->setBorder(true); + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb($plan->getObjectName()) + ->setBorder(true); - list($step_list, $has_any_conflicts, $would_deadlock) = + list($step_list, $has_any_conflicts, $would_deadlock, $steps) = $this->buildStepList($plan); $error = null; - if ($would_deadlock) { - $error = pht('This build plan will deadlock when executed, due to '. - 'circular dependencies present in the build plan. '. - 'Examine the step list and resolve the deadlock.'); + if (!$steps) { + $error = pht( + 'This build plan does not have any build steps yet, so it will '. + 'not do anything when run.'); + } else if ($would_deadlock) { + $error = pht( + 'This build plan will deadlock when executed, due to circular '. + 'dependencies present in the build plan. Examine the step list '. + 'and resolve the deadlock.'); } else if ($has_any_conflicts) { // A deadlocking build will also cause all the artifacts to be // invalid, so we just skip showing this message if that's the // case. - $error = pht('This build plan has conflicts in one or more build steps. '. - 'Examine the step list and resolve the listed errors.'); + $error = pht( + 'This build plan has conflicts in one or more build steps. '. + 'Examine the step list and resolve the listed errors.'); } if ($error) { @@ -59,18 +60,28 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->appendChild($error); } + $builds_view = $this->newBuildsView($plan); + + $timeline = $this->buildTransactionTimeline( + $plan, + new HarbormasterBuildPlanTransactionQuery()); + $timeline->setShouldTerminate(true); + $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setCurtain($curtain) - ->setMainColumn(array( - $error, - $step_list, - $timeline, - )); + ->setMainColumn( + array( + $error, + $step_list, + $builds_view, + $timeline, + )); return $this->newPage() ->setTitle($title) ->setCrumbs($crumbs) + ->setPageObjectPHIDs(array($plan->getPHID())) ->appendChild($view); } @@ -213,7 +224,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->appendChild($step_list); - return array($step_box, $has_any_conflicts, $is_deadlocking); + return array($step_box, $has_any_conflicts, $is_deadlocking, $steps); } private function buildCurtainView(HarbormasterBuildPlan $plan) { @@ -376,7 +387,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { array $steps) { $has_conflicts = false; - if (count($step_phids) === 0) { + if (!$step_phids) { return null; } @@ -436,4 +447,41 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { return array($ui, $has_conflicts); } + + private function newBuildsView(HarbormasterBuildPlan $plan) { + $viewer = $this->getViewer(); + + $builds = id(new HarbormasterBuildQuery()) + ->setViewer($viewer) + ->withBuildPlanPHIDs(array($plan->getPHID())) + ->setLimit(10) + ->execute(); + + $list = id(new HarbormasterBuildView()) + ->setViewer($viewer) + ->setBuilds($builds) + ->newObjectList(); + + $list->setNoDataString(pht('No recent builds.')); + + $more_href = new PhutilURI( + $this->getApplicationURI('/build/'), + array('plan' => $plan->getPHID())); + + $more_link = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-list-ul') + ->setText(pht('View All Builds')) + ->setHref($more_href); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Recent Builds')) + ->addActionLink($more_link); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($list); + } + } diff --git a/src/applications/harbormaster/query/HarbormasterBuildSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildSearchEngine.php index 4cf6a83701..b8140d84f6 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildSearchEngine.php +++ b/src/applications/harbormaster/query/HarbormasterBuildSearchEngine.php @@ -128,49 +128,14 @@ final class HarbormasterBuildSearchEngine $viewer = $this->requireViewer(); - $buildables = mpull($builds, 'getBuildable'); - $object_phids = mpull($buildables, 'getBuildablePHID'); - $initiator_phids = mpull($builds, 'getInitiatorPHID'); - $phids = array_mergev(array($initiator_phids, $object_phids)); - $phids = array_unique(array_filter($phids)); + $list = id(new HarbormasterBuildView()) + ->setViewer($viewer) + ->setBuilds($builds) + ->newObjectList(); - $handles = $viewer->loadHandles($phids); - - $list = new PHUIObjectItemListView(); - foreach ($builds as $build) { - $id = $build->getID(); - $initiator = $handles[$build->getInitiatorPHID()]; - $buildable_object = $handles[$build->getBuildable()->getBuildablePHID()]; - - $item = id(new PHUIObjectItemView()) - ->setViewer($viewer) - ->setObject($build) - ->setObjectName(pht('Build %d', $build->getID())) - ->setHeader($build->getName()) - ->setHref($build->getURI()) - ->setEpoch($build->getDateCreated()) - ->addAttribute($buildable_object->getName()); - - if ($initiator) { - $item->addHandleIcon($initiator, $initiator->getName()); - } - - $status = $build->getBuildStatus(); - - $status_icon = HarbormasterBuildStatus::getBuildStatusIcon($status); - $status_color = HarbormasterBuildStatus::getBuildStatusColor($status); - $status_label = HarbormasterBuildStatus::getBuildStatusName($status); - - $item->setStatusIcon("{$status_icon} {$status_color}", $status_label); - - $list->addItem($item); - } - - $result = new PhabricatorApplicationSearchResultView(); - $result->setObjectList($list); - $result->setNoDataString(pht('No builds found.')); - - return $result; + return id(new PhabricatorApplicationSearchResultView()) + ->setObjectList($list) + ->setNoDataString(pht('No builds found.')); } } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 602e388477..84668b79f0 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -193,6 +193,10 @@ final class HarbormasterBuild extends HarbormasterDAO return HarbormasterBuildStatus::newBuildStatusObject($status_key); } + public function getObjectName() { + return pht('Build %d', $this->getID()); + } + /* -( Build Commands )----------------------------------------------------- */ diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index c177c2e7b7..9d2266e487 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -91,7 +91,7 @@ final class HarbormasterBuildPlan extends HarbormasterDAO } public function getObjectName() { - return pht('Build Plan %d', $this->getID()); + return pht('Plan %d', $this->getID()); } diff --git a/src/applications/harbormaster/view/HarbormasterBuildView.php b/src/applications/harbormaster/view/HarbormasterBuildView.php new file mode 100644 index 0000000000..54f5abe093 --- /dev/null +++ b/src/applications/harbormaster/view/HarbormasterBuildView.php @@ -0,0 +1,67 @@ +builds = $builds; + return $this; + } + + public function getBuilds() { + return $this->builds; + } + + public function render() { + return $this->newObjectList(); + } + + public function newObjectList() { + $viewer = $this->getViewer(); + $builds = $this->getBuilds(); + + $buildables = mpull($builds, 'getBuildable'); + $object_phids = mpull($buildables, 'getBuildablePHID'); + $initiator_phids = mpull($builds, 'getInitiatorPHID'); + $phids = array_mergev(array($initiator_phids, $object_phids)); + $phids = array_unique(array_filter($phids)); + + $handles = $viewer->loadHandles($phids); + + $list = new PHUIObjectItemListView(); + foreach ($builds as $build) { + $id = $build->getID(); + $initiator = $handles[$build->getInitiatorPHID()]; + $buildable_object = $handles[$build->getBuildable()->getBuildablePHID()]; + + $item = id(new PHUIObjectItemView()) + ->setViewer($viewer) + ->setObject($build) + ->setObjectName($build->getObjectName()) + ->setHeader($build->getName()) + ->setHref($build->getURI()) + ->setEpoch($build->getDateCreated()) + ->addAttribute($buildable_object->getName()); + + if ($initiator) { + $item->addByline($initiator->renderLink()); + } + + $status = $build->getBuildStatus(); + + $status_icon = HarbormasterBuildStatus::getBuildStatusIcon($status); + $status_color = HarbormasterBuildStatus::getBuildStatusColor($status); + $status_label = HarbormasterBuildStatus::getBuildStatusName($status); + + $item->setStatusIcon("{$status_icon} {$status_color}", $status_label); + + $list->addItem($item); + } + + return $list; + } + +} From 4cc556b576abbf6ccfdc0239f30428576d80e7c4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 05:43:13 -0800 Subject: [PATCH 14/43] Clean up a PhutilURI "alter()" callsite in Diffusion blame Summary: See . Test Plan: Viewed blame, clicked "Skip Past This Commit". Got jumped to the right place instead of a URI exception. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20222 --- .../controller/DiffusionBrowseController.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index fcef87b7ef..1493d658e6 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -709,8 +709,17 @@ final class DiffusionBrowseController extends DiffusionController { 'path' => $path, )); - $before_uri = $before_uri->alter('renamed', $renamed); - $before_uri = $before_uri->alter('follow', $follow); + if ($renamed === null) { + $before_uri->removeQueryParam('renamed'); + } else { + $before_uri->replaceQueryParam('renamed', $renamed); + } + + if ($follow === null) { + $before_uri->removeQueryParam('follow'); + } else { + $before_uri->replaceQueryParam('follow', $follow); + } return id(new AphrontRedirectResponse())->setURI($before_uri); } From 75dfae10118ad4c5630d7ea3046061a11303d8cb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 08:05:07 -0800 Subject: [PATCH 15/43] Don't require any special capabilities to apply a "closed a subtask" transaction to a parent task Summary: See PHI1059. If you close a task, we apply an "alice closed a subtask: X" transaction to its parents. This transaction is purely informative, but currently requires `CAN_EDIT` permission after T13186. However, we'd prefer to post this transaction anyway, even if: the parent is locked; or the parent is not editable by the acting user. Replace the implicit `CAN_EDIT` requirement with no requirement. (This transaction is only applied internally (by closing a subtask) and can't be applied via the API or any other channel, so this doesn't let attackers spam a bunch of bogus subtask closures all over the place or anything.) Test Plan: - Created a parent task A with subtask B. - Put task A into an "Edits Locked" status. - As a user other than the owner of A, closed B. Then: - Before: Policy exception when trying to apply the "alice closed a subtask: B" transaction to A. - After: B closed, A got a transaction despite being locked. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20223 --- .../xaction/ManiphestTaskUnblockTransaction.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php index 8833e62b79..cb6c80604d 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php @@ -123,4 +123,14 @@ final class ManiphestTaskUnblockTransaction return parent::shouldHideForFeed(); } + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + + // When you close a task, we want to apply this transaction to its parents + // even if you can not edit (or even see) those parents, so don't require + // any capabilities. See PHI1059. + + return null; + } } From 27ea775fda5f9a158a7e92a20f6b3c6ba7696a39 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 09:34:30 -0800 Subject: [PATCH 16/43] Fix a log warning when searching for ranges on custom "Date" fields Summary: See . It looks like this blames to D19126, which added some more complex constraint logic but overlooked "range" constraints, which are handled separately. Test Plan: - Added a custom "date" field to Maniphest with `"search": true`. - Executed a range query against the field. Then: - Before: Warnings about undefined indexes in the log. - After: No such warnings. Reviewers: amckinley Reviewed By: amckinley Subscribers: jbrownEP Differential Revision: https://secure.phabricator.com/D20225 --- .../query/policy/PhabricatorCursorPagedPolicyAwareQuery.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 9f7a69909a..773f78b3a6 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1233,6 +1233,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery 'index' => $index->getIndexKey(), 'alias' => $alias, 'value' => array($min, $max), + 'data' => null, + 'constraints' => null, ); return $this; From 54006f481729afc6d96c5ae833921dcd62a37d53 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 09:44:20 -0800 Subject: [PATCH 17/43] Stop "Mute Notifications" on Bulk Jobs from fataling Summary: See . Bulk Jobs have an "edge" table but currently do not support edge transactions. Add support. This stops "Mute Notifications" from fataling. The action probably doesn't do what the reporting user expects (it stops edits to the job object from sending notifications; it does not stop the edits the job performs from sending notifications) but I think this change puts us in a better place no matter what, even if we eventually clarify or remove this behavior. Test Plan: Clicked "Mute Notifications" on a bulk job, got an effect instead of a fatal. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20226 --- .../daemon/workers/editor/PhabricatorWorkerBulkJobEditor.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/infrastructure/daemon/workers/editor/PhabricatorWorkerBulkJobEditor.php b/src/infrastructure/daemon/workers/editor/PhabricatorWorkerBulkJobEditor.php index b23c987d6d..e94ca6dc49 100644 --- a/src/infrastructure/daemon/workers/editor/PhabricatorWorkerBulkJobEditor.php +++ b/src/infrastructure/daemon/workers/editor/PhabricatorWorkerBulkJobEditor.php @@ -15,6 +15,7 @@ final class PhabricatorWorkerBulkJobEditor $types = parent::getTransactionTypes(); $types[] = PhabricatorWorkerBulkJobTransaction::TYPE_STATUS; + $types[] = PhabricatorTransactions::TYPE_EDGE; return $types; } From bfe8f43f1a6f4eb3a30f0a74385f2b806c58be53 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 10:36:16 -0800 Subject: [PATCH 18/43] Use "QUERY_STRING", not "REQUEST_URI", to parse raw request parameters Summary: Fixes T13260. "QUERY_STRING" and "REQUEST_URI" are similar for our purposes here, but our nginx documentation tells you to pass "QUERY_STRING" and doesn't tell you to pass "REQUEST_URI". We also use "QUERY_STRING" in a couple of other places already, and already have a setup check for it. Use "QUERY_STRING" instead of "REQUEST_URI". Test Plan: Visited `/oauth/google/?a=b`, got redirected with parameters preserved. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13260 Differential Revision: https://secure.phabricator.com/D20227 --- src/aphront/AphrontRequest.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index 46d1266b08..48004a521f 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -591,15 +591,11 @@ final class AphrontRequest extends Phobject { } public function getRequestURI() { - $request_uri = idx($_SERVER, 'REQUEST_URI', '/'); + $uri_path = phutil_escape_uri($this->getPath()); + $uri_query = idx($_SERVER, 'QUERY_STRING', ''); - $uri = new PhutilURI($request_uri); - $uri->removeQueryParam('__path__'); - - $path = phutil_escape_uri($this->getPath()); - $uri->setPath($path); - - return $uri; + return id(new PhutilURI($uri_path.'?'.$uri_query)) + ->removeQueryParam('__path__'); } public function getAbsoluteRequestURI() { From e15fff00a6404319d01e30b0a673a1a2ce289ca9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Mar 2019 09:45:41 -0800 Subject: [PATCH 19/43] Use "LogLevel=ERROR" to try to improve "ssh" hostkey behavior without doing anything extreme/hacky Summary: Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful. In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking. For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see). Test Plan: - Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels. - With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning). - With `quiet`, got nothing (bad: we want the credential error). - With `ERROR`, got no warning but did get an error (good!). Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet". Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13121 Differential Revision: https://secure.phabricator.com/D20240 --- src/applications/diffusion/ssh/DiffusionSSHWorkflow.php | 4 ++-- .../drydock/interface/command/DrydockSSHCommandInterface.php | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index b2d1d25f44..57dc83953d 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -127,9 +127,9 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { // This is suppressing "added
to the list of known hosts" // messages, which are confusing and irrelevant when they arise from // proxied requests. It might also be suppressing lots of useful errors, - // of course. Ideally, we would enforce host keys eventually. + // of course. Ideally, we would enforce host keys eventually. See T13121. $options[] = '-o'; - $options[] = 'LogLevel=quiet'; + $options[] = 'LogLevel=ERROR'; // NOTE: We prefix the command with "@username", which the far end of the // connection will parse in order to act as the specified user. This diff --git a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php index 1aab14b57b..b1eebd92a1 100644 --- a/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php +++ b/src/applications/drydock/interface/command/DrydockSSHCommandInterface.php @@ -30,8 +30,11 @@ final class DrydockSSHCommandInterface extends DrydockCommandInterface { $full_command = call_user_func_array('csprintf', $argv); $flags = array(); + + // See T13121. Attempt to suppress the "Permanently added X to list of + // known hosts" message without suppressing anything important. $flags[] = '-o'; - $flags[] = 'LogLevel=quiet'; + $flags[] = 'LogLevel=ERROR'; $flags[] = '-o'; $flags[] = 'StrictHostKeyChecking=no'; From 9b0b50fbf4694006d622e64643e8f9d4b44cac27 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 4 Mar 2019 16:48:01 -0800 Subject: [PATCH 20/43] Give "bin/worker" flags to repeat and retry tasks Summary: See PHI1063. See PHI1114. Ref T13253. Currently, you can't `bin/worker execute` an archived task and can't `bin/worker retry` a successful task. Although it's good not to do these things by default (particularly, retrying a successful task will double its effects), there are plenty of cases where you want to re-run something for testing/development/debugging and don't care that the effect will repeat (you're in a dev environment, the effect doesn't matter, etc). Test Plan: Ran `bin/worker execute/retry` against archived/successful tasks. Got prompted to add more flags, then got re-execution. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13253 Differential Revision: https://secure.phabricator.com/D20246 --- ...ricatorWorkerManagementExecuteWorkflow.php | 49 +++++++++++++++++-- ...abricatorWorkerManagementRetryWorkflow.php | 32 ++++++++---- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php index 2acc8452ea..f3c6be520a 100644 --- a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php +++ b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php @@ -11,23 +11,64 @@ final class PhabricatorWorkerManagementExecuteWorkflow pht( 'Execute a task explicitly. This command ignores leases, is '. 'dangerous, and may cause work to be performed twice.')) - ->setArguments($this->getTaskSelectionArguments()); + ->setArguments( + array_merge( + array( + array( + 'name' => 'retry', + 'help' => pht('Retry archived tasks.'), + ), + array( + 'name' => 'repeat', + 'help' => pht('Repeat archived, successful tasks.'), + ), + ), + $this->getTaskSelectionArguments())); } public function execute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); $tasks = $this->loadTasks($args); + $is_retry = $args->getArg('retry'); + $is_repeat = $args->getArg('repeat'); + foreach ($tasks as $task) { $can_execute = !$task->isArchived(); if (!$can_execute) { - $console->writeOut( + if (!$is_retry) { + $console->writeOut( + "** %s ** %s\n", + pht('ARCHIVED'), + pht( + '%s is already archived, and will not be executed. '. + 'Use "--retry" to execute archived tasks.', + $this->describeTask($task))); + continue; + } + + $result_success = PhabricatorWorkerArchiveTask::RESULT_SUCCESS; + if ($task->getResult() == $result_success) { + if (!$is_repeat) { + $console->writeOut( + "** %s ** %s\n", + pht('SUCCEEDED'), + pht( + '%s has already succeeded, and will not be retried. '. + 'Use "--repeat" to repeat successful tasks.', + $this->describeTask($task))); + continue; + } + } + + echo tsprintf( "** %s ** %s\n", pht('ARCHIVED'), pht( - '%s is already archived, and can not be executed.', + 'Unarchiving %s.', $this->describeTask($task))); - continue; + + $task = $task->unarchiveTask(); } // NOTE: This ignores leases, maybe it should respect them without diff --git a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementRetryWorkflow.php b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementRetryWorkflow.php index 6dbebd168d..538a70add8 100644 --- a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementRetryWorkflow.php +++ b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementRetryWorkflow.php @@ -10,15 +10,24 @@ final class PhabricatorWorkerManagementRetryWorkflow ->setSynopsis( pht( 'Retry selected tasks which previously failed permanently or '. - 'were cancelled. Only archived, unsuccessful tasks can be '. - 'retried.')) - ->setArguments($this->getTaskSelectionArguments()); + 'were cancelled. Only archived tasks can be retried.')) + ->setArguments( + array_merge( + array( + array( + 'name' => 'repeat', + 'help' => pht( + 'Repeat tasks which already completed successfully.'), + ), + ), + $this->getTaskSelectionArguments())); } public function execute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); $tasks = $this->loadTasks($args); + $is_repeat = $args->getArg('repeat'); foreach ($tasks as $task) { if (!$task->isArchived()) { $console->writeOut( @@ -32,13 +41,16 @@ final class PhabricatorWorkerManagementRetryWorkflow $result_success = PhabricatorWorkerArchiveTask::RESULT_SUCCESS; if ($task->getResult() == $result_success) { - $console->writeOut( - "** %s ** %s\n", - pht('SUCCEEDED'), - pht( - '%s has already succeeded, and can not be retried.', - $this->describeTask($task))); - continue; + if (!$is_repeat) { + $console->writeOut( + "** %s ** %s\n", + pht('SUCCEEDED'), + pht( + '%s has already succeeded, and will not be repeated. '. + 'Use "--repeat" to repeat successful tasks.', + $this->describeTask($task))); + continue; + } } $task->unarchiveTask(); From 34e90d8f5139f3afac2230536933eb812dd428d1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Mar 2019 06:13:58 -0800 Subject: [PATCH 21/43] Clean up a few "%Q" stragglers in SVN repository browsing code Summary: See . Test Plan: Browed a Subversion repository in Diffusion. These are all reachable from the main landing page if the repository has commits/files, I think. Before change: errors in log; after change: no issues. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20244 --- .../conduit/DiffusionBrowseQueryConduitAPIMethod.php | 12 +++++++----- .../DiffusionHistoryQueryConduitAPIMethod.php | 12 ++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php index fe99471b0c..0b6ae19e32 100644 --- a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php @@ -368,9 +368,9 @@ final class DiffusionBrowseQueryConduitAPIMethod } if ($commit) { - $slice_clause = 'AND svnCommit <= '.(int)$commit; + $slice_clause = qsprintf($conn_r, 'AND svnCommit <= %d', $commit); } else { - $slice_clause = ''; + $slice_clause = qsprintf($conn_r, ''); } $index = queryfx_all( @@ -439,9 +439,11 @@ final class DiffusionBrowseQueryConduitAPIMethod $sql = array(); foreach ($index as $row) { - $sql[] = - '(pathID = '.(int)$row['pathID'].' AND '. - 'svnCommit = '.(int)$row['maxCommit'].')'; + $sql[] = qsprintf( + $conn_r, + '(pathID = %d AND svnCommit = %d)', + $row['pathID'], + $row['maxCommit']); } $browse = queryfx_all( diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php index 4c1d39e8c8..ebce21dd6f 100644 --- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php @@ -215,13 +215,17 @@ final class DiffusionHistoryQueryConduitAPIMethod return array(); } - $filter_query = ''; + $filter_query = qsprintf($conn_r, ''); if ($need_direct_changes) { if ($need_child_changes) { - $type = DifferentialChangeType::TYPE_CHILD; - $filter_query = 'AND (isDirect = 1 OR changeType = '.$type.')'; + $filter_query = qsprintf( + $conn_r, + 'AND (isDirect = 1 OR changeType = %s)', + DifferentialChangeType::TYPE_CHILD); } else { - $filter_query = 'AND (isDirect = 1)'; + $filter_query = qsprintf( + $conn_r, + 'AND (isDirect = 1)'); } } From ee2bc07c9025695571f35a34fb93a088be533c9e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Mar 2019 06:05:40 -0800 Subject: [PATCH 22/43] No-op old search indexing migrations which no longer run and have been obsoleted by upgrade "activities" Summary: See T13253. After D20200 (which changed the task schema) these migrations no longer run, since the PHP code will expect a column to exist that won't exist until a `20190220.` migration runs. We don't need these migrations, since anyone upgrading through September 2017 gets a "rebuild search indexes" activity anyway (see T11932). Just no-op them. Test Plan: Grepped for `queueDocumentForIndexing()` in `autopatches/`, removed all of it. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D20243 --- .../autopatches/20151221.search.3.reindex.php | 10 +--------- .../20160221.almanac.2.devicei.php | 10 +--------- .../20160221.almanac.4.servicei.php | 10 +--------- .../20160221.almanac.6.networki.php | 10 +--------- .../20160227.harbormaster.2.plani.php | 10 +--------- .../autopatches/20160303.drydock.2.bluei.php | 10 +--------- .../20160308.nuance.04.sourcei.php | 10 +--------- .../autopatches/20160406.badges.ngrams.php | 10 +--------- .../sql/autopatches/20160927.phurl.ngrams.php | 10 +--------- .../20161011.conpherence.ngrams.php | 10 +--------- .../20161216.dashboard.ngram.02.php | 20 +------------------ .../sql/autopatches/20170526.milestones.php | 10 +--------- .../20171026.ferret.05.ponder.index.php | 10 +--------- 13 files changed, 13 insertions(+), 127 deletions(-) diff --git a/resources/sql/autopatches/20151221.search.3.reindex.php b/resources/sql/autopatches/20151221.search.3.reindex.php index 09556d5ea0..623ba7bf6a 100644 --- a/resources/sql/autopatches/20151221.search.3.reindex.php +++ b/resources/sql/autopatches/20151221.search.3.reindex.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160221.almanac.2.devicei.php b/resources/sql/autopatches/20160221.almanac.2.devicei.php index aea17d0ad6..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160221.almanac.2.devicei.php +++ b/resources/sql/autopatches/20160221.almanac.2.devicei.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160221.almanac.4.servicei.php b/resources/sql/autopatches/20160221.almanac.4.servicei.php index 97211ca7b5..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160221.almanac.4.servicei.php +++ b/resources/sql/autopatches/20160221.almanac.4.servicei.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160221.almanac.6.networki.php b/resources/sql/autopatches/20160221.almanac.6.networki.php index 263defbb33..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160221.almanac.6.networki.php +++ b/resources/sql/autopatches/20160221.almanac.6.networki.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160227.harbormaster.2.plani.php b/resources/sql/autopatches/20160227.harbormaster.2.plani.php index 6dea004c06..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160227.harbormaster.2.plani.php +++ b/resources/sql/autopatches/20160227.harbormaster.2.plani.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160303.drydock.2.bluei.php b/resources/sql/autopatches/20160303.drydock.2.bluei.php index c0b68c2262..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160303.drydock.2.bluei.php +++ b/resources/sql/autopatches/20160303.drydock.2.bluei.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160308.nuance.04.sourcei.php b/resources/sql/autopatches/20160308.nuance.04.sourcei.php index eb0d1da113..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160308.nuance.04.sourcei.php +++ b/resources/sql/autopatches/20160308.nuance.04.sourcei.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160406.badges.ngrams.php b/resources/sql/autopatches/20160406.badges.ngrams.php index ce8d8896ef..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160406.badges.ngrams.php +++ b/resources/sql/autopatches/20160406.badges.ngrams.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20160927.phurl.ngrams.php b/resources/sql/autopatches/20160927.phurl.ngrams.php index 74cf61efa5..623ba7bf6a 100644 --- a/resources/sql/autopatches/20160927.phurl.ngrams.php +++ b/resources/sql/autopatches/20160927.phurl.ngrams.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20161011.conpherence.ngrams.php b/resources/sql/autopatches/20161011.conpherence.ngrams.php index 457143f6c7..623ba7bf6a 100644 --- a/resources/sql/autopatches/20161011.conpherence.ngrams.php +++ b/resources/sql/autopatches/20161011.conpherence.ngrams.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20161216.dashboard.ngram.02.php b/resources/sql/autopatches/20161216.dashboard.ngram.02.php index a7abc99b23..623ba7bf6a 100644 --- a/resources/sql/autopatches/20161216.dashboard.ngram.02.php +++ b/resources/sql/autopatches/20161216.dashboard.ngram.02.php @@ -1,21 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} - -$table_dbp = new PhabricatorDashboardPanel(); - -foreach (new LiskMigrationIterator($table_dbp) as $panel) { - PhabricatorSearchWorker::queueDocumentForIndexing( - $panel->getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20170526.milestones.php b/resources/sql/autopatches/20170526.milestones.php index 2e30ac4775..623ba7bf6a 100644 --- a/resources/sql/autopatches/20170526.milestones.php +++ b/resources/sql/autopatches/20170526.milestones.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. diff --git a/resources/sql/autopatches/20171026.ferret.05.ponder.index.php b/resources/sql/autopatches/20171026.ferret.05.ponder.index.php index 20489846d2..623ba7bf6a 100644 --- a/resources/sql/autopatches/20171026.ferret.05.ponder.index.php +++ b/resources/sql/autopatches/20171026.ferret.05.ponder.index.php @@ -1,11 +1,3 @@ getPHID(), - array( - 'force' => true, - )); -} +// This was an old reindexing migration that has been obsoleted. See T13253. From d192d04586ecb5153d87a1f961651abb496a8530 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Mar 2019 07:13:16 -0800 Subject: [PATCH 23/43] Make it more visually clear that you can click things in the "Big List of Clickable Things" UI element Summary: Ref T13259. An install provided feedback that it wasn't obvious you could click the buttons in this UI. Make it more clear that these are clickable buttons. Test Plan: {F6251585} {F6251586} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13259 Differential Revision: https://secure.phabricator.com/D20238 --- resources/celerity/map.php | 12 ++++----- .../css/phui/object-item/phui-oi-big-ui.css | 26 ++++++++++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3188de0052..11e7fe4118 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'e3c1a8f2', + 'core.pkg.css' => '34ce1741', 'core.pkg.js' => '2cda17a4', 'differential.pkg.css' => 'ab23bd75', 'differential.pkg.js' => '67e02996', @@ -127,7 +127,7 @@ return array( 'rsrc/css/phui/calendar/phui-calendar-list.css' => 'ccd7e4e2', 'rsrc/css/phui/calendar/phui-calendar-month.css' => 'cb758c42', 'rsrc/css/phui/calendar/phui-calendar.css' => 'f11073aa', - 'rsrc/css/phui/object-item/phui-oi-big-ui.css' => '9e037c7a', + 'rsrc/css/phui/object-item/phui-oi-big-ui.css' => '534f1757', 'rsrc/css/phui/object-item/phui-oi-color.css' => 'b517bfa0', 'rsrc/css/phui/object-item/phui-oi-drag-ui.css' => 'da15d3dc', 'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '490e2e2e', @@ -834,7 +834,7 @@ return array( 'phui-lightbox-css' => '4ebf22da', 'phui-list-view-css' => '470b1adb', 'phui-object-box-css' => 'f434b6be', - 'phui-oi-big-ui-css' => '9e037c7a', + 'phui-oi-big-ui-css' => '534f1757', 'phui-oi-color-css' => 'b517bfa0', 'phui-oi-drag-ui-css' => 'da15d3dc', 'phui-oi-flush-ui-css' => '490e2e2e', @@ -1345,6 +1345,9 @@ return array( 'javelin-dom', 'javelin-fx', ), + '534f1757' => array( + 'phui-oi-list-view-css', + ), '541f81c3' => array( 'javelin-install', ), @@ -1721,9 +1724,6 @@ return array( 'javelin-uri', 'phabricator-textareautils', ), - '9e037c7a' => array( - 'phui-oi-list-view-css', - ), '9f081f05' => array( 'javelin-behavior', 'javelin-dom', diff --git a/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css b/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css index a793c018c3..2d2163f9e9 100644 --- a/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css +++ b/webroot/rsrc/css/phui/object-item/phui-oi-big-ui.css @@ -13,7 +13,12 @@ } .phui-oi-list-big .phui-oi-image-icon { - margin: 8px 2px 12px; + margin: 12px 2px 12px; + text-align: center; +} + +.phui-oi-list-big .phui-oi-image-icon .phui-icon-view { + position: relative; } .phui-oi-list-big a.phui-oi-link { @@ -31,7 +36,7 @@ } .device-desktop .phui-oi-list-big .phui-oi { - margin-bottom: 4px; + margin-bottom: 8px; } .phui-oi-list-big .phui-oi-col0 { @@ -60,13 +65,28 @@ border-radius: 3px; } +.phui-oi-list-big .phui-oi-frame { + padding: 2px 8px; +} + +.phui-oi-list-big .phui-oi-linked-container { + border: 1px solid {$lightblueborder}; + border-radius: 4px; + box-shadow: 1px 1px 2px rgba(0, 0, 0, 0.035); +} + +.phui-oi-list-big .phui-oi-disabled { + border-radius: 4px; + background: {$lightgreybackground}; +} + .device-desktop .phui-oi-linked-container { cursor: pointer; } .device-desktop .phui-oi-linked-container:hover { background-color: {$hoverblue}; - border-radius: 3px; + border-color: {$blueborder}; } .device-desktop .phui-oi-linked-container a:hover { From 920ab13cfb86232c9732e978ff847bdb9d05bf3b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 19:13:53 -0800 Subject: [PATCH 24/43] Correct a possible fatal in the non-CSRF Duo MFA workflow Summary: Ref T13259. If we miss the separate CSRF step in Duo and proceed directly to prompting, we may fail to build a response which turns into a real control and fatal on `null->setLabel()`. Instead, let MFA providers customize their "bare prompt dialog" response, then make Duo use the same "you have an outstanding request" response for the CSRF and no-CSRF workflows. Test Plan: Hit Duo auth on a non-CSRF workflow (e.g., edit an MFA provider with Duo enabled). Previously: `setLabel()` fatal. After patch: smooth sailing. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13259 Differential Revision: https://secure.phabricator.com/D20234 --- .../engine/PhabricatorAuthSessionEngine.php | 9 ++++- .../auth/factor/PhabricatorAuthFactor.php | 34 +++++++++++++++++++ .../auth/factor/PhabricatorDuoAuthFactor.php | 13 +++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index c052805224..38ae2201b8 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -714,7 +714,14 @@ final class PhabricatorAuthSessionEngine extends Phobject { if (isset($validation_results[$factor_phid])) { continue; } - $validation_results[$factor_phid] = new PhabricatorAuthFactorResult(); + + $issued_challenges = idx($challenge_map, $factor_phid, array()); + + $validation_results[$factor_phid] = $impl->getResultForPrompt( + $factor, + $viewer, + $request, + $issued_challenges); } throw id(new PhabricatorAuthHighSecurityRequiredException()) diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php index d7e6e60ecc..fefd9b5fd1 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -221,6 +221,40 @@ abstract class PhabricatorAuthFactor extends Phobject { return $result; } + final public function getResultForPrompt( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + AphrontRequest $request, + array $challenges) { + assert_instances_of($challenges, 'PhabricatorAuthChallenge'); + + $result = $this->newResultForPrompt( + $config, + $viewer, + $request, + $challenges); + + if (!$this->isAuthResult($result)) { + throw new Exception( + pht( + 'Expected "newResultForPrompt()" to return an object of class "%s", '. + 'but it returned something else ("%s"; in "%s").', + 'PhabricatorAuthFactorResult', + phutil_describe_type($result), + get_class($this))); + } + + return $result; + } + + protected function newResultForPrompt( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + AphrontRequest $request, + array $challenges) { + return $this->newResult(); + } + abstract protected function newResultFromIssuedChallenges( PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer, diff --git a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php index 66bd7c9ebd..a84337a764 100644 --- a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php @@ -681,6 +681,19 @@ final class PhabricatorDuoAuthFactor AphrontRequest $request, array $challenges) { + return $this->getResultForPrompt( + $config, + $viewer, + $request, + $challenges); + } + + protected function newResultForPrompt( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + AphrontRequest $request, + array $challenges) { + $result = $this->newResult() ->setIsContinue(true) ->setErrorMessage( From cc8dda62990524c5cd7282ac8b0f38a805ecec38 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Mar 2019 06:54:32 -0800 Subject: [PATCH 25/43] Recognize the official "Go" magic regexp for generated code as generated Summary: See PHI1112. See T784. Although some more general/flexible solution is arriving eventually, adding this rule seems reasonable for now, since it's not a big deal if we remove it later to replace this with some fancier system. Test Plan: Created a diff with the official Go generated marker, saw the changeset marked as generated. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20237 --- .../differential/engine/DifferentialChangesetEngine.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/differential/engine/DifferentialChangesetEngine.php b/src/applications/differential/engine/DifferentialChangesetEngine.php index d72db025ad..23382e6a81 100644 --- a/src/applications/differential/engine/DifferentialChangesetEngine.php +++ b/src/applications/differential/engine/DifferentialChangesetEngine.php @@ -54,6 +54,12 @@ final class DifferentialChangesetEngine extends Phobject { if (strpos($new_data, '@'.'generated') !== false) { return true; } + + // See PHI1112. This is the official pattern for marking Go code as + // generated. + if (preg_match('(^// Code generated .* DO NOT EDIT\.$)m', $new_data)) { + return true; + } } return false; From c116deef6398fc7381306c66f0b92ed9f8c457dc Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 08:25:13 -0800 Subject: [PATCH 26/43] Remove "Effective User" attachment from Repository Identities Summary: See . It's possible for an Idenitity to be bound to user X, then for that user to be deleted, e.g. with `bin/remove destroy X`. In this case, we'll fail to load/attach the user to the identity. Currently, we don't actually use this anywhere, so just stop loading it. Once we start using it (if we ever do), we could figure out what an appropriate policy is in this case. Test Plan: Browsed around Diffusion, grepped for affected "effective user" methods and found no callsites. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20224 --- .../PhabricatorRepositoryIdentityQuery.php | 23 ------------------- .../storage/PhabricatorRepositoryIdentity.php | 11 --------- 2 files changed, 34 deletions(-) diff --git a/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php b/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php index ef038f045f..c64b1a296b 100644 --- a/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php @@ -124,29 +124,6 @@ final class PhabricatorRepositoryIdentityQuery return $where; } - protected function didFilterPage(array $identities) { - $user_ids = array_filter( - mpull($identities, 'getCurrentEffectiveUserPHID', 'getID')); - if (!$user_ids) { - return $identities; - } - - $users = id(new PhabricatorPeopleQuery()) - ->withPHIDs($user_ids) - ->setViewer($this->getViewer()) - ->execute(); - $users = mpull($users, null, 'getPHID'); - - foreach ($identities as $identity) { - if ($identity->hasEffectiveUser()) { - $user = idx($users, $identity->getCurrentEffectiveUserPHID()); - $identity->attachEffectiveUser($user); - } - } - - return $identities; - } - public function getQueryApplicationClass() { return 'PhabricatorDiffusionApplication'; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php index 76c6aed9e0..e3833bd10e 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php +++ b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php @@ -14,17 +14,6 @@ final class PhabricatorRepositoryIdentity protected $manuallySetUserPHID; protected $currentEffectiveUserPHID; - private $effectiveUser = self::ATTACHABLE; - - public function attachEffectiveUser(PhabricatorUser $user) { - $this->effectiveUser = $user; - return $this; - } - - public function getEffectiveUser() { - return $this->assertAttached($this->effectiveUser); - } - protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, From ea6c0c9bdebfdbffeb524412651b5ae675c32887 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 19:41:56 -0800 Subject: [PATCH 27/43] Refine the "Mangled Webserver Response" setup check Summary: Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer. If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response". Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do. Test Plan: - Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before). - Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before). - Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13259 Differential Revision: https://secure.phabricator.com/D20235 --- .../AphrontApplicationConfiguration.php | 1 - .../check/PhabricatorWebServerSetupCheck.php | 41 +++++++++---------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 8cd27fa62b..a479209125 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -776,7 +776,6 @@ final class AphrontApplicationConfiguration 'filler' => str_repeat('Q', 1024 * 16), ); - return id(new AphrontJSONResponse()) ->setAddJSONShield(false) ->setContent($result); diff --git a/src/applications/config/check/PhabricatorWebServerSetupCheck.php b/src/applications/config/check/PhabricatorWebServerSetupCheck.php index 8f6885e8e8..284b5e2a5f 100644 --- a/src/applications/config/check/PhabricatorWebServerSetupCheck.php +++ b/src/applications/config/check/PhabricatorWebServerSetupCheck.php @@ -129,30 +129,16 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck { } $structure = null; - $caught = null; $extra_whitespace = ($body !== trim($body)); - if (!$extra_whitespace) { - try { - $structure = phutil_json_decode($body); - } catch (Exception $ex) { - $caught = $ex; - } + try { + $structure = phutil_json_decode(trim($body)); + } catch (Exception $ex) { + // Ignore the exception, we only care if the decode worked or not. } - if (!$structure) { - if ($extra_whitespace) { - $message = pht( - 'Phabricator sent itself a test request and expected to get a bare '. - 'JSON response back, but the response had extra whitespace at '. - 'the beginning or end.'. - "\n\n". - 'This usually means you have edited a file and left whitespace '. - 'characters before the opening %s tag, or after a closing %s tag. '. - 'Remove any leading whitespace, and prefer to omit closing tags.', - phutil_tag('tt', array(), '')); - } else { + if (!$structure || $extra_whitespace) { + if (!$structure) { $short = id(new PhutilUTF8StringTruncator()) ->setMaximumGlyphs(1024) ->truncateString($body); @@ -166,6 +152,17 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck { "\n\n". 'Something is misconfigured or otherwise mangling responses.', phutil_tag('pre', array(), $short)); + } else { + $message = pht( + 'Phabricator sent itself a test request and expected to get a bare '. + 'JSON response back. It received a JSON response, but the response '. + 'had extra whitespace at the beginning or end.'. + "\n\n". + 'This usually means you have edited a file and left whitespace '. + 'characters before the opening %s tag, or after a closing %s tag. '. + 'Remove any leading whitespace, and prefer to omit closing tags.', + phutil_tag('tt', array(), '')); } $this->newIssue('webserver.mangle') @@ -174,7 +171,9 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck { ->setMessage($message); // We can't run the other checks if we could not decode the response. - return; + if (!$structure) { + return; + } } $actual_user = idx($structure, 'user'); From d36d0efc35706f4bf1c327f5d4ffc76fbde92426 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Feb 2019 06:26:03 -0800 Subject: [PATCH 28/43] Add behaviors to Build Plans: hold drafts, affect buildables, warn on landing, restartable, runnable Summary: Depends on D20219. Ref T13258. Ref T11415. Installs sometimes have long-running builds or unimportant builds which they may not want to hold up drafts, affect buildable status, or warn during `arc land`. Some builds have side effects (like deployment or merging) and are not idempotent. They can cause problems if restarted. In other cases, builds are isolated and idempotent and generally safe, and it's okay for marketing interns to restart them. To address these cases, add "Behaviors" to Build Plans: - Hold Drafts: Controls how the build affects revision promotion from "Draft". - Warn on Land: Controls the "arc land" warning. - Affects Buildable: Controls whether we care about this build when figuring out if a buildable passed or failed overall. - Restartable: Controls whether this build may restart or not. - Runnable: Allows you to weaken the requirements to run the build if you're confident it's safe to run it on arbitrary old versions of things. NOTE: This only implements UI, none of these options actually do anything yet. Test Plan: Mostly poked around the UI. I'll actually implement these behaviors next, and vet them more thoroughly. {F6244828} {F6244830} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258, T11415 Differential Revision: https://secure.phabricator.com/D20220 --- .../20190226.harbor.01.planprops.sql | 2 + .../20190226.harbor.02.planvalue.sql | 2 + src/__phutil_library_map__.php | 8 + .../PhabricatorHarbormasterApplication.php | 2 + .../HarbormasterPlanBehaviorController.php | 92 +++++ .../HarbormasterPlanViewController.php | 73 ++++ .../HarbormasterBuildPlanEditEngine.php | 32 +- .../plan/HarbormasterBuildPlanBehavior.php | 348 ++++++++++++++++++ .../HarbormasterBuildPlanBehaviorOption.php | 57 +++ .../configuration/HarbormasterBuildPlan.php | 13 + ...rbormasterBuildPlanBehaviorTransaction.php | 127 +++++++ 11 files changed, 755 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20190226.harbor.01.planprops.sql create mode 100644 resources/sql/autopatches/20190226.harbor.02.planvalue.sql create mode 100644 src/applications/harbormaster/controller/HarbormasterPlanBehaviorController.php create mode 100644 src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php create mode 100644 src/applications/harbormaster/plan/HarbormasterBuildPlanBehaviorOption.php create mode 100644 src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanBehaviorTransaction.php diff --git a/resources/sql/autopatches/20190226.harbor.01.planprops.sql b/resources/sql/autopatches/20190226.harbor.01.planprops.sql new file mode 100644 index 0000000000..324139669e --- /dev/null +++ b/resources/sql/autopatches/20190226.harbor.01.planprops.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan + ADD properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20190226.harbor.02.planvalue.sql b/resources/sql/autopatches/20190226.harbor.02.planvalue.sql new file mode 100644 index 0000000000..b1929abf59 --- /dev/null +++ b/resources/sql/autopatches/20190226.harbor.02.planvalue.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_harbormaster.harbormaster_buildplan + SET properties = '{}' WHERE properties = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b80940a25c..15c7637e3d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1328,6 +1328,9 @@ phutil_register_library_map(array( 'HarbormasterBuildMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildMessageQuery.php', 'HarbormasterBuildPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPHIDType.php', 'HarbormasterBuildPlan' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php', + 'HarbormasterBuildPlanBehavior' => 'applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php', + 'HarbormasterBuildPlanBehaviorOption' => 'applications/harbormaster/plan/HarbormasterBuildPlanBehaviorOption.php', + 'HarbormasterBuildPlanBehaviorTransaction' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanBehaviorTransaction.php', 'HarbormasterBuildPlanDatasource' => 'applications/harbormaster/typeahead/HarbormasterBuildPlanDatasource.php', 'HarbormasterBuildPlanDefaultEditCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php', 'HarbormasterBuildPlanDefaultViewCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php', @@ -1424,6 +1427,7 @@ phutil_register_library_map(array( 'HarbormasterMessageType' => 'applications/harbormaster/engine/HarbormasterMessageType.php', 'HarbormasterObject' => 'applications/harbormaster/storage/HarbormasterObject.php', 'HarbormasterOtherBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterOtherBuildStepGroup.php', + 'HarbormasterPlanBehaviorController' => 'applications/harbormaster/controller/HarbormasterPlanBehaviorController.php', 'HarbormasterPlanController' => 'applications/harbormaster/controller/HarbormasterPlanController.php', 'HarbormasterPlanDisableController' => 'applications/harbormaster/controller/HarbormasterPlanDisableController.php', 'HarbormasterPlanEditController' => 'applications/harbormaster/controller/HarbormasterPlanEditController.php', @@ -6942,6 +6946,9 @@ phutil_register_library_map(array( 'PhabricatorConduitResultInterface', 'PhabricatorProjectInterface', ), + 'HarbormasterBuildPlanBehavior' => 'Phobject', + 'HarbormasterBuildPlanBehaviorOption' => 'Phobject', + 'HarbormasterBuildPlanBehaviorTransaction' => 'HarbormasterBuildPlanTransactionType', 'HarbormasterBuildPlanDatasource' => 'PhabricatorTypeaheadDatasource', 'HarbormasterBuildPlanDefaultEditCapability' => 'PhabricatorPolicyCapability', 'HarbormasterBuildPlanDefaultViewCapability' => 'PhabricatorPolicyCapability', @@ -7057,6 +7064,7 @@ phutil_register_library_map(array( 'HarbormasterMessageType' => 'Phobject', 'HarbormasterObject' => 'HarbormasterDAO', 'HarbormasterOtherBuildStepGroup' => 'HarbormasterBuildStepGroup', + 'HarbormasterPlanBehaviorController' => 'HarbormasterPlanController', 'HarbormasterPlanController' => 'HarbormasterController', 'HarbormasterPlanDisableController' => 'HarbormasterPlanController', 'HarbormasterPlanEditController' => 'HarbormasterPlanController', diff --git a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php index 80be90b375..4b369e821e 100644 --- a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php +++ b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php @@ -83,6 +83,8 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication { => 'HarbormasterPlanEditController', 'order/(?:(?P\d+)/)?' => 'HarbormasterPlanOrderController', 'disable/(?P\d+)/' => 'HarbormasterPlanDisableController', + 'behavior/(?P\d+)/(?P[^/]+)/' => + 'HarbormasterPlanBehaviorController', 'run/(?P\d+)/' => 'HarbormasterPlanRunController', '(?P\d+)/' => 'HarbormasterPlanViewController', ), diff --git a/src/applications/harbormaster/controller/HarbormasterPlanBehaviorController.php b/src/applications/harbormaster/controller/HarbormasterPlanBehaviorController.php new file mode 100644 index 0000000000..8f1fece691 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterPlanBehaviorController.php @@ -0,0 +1,92 @@ +getViewer(); + + $plan = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getURIData('id'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$plan) { + return new Aphront404Response(); + } + + $behavior_key = $request->getURIData('behaviorKey'); + $metadata_key = HarbormasterBuildPlanBehavior::getTransactionMetadataKey(); + + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + $behavior = idx($behaviors, $behavior_key); + if (!$behavior) { + return new Aphront404Response(); + } + + $plan_uri = $plan->getURI(); + + $v_option = $behavior->getPlanOption($plan)->getKey(); + if ($request->isFormPost()) { + $v_option = $request->getStr('option'); + + $xactions = array(); + + $xactions[] = id(new HarbormasterBuildPlanTransaction()) + ->setTransactionType( + HarbormasterBuildPlanBehaviorTransaction::TRANSACTIONTYPE) + ->setMetadataValue($metadata_key, $behavior_key) + ->setNewValue($v_option); + + $editor = id(new HarbormasterBuildPlanEditor()) + ->setActor($viewer) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->setContentSourceFromRequest($request); + + $editor->applyTransactions($plan, $xactions); + + return id(new AphrontRedirectResponse())->setURI($plan_uri); + } + + $select_control = id(new AphrontFormRadioButtonControl()) + ->setName('option') + ->setValue($v_option) + ->setLabel(pht('Option')); + + foreach ($behavior->getOptions() as $option) { + $icon = id(new PHUIIconView()) + ->setIcon($option->getIcon()); + + $select_control->addButton( + $option->getKey(), + array( + $icon, + ' ', + $option->getName(), + ), + $option->getDescription()); + } + + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendInstructions( + pht( + 'Choose a build plan behavior for "%s".', + phutil_tag('strong', array(), $behavior->getName()))) + ->appendRemarkupInstructions($behavior->getEditInstructions()) + ->appendControl($select_control); + + return $this->newDialog() + ->setTitle(pht('Edit Behavior: %s', $behavior->getName())) + ->appendForm($form) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->addSubmitButton(pht('Save Changes')) + ->addCancelButton($plan_uri); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index f2ead2c3db..0ef6162aad 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -61,6 +61,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { } $builds_view = $this->newBuildsView($plan); + $options_view = $this->newOptionsView($plan); $timeline = $this->buildTransactionTimeline( $plan, @@ -74,6 +75,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { array( $error, $step_list, + $options_view, $builds_view, $timeline, )); @@ -484,4 +486,75 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->appendChild($list); } + + private function newOptionsView(HarbormasterBuildPlan $plan) { + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $plan, + PhabricatorPolicyCapability::CAN_EDIT); + + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + + $rows = array(); + foreach ($behaviors as $behavior) { + $option = $behavior->getPlanOption($plan); + + $icon = $option->getIcon(); + $icon = id(new PHUIIconView())->setIcon($icon); + + $edit_uri = new PhutilURI( + $this->getApplicationURI( + urisprintf( + 'plan/behavior/%d/%s/', + $plan->getID(), + $behavior->getKey()))); + + $edit_button = id(new PHUIButtonView()) + ->setTag('a') + ->setColor(PHUIButtonView::GREY) + ->setSize(PHUIButtonView::SMALL) + ->setDisabled(!$can_edit) + ->setWorkflow(true) + ->setText(pht('Edit')) + ->setHref($edit_uri); + + $rows[] = array( + $icon, + $behavior->getName(), + $option->getName(), + $option->getDescription(), + $edit_button, + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + null, + pht('Name'), + pht('Behavior'), + pht('Details'), + null, + )) + ->setColumnClasses( + array( + null, + 'pri', + null, + 'wide', + null, + )); + + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Plan Behaviors')); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setTable($table); + } + } diff --git a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php index 35a417d9d4..c0fa80d71b 100644 --- a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php +++ b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditEngine.php @@ -77,7 +77,7 @@ final class HarbormasterBuildPlanEditEngine } protected function buildCustomEditFields($object) { - return array( + $fields = array( id(new PhabricatorTextEditField()) ->setKey('name') ->setLabel(pht('Name')) @@ -89,6 +89,36 @@ final class HarbormasterBuildPlanEditEngine ->setConduitTypeDescription(pht('New plan name.')) ->setValue($object->getName()), ); + + + $metadata_key = HarbormasterBuildPlanBehavior::getTransactionMetadataKey(); + + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + foreach ($behaviors as $behavior) { + $key = $behavior->getKey(); + + // Get the raw key off the object so that we don't reset stuff to + // default values by mistake if a behavior goes missing somehow. + $storage_key = HarbormasterBuildPlanBehavior::getStorageKeyForBehaviorKey( + $key); + $behavior_option = $object->getPlanProperty($storage_key); + + if (!strlen($behavior_option)) { + $behavior_option = $behavior->getPlanOption($object)->getKey(); + } + + $fields[] = id(new PhabricatorSelectEditField()) + ->setIsFormField(false) + ->setKey(sprintf('behavior.%s', $behavior->getKey())) + ->setMetadataValue($metadata_key, $behavior->getKey()) + ->setLabel(pht('Behavior: %s', $behavior->getName())) + ->setTransactionType( + HarbormasterBuildPlanBehaviorTransaction::TRANSACTIONTYPE) + ->setValue($behavior_option) + ->setOptions($behavior->getOptionMap()); + } + + return $fields; } } diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php new file mode 100644 index 0000000000..b0f722a1ca --- /dev/null +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -0,0 +1,348 @@ +key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setName($name) { + $this->name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setEditInstructions($edit_instructions) { + $this->editInstructions = $edit_instructions; + return $this; + } + + public function getEditInstructions() { + return $this->editInstructions; + } + + public function getOptionMap() { + return mpull($this->options, 'getName', 'getKey'); + } + + public function setOptions(array $options) { + assert_instances_of($options, 'HarbormasterBuildPlanBehaviorOption'); + + $key_map = array(); + $default = null; + + foreach ($options as $option) { + $key = $option->getKey(); + + if (isset($key_map[$key])) { + throw new Exception( + pht( + 'Multiple behavior options (for behavior "%s") have the same '. + 'key ("%s"). Each option must have a unique key.', + $this->getKey(), + $key)); + } + $key_map[$key] = true; + + if ($option->getIsDefault()) { + if ($default === null) { + $default = $key; + } else { + throw new Exception( + pht( + 'Multiple behavior options (for behavior "%s") are marked as '. + 'default options ("%s" and "%s"). Exactly one option must be '. + 'marked as the default option.', + $this->getKey(), + $default, + $key)); + } + } + } + + if ($default === null) { + throw new Exception( + pht( + 'No behavior option is marked as the default option (for '. + 'behavior "%s"). Exactly one option must be marked as the '. + 'default option.', + $this->getKey())); + } + + $this->options = mpull($options, null, 'getKey'); + $this->defaultKey = $default; + + return $this; + } + + public function getOptions() { + return $this->options; + } + + public function getPlanOption(HarbormasterBuildPlan $plan) { + $behavior_key = $this->getKey(); + $storage_key = self::getStorageKeyForBehaviorKey($behavior_key); + + $plan_value = $plan->getPlanProperty($storage_key); + if (isset($this->options[$plan_value])) { + return $this->options[$plan_value]; + } + + return idx($this->options, $this->defaultKey); + } + + public static function getTransactionMetadataKey() { + return 'behavior-key'; + } + + public static function getStorageKeyForBehaviorKey($behavior_key) { + return sprintf('behavior.%s', $behavior_key); + } + + public static function newPlanBehaviors() { + $draft_options = array( + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('always') + ->setIcon('fa-check-circle-o green') + ->setName(pht('Always')) + ->setIsDefault(true) + ->setDescription( + pht( + 'Revisions are not sent for review until the build completes, '. + 'and are returned to the author for updates if the build fails.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('building') + ->setIcon('fa-pause-circle-o yellow') + ->setName(pht('If Building')) + ->setDescription( + pht( + 'Revisions are not sent for review until the build completes, '. + 'but they will be sent for review even if it fails.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('never') + ->setIcon('fa-circle-o red') + ->setName(pht('Never')) + ->setDescription( + pht( + 'Revisions are sent for review regardless of the status of the '. + 'build.')), + ); + + $land_options = array( + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('always') + ->setIcon('fa-check-circle-o green') + ->setName(pht('Always')) + ->setIsDefault(true) + ->setDescription( + pht( + '"arc land" warns if the build is still running or has '. + 'failed.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('building') + ->setIcon('fa-pause-circle-o yellow') + ->setName(pht('If Building')) + ->setDescription( + pht( + '"arc land" warns if the build is still running, but ignores '. + 'the build if it has failed.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('complete') + ->setIcon('fa-dot-circle-o yellow') + ->setName(pht('If Complete')) + ->setDescription( + pht( + '"arc land" warns if the build has failed, but ignores the '. + 'build if it is still running.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('never') + ->setIcon('fa-circle-o red') + ->setName(pht('Never')) + ->setDescription( + pht( + '"arc land" never warns that the build is still running or '. + 'has failed.')), + ); + + $aggregate_options = array( + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('always') + ->setIcon('fa-check-circle-o green') + ->setName(pht('Always')) + ->setIsDefault(true) + ->setDescription( + pht( + 'The buildable waits for the build, and fails if the '. + 'build fails.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('building') + ->setIcon('fa-pause-circle-o yellow') + ->setName(pht('If Building')) + ->setDescription( + pht( + 'The buildable waits for the build, but does not fail '. + 'if the build fails.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('never') + ->setIcon('fa-circle-o red') + ->setName(pht('Never')) + ->setDescription( + pht( + 'The buildable does not wait for the build.')), + ); + + $restart_options = array( + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('always') + ->setIcon('fa-repeat green') + ->setName(pht('Always')) + ->setIsDefault(true) + ->setDescription( + pht('The build may be restarted.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('never') + ->setIcon('fa-times red') + ->setName(pht('Never')) + ->setDescription( + pht('The build may not be restarted.')), + ); + + $run_options = array( + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('edit') + ->setIcon('fa-pencil green') + ->setName(pht('If Editable')) + ->setIsDefault(true) + ->setDescription( + pht('Only users who can edit the plan can run it manually.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey('view') + ->setIcon('fa-exclamation-triangle yellow') + ->setName(pht('If Viewable')) + ->setDescription( + pht( + 'Any user who can view the plan can run it manually.')), + ); + + $behaviors = array( + id(new self()) + ->setKey('hold-drafts') + ->setName(pht('Hold Drafts')) + ->setEditInstructions( + pht( + 'When users create revisions in Differential, the default '. + 'behavior is to hold them in the "Draft" state until all builds '. + 'pass. Once builds pass, the revisions promote and are sent for '. + 'review, which notifies reviewers.'. + "\n\n". + 'The general intent of this workflow is to make sure reviewers '. + 'are only spending time on review once changes survive automated '. + 'tests. If a change does not pass tests, it usually is not '. + 'really ready for review.'. + "\n\n". + 'If you want to promote revisions out of "Draft" before builds '. + 'pass, or promote revisions even when builds fail, you can '. + 'change the promotion behavior. This may be useful if you have '. + 'very long-running builds, or some builds which are not very '. + 'important.'. + "\n\n". + 'Users may always use "Request Review" to promote a "Draft" '. + 'revision, even if builds have failed or are still in progress.')) + ->setOptions($draft_options), + id(new self()) + ->setKey('arc-land') + ->setName(pht('Warn When Landing')) + ->setEditInstructions( + pht( + 'When a user attempts to `arc land` a revision and that revision '. + 'has ongoing or failed builds, the default behavior of `arc` is '. + 'to warn them about those builds and give them a chance to '. + 'reconsider: they may want to wait for ongoing builds to '. + 'complete, or fix failed builds before landing the change.'. + "\n\n". + 'If you do not want to warn users about this build, you can '. + 'change the warning behavior. This may be useful if the build '. + 'takes a long time to run (so you do not expect users to wait '. + 'for it) or the outcome is not important.'. + "\n\n". + 'This warning is only advisory. Users may always elect to ignore '. + 'this warning and continue, even if builds have failed.')) + ->setOptions($land_options), + id(new self()) + ->setKey('buildable') + ->setEditInstructions( + pht( + 'The overall state of a buildable (like a commit or revision) is '. + 'normally the aggregation of the individual states of all builds '. + 'that have run against it.'. + "\n\n". + 'Buildables are "building" until all builds pass (which changes '. + 'them to "pass"), or any build fails (which changes them to '. + '"fail").'. + "\n\n". + 'You can change this behavior if you do not want to wait for this '. + 'build, or do not care if it fails.')) + ->setName(pht('Affects Buildable')) + ->setOptions($aggregate_options), + id(new self()) + ->setKey('restartable') + ->setEditInstructions( + pht( + 'Usually, builds may be restarted. This may be useful if you '. + 'suspect a build has failed for environmental or circumstantial '. + 'reasons unrelated to the actual code, and want to give it '. + 'another chance at glory.'. + "\n\n". + 'If you want to prevent a build from being restarted, you can '. + 'change the behavior here. This may be useful to prevent '. + 'accidents where a build with a dangerous side effect (like '. + 'deployment) is restarted improperly.')) + ->setName(pht('Restartable')) + ->setOptions($restart_options), + id(new self()) + ->setKey('runnable') + ->setEditInstructions( + pht( + 'To run a build manually, you normally must have permission to '. + 'edit the related build plan. If you would prefer that anyone who '. + 'can see the build plan be able to run and restart the build, you '. + 'can change the behavior here.'. + "\n\n". + 'Note that this affects both {nav Run Plan Manually} and '. + '{nav Restart Build}, since the two actions are largely '. + 'equivalent.'. + "\n\n". + 'WARNING: This may be unsafe, particularly if the build has '. + 'side effects like deployment.'. + "\n\n". + 'If you weaken this policy, an attacker with control of an '. + 'account that has "Can View" permission but not "Can Edit" '. + 'permission can manually run this build against any old version '. + 'of the code, including versions with known security issues.'. + "\n\n". + 'If running the build has a side effect like deploying code, '. + 'they can force deployment of a vulnerable version and then '. + 'escalate into an attack against the deployed service.')) + ->setName(pht('Runnable')) + ->setOptions($run_options), + ); + + return mpull($behaviors, null, 'getKey'); + } + +} diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehaviorOption.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehaviorOption.php new file mode 100644 index 0000000000..65b9662b9f --- /dev/null +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehaviorOption.php @@ -0,0 +1,57 @@ +name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setKey($key) { + $this->key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setDescription($description) { + $this->description = $description; + return $this; + } + + public function getDescription() { + return $this->description; + } + + public function setIsDefault($is_default) { + $this->isDefault = $is_default; + return $this; + } + + public function getIsDefault() { + return $this->isDefault; + } + + public function setIcon($icon) { + $this->icon = $icon; + return $this; + } + + public function getIcon() { + return $this->icon; + } + +} diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index 9d2266e487..8f8b0a20a8 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -17,6 +17,7 @@ final class HarbormasterBuildPlan extends HarbormasterDAO protected $planAutoKey; protected $viewPolicy; protected $editPolicy; + protected $properties = array(); const STATUS_ACTIVE = 'active'; const STATUS_DISABLED = 'disabled'; @@ -45,6 +46,9 @@ final class HarbormasterBuildPlan extends HarbormasterDAO protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort128', 'planStatus' => 'text32', @@ -94,6 +98,15 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return pht('Plan %d', $this->getID()); } + public function getPlanProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setPlanProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + /* -( Autoplans )---------------------------------------------------------- */ diff --git a/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanBehaviorTransaction.php b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanBehaviorTransaction.php new file mode 100644 index 0000000000..7a65eefdfa --- /dev/null +++ b/src/applications/harbormaster/xaction/plan/HarbormasterBuildPlanBehaviorTransaction.php @@ -0,0 +1,127 @@ +getBehavior(); + return $behavior->getPlanOption($object)->getKey(); + } + + public function applyInternalEffects($object, $value) { + $key = $this->getStorageKey(); + return $object->setPlanProperty($key, $value); + } + + public function getTitle() { + $old_value = $this->getOldValue(); + $new_value = $this->getNewValue(); + + $behavior = $this->getBehavior(); + if ($behavior) { + $behavior_name = $behavior->getName(); + + $options = $behavior->getOptions(); + if (isset($options[$old_value])) { + $old_value = $options[$old_value]->getName(); + } + + if (isset($options[$new_value])) { + $new_value = $options[$new_value]->getName(); + } + } else { + $behavior_name = $this->getBehaviorKey(); + } + + return pht( + '%s changed the %s behavior for this plan from %s to %s.', + $this->renderAuthor(), + $this->renderValue($behavior_name), + $this->renderValue($old_value), + $this->renderValue($new_value)); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + $behaviors = mpull($behaviors, null, 'getKey'); + + foreach ($xactions as $xaction) { + $key = $this->getBehaviorKeyForTransaction($xaction); + + if (!isset($behaviors[$key])) { + $errors[] = $this->newInvalidError( + pht( + 'No behavior with key "%s" exists. Valid keys are: %s.', + $key, + implode(', ', array_keys($behaviors))), + $xaction); + continue; + } + + $behavior = $behaviors[$key]; + $options = $behavior->getOptions(); + + $storage_key = HarbormasterBuildPlanBehavior::getStorageKeyForBehaviorKey( + $key); + $old = $object->getPlanProperty($storage_key); + $new = $xaction->getNewValue(); + + if ($old === $new) { + continue; + } + + if (!isset($options[$new])) { + $errors[] = $this->newInvalidError( + pht( + 'Value "%s" is not a valid option for behavior "%s". Valid '. + 'options are: %s.', + $new, + $key, + implode(', ', array_keys($options))), + $xaction); + continue; + } + } + + return $errors; + } + + public function getTransactionTypeForConduit($xaction) { + return 'behavior'; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array( + 'key' => $this->getBehaviorKeyForTransaction($xaction), + 'old' => $xaction->getOldValue(), + 'new' => $xaction->getNewValue(), + ); + } + + private function getBehaviorKeyForTransaction( + PhabricatorApplicationTransaction $xaction) { + $metadata_key = HarbormasterBuildPlanBehavior::getTransactionMetadataKey(); + return $xaction->getMetadataValue($metadata_key); + } + + private function getBehaviorKey() { + $metadata_key = HarbormasterBuildPlanBehavior::getTransactionMetadataKey(); + return $this->getMetadataValue($metadata_key); + } + + private function getBehavior() { + $behavior_key = $this->getBehaviorKey(); + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + return idx($behaviors, $behavior_key); + } + + private function getStorageKey() { + return HarbormasterBuildPlanBehavior::getStorageKeyForBehaviorKey( + $this->getBehaviorKey()); + } + +} From 983cf885e7ce2d3765cd248c9ae43979bd074dcf Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 11:00:20 -0800 Subject: [PATCH 29/43] Expose Build Plan behaviors via "harbormaster.buildplan.search" Summary: Ref T13258. This will support changing behaviors in "arc land". Test Plan: Called "harbormaster.buildplan.search", saw behavior information in results. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20228 --- .../configuration/HarbormasterBuildPlan.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index 8f8b0a20a8..faecb79a3a 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -233,15 +233,31 @@ final class HarbormasterBuildPlan extends HarbormasterDAO ->setKey('status') ->setType('map') ->setDescription(pht('The current status of this build plan.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('behaviors') + ->setType('map') + ->setDescription(pht('Behavior configuration for the build plan.')), ); } public function getFieldValuesForConduit() { + $behavior_map = array(); + + $behaviors = HarbormasterBuildPlanBehavior::newPlanBehaviors(); + foreach ($behaviors as $behavior) { + $option = $behavior->getPlanOption($this); + + $behavior_map[$behavior->getKey()] = array( + 'value' => $option->getKey(), + ); + } + return array( 'name' => $this->getName(), 'status' => array( 'value' => $this->getPlanStatus(), ), + 'behaviors' => $behavior_map, ); } From ee0ad4703ebdc47345ba220923ca42609ed7c3a3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 11:17:04 -0800 Subject: [PATCH 30/43] Make the new Build Plan "Runnable" behavior work Summary: Ref T13258. Fixes T11415. This makes "Runnable" actually do something: - With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan. - If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions. This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago. Test Plan: - Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user. - With "If Editable", unable to run, pause, resume, abort, or restart as another user. - With "If Viewable", those actions work. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258, T11415 Differential Revision: https://secure.phabricator.com/D20229 --- src/__phutil_library_map__.php | 3 ++ .../HarbormasterBuildPlanPolicyCodex.php | 38 ++++++++++++++++ .../HarbormasterPlanRunController.php | 7 +-- .../HarbormasterPlanViewController.php | 2 +- .../plan/HarbormasterBuildPlanBehavior.php | 28 +++++++++--- .../storage/build/HarbormasterBuild.php | 11 +++-- .../configuration/HarbormasterBuildPlan.php | 44 ++++++++++++++++++- 7 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 15c7637e3d..4eedb44b22 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1340,6 +1340,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanNameNgrams.php', 'HarbormasterBuildPlanNameTransaction' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php', 'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php', + 'HarbormasterBuildPlanPolicyCodex' => 'applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php', 'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php', 'HarbormasterBuildPlanSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php', 'HarbormasterBuildPlanSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php', @@ -6945,6 +6946,7 @@ phutil_register_library_map(array( 'PhabricatorNgramsInterface', 'PhabricatorConduitResultInterface', 'PhabricatorProjectInterface', + 'PhabricatorPolicyCodexInterface', ), 'HarbormasterBuildPlanBehavior' => 'Phobject', 'HarbormasterBuildPlanBehaviorOption' => 'Phobject', @@ -6958,6 +6960,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'PhabricatorSearchNgrams', 'HarbormasterBuildPlanNameTransaction' => 'HarbormasterBuildPlanTransactionType', 'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType', + 'HarbormasterBuildPlanPolicyCodex' => 'PhabricatorPolicyCodex', 'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildPlanSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'HarbormasterBuildPlanSearchEngine' => 'PhabricatorApplicationSearchEngine', diff --git a/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php b/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php new file mode 100644 index 0000000000..a17f2fb293 --- /dev/null +++ b/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php @@ -0,0 +1,38 @@ +getObject(); + $run_with_view = $object->canRunWithoutEditCapability(); + + $rules = array(); + + $rules[] = $this->newRule() + ->setCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->setIsActive(!$run_with_view) + ->setDescription( + pht( + 'You must have edit permission on this build plan to pause, '. + 'abort, resume, or restart it.')); + + $rules[] = $this->newRule() + ->setCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->setIsActive(!$run_with_view) + ->setDescription( + pht( + 'You must have edit permission on this build plan to run it '. + 'manually.')); + + return $rules; + } + + +} diff --git a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php index fd227ee554..5d80d421aa 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php @@ -9,16 +9,13 @@ final class HarbormasterPlanRunController extends HarbormasterPlanController { $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) ->withIDs(array($plan_id)) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) ->executeOne(); if (!$plan) { return new Aphront404Response(); } + $plan->assertHasRunCapability($viewer); + $cancel_uri = $this->getApplicationURI("plan/{$plan_id}/"); if (!$plan->canRunManually()) { diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 0ef6162aad..141f321caa 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -266,7 +266,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setIcon('fa-ban')); } - $can_run = ($can_edit && $plan->canRunManually()); + $can_run = ($plan->hasRunCapability($viewer) && $plan->canRunManually()); $curtain->addAction( id(new PhabricatorActionView()) diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index b0f722a1ca..893d351065 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -9,6 +9,10 @@ final class HarbormasterBuildPlanBehavior private $defaultKey; private $editInstructions; + const BEHAVIOR_RUNNABLE = 'runnable'; + const RUNNABLE_IF_VIEWABLE = 'view'; + const RUNNABLE_IF_EDITABLE = 'edit'; + public function setKey($key) { $this->key = $key; return $this; @@ -114,6 +118,19 @@ final class HarbormasterBuildPlanBehavior return sprintf('behavior.%s', $behavior_key); } + public static function getBehavior($key) { + $behaviors = self::newPlanBehaviors(); + + if (!isset($behaviors[$key])) { + throw new Exception( + pht( + 'No build plan behavior with key "%s" exists.', + $key)); + } + + return $behaviors[$key]; + } + public static function newPlanBehaviors() { $draft_options = array( id(new HarbormasterBuildPlanBehaviorOption()) @@ -224,14 +241,14 @@ final class HarbormasterBuildPlanBehavior $run_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('edit') + ->setKey(self::RUNNABLE_IF_EDITABLE) ->setIcon('fa-pencil green') ->setName(pht('If Editable')) ->setIsDefault(true) ->setDescription( pht('Only users who can edit the plan can run it manually.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('view') + ->setKey(self::RUNNABLE_IF_VIEWABLE) ->setIcon('fa-exclamation-triangle yellow') ->setName(pht('If Viewable')) ->setDescription( @@ -315,7 +332,7 @@ final class HarbormasterBuildPlanBehavior ->setName(pht('Restartable')) ->setOptions($restart_options), id(new self()) - ->setKey('runnable') + ->setKey(self::BEHAVIOR_RUNNABLE) ->setEditInstructions( pht( 'To run a build manually, you normally must have permission to '. @@ -323,9 +340,8 @@ final class HarbormasterBuildPlanBehavior 'can see the build plan be able to run and restart the build, you '. 'can change the behavior here.'. "\n\n". - 'Note that this affects both {nav Run Plan Manually} and '. - '{nav Restart Build}, since the two actions are largely '. - 'equivalent.'. + 'Note that this controls access to all build management actions: '. + '"Run Plan Manually", "Restart", "Abort", "Pause", and "Resume".'. "\n\n". 'WARNING: This may be unsafe, particularly if the build has '. 'side effects like deployment.'. diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 84668b79f0..9b7b64d06b 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -334,14 +334,17 @@ final class HarbormasterBuild extends HarbormasterDAO } public function assertCanIssueCommand(PhabricatorUser $viewer, $command) { - $need_edit = false; + $plan = $this->getBuildPlan(); + + $need_edit = true; switch ($command) { case HarbormasterBuildCommand::COMMAND_RESTART: - break; case HarbormasterBuildCommand::COMMAND_PAUSE: case HarbormasterBuildCommand::COMMAND_RESUME: case HarbormasterBuildCommand::COMMAND_ABORT: - $need_edit = true; + if ($plan->canRunWithoutEditCapability()) { + $need_edit = false; + } break; default: throw new Exception( @@ -355,7 +358,7 @@ final class HarbormasterBuild extends HarbormasterDAO if ($need_edit) { PhabricatorPolicyFilter::requireCapability( $viewer, - $this->getBuildPlan(), + $plan, PhabricatorPolicyCapability::CAN_EDIT); } } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index faecb79a3a..798201f490 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -10,7 +10,8 @@ final class HarbormasterBuildPlan extends HarbormasterDAO PhabricatorSubscribableInterface, PhabricatorNgramsInterface, PhabricatorConduitResultInterface, - PhabricatorProjectInterface { + PhabricatorProjectInterface, + PhabricatorPolicyCodexInterface { protected $name; protected $planStatus; @@ -133,7 +134,6 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return true; } - public function getName() { $autoplan = $this->getAutoplan(); if ($autoplan) { @@ -143,6 +143,38 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return parent::getName(); } + public function hasRunCapability(PhabricatorUser $viewer) { + try { + $this->assertHasRunCapability($viewer); + return true; + } catch (PhabricatorPolicyException $ex) { + return false; + } + } + + public function canRunWithoutEditCapability() { + $runnable = HarbormasterBuildPlanBehavior::BEHAVIOR_RUNNABLE; + $if_viewable = HarbormasterBuildPlanBehavior::RUNNABLE_IF_VIEWABLE; + + $option = HarbormasterBuildPlanBehavior::getBehavior($runnable) + ->getPlanOption($this); + + return ($option->getKey() === $if_viewable); + } + + public function assertHasRunCapability(PhabricatorUser $viewer) { + if ($this->canRunWithoutEditCapability()) { + $capability = PhabricatorPolicyCapability::CAN_VIEW; + } else { + $capability = PhabricatorPolicyCapability::CAN_EDIT; + } + + PhabricatorPolicyFilter::requireCapability( + $viewer, + $this, + $capability); + } + /* -( PhabricatorSubscribableInterface )----------------------------------- */ @@ -265,4 +297,12 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return array(); } + +/* -( PhabricatorPolicyCodexInterface )------------------------------------ */ + + + public function newPolicyCodex() { + return new HarbormasterBuildPlanPolicyCodex(); + } + } From 578de333dfa5a739fac9326563672689fea99538 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 12:03:28 -0800 Subject: [PATCH 31/43] Make the new Build Plan behavior "Restartable" work Summary: Ref T13258. Implements the "Restartable" behavior, to control whether a build may be restarted or not. This is fairly straightforward because there are already other existing reasons that a build may not be able to be restarted. Test Plan: Restarted a build. Marked it as not restartable, saw "Restart" action become disabled. Tried to restart it anyway, got a useful error message. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20230 --- .../controller/HarbormasterBuildActionController.php | 10 +++++++--- .../plan/HarbormasterBuildPlanBehavior.php | 10 +++++++--- .../harbormaster/storage/build/HarbormasterBuild.php | 5 +++++ .../storage/configuration/HarbormasterBuildPlan.php | 10 ++++++++++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php index 843ffd4702..b56c7de7f7 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php @@ -64,6 +64,11 @@ final class HarbormasterBuildActionController 'restart. Side effects of the build will occur again. Really '. 'restart build?'); $submit = pht('Restart Build'); + } else if (!$build->getBuildPlan()->canRestartBuildPlan()) { + $title = pht('Not Restartable'); + $body = pht( + 'The build plan for this build is not restartable, so you '. + 'can not restart the build.'); } else { $title = pht('Unable to Restart Build'); if ($build->isRestarting()) { @@ -135,8 +140,7 @@ final class HarbormasterBuildActionController break; } - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + $dialog = $this->newDialog() ->setTitle($title) ->appendChild($body) ->addCancelButton($return_uri); @@ -145,7 +149,7 @@ final class HarbormasterBuildActionController $dialog->addSubmitButton($submit); } - return id(new AphrontDialogResponse())->setDialog($dialog); + return $dialog; } } diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index 893d351065..66c186a0d1 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -13,6 +13,10 @@ final class HarbormasterBuildPlanBehavior const RUNNABLE_IF_VIEWABLE = 'view'; const RUNNABLE_IF_EDITABLE = 'edit'; + const BEHAVIOR_RESTARTABLE = 'restartable'; + const RESTARTABLE_ALWAYS = 'always'; + const RESTARTABLE_NEVER = 'never'; + public function setKey($key) { $this->key = $key; return $this; @@ -225,14 +229,14 @@ final class HarbormasterBuildPlanBehavior $restart_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('always') + ->setKey(self::RESTARTABLE_ALWAYS) ->setIcon('fa-repeat green') ->setName(pht('Always')) ->setIsDefault(true) ->setDescription( pht('The build may be restarted.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('never') + ->setKey(self::RESTARTABLE_NEVER) ->setIcon('fa-times red') ->setName(pht('Never')) ->setDescription( @@ -317,7 +321,7 @@ final class HarbormasterBuildPlanBehavior ->setName(pht('Affects Buildable')) ->setOptions($aggregate_options), id(new self()) - ->setKey('restartable') + ->setKey(self::BEHAVIOR_RESTARTABLE) ->setEditInstructions( pht( 'Usually, builds may be restarted. This may be useful if you '. diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 9b7b64d06b..063f81ff1e 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -215,6 +215,11 @@ final class HarbormasterBuild extends HarbormasterDAO return false; } + $plan = $this->getBuildPlan(); + if (!$plan->canRestartBuildPlan()) { + return false; + } + return !$this->isRestarting(); } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index 798201f490..efe62a6f84 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -175,6 +175,16 @@ final class HarbormasterBuildPlan extends HarbormasterDAO $capability); } + public function canRestartBuildPlan() { + $restartable = HarbormasterBuildPlanBehavior::BEHAVIOR_RESTARTABLE; + $is_restartable = HarbormasterBuildPlanBehavior::RESTARTABLE_ALWAYS; + + $option = HarbormasterBuildPlanBehavior::getBehavior($restartable) + ->getPlanOption($this); + + return ($option->getKey() === $is_restartable); + } + /* -( PhabricatorSubscribableInterface )----------------------------------- */ From 718cdc24471ace13a6f2c9e2d66e5d2019dcb436 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 13:02:14 -0800 Subject: [PATCH 32/43] Implement Build Plan "Hold Drafts" behavior Summary: Ref T13258. Makes the new "Hold Drafts" behavior actually work. Test Plan: - Created a build plan which does "Make HTTP Request" somewhere random and then waits for a message. - Created a Herald rule which "Always" runs this plan. - Created revisions, loaded them, then sent their build targets a "fail" message a short time later. - With "Always": Current behavior. Revision was held as a draft while building, and returned to me for changes when the build failed. - With "If Building": Revision was held as a draft while building, but promoted once the build failed. - With "Never": Revision promoted immediately, ignoring the build completely. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20232 --- .../storage/DifferentialRevision.php | 37 ++++++++++++++++++- .../plan/HarbormasterBuildPlanBehavior.php | 13 +++++-- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 3397f9cb03..a2a058568b 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -877,7 +877,7 @@ final class DifferentialRevision extends DifferentialDAO PhabricatorUser $viewer, array $phids) { - return id(new HarbormasterBuildQuery()) + $builds = id(new HarbormasterBuildQuery()) ->setViewer($viewer) ->withBuildablePHIDs($phids) ->withAutobuilds(false) @@ -893,6 +893,41 @@ final class DifferentialRevision extends DifferentialDAO HarbormasterBuildStatus::STATUS_DEADLOCKED, )) ->execute(); + + // Filter builds based on the "Hold Drafts" behavior of their associated + // build plans. + + $hold_drafts = HarbormasterBuildPlanBehavior::BEHAVIOR_DRAFTS; + $behavior = HarbormasterBuildPlanBehavior::getBehavior($hold_drafts); + + $key_never = HarbormasterBuildPlanBehavior::DRAFTS_NEVER; + $key_building = HarbormasterBuildPlanBehavior::DRAFTS_IF_BUILDING; + + foreach ($builds as $key => $build) { + $plan = $build->getBuildPlan(); + $hold_key = $behavior->getPlanOption($plan)->getKey(); + + $hold_never = ($hold_key === $key_never); + $hold_building = ($hold_key === $key_building); + + // If the build "Never" holds drafts from promoting, we don't care what + // the status is. + if ($hold_never) { + unset($builds[$key]); + continue; + } + + // If the build holds drafts from promoting "While Building", we only + // care about the status until it completes. + if ($hold_building) { + if ($build->isComplete()) { + unset($builds[$key]); + continue; + } + } + } + + return $builds; } diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index 66c186a0d1..63fc263fcb 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -17,6 +17,11 @@ final class HarbormasterBuildPlanBehavior const RESTARTABLE_ALWAYS = 'always'; const RESTARTABLE_NEVER = 'never'; + const BEHAVIOR_DRAFTS = 'hold-drafts'; + const DRAFTS_ALWAYS = 'always'; + const DRAFTS_IF_BUILDING = 'building'; + const DRAFTS_NEVER = 'never'; + public function setKey($key) { $this->key = $key; return $this; @@ -138,7 +143,7 @@ final class HarbormasterBuildPlanBehavior public static function newPlanBehaviors() { $draft_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('always') + ->setKey(self::DRAFTS_ALWAYS) ->setIcon('fa-check-circle-o green') ->setName(pht('Always')) ->setIsDefault(true) @@ -147,7 +152,7 @@ final class HarbormasterBuildPlanBehavior 'Revisions are not sent for review until the build completes, '. 'and are returned to the author for updates if the build fails.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('building') + ->setKey(self::DRAFTS_IF_BUILDING) ->setIcon('fa-pause-circle-o yellow') ->setName(pht('If Building')) ->setDescription( @@ -155,7 +160,7 @@ final class HarbormasterBuildPlanBehavior 'Revisions are not sent for review until the build completes, '. 'but they will be sent for review even if it fails.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('never') + ->setKey(self::DRAFTS_NEVER) ->setIcon('fa-circle-o red') ->setName(pht('Never')) ->setDescription( @@ -262,7 +267,7 @@ final class HarbormasterBuildPlanBehavior $behaviors = array( id(new self()) - ->setKey('hold-drafts') + ->setKey(self::BEHAVIOR_DRAFTS) ->setName(pht('Hold Drafts')) ->setEditInstructions( pht( From f97df9ebea90a240e66efa0ad7ac49c3de57086a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 13:19:21 -0800 Subject: [PATCH 33/43] Implement Build Plan behavior "Affects Buildable" Summary: Ref T13258. Make the "Affects Buildable" option actually work. Test Plan: - As in previous change, created a "wait for HTTP request" build plan and had it always run against every revision. - Created revisions, waited a bit, then sent the build a "Fail" message, with different values of "Affects Buildable": - "Always": Same behavior as today. Buildable waited for the build, then failed when it failed. - "While Building": Buildable waited for the build, but passed even though it failed (buildable has green checkmark even though build is red): {F6250359} - "Never": Buildable passed immediately (buildable has green checkmark even though build is still running): {F6250360} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20233 --- .../engine/HarbormasterBuildEngine.php | 24 +++++++++++++++++++ .../plan/HarbormasterBuildPlanBehavior.php | 13 ++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index 170e4c8a5c..447bd53704 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -497,9 +497,33 @@ final class HarbormasterBuildEngine extends Phobject { // passed everything it needs to. if (!$buildable->isPreparing()) { + $behavior_key = HarbormasterBuildPlanBehavior::BEHAVIOR_BUILDABLE; + $behavior = HarbormasterBuildPlanBehavior::getBehavior($behavior_key); + + $key_never = HarbormasterBuildPlanBehavior::BUILDABLE_NEVER; + $key_building = HarbormasterBuildPlanBehavior::BUILDABLE_IF_BUILDING; + $all_pass = true; $any_fail = false; foreach ($buildable->getBuilds() as $build) { + $plan = $build->getBuildPlan(); + $option = $behavior->getPlanOption($plan); + $option_key = $option->getKey(); + + $is_never = ($option_key === $key_never); + $is_building = ($option_key === $key_building); + + // If this build "Never" affects the buildable, ignore it. + if ($is_never) { + continue; + } + + // If this build affects the buildable "If Building", but is already + // complete, ignore it. + if ($is_building && $build->isComplete()) { + continue; + } + if (!$build->isPassed()) { $all_pass = false; } diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index 63fc263fcb..690619f56e 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -22,6 +22,11 @@ final class HarbormasterBuildPlanBehavior const DRAFTS_IF_BUILDING = 'building'; const DRAFTS_NEVER = 'never'; + const BEHAVIOR_BUILDABLE = 'buildable'; + const BUILDABLE_ALWAYS = 'always'; + const BUILDABLE_IF_BUILDING = 'building'; + const BUILDABLE_NEVER = 'never'; + public function setKey($key) { $this->key = $key; return $this; @@ -207,7 +212,7 @@ final class HarbormasterBuildPlanBehavior $aggregate_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('always') + ->setKey(self::BUILDABLE_ALWAYS) ->setIcon('fa-check-circle-o green') ->setName(pht('Always')) ->setIsDefault(true) @@ -216,7 +221,7 @@ final class HarbormasterBuildPlanBehavior 'The buildable waits for the build, and fails if the '. 'build fails.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('building') + ->setKey(self::BUILDABLE_IF_BUILDING) ->setIcon('fa-pause-circle-o yellow') ->setName(pht('If Building')) ->setDescription( @@ -224,7 +229,7 @@ final class HarbormasterBuildPlanBehavior 'The buildable waits for the build, but does not fail '. 'if the build fails.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('never') + ->setKey(self::BUILDABLE_NEVER) ->setIcon('fa-circle-o red') ->setName(pht('Never')) ->setDescription( @@ -310,7 +315,7 @@ final class HarbormasterBuildPlanBehavior 'this warning and continue, even if builds have failed.')) ->setOptions($land_options), id(new self()) - ->setKey('buildable') + ->setKey(self::BEHAVIOR_BUILDABLE) ->setEditInstructions( pht( 'The overall state of a buildable (like a commit or revision) is '. From 7e468123446742f7b398d55cca525eb53d475b0c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Mar 2019 07:45:04 -0800 Subject: [PATCH 34/43] Add a warning to revision timelines when changes land with ongoing or failed builds Summary: Ref T13258. The general idea here is "if arc land prompted you and you hit 'y', you get a warning about it on the timeline". This is similar to the existing warning about landing revisions in the wrong state and hitting "y" to get through that. See D18808, previously. These warnings make it easier to catch process issues at a glance, especially because the overall build status is now more complicated (and may legally include some failures on tests which are marked as unimportant). The transaction stores which builds had problems, but I'm not doing anything to render that for now. I think you can usually figure it out from the UI already; if not, we could refine this. Test Plan: - Used `bin/differential attach-commit` to trigger extraction/attachment. - Attached a commit to a revision with various build states, and various build plan "Warn When Landing" flags. - Got sensible warnings and non-warnings based on "Warn When Landing" setting. {F6251631} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20239 --- src/__phutil_library_map__.php | 2 + .../DifferentialDiffExtractionEngine.php | 95 +++++++++++++++++++ ...erentialRevisionWrongBuildsTransaction.php | 37 ++++++++ .../plan/HarbormasterBuildPlanBehavior.php | 21 ++-- 4 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4eedb44b22..48cf3f76fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -653,6 +653,7 @@ phutil_register_library_map(array( 'DifferentialRevisionUpdateTransaction' => 'applications/differential/xaction/DifferentialRevisionUpdateTransaction.php', 'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php', 'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php', + 'DifferentialRevisionWrongBuildsTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php', 'DifferentialRevisionWrongStateTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php', 'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php', 'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php', @@ -6181,6 +6182,7 @@ phutil_register_library_map(array( 'DifferentialRevisionUpdateTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionViewController' => 'DifferentialController', 'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType', + 'DifferentialRevisionWrongBuildsTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index 861d2ad220..7b94b1958b 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -285,6 +285,24 @@ final class DifferentialDiffExtractionEngine extends Phobject { ->setNewValue($revision->getModernRevisionStatus()); } + $concerning_builds = $this->loadConcerningBuilds($revision); + if ($concerning_builds) { + $build_list = array(); + foreach ($concerning_builds as $build) { + $build_list[] = array( + 'phid' => $build->getPHID(), + 'status' => $build->getBuildStatus(), + ); + } + + $wrong_builds = + DifferentialRevisionWrongBuildsTransaction::TRANSACTIONTYPE; + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType($wrong_builds) + ->setNewValue($build_list); + } + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; $xactions[] = id(new DifferentialTransaction()) @@ -322,4 +340,81 @@ final class DifferentialDiffExtractionEngine extends Phobject { return $result_data; } + private function loadConcerningBuilds(DifferentialRevision $revision) { + $viewer = $this->getViewer(); + $diff = $revision->getActiveDiff(); + + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(array($diff->getPHID())) + ->needBuilds(true) + ->withManualBuildables(false) + ->execute(); + if (!$buildables) { + return array(); + } + + + $land_key = HarbormasterBuildPlanBehavior::BEHAVIOR_LANDWARNING; + $behavior = HarbormasterBuildPlanBehavior::getBehavior($land_key); + + $key_never = HarbormasterBuildPlanBehavior::LANDWARNING_NEVER; + $key_building = HarbormasterBuildPlanBehavior::LANDWARNING_IF_BUILDING; + $key_complete = HarbormasterBuildPlanBehavior::LANDWARNING_IF_COMPLETE; + + $concerning_builds = array(); + foreach ($buildables as $buildable) { + $builds = $buildable->getBuilds(); + foreach ($builds as $build) { + $plan = $build->getBuildPlan(); + $option = $behavior->getPlanOption($plan); + $behavior_value = $option->getKey(); + + $if_never = ($behavior_value === $key_never); + if ($if_never) { + continue; + } + + $if_building = ($behavior_value === $key_building); + if ($if_building && $build->isComplete()) { + continue; + } + + $if_complete = ($behavior_value === $key_complete); + if ($if_complete) { + if (!$build->isComplete()) { + continue; + } + + // TODO: If you "arc land" and a build with "Warn: If Complete" + // is still running, you may not see a warning, and push the revision + // in good faith. The build may then complete before we get here, so + // we now see a completed, failed build. + + // For now, just err on the side of caution and assume these builds + // were in a good state when we prompted the user, even if they're in + // a bad state now. + + // We could refine this with a rule like "if the build finished + // within a couple of minutes before the push happened, assume it was + // in good faith", but we don't currently have an especially + // convenient way to check when the build finished or when the commit + // was pushed or discovered, and this would create some issues in + // cases where the repository is observed and the fetch pipeline + // stalls for a while. + + continue; + } + + if ($build->isPassed()) { + continue; + } + + $concerning_builds[] = $build; + } + } + + return $concerning_builds; + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php new file mode 100644 index 0000000000..260813b75b --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php @@ -0,0 +1,37 @@ +key = $key; return $this; @@ -176,7 +182,7 @@ final class HarbormasterBuildPlanBehavior $land_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('always') + ->setKey(self::LANDWARNING_ALWAYS) ->setIcon('fa-check-circle-o green') ->setName(pht('Always')) ->setIsDefault(true) @@ -185,7 +191,7 @@ final class HarbormasterBuildPlanBehavior '"arc land" warns if the build is still running or has '. 'failed.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('building') + ->setKey(self::LANDWARNING_IF_BUILDING) ->setIcon('fa-pause-circle-o yellow') ->setName(pht('If Building')) ->setDescription( @@ -193,7 +199,7 @@ final class HarbormasterBuildPlanBehavior '"arc land" warns if the build is still running, but ignores '. 'the build if it has failed.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('complete') + ->setKey(self::LANDWARNING_IF_COMPLETE) ->setIcon('fa-dot-circle-o yellow') ->setName(pht('If Complete')) ->setDescription( @@ -201,7 +207,7 @@ final class HarbormasterBuildPlanBehavior '"arc land" warns if the build has failed, but ignores the '. 'build if it is still running.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('never') + ->setKey(self::LANDWARNING_NEVER) ->setIcon('fa-circle-o red') ->setName(pht('Never')) ->setDescription( @@ -296,7 +302,7 @@ final class HarbormasterBuildPlanBehavior 'revision, even if builds have failed or are still in progress.')) ->setOptions($draft_options), id(new self()) - ->setKey('arc-land') + ->setKey(self::BEHAVIOR_LANDWARNING) ->setName(pht('Warn When Landing')) ->setEditInstructions( pht( @@ -312,7 +318,10 @@ final class HarbormasterBuildPlanBehavior 'for it) or the outcome is not important.'. "\n\n". 'This warning is only advisory. Users may always elect to ignore '. - 'this warning and continue, even if builds have failed.')) + 'this warning and continue, even if builds have failed.'. + "\n\n". + 'This setting also affects the warning that is published to '. + 'revisions when commits land with ongoing or failed builds.')) ->setOptions($land_options), id(new self()) ->setKey(self::BEHAVIOR_BUILDABLE) From a3ebaac0f026411268ca1dd2856b6762acebcc23 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Mar 2019 08:30:18 -0800 Subject: [PATCH 35/43] Tweak the visual style of the ">>" / "<<" depth change indicators slightly Summary: Ref T13249. - When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes. - Move the markers slightly to the left, to align them with the gutter. - Make them slightly opaque so they're a little less prominent. Test Plan: See screenshots. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20251 --- resources/celerity/map.php | 12 ++++----- .../DifferentialChangesetTwoUpRenderer.php | 26 +++++++++++++------ .../differential/changeset-view.css | 3 +++ 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 11e7fe4118..7f9aaa4995 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', 'core.pkg.js' => '2cda17a4', - 'differential.pkg.css' => 'ab23bd75', + 'differential.pkg.css' => '1755a478', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => 'd92bed0d', + 'rsrc/css/application/differential/changeset-view.css' => '4193eeff', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -540,7 +540,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => 'd92bed0d', + 'differential-changeset-view-css' => '4193eeff', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1220,6 +1220,9 @@ return array( 'javelin-behavior', 'javelin-uri', ), + '4193eeff' => array( + 'phui-inline-comment-view-css', + ), '4234f572' => array( 'syntax-default-css', ), @@ -1997,9 +2000,6 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), - 'd92bed0d' => array( - 'phui-inline-comment-view-css', - ), 'da15d3dc' => array( 'phui-oi-list-view-css', ), diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 7efd29519e..d803e92c6c 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -162,7 +162,20 @@ final class DifferentialChangesetTwoUpRenderer } else if (empty($new_lines[$ii])) { $o_class = 'old old-full'; } else { - $o_class = 'old'; + if (isset($depth_only[$ii])) { + if ($depth_only[$ii] == '>') { + // When a line has depth-only change, we only highlight the + // left side of the diff if the depth is decreasing. When the + // depth is increasing, the ">>" marker on the right hand side + // of the diff generally provides enough visibility on its own. + + $o_class = ''; + } else { + $o_class = 'old'; + } + } else { + $o_class = 'old'; + } } $o_classes = $o_class; } @@ -200,13 +213,10 @@ final class DifferentialChangesetTwoUpRenderer } else if (empty($old_lines[$ii])) { $n_class = 'new new-full'; } else { - - // NOTE: At least for the moment, I'm intentionally clearing the - // line highlighting only on the right side of the diff when a - // line has only depth changes. When a block depth is decreased, - // this gives us a large color block on the left (to make it easy - // to see the depth change) but a clean diff on the right (to make - // it easy to pick out actual code changes). + // When a line has a depth-only change, never highlight it on + // the right side. The ">>" marker generally provides enough + // visibility on its own for indent depth increases, and the left + // side is still highlighted for indent depth decreases. if (isset($depth_only[$ii])) { $n_class = ''; diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 6ed939a2ee..844690abd3 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -126,6 +126,9 @@ background-size: 12px 12px; background-repeat: no-repeat; background-position: left center; + position: relative; + left: -8px; + opacity: 0.5; } .differential-diff td span.depth-out { From 9918ea1fb7caa6e8abdbc286eccdc329212f2f75 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Mar 2019 16:22:52 -0800 Subject: [PATCH 36/43] Fix an exception with user cache generation in "bin/conduit call --as " Summary: Ref T13249. Using "--as" to call some Conduit methods as a user can currently fatal when trying to access settings/preferences. Allow inline regeneration of user caches. Test Plan: Called `project.edit` to add a member. Before: constructing a policy field tried to access the user's preferences and failed. After: Smooth sailing. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20255 --- .../management/PhabricatorConduitCallManagementWorkflow.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php index f9ba48b372..dc241a04b4 100644 --- a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php +++ b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php @@ -58,6 +58,10 @@ final class PhabricatorConduitCallManagementWorkflow 'No such user "%s" exists.', $as)); } + + // Allow inline generation of user caches for the user we're acting + // as, since some calls may read user preferences. + $actor->setAllowInlineCacheGeneration(true); } else { $actor = $viewer; } From bacf1f44e00f33f5e076363b36c7fd86f544540a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 04:48:41 -0800 Subject: [PATCH 37/43] Modularize HeraldRule transactions Summary: Ref T13249. See PHI1115. I initially wanted to make `bin/policy unlock --owner H123` work to transfer ownership of a Herald rule, although I'm no longer really sure this makes much sense. In any case, this makes things a little better and more modern. I removed the storage table for rule comments. Adding comments to Herald rules doesn't work and probably doesn't make much sense. Test Plan: Created and edited Herald rules, grepped for all the transaction type constants. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20258 --- .../20190307.herald.01.comments.sql | 1 + src/__phutil_library_map__.php | 12 +- .../controller/HeraldDisableController.php | 2 +- .../controller/HeraldRuleController.php | 14 +- .../herald/editor/HeraldRuleEditor.php | 77 ----------- .../herald/storage/HeraldRuleTransaction.php | 121 +----------------- .../storage/HeraldRuleTransactionComment.php | 10 -- .../xaction/HeraldRuleDisableTransaction.php | 32 +++++ .../xaction/HeraldRuleEditTransaction.php | 56 ++++++++ .../xaction/HeraldRuleNameTransaction.php | 48 +++++++ .../xaction/HeraldRuleTransactionType.php | 4 + 11 files changed, 166 insertions(+), 211 deletions(-) create mode 100644 resources/sql/autopatches/20190307.herald.01.comments.sql delete mode 100644 src/applications/herald/storage/HeraldRuleTransactionComment.php create mode 100644 src/applications/herald/xaction/HeraldRuleDisableTransaction.php create mode 100644 src/applications/herald/xaction/HeraldRuleEditTransaction.php create mode 100644 src/applications/herald/xaction/HeraldRuleNameTransaction.php create mode 100644 src/applications/herald/xaction/HeraldRuleTransactionType.php diff --git a/resources/sql/autopatches/20190307.herald.01.comments.sql b/resources/sql/autopatches/20190307.herald.01.comments.sql new file mode 100644 index 0000000000..ff9cb9af88 --- /dev/null +++ b/resources/sql/autopatches/20190307.herald.01.comments.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_herald.herald_ruletransaction_comment; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 48cf3f76fe..6d0b93b1be 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1535,10 +1535,13 @@ phutil_register_library_map(array( 'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', 'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php', + 'HeraldRuleDisableTransaction' => 'applications/herald/xaction/HeraldRuleDisableTransaction.php', + 'HeraldRuleEditTransaction' => 'applications/herald/xaction/HeraldRuleEditTransaction.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', + 'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', 'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php', @@ -1546,7 +1549,7 @@ phutil_register_library_map(array( 'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php', 'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php', 'HeraldRuleTransaction' => 'applications/herald/storage/HeraldRuleTransaction.php', - 'HeraldRuleTransactionComment' => 'applications/herald/storage/HeraldRuleTransactionComment.php', + 'HeraldRuleTransactionType' => 'applications/herald/xaction/HeraldRuleTransactionType.php', 'HeraldRuleTranscript' => 'applications/herald/storage/transcript/HeraldRuleTranscript.php', 'HeraldRuleTypeConfig' => 'applications/herald/config/HeraldRuleTypeConfig.php', 'HeraldRuleTypeDatasource' => 'applications/herald/typeahead/HeraldRuleTypeDatasource.php', @@ -7188,18 +7191,21 @@ phutil_register_library_map(array( 'HeraldRuleAdapterField' => 'HeraldRuleField', 'HeraldRuleController' => 'HeraldController', 'HeraldRuleDatasource' => 'PhabricatorTypeaheadDatasource', + 'HeraldRuleDisableTransaction' => 'HeraldRuleTransactionType', + 'HeraldRuleEditTransaction' => 'HeraldRuleTransactionType', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleField' => 'HeraldField', 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', 'HeraldRuleListController' => 'HeraldController', + 'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HeraldRuleSerializer' => 'Phobject', 'HeraldRuleTestCase' => 'PhabricatorTestCase', - 'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction', - 'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment', + 'HeraldRuleTransaction' => 'PhabricatorModularTransaction', + 'HeraldRuleTransactionType' => 'PhabricatorModularTransactionType', 'HeraldRuleTranscript' => 'Phobject', 'HeraldRuleTypeConfig' => 'Phobject', 'HeraldRuleTypeDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/herald/controller/HeraldDisableController.php b/src/applications/herald/controller/HeraldDisableController.php index def87049f7..765237930c 100644 --- a/src/applications/herald/controller/HeraldDisableController.php +++ b/src/applications/herald/controller/HeraldDisableController.php @@ -31,7 +31,7 @@ final class HeraldDisableController extends HeraldController { if ($request->isFormPost()) { $xaction = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_DISABLE) + ->setTransactionType(HeraldRuleDisableTransaction::TRANSACTIONTYPE) ->setNewValue($is_disable); id(new HeraldRuleEditor()) diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index d400f8ae90..d05ed2d525 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -359,11 +359,21 @@ final class HeraldRuleController extends HeraldController { $repetition_policy); $xactions = array(); + + // Until this moves to EditEngine, manually add a "CREATE" transaction + // if we're creating a new rule. This improves rendering of the initial + // group of transactions. + $is_new = (bool)(!$rule->getID()); + if ($is_new) { + $xactions[] = id(new HeraldRuleTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_CREATE); + } + $xactions[] = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_EDIT) + ->setTransactionType(HeraldRuleEditTransaction::TRANSACTIONTYPE) ->setNewValue($new_state); $xactions[] = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_NAME) + ->setTransactionType(HeraldRuleNameTransaction::TRANSACTIONTYPE) ->setNewValue($new_name); try { diff --git a/src/applications/herald/editor/HeraldRuleEditor.php b/src/applications/herald/editor/HeraldRuleEditor.php index 3ba5c4f8ac..8bc3224c77 100644 --- a/src/applications/herald/editor/HeraldRuleEditor.php +++ b/src/applications/herald/editor/HeraldRuleEditor.php @@ -11,82 +11,6 @@ final class HeraldRuleEditor return pht('Herald Rules'); } - public function getTransactionTypes() { - $types = parent::getTransactionTypes(); - - $types[] = PhabricatorTransactions::TYPE_COMMENT; - $types[] = HeraldRuleTransaction::TYPE_EDIT; - $types[] = HeraldRuleTransaction::TYPE_NAME; - $types[] = HeraldRuleTransaction::TYPE_DISABLE; - - return $types; - } - - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return (int)$object->getIsDisabled(); - case HeraldRuleTransaction::TYPE_EDIT: - return id(new HeraldRuleSerializer()) - ->serializeRule($object); - case HeraldRuleTransaction::TYPE_NAME: - return $object->getName(); - } - - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return (int)$xaction->getNewValue(); - case HeraldRuleTransaction::TYPE_EDIT: - case HeraldRuleTransaction::TYPE_NAME: - return $xaction->getNewValue(); - } - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return $object->setIsDisabled($xaction->getNewValue()); - case HeraldRuleTransaction::TYPE_NAME: - return $object->setName($xaction->getNewValue()); - case HeraldRuleTransaction::TYPE_EDIT: - $new_state = id(new HeraldRuleSerializer()) - ->deserializeRuleComponents($xaction->getNewValue()); - $object->setMustMatchAll((int)$new_state['match_all']); - $object->attachConditions($new_state['conditions']); - $object->attachActions($new_state['actions']); - - $new_repetition = $new_state['repetition_policy']; - $object->setRepetitionPolicyStringConstant($new_repetition); - - return $object; - } - - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_EDIT: - $object->saveConditions($object->getConditions()); - $object->saveActions($object->getActions()); - break; - } - return; - } - protected function shouldApplyHeraldRules( PhabricatorLiskDAO $object, array $xactions) { @@ -137,7 +61,6 @@ final class HeraldRuleEditor return pht('[Herald]'); } - protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/herald/storage/HeraldRuleTransaction.php b/src/applications/herald/storage/HeraldRuleTransaction.php index b1bd563749..7fa7667ec7 100644 --- a/src/applications/herald/storage/HeraldRuleTransaction.php +++ b/src/applications/herald/storage/HeraldRuleTransaction.php @@ -1,11 +1,9 @@ getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return 'red'; - } else { - return 'green'; - } - } - - return parent::getColor(); - } - - public function getActionName() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return pht('Disabled'); - } else { - return pht('Enabled'); - } - case self::TYPE_NAME: - return pht('Renamed'); - } - - return parent::getActionName(); - } - - public function getIcon() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return 'fa-ban'; - } else { - return 'fa-check'; - } - } - - return parent::getIcon(); - } - - - public function getTitle() { - $author_phid = $this->getAuthorPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return pht( - '%s disabled this rule.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s enabled this rule.', - $this->renderHandleLink($author_phid)); - } - case self::TYPE_NAME: - if ($old == null) { - return pht( - '%s created this rule.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s renamed this rule from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $old, - $new); - } - case self::TYPE_EDIT: - return pht( - '%s edited this rule.', - $this->renderHandleLink($author_phid)); - } - - return parent::getTitle(); - } - - public function hasChangeDetails() { - switch ($this->getTransactionType()) { - case self::TYPE_EDIT: - return true; - } - return parent::hasChangeDetails(); - } - - public function renderChangeDetails(PhabricatorUser $viewer) { - $json = new PhutilJSON(); - switch ($this->getTransactionType()) { - case self::TYPE_EDIT: - return $this->renderTextCorpusChangeDetails( - $viewer, - $json->encodeFormatted($this->getOldValue()), - $json->encodeFormatted($this->getNewValue())); - } - - return $this->renderTextCorpusChangeDetails( - $viewer, - $this->getOldValue(), - $this->getNewValue()); + public function getBaseTransactionClass() { + return 'HeraldRuleTransactionType'; } } diff --git a/src/applications/herald/storage/HeraldRuleTransactionComment.php b/src/applications/herald/storage/HeraldRuleTransactionComment.php deleted file mode 100644 index 56022ef863..0000000000 --- a/src/applications/herald/storage/HeraldRuleTransactionComment.php +++ /dev/null @@ -1,10 +0,0 @@ -getIsDisabled(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + } + + public function getTitle() { + if ($this->getNewValue()) { + return pht( + '%s disabled this rule.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this rule.', + $this->renderAuthor()); + } + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleEditTransaction.php b/src/applications/herald/xaction/HeraldRuleEditTransaction.php new file mode 100644 index 0000000000..c4b03983fb --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleEditTransaction.php @@ -0,0 +1,56 @@ +serializeRule($object); + } + + public function applyInternalEffects($object, $value) { + $new_state = id(new HeraldRuleSerializer()) + ->deserializeRuleComponents($value); + + $object->setMustMatchAll((int)$new_state['match_all']); + $object->attachConditions($new_state['conditions']); + $object->attachActions($new_state['actions']); + + $new_repetition = $new_state['repetition_policy']; + $object->setRepetitionPolicyStringConstant($new_repetition); + } + + public function applyExternalEffects($object, $value) { + $object->saveConditions($object->getConditions()); + $object->saveActions($object->getActions()); + } + + public function getTitle() { + return pht( + '%s edited this rule.', + $this->renderAuthor()); + } + + public function hasChangeDetailView() { + return true; + } + + public function newChangeDetailView() { + $viewer = $this->getViewer(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $json = new PhutilJSON(); + $old_json = $json->encodeFormatted($old); + $new_json = $json->encodeFormatted($new); + + return id(new PhabricatorApplicationTransactionTextDiffDetailView()) + ->setViewer($viewer) + ->setOldText($old_json) + ->setNewText($new_json); + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleNameTransaction.php b/src/applications/herald/xaction/HeraldRuleNameTransaction.php new file mode 100644 index 0000000000..39ce289d34 --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleNameTransaction.php @@ -0,0 +1,48 @@ +getName(); + } + + public function applyInternalEffects($object, $value) { + $object->setName($value); + } + + public function getTitle() { + return pht( + '%s renamed this rule from %s to %s.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { + $errors[] = $this->newRequiredError( + pht('Rules must have a name.')); + } + + $max_length = $object->getColumnMaximumByteLength('name'); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + $new_length = strlen($new_value); + if ($new_length > $max_length) { + $errors[] = $this->newInvalidError( + pht( + 'Rule names can be no longer than %s characters.', + new PhutilNumber($max_length))); + } + } + + return $errors; + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleTransactionType.php b/src/applications/herald/xaction/HeraldRuleTransactionType.php new file mode 100644 index 0000000000..81c6846b1f --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleTransactionType.php @@ -0,0 +1,4 @@ + Date: Wed, 6 Mar 2019 20:21:51 -0800 Subject: [PATCH 38/43] Update "bin/policy unlock" to be more surgical, flexible, modular, and modern Summary: See PHI1115. Ref T13249. Currently, you can `bin/policy unlock` objects which have become inaccessible through some sort of policy mistake. This script uses a very blunt mechanism to perform unlocks: just manually calling `setXPolicy()` and then trying to `save()` the object. Improve things a bit: - More surgical: allow selection of which policies you want to adjust with "--view", "--edit", and "--owner" (potentially important for some objects like Herald rules which don't have policies, and "edit-locked" tasks which basically ignore the edit policy). - More flexible: Instead of unlocking into "All Users" (which could be bad for stuff like Passphrase credentials, since you create a short window where anyone can access them), take a username as a parameter and set the policy to "just that user". Normally, you'd run this as `bin/policy unlock --view myself --edit myself` or similar, now. - More modular: We can't do "owner" transactions in a generic way, but lay the groundwork for letting applications support providing an owner reassignment mechanism. - More modern: Use transactions, not raw `set()` + `save()`. This previously had some hard-coded logic around unlocking applications. I've removed it, and the new generic stuff doesn't actually work. It probably should be made to work at some point, but I believe it's exceptionally difficult to lock yourself out of applications, and you can unlock them with `bin/config set phabricator.application-settings ...` anyway so I'm not too worried about this. It's also hard to figure out the PHID of an application and no one has ever asked about this so I'd guess the reasonable use rate of `bin/policy unlock` to unlock applications in the wild may be zero. Test Plan: - Used `bin/policy unlock` to unlock some objects, saw sensible transactions. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20256 --- src/__phutil_library_map__.php | 4 + ...bricatorPolicyManagementUnlockWorkflow.php | 175 ++++++++++-------- .../engine/PhabricatorDefaultUnlockEngine.php | 4 + .../system/engine/PhabricatorUnlockEngine.php | 75 ++++++++ 4 files changed, 181 insertions(+), 77 deletions(-) create mode 100644 src/applications/system/engine/PhabricatorDefaultUnlockEngine.php create mode 100644 src/applications/system/engine/PhabricatorUnlockEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6d0b93b1be..a2b9caf3ab 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2974,6 +2974,7 @@ phutil_register_library_map(array( 'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php', 'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php', 'PhabricatorDefaultSyntaxStyle' => 'infrastructure/syntax/PhabricatorDefaultSyntaxStyle.php', + 'PhabricatorDefaultUnlockEngine' => 'applications/system/engine/PhabricatorDefaultUnlockEngine.php', 'PhabricatorDestructibleCodex' => 'applications/system/codex/PhabricatorDestructibleCodex.php', 'PhabricatorDestructibleCodexInterface' => 'applications/system/interface/PhabricatorDestructibleCodexInterface.php', 'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php', @@ -4701,6 +4702,7 @@ phutil_register_library_map(array( 'PhabricatorUnitTestContentSource' => 'infrastructure/contentsource/PhabricatorUnitTestContentSource.php', 'PhabricatorUnitsTestCase' => 'view/__tests__/PhabricatorUnitsTestCase.php', 'PhabricatorUnknownContentSource' => 'infrastructure/contentsource/PhabricatorUnknownContentSource.php', + 'PhabricatorUnlockEngine' => 'applications/system/engine/PhabricatorUnlockEngine.php', 'PhabricatorUnsubscribedFromObjectEdgeType' => 'applications/transactions/edges/PhabricatorUnsubscribedFromObjectEdgeType.php', 'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php', 'PhabricatorUserApproveTransaction' => 'applications/people/xaction/PhabricatorUserApproveTransaction.php', @@ -8871,6 +8873,7 @@ phutil_register_library_map(array( 'PhabricatorDebugController' => 'PhabricatorController', 'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorDefaultSyntaxStyle' => 'PhabricatorSyntaxStyle', + 'PhabricatorDefaultUnlockEngine' => 'PhabricatorUnlockEngine', 'PhabricatorDestructibleCodex' => 'Phobject', 'PhabricatorDestructionEngine' => 'Phobject', 'PhabricatorDestructionEngineExtension' => 'Phobject', @@ -10881,6 +10884,7 @@ phutil_register_library_map(array( 'PhabricatorUnitTestContentSource' => 'PhabricatorContentSource', 'PhabricatorUnitsTestCase' => 'PhabricatorTestCase', 'PhabricatorUnknownContentSource' => 'PhabricatorContentSource', + 'PhabricatorUnlockEngine' => 'Phobject', 'PhabricatorUnsubscribedFromObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorUser' => array( 'PhabricatorUserDAO', diff --git a/src/applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php b/src/applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php index 33f7e209c2..64a32b7186 100644 --- a/src/applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php +++ b/src/applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php @@ -8,40 +8,72 @@ final class PhabricatorPolicyManagementUnlockWorkflow ->setName('unlock') ->setSynopsis( pht( - 'Unlock an object by setting its policies to allow anyone to view '. - 'and edit it.')) - ->setExamples('**unlock** D123') + 'Unlock an object which has policies that prevent it from being '. + 'viewed or edited.')) + ->setExamples('**unlock** --view __user__ __object__') ->setArguments( array( array( - 'name' => 'objects', - 'wildcard' => true, + 'name' => 'view', + 'param' => 'username', + 'help' => pht( + 'Change the view policy of an object so that the specified '. + 'user may view it.'), + ), + array( + 'name' => 'edit', + 'param' => 'username', + 'help' => pht( + 'Change the edit policy of an object so that the specified '. + 'user may edit it.'), + ), + array( + 'name' => 'owner', + 'param' => 'username', + 'help' => pht( + 'Change the owner of an object to the specified user.'), + ), + array( + 'name' => 'objects', + 'wildcard' => true, ), )); } public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); $viewer = $this->getViewer(); - $obj_names = $args->getArg('objects'); - if (!$obj_names) { + $object_names = $args->getArg('objects'); + if (!$object_names) { throw new PhutilArgumentUsageException( pht('Specify the name of an object to unlock.')); - } else if (count($obj_names) > 1) { + } else if (count($object_names) > 1) { throw new PhutilArgumentUsageException( pht('Specify the name of exactly one object to unlock.')); } + $object_name = head($object_names); + $object = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->withNames($obj_names) + ->withNames(array($object_name)) ->executeOne(); - if (!$object) { - $name = head($obj_names); throw new PhutilArgumentUsageException( - pht("No such object '%s'!", $name)); + pht( + 'Unable to find any object with the specified name ("%s").', + $object_name)); + } + + $view_user = $this->loadUser($args->getArg('view')); + $edit_user = $this->loadUser($args->getArg('edit')); + $owner_user = $this->loadUser($args->getArg('owner')); + + if (!$view_user && !$edit_user && !$owner_user) { + throw new PhutilArgumentUsageException( + pht( + 'Choose which capabilities to unlock with "--view", "--edit", '. + 'or "--owner".')); } $handle = id(new PhabricatorHandleQuery()) @@ -49,84 +81,73 @@ final class PhabricatorPolicyManagementUnlockWorkflow ->withPHIDs(array($object->getPHID())) ->executeOne(); - if ($object instanceof PhabricatorApplication) { - $application = $object; + echo tsprintf( + "** %s ** %s\n", + pht('UNLOCKING'), + pht('Unlocking: %s', $handle->getFullName())); - $console->writeOut( - "%s\n", - pht('Unlocking Application: %s', $handle->getFullName())); + $engine = PhabricatorUnlockEngine::newUnlockEngineForObject($object); - // For applications, we can't unlock them in a normal way and don't want - // to unlock every capability, just view and edit. - $capabilities = array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - ); + $xactions = array(); + if ($view_user) { + $xactions[] = $engine->newUnlockViewTransactions($object, $view_user); + } + if ($edit_user) { + $xactions[] = $engine->newUnlockEditTransactions($object, $edit_user); + } + if ($owner_user) { + $xactions[] = $engine->newUnlockOwnerTransactions($object, $owner_user); + } + $xactions = array_mergev($xactions); - $key = 'phabricator.application-settings'; - $config_entry = PhabricatorConfigEntry::loadConfigEntry($key); - $value = $config_entry->getValue(); + $policy_application = new PhabricatorPolicyApplication(); + $content_source = $this->newContentSource(); - foreach ($capabilities as $capability) { - if ($application->isCapabilityEditable($capability)) { - unset($value[$application->getPHID()]['policy'][$capability]); - } - } + $editor = $object->getApplicationTransactionEditor() + ->setActor($viewer) + ->setActingAsPHID($policy_application->getPHID()) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) + ->setContentSource($content_source); - $config_entry->setValue($value); - $config_entry->save(); + $editor->applyTransactions($object, $xactions); - $console->writeOut("%s\n", pht('Saved application.')); + echo tsprintf( + "** %s ** %s\n", + pht('UNLOCKED'), + pht('Modified object policies.')); - return 0; + $uri = $handle->getURI(); + if (strlen($uri)) { + echo tsprintf( + "\n **%s**: __%s__\n\n", + pht('Object URI'), + PhabricatorEnv::getURI($uri)); } - $console->writeOut("%s\n", pht('Unlocking: %s', $handle->getFullName())); + return 0; + } - $updated = false; - foreach ($object->getCapabilities() as $capability) { - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - try { - $object->setViewPolicy(PhabricatorPolicies::POLICY_USER); - $console->writeOut("%s\n", pht('Unlocked view policy.')); - $updated = true; - } catch (Exception $ex) { - $console->writeOut("%s\n", pht('View policy is not mutable.')); - } - break; - case PhabricatorPolicyCapability::CAN_EDIT: - try { - $object->setEditPolicy(PhabricatorPolicies::POLICY_USER); - $console->writeOut("%s\n", pht('Unlocked edit policy.')); - $updated = true; - } catch (Exception $ex) { - $console->writeOut("%s\n", pht('Edit policy is not mutable.')); - } - break; - case PhabricatorPolicyCapability::CAN_JOIN: - try { - $object->setJoinPolicy(PhabricatorPolicies::POLICY_USER); - $console->writeOut("%s\n", pht('Unlocked join policy.')); - $updated = true; - } catch (Exception $ex) { - $console->writeOut("%s\n", pht('Join policy is not mutable.')); - } - break; - } + private function loadUser($username) { + $viewer = $this->getViewer(); + + if ($username === null) { + return null; } - if ($updated) { - $object->save(); - $console->writeOut("%s\n", pht('Saved object.')); - } else { - $console->writeOut( - "%s\n", + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withUsernames(array($username)) + ->executeOne(); + + if (!$user) { + throw new PhutilArgumentUsageException( pht( - 'Object has no mutable policies. Try unlocking parent/container '. - 'object instead. For example, to gain access to a commit, unlock '. - 'the repository it belongs to.')); + 'No user with username "%s" exists.', + $username)); } + + return $user; } } diff --git a/src/applications/system/engine/PhabricatorDefaultUnlockEngine.php b/src/applications/system/engine/PhabricatorDefaultUnlockEngine.php new file mode 100644 index 0000000000..624191ad21 --- /dev/null +++ b/src/applications/system/engine/PhabricatorDefaultUnlockEngine.php @@ -0,0 +1,4 @@ +canApplyTransactionType($object, $type_view)) { + throw new Exception( + pht( + 'Object view policy can not be unlocked because this object '. + 'does not have a mutable view policy.')); + } + + return array( + $this->newTransaction($object) + ->setTransactionType($type_view) + ->setNewValue($user->getPHID()), + ); + } + + public function newUnlockEditTransactions($object, $user) { + $type_edit = PhabricatorTransactions::TYPE_EDIT_POLICY; + + if (!$this->canApplyTransactionType($object, $type_edit)) { + throw new Exception( + pht( + 'Object edit policy can not be unlocked because this object '. + 'does not have a mutable edit policy.')); + } + + return array( + $this->newTransaction($object) + ->setTransactionType($type_edit) + ->setNewValue($user->getPHID()), + ); + } + + public function newUnlockOwnerTransactions($object, $user) { + throw new Exception( + pht( + 'Object owner can not be unlocked: the unlocking engine ("%s") for '. + 'this object does not implement an owner unlocking mechanism.', + get_class($this))); + } + + final protected function canApplyTransactionType($object, $type) { + $xaction_types = $object->getApplicationTransactionEditor() + ->getTransactionTypesForObject($object); + + $xaction_types = array_fuse($xaction_types); + + return isset($xaction_types[$type]); + } + + final protected function newTransaction($object) { + return $object->getApplicationTransactionTemplate(); + } + + +} From 77221bee72cbe6fb8488d030a121b954c7ef1e77 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Mar 2019 20:48:27 -0800 Subject: [PATCH 39/43] Allow objects to specify custom policy unlocking behavior, and tasks to have owners unlocked Summary: Depends on D20256. Ref T13249. See PHI1115. This primarily makes `bin/policy unlock --owner epriestley T123` work. This is important for "Edit Locked" tasks, since changing the edit policy doesn't really do anything. Test Plan: Hard-locked a task as "alice", reassigned it to myself with `bin/policy unlock --owner epriestley`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20257 --- src/__phutil_library_map__.php | 4 ++++ .../engine/ManiphestTaskUnlockEngine.php | 14 ++++++++++++++ .../maniphest/storage/ManiphestTask.php | 11 ++++++++++- .../system/engine/PhabricatorUnlockEngine.php | 8 +++++++- .../PhabricatorUnlockableInterface.php | 18 ++++++++++++++++++ 5 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 src/applications/maniphest/engine/ManiphestTaskUnlockEngine.php create mode 100644 src/applications/system/interface/PhabricatorUnlockableInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a2b9caf3ab..def6e0de7d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1784,6 +1784,7 @@ phutil_register_library_map(array( 'ManiphestTaskTitleTransaction' => 'applications/maniphest/xaction/ManiphestTaskTitleTransaction.php', 'ManiphestTaskTransactionType' => 'applications/maniphest/xaction/ManiphestTaskTransactionType.php', 'ManiphestTaskUnblockTransaction' => 'applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php', + 'ManiphestTaskUnlockEngine' => 'applications/maniphest/engine/ManiphestTaskUnlockEngine.php', 'ManiphestTransaction' => 'applications/maniphest/storage/ManiphestTransaction.php', 'ManiphestTransactionComment' => 'applications/maniphest/storage/ManiphestTransactionComment.php', 'ManiphestTransactionEditor' => 'applications/maniphest/editor/ManiphestTransactionEditor.php', @@ -4703,6 +4704,7 @@ phutil_register_library_map(array( 'PhabricatorUnitsTestCase' => 'view/__tests__/PhabricatorUnitsTestCase.php', 'PhabricatorUnknownContentSource' => 'infrastructure/contentsource/PhabricatorUnknownContentSource.php', 'PhabricatorUnlockEngine' => 'applications/system/engine/PhabricatorUnlockEngine.php', + 'PhabricatorUnlockableInterface' => 'applications/system/interface/PhabricatorUnlockableInterface.php', 'PhabricatorUnsubscribedFromObjectEdgeType' => 'applications/transactions/edges/PhabricatorUnsubscribedFromObjectEdgeType.php', 'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php', 'PhabricatorUserApproveTransaction' => 'applications/people/xaction/PhabricatorUserApproveTransaction.php', @@ -7419,6 +7421,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineLockableInterface', 'PhabricatorEditEngineMFAInterface', 'PhabricatorPolicyCodexInterface', + 'PhabricatorUnlockableInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', @@ -7496,6 +7499,7 @@ phutil_register_library_map(array( 'ManiphestTaskTitleTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskTransactionType' => 'PhabricatorModularTransactionType', 'ManiphestTaskUnblockTransaction' => 'ManiphestTaskTransactionType', + 'ManiphestTaskUnlockEngine' => 'PhabricatorUnlockEngine', 'ManiphestTransaction' => 'PhabricatorModularTransaction', 'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment', 'ManiphestTransactionEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/applications/maniphest/engine/ManiphestTaskUnlockEngine.php b/src/applications/maniphest/engine/ManiphestTaskUnlockEngine.php new file mode 100644 index 0000000000..b223724a77 --- /dev/null +++ b/src/applications/maniphest/engine/ManiphestTaskUnlockEngine.php @@ -0,0 +1,14 @@ +newTransaction($object) + ->setTransactionType(ManiphestTaskOwnerTransaction::TRANSACTIONTYPE) + ->setNewValue($user->getPHID()), + ); + } + +} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 0193830b39..400bace650 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -21,7 +21,8 @@ final class ManiphestTask extends ManiphestDAO PhabricatorEditEngineSubtypeInterface, PhabricatorEditEngineLockableInterface, PhabricatorEditEngineMFAInterface, - PhabricatorPolicyCodexInterface { + PhabricatorPolicyCodexInterface, + PhabricatorUnlockableInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -649,4 +650,12 @@ final class ManiphestTask extends ManiphestDAO return new ManiphestTaskPolicyCodex(); } + +/* -( PhabricatorUnlockableInterface )------------------------------------- */ + + + public function newUnlockEngine() { + return new ManiphestTaskUnlockEngine(); + } + } diff --git a/src/applications/system/engine/PhabricatorUnlockEngine.php b/src/applications/system/engine/PhabricatorUnlockEngine.php index a6cd57e30b..8afcc2873e 100644 --- a/src/applications/system/engine/PhabricatorUnlockEngine.php +++ b/src/applications/system/engine/PhabricatorUnlockEngine.php @@ -13,7 +13,13 @@ abstract class PhabricatorUnlockEngine 'PhabricatorApplicationTransactionInterface')); } - return new PhabricatorDefaultUnlockEngine(); + if ($object instanceof PhabricatorUnlockableInterface) { + $engine = $object->newUnlockEngine(); + } else { + $engine = new PhabricatorDefaultUnlockEngine(); + } + + return $engine; } public function newUnlockViewTransactions($object, $user) { diff --git a/src/applications/system/interface/PhabricatorUnlockableInterface.php b/src/applications/system/interface/PhabricatorUnlockableInterface.php new file mode 100644 index 0000000000..1a95215e8c --- /dev/null +++ b/src/applications/system/interface/PhabricatorUnlockableInterface.php @@ -0,0 +1,18 @@ +>>UnlockEngine(); + } + +*/ From 950a7bbb19e3405698f2e92abf6601e1e7649f8a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 06:10:11 -0800 Subject: [PATCH 40/43] On Harbormaster build plans, show which Herald rules trigger builds Summary: Ref T13258. Provide an easy way to find rules which trigger a particular build plan from the build plan page. The implementation here ends up a little messy: we can't just search for `actionType = 'build' AND targetPHID = ''` since the field is a blob of JSON. Instead, make rules indexable and write a "build plan is affected by rule actions" edge when indexing rules, then search on that edge. For now, only "Run Build Plan: ..." rules actually write this edge, since I think (?) that it doesn't really have meaningful values for other edge types today. Maybe "Call Webhooks", and you could get a link from a hook to rules that trigger it? Reasonable to do in the future. Things end up a little bit rough overall, but I think this panel is pretty useful to add to the Build Plan page. This index needs to be rebuilt with `bin/search index --type HeraldRule`. I'll call this out in the changelog but I'm not planning to explicitly migrate or add an activity, since this is only really important for larger installs and they probably (?) read the changelog. As rules are edited over time, this will converge to the right behavior even if you don't rebuild the index. Test Plan: {F6260095} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20259 --- src/__phutil_library_map__.php | 7 ++ .../HarbormasterPlanViewController.php | 38 ++++++++ .../HarbormasterRunBuildPlansHeraldAction.php | 5 + .../herald/action/HeraldAction.php | 4 + .../HeraldRuleActionAffectsObjectEdgeType.php | 8 ++ .../herald/editor/HeraldRuleEditor.php | 4 + .../HeraldRuleIndexEngineExtension.php | 92 +++++++++++++++++++ .../herald/query/HeraldRuleQuery.php | 28 ++++++ .../herald/query/HeraldRuleSearchEngine.php | 52 +++-------- .../herald/storage/HeraldRule.php | 1 + .../herald/view/HeraldRuleListView.php | 65 +++++++++++++ 11 files changed, 264 insertions(+), 40 deletions(-) create mode 100644 src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php create mode 100644 src/applications/herald/engineextension/HeraldRuleIndexEngineExtension.php create mode 100644 src/applications/herald/view/HeraldRuleListView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index def6e0de7d..1a5ab46743 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1531,6 +1531,7 @@ phutil_register_library_map(array( 'HeraldRemarkupFieldValue' => 'applications/herald/value/HeraldRemarkupFieldValue.php', 'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', + 'HeraldRuleActionAffectsObjectEdgeType' => 'applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php', 'HeraldRuleAdapter' => 'applications/herald/adapter/HeraldRuleAdapter.php', 'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', @@ -1540,7 +1541,9 @@ phutil_register_library_map(array( 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', + 'HeraldRuleIndexEngineExtension' => 'applications/herald/engineextension/HeraldRuleIndexEngineExtension.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', + 'HeraldRuleListView' => 'applications/herald/view/HeraldRuleListView.php', 'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', @@ -7189,8 +7192,10 @@ phutil_register_library_map(array( 'PhabricatorFlaggableInterface', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorIndexableInterface', 'PhabricatorSubscribableInterface', ), + 'HeraldRuleActionAffectsObjectEdgeType' => 'PhabricatorEdgeType', 'HeraldRuleAdapter' => 'HeraldAdapter', 'HeraldRuleAdapterField' => 'HeraldRuleField', 'HeraldRuleController' => 'HeraldController', @@ -7200,7 +7205,9 @@ phutil_register_library_map(array( 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleField' => 'HeraldField', 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', + 'HeraldRuleIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'HeraldRuleListController' => 'HeraldController', + 'HeraldRuleListView' => 'AphrontView', 'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 141f321caa..731c4fa784 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -62,6 +62,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $builds_view = $this->newBuildsView($plan); $options_view = $this->newOptionsView($plan); + $rules_view = $this->newRulesView($plan); $timeline = $this->buildTransactionTimeline( $plan, @@ -76,6 +77,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $error, $step_list, $options_view, + $rules_view, $builds_view, $timeline, )); @@ -486,6 +488,42 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->appendChild($list); } + private function newRulesView(HarbormasterBuildPlan $plan) { + $viewer = $this->getViewer(); + + $rules = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withDisabled(false) + ->withAffectedObjectPHIDs(array($plan->getPHID())) + ->needValidateAuthors(true) + ->execute(); + + $list = id(new HeraldRuleListView()) + ->setViewer($viewer) + ->setRules($rules) + ->newObjectList(); + + $list->setNoDataString(pht('No active Herald rules trigger this build.')); + + $more_href = new PhutilURI( + '/herald/', + array('affectedPHID' => $plan->getPHID())); + + $more_link = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-list-ul') + ->setText(pht('View All Rules')) + ->setHref($more_href); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Run By Herald Rules')) + ->addActionLink($more_link); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($list); + } private function newOptionsView(HarbormasterBuildPlan $plan) { $viewer = $this->getViewer(); diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 8c718e5f5d..9fc053e8ae 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -91,4 +91,9 @@ final class HarbormasterRunBuildPlansHeraldAction 'Run build plans: %s.', $this->renderHandleList($value)); } + + public function getPHIDsAffectedByAction(HeraldActionRecord $record) { + return $record->getTarget(); + } + } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index 04884a94d4..a9740d1736 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -401,4 +401,8 @@ abstract class HeraldAction extends Phobject { return null; } + public function getPHIDsAffectedByAction(HeraldActionRecord $record) { + return array(); + } + } diff --git a/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php b/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php new file mode 100644 index 0000000000..35a30773ac --- /dev/null +++ b/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php @@ -0,0 +1,8 @@ +getPHID(), + $edge_type); + $old_edges = array_fuse($old_edges); + + $new_edges = $this->getPHIDsAffectedByActions($object); + $new_edges = array_fuse($new_edges); + + $add_edges = array_diff_key($new_edges, $old_edges); + $rem_edges = array_diff_key($old_edges, $new_edges); + + if (!$add_edges && !$rem_edges) { + return; + } + + $editor = new PhabricatorEdgeEditor(); + + foreach ($add_edges as $phid) { + $editor->addEdge($object->getPHID(), $edge_type, $phid); + } + + foreach ($rem_edges as $phid) { + $editor->removeEdge($object->getPHID(), $edge_type, $phid); + } + + $editor->save(); + } + + public function getIndexVersion($object) { + $phids = $this->getPHIDsAffectedByActions($object); + sort($phids); + $phids = implode(':', $phids); + return PhabricatorHash::digestForIndex($phids); + } + + private function getPHIDsAffectedByActions(HeraldRule $rule) { + $viewer = $this->getViewer(); + + $rule = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withIDs(array($rule->getID())) + ->needConditionsAndActions(true) + ->executeOne(); + if (!$rule) { + return array(); + } + + $phids = array(); + + $actions = HeraldAction::getAllActions(); + foreach ($rule->getActions() as $action_record) { + $action = idx($actions, $action_record->getAction()); + + if (!$action) { + continue; + } + + foreach ($action->getPHIDsAffectedByAction($action_record) as $phid) { + $phids[] = $phid; + } + } + + $phids = array_fuse($phids); + return array_keys($phids); + } + +} diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index e6dba43c7a..e346a998d4 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -11,6 +11,7 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $active; private $datasourceQuery; private $triggerObjectPHIDs; + private $affectedObjectPHIDs; private $needConditionsAndActions; private $needAppliedToPHIDs; @@ -61,6 +62,11 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $this; } + public function withAffectedObjectPHIDs(array $phids) { + $this->affectedObjectPHIDs = $phids; + return $this; + } + public function needConditionsAndActions($need) { $this->needConditionsAndActions = $need; return $this; @@ -261,9 +267,31 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { $this->triggerObjectPHIDs); } + if ($this->affectedObjectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'edge_affects.dst IN (%Ls)', + $this->affectedObjectPHIDs); + } + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->affectedObjectPHIDs !== null) { + $joins[] = qsprintf( + $conn, + 'JOIN %T edge_affects ON rule.phid = edge_affects.src + AND edge_affects.type = %d', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + HeraldRuleActionAffectsObjectEdgeType::EDGECONST); + } + + return $joins; + } + private function validateRuleAuthors(array $rules) { // "Global" and "Object" rules always have valid authors. foreach ($rules as $key => $rule) { diff --git a/src/applications/herald/query/HeraldRuleSearchEngine.php b/src/applications/herald/query/HeraldRuleSearchEngine.php index 47a6832731..95e3079717 100644 --- a/src/applications/herald/query/HeraldRuleSearchEngine.php +++ b/src/applications/herald/query/HeraldRuleSearchEngine.php @@ -55,6 +55,10 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { pht('(Show All)'), pht('Show Only Disabled Rules'), pht('Show Only Enabled Rules')), + id(new PhabricatorPHIDsSearchField()) + ->setLabel(pht('Affected Objects')) + ->setKey('affectedPHIDs') + ->setAliases(array('affectedPHID')), ); } @@ -81,6 +85,10 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { $query->withActive($map['active']); } + if ($map['affectedPHIDs']) { + $query->withAffectedObjectPHIDs($map['affectedPHIDs']); + } + return $query; } @@ -127,54 +135,18 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { PhabricatorSavedQuery $query, array $handles) { assert_instances_of($rules, 'HeraldRule'); - $viewer = $this->requireViewer(); - $handles = $viewer->loadHandles(mpull($rules, 'getAuthorPHID')); - $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer); - - $list = id(new PHUIObjectItemListView()) - ->setUser($viewer); - foreach ($rules as $rule) { - $monogram = $rule->getMonogram(); - - $item = id(new PHUIObjectItemView()) - ->setObjectName($monogram) - ->setHeader($rule->getName()) - ->setHref("/{$monogram}"); - - if ($rule->isPersonalRule()) { - $item->addIcon('fa-user', pht('Personal Rule')); - $item->addByline( - pht( - 'Authored by %s', - $handles[$rule->getAuthorPHID()]->renderLink())); - } else if ($rule->isObjectRule()) { - $item->addIcon('fa-briefcase', pht('Object Rule')); - } else { - $item->addIcon('fa-globe', pht('Global Rule')); - } - - if ($rule->getIsDisabled()) { - $item->setDisabled(true); - $item->addIcon('fa-lock grey', pht('Disabled')); - } else if (!$rule->hasValidAuthor()) { - $item->setDisabled(true); - $item->addIcon('fa-user grey', pht('Author Not Active')); - } - - $content_type_name = idx($content_type_map, $rule->getContentType()); - $item->addAttribute(pht('Affects: %s', $content_type_name)); - - $list->addItem($item); - } + $list = id(new HeraldRuleListView()) + ->setViewer($viewer) + ->setRules($rules) + ->newObjectList(); $result = new PhabricatorApplicationSearchResultView(); $result->setObjectList($list); $result->setNoDataString(pht('No rules found.')); return $result; - } protected function getNewUserBody() { diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 1b005898cb..a9c131e717 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -6,6 +6,7 @@ final class HeraldRule extends HeraldDAO PhabricatorFlaggableInterface, PhabricatorPolicyInterface, PhabricatorDestructibleInterface, + PhabricatorIndexableInterface, PhabricatorSubscribableInterface { const TABLE_RULE_APPLIED = 'herald_ruleapplied'; diff --git a/src/applications/herald/view/HeraldRuleListView.php b/src/applications/herald/view/HeraldRuleListView.php new file mode 100644 index 0000000000..150499ce87 --- /dev/null +++ b/src/applications/herald/view/HeraldRuleListView.php @@ -0,0 +1,65 @@ +rules = $rules; + return $this; + } + + public function render() { + return $this->newObjectList(); + } + + public function newObjectList() { + $viewer = $this->getViewer(); + $rules = $this->rules; + + $handles = $viewer->loadHandles(mpull($rules, 'getAuthorPHID')); + + $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer); + + $list = id(new PHUIObjectItemListView()) + ->setViewer($viewer); + foreach ($rules as $rule) { + $monogram = $rule->getMonogram(); + + $item = id(new PHUIObjectItemView()) + ->setObjectName($monogram) + ->setHeader($rule->getName()) + ->setHref($rule->getURI()); + + if ($rule->isPersonalRule()) { + $item->addIcon('fa-user', pht('Personal Rule')); + $item->addByline( + pht( + 'Authored by %s', + $handles[$rule->getAuthorPHID()]->renderLink())); + } else if ($rule->isObjectRule()) { + $item->addIcon('fa-briefcase', pht('Object Rule')); + } else { + $item->addIcon('fa-globe', pht('Global Rule')); + } + + if ($rule->getIsDisabled()) { + $item->setDisabled(true); + $item->addIcon('fa-lock grey', pht('Disabled')); + } else if (!$rule->hasValidAuthor()) { + $item->setDisabled(true); + $item->addIcon('fa-user grey', pht('Author Not Active')); + } + + $content_type_name = idx($content_type_map, $rule->getContentType()); + $item->addAttribute(pht('Affects: %s', $content_type_name)); + + $list->addItem($item); + } + + return $list; + } + +} From 9913754a2aa25f46f5f85278153a65cc8a2da342 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 08:27:36 -0800 Subject: [PATCH 41/43] Improve utilization of "AuthTemporaryToken" table keys in LFS authentication queries Summary: See PHI1123. The key on this table is `` but we currently query for only ``. This can't use the key. Constrain the query to the resource we expect (the repository) so it can use the key. Test Plan: Pushed files using LFS. See PHI1123 for more, likely. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20261 --- .../controller/DiffusionServeController.php | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index cb4ad0ba95..aea901f100 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -192,7 +192,10 @@ final class DiffusionServeController extends DiffusionController { // Try Git LFS auth first since we can usually reject it without doing // any queries, since the username won't match the one we expect or the // request won't be LFS. - $viewer = $this->authenticateGitLFSUser($username, $password); + $viewer = $this->authenticateGitLFSUser( + $username, + $password, + $identifier); // If that failed, try normal auth. Note that we can use normal auth on // LFS requests, so this isn't strictly an alternative to LFS auth. @@ -655,7 +658,8 @@ final class DiffusionServeController extends DiffusionController { private function authenticateGitLFSUser( $username, - PhutilOpaqueEnvelope $password) { + PhutilOpaqueEnvelope $password, + $identifier) { // Never accept these credentials for requests which aren't LFS requests. if (!$this->getIsGitLFSRequest()) { @@ -668,11 +672,31 @@ final class DiffusionServeController extends DiffusionController { return null; } + // See PHI1123. We need to be able to constrain the token query with + // "withTokenResources(...)" to take advantage of the key on the table. + // In this case, the repository PHID is the "resource" we're after. + + // In normal workflows, we figure out the viewer first, then use the + // viewer to load the repository, but that won't work here. Load the + // repository as the omnipotent viewer, then use the repository PHID to + // look for a token. + + $omnipotent_viewer = PhabricatorUser::getOmnipotentUser(); + + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer($omnipotent_viewer) + ->withIdentifiers(array($identifier)) + ->executeOne(); + if (!$repository) { + return null; + } + $lfs_pass = $password->openEnvelope(); $lfs_hash = PhabricatorHash::weakDigest($lfs_pass); $token = id(new PhabricatorAuthTemporaryTokenQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($omnipotent_viewer) + ->withTokenResources(array($repository->getPHID())) ->withTokenTypes(array(DiffusionGitLFSTemporaryTokenType::TOKENTYPE)) ->withTokenCodes(array($lfs_hash)) ->withExpired(false) @@ -682,7 +706,7 @@ final class DiffusionServeController extends DiffusionController { } $user = id(new PhabricatorPeopleQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($omnipotent_viewer) ->withPHIDs(array($token->getUserPHID())) ->executeOne(); From 1d4f6bd44458205a4475399b72461ab1f838886c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 06:30:02 -0800 Subject: [PATCH 42/43] Index "Call Webhook" in Herald, and show calling rules on the Webhook page Summary: Depends on D20259. Now that we can index Herald rules to affected objects, show callers on the "Webhooks" UI. A few other rule types could get indexes too ("Sign Legalpad Documents", "Add Reviewers", "Add Subscribers"), but I think they're less likely to be useful since those triggers are usually more obvious (the transaction timeline makes it clearer what happened/why). We could revisit this in the future now that it's a possibility. Test Plan: {F6260106} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D20260 --- .../HarbormasterPlanViewController.php | 1 + .../herald/action/HeraldCallWebhookAction.php | 4 ++ .../HeraldWebhookViewController.php | 41 +++++++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 731c4fa784..a9af90f2a5 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -496,6 +496,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->withDisabled(false) ->withAffectedObjectPHIDs(array($plan->getPHID())) ->needValidateAuthors(true) + ->setLimit(10) ->execute(); $list = id(new HeraldRuleListView()) diff --git a/src/applications/herald/action/HeraldCallWebhookAction.php b/src/applications/herald/action/HeraldCallWebhookAction.php index 953958e5c6..186a7a741f 100644 --- a/src/applications/herald/action/HeraldCallWebhookAction.php +++ b/src/applications/herald/action/HeraldCallWebhookAction.php @@ -63,4 +63,8 @@ final class HeraldCallWebhookAction extends HeraldAction { return new HeraldWebhookDatasource(); } + public function getPHIDsAffectedByAction(HeraldActionRecord $record) { + return $record->getTarget(); + } + } diff --git a/src/applications/herald/controller/HeraldWebhookViewController.php b/src/applications/herald/controller/HeraldWebhookViewController.php index d8e5eb3c54..5f6be9816c 100644 --- a/src/applications/herald/controller/HeraldWebhookViewController.php +++ b/src/applications/herald/controller/HeraldWebhookViewController.php @@ -73,12 +73,15 @@ final class HeraldWebhookViewController ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setTable($requests_table); + $rules_view = $this->newRulesView($hook); + $hook_view = id(new PHUITwoColumnView()) ->setHeader($header) ->setMainColumn( array( $warnings, $properties_view, + $rules_view, $requests_view, $timeline, )) @@ -194,4 +197,42 @@ final class HeraldWebhookViewController ->appendChild($properties); } + private function newRulesView(HeraldWebhook $hook) { + $viewer = $this->getViewer(); + + $rules = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withDisabled(false) + ->withAffectedObjectPHIDs(array($hook->getPHID())) + ->needValidateAuthors(true) + ->setLimit(10) + ->execute(); + + $list = id(new HeraldRuleListView()) + ->setViewer($viewer) + ->setRules($rules) + ->newObjectList(); + + $list->setNoDataString(pht('No active Herald rules call this webhook.')); + + $more_href = new PhutilURI( + '/herald/', + array('affectedPHID' => $hook->getPHID())); + + $more_link = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-list-ul') + ->setText(pht('View All Rules')) + ->setHref($more_href); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Called By Herald Rules')) + ->addActionLink($more_link); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($list); + } + } From c1bff3b8013e1d2d1293a8a843e15845edfd7194 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Mar 2019 09:32:31 -0800 Subject: [PATCH 43/43] Add an "Restartable: If Failed" behavior to Harbormaster build plans Summary: Ref T13249. Ref T13258. In some cases, builds are not idempotent and should not be restarted casually. If the scary part is at the very end (deploy / provision / whatever), it could be okay to restart them if they previously failed. Also, make the "reasons why you can't restart" and "explanations of why you can't restart" logic a little more cohesive. Test Plan: - Tried to restart builds in various states (failed/not failed, restartable always/if failed/never, already restarted), got appropriate errors or restarts. - (I'm not sure the "Autoplan" error is normally reachable, since you can't edit autoplans to configure things to let you try to restart them.) Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258, T13249 Differential Revision: https://secure.phabricator.com/D20252 --- src/__phutil_library_map__.php | 2 + .../constants/HarbormasterBuildStatus.php | 4 ++ .../HarbormasterBuildActionController.php | 18 ++---- .../HarbormasterRestartException.php | 33 +++++++++++ .../plan/HarbormasterBuildPlanBehavior.php | 7 +++ .../storage/build/HarbormasterBuild.php | 56 +++++++++++++++++-- .../configuration/HarbormasterBuildPlan.php | 10 ---- 7 files changed, 104 insertions(+), 26 deletions(-) create mode 100644 src/applications/harbormaster/exception/HarbormasterRestartException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1a5ab46743..651fad9d12 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1443,6 +1443,7 @@ phutil_register_library_map(array( 'HarbormasterQueryBuildsConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php', 'HarbormasterQueryBuildsSearchEngineAttachment' => 'applications/harbormaster/engineextension/HarbormasterQueryBuildsSearchEngineAttachment.php', 'HarbormasterRemarkupRule' => 'applications/harbormaster/remarkup/HarbormasterRemarkupRule.php', + 'HarbormasterRestartException' => 'applications/harbormaster/exception/HarbormasterRestartException.php', 'HarbormasterRunBuildPlansHeraldAction' => 'applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php', 'HarbormasterSchemaSpec' => 'applications/harbormaster/storage/HarbormasterSchemaSpec.php', 'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php', @@ -7093,6 +7094,7 @@ phutil_register_library_map(array( 'HarbormasterQueryBuildsConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterQueryBuildsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'HarbormasterRemarkupRule' => 'PhabricatorObjectRemarkupRule', + 'HarbormasterRestartException' => 'Exception', 'HarbormasterRunBuildPlansHeraldAction' => 'HeraldAction', 'HarbormasterSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'HarbormasterScratchTable' => 'HarbormasterDAO', diff --git a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php index 7437e48f6c..3ceb8e0686 100644 --- a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php +++ b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php @@ -52,6 +52,10 @@ final class HarbormasterBuildStatus extends Phobject { return ($this->key === self::STATUS_PASSED); } + public function isFailed() { + return ($this->key === self::STATUS_FAILED); + } + /** * Get a human readable name for a build status constant. diff --git a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php index b56c7de7f7..6a4a2b1fee 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php @@ -64,19 +64,13 @@ final class HarbormasterBuildActionController 'restart. Side effects of the build will occur again. Really '. 'restart build?'); $submit = pht('Restart Build'); - } else if (!$build->getBuildPlan()->canRestartBuildPlan()) { - $title = pht('Not Restartable'); - $body = pht( - 'The build plan for this build is not restartable, so you '. - 'can not restart the build.'); } else { - $title = pht('Unable to Restart Build'); - if ($build->isRestarting()) { - $body = pht( - 'This build is already restarting. You can not reissue a '. - 'restart command to a restarting build.'); - } else { - $body = pht('You can not restart this build.'); + try { + $build->assertCanRestartBuild(); + throw new Exception(pht('Expected to be unable to restart build.')); + } catch (HarbormasterRestartException $ex) { + $title = $ex->getTitle(); + $body = $ex->getBody(); } } break; diff --git a/src/applications/harbormaster/exception/HarbormasterRestartException.php b/src/applications/harbormaster/exception/HarbormasterRestartException.php new file mode 100644 index 0000000000..bd0b86184a --- /dev/null +++ b/src/applications/harbormaster/exception/HarbormasterRestartException.php @@ -0,0 +1,33 @@ +setTitle($title); + $this->appendParagraph($body); + + parent::__construct($title); + } + + public function setTitle($title) { + $this->title = $title; + return $this; + } + + public function getTitle() { + return $this->title; + } + + public function appendParagraph($description) { + $this->body[] = $description; + return $this; + } + + public function getBody() { + return $this->body; + } + +} diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index 3c63b3e040..d8e857e711 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -15,6 +15,7 @@ final class HarbormasterBuildPlanBehavior const BEHAVIOR_RESTARTABLE = 'restartable'; const RESTARTABLE_ALWAYS = 'always'; + const RESTARTABLE_IF_FAILED = 'failed'; const RESTARTABLE_NEVER = 'never'; const BEHAVIOR_DRAFTS = 'hold-drafts'; @@ -251,6 +252,12 @@ final class HarbormasterBuildPlanBehavior ->setIsDefault(true) ->setDescription( pht('The build may be restarted.')), + id(new HarbormasterBuildPlanBehaviorOption()) + ->setKey(self::RESTARTABLE_IF_FAILED) + ->setIcon('fa-times-circle-o yellow') + ->setName(pht('If Failed')) + ->setDescription( + pht('The build may be restarted if it has failed.')), id(new HarbormasterBuildPlanBehaviorOption()) ->setKey(self::RESTARTABLE_NEVER) ->setIcon('fa-times red') diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 063f81ff1e..70c26827ec 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -183,6 +183,10 @@ final class HarbormasterBuild extends HarbormasterDAO return $this->getBuildStatusObject()->isPassed(); } + public function isFailed() { + return $this->getBuildStatusObject()->isFailed(); + } + public function getURI() { $id = $this->getID(); return "/harbormaster/build/{$id}/"; @@ -211,16 +215,60 @@ final class HarbormasterBuild extends HarbormasterDAO } public function canRestartBuild() { + try { + $this->assertCanRestartBuild(); + return true; + } catch (HarbormasterRestartException $ex) { + return false; + } + } + + public function assertCanRestartBuild() { if ($this->isAutobuild()) { - return false; + throw new HarbormasterRestartException( + pht('Can Not Restart Autobuild'), + pht( + 'This build can not be restarted because it is an automatic '. + 'build.')); } + $restartable = HarbormasterBuildPlanBehavior::BEHAVIOR_RESTARTABLE; $plan = $this->getBuildPlan(); - if (!$plan->canRestartBuildPlan()) { - return false; + + $option = HarbormasterBuildPlanBehavior::getBehavior($restartable) + ->getPlanOption($plan); + $option_key = $option->getKey(); + + $never_restartable = HarbormasterBuildPlanBehavior::RESTARTABLE_NEVER; + $is_never = ($option_key === $never_restartable); + if ($is_never) { + throw new HarbormasterRestartException( + pht('Build Plan Prevents Restart'), + pht( + 'This build can not be restarted because the build plan is '. + 'configured to prevent the build from restarting.')); } - return !$this->isRestarting(); + $failed_restartable = HarbormasterBuildPlanBehavior::RESTARTABLE_IF_FAILED; + $is_failed = ($option_key === $failed_restartable); + if ($is_failed) { + if (!$this->isFailed()) { + throw new HarbormasterRestartException( + pht('Only Restartable if Failed'), + pht( + 'This build can not be restarted because the build plan is '. + 'configured to prevent the build from restarting unless it '. + 'has failed, and it has not failed.')); + } + } + + if ($this->isRestarting()) { + throw new HarbormasterRestartException( + pht('Already Restarting'), + pht( + 'This build is already restarting. You can not reissue a restart '. + 'command to a restarting build.')); + } } public function canPauseBuild() { diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index efe62a6f84..798201f490 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -175,16 +175,6 @@ final class HarbormasterBuildPlan extends HarbormasterDAO $capability); } - public function canRestartBuildPlan() { - $restartable = HarbormasterBuildPlanBehavior::BEHAVIOR_RESTARTABLE; - $is_restartable = HarbormasterBuildPlanBehavior::RESTARTABLE_ALWAYS; - - $option = HarbormasterBuildPlanBehavior::getBehavior($restartable) - ->getPlanOption($this); - - return ($option->getKey() === $is_restartable); - } - /* -( PhabricatorSubscribableInterface )----------------------------------- */