From 65bc2b1ac53d2a51d93e6e02fd878ddfd261ec93 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Feb 2014 16:32:55 -0800 Subject: [PATCH] Implement "Pro" version of revision editor, with one field Summary: Ref T3886. I spent a few hours trying to make `DifferentialFieldSpecification` extend `PhabricatorCustomField` so I could be more blunt in my approach here and just swap the whole thing over in one go (more or less like I did with Maniphest) but we have a ton of custom fields and things felt really shaky and the change was enormous and hard to keep track of. Instead, I'm going to do this more gradually and go field-by-field. This implements a CustomField version of the "Title" field. (There are no links to this in the UI.) Test Plan: {F115353} {F115354} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3886 Differential Revision: https://secure.phabricator.com/D8276 --- src/__phutil_library_map__.php | 9 + .../PhabricatorApplicationDifferential.php | 2 + .../DifferentialRevisionEditControllerPro.php | 159 ++++++++++++++++++ .../customfield/DifferentialCustomField.php | 6 + .../customfield/DifferentialTitleField.php | 142 ++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 100 +++++++++++ .../storage/DifferentialRevision.php | 29 +++- 7 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 src/applications/differential/controller/DifferentialRevisionEditControllerPro.php create mode 100644 src/applications/differential/customfield/DifferentialCustomField.php create mode 100644 src/applications/differential/customfield/DifferentialTitleField.php create mode 100644 src/applications/differential/editor/DifferentialTransactionEditor.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4269d031a4..44f4896a3e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -355,6 +355,7 @@ phutil_register_library_map(array( 'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/DifferentialCommitsFieldSpecification.php', 'DifferentialConflictsFieldSpecification' => 'applications/differential/field/specification/DifferentialConflictsFieldSpecification.php', 'DifferentialController' => 'applications/differential/controller/DifferentialController.php', + 'DifferentialCustomField' => 'applications/differential/customfield/DifferentialCustomField.php', 'DifferentialCustomFieldDependsOnParser' => 'applications/differential/field/parser/DifferentialCustomFieldDependsOnParser.php', 'DifferentialCustomFieldDependsOnParserTestCase' => 'applications/differential/field/parser/__tests__/DifferentialCustomFieldDependsOnParserTestCase.php', 'DifferentialCustomFieldNumericIndex' => 'applications/differential/storage/DifferentialCustomFieldNumericIndex.php', @@ -444,6 +445,7 @@ phutil_register_library_map(array( 'DifferentialRevisionDetailRenderer' => 'applications/differential/controller/DifferentialRevisionDetailRenderer.php', 'DifferentialRevisionDetailView' => 'applications/differential/view/DifferentialRevisionDetailView.php', 'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php', + 'DifferentialRevisionEditControllerPro' => 'applications/differential/controller/DifferentialRevisionEditControllerPro.php', 'DifferentialRevisionEditor' => 'applications/differential/editor/DifferentialRevisionEditor.php', 'DifferentialRevisionIDFieldParserTestCase' => 'applications/differential/field/specification/__tests__/DifferentialRevisionIDFieldParserTestCase.php', 'DifferentialRevisionIDFieldSpecification' => 'applications/differential/field/specification/DifferentialRevisionIDFieldSpecification.php', @@ -462,9 +464,11 @@ phutil_register_library_map(array( 'DifferentialSummaryFieldSpecification' => 'applications/differential/field/specification/DifferentialSummaryFieldSpecification.php', 'DifferentialTasksAttacher' => 'applications/differential/DifferentialTasksAttacher.php', 'DifferentialTestPlanFieldSpecification' => 'applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php', + 'DifferentialTitleField' => 'applications/differential/customfield/DifferentialTitleField.php', 'DifferentialTitleFieldSpecification' => 'applications/differential/field/specification/DifferentialTitleFieldSpecification.php', 'DifferentialTransaction' => 'applications/differential/storage/DifferentialTransaction.php', 'DifferentialTransactionComment' => 'applications/differential/storage/DifferentialTransactionComment.php', + 'DifferentialTransactionEditor' => 'applications/differential/editor/DifferentialTransactionEditor.php', 'DifferentialTransactionQuery' => 'applications/differential/query/DifferentialTransactionQuery.php', 'DifferentialTransactionView' => 'applications/differential/view/DifferentialTransactionView.php', 'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/DifferentialUnitFieldSpecification.php', @@ -2887,6 +2891,7 @@ phutil_register_library_map(array( 'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialConflictsFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialController' => 'PhabricatorController', + 'DifferentialCustomField' => 'PhabricatorCustomField', 'DifferentialCustomFieldDependsOnParser' => 'PhabricatorCustomFieldMonogramParser', 'DifferentialCustomFieldDependsOnParserTestCase' => 'PhabricatorTestCase', 'DifferentialCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage', @@ -2975,9 +2980,11 @@ phutil_register_library_map(array( 4 => 'PhrequentTrackableInterface', 5 => 'HarbormasterBuildableInterface', 6 => 'PhabricatorSubscribableInterface', + 7 => 'PhabricatorCustomFieldInterface', ), 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', + 'DifferentialRevisionEditControllerPro' => 'DifferentialController', 'DifferentialRevisionEditor' => 'PhabricatorEditor', 'DifferentialRevisionIDFieldParserTestCase' => 'PhabricatorTestCase', 'DifferentialRevisionIDFieldSpecification' => 'DifferentialFieldSpecification', @@ -2998,9 +3005,11 @@ phutil_register_library_map(array( 'DifferentialSubscribeController' => 'DifferentialController', 'DifferentialSummaryFieldSpecification' => 'DifferentialFreeformFieldSpecification', 'DifferentialTestPlanFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialTitleField' => 'DifferentialCustomField', 'DifferentialTitleFieldSpecification' => 'DifferentialFreeformFieldSpecification', 'DifferentialTransaction' => 'PhabricatorApplicationTransaction', 'DifferentialTransactionComment' => 'PhabricatorApplicationTransactionComment', + 'DifferentialTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'DifferentialTransactionView' => 'PhabricatorApplicationTransactionView', 'DifferentialUnitFieldSpecification' => 'DifferentialFieldSpecification', diff --git a/src/applications/differential/application/PhabricatorApplicationDifferential.php b/src/applications/differential/application/PhabricatorApplicationDifferential.php index 33234225a2..e08d001987 100644 --- a/src/applications/differential/application/PhabricatorApplicationDifferential.php +++ b/src/applications/differential/application/PhabricatorApplicationDifferential.php @@ -49,6 +49,8 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication { 'changeset/' => 'DifferentialChangesetViewController', 'revision/edit/(?:(?P[1-9]\d*)/)?' => 'DifferentialRevisionEditController', + 'revision/editpro/(?:(?P[1-9]\d*)/)?' + => 'DifferentialRevisionEditControllerPro', 'revision/land/(?:(?P[1-9]\d*))/(?P[^/]+)/' => 'DifferentialRevisionLandController', 'comment/' => array( diff --git a/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php b/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php new file mode 100644 index 0000000000..d998d754c3 --- /dev/null +++ b/src/applications/differential/controller/DifferentialRevisionEditControllerPro.php @@ -0,0 +1,159 @@ +id = idx($data, 'id'); + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + if (!$this->id) { + $this->id = $request->getInt('revisionID'); + } + + if ($this->id) { + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($this->id)) + ->needRelationships(true) + ->needReviewerStatus(true) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$revision) { + return new Aphront404Response(); + } + } else { + $revision = DifferentialRevision::initializeNewRevision($viewer); + } + + $diff_id = $request->getInt('diffID'); + if ($diff_id) { + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withIDs(array($diff_id)) + ->executeOne(); + if (!$diff) { + return new Aphront404Response(); + } + if ($diff->getRevisionID()) { + // TODO: Redirect? + throw new Exception("This diff is already attached to a revision!"); + } + } else { + $diff = null; + } + + $field_list = PhabricatorCustomField::getObjectFields( + $revision, + PhabricatorCustomField::ROLE_EDIT); + $field_list + ->setViewer($viewer) + ->readFieldsFromStorage($revision); + + $validation_exception = null; + if ($request->isFormPost() && !$request->getStr('viaDiffView')) { + $xactions = $field_list->buildFieldTransactionsFromRequest( + new DifferentialTransaction(), + $request); + + $editor = id(new DifferentialTransactionEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true); + + try { + $editor->applyTransactions($revision, $xactions); + $revision_uri = '/D'.$revision->getID(); + return id(new AphrontRedirectResponse())->setURI($revision_uri); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + } + } + + + $form = new AphrontFormView(); + $form->setUser($request->getUser()); + if ($diff) { + $form->addHiddenInput('diffID', $diff->getID()); + } + + if ($revision->getID()) { + $form->setAction( + '/differential/revision/editpro/'.$revision->getID().'/'); + } else { + $form->setAction('/differential/revision/editpro/'); + } + + if ($diff && $revision->getID()) { + $form + ->appendChild( + id(new AphrontFormTextAreaControl()) + ->setLabel(pht('Comments')) + ->setName('comments') + ->setCaption(pht("Explain what's new in this diff.")) + ->setValue($request->getStr('comments'))) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue(pht('Save'))) + ->appendChild( + id(new AphrontFormDividerControl())); + } + + $field_list->appendFieldsToForm($form); + + $submit = id(new AphrontFormSubmitControl()) + ->setValue('Save'); + if ($diff) { + $submit->addCancelButton('/differential/diff/'.$diff->getID().'/'); + } else { + $submit->addCancelButton('/D'.$revision->getID()); + } + + $form->appendChild($submit); + + $crumbs = $this->buildApplicationCrumbs(); + if ($revision->getID()) { + if ($diff) { + $title = pht('Update Differential Revision'); + $crumbs->addTextCrumb( + 'D'.$revision->getID(), + '/differential/diff/'.$diff->getID().'/'); + } else { + $title = pht('Edit Differential Revision'); + $crumbs->addTextCrumb( + 'D'.$revision->getID(), + '/D'.$revision->getID()); + } + } else { + $title = pht('Create New Differential Revision'); + } + + $form_box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->setValidationException($validation_exception) + ->setForm($form); + + $crumbs->addTextCrumb($title); + + return $this->buildApplicationPage( + array( + $crumbs, + $form_box, + ), + array( + 'title' => $title, + 'device' => true, + )); + } + +} diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php new file mode 100644 index 0000000000..2066ea8683 --- /dev/null +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -0,0 +1,6 @@ +fieldError = $field_error; + return $this; + } + + public function getFieldError() { + return $this->fieldError; + } + + public function getFieldKey() { + return 'differential:title'; + } + + public function getFieldName() { + return pht('Title'); + } + + public function getFieldDescription() { + return pht('Stores the revision title.'); + } + + public function canDisableField() { + return false; + } + + public function shouldAppearInApplicationTransactions() { + return true; + } + + public function shouldAppearInEditView() { + return true; + } + + protected function didSetObject(PhabricatorCustomFieldInterface $object) { + $this->value = $object->getTitle(); + } + + public function getOldValueForApplicationTransactions() { + return $this->getObject()->getTitle(); + } + + public function getNewValueForApplicationTransactions() { + return $this->value; + } + + public function validateApplicationTransactions( + PhabricatorApplicationTransactionEditor $editor, + $type, + array $xactions) { + + $errors = parent::validateApplicationTransactions( + $editor, + $type, + $xactions); + + $transaction = null; + foreach ($xactions as $xaction) { + $value = $xaction->getNewValue(); + if (!strlen($value)) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('You must choose a title for this revision.'), + $xaction); + $error->setIsMissingFieldError(true); + $errors[] = $error; + $this->setFieldError(pht('Required')); + } + } + } + + public function applyApplicationTransactionInternalEffects( + PhabricatorApplicationTransaction $xaction) { + $this->getObject()->setTitle($xaction->getNewValue()); + } + + public function readValueFromRequest(AphrontRequest $request) { + $this->value = $request->getStr($this->getFieldKey()); + } + + public function renderEditControl() { + return id(new AphrontFormTextAreaControl()) + ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT) + ->setName($this->getFieldKey()) + ->setValue($this->value) + ->setError($this->getFieldError()) + ->setLabel($this->getFieldName()); + } + + public function getApplicationTransactionTitle( + PhabricatorApplicationTransaction $xaction) { + $author_phid = $xaction->getAuthorPHID(); + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + if (strlen($old)) { + return pht( + '%s retitled this revision from "%s" to "%s".', + $xaction->renderHandleLink($author_phid), + $old, + $new); + } else { + return pht( + '%s created this revision.', + $xaction->renderHandleLink($author_phid)); + } + } + + public function getApplicationTransactionTitleForFeed( + PhabricatorApplicationTransaction $xaction, + PhabricatorFeedStory $story) { + + $object_phid = $xaction->getObjectPHID(); + $author_phid = $xaction->getAuthorPHID(); + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + if (strlen($old)) { + return pht( + '%s retitled %s, from "%s" to "%s".', + $xaction->renderHandleLink($author_phid), + $xaction->renderHandleLink($object_phid), + $old, + $new); + } else { + return pht( + '%s created %s.', + $xaction->renderHandleLink($author_phid), + $xaction->renderHandleLink($object_phid)); + } + } + + +} diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php new file mode 100644 index 0000000000..70743468dd --- /dev/null +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -0,0 +1,100 @@ +getTransactionType()) { + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + switch ($type) { + } + + return $errors; + } + + + protected function requireCapabilities( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::requireCapabilities($object, $xaction); + } + + protected function supportsSearch() { + return true; + } + + protected function extractFilePHIDsFromCustomTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::extractFilePHIDsFromCustomTransaction($object, $xaction); + } + +} diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 664d417bc4..c52a38e94c 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -7,7 +7,8 @@ final class DifferentialRevision extends DifferentialDAO PhabricatorFlaggableInterface, PhrequentTrackableInterface, HarbormasterBuildableInterface, - PhabricatorSubscribableInterface { + PhabricatorSubscribableInterface, + PhabricatorCustomFieldInterface { protected $title = ''; protected $originalTitle; @@ -39,6 +40,7 @@ final class DifferentialRevision extends DifferentialDAO private $repository = self::ATTACHABLE; private $reviewerStatus = self::ATTACHABLE; + private $customFields = self::ATTACHABLE; const TABLE_COMMIT = 'differential_commit'; @@ -436,4 +438,29 @@ final class DifferentialRevision extends DifferentialDAO return false; } + +/* -( PhabricatorCustomFieldInterface )------------------------------------ */ + + + public function getCustomFieldSpecificationForRole($role) { + return array_fill_keys( + array( + + ), + array('disabled' => false)); + } + + public function getCustomFieldBaseClass() { + return 'DifferentialCustomField'; + } + + public function getCustomFields() { + return $this->assertAttached($this->customFields); + } + + public function attachCustomFields(PhabricatorCustomFieldAttachment $fields) { + $this->customFields = $fields; + return $this; + } + }