From c924351a58e40fe391c587899e322be006e98c7f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Dec 2017 16:11:58 -0800 Subject: [PATCH 1/6] Mark sessions as "signed all documents" when Legalpad has been uninstalled See PHI238. When an install uninstalls "Legalpad", we were incorrectly failing to mark sessions as "Signed All Required Documents" by bailing early. Test Plan: Uninstalled Legalpad, logged in. --- .../base/controller/PhabricatorController.php | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 0fee880378..5952a806ed 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -562,25 +562,25 @@ abstract class PhabricatorController extends AphrontController { return null; } + $must_sign_docs = array(); + $sign_docs = array(); + $legalpad_class = 'PhabricatorLegalpadApplication'; $legalpad_installed = PhabricatorApplication::isClassInstalledForViewer( $legalpad_class, $viewer); - if (!$legalpad_installed) { - return null; - } + if ($legalpad_installed) { + $sign_docs = id(new LegalpadDocumentQuery()) + ->setViewer($viewer) + ->withSignatureRequired(1) + ->needViewerSignatures(true) + ->setOrder('oldest') + ->execute(); - $sign_docs = id(new LegalpadDocumentQuery()) - ->setViewer($viewer) - ->withSignatureRequired(1) - ->needViewerSignatures(true) - ->setOrder('oldest') - ->execute(); - - $must_sign_docs = array(); - foreach ($sign_docs as $sign_doc) { - if (!$sign_doc->getUserSignature($viewer->getPHID())) { - $must_sign_docs[] = $sign_doc; + foreach ($sign_docs as $sign_doc) { + if (!$sign_doc->getUserSignature($viewer->getPHID())) { + $must_sign_docs[] = $sign_doc; + } } } From 46d496b8cc061db11954bcd2c03b882a5573eaad Mon Sep 17 00:00:00 2001 From: lkassianik Date: Tue, 5 Dec 2017 19:24:57 +0000 Subject: [PATCH 2/6] Fix error for URL's that could mean several commits Summary: Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct. Test Plan: - Navigate to `install/rPbd3c23` - User should see a list view providing links to `install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38` and `install/rPbd3c239d5aada68a31db5742bbb8ec099074a561` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T13001 Differential Revision: https://secure.phabricator.com/D18816 --- .../controller/DiffusionCommitController.php | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index cdce8702ca..11463ef97d 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -39,20 +39,23 @@ final class DiffusionCommitController extends DiffusionController { return $this->buildRawDiffResponse($drequest); } - $commit = id(new DiffusionCommitQuery()) + $commits = id(new DiffusionCommitQuery()) ->setViewer($viewer) ->withRepository($repository) ->withIdentifiers(array($commit_identifier)) ->needCommitData(true) ->needAuditRequests(true) - ->executeOne(); + ->setLimit(100) + ->execute(); + + $multiple_results = count($commits) > 1; $crumbs = $this->buildCrumbs(array( - 'commit' => true, + 'commit' => !$multiple_results, )); $crumbs->setBorder(true); - if (!$commit) { + if (!$commits) { if (!$this->getCommitExists()) { return new Aphront404Response(); } @@ -70,7 +73,40 @@ final class DiffusionCommitController extends DiffusionController { ->setTitle($title) ->setCrumbs($crumbs) ->appendChild($error); + } else if ($multiple_results) { + $warning_message = + pht( + 'The identifier %s is ambiguous and matches more than one commit.', + phutil_tag( + 'strong', + array(), + $commit_identifier)); + + $error = id(new PHUIInfoView()) + ->setTitle(pht('Ambiguous Commit')) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->appendChild($warning_message); + + $list = id(new DiffusionCommitListView()) + ->setViewer($viewer) + ->setCommits($commits) + ->setNoDataString(pht('No recent commits.')); + + $crumbs->addTextCrumb(pht('Ambiguous Commit')); + + $matched_commits = id(new PHUITwoColumnView()) + ->setFooter(array( + $error, + $list, + )); + + return $this->newPage() + ->setTitle(pht('Ambiguous Commit')) + ->setCrumbs($crumbs) + ->appendChild($matched_commits); + } else { + $commit = head($commits); } $audit_requests = $commit->getAudits(); From a989dd181dd1f1115d728595c2adb482784c4ce0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Dec 2017 05:35:06 -0800 Subject: [PATCH 3/6] Fix Mercurial commit history ordering Summary: See . In D18769, I rewrote this from using the `--branch` flag (which is unsafe and does not function on branches named `--config=x.y` and such). However, this rewrite accidentally changed the result order, which impacted Mercurial commit hisotry lists and graphs. Swap the order of the constraints so we get newest-to-oldest again, as expected. Test Plan: Viewed a Mercurial repository's history graph, saw sensible chronology after the patch. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18817 --- .../diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php index e36c0a124e..4c1d39e8c8 100644 --- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php @@ -130,7 +130,7 @@ final class DiffusionHistoryQueryConduitAPIMethod } else { $path_arg = ''; $revset_arg = hgsprintf( - 'branch(%s) and reverse(ancestors(%s))', + 'reverse(ancestors(%s)) and branch(%s)', $drequest->getBranch(), $commit_hash); } From 4f8340c05f2bf51d6b86e1e589153fc82ddbb708 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Dec 2017 05:42:03 -0800 Subject: [PATCH 4/6] Restore the "Log In" menubar action Summary: See . In T13024, I rewrote the main menu bar to hide potentially sensitive items (like notification and message counts and saved search filters) until users fully log in. However, the "Log In" item got caught in this too. For clarity, rename `shouldAllowPartialSessions()` to `shouldRequireFullSession()` (since logged-out users don't have any session at all, so it would be a bit misleading to say that "Log In" "allows" a partial session). Then let "Log In" work again for logged-out users. (In most cases, users are prompted to log in when they take an action which requires them to be logged in -- like creating or editing an object, or adding comments -- so this item doesn't really need to exist. However, it aligns better with user expectations in many cases to have it present, and some reasonable operations like "Check if I have notifications/messages" don't have an obvious thing to click otherwise.) Test Plan: Viewed site in an incognito window, saw "Log In" button again. Browsed normally, saw normal menu. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18818 --- .../auth/extension/PhabricatorAuthMainMenuBarExtension.php | 4 ++++ .../people/engineextension/PeopleMainMenuBarExtension.php | 4 ++-- src/view/page/menu/PhabricatorMainMenuBarExtension.php | 4 ++-- src/view/page/menu/PhabricatorMainMenuView.php | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php index d9d251ab07..d9fb5d013b 100644 --- a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php +++ b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php @@ -9,6 +9,10 @@ final class PhabricatorAuthMainMenuBarExtension return true; } + public function shouldRequireFullSession() { + return false; + } + public function getExtensionOrder() { return 900; } diff --git a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php index 01f1ba7cc1..bed9dde44e 100644 --- a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php +++ b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php @@ -9,8 +9,8 @@ final class PeopleMainMenuBarExtension return $viewer->isLoggedIn(); } - public function shouldAllowPartialSessions() { - return true; + public function shouldRequireFullSession() { + return false; } public function getExtensionOrder() { diff --git a/src/view/page/menu/PhabricatorMainMenuBarExtension.php b/src/view/page/menu/PhabricatorMainMenuBarExtension.php index d893cccac9..e66b1f2c7d 100644 --- a/src/view/page/menu/PhabricatorMainMenuBarExtension.php +++ b/src/view/page/menu/PhabricatorMainMenuBarExtension.php @@ -51,8 +51,8 @@ abstract class PhabricatorMainMenuBarExtension extends Phobject { return true; } - public function shouldAllowPartialSessions() { - return false; + public function shouldRequireFullSession() { + return true; } public function isExtensionEnabledForViewer(PhabricatorUser $viewer) { diff --git a/src/view/page/menu/PhabricatorMainMenuView.php b/src/view/page/menu/PhabricatorMainMenuView.php index ba4bda41ea..ad9510f348 100644 --- a/src/view/page/menu/PhabricatorMainMenuView.php +++ b/src/view/page/menu/PhabricatorMainMenuView.php @@ -106,7 +106,7 @@ final class PhabricatorMainMenuView extends AphrontView { if (!$is_full) { foreach ($extensions as $key => $extension) { - if (!$extension->shouldAllowPartialSessions()) { + if ($extension->shouldRequireFullSession()) { unset($extensions[$key]); } } From 6d3baa92f90813b77b2acbc1e6860ada5f28f60c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Dec 2017 06:47:48 -0800 Subject: [PATCH 5/6] Execute Herald again after promoting revisions out of the "Draft" state Summary: Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald. This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should. Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes. Test Plan: - Created a revision, reviewed Herald transcripts. - Saw three Herald passes: - First pass (revision creation) triggered builds and no email. - Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft). - Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13027, T2543 Differential Revision: https://secure.phabricator.com/D18819 --- .../editor/DifferentialTransactionEditor.php | 16 ++------ ...entialRevisionRequestReviewTransaction.php | 14 ++++--- ...habricatorApplicationTransactionEditor.php | 39 +++++++++++++++++++ 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index ae652ba0ec..b2fc179002 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1588,11 +1588,8 @@ final class DifferentialTransactionEditor ->setAuthorPHID($author_phid) ->setTransactionType( DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE) - ->setOldValue(false) ->setNewValue(true); - $xaction = $this->populateTransaction($object, $xaction); - // If we're creating this revision and immediately moving it out of // the draft state, mark this as a create transaction so it gets // hidden in the timeline and mail, since it isn't interesting: it @@ -1601,15 +1598,10 @@ final class DifferentialTransactionEditor $xaction->setIsCreateTransaction(true); } - $object->openTransaction(); - $object - ->setStatus(DifferentialRevisionStatus::NEEDS_REVIEW) - ->save(); - - $xaction->save(); - $object->saveTransaction(); - - $xactions[] = $xaction; + // Queue this transaction and apply it separately after the current + // batch of transactions finishes so that Herald can fire on the new + // revision state. See T13027 for discussion. + $this->queueTransaction($xaction); } } diff --git a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php index 465b5234d5..ed0d20a0b6 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php @@ -57,11 +57,15 @@ final class DifferentialRevisionRequestReviewTransaction 'revisions.')); } - if (!$this->isViewerRevisionAuthor($object, $viewer)) { - throw new Exception( - pht( - 'You can not request review of this revision because you are not '. - 'the author of the revision.')); + // When revisions automatically promote out of "Draft" after builds finish, + // the viewer may be acting as the Harbormaster application. + if (!$viewer->isOmnipotent()) { + if (!$this->isViewerRevisionAuthor($object, $viewer)) { + throw new Exception( + pht( + 'You can not request review of this revision because you are not '. + 'the author of the revision.')); + } } } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 175a5775ee..8e6d816769 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -70,6 +70,8 @@ abstract class PhabricatorApplicationTransactionEditor private $feedRelatedPHIDs = array(); private $modularTypes; + private $transactionQueue = array(); + const STORAGE_ENCODING_BINARY = 'binary'; /** @@ -1174,6 +1176,8 @@ abstract class PhabricatorApplicationTransactionEditor 'priority' => PhabricatorWorker::PRIORITY_ALERTS, )); + $this->flushTransactionQueue($object); + return $xactions; } @@ -3864,4 +3868,39 @@ abstract class PhabricatorApplicationTransactionEditor return pht('%s created an object: %s.', $author, $object); } +/* -( Queue )-------------------------------------------------------------- */ + + protected function queueTransaction( + PhabricatorApplicationTransaction $xaction) { + $this->transactionQueue[] = $xaction; + return $this; + } + + private function flushTransactionQueue($object) { + if (!$this->transactionQueue) { + return; + } + + $xactions = $this->transactionQueue; + $this->transactionQueue = array(); + + $editor = $this->newQueueEditor(); + + return $editor->applyTransactions($object, $xactions); + } + + private function newQueueEditor() { + $editor = id(newv(get_class($this), array())) + ->setActor($this->getActor()) + ->setContentSource($this->getContentSource()) + ->setContinueOnNoEffect($this->getContinueOnNoEffect()) + ->setContinueOnMissingFields($this->getContinueOnMissingFields()); + + if ($this->actingAsPHID !== null) { + $editor->setActingAsPHID($this->actingAsPHID); + } + + return $editor; + } + } From 60e5c0ec1b3e13cb2b2f9b73525f8354c206c23f Mon Sep 17 00:00:00 2001 From: Tim Hirsh Date: Fri, 8 Dec 2017 11:54:42 -0500 Subject: [PATCH 6/6] Add drydock.blueprint.edit Conduit method Summary: Ref: https://admin.phacility.com/PHI243 Since our use case primarily focuses on transaction editing, this patch implements the `drydock.blueprint.edit` api method with the understanding that: a) this is a work in progress b) object editing is supported, but object creation is not yet implemented Test Plan: * updated existing blueprints via Conduit UI * regression tested `maniphest.edit` by creating new and updating existing tasks Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, yelirekim, jcox Differential Revision: https://secure.phabricator.com/D18822 --- src/__phutil_library_map__.php | 2 ++ .../DrydockBlueprintEditConduitAPIMethod.php | 20 +++++++++++++++++++ .../editor/DrydockBlueprintEditEngine.php | 11 ++++++++++ .../editengine/PhabricatorEditEngine.php | 11 +++++++++- 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 11be8a50f0..35be621604 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1004,6 +1004,7 @@ phutil_register_library_map(array( 'DrydockBlueprintCustomField' => 'applications/drydock/customfield/DrydockBlueprintCustomField.php', 'DrydockBlueprintDatasource' => 'applications/drydock/typeahead/DrydockBlueprintDatasource.php', 'DrydockBlueprintDisableController' => 'applications/drydock/controller/DrydockBlueprintDisableController.php', + 'DrydockBlueprintEditConduitAPIMethod' => 'applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php', 'DrydockBlueprintEditController' => 'applications/drydock/controller/DrydockBlueprintEditController.php', 'DrydockBlueprintEditEngine' => 'applications/drydock/editor/DrydockBlueprintEditEngine.php', 'DrydockBlueprintEditor' => 'applications/drydock/editor/DrydockBlueprintEditor.php', @@ -6090,6 +6091,7 @@ phutil_register_library_map(array( 'DrydockBlueprintCustomField' => 'PhabricatorCustomField', 'DrydockBlueprintDatasource' => 'PhabricatorTypeaheadDatasource', 'DrydockBlueprintDisableController' => 'DrydockBlueprintController', + 'DrydockBlueprintEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'DrydockBlueprintEditController' => 'DrydockBlueprintController', 'DrydockBlueprintEditEngine' => 'PhabricatorEditEngine', 'DrydockBlueprintEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php b/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php new file mode 100644 index 0000000000..764c077d87 --- /dev/null +++ b/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php @@ -0,0 +1,20 @@ +setBlueprintImplementation($impl); + return $this->newEditableObject(); + } + protected function newObjectQuery() { return new DrydockBlueprintQuery(); } diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 6c9cb5e201..8ba49f4d77 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -630,6 +630,15 @@ abstract class PhabricatorEditEngine return $this->isCreate; } + /** + * Initialize a new object for documentation creation. + * + * @return object Newly initialized object. + * @task load + */ + protected function newEditableObjectForDocumentation() { + return $this->newEditableObject(); + } /** * Flag this workflow as a create or edit. @@ -2198,7 +2207,7 @@ abstract class PhabricatorEditEngine return array(); } - $object = $this->newEditableObject(); + $object = $this->newEditableObjectForDocumentation(); $fields = $this->buildEditFields($object); return $this->getConduitEditTypesFromFields($fields); }