From 650e74933abb94fa3b37d0cbae4b6323a0633ecd Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Sep 2018 11:26:30 -0700 Subject: [PATCH 01/13] Update some inline comment logic to use more modern "Viewer"-oriented calls/variables Summary: Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls. Instead, use `$viewer = $this->getViewer()`. This is just a small consistency update with no behavioral changes. Test Plan: Viewed and added inlines in Differential and Diffusion. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195, T8573 Differential Revision: https://secure.phabricator.com/D19633 --- ...DifferentialInlineCommentEditController.php | 12 +++++------- .../DiffusionInlineCommentController.php | 12 +++++------- .../PhabricatorInlineCommentController.php | 18 +++++++++--------- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 926158cb41..2e44e86822 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -77,19 +77,17 @@ final class DifferentialInlineCommentEditController } protected function loadCommentForEdit($id) { - $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); $inline = $this->loadComment($id); - if (!$this->canEditInlineComment($user, $inline)) { + if (!$this->canEditInlineComment($viewer, $inline)) { throw new Exception(pht('That comment is not editable!')); } return $inline; } protected function loadCommentForDone($id) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); $inline = $this->loadComment($id); if (!$inline) { @@ -128,11 +126,11 @@ final class DifferentialInlineCommentEditController } private function canEditInlineComment( - PhabricatorUser $user, + PhabricatorUser $viewer, DifferentialInlineComment $inline) { // Only the author may edit a comment. - if ($inline->getAuthorPHID() != $user->getPHID()) { + if ($inline->getAuthorPHID() != $viewer->getPHID()) { return false; } diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php index 8af9a1cd65..bc7448a9e2 100644 --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/src/applications/diffusion/controller/DiffusionInlineCommentController.php @@ -50,19 +50,17 @@ final class DiffusionInlineCommentController } protected function loadCommentForEdit($id) { - $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); $inline = $this->loadComment($id); - if (!$this->canEditInlineComment($user, $inline)) { + if (!$this->canEditInlineComment($viewer, $inline)) { throw new Exception(pht('That comment is not editable!')); } return $inline; } protected function loadCommentForDone($id) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); $inline = $this->loadComment($id); if (!$inline) { @@ -86,11 +84,11 @@ final class DiffusionInlineCommentController } private function canEditInlineComment( - PhabricatorUser $user, + PhabricatorUser $viewer, PhabricatorAuditInlineComment $inline) { // Only the author may edit a comment. - if ($inline->getAuthorPHID() != $user->getPHID()) { + if ($inline->getAuthorPHID() != $viewer->getPHID()) { return false; } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index d7f1a9dd3a..22e410612a 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -88,7 +88,7 @@ abstract class PhabricatorInlineCommentController public function processRequest() { $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); $this->readRequestParameters(); @@ -221,7 +221,7 @@ abstract class PhabricatorInlineCommentController $inline = $this->createComment() ->setChangesetID($this->getChangesetID()) - ->setAuthorPHID($user->getPHID()) + ->setAuthorPHID($viewer->getPHID()) ->setLineNumber($this->getLineNumber()) ->setLineLength($this->getLineLength()) ->setIsNewFile($this->getIsNewFile()) @@ -313,10 +313,10 @@ abstract class PhabricatorInlineCommentController private function buildEditDialog() { $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); $edit_dialog = id(new PHUIDiffInlineCommentEditView()) - ->setUser($user) + ->setUser($viewer) ->setSubmitURI($request->getRequestURI()) ->setIsOnRight($this->getIsOnRight()) ->setIsNewFile($this->getIsNewFile()) @@ -342,22 +342,22 @@ abstract class PhabricatorInlineCommentController $on_right) { $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($user); + $engine->setViewer($viewer); $engine->addObject( $inline, PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); $engine->process(); - $phids = array($user->getPHID()); + $phids = array($viewer->getPHID()); $handles = $this->loadViewerHandles($phids); $object_owner_phid = $this->loadObjectOwnerPHID($inline); $view = id(new PHUIDiffInlineCommentDetailView()) - ->setUser($user) + ->setUser($viewer) ->setInlineComment($inline) ->setIsOnRight($on_right) ->setMarkupEngine($engine) @@ -378,7 +378,7 @@ abstract class PhabricatorInlineCommentController private function renderTextArea($text) { return id(new PhabricatorRemarkupControl()) - ->setUser($this->getRequest()->getUser()) + ->setViewer($this->getViewer()) ->setSigil('differential-inline-comment-edit-textarea') ->setName('text') ->setValue($text) From 041392988ece4d1f7a0eff5e08d58bb0bba4ca70 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Sep 2018 10:41:13 -0700 Subject: [PATCH 02/13] When a transaction adds more than 100 inline comments, include only the first 100 in email Summary: Ref T13195. An install had a user apply a transaction which added about 1,000 inline comments. Rendering the email for this transaction took a very long time because the context section for each comment must be highlighted separately. We can make the highlighting faster (in this case, by porting the lexer to PHP) but it's also sort of silly to include more than 100 inlines in an email. These emails are likely to be truncated by outbound size rules at some point anyway. Instead, limit inlines rendered directly into email to the first 100 per transaction group. Test Plan: Set limit to 2, added 4 comments, viewed text and HTML emails: {F5859967} {F5859968} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19632 --- .../editor/DifferentialTransactionEditor.php | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 9c24a50b4e..dc47683875 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -892,6 +892,17 @@ final class DifferentialTransactionEditor array $inlines, PhabricatorMetaMTAMailBody $body) { + $limit = 100; + $limit_note = null; + if (count($inlines) > $limit) { + $limit_note = pht( + '(Showing first %s of %s inlines.)', + new PhutilNumber($limit), + phutil_count($inlines)); + + $inlines = array_slice($inlines, 0, $limit, true); + } + $section = id(new DifferentialInlineCommentMailView()) ->setViewer($this->getActor()) ->setInlines($inlines) @@ -900,6 +911,9 @@ final class DifferentialTransactionEditor $header = pht('INLINE COMMENTS'); $section_text = "\n".$section->getPlaintext(); + if ($limit_note) { + $section_text = $limit_note."\n".$section_text; + } $style = array( 'margin: 6px 0 12px 0;', @@ -912,6 +926,16 @@ final class DifferentialTransactionEditor ), $section->getHTML()); + if ($limit_note) { + $section_html = array( + phutil_tag( + 'em', + array(), + $limit_note), + $section_html, + ); + } + $body->addPlaintextSection($header, $section_text, false); $body->addHTMLSection($header, $section_html); } From e6ee5ee9a10488f89aa6b17d080a832e332f1c9c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Sep 2018 12:44:11 -0700 Subject: [PATCH 03/13] When applying repository operations via Drydock, provide more context on OperationType Summary: Ref T13195. See PHI845. For custom OperationTypes, provide access to the Interface and Operation via getters. This is just for convenience, since passing these around everywhere can be a bit of a pain if you have a deep-ish stack of things or love using callbacks or whatever. Test Plan: Landed a revision via upstream Drydock operations. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19636 --- .../DrydockRepositoryOperationType.php | 23 +++++++++++++++++++ .../query/DrydockRepositoryOperationQuery.php | 7 +++++- .../storage/DrydockRepositoryOperation.php | 6 ++--- ...DrydockRepositoryOperationUpdateWorker.php | 5 ---- 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index 7ec3689aff..32b562a303 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -3,6 +3,8 @@ abstract class DrydockRepositoryOperationType extends Phobject { private $viewer; + private $operation; + private $interface; abstract public function applyOperation( DrydockRepositoryOperation $operation, @@ -29,6 +31,27 @@ abstract class DrydockRepositoryOperationType extends Phobject { return $this->viewer; } + final public function setOperation(DrydockRepositoryOperation $operation) { + $this->operation = $operation; + return $this; + } + + final public function getOperation() { + return $this->operation; + } + + final public function setInterface(DrydockInterface $interface) { + $this->interface = $interface; + return $this; + } + + final public function getInterface() { + if (!$this->interface) { + throw new PhutilInvalidStateException('setInterface'); + } + return $this->interface; + } + final public function getOperationConstant() { return $this->getPhobjectClassConstant('OPCONST', 32); } diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php index fb1f6583a8..a5fbe0acc9 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -62,6 +62,8 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { protected function willFilterPage(array $operations) { $implementations = DrydockRepositoryOperationType::getAllOperationTypes(); + $viewer = $this->getViewer(); + foreach ($operations as $key => $operation) { $impl = idx($implementations, $operation->getOperationType()); if (!$impl) { @@ -69,7 +71,10 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { unset($operations[$key]); continue; } - $impl = clone $impl; + $impl = id(clone $impl) + ->setViewer($viewer) + ->setOperation($operation); + $operation->attachImplementation($impl); } diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index 4e25280c3f..809a10a116 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -137,9 +137,9 @@ final class DrydockRepositoryOperation extends DrydockDAO } public function applyOperation(DrydockInterface $interface) { - return $this->getImplementation()->applyOperation( - $this, - $interface); + $impl = $this->getImplementation(); + $impl->setInterface($interface); + return $impl->applyOperation($this, $interface); } public function getOperationDescription(PhabricatorUser $viewer) { diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php index dfd603fd12..2ee9324fb6 100644 --- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php @@ -25,8 +25,6 @@ final class DrydockRepositoryOperationUpdateWorker private function handleUpdate(DrydockRepositoryOperation $operation) { - $viewer = $this->getViewer(); - $operation_state = $operation->getOperationState(); switch ($operation_state) { @@ -53,9 +51,6 @@ final class DrydockRepositoryOperationUpdateWorker // waiting for a lease we're holding. try { - $operation->getImplementation() - ->setViewer($viewer); - $lease = $this->loadWorkingCopyLease($operation); $interface = $lease->getInterface( From 7967fab5ccc6d189c4ddf9927e30a6b6133f83f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 13:47:43 -0700 Subject: [PATCH 04/13] Remove outdated "rebuild_summaries.php" script Summary: Ref T13195. See PHI842. Alternative to D19638. Instead of doing all the stuff in D19638, //just// remove the `rebuild_summaries.php` script. This script is outdated, copy/pastes the rebuild logic, and doesn't understand unreachable commits. If we had some use for it it should move to `bin/repository rebuild-summary ...` or similar, but it's not clear there's any use for it. The incremental summary rebuilds seem to work fine as-is. Test Plan: Grepped for callers or documentation referencing this script, found nothing. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19643 --- scripts/repository/rebuild_summaries.php | 53 ------------------------ 1 file changed, 53 deletions(-) delete mode 100755 scripts/repository/rebuild_summaries.php diff --git a/scripts/repository/rebuild_summaries.php b/scripts/repository/rebuild_summaries.php deleted file mode 100755 index de3d5acca0..0000000000 --- a/scripts/repository/rebuild_summaries.php +++ /dev/null @@ -1,53 +0,0 @@ -#!/usr/bin/env php -establishConnection('w'); -$sizes = queryfx_all( - $conn_w, - 'SELECT repositoryID, count(*) N FROM %T GROUP BY repositoryID', - $commit->getTableName()); -$sizes = ipull($sizes, 'N', 'repositoryID'); - -$maxes = queryfx_all( - $conn_w, - 'SELECT repositoryID, max(epoch) maxEpoch FROM %T GROUP BY repositoryID', - $commit->getTableName()); -$maxes = ipull($maxes, 'maxEpoch', 'repositoryID'); - - -$repository_ids = array_keys($sizes + $maxes); - -echo pht('Updating %d repositories', count($repository_ids)); - -foreach ($repository_ids as $repository_id) { - $last_commit = queryfx_one( - $conn_w, - 'SELECT id FROM %T WHERE repositoryID = %d AND epoch = %d LIMIT 1', - $commit->getTableName(), - $repository_id, - idx($maxes, $repository_id, 0)); - if ($last_commit) { - $last_commit = $last_commit['id']; - } else { - $last_commit = 0; - } - queryfx( - $conn_w, - 'INSERT INTO %T (repositoryID, lastCommitID, size, epoch) - VALUES (%d, %d, %d, %d) ON DUPLICATE KEY UPDATE - lastCommitID = VALUES(lastCommitID), - size = VALUES(size), - epoch = VALUES(epoch)', - PhabricatorRepository::TABLE_SUMMARY, - $repository_id, - $last_commit, - idx($sizes, $repository_id, 0), - idx($maxes, $repository_id, 0)); - echo '.'; -} -echo "\n".pht('Done.')."\n"; From a20f0674a98525fd1bae9826f5f1e2bb0f437439 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 09:28:50 -0700 Subject: [PATCH 05/13] Improve some documentation for "diffusion.commit.search" Summary: Ref T13195. See PHI851. Start by making some minor improvements here: - Clarify that the example of what constraints look like is just an example. - Add descriptions for parameters missing descriptions. Test Plan: Looked at API method page, saw more helpful/complete instructions. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19640 --- .../query/PhabricatorCommitSearchEngine.php | 25 ++++++++++++++----- .../PhabricatorSearchEngineAPIMethod.php | 4 ++- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 993626e1e3..e1b294a9d6 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -67,35 +67,48 @@ final class PhabricatorCommitSearchEngine ->setKey('responsiblePHIDs') ->setConduitKey('responsible') ->setAliases(array('responsible', 'responsibles', 'responsiblePHID')) - ->setDatasource(new DifferentialResponsibleDatasource()), + ->setDatasource(new DifferentialResponsibleDatasource()) + ->setDescription( + pht( + 'Find commits where given users, projects, or packages are '. + 'responsible for the next steps in the audit workflow.')), id(new PhabricatorUsersSearchField()) ->setLabel(pht('Authors')) ->setKey('authorPHIDs') ->setConduitKey('authors') - ->setAliases(array('author', 'authors', 'authorPHID')), + ->setAliases(array('author', 'authors', 'authorPHID')) + ->setDescription(pht('Find commits authored by particular users.')), id(new PhabricatorSearchDatasourceField()) ->setLabel(pht('Auditors')) ->setKey('auditorPHIDs') ->setConduitKey('auditors') ->setAliases(array('auditor', 'auditors', 'auditorPHID')) - ->setDatasource(new DiffusionAuditorFunctionDatasource()), + ->setDatasource(new DiffusionAuditorFunctionDatasource()) + ->setDescription( + pht( + 'Find commits where given users, projects, or packages are '. + 'auditors.')), id(new PhabricatorSearchCheckboxesField()) ->setLabel(pht('Audit Status')) ->setKey('statuses') ->setAliases(array('status')) - ->setOptions(PhabricatorAuditCommitStatusConstants::getStatusNameMap()), + ->setOptions(PhabricatorAuditCommitStatusConstants::getStatusNameMap()) + ->setDescription(pht('Find commits with given audit statuses.')), id(new PhabricatorSearchDatasourceField()) ->setLabel(pht('Repositories')) ->setKey('repositoryPHIDs') ->setConduitKey('repositories') ->setAliases(array('repository', 'repositories', 'repositoryPHID')) - ->setDatasource(new DiffusionRepositoryFunctionDatasource()), + ->setDatasource(new DiffusionRepositoryFunctionDatasource()) + ->setDescription(pht('Find commits in particular repositories.')), id(new PhabricatorSearchDatasourceField()) ->setLabel(pht('Packages')) ->setKey('packagePHIDs') ->setConduitKey('packages') ->setAliases(array('package', 'packages', 'packagePHID')) - ->setDatasource(new PhabricatorOwnersPackageDatasource()), + ->setDatasource(new PhabricatorOwnersPackageDatasource()) + ->setDescription( + pht('Find commits which affect given packages.')), id(new PhabricatorSearchThreeStateField()) ->setLabel(pht('Unreachable')) ->setKey('unreachable') diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 3d13f2c3d9..4a8fccb876 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -161,7 +161,9 @@ If you specify both a `queryKey` and `constraints`, the builtin or saved query will be applied first as a starting point, then any additional values in `constraints` will be applied, overwriting the defaults from the original query. -Specify constraints like this: +Different endpoints support different constraints. The constraints this method +supports are detailed below. As an example, you might specify constraints like +this: ```lang=json, name="Example Custom Constraints" { From 5a38b75f16ea8ba9ec95ce85a16b8dfa8382c173 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 10:06:47 -0700 Subject: [PATCH 06/13] In Conduit, let checkbox constraints self-document Summary: Ref T13195. Ref PHI851. Currently, checkbox constraints don't tell you what values are supported on the API page. You can figure this out with "Inspect Element" or by viewing the source code, but just render a nice table instead. Test Plan: {F5862969} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19641 --- src/__phutil_library_map__.php | 2 + .../data/ConduitConstantDescription.php | 26 +++++++++++ .../PhabricatorSearchEngineAPIMethod.php | 43 ++++++++++++++++++- .../PhabricatorSearchCheckboxesField.php | 12 ++++++ .../search/field/PhabricatorSearchField.php | 4 ++ 5 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 src/applications/conduit/data/ConduitConstantDescription.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3abc10ef49..9774741060 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -313,6 +313,7 @@ phutil_register_library_map(array( 'ConduitCallTestCase' => 'applications/conduit/call/__tests__/ConduitCallTestCase.php', 'ConduitColumnsParameterType' => 'applications/conduit/parametertype/ConduitColumnsParameterType.php', 'ConduitConnectConduitAPIMethod' => 'applications/conduit/method/ConduitConnectConduitAPIMethod.php', + 'ConduitConstantDescription' => 'applications/conduit/data/ConduitConstantDescription.php', 'ConduitEpochParameterType' => 'applications/conduit/parametertype/ConduitEpochParameterType.php', 'ConduitException' => 'applications/conduit/protocol/exception/ConduitException.php', 'ConduitGetCapabilitiesConduitAPIMethod' => 'applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php', @@ -5628,6 +5629,7 @@ phutil_register_library_map(array( 'ConduitCallTestCase' => 'PhabricatorTestCase', 'ConduitColumnsParameterType' => 'ConduitParameterType', 'ConduitConnectConduitAPIMethod' => 'ConduitAPIMethod', + 'ConduitConstantDescription' => 'Phobject', 'ConduitEpochParameterType' => 'ConduitParameterType', 'ConduitException' => 'Exception', 'ConduitGetCapabilitiesConduitAPIMethod' => 'ConduitAPIMethod', diff --git a/src/applications/conduit/data/ConduitConstantDescription.php b/src/applications/conduit/data/ConduitConstantDescription.php new file mode 100644 index 0000000000..fe2af2b716 --- /dev/null +++ b/src/applications/conduit/data/ConduitConstantDescription.php @@ -0,0 +1,26 @@ +key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setValue($value) { + $this->value = $value; + return $this; + } + + public function getValue() { + return $this->value; + } + +} diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 4a8fccb876..335ba58852 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -190,15 +190,26 @@ EOTEXT $fields, array('ids', 'phids')) + $fields; + $constant_lists = array(); + $rows = array(); foreach ($fields as $field) { $key = $field->getConduitKey(); $label = $field->getLabel(); + $constants = $field->newConduitConstants(); + $type_object = $field->getConduitParameterType(); if ($type_object) { $type = $type_object->getTypeName(); $description = $field->getDescription(); + if ($constants) { + $description = array( + $description, + ' ', + phutil_tag('em', array(), pht('(See table below.)')), + ); + } } else { $type = null; $description = phutil_tag('em', array(), pht('Not supported.')); @@ -210,6 +221,35 @@ EOTEXT $type, $description, ); + + if ($constants) { + $constant_lists[] = $this->buildRemarkup( + pht( + 'Constants supported by the `%s` constraint:', + 'statuses')); + + $constants_rows = array(); + foreach ($constants as $constant) { + $constants_rows[] = array( + $constant->getKey(), + $constant->getValue(), + ); + } + + $constants_table = id(new AphrontTableView($constants_rows)) + ->setHeaders( + array( + pht('Key'), + pht('Value'), + )) + ->setColumnClasses( + array( + 'pre', + 'wide', + )); + + $constant_lists[] = $constants_table; + } } $table = id(new AphrontTableView($rows)) @@ -233,7 +273,8 @@ EOTEXT ->setCollapsed(true) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->appendChild($this->buildRemarkup($info)) - ->appendChild($table); + ->appendChild($table) + ->appendChild($constant_lists); } private function buildOrderBox( diff --git a/src/applications/search/field/PhabricatorSearchCheckboxesField.php b/src/applications/search/field/PhabricatorSearchCheckboxesField.php index e1a72f4576..0985c976ce 100644 --- a/src/applications/search/field/PhabricatorSearchCheckboxesField.php +++ b/src/applications/search/field/PhabricatorSearchCheckboxesField.php @@ -49,4 +49,16 @@ final class PhabricatorSearchCheckboxesField return new ConduitStringListParameterType(); } + public function newConduitConstants() { + $list = array(); + + foreach ($this->getOptions() as $key => $option) { + $list[] = id(new ConduitConstantDescription()) + ->setKey($key) + ->setValue($option); + } + + return $list; + } + } diff --git a/src/applications/search/field/PhabricatorSearchField.php b/src/applications/search/field/PhabricatorSearchField.php index 1a038ec665..36db0523b7 100644 --- a/src/applications/search/field/PhabricatorSearchField.php +++ b/src/applications/search/field/PhabricatorSearchField.php @@ -382,6 +382,10 @@ abstract class PhabricatorSearchField extends Phobject { return $this->enableForConduit; } + public function newConduitConstants() { + return array(); + } + /* -( Utility Methods )----------------------------------------------------- */ From ef26b06ca861d56f54e254699695076daa7fc4db Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 10:26:02 -0700 Subject: [PATCH 07/13] Begin transitioning audits to modern (string) status constants, from legacy (integer) status constants Summary: Ref T13195. See PHI851. Audits currently have older integer status constants. We've moved almost all object types away from this to string constants (which are better in basically every way, and particularly way better for exposing over the API). Commits/audits are currently accessible over the API and expose these constants via a "statuses" constraint. Prepare to move toward modern string constants by defining a new, more modern map of status details and defining the existing methods in terms of it. Test Plan: Browsed audits checking for icons/names/open-ness, saw no changes. This change should have no user-visible effects, as it just reorganizes code. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19642 --- .../PhabricatorAuditCommitStatusConstants.php | 132 +++++++++--------- 1 file changed, 68 insertions(+), 64 deletions(-) diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index 31bb2c22f7..877fd0b4ba 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -9,17 +9,16 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { const FULLY_AUDITED = 4; const NEEDS_VERIFICATION = 5; - public static function getStatusNameMap() { - $map = array( - self::NONE => pht('No Audits'), - self::NEEDS_AUDIT => pht('Audit Required'), - self::CONCERN_RAISED => pht('Concern Raised'), - self::NEEDS_VERIFICATION => pht('Needs Verification'), - self::PARTIALLY_AUDITED => pht('Partially Audited'), - self::FULLY_AUDITED => pht('Audited'), - ); + const MODERN_NONE = 'none'; + const MODERN_NEEDS_AUDIT = 'needs-audit'; + const MODERN_CONCERN_RAISED = 'concern-raised'; + const MODERN_PARTIALLY_AUDITED = 'partially-audited'; + const MODERN_AUDITED = 'audited'; + const MODERN_NEEDS_VERIFICATION = 'needs-verification'; - return $map; + public static function getStatusNameMap() { + $map = self::getMap(); + return ipull($map, 'name', 'legacy'); } public static function getStatusName($code) { @@ -27,66 +26,71 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { } public static function getOpenStatusConstants() { - return array( - self::CONCERN_RAISED, - self::NEEDS_AUDIT, - self::NEEDS_VERIFICATION, - self::PARTIALLY_AUDITED, - ); + $constants = array(); + foreach (self::getMap() as $map) { + if (!$map['closed']) { + $constants[] = $map['legacy']; + } + } + return $constants; } public static function getStatusColor($code) { - switch ($code) { - case self::CONCERN_RAISED: - $color = 'red'; - break; - case self::NEEDS_AUDIT: - $color = 'orange'; - break; - case self::PARTIALLY_AUDITED: - $color = 'yellow'; - break; - case self::FULLY_AUDITED: - $color = 'green'; - break; - case self::NONE: - $color = 'bluegrey'; - break; - case self::NEEDS_VERIFICATION: - $color = 'indigo'; - break; - default: - $color = null; - break; - } - return $color; + $map = self::getMap(); + $map = ipull($map, 'color', 'legacy'); + return idx($map, $code); } public static function getStatusIcon($code) { - switch ($code) { - case self::CONCERN_RAISED: - $icon = 'fa-times-circle'; - break; - case self::NEEDS_AUDIT: - $icon = 'fa-exclamation-circle'; - break; - case self::PARTIALLY_AUDITED: - $icon = 'fa-check-circle-o'; - break; - case self::FULLY_AUDITED: - $icon = 'fa-check-circle'; - break; - case self::NONE: - $icon = 'fa-check'; - break; - case self::NEEDS_VERIFICATION: - $icon = 'fa-refresh'; - break; - default: - $icon = null; - break; - } - return $icon; + $map = self::getMap(); + $map = ipull($map, 'icon', 'legacy'); + return idx($map, $code); } + private static function getMap() { + return array( + self::MODERN_NONE => array( + 'name' => pht('No Audits'), + 'legacy' => self::NONE, + 'icon' => 'fa-check', + 'color' => 'bluegrey', + 'closed' => true, + ), + self::MODERN_NEEDS_AUDIT => array( + 'name' => pht('Audit Required'), + 'legacy' => self::NEEDS_AUDIT, + 'icon' => 'fa-exclamation-circle', + 'color' => 'orange', + 'closed' => false, + ), + self::MODERN_CONCERN_RAISED => array( + 'name' => pht('Concern Raised'), + 'legacy' => self::CONCERN_RAISED, + 'icon' => 'fa-times-circle', + 'color' => 'red', + 'closed' => false, + ), + self::MODERN_PARTIALLY_AUDITED => array( + 'name' => pht('Partially Audited'), + 'legacy' => self::PARTIALLY_AUDITED, + 'icon' => 'fa-check-circle-o', + 'color' => 'yellow', + 'closed' => false, + ), + self::MODERN_AUDITED => array( + 'name' => pht('Audited'), + 'legacy' => self::FULLY_AUDITED, + 'icon' => 'fa-check-circle', + 'color' => 'green', + 'closed' => true, + ), + self::MODERN_NEEDS_VERIFICATION => array( + 'name' => pht('Needs Verification'), + 'legacy' => self::NEEDS_VERIFICATION, + 'icon' => 'fa-refresh', + 'color' => 'indigo', + 'closed' => false, + ), + ); + } } From e8e5dc0f56900f0fa74d9733aae87b063e534452 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 6 Sep 2018 10:55:23 -0700 Subject: [PATCH 08/13] Make a language improvement ("inlines" -> "inline comments") Summary: See D19632. Agreed that this is more clear. Test Plan: Read carefully. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19644 --- .../differential/editor/DifferentialTransactionEditor.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index dc47683875..29dac38b5a 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -896,7 +896,7 @@ final class DifferentialTransactionEditor $limit_note = null; if (count($inlines) > $limit) { $limit_note = pht( - '(Showing first %s of %s inlines.)', + '(Showing first %s of %s inline comments.)', new PhutilNumber($limit), phutil_count($inlines)); From ab4d33bede84f03c295a7d46b21c8a5518adf5e6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 13:47:31 -0700 Subject: [PATCH 09/13] Allow underscores to appear in Harbormaster variable names Summary: See PHI859. Ref T13195. The regexp for replacing variables currently does not include underscores. Include underscores. I also made a note in T13088: we should (almost certainly?) throw immediately if you try to pass a bogus variable name as a custom parameter, but this is a slightly larger change. Test Plan: - Made an "http request" build plan with `?x=${initiator.phid}&y={$some_variable}`. - Added `some_variable` as a parameter to the parameter collection. - Before patch: `initiator.phid` was replaced, but `some_variable` was not. - After patch: both variables are replaced. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19645 --- .../harbormaster/step/HarbormasterBuildStepImplementation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php index ccf97451b7..e0dfe05cc0 100644 --- a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php @@ -193,7 +193,7 @@ abstract class HarbormasterBuildStepImplementation extends Phobject { * @return string String with variables replaced safely into it. */ protected function mergeVariables($function, $pattern, array $variables) { - $regexp = '@\\$\\{(?P[a-z\\./-]+)\\}@'; + $regexp = '@\\$\\{(?P[a-z\\./_-]+)\\}@'; $matches = null; preg_match_all($regexp, $pattern, $matches); From a1ce23b9f557a40151007ed9b0d40543d60448d1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Sep 2018 08:30:18 -0700 Subject: [PATCH 10/13] Introduce an AuditStatus object for commits and move some callsites to it Summary: Ref T13195. See PHI851. Add an object, analogous to the `DifferentialRevisionStatus` object, to handle audit status management. This will primarily make it easier to swap storage over to strings later, but also cleans things up a bit. Test Plan: Viewed audit/commit lists, saw sensible state icons. Ran `bin/audit synchronize`, got sensible output. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19646 --- .../PhabricatorAuditCommitStatusConstants.php | 44 +++++++++++++++++++ ...atorAuditSynchronizeManagementWorkflow.php | 12 +++-- .../audit/view/PhabricatorAuditListView.php | 11 ++--- .../view/DiffusionHistoryTableView.php | 8 ++-- .../PhabricatorRepositoryCommitPHIDType.php | 8 ++-- .../storage/PhabricatorRepositoryCommit.php | 5 +++ 6 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index 877fd0b4ba..e559862c83 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -2,6 +2,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { + private $key; + private $spec = array(); + const NONE = 0; const NEEDS_AUDIT = 1; const CONCERN_RAISED = 2; @@ -16,6 +19,47 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { const MODERN_AUDITED = 'audited'; const MODERN_NEEDS_VERIFICATION = 'needs-verification'; + public static function newForLegacyStatus($status) { + $map = self::getMap(); + + foreach ($map as $key => $spec) { + if (idx($spec, 'legacy') == $status) { + return self::newForStatus($key); + } + } + + return self::newForStatus($status); + } + + public static function newForStatus($status) { + $result = new self(); + + $result->key = $status; + + $map = self::getMap(); + if (isset($map[$status])) { + $result->spec = $map[$status]; + } + + return $result; + } + + public function getKey() { + return $this->key; + } + + public function getIcon() { + return idx($this->spec, 'icon'); + } + + public function getColor() { + return idx($this->spec, 'color'); + } + + public function getName() { + return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key)); + } + public static function getStatusNameMap() { $map = self::getMap(); return ipull($map, 'name', 'legacy'); diff --git a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php index db6ce096c4..96d06e65c2 100644 --- a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php @@ -30,11 +30,11 @@ final class PhabricatorAuditSynchronizeManagementWorkflow continue; } - $old_status = $commit->getAuditStatus(); + $old_status = $commit->getAuditStatusObject(); $commit->updateAuditStatus($commit->getAudits()); - $new_status = $commit->getAuditStatus(); + $new_status = $commit->getAuditStatusObject(); - if ($old_status == $new_status) { + if ($old_status->getKey() == $new_status->getKey()) { echo tsprintf( "%s\n", pht( @@ -46,10 +46,8 @@ final class PhabricatorAuditSynchronizeManagementWorkflow pht( 'Updating "%s": "%s" -> "%s".', $commit->getDisplayName(), - PhabricatorAuditCommitStatusConstants::getStatusName( - $old_status), - PhabricatorAuditCommitStatusConstants::getStatusName( - $new_status))); + $old_status->getName(), + $new_status->getName())); $commit->save(); } diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php index cb9fecac3a..fb56e7cd55 100644 --- a/src/applications/audit/view/PhabricatorAuditListView.php +++ b/src/applications/audit/view/PhabricatorAuditListView.php @@ -120,14 +120,11 @@ final class PhabricatorAuditListView extends AphrontView { $commit_desc = $this->getCommitDescription($commit_phid); $committed = phabricator_datetime($commit->getEpoch(), $viewer); - $status = $commit->getAuditStatus(); + $status = $commit->getAuditStatusObject(); - $status_text = - PhabricatorAuditCommitStatusConstants::getStatusName($status); - $status_color = - PhabricatorAuditCommitStatusConstants::getStatusColor($status); - $status_icon = - PhabricatorAuditCommitStatusConstants::getStatusIcon($status); + $status_text = $status->getName(); + $status_color = $status->getColor(); + $status_icon = $status->getIcon(); $author_phid = $commit->getAuthorPHID(); if ($author_phid) { diff --git a/src/applications/diffusion/view/DiffusionHistoryTableView.php b/src/applications/diffusion/view/DiffusionHistoryTableView.php index 3885bbf47c..cfd7019679 100644 --- a/src/applications/diffusion/view/DiffusionHistoryTableView.php +++ b/src/applications/diffusion/view/DiffusionHistoryTableView.php @@ -114,10 +114,10 @@ final class DiffusionHistoryTableView extends DiffusionHistoryView { 'type' => $history->getFileType(), )); - $status = $commit->getAuditStatus(); - $icon = PhabricatorAuditCommitStatusConstants::getStatusIcon($status); - $color = PhabricatorAuditCommitStatusConstants::getStatusColor($status); - $name = PhabricatorAuditCommitStatusConstants::getStatusName($status); + $status = $commit->getAuditStatusObject(); + $icon = $status->getIcon(); + $color = $status->getColor(); + $name = $status->getName(); $audit_view = id(new PHUIIconView()) ->setIcon($icon, $color) diff --git a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php index b5abd40032..c37bdc04f9 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php @@ -82,10 +82,10 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType { $handle->setURI($commit->getURI()); $handle->setTimestamp($commit->getEpoch()); - $status = $commit->getAuditStatus(); - $icon = PhabricatorAuditCommitStatusConstants::getStatusIcon($status); - $color = PhabricatorAuditCommitStatusConstants::getStatusColor($status); - $name = PhabricatorAuditCommitStatusConstants::getStatusName($status); + $status = $commit->getAuditStatusObject(); + $icon = $status->getIcon(); + $color = $status->getColor(); + $name = $status->getName(); $handle ->setStateIcon($icon) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index f36869686f..447db08e6c 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -530,6 +530,11 @@ final class PhabricatorRepositoryCommit return $data->getCommitDetail('authorPHID'); } + public function getAuditStatusObject() { + $status = $this->getAuditStatus(); + return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($status); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ public function getCapabilities() { From 046c1b5b828d2ead3ffc5de26ab5abae43d3ae8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Sep 2018 09:42:16 -0700 Subject: [PATCH 11/13] Expose "isDraft" and "holdAsDraft" revision properties over Conduit Summary: Ref T13195. See PHI861. The "+ Draft" flag is not currently exposed over the API, but seems stable enough to expose. Also expose the "hold as draft" flag, normally `arc diff --draft`. Today, you can get "+ Draft" with some other state by: - abandoning a draft revision ("Abandoned + Draft"); or - using `arc diff --plan-changes` with an older `arc` version ("Changes Planned + Draft"). Test Plan: Called `differential.revision.search` via Conduit and got sensible-looking results for revisions in various states. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19648 --- .../storage/DifferentialRevision.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index a432c02a96..9b169f3c67 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -1152,6 +1152,20 @@ final class DifferentialRevision extends DifferentialDAO ->setKey('testPlan') ->setType('string') ->setDescription(pht('Revision test plan.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('isDraft') + ->setType('bool') + ->setDescription( + pht( + 'True if this revision is in any draft state, and thus not '. + 'notifying reviewers and subscribers about changes.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('holdAsDraft') + ->setType('bool') + ->setDescription( + pht( + 'True if this revision is being held as a draft. It will not be '. + 'automatically submitted for review even if tests pass.')), ); } @@ -1172,6 +1186,8 @@ final class DifferentialRevision extends DifferentialDAO 'diffPHID' => $this->getActiveDiffPHID(), 'summary' => $this->getSummary(), 'testPlan' => $this->getTestPlan(), + 'isDraft' => !$this->getShouldBroadcast(), + 'holdAsDraft' => (bool)$this->getHoldAsDraft(), ); } From 16a6fc8341ea2fa9b940c914e5f7a961c653eb93 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Sep 2018 11:33:24 -0700 Subject: [PATCH 12/13] Allow reviewers to mark their own inlines as "Done" before they submit them Summary: Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them. If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response. Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195, T8573 Differential Revision: https://secure.phabricator.com/D19634 --- .../audit/editor/PhabricatorAuditEditor.php | 24 ++++++++++++----- ...ifferentialInlineCommentEditController.php | 17 ++++++++++-- .../editor/DifferentialTransactionEditor.php | 27 ++++++++++++++----- .../DiffusionInlineCommentController.php | 18 ++++++++++--- .../view/PHUIDiffInlineCommentDetailView.php | 21 ++++++++++----- 5 files changed, 83 insertions(+), 24 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 984e2c1472..c04bb8185b 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -258,19 +258,31 @@ final class PhabricatorAuditEditor $this->didExpandInlineState = true; $actor_phid = $this->getActingAsPHID(); - $actor_is_author = ($object->getAuthorPHID() == $actor_phid); - if (!$actor_is_author) { - break; - } + $author_phid = $object->getAuthorPHID(); + $actor_is_author = ($actor_phid == $author_phid); $state_map = PhabricatorTransactions::getInlineStateMap(); - $inlines = id(new DiffusionDiffInlineCommentQuery()) + $query = id(new DiffusionDiffInlineCommentQuery()) ->setViewer($this->getActor()) ->withCommitPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)) + ->withFixedStates(array_keys($state_map)); + + $inlines = array(); + + $inlines[] = id(clone $query) + ->withAuthorPHIDs(array($actor_phid)) + ->withHasTransaction(false) ->execute(); + if ($actor_is_author) { + $inlines[] = id(clone $query) + ->withHasTransaciton(true) + ->execute(); + } + + $inlines = array_mergev($inlines); + if (!$inlines) { break; } diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 2e44e86822..9741cc93ee 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -118,8 +118,21 @@ final class DifferentialInlineCommentEditController throw new Exception(pht('Unable to load revision.')); } - if ($revision->getAuthorPHID() !== $viewer->getPHID()) { - throw new Exception(pht('You are not the revision owner.')); + $viewer_phid = $viewer->getPHID(); + $is_owner = ($viewer_phid == $revision->getAuthorPHID()); + $is_author = ($viewer_phid == $inline->getAuthorPHID()); + $is_draft = ($inline->isDraft()); + + if ($is_owner) { + // You own the revision, so you can mark the comment as "Done". + } else if ($is_author && $is_draft) { + // You made this comment and it's still a draft, so you can mark + // it as "Done". + } else { + throw new Exception( + pht( + 'You are not the revision owner, and this is not a draft comment '. + 'you authored.')); } return $inline; diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 29dac38b5a..c452cb9346 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -248,19 +248,34 @@ final class DifferentialTransactionEditor $this->didExpandInlineState = true; $actor_phid = $this->getActingAsPHID(); - $actor_is_author = ($object->getAuthorPHID() == $actor_phid); - if (!$actor_is_author) { - break; - } + $author_phid = $object->getAuthorPHID(); + $actor_is_author = ($actor_phid == $author_phid); $state_map = PhabricatorTransactions::getInlineStateMap(); - $inlines = id(new DifferentialDiffInlineCommentQuery()) + $query = id(new DifferentialDiffInlineCommentQuery()) ->setViewer($this->getActor()) ->withRevisionPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)) + ->withFixedStates(array_keys($state_map)); + + $inlines = array(); + + // We're going to undraft any "done" marks on your own inlines. + $inlines[] = id(clone $query) + ->withAuthorPHIDs(array($actor_phid)) + ->withHasTransaction(false) ->execute(); + // If you're the author, we also undraft any "done" marks on other + // inlines. + if ($actor_is_author) { + $inlines[] = id(clone $query) + ->withHasTransaction(true) + ->execute(); + } + + $inlines = array_mergev($inlines); + if (!$inlines) { break; } diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php index bc7448a9e2..ce69511dde 100644 --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/src/applications/diffusion/controller/DiffusionInlineCommentController.php @@ -75,9 +75,21 @@ final class DiffusionInlineCommentController throw new Exception(pht('Failed to load commit.')); } - if ((!$commit->getAuthorPHID()) || - ($commit->getAuthorPHID() != $viewer->getPHID())) { - throw new Exception(pht('You can not mark this comment as complete.')); + $owner_phid = $commit->getAuthorPHID(); + $viewer_phid = $viewer->getPHID(); + $viewer_is_owner = ($owner_phid && ($owner_phid == $viewer_phid)); + $viewer_is_author = ($viewer_phid == $inline->getAuthorPHID()); + $is_draft = $inline->isDraft(); + + if ($viewer_is_owner) { + // You can mark inlines on your own commits as "Done". + } else if ($viewer_is_author && $is_draft) { + // You can mark your own unsubmitted inlines as "Done". + } else { + throw new Exception( + pht( + 'You can not mark this comment as complete: you did not author '. + 'the commit and the comment is not a draft you wrote.')); } return $inline; diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index a32197059a..4da3bbd8fa 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -287,15 +287,24 @@ final class PHUIDiffInlineCommentDetailView $done_button = null; + $mark_done = $this->getCanMarkDone(); + + // Allow users to mark their own draft inlines as "Done". + if ($viewer_phid == $inline->getAuthorPHID()) { + if ($inline->isDraft()) { + $mark_done = true; + } + } + if (!$is_synthetic) { $draft_state = false; switch ($inline->getFixedState()) { case PhabricatorInlineCommentInterface::STATE_DRAFT: - $is_done = ($this->getCanMarkDone()); + $is_done = $mark_done; $draft_state = true; break; case PhabricatorInlineCommentInterface::STATE_UNDRAFT: - $is_done = !($this->getCanMarkDone()); + $is_done = !$mark_done; $draft_state = true; break; case PhabricatorInlineCommentInterface::STATE_DONE: @@ -309,7 +318,7 @@ final class PHUIDiffInlineCommentDetailView // If you don't have permission to mark the comment as "Done", you also // can not see the draft state. - if (!$this->getCanMarkDone()) { + if (!$mark_done) { $draft_state = false; } @@ -321,21 +330,19 @@ final class PHUIDiffInlineCommentDetailView $classes[] = 'inline-state-is-draft'; } - if ($this->getCanMarkDone()) { + if ($mark_done && !$this->preview) { $done_input = javelin_tag( 'input', array( 'type' => 'checkbox', 'checked' => ($is_done ? 'checked' : null), - 'disabled' => ($this->getCanMarkDone() ? null : 'disabled'), 'class' => 'differential-inline-done', 'sigil' => 'differential-inline-done', )); $done_button = phutil_tag( 'label', array( - 'class' => 'differential-inline-done-label '. - ($this->getCanMarkDone() ? null : 'done-is-disabled'), + 'class' => 'differential-inline-done-label ', ), array( $done_input, From 2a03367a50c0ab2dbeceda0f802bf6e29ae55055 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Sep 2018 11:33:28 -0700 Subject: [PATCH 13/13] When authors add inlines to their own revisions/commits, mark them as "Done" by default Summary: Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual. (If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.) Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195, T8573 Differential Revision: https://secure.phabricator.com/D19635 --- .../diff/PhabricatorInlineCommentController.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 22e410612a..e6572672ce 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -231,6 +231,15 @@ abstract class PhabricatorInlineCommentController $inline->setReplyToCommentPHID($this->getReplyToCommentPHID()); } + // If you own this object, mark your own inlines as "Done" by default. + $owner_phid = $this->loadObjectOwnerPHID($inline); + if ($owner_phid) { + if ($viewer->getPHID() == $owner_phid) { + $fixed_state = PhabricatorInlineCommentInterface::STATE_DRAFT; + $inline->setFixedState($fixed_state); + } + } + $this->saveComment($inline); return $this->buildRenderedCommentResponse(