From 6ffbee115ba38fff5a329ccab44793fc81e8cfda Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Jun 2013 09:55:55 -0700 Subject: [PATCH] Add ApplicationTransactions/CustomField based user profile editor Summary: Adds a profile edit controller (with just one field and on links to it) that uses ApplicationTransactions and CustomField. {F45617} My plan is to move the other profile fields to this interface and get rid of Settings -> Profile. Basically, these will be "settings": - Sex - Language - Timezone These will be "profile": - Real Name - Title - Blurb - Profile Image (but I'm going to put this on a separate UI) - Other custom fields Test Plan: Edited my realname using the new interface. Reviewers: chad, seporaitis Reviewed By: chad CC: aran Differential Revision: https://secure.phabricator.com/D6152 --- .../sql/patches/20130606.userxactions.sql | 23 +++++ src/__phutil_library_map__.php | 17 ++++ .../PhabricatorApplicationPeople.php | 2 + ...PhabricatorPeopleProfileEditController.php | 94 +++++++++++++++++++ .../PhabricatorUserCustomField.php | 32 +++++++ .../PhabricatorUserCustomFieldInterface.php | 9 ++ .../PhabricatorUserRealNameField.php | 48 ++++++++++ .../editor/PhabricatorUserProfileEditor.php | 7 ++ .../people/storage/PhabricatorUser.php | 30 +++++- ...habricatorApplicationTransactionEditor.php | 2 +- .../field/PhabricatorCustomField.php | 73 ++++++++++---- .../field/PhabricatorStandardCustomField.php | 72 ++++++++++++++ .../PhabricatorCustomFieldInterface.php | 2 +- .../patch/PhabricatorBuiltinPatchList.php | 4 + 14 files changed, 392 insertions(+), 23 deletions(-) create mode 100644 resources/sql/patches/20130606.userxactions.sql create mode 100644 src/applications/people/controller/PhabricatorPeopleProfileEditController.php create mode 100644 src/applications/people/customfield/PhabricatorUserCustomField.php create mode 100644 src/applications/people/customfield/PhabricatorUserCustomFieldInterface.php create mode 100644 src/applications/people/customfield/PhabricatorUserRealNameField.php create mode 100644 src/applications/people/editor/PhabricatorUserProfileEditor.php create mode 100644 src/infrastructure/customfield/field/PhabricatorStandardCustomField.php diff --git a/resources/sql/patches/20130606.userxactions.sql b/resources/sql/patches/20130606.userxactions.sql new file mode 100644 index 0000000000..3d266e826c --- /dev/null +++ b/resources/sql/patches/20130606.userxactions.sql @@ -0,0 +1,23 @@ +CREATE TABLE {$NAMESPACE}_user.user_transaction ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + phid VARCHAR(64) NOT NULL COLLATE utf8_bin, + authorPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + viewPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + editPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + commentPHID VARCHAR(64) COLLATE utf8_bin, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) NOT NULL COLLATE utf8_bin, + oldValue LONGTEXT NOT NULL COLLATE utf8_bin, + newValue LONGTEXT NOT NULL COLLATE utf8_bin, + metadata LONGTEXT NOT NULL COLLATE utf8_bin, + contentSource LONGTEXT NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + + UNIQUE KEY `key_phid` (phid), + KEY `key_object` (objectPHID) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; + + diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 07436b6d72..6a63e4d36f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1279,6 +1279,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleListController' => 'applications/people/controller/PhabricatorPeopleListController.php', 'PhabricatorPeopleLogsController' => 'applications/people/controller/PhabricatorPeopleLogsController.php', 'PhabricatorPeopleProfileController' => 'applications/people/controller/PhabricatorPeopleProfileController.php', + 'PhabricatorPeopleProfileEditController' => 'applications/people/controller/PhabricatorPeopleProfileEditController.php', 'PhabricatorPeopleQuery' => 'applications/people/query/PhabricatorPeopleQuery.php', 'PhabricatorPeopleSearchEngine' => 'applications/people/query/PhabricatorPeopleSearchEngine.php', 'PhabricatorPeopleTestDataGenerator' => 'applications/people/lipsum/PhabricatorPeopleTestDataGenerator.php', @@ -1471,6 +1472,7 @@ phutil_register_library_map(array( 'PhabricatorSlugTestCase' => 'infrastructure/util/__tests__/PhabricatorSlugTestCase.php', 'PhabricatorSortTableExample' => 'applications/uiexample/examples/PhabricatorSortTableExample.php', 'PhabricatorSourceCodeView' => 'view/layout/PhabricatorSourceCodeView.php', + 'PhabricatorStandardCustomField' => 'infrastructure/customfield/field/PhabricatorStandardCustomField.php', 'PhabricatorStandardPageView' => 'view/page/PhabricatorStandardPageView.php', 'PhabricatorStatusController' => 'applications/system/PhabricatorStatusController.php', 'PhabricatorStorageFixtureScopeGuard' => 'infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php', @@ -1542,6 +1544,8 @@ phutil_register_library_map(array( 'PhabricatorUITooltipExample' => 'applications/uiexample/examples/PhabricatorUITooltipExample.php', 'PhabricatorUnitsTestCase' => 'view/__tests__/PhabricatorUnitsTestCase.php', 'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php', + 'PhabricatorUserCustomField' => 'applications/people/customfield/PhabricatorUserCustomField.php', + 'PhabricatorUserCustomFieldInterface' => 'applications/people/customfield/PhabricatorUserCustomFieldInterface.php', 'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', 'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', 'PhabricatorUserEmail' => 'applications/people/storage/PhabricatorUserEmail.php', @@ -1550,6 +1554,8 @@ phutil_register_library_map(array( 'PhabricatorUserOAuthInfo' => 'applications/people/storage/PhabricatorUserOAuthInfo.php', 'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', 'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php', + 'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php', + 'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php', 'PhabricatorUserSSHKey' => 'applications/settings/storage/PhabricatorUserSSHKey.php', 'PhabricatorUserSearchIndexer' => 'applications/people/search/PhabricatorUserSearchIndexer.php', 'PhabricatorUserStatus' => 'applications/people/storage/PhabricatorUserStatus.php', @@ -2376,6 +2382,7 @@ phutil_register_library_map(array( array( 0 => 'DivinerDAO', 1 => 'PhabricatorPolicyInterface', + 2 => 'PhabricatorMarkupInterface', ), 'DivinerPublishCache' => 'DivinerDiskCache', 'DivinerRemarkupRuleSymbol' => 'PhutilRemarkupRule', @@ -3118,6 +3125,7 @@ phutil_register_library_map(array( ), 'PhabricatorPeopleLogsController' => 'PhabricatorPeopleController', 'PhabricatorPeopleProfileController' => 'PhabricatorPeopleController', + 'PhabricatorPeopleProfileEditController' => 'PhabricatorPeopleController', 'PhabricatorPeopleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorPeopleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorPeopleTestDataGenerator' => 'PhabricatorTestDataGenerator', @@ -3304,6 +3312,7 @@ phutil_register_library_map(array( 'PhabricatorSlugTestCase' => 'PhabricatorTestCase', 'PhabricatorSortTableExample' => 'PhabricatorUIExample', 'PhabricatorSourceCodeView' => 'AphrontView', + 'PhabricatorStandardCustomField' => 'PhabricatorCustomField', 'PhabricatorStandardPageView' => 'PhabricatorBarePageView', 'PhabricatorStatusController' => 'PhabricatorController', 'PhabricatorStorageManagementDatabasesWorkflow' => 'PhabricatorStorageManagementWorkflow', @@ -3374,6 +3383,12 @@ phutil_register_library_map(array( 0 => 'PhabricatorUserDAO', 1 => 'PhutilPerson', 2 => 'PhabricatorPolicyInterface', + 3 => 'PhabricatorCustomFieldInterface', + ), + 'PhabricatorUserCustomField' => + array( + 0 => 'PhabricatorCustomField', + 1 => 'PhabricatorUserCustomFieldInterface', ), 'PhabricatorUserDAO' => 'PhabricatorLiskDAO', 'PhabricatorUserEditor' => 'PhabricatorEditor', @@ -3383,6 +3398,8 @@ phutil_register_library_map(array( 'PhabricatorUserOAuthInfo' => 'PhabricatorUserDAO', 'PhabricatorUserPreferences' => 'PhabricatorUserDAO', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', + 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField', 'PhabricatorUserSSHKey' => 'PhabricatorUserDAO', 'PhabricatorUserSearchIndexer' => 'PhabricatorSearchDocumentIndexer', 'PhabricatorUserStatus' => 'PhabricatorUserDAO', diff --git a/src/applications/people/application/PhabricatorApplicationPeople.php b/src/applications/people/application/PhabricatorApplicationPeople.php index b6a12fc6b9..bbff5d12e4 100644 --- a/src/applications/people/application/PhabricatorApplicationPeople.php +++ b/src/applications/people/application/PhabricatorApplicationPeople.php @@ -44,6 +44,8 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication { 'edit/(?:(?P[1-9]\d*)/(?:(?P\w+)/)?)?' => 'PhabricatorPeopleEditController', 'ldap/' => 'PhabricatorPeopleLdapController', + 'editprofile/(?P[1-9]\d*)/' => + 'PhabricatorPeopleProfileEditController', ), '/p/(?P[\w._-]+)/(?:(?P\w+)/)?' => 'PhabricatorPeopleProfileController', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php new file mode 100644 index 0000000000..c1d041a76d --- /dev/null +++ b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php @@ -0,0 +1,94 @@ +id = $data['id']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withIDs(array($this->id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$user) { + return new Aphront404Response(); + } + + $profile_uri = '/p/'.$user->getUsername().'/'; + + $fields = PhabricatorCustomField::getObjectFields( + $user, + PhabricatorUserCustomFieldInterface::ROLE_EDIT); + + if ($request->isFormPost()) { + $xactions = array(); + foreach ($fields as $field) { + $field->setValueFromRequest($request); + $xactions[] = id(new PhabricatorUserTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_CUSTOMFIELD) + ->setMetadataValue('customfield:key', $field->getFieldKey()) + ->setNewValue($field->getNewValueForApplicationTransactions()); + } + + $editor = id(new PhabricatorUserProfileEditor()) + ->setActor($viewer) + ->setContentSource( + PhabricatorContentSource::newFromRequest($request)) + ->setContinueOnNoEffect(true); + + $editor->applyTransactions($user, $xactions); + + return id(new AphrontRedirectResponse())->setURI($profile_uri); + } + + $title = pht('Edit Profile'); + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($user->getUsername()) + ->setHref($profile_uri)); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName($title)); + + $form = id(new AphrontFormView()) + ->setUser($viewer); + + foreach ($fields as $field) { + $form->appendChild($field->renderEditControl()); + } + + $form + ->appendChild( + id(new AphrontFormSubmitControl()) + ->addCancelButton($profile_uri) + ->setValue(pht('Save Profile'))); + + return $this->buildApplicationPage( + array( + $crumbs, + $form, + ), + array( + 'title' => $title, + 'device' => true, + 'dust' => true, + )); + } +} diff --git a/src/applications/people/customfield/PhabricatorUserCustomField.php b/src/applications/people/customfield/PhabricatorUserCustomField.php new file mode 100644 index 0000000000..d8ebfdccd8 --- /dev/null +++ b/src/applications/people/customfield/PhabricatorUserCustomField.php @@ -0,0 +1,32 @@ +shouldAppearOnProfileEdit(); + } + return parent::shouldEnableForRole($role); + } + + public function shouldAppearOnProfileEdit() { + return true; + } + + +/* -( PhabricatorCustomField )--------------------------------------------- */ + + + public function canDisableField() { + return false; + } + + public function shouldAppearInApplicationTransactions() { + return true; + } + +} diff --git a/src/applications/people/customfield/PhabricatorUserCustomFieldInterface.php b/src/applications/people/customfield/PhabricatorUserCustomFieldInterface.php new file mode 100644 index 0000000000..ba065cac00 --- /dev/null +++ b/src/applications/people/customfield/PhabricatorUserCustomFieldInterface.php @@ -0,0 +1,9 @@ +value = $object->getRealName(); + } + + public function getOldValueForApplicationTransactions() { + return $this->getObject()->getRealName(); + } + + public function getNewValueForApplicationTransactions() { + return $this->value; + } + + public function applyApplicationTransactionInternalEffects( + PhabricatorApplicationTransaction $xaction) { + $this->getObject()->setRealName($xaction->getNewValue()); + } + + public function setValueFromRequest(AphrontRequest $request) { + $this->value = $request->getStr($this->getFieldKey()); + } + + public function renderEditControl() { + return id(new AphrontFormTextControl()) + ->setName($this->getFieldKey()) + ->setValue($this->value) + ->setLabel($this->getFieldName()); + } + +} diff --git a/src/applications/people/editor/PhabricatorUserProfileEditor.php b/src/applications/people/editor/PhabricatorUserProfileEditor.php new file mode 100644 index 0000000000..07a17b6c53 --- /dev/null +++ b/src/applications/people/editor/PhabricatorUserProfileEditor.php @@ -0,0 +1,7 @@ +customFields, $role) === null) { + PhabricatorCustomField::raiseUnattachedException($this, $role); + } + return $this->customFields[$role]; + } + + public function attachCustomFields($role, array $fields) { + $this->customFields[$role] = $fields; + return $this; + } + } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index e4308c833c..e55032e50a 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -146,7 +146,7 @@ abstract class PhabricatorApplicationTransactionEditor return $this->getEdgeTransactionNewValue($xaction); case PhabricatorTransactions::TYPE_CUSTOMFIELD: $field = $this->getCustomFieldForTransaction($object, $xaction); - return $field->getNewValueForApplicationTransactions(); + return $field->getNewValueFromApplicationTransactions($xaction); default: return $this->getCustomTransactionNewValue($object, $xaction); } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 8de6c90e54..f904bc5209 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -14,7 +14,7 @@ abstract class PhabricatorCustomField { private $object; const ROLE_APPLICATIONTRANSACTIONS = 'ApplicationTransactions'; - const ROLE_APPLICAITONSEARCH = 'ApplicationSearch'; + const ROLE_APPLICATIONSEARCH = 'ApplicationSearch'; const ROLE_STORAGE = 'storage'; const ROLE_DEFAULT = 'default'; @@ -45,14 +45,6 @@ abstract class PhabricatorCustomField { } catch (PhabricatorCustomFieldNotAttachedException $ex) { $base_class = $object->getCustomFieldBaseClass(); - if (!($base_class instanceof PhabricatorCustomField)) { - $obj_class = get_class($object); - throw new Exception( - "Object (of class '{$obj_class}') returned '{$base_class}' as its ". - "getCustomFieldBaseClass(), but this is not a recognized subclass ". - "of PhabricatorCustomField."); - } - $spec = $object->getCustomFieldSpecificationForRole($role); if (!is_array($spec)) { $obj_class = get_class($object); @@ -87,7 +79,6 @@ abstract class PhabricatorCustomField { PhabricatorCustomFieldInterface $object, $role, $field_key) { - return idx(self::getObjectFields($object, $role), $field_key); } @@ -96,12 +87,6 @@ abstract class PhabricatorCustomField { * @task apps */ public static function buildFieldList($base_class, array $spec) { - $this_class = __CLASS__; - if (!($base_class instanceof $this_class)) { - throw new Exception( - "Base class ('{$base_class}') must extend '{$this_class}'."); - } - $field_objects = id(new PhutilSymbolLoader()) ->setAncestorClass($base_class) ->loadObjects(); @@ -124,12 +109,12 @@ abstract class PhabricatorCustomField { } foreach ($fields as $key => $field) { - if (!$field->isEnabled()) { + if (!$field->isFieldEnabled()) { unset($fields[$key]); } } - $fields = array_select_keys($fields, array_keys($spec)); + $fields = array_select_keys($fields, array_keys($spec)) + $fields; foreach ($spec as $key => $config) { if (empty($fields[$key])) { @@ -151,7 +136,7 @@ abstract class PhabricatorCustomField { /** * Return a key which uniquely identifies this field, like - * "mycompany.dinosaur.count". Normally you should provide some level of + * "mycompany:dinosaur:count". Normally you should provide some level of * namespacing to prevent collisions. * * @return string String which uniquely identifies this field. @@ -278,6 +263,7 @@ abstract class PhabricatorCustomField { */ final public function setObject(PhabricatorCustomFieldInterface $object) { $this->object = $object; + $this->didSetObject($object); return $this; } @@ -293,6 +279,17 @@ abstract class PhabricatorCustomField { } + /** + * This is a hook, primarily for subclasses to load object data. + * + * @return PhabricatorCustomFieldInterface The object this field belongs to. + * @return void + */ + protected function didSetObject(PhabricatorCustomFieldInterface $object) { + return; + } + + /** * @task context */ @@ -523,6 +520,14 @@ abstract class PhabricatorCustomField { } + /** + * @task appxaction + */ + public function getNewValueForApplicationTransactions() { + return $this->getValueForStorage(); + } + + /** * @task appxaction */ @@ -534,7 +539,7 @@ abstract class PhabricatorCustomField { /** * @task appxaction */ - public function getNewValueForApplicationTransactions( + public function getNewValueFromApplicationTransactions( PhabricatorApplicationTransaction $xaction) { return $xaction->getNewValue(); } @@ -593,4 +598,32 @@ abstract class PhabricatorCustomField { return; } + +/* -( Edit View )---------------------------------------------------------- */ + + + /** + * @task edit + */ + public function shouldAppearOnEditView() { + return false; + } + + + /** + * @task edit + */ + public function readValueFromRequest(AphrontRequest $request) { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + + /** + * @task edit + */ + public function renderEditControl() { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + } diff --git a/src/infrastructure/customfield/field/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/field/PhabricatorStandardCustomField.php new file mode 100644 index 0000000000..46ab6af761 --- /dev/null +++ b/src/infrastructure/customfield/field/PhabricatorStandardCustomField.php @@ -0,0 +1,72 @@ +fieldKey = $key; + } + + public function setFieldName($name) { + $this->fieldName = $name; + return $this; + } + + public function setFieldType($type) { + $this->fieldType = $type; + return $this; + } + + public function getFieldValue() { + return $this->fieldValue; + } + + public function setFieldValue($value) { + $this->fieldValue = $value; + return $this; + } + + public function setFieldDescription($description) { + $this->fieldDescription = $description; + return $this; + } + + +/* -( PhabricatorCustomField )--------------------------------------------- */ + + + public function getFieldKey() { + return $this->fieldKey; + } + + public function getFieldName() { + return coalesce($this->fieldName, parent::getFieldName()); + } + + public function getFieldDescription() { + return coalesce($this->fieldDescription, parent::getFieldDescription()); + } + + public function getStorageKey() { + return $this->getFieldKey(); + } + + public function getValueForStorage() { + return $this->getFieldValue(); + } + + public function setValueFromStorage($value) { + return $this->setFieldValue($value); + } + + public function shouldAppearInApplicationTransactions() { + return true; + } + +} diff --git a/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php b/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php index 1695f6468b..c31f767629 100644 --- a/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php +++ b/src/infrastructure/customfield/interface/PhabricatorCustomFieldInterface.php @@ -30,7 +30,7 @@ interface PhabricatorCustomFieldInterface { if (idx($this->customFields, $role) === null) { PhabricatorCustomField::raiseUnattachedException($this, $role); } - return $this->customFields; + return $this->customFields[$role]; } public function attachCustomFields($role, array $fields) { diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index f6e12ac0ca..6fc7d0d8de 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1350,6 +1350,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130602.namedqueries.sql'), ), + '20130606.userxactions.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130606.userxactions.sql'), + ), ); } }