From 1a1da7d6c2764d12561acb23c5bcbfb4ed1debbb Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Dec 2011 19:03:28 -0800 Subject: [PATCH] Integrate auxiliary fields into Maniphest transactions Summary: - When changing auxiliary field values, use transactions. - Clean up some of the load/save logic for auxiliary fields so it's a little more performant. NOTE: The transaction display of auxiliary fields is incredibly hacky, I'll follow up with a more nuanced approach but wanted to limit scope here. Test Plan: Created and edited tasks with custom fields configured; created and edited tasks without custom fields configured. Reviewers: btrahan, jungejason, zeeg Reviewed By: jungejason CC: aran, jungejason Maniphest Tasks: T418 Differential Revision: https://secure.phabricator.com/D1283 --- .../ManiphestTransactionType.php | 1 + .../ManiphestTaskDetailController.php | 17 +-- .../taskedit/ManiphestTaskEditController.php | 52 ++++---- .../ManiphestTransactionEditor.php | 12 ++ .../maniphest/storage/task/ManiphestTask.php | 115 +++++++++++++----- .../maniphest/storage/task/__init__.php | 2 + .../transaction/ManiphestTransaction.php | 26 +++- .../storage/transaction/__init__.php | 2 + .../ManiphestTransactionDetailView.php | 22 ++++ 9 files changed, 182 insertions(+), 67 deletions(-) diff --git a/src/applications/maniphest/constants/transactiontype/ManiphestTransactionType.php b/src/applications/maniphest/constants/transactiontype/ManiphestTransactionType.php index 01706429c9..1d1444b3c6 100644 --- a/src/applications/maniphest/constants/transactiontype/ManiphestTransactionType.php +++ b/src/applications/maniphest/constants/transactiontype/ManiphestTransactionType.php @@ -32,6 +32,7 @@ final class ManiphestTransactionType extends ManiphestConstants { const TYPE_TITLE = 'title'; const TYPE_DESCRIPTION = 'description'; + const TYPE_AUXILIARY = 'aux'; public static function getTransactionTypeMap() { return array( diff --git a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php index 0bf1113ec5..34d5873278 100644 --- a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php @@ -47,9 +47,6 @@ class ManiphestTaskDetailController extends ManiphestController { $parent_task = id(new ManiphestTask())->load($workflow); } - $extensions = ManiphestTaskExtensions::newExtensions(); - $aux_fields = $extensions->getAuxiliaryFieldSpecifications(); - $transactions = id(new ManiphestTransaction())->loadAllWhere( 'taskID = %d ORDER BY id ASC', $task->getID()); @@ -137,18 +134,14 @@ class ManiphestTaskDetailController extends ManiphestController { $dict['Projects'] = 'None'; } + $extensions = ManiphestTaskExtensions::newExtensions(); + $aux_fields = $extensions->getAuxiliaryFieldSpecifications(); if ($aux_fields) { + $task->loadAndAttachAuxiliaryAttributes(); foreach ($aux_fields as $aux_field) { - $attribute = $task->loadAuxiliaryAttribute( - $aux_field->getAuxiliaryKey() - ); - - if ($attribute) { - $aux_field->setValue($attribute->getValue()); - } - + $aux_key = $aux_field->getAuxiliaryKey(); + $aux_field->setValue($task->getAuxiliaryAttribute($aux_key)); $value = $aux_field->renderForDetailView(); - if (strlen($value)) { $dict[$aux_field->getLabel()] = $value; } diff --git a/src/applications/maniphest/controller/taskedit/ManiphestTaskEditController.php b/src/applications/maniphest/controller/taskedit/ManiphestTaskEditController.php index 80680ef8f4..044cc733b9 100644 --- a/src/applications/maniphest/controller/taskedit/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/taskedit/ManiphestTaskEditController.php @@ -200,8 +200,20 @@ class ManiphestTaskEditController extends ManiphestController { $transactions[] = $transaction; } - if ($transactions) { + if ($aux_fields) { + $task->loadAndAttachAuxiliaryAttributes(); + foreach ($aux_fields as $aux_field) { + $transaction = clone $template; + $transaction->setTransactionType( + ManiphestTransactionType::TYPE_AUXILIARY); + $aux_key = $aux_field->getAuxiliaryKey(); + $transaction->setMetadataValue('aux:key', $aux_key); + $transaction->setNewValue($aux_field->getValueForStorage()); + $transactions[] = $transaction; + } + } + if ($transactions) { $event = new PhabricatorEvent( PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK, array( @@ -220,14 +232,6 @@ class ManiphestTaskEditController extends ManiphestController { $editor->applyTransactions($task, $transactions); } - // TODO: Capture auxiliary field changes in a transaction - foreach ($aux_fields as $aux_field) { - $task->setAuxiliaryAttribute( - $aux_field->getAuxiliaryKey(), - $aux_field->getValueForStorage() - ); - } - if ($parent_task) { $type_task = PhabricatorPHIDConstants::PHID_TYPE_TASK; @@ -413,27 +417,27 @@ class ManiphestTaskEditController extends ManiphestController { 'Create New Project')) ->setDatasource('/typeahead/common/projects/')); - $attributes = $task->loadAuxiliaryAttributes(); - $attributes = mpull($attributes, 'getValue', 'getName'); + if ($aux_fields) { - foreach ($aux_fields as $aux_field) { if (!$request->isFormPost()) { - $attribute = null; - - if (isset($attributes[$aux_field->getAuxiliaryKey()])) { - $attribute = $attributes[$aux_field->getAuxiliaryKey()]; - $aux_field->setValueFromStorage($attribute); + $task->loadAndAttachAuxiliaryAttributes(); + foreach ($aux_fields as $aux_field) { + $aux_key = $aux_field->getAuxiliaryKey(); + $value = $task->getAuxiliaryAttribute($aux_key); + $aux_field->setValueFromStorage($value); } } - if ($aux_field->isRequired() && !$aux_field->getError() - && !$aux_field->getValue()) { - $aux_field->setError(true); + foreach ($aux_fields as $aux_field) { + if ($aux_field->isRequired() && + !$aux_field->getError() && + !$aux_field->getValue()) { + $aux_field->setError(true); + } + + $aux_control = $aux_field->renderControl(); + $form->appendChild($aux_control); } - - $aux_control = $aux_field->renderControl(); - - $form->appendChild($aux_control); } require_celerity_resource('aphront-error-view-css'); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index f3f153bab3..6ac800617f 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -72,6 +72,14 @@ class ManiphestTransactionEditor { $old = $task->getProjectPHIDs(); $value_is_phid_set = true; break; + case ManiphestTransactionType::TYPE_AUXILIARY: + $aux_key = $transaction->getMetadataValue('aux:key'); + if (!$aux_key) { + throw new Exception( + "Expected 'aux:key' metadata on TYPE_AUXILIARY transaction."); + } + $old = $task->getAuxiliaryAttribute($aux_key); + break; default: throw new Exception('Unknown action type.'); } @@ -150,6 +158,10 @@ class ManiphestTransactionEditor { case ManiphestTransactionType::TYPE_PROJECTS: $task->setProjectPHIDs($new); break; + case ManiphestTransactionType::TYPE_AUXILIARY: + $aux_key = $transaction->getMetadataValue('aux:key'); + $task->setAuxiliaryAttribute($aux_key, $new); + break; default: throw new Exception('Unknown action type.'); } diff --git a/src/applications/maniphest/storage/task/ManiphestTask.php b/src/applications/maniphest/storage/task/ManiphestTask.php index 117acca38b..5f7a54cfc4 100644 --- a/src/applications/maniphest/storage/task/ManiphestTask.php +++ b/src/applications/maniphest/storage/task/ManiphestTask.php @@ -41,6 +41,9 @@ class ManiphestTask extends ManiphestDAO { protected $ownerOrdering; + private $auxiliaryAttributes; + private $auxiliaryDirty = array(); + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -87,44 +90,48 @@ class ManiphestTask extends ManiphestDAO { return $this; } - public function setAuxiliaryAttribute($key, $val) { - $this->removeAuxiliaryAttribute($key); - - $attribute = new ManiphestTaskAuxiliaryStorage(); - $attribute->setTaskPHID($this->phid); - $attribute->setName($key); - $attribute->setValue($val); - $attribute->save(); - } - - public function loadAuxiliaryAttribute($key) { - $attribute = id(new ManiphestTaskAuxiliaryStorage())->loadOneWhere( - 'taskPHID = %s AND name = %s', - $this->getPHID(), - $key); - - return $attribute; - } - - public function removeAuxiliaryAttribute($key) { - $attribute = id(new ManiphestTaskAuxiliaryStorage())->loadOneWhere( - 'taskPHID = %s AND name = %s', - $this->getPHID(), - $key); - - if ($attribute) { - $attribute->delete(); + public function getAuxiliaryAttribute($key, $default = null) { + if ($this->auxiliaryAttributes === null) { + throw new Exception("Attach auxiliary attributes before getting them!"); } + return idx($this->auxiliaryAttributes, $key, $default); } - public function loadAuxiliaryAttributes() { - $attributes = id(new ManiphestTaskAuxiliaryStorage())->loadAllWhere( + public function setAuxiliaryAttribute($key, $val) { + if ($this->auxiliaryAttributes === null) { + throw new Exception("Attach auxiliary attributes before setting them!"); + } + $this->auxiliaryAttributes[$key] = $val; + $this->auxiliaryDirty[$key] = true; + return $this; + } + + public function attachAuxiliaryAttributes(array $attrs) { + if ($this->auxiliaryDirty) { + throw new Exception( + "This object has dirty attributes, you can not attach new attributes ". + "without writing or discarding the dirty attributes."); + } + $this->auxiliaryAttributes = $attrs; + return $this; + } + + public function loadAndAttachAuxiliaryAttributes() { + if (!$this->getPHID()) { + $this->auxiliaryAttributes = array(); + return; + } + + $storage = id(new ManiphestTaskAuxiliaryStorage())->loadAllWhere( 'taskPHID = %s', $this->getPHID()); - return $attributes; + $this->auxiliaryAttributes = mpull($storage, 'getValue', 'getName'); + + return $this; } + public function save() { if (!$this->mailKey) { $this->mailKey = Filesystem::readRandomCharacters(20); @@ -146,7 +153,55 @@ class ManiphestTask extends ManiphestDAO { $this->subscribersNeedUpdate = false; } + if ($this->auxiliaryDirty) { + $this->writeAuxiliaryUpdates(); + $this->auxiliaryDirty = array(); + } + return $result; } + private function writeAuxiliaryUpdates() { + $table = new ManiphestTaskAuxiliaryStorage(); + $conn_w = $table->establishConnection('w'); + $update = array(); + $remove = array(); + + foreach ($this->auxiliaryDirty as $key => $dirty) { + $value = $this->getAuxiliaryAttribute($key); + if ($value === null) { + $remove[$key] = true; + } else { + $update[$key] = $value; + } + } + + if ($remove) { + queryfx( + $conn_w, + 'DELETE FROM %T WHERE taskPHID = %s AND name IN (%Ls)', + $table->getTableName(), + $this->getPHID(), + array_keys($remove)); + } + + if ($update) { + $sql = array(); + foreach ($update as $key => $val) { + $sql[] = qsprintf( + $conn_w, + '(%s, %s, %s)', + $this->getPHID(), + $key, + $val); + } + queryfx( + $conn_w, + 'INSERT INTO %T (taskPHID, name, value) VALUES %Q + ON DUPLICATE KEY UPDATE value = VALUES(value)', + $table->getTableName(), + implode(', ', $sql)); + } + } + } diff --git a/src/applications/maniphest/storage/task/__init__.php b/src/applications/maniphest/storage/task/__init__.php index ec466125bf..2c95ef8909 100644 --- a/src/applications/maniphest/storage/task/__init__.php +++ b/src/applications/maniphest/storage/task/__init__.php @@ -12,6 +12,8 @@ phutil_require_module('phabricator', 'applications/maniphest/storage/subscriber' phutil_require_module('phabricator', 'applications/maniphest/storage/taskproject'); phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); +phutil_require_module('phabricator', 'storage/qsprintf'); +phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phutil', 'filesystem'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/maniphest/storage/transaction/ManiphestTransaction.php b/src/applications/maniphest/storage/transaction/ManiphestTransaction.php index fbe5ded867..b641a9833b 100644 --- a/src/applications/maniphest/storage/transaction/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/transaction/ManiphestTransaction.php @@ -81,6 +81,21 @@ class ManiphestTransaction extends ManiphestDAO { return $phids; } + public function getMetadataValue($key, $default = null) { + if (!is_array($this->metadata)) { + return $default; + } + return idx($this->metadata, $key, $default); + } + + public function setMetadataValue($key, $value) { + if (!is_array($this->metadata)) { + $this->metadata = array(); + } + $this->metadata[$key] = $value; + return $this; + } + public function canGroupWith($target) { if ($target->getAuthorPHID() != $this->getAuthorPHID()) { return false; @@ -95,7 +110,16 @@ class ManiphestTransaction extends ManiphestDAO { } if ($target->getTransactionType() == $this->getTransactionType()) { - return false; + $aux_type = ManiphestTransactionType::TYPE_AUXILIARY; + if ($this->getTransactionType() == $aux_type) { + $that_key = $target->getMetadataValue('aux:key'); + $this_key = $this->getMetadataValue('aux:key'); + if ($that_key == $this_key) { + return false; + } + } else { + return false; + } } return true; diff --git a/src/applications/maniphest/storage/transaction/__init__.php b/src/applications/maniphest/storage/transaction/__init__.php index f095c1223e..54cfd92c58 100644 --- a/src/applications/maniphest/storage/transaction/__init__.php +++ b/src/applications/maniphest/storage/transaction/__init__.php @@ -10,5 +10,7 @@ phutil_require_module('phabricator', 'applications/maniphest/constants/transacti phutil_require_module('phabricator', 'applications/maniphest/storage/base'); phutil_require_module('phabricator', 'applications/metamta/contentsource/source'); +phutil_require_module('phutil', 'utils'); + phutil_require_source('ManiphestTransaction.php'); diff --git a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php index 09c30cfeb5..0c345921f2 100644 --- a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php @@ -528,6 +528,28 @@ class ManiphestTransactionDetailView extends ManiphestView { $desc = 'changed attached '.$plural.', added: '.$add_desc.'; '. 'removed: '.$rem_desc; } + break; + case ManiphestTransactionType::TYPE_AUXILIARY: + + // TODO: This is temporary and hacky! Allow auxiliary fields to + // customize this. + + $old_esc = phutil_escape_html($old); + $new_esc = phutil_escape_html($new); + + $aux_key = $transaction->getMetadataValue('aux:key'); + if ($old === null) { + $verb = "Set Field"; + $desc = "set field '{$aux_key}' to '{$new_esc}'"; + } else if ($new === null) { + $verb = "Removed Field"; + $desc = "removed field '{$aux_key}'"; + } else { + $verb = "Updated Field"; + $desc = "updated field '{$aux_key}' ". + "from '{$old_esc}' to '{$new_esc}'"; + } + break; default: return array($type, ' brazenly '.$type."'d", $classes);