From 7ef236c9a64779459eeeb0e6509cf88975b29102 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 19 Nov 2014 11:11:09 -0800 Subject: [PATCH] Differential - refine selecting a repository diff --> revision workflow Summary: Fixes T6200. Ref T6237. When creating a diff from the web view, allow the user to select the repository at that time. When viewing a diff that has no associated revision and then creating a revision, pass along the repository phid to the create revision controller. Within the create revision controller, default the repository selector to this repository phid. Finally, in the editor, stop aggressively resetting the repository phid for every TYPE_UPDATE; rather, do so if its not a new object -- the diff should reign supreme in that case -- or if there's no repository -- let the diff be the guide. Test Plan: - made a diff with an associated repo, made a revision from the diff, saw the associated repo and it stuck on save! - made a diff with an associated repo, made a revision from the diff but changed the repo and it stuck on save! - made a diff with an associated repo, made a revision from the diff but changed the repo to nothing and it stuck on save! Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T6237, T6200 Differential Revision: https://secure.phabricator.com/D10872 --- ...ferentialCreateRawDiffConduitAPIMethod.php | 1 + .../DifferentialDiffCreateController.php | 21 ++++++++++++- .../DifferentialDiffViewController.php | 3 ++ .../DifferentialRevisionEditController.php | 30 +++++++++++++++---- .../editor/DifferentialDiffEditor.php | 8 ++++- .../editor/DifferentialTransactionEditor.php | 12 +++++++- 6 files changed, 67 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php index 219f5e6dd2..68531ea6bc 100644 --- a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php @@ -61,6 +61,7 @@ final class DifferentialCreateRawDiffConduitAPIMethod id(new DifferentialDiffEditor()) ->setActor($viewer) ->setContentSourceFromConduitRequest($request) + ->setLookupRepository(false) // respect user choice ->applyTransactions($diff, $xactions); return $this->buildDiffInfoDictionary($diff); diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php index d2493b9f60..c092597fbb 100644 --- a/src/applications/differential/controller/DifferentialDiffCreateController.php +++ b/src/applications/differential/controller/DifferentialDiffCreateController.php @@ -7,12 +7,20 @@ final class DifferentialDiffCreateController extends DifferentialController { $request = $this->getRequest(); $diff = null; + $repository_phid = null; + $repository_value = array(); $errors = array(); $e_diff = null; $e_file = null; $validation_exception = null; if ($request->isFormPost()) { + $repository_tokenizer = $request->getArr( + id(new DifferentialRepositoryField())->getFieldKey()); + if ($repository_tokenizer) { + $repository_phid = reset($repository_tokenizer); + } + if ($request->getFileExists('diff-file')) { $diff = PhabricatorFile::readUploadedFileData($_FILES['diff-file']); } else { @@ -33,7 +41,7 @@ final class DifferentialDiffCreateController extends DifferentialController { 'differential.createrawdiff', array( 'diff' => $diff, - )); + 'repositoryPHID' => $repository_phid,)); $call->setUser($request->getUser()); $result = $call->execute(); $path = id(new PhutilURI($result['uri']))->getPath(); @@ -56,6 +64,10 @@ final class DifferentialDiffCreateController extends DifferentialController { $cancel_uri = $this->getApplicationURI(); + if ($repository_phid) { + $repository_value = $this->loadViewerHandles(array($repository_phid)); + } + $form ->setAction('/differential/diff/create/') ->setEncType('multipart/form-data') @@ -81,6 +93,13 @@ final class DifferentialDiffCreateController extends DifferentialController { ->setLabel(pht('Raw Diff From File')) ->setName('diff-file') ->setError($e_file)) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setName(id(new DifferentialRepositoryField())->getFieldKey()) + ->setLabel(pht('Repository')) + ->setDatasource(new DiffusionRepositoryDatasource()) + ->setValue($repository_value) + ->setLimit(1)) ->appendChild( id(new AphrontFormSubmitControl()) ->addCancelButton($cancel_uri) diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index 9bb61cff3b..39f2d18b15 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -80,6 +80,9 @@ final class DifferentialDiffViewController extends DifferentialController { ->setAction('/differential/revision/edit/') ->addHiddenInput('diffID', $diff->getID()) ->addHiddenInput('viaDiffView', 1) + ->addHiddenInput( + id(new DifferentialRepositoryField())->getFieldKey(), + $diff->getRepositoryPHID()) ->appendRemarkupInstructions( pht( 'Review the diff for correctness. When you are satisfied, either '. diff --git a/src/applications/differential/controller/DifferentialRevisionEditController.php b/src/applications/differential/controller/DifferentialRevisionEditController.php index 12dac2ca1d..14bcdf0476 100644 --- a/src/applications/differential/controller/DifferentialRevisionEditController.php +++ b/src/applications/differential/controller/DifferentialRevisionEditController.php @@ -71,16 +71,41 @@ final class DifferentialRevisionEditController ->setViewer($viewer) ->readFieldsFromStorage($revision); + if ($request->getStr('viaDiffView') && $diff) { + $repo_key = id(new DifferentialRepositoryField())->getFieldKey(); + $repository_field = idx( + $field_list->getFields(), + $repo_key); + if ($repository_field) { + $repository_field->setValue($request->getStr($repo_key)); + } + } + $validation_exception = null; if ($request->isFormPost() && !$request->getStr('viaDiffView')) { + + $editor = id(new DifferentialTransactionEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true); + $xactions = $field_list->buildFieldTransactionsFromRequest( new DifferentialTransaction(), $request); if ($diff) { + $repository_phid = null; + $repository_tokenizer = $request->getArr( + id(new DifferentialRepositoryField())->getFieldKey()); + if ($repository_tokenizer) { + $repository_phid = reset($repository_tokenizer); + } + $xactions[] = id(new DifferentialTransaction()) ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) ->setNewValue($diff->getPHID()); + + $editor->setRepositoryPHIDOverride($repository_phid); } $comments = $request->getStr('comments'); @@ -92,11 +117,6 @@ final class DifferentialRevisionEditController ->setContent($comments)); } - $editor = id(new DifferentialTransactionEditor()) - ->setActor($viewer) - ->setContentSourceFromRequest($request) - ->setContinueOnNoEffect(true); - try { $editor->applyTransactions($revision, $xactions); $revision_uri = '/D'.$revision->getID(); diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php index 0d87df60de..c2513dab7a 100644 --- a/src/applications/differential/editor/DifferentialDiffEditor.php +++ b/src/applications/differential/editor/DifferentialDiffEditor.php @@ -4,6 +4,12 @@ final class DifferentialDiffEditor extends PhabricatorApplicationTransactionEditor { private $diffDataDict; + private $lookupRepository = true; + + public function setLookupRepository($bool) { + $this->lookupRepository = $bool; + return $this; + } public function getEditorApplicationClass() { return 'PhabricatorDifferentialApplication'; @@ -80,7 +86,7 @@ final class DifferentialDiffEditor // is old, or couldn't figure out which repository the working copy // belongs to), apply heuristics to try to figure it out. - if (!$object->getRepositoryPHID()) { + if ($this->lookupRepository && !$object->getRepositoryPHID()) { $repository = id(new DifferentialRepositoryLookup()) ->setDiff($object) ->setViewer($this->getActor()) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index a90ead7c0a..f2adcfc78f 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -6,6 +6,7 @@ final class DifferentialTransactionEditor private $heraldEmailPHIDs; private $changedPriorToCommitURI; private $isCloseByCommit; + private $repositoryPHIDOverride = false; public function getEditorApplicationClass() { return 'PhabricatorDifferentialApplication'; @@ -45,6 +46,11 @@ final class DifferentialTransactionEditor return $this->changedPriorToCommitURI; } + public function setRepositoryPHIDOverride($phid_or_null) { + $this->repositoryPHIDOverride = $phid_or_null; + return $this; + } + public function getTransactionTypes() { $types = parent::getTransactionTypes(); @@ -205,7 +211,11 @@ final class DifferentialTransactionEditor $diff = $this->requireDiff($xaction->getNewValue()); $object->setLineCount($diff->getLineCount()); - $object->setRepositoryPHID($diff->getRepositoryPHID()); + if ($this->repositoryPHIDOverride !== false) { + $object->setRepositoryPHID($this->repositoryPHIDOverride); + } else { + $object->setRepositoryPHID($diff->getRepositoryPHID()); + } $object->setArcanistProjectPHID($diff->getArcanistProjectPHID()); $object->attachActiveDiff($diff);