From 938b63aaa989fe89cad6d34e9bf1db49691bd931 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Aug 2013 09:40:42 -0700 Subject: [PATCH] Simplify and improve PhabricatorCustomField APIs Summary: Ref T1703. Ref T3718. The `PhabricatorCustomFieldList` seems like a pretty good idea. Move more code into it to make it harder to get wrong. Also the sequencing on old/new values for these transactions was a bit off; fix that up. Test Plan: Edited standard and custom profile fields. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1703, T3718 Differential Revision: https://secure.phabricator.com/D6751 --- .../PhabricatorPeopleProfileController.php | 3 +- ...PhabricatorPeopleProfileEditController.php | 17 +++------- .../people/storage/PhabricatorUser.php | 4 +-- ...habricatorApplicationTransactionEditor.php | 15 ++++++--- .../field/PhabricatorCustomField.php | 9 +++--- .../field/PhabricatorCustomFieldList.php | 31 +++++++++++++++++++ .../PhabricatorCustomFieldInterface.php | 6 ++-- 7 files changed, 57 insertions(+), 28 deletions(-) diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php index a0b84aa705..a15797808d 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -97,10 +97,9 @@ final class PhabricatorPeopleProfileController ->setUser($viewer) ->setObject($user); - $fields = PhabricatorCustomField::getObjectFields( + $field_list = PhabricatorCustomField::getObjectFields( $user, PhabricatorCustomField::ROLE_VIEW); - $field_list = new PhabricatorCustomFieldList($fields); $field_list->appendFieldsToPropertyList($user, $viewer, $view); return $view; diff --git a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php index 9f752257e0..cf75625047 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php @@ -32,20 +32,15 @@ final class PhabricatorPeopleProfileEditController $profile_uri = '/p/'.$user->getUsername().'/'; - $fields = PhabricatorCustomField::getObjectFields( + $field_list = PhabricatorCustomField::getObjectFields( $user, PhabricatorCustomField::ROLE_EDIT); - $field_list = new PhabricatorCustomFieldList($fields); + $field_list->readFieldsFromStorage($user); if ($request->isFormPost()) { - $xactions = array(); - foreach ($fields as $field) { - $field->readValueFromRequest($request); - $xactions[] = id(new PhabricatorUserTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD) - ->setMetadataValue('customfield:key', $field->getFieldKey()) - ->setNewValue($field->getNewValueForApplicationTransactions()); - } + $xactions = $field_list->buildFieldTransactionsFromRequest( + new PhabricatorUserTransaction(), + $request); $editor = id(new PhabricatorUserProfileEditor()) ->setActor($viewer) @@ -56,8 +51,6 @@ final class PhabricatorPeopleProfileEditController $editor->applyTransactions($user, $xactions); return id(new AphrontRedirectResponse())->setURI($profile_uri); - } else { - $field_list->readFieldsFromStorage($user); } $title = pht('Edit Profile'); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 7e3b1b8417..7bea2f0441 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -850,8 +850,8 @@ EOBODY; return $this->customFields[$role]; } - public function attachCustomFields($role, array $fields) { - $this->customFields[$role] = $fields; + public function attachCustomFields($role, PhabricatorCustomFieldList $list) { + $this->customFields[$role] = $list; return $this; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 8369e0712b..50490f6a82 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -125,8 +125,9 @@ abstract class PhabricatorApplicationTransactionEditor } return $old_edges; case PhabricatorTransactions::TYPE_CUSTOMFIELD: - $field = $this->getCustomFieldForTransaction($object, $xaction); - return $field->getOldValueForApplicationTransactions(); + // NOTE: Custom fields have their old value pre-populated when they are + // built by PhabricatorCustomFieldList. + return $xaction->getOldValue(); default: return $this->getCustomTransactionOldValue($object, $xaction); } @@ -585,9 +586,13 @@ abstract class PhabricatorApplicationTransactionEditor throw new Exception( "You can not apply transactions which already have commentVersions!"); } - if ($xaction->getOldValue() !== null) { - throw new Exception( - "You can not apply transactions which already have oldValue!"); + + $custom_field_type = PhabricatorTransactions::TYPE_CUSTOMFIELD; + if ($xaction->getTransactionType() != $custom_field_type) { + if ($xaction->getOldValue() !== null) { + throw new Exception( + "You can not apply transactions which already have oldValue!"); + } } $type = $xaction->getTransactionType(); diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 71f2a06a13..6902d70ed4 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -49,7 +49,7 @@ abstract class PhabricatorCustomField { $role) { try { - $fields = $object->getCustomFields($role); + $field_list = $object->getCustomFields($role); } catch (PhabricatorCustomFieldNotAttachedException $ex) { $base_class = $object->getCustomFieldBaseClass(); @@ -73,10 +73,11 @@ abstract class PhabricatorCustomField { $field->setObject($object); } - $object->attachCustomFields($role, $fields); + $field_list = new PhabricatorCustomFieldList($fields); + $object->attachCustomFields($role, $field_list); } - return $fields; + return $field_list; } @@ -87,7 +88,7 @@ abstract class PhabricatorCustomField { PhabricatorCustomFieldInterface $object, $role, $field_key) { - return idx(self::getObjectFields($object, $role), $field_key); + return idx(self::getObjectFields($object, $role)->getFields(), $field_key); } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php index 75b3ae175c..e5d17822f8 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php @@ -16,6 +16,9 @@ final class PhabricatorCustomFieldList extends Phobject { $this->fields = $fields; } + public function getFields() { + return $this->fields; + } /** * Read stored values for all fields which support storage. @@ -120,4 +123,32 @@ final class PhabricatorCustomFieldList extends Phobject { } } + public function buildFieldTransactionsFromRequest( + PhabricatorApplicationTransaction $template, + AphrontRequest $request) { + + $xactions = array(); + + $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS; + foreach ($this->fields as $field) { + if (!$field->shouldEnableForRole($role)) { + continue; + } + + $old_value = $field->getOldValueForApplicationTransactions(); + + $field->readValueFromRequest($request); + + $xaction = id(clone $template) + ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD) + ->setMetadataValue('customfield:key', $field->getFieldKey()) + ->setOldValue($old_value) + ->setNewValue($field->getNewValueForApplicationTransactions()); + + $xactions[] = $xaction; + } + + return $xactions; + } + } diff --git a/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php b/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php index c31f767629..6bec3b345a 100644 --- a/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php +++ b/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php @@ -5,7 +5,7 @@ interface PhabricatorCustomFieldInterface { public function getCustomFieldBaseClass(); public function getCustomFieldSpecificationForRole($role); public function getCustomFields($role); - public function attachCustomFields($role, array $fields); + public function attachCustomFields($role, PhabricatorCustomFieldList $list); } @@ -33,8 +33,8 @@ interface PhabricatorCustomFieldInterface { return $this->customFields[$role]; } - public function attachCustomFields($role, array $fields) { - $this->customFields[$role] = $fields; + public function attachCustomFields($role, PhabricatorCustomFieldList $list) { + $this->customFields[$role] = $list; return $this; }