From 3ef270b2926eabe16f9efb9bed6912c0dba206e7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 22 Aug 2015 15:14:05 -0700 Subject: [PATCH 01/18] Allow transaction publishers to pass binary data to workers Summary: Ref T8672. Ref T9187. Root issue in at least one case is: - User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS). - We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things. - When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8. - When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases. - TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did. - The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data). - When we queue workers, we serialize the state data with JSON. So far, so good. But this is where things go wrong: - JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it. Then we get to the worker, and things go wrong-er: - Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely. This applies several fixes: # When queueing tasks, fail loudly when JSON encoding fails. # In the worker, fail permanently when data can't be decoded. # Allow Editors to specify that some of their data is binary and needs special handling. This is fairly messy, but some simpler alternatives don't seem like good ways forward: - We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment. - We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases. - We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around. The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were. Test Plan: - Created a commit with a bunch of Shift-JIS stuff in a file. - Tried to import it. Prior to patch: - Broken PublishWorker with distant, irrelevant error message. With patch partially applied (only new error checking): - Explicit, local error message about bad key in serialized data. With patch fully applied: - Import went fine and mail generated. Reviewers: chad Reviewed By: chad Subscribers: devurandom, nevogd Maniphest Tasks: T8672, T9187 Differential Revision: https://secure.phabricator.com/D13939 --- .../audit/editor/PhabricatorAuditEditor.php | 6 + ...habricatorApplicationTransactionEditor.php | 128 ++++++++++++++++-- ...torApplicationTransactionPublishWorker.php | 7 +- src/infrastructure/storage/lisk/LiskDAO.php | 2 +- 4 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 7c0f3d2e34..51efdf175b 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -949,6 +949,12 @@ final class PhabricatorAuditEditor ); } + protected function getCustomWorkerStateEncoding() { + return array( + 'rawPatch' => self::STORAGE_ENCODING_BINARY, + ); + } + protected function loadCustomWorkerState(array $state) { $this->rawPatch = idx($state, 'rawPatch'); $this->affectedFiles = idx($state, 'affectedFiles'); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index a5044360de..851cc24cc0 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -69,6 +69,8 @@ abstract class PhabricatorApplicationTransactionEditor private $feedNotifyPHIDs = array(); private $feedRelatedPHIDs = array(); + const STORAGE_ENCODING_BINARY = 'binary'; + /** * Get the class name for the application this editor is a part of. * @@ -2637,6 +2639,21 @@ abstract class PhabricatorApplicationTransactionEditor } + /** + * @task mail + */ + private function runHeraldMailRules(array $messages) { + foreach ($messages as $message) { + $engine = new HeraldEngine(); + $adapter = id(new PhabricatorMailOutboundMailHeraldAdapter()) + ->setObject($message); + + $rules = $engine->loadRulesForAdapter($adapter); + $effects = $engine->applyRules($rules, $adapter); + $engine->applyEffects($effects, $adapter, $rules); + } + } + /* -( Publishing Feed Stories )-------------------------------------------- */ @@ -3060,9 +3077,13 @@ abstract class PhabricatorApplicationTransactionEditor $state[$property] = $this->$property; } + $custom_state = $this->getCustomWorkerState(); + $custom_encoding = $this->getCustomWorkerStateEncoding(); + $state += array( 'excludeMailRecipientPHIDs' => $this->getExcludeMailRecipientPHIDs(), - 'custom' => $this->getCustomWorkerState(), + 'custom' => $this->encodeStateForStorage($custom_state, $custom_encoding), + 'custom.encoding' => $custom_encoding, ); return $state; @@ -3080,6 +3101,21 @@ abstract class PhabricatorApplicationTransactionEditor } + /** + * Hook; return storage encoding for custom properties which need to be + * passed to workers. + * + * This primarily allows binary data to be passed to workers and survive + * JSON encoding. + * + * @return dict Property encodings. + * @task workers + */ + protected function getCustomWorkerStateEncoding() { + return array(); + } + + /** * Load editor state using a dictionary emitted by @{method:getWorkerState}. * @@ -3097,7 +3133,10 @@ abstract class PhabricatorApplicationTransactionEditor $exclude = idx($state, 'excludeMailRecipientPHIDs', array()); $this->setExcludeMailRecipientPHIDs($exclude); - $custom = idx($state, 'custom', array()); + $custom_state = idx($state, 'custom', array()); + $custom_encodings = idx($state, 'custom.encoding', array()); + $custom = $this->decodeStateFromStorage($custom_state, $custom_encodings); + $this->loadCustomWorkerState($custom); return $this; @@ -3143,16 +3182,85 @@ abstract class PhabricatorApplicationTransactionEditor ); } - private function runHeraldMailRules(array $messages) { - foreach ($messages as $message) { - $engine = new HeraldEngine(); - $adapter = id(new PhabricatorMailOutboundMailHeraldAdapter()) - ->setObject($message); + /** + * Apply encodings prior to storage. + * + * See @{method:getCustomWorkerStateEncoding}. + * + * @param map Map of values to encode. + * @param map Map of encodings to apply. + * @return map Map of encoded values. + * @task workers + */ + final private function encodeStateForStorage( + array $state, + array $encodings) { - $rules = $engine->loadRulesForAdapter($adapter); - $effects = $engine->applyRules($rules, $adapter); - $engine->applyEffects($effects, $adapter, $rules); + foreach ($state as $key => $value) { + $encoding = idx($encodings, $key); + switch ($encoding) { + case self::STORAGE_ENCODING_BINARY: + // The mechanics of this encoding (serialize + base64) are a little + // awkward, but it allows us encode arrays and still be JSON-safe + // with binary data. + + $value = @serialize($value); + if ($value === false) { + throw new Exception( + pht( + 'Failed to serialize() value for key "%s".', + $key)); + } + + $value = base64_encode($value); + if ($value === false) { + throw new Exception( + pht( + 'Failed to base64 encode value for key "%s".', + $key)); + } + break; + } + $state[$key] = $value; } + + return $state; + } + + + /** + * Undo storage encoding applied when storing state. + * + * See @{method:getCustomWorkerStateEncoding}. + * + * @param map Map of encoded values. + * @param map Map of encodings. + * @return map Map of decoded values. + * @task workers + */ + final private function decodeStateFromStorage( + array $state, + array $encodings) { + + foreach ($state as $key => $value) { + $encoding = idx($encodings, $key); + switch ($encoding) { + case self::STORAGE_ENCODING_BINARY: + $value = base64_decode($value); + if ($value === false) { + throw new Exception( + pht( + 'Failed to base64_decode() value for key "%s".', + $key)); + } + + $value = unserialize($value); + break; + } + $state[$key] = $value; + } + + return $state; } } diff --git a/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php b/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php index 486b4da2de..c1ba981066 100644 --- a/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php +++ b/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php @@ -26,9 +26,14 @@ final class PhabricatorApplicationTransactionPublishWorker * Load the object the transactions affect. */ private function loadObject() { - $data = $this->getTaskData(); $viewer = PhabricatorUser::getOmnipotentUser(); + $data = $this->getTaskData(); + if (!is_array($data)) { + throw new PhabricatorWorkerPermanentFailureException( + pht('Task has invalid task data.')); + } + $phid = idx($data, 'objectPHID'); if (!$phid) { throw new PhabricatorWorkerPermanentFailureException( diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index d43536c911..88c3e83e74 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -1651,7 +1651,7 @@ abstract class LiskDAO extends Phobject { if ($deserialize) { $data[$col] = json_decode($data[$col], true); } else { - $data[$col] = json_encode($data[$col]); + $data[$col] = phutil_json_encode($data[$col]); } break; default: From b5672e7e55bd9f9a90f7971f4808d19474b3febc Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Aug 2015 07:19:35 -0700 Subject: [PATCH 02/18] Add a main page to Nuance Summary: Ref T8783. There's nothing at `/nuance/` right now, put something basic there. Test Plan: {F747078} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8783 Differential Revision: https://secure.phabricator.com/D13965 --- src/__phutil_library_map__.php | 2 + .../PhabricatorNuanceApplication.php | 1 + .../controller/NuanceConsoleController.php | 46 +++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 src/applications/nuance/controller/NuanceConsoleController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0942ad73d6..50992dc4b3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1269,6 +1269,7 @@ phutil_register_library_map(array( 'MultimeterSampleController' => 'applications/multimeter/controller/MultimeterSampleController.php', 'MultimeterViewer' => 'applications/multimeter/storage/MultimeterViewer.php', 'NuanceConduitAPIMethod' => 'applications/nuance/conduit/NuanceConduitAPIMethod.php', + 'NuanceConsoleController' => 'applications/nuance/controller/NuanceConsoleController.php', 'NuanceController' => 'applications/nuance/controller/NuanceController.php', 'NuanceCreateItemConduitAPIMethod' => 'applications/nuance/conduit/NuanceCreateItemConduitAPIMethod.php', 'NuanceDAO' => 'applications/nuance/storage/NuanceDAO.php', @@ -5049,6 +5050,7 @@ phutil_register_library_map(array( 'MultimeterSampleController' => 'MultimeterController', 'MultimeterViewer' => 'MultimeterDimension', 'NuanceConduitAPIMethod' => 'ConduitAPIMethod', + 'NuanceConsoleController' => 'NuanceController', 'NuanceController' => 'PhabricatorController', 'NuanceCreateItemConduitAPIMethod' => 'NuanceConduitAPIMethod', 'NuanceDAO' => 'PhabricatorLiskDAO', diff --git a/src/applications/nuance/application/PhabricatorNuanceApplication.php b/src/applications/nuance/application/PhabricatorNuanceApplication.php index 2d82f3f0b0..ce19afcd11 100644 --- a/src/applications/nuance/application/PhabricatorNuanceApplication.php +++ b/src/applications/nuance/application/PhabricatorNuanceApplication.php @@ -38,6 +38,7 @@ final class PhabricatorNuanceApplication extends PhabricatorApplication { public function getRoutes() { return array( '/nuance/' => array( + '' => 'NuanceConsoleController', 'item/' => array( 'view/(?P[1-9]\d*)/' => 'NuanceItemViewController', 'edit/(?P[1-9]\d*)/' => 'NuanceItemEditController', diff --git a/src/applications/nuance/controller/NuanceConsoleController.php b/src/applications/nuance/controller/NuanceConsoleController.php new file mode 100644 index 0000000000..78ab474fb6 --- /dev/null +++ b/src/applications/nuance/controller/NuanceConsoleController.php @@ -0,0 +1,46 @@ +getViewer(); + + $menu = id(new PHUIObjectItemListView()) + ->setUser($viewer); + + $menu->addItem( + id(new PHUIObjectItemView()) + ->setHeader(pht('Queues')) + ->setHref($this->getApplicationURI('queue/')) + ->setFontIcon('fa-align-left') + ->addAttribute(pht('Manage Nuance queues.'))); + + $menu->addItem( + id(new PHUIObjectItemView()) + ->setHeader(pht('Sources')) + ->setHref($this->getApplicationURI('source/')) + ->setFontIcon('fa-filter') + ->addAttribute(pht('Manage Nuance sources.'))); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb(pht('Console')); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Console')) + ->setObjectList($menu); + + return $this->buildApplicationPage( + array( + $crumbs, + $box, + ), + array( + 'title' => pht('Nuance Console'), + )); + } + +} From 5caeb5c4db9982324f831bbb4b0c53acef57708e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Aug 2015 07:39:04 -0700 Subject: [PATCH 03/18] Allow Nuance source definitions to add actions to source views Summary: Ref T8783. If you have a source (like a "report bug" form), let it put a link (like "View Form") on the source detail page. This also straightens out getting definitions from sources, which had a bug with the modern way we do `PhutilClassMapQuery`. Specifically, if you called the old mechanism on two different sources, they'd return the same definition object, but they need to return different definitions. Test Plan: {F747093} {F747092} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8783 Differential Revision: https://secure.phabricator.com/D13966 --- .../NuanceSourceActionController.php | 2 +- .../controller/NuanceSourceEditController.php | 2 +- .../controller/NuanceSourceViewController.php | 9 ++++++- .../NuancePhabricatorFormSourceDefinition.php | 11 ++++++++ .../nuance/source/NuanceSourceDefinition.php | 19 +++++-------- .../nuance/storage/NuanceSource.php | 27 +++++++++++++++++++ 6 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/applications/nuance/controller/NuanceSourceActionController.php b/src/applications/nuance/controller/NuanceSourceActionController.php index 6e9fabe455..06739cea1e 100644 --- a/src/applications/nuance/controller/NuanceSourceActionController.php +++ b/src/applications/nuance/controller/NuanceSourceActionController.php @@ -13,7 +13,7 @@ final class NuanceSourceActionController extends NuanceController { return new Aphront404Response(); } - $def = NuanceSourceDefinition::getDefinitionForSource($source); + $def = $source->requireDefinition(); $def->setActor($viewer); $response = $def->handleActionRequest($request); diff --git a/src/applications/nuance/controller/NuanceSourceEditController.php b/src/applications/nuance/controller/NuanceSourceEditController.php index 79bc8c94dc..18234bc778 100644 --- a/src/applications/nuance/controller/NuanceSourceEditController.php +++ b/src/applications/nuance/controller/NuanceSourceEditController.php @@ -41,7 +41,7 @@ final class NuanceSourceEditController extends NuanceController { $cancel_uri = $source->getURI(); } - $definition = NuanceSourceDefinition::getDefinitionForSource($source); + $definition = $source->requireDefinition(); $definition->setActor($viewer); $response = $definition->buildEditLayout($request); diff --git a/src/applications/nuance/controller/NuanceSourceViewController.php b/src/applications/nuance/controller/NuanceSourceViewController.php index edff9fb0a1..e902029b11 100644 --- a/src/applications/nuance/controller/NuanceSourceViewController.php +++ b/src/applications/nuance/controller/NuanceSourceViewController.php @@ -77,6 +77,13 @@ final class NuanceSourceViewController extends NuanceController { ->setDisabled(!$can_edit) ->setWorkflow(!$can_edit)); + $request = $this->getRequest(); + $definition = $source->requireDefinition(); + $source_actions = $definition->getSourceViewActions($request); + foreach ($source_actions as $source_action) { + $actions->addAction($source_action); + } + return $actions; } @@ -90,7 +97,7 @@ final class NuanceSourceViewController extends NuanceController { ->setObject($source) ->setActionList($actions); - $definition = NuanceSourceDefinition::getDefinitionForSource($source); + $definition = $source->requireDefinition(); $properties->addProperty( pht('Source Type'), $definition->getName()); diff --git a/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php b/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php index 2b596cc198..4cc3d7c610 100644 --- a/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php +++ b/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php @@ -15,6 +15,17 @@ final class NuancePhabricatorFormSourceDefinition return 'phabricator-form'; } + public function getSourceViewActions(AphrontRequest $request) { + $actions = array(); + + $actions[] = id(new PhabricatorActionView()) + ->setName(pht('View Form')) + ->setIcon('fa-align-justify') + ->setHref($this->getActionURI()); + + return $actions; + } + public function updateItems() { return null; } diff --git a/src/applications/nuance/source/NuanceSourceDefinition.php b/src/applications/nuance/source/NuanceSourceDefinition.php index 621fa615f9..2c030badfa 100644 --- a/src/applications/nuance/source/NuanceSourceDefinition.php +++ b/src/applications/nuance/source/NuanceSourceDefinition.php @@ -43,18 +43,8 @@ abstract class NuanceSourceDefinition extends Phobject { return $source; } - /** - * Gives a @{class:NuanceSourceDefinition} object for a given - * @{class:NuanceSource}. Note you still need to @{method:setActor} - * before the @{class:NuanceSourceDefinition} object will be useful. - */ - public static function getDefinitionForSource(NuanceSource $source) { - $definitions = self::getAllDefinitions(); - $map = mpull($definitions, null, 'getSourceTypeConstant'); - $definition = $map[$source->getType()]; - $definition->setSourceObject($source); - - return $definition; + public function getSourceViewActions(AphrontRequest $request) { + return array(); } public static function getAllDefinitions() { @@ -286,4 +276,9 @@ abstract class NuanceSourceDefinition extends Phobject { return new Aphront404Response(); } + public function getActionURI($path = null) { + $source_id = $this->getSourceObject()->getID(); + return '/action/'.$source_id.'/'.ltrim($path, '/'); + } + } diff --git a/src/applications/nuance/storage/NuanceSource.php b/src/applications/nuance/storage/NuanceSource.php index 01f0d6e620..6ac04ba33a 100644 --- a/src/applications/nuance/storage/NuanceSource.php +++ b/src/applications/nuance/storage/NuanceSource.php @@ -12,6 +12,8 @@ final class NuanceSource extends NuanceDAO protected $viewPolicy; protected $editPolicy; + private $definition; + protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -62,6 +64,31 @@ final class NuanceSource extends NuanceDAO ->setEditPolicy($edit_policy); } + public function getDefinition() { + if ($this->definition === null) { + $definitions = NuanceSourceDefinition::getAllDefinitions(); + if (isset($definitions[$this->getType()])) { + $definition = clone $definitions[$this->getType()]; + $definition->setSourceObject($this); + $this->definition = $definition; + } + } + + return $this->definition; + } + + public function requireDefinition() { + $definition = $this->getDefinition(); + if (!$definition) { + throw new Exception( + pht( + 'Unable to load source definition implementation for source '. + 'type "%s".', + $this->getType())); + } + return $definition; + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ From 459e0d2fa3f8989bc1470138ef638604ece61d1d Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Aug 2015 08:31:47 -0700 Subject: [PATCH 04/18] Tune document details in Legalpad Summary: Fixes T9245. These picked up some possibly-confusing metadata, like in the screenshot on T9245 where "Subscribers" appears in the middle of the page for no obvious reason. - Make these pages a little cleaner by removing elements which aren't important for signing agreements. - Use the last time the actual document text was updated as the modification time, not the last time the "Document" object was modified. The latter will change for trivial things like altering the view/edit policy, but that could be confusing if you see that a TOS was "last updated yesterday" but can't figure out what actually changed (since nothing changed). Test Plan: Viewed signature page for a document. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9245 Differential Revision: https://secure.phabricator.com/D13982 --- .../LegalpadDocumentSignController.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php index cde879da42..40697eef30 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -234,11 +234,19 @@ final class LegalpadDocumentSignController extends LegalpadController { $document, PhabricatorPolicyCapability::CAN_EDIT); + // Use the last content update as the modified date. We don't want to + // show that a document like a TOS was "updated" by an incidental change + // to a field like the preamble or privacy settings which does not acutally + // affect the content of the agreement. + $content_updated = $document_body->getDateCreated(); + + // NOTE: We're avoiding `setPolicyObject()` here so we don't pick up + // extra UI elements that are unnecessary and clutter the signature page. + // These details are available on the "Manage" page. $header = id(new PHUIHeaderView()) ->setHeader($title) ->setUser($viewer) - ->setPolicyObject($document) - ->setEpoch($document->getDateModified()) + ->setEpoch($content_updated) ->addActionLink( id(new PHUIButtonView()) ->setTag('a') @@ -258,15 +266,16 @@ final class LegalpadDocumentSignController extends LegalpadController { 'default', $viewer); + // NOTE: We're avoiding `setObject()` here so we don't pick up extra UI + // elements like "Subscribers". This information is available on the + // "Manage" page, but just clutters up the "Signature" page. $preamble = id(new PHUIPropertyListView()) ->setUser($viewer) - ->setObject($document) ->addSectionHeader(pht('Preamble')) ->addTextContent($preamble_text); $preamble_box = new PHUIPropertyGroupView(); $preamble_box->addPropertyList($preamble); - } $content = id(new PHUIDocumentView()) From 9e3f3e692d7f941a35ab6174d81358f1d42a9248 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Aug 2015 08:31:53 -0700 Subject: [PATCH 05/18] Use didRejectResult() when querying workboard columns Summary: Fixes T9250. Ref T4345. Test Plan: Faked a policy error: {F748975} Reviewers: joshuaspence, chad Reviewed By: chad Maniphest Tasks: T4345, T9250 Differential Revision: https://secure.phabricator.com/D13981 --- .../query/PhabricatorProjectColumnQuery.php | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/applications/project/query/PhabricatorProjectColumnQuery.php b/src/applications/project/query/PhabricatorProjectColumnQuery.php index c486968070..2fe901cec7 100644 --- a/src/applications/project/query/PhabricatorProjectColumnQuery.php +++ b/src/applications/project/query/PhabricatorProjectColumnQuery.php @@ -28,19 +28,12 @@ final class PhabricatorProjectColumnQuery return $this; } + public function newResultObject() { + return new PhabricatorProjectColumn(); + } + protected function loadPage() { - $table = new PhabricatorProjectColumn(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $page) { @@ -60,6 +53,7 @@ final class PhabricatorProjectColumnQuery $phid = $column->getProjectPHID(); $project = idx($projects, $phid); if (!$project) { + $this->didRejectResult($page[$key]); unset($page[$key]); continue; } @@ -69,40 +63,38 @@ final class PhabricatorProjectColumnQuery return $page; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->projectPHIDs) { + if ($this->projectPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'projectPHID IN (%Ls)', $this->projectPHIDs); } if ($this->statuses !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'status IN (%Ld)', $this->statuses); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { From c69d4658918c10d4f26fe5844fd219667d8e5799 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Aug 2015 08:34:52 -0700 Subject: [PATCH 06/18] Add basic "View" and "Edit" features to Nuance Summary: Ref T8783. The "View" UI is where a user would check their request for feedback or a resolution, if it's something that makes sense for them to interact with from the web UI. The "Edit" UI is the manage/admin UI where you'd respond to a request. It's similar to the view UI but will have actions and eventually some queue UI, etc. (I don't think items need a normal "Edit" UI -- it doesn't make sense to "Edit" a tweet or inbound email -- but maybe this will shuffle around a little eventually.) Test Plan: View {F747218} Edit {F747219} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8783 Differential Revision: https://secure.phabricator.com/D13980 --- .../controller/NuanceItemEditController.php | 85 ++++++++++++++++--- .../controller/NuanceItemViewController.php | 67 ++++++++++++++- .../nuance/phid/NuanceRequestorPHIDType.php | 4 +- .../nuance/query/NuanceItemQuery.php | 41 ++++++--- .../NuancePhabricatorFormSourceDefinition.php | 31 ++++++- .../nuance/source/NuanceSourceDefinition.php | 14 +++ .../nuance/storage/NuanceItem.php | 2 + 7 files changed, 214 insertions(+), 30 deletions(-) diff --git a/src/applications/nuance/controller/NuanceItemEditController.php b/src/applications/nuance/controller/NuanceItemEditController.php index 89bfbda5c9..bad409a801 100644 --- a/src/applications/nuance/controller/NuanceItemEditController.php +++ b/src/applications/nuance/controller/NuanceItemEditController.php @@ -3,30 +3,91 @@ final class NuanceItemEditController extends NuanceController { public function handleRequest(AphrontRequest $request) { - $viewer = $request->getViewer(); + $viewer = $this->getViewer(); $id = $request->getURIData('id'); - if (!$id) { - $item = new NuanceItem(); - } else { - $item = id(new NuanceItemQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->executeOne(); - } - + $item = id(new NuanceItemQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); if (!$item) { return new Aphront404Response(); } + $title = pht('Item %d', $item->getID()); + $crumbs = $this->buildApplicationCrumbs(); - $title = 'TODO'; + $crumbs->addTextCrumb($title); + $crumbs->addTextCrumb(pht('Edit')); + + $properties = $this->buildPropertyView($item); + $actions = $this->buildActionView($item); + $properties->setActionList($actions); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->addPropertyList($properties); return $this->buildApplicationPage( - $crumbs, + array( + $crumbs, + $box, + ), array( 'title' => $title, )); } + private function buildPropertyView(NuanceItem $item) { + $viewer = $this->getViewer(); + + $properties = id(new PHUIPropertyListView()) + ->setUser($viewer) + ->setObject($item); + + $properties->addProperty( + pht('Date Created'), + phabricator_datetime($item->getDateCreated(), $viewer)); + + $properties->addProperty( + pht('Requestor'), + $viewer->renderHandle($item->getRequestorPHID())); + + $properties->addProperty( + pht('Source'), + $viewer->renderHandle($item->getSourcePHID())); + + $source = $item->getSource(); + $definition = $source->requireDefinition(); + + $definition->renderItemEditProperties( + $viewer, + $item, + $properties); + + return $properties; + } + + private function buildActionView(NuanceItem $item) { + $viewer = $this->getViewer(); + $id = $item->getID(); + + $actions = id(new PhabricatorActionListView()) + ->setUser($viewer); + + $actions->addAction( + id(new PhabricatorActionView()) + ->setName(pht('View Item')) + ->setIcon('fa-eye') + ->setHref($this->getApplicationURI("item/view/{$id}/"))); + + return $actions; + } + + } diff --git a/src/applications/nuance/controller/NuanceItemViewController.php b/src/applications/nuance/controller/NuanceItemViewController.php index 9c7401cafe..d325afbd29 100644 --- a/src/applications/nuance/controller/NuanceItemViewController.php +++ b/src/applications/nuance/controller/NuanceItemViewController.php @@ -3,25 +3,84 @@ final class NuanceItemViewController extends NuanceController { public function handleRequest(AphrontRequest $request) { - $viewer = $request->getViewer(); + $viewer = $this->getViewer(); $id = $request->getURIData('id'); $item = id(new NuanceItemQuery()) ->setViewer($viewer) ->withIDs(array($id)) ->executeOne(); - if (!$item) { return new Aphront404Response(); } + $title = pht('Item %d', $item->getID()); + $crumbs = $this->buildApplicationCrumbs(); - $title = 'TODO'; + $crumbs->addTextCrumb($title); + + $properties = $this->buildPropertyView($item); + $actions = $this->buildActionView($item); + $properties->setActionList($actions); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->addPropertyList($properties); return $this->buildApplicationPage( - $crumbs, + array( + $crumbs, + $box, + ), array( 'title' => $title, )); } + + private function buildPropertyView(NuanceItem $item) { + $viewer = $this->getViewer(); + + $properties = id(new PHUIPropertyListView()) + ->setUser($viewer) + ->setObject($item); + + $properties->addProperty( + pht('Date Created'), + phabricator_datetime($item->getDateCreated(), $viewer)); + + $source = $item->getSource(); + $definition = $source->requireDefinition(); + + $definition->renderItemViewProperties( + $viewer, + $item, + $properties); + + return $properties; + } + + private function buildActionView(NuanceItem $item) { + $viewer = $this->getViewer(); + $id = $item->getID(); + + $actions = id(new PhabricatorActionListView()) + ->setUser($viewer); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $item, + PhabricatorPolicyCapability::CAN_EDIT); + + $actions->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Edit Item')) + ->setIcon('fa-pencil') + ->setHref($this->getApplicationURI("item/edit/{$id}/")) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit)); + + return $actions; + } + + } diff --git a/src/applications/nuance/phid/NuanceRequestorPHIDType.php b/src/applications/nuance/phid/NuanceRequestorPHIDType.php index 0e08322a80..2cf06bbb10 100644 --- a/src/applications/nuance/phid/NuanceRequestorPHIDType.php +++ b/src/applications/nuance/phid/NuanceRequestorPHIDType.php @@ -29,7 +29,9 @@ final class NuanceRequestorPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $requestor = $objects[$phid]; - $handle->setName($requestor->getBestName()); + // TODO: This is currently useless and should be far more informative. + $handle->setName(pht('Requestor %d', $requestor->getID())); + $handle->setURI($requestor->getURI()); } } diff --git a/src/applications/nuance/query/NuanceItemQuery.php b/src/applications/nuance/query/NuanceItemQuery.php index cc3f79c915..fbcac6e5b7 100644 --- a/src/applications/nuance/query/NuanceItemQuery.php +++ b/src/applications/nuance/query/NuanceItemQuery.php @@ -17,25 +17,42 @@ final class NuanceItemQuery return $this; } - public function withSourcePHIDs($source_phids) { + public function withSourcePHIDs(array $source_phids) { $this->sourcePHIDs = $source_phids; return $this; } + public function newResultObject() { + return new NuanceItem(); + } + protected function loadPage() { - $table = new NuanceItem(); - $conn = $table->establishConnection('r'); + return $this->loadStandardPage($this->newResultObject()); + } - $data = queryfx_all( - $conn, - '%Q FROM %T %Q %Q %Q', - $this->buildSelectClause($conn), - $table->getTableName(), - $this->buildWhereClause($conn), - $this->buildOrderClause($conn), - $this->buildLimitClause($conn)); + protected function willFilterPage(array $items) { + $source_phids = mpull($items, 'getSourcePHID'); - return $table->loadAllFromArray($data); + // NOTE: We always load sources, even if the viewer can't formally see + // them. If they can see the item, they're allowed to be aware of the + // source in some sense. + $sources = id(new NuanceSourceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($source_phids) + ->execute(); + $sources = mpull($sources, null, 'getPHID'); + + foreach ($items as $key => $item) { + $source = idx($sources, $item->getSourcePHID()); + if (!$source) { + $this->didRejectResult($items[$key]); + unset($items[$key]); + continue; + } + $item->attachSource($source); + } + + return $items; } protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { diff --git a/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php b/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php index 4cc3d7c610..607b0eda6c 100644 --- a/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php +++ b/src/applications/nuance/source/NuancePhabricatorFormSourceDefinition.php @@ -61,7 +61,7 @@ final class NuancePhabricatorFormSourceDefinition if ($request->isFormPost()) { $properties = array( - 'complaint' => (string)$request->getStr('text'), + 'complaint' => (string)$request->getStr('complaint'), ); $content_source = PhabricatorContentSource::newFromRequest($request); @@ -100,4 +100,33 @@ final class NuancePhabricatorFormSourceDefinition return $box; } + public function renderItemViewProperties( + PhabricatorUser $viewer, + NuanceItem $item, + PHUIPropertyListView $view) { + $this->renderItemCommonProperties($viewer, $item, $view); + } + + public function renderItemEditProperties( + PhabricatorUser $viewer, + NuanceItem $item, + PHUIPropertyListView $view) { + $this->renderItemCommonProperties($viewer, $item, $view); + } + + private function renderItemCommonProperties( + PhabricatorUser $viewer, + NuanceItem $item, + PHUIPropertyListView $view) { + + $complaint = $item->getNuanceProperty('complaint'); + $complaint = PhabricatorMarkupEngine::renderOneObject( + id(new PhabricatorMarkupOneOff())->setContent($complaint), + 'default', + $viewer); + + $view->addSectionHeader(pht('Complaint')); + $view->addTextContent($complaint); + } + } diff --git a/src/applications/nuance/source/NuanceSourceDefinition.php b/src/applications/nuance/source/NuanceSourceDefinition.php index 2c030badfa..06d302f587 100644 --- a/src/applications/nuance/source/NuanceSourceDefinition.php +++ b/src/applications/nuance/source/NuanceSourceDefinition.php @@ -268,6 +268,20 @@ abstract class NuanceSourceDefinition extends Phobject { return $item; } + public function renderItemViewProperties( + PhabricatorUser $viewer, + NuanceItem $item, + PHUIPropertyListView $view) { + return; + } + + public function renderItemEditProperties( + PhabricatorUser $viewer, + NuanceItem $item, + PHUIPropertyListView $view) { + return; + } + /* -( Handling Action Requests )------------------------------------------- */ diff --git a/src/applications/nuance/storage/NuanceItem.php b/src/applications/nuance/storage/NuanceItem.php index 196afc44ca..2335cd4d42 100644 --- a/src/applications/nuance/storage/NuanceItem.php +++ b/src/applications/nuance/storage/NuanceItem.php @@ -19,6 +19,8 @@ final class NuanceItem protected $mailKey; protected $dateNuanced; + private $source = self::ATTACHABLE; + public static function initializeNewItem() { return id(new NuanceItem()) ->setDateNuanced(time()) From 254b394f2c2b299e29f8a16f93aa80488028afe7 Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Mon, 24 Aug 2015 17:20:59 +1000 Subject: [PATCH 07/18] Associate Harbormaster build target with leases Summary: Ref T1049. This ensures the Harbormaster build target is associated with leases, so in the future we can query things and find out whether builds are still running with associated leases. Test Plan: Leased a host, checked the DB and saw the field populated. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: joshuaspence, Korvin, epriestley Maniphest Tasks: T1049 Differential Revision: https://secure.phabricator.com/D10870 --- .../step/HarbormasterLeaseHostBuildStepImplementation.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php index 127025fd26..b14d0975f7 100644 --- a/src/applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php @@ -24,6 +24,7 @@ final class HarbormasterLeaseHostBuildStepImplementation // Create the lease. $lease = id(new DrydockLease()) ->setResourceType('host') + ->setOwnerPHID($build_target->getPHID()) ->setAttributes( array( 'platform' => $settings['platform'], From ea3d528c4c8f2e7a05494af9478c4302aa462604 Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Mon, 24 Aug 2015 17:22:32 +1000 Subject: [PATCH 08/18] Show time on Drydock logs Summary: Show the time in addition to the date in the Drydock logs. Test Plan: Brought forward from D10479. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: joshuaspence, Korvin, epriestley Differential Revision: https://secure.phabricator.com/D10909 --- src/applications/drydock/view/DrydockLogListView.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/drydock/view/DrydockLogListView.php b/src/applications/drydock/view/DrydockLogListView.php index 65f4969d5e..51a385fbb2 100644 --- a/src/applications/drydock/view/DrydockLogListView.php +++ b/src/applications/drydock/view/DrydockLogListView.php @@ -35,7 +35,7 @@ final class DrydockLogListView extends AphrontView { ), $log->getLeaseID()), $log->getMessage(), - phabricator_date($log->getEpoch(), $viewer), + phabricator_datetime($log->getEpoch(), $viewer), ); } From 0d4f9363a0f2b4141e922e82117d3e0194bbb57a Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Mon, 24 Aug 2015 21:13:20 +1000 Subject: [PATCH 09/18] Improve Drydock log search engine Summary: Ref T2015. This allows searching based on blueprints, resources or leases when viewing the logs, which helps when searching for events that occured to a particular blueprint / resource / lease. Unlike the logs shown on the resource / lease pages, the search engine supports paging properly, which means it can be used to find entries in the past. Test Plan: Used the Drydock log search page. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: joshuaspence, Korvin, epriestley Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D10874 --- src/__phutil_library_map__.php | 6 +++ .../drydock/phid/DrydockLeasePHIDType.php | 3 ++ .../drydock/phid/DrydockResourcePHIDType.php | 1 + .../drydock/query/DrydockBlueprintQuery.php | 13 ++++++ .../drydock/query/DrydockLeaseQuery.php | 21 +++++++-- .../drydock/query/DrydockLogSearchEngine.php | 45 +++++++++++++++++-- .../drydock/query/DrydockResourceQuery.php | 13 ++++++ .../drydock/storage/DrydockBlueprint.php | 2 +- .../typeahead/DrydockBlueprintDatasource.php | 36 +++++++++++++++ .../typeahead/DrydockLeaseDatasource.php | 36 +++++++++++++++ .../typeahead/DrydockResourceDatasource.php | 36 +++++++++++++++ .../drydock/view/DrydockLogListView.php | 7 ++- 12 files changed, 210 insertions(+), 9 deletions(-) create mode 100644 src/applications/drydock/typeahead/DrydockBlueprintDatasource.php create mode 100644 src/applications/drydock/typeahead/DrydockLeaseDatasource.php create mode 100644 src/applications/drydock/typeahead/DrydockResourceDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 50992dc4b3..c70fc08ca0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -798,6 +798,7 @@ phutil_register_library_map(array( 'DrydockBlueprintCoreCustomField' => 'applications/drydock/customfield/DrydockBlueprintCoreCustomField.php', 'DrydockBlueprintCreateController' => 'applications/drydock/controller/DrydockBlueprintCreateController.php', 'DrydockBlueprintCustomField' => 'applications/drydock/customfield/DrydockBlueprintCustomField.php', + 'DrydockBlueprintDatasource' => 'applications/drydock/typeahead/DrydockBlueprintDatasource.php', 'DrydockBlueprintEditController' => 'applications/drydock/controller/DrydockBlueprintEditController.php', 'DrydockBlueprintEditor' => 'applications/drydock/editor/DrydockBlueprintEditor.php', 'DrydockBlueprintImplementation' => 'applications/drydock/blueprint/DrydockBlueprintImplementation.php', @@ -822,6 +823,7 @@ phutil_register_library_map(array( 'DrydockInterface' => 'applications/drydock/interface/DrydockInterface.php', 'DrydockLease' => 'applications/drydock/storage/DrydockLease.php', 'DrydockLeaseController' => 'applications/drydock/controller/DrydockLeaseController.php', + 'DrydockLeaseDatasource' => 'applications/drydock/typeahead/DrydockLeaseDatasource.php', 'DrydockLeaseListController' => 'applications/drydock/controller/DrydockLeaseListController.php', 'DrydockLeaseListView' => 'applications/drydock/view/DrydockLeaseListView.php', 'DrydockLeasePHIDType' => 'applications/drydock/phid/DrydockLeasePHIDType.php', @@ -847,6 +849,7 @@ phutil_register_library_map(array( 'DrydockResource' => 'applications/drydock/storage/DrydockResource.php', 'DrydockResourceCloseController' => 'applications/drydock/controller/DrydockResourceCloseController.php', 'DrydockResourceController' => 'applications/drydock/controller/DrydockResourceController.php', + 'DrydockResourceDatasource' => 'applications/drydock/typeahead/DrydockResourceDatasource.php', 'DrydockResourceListController' => 'applications/drydock/controller/DrydockResourceListController.php', 'DrydockResourceListView' => 'applications/drydock/view/DrydockResourceListView.php', 'DrydockResourcePHIDType' => 'applications/drydock/phid/DrydockResourcePHIDType.php', @@ -4479,6 +4482,7 @@ phutil_register_library_map(array( ), 'DrydockBlueprintCreateController' => 'DrydockBlueprintController', 'DrydockBlueprintCustomField' => 'PhabricatorCustomField', + 'DrydockBlueprintDatasource' => 'PhabricatorTypeaheadDatasource', 'DrydockBlueprintEditController' => 'DrydockBlueprintController', 'DrydockBlueprintEditor' => 'PhabricatorApplicationTransactionEditor', 'DrydockBlueprintImplementation' => 'Phobject', @@ -4506,6 +4510,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', ), 'DrydockLeaseController' => 'DrydockController', + 'DrydockLeaseDatasource' => 'PhabricatorTypeaheadDatasource', 'DrydockLeaseListController' => 'DrydockLeaseController', 'DrydockLeaseListView' => 'AphrontView', 'DrydockLeasePHIDType' => 'PhabricatorPHIDType', @@ -4537,6 +4542,7 @@ phutil_register_library_map(array( ), 'DrydockResourceCloseController' => 'DrydockResourceController', 'DrydockResourceController' => 'DrydockController', + 'DrydockResourceDatasource' => 'PhabricatorTypeaheadDatasource', 'DrydockResourceListController' => 'DrydockResourceController', 'DrydockResourceListView' => 'AphrontView', 'DrydockResourcePHIDType' => 'PhabricatorPHIDType', diff --git a/src/applications/drydock/phid/DrydockLeasePHIDType.php b/src/applications/drydock/phid/DrydockLeasePHIDType.php index 157bef6679..c3006164e5 100644 --- a/src/applications/drydock/phid/DrydockLeasePHIDType.php +++ b/src/applications/drydock/phid/DrydockLeasePHIDType.php @@ -29,6 +29,9 @@ final class DrydockLeasePHIDType extends PhabricatorPHIDType { $lease = $objects[$phid]; $id = $lease->getID(); + $handle->setName(pht( + 'Lease %d', + $id)); $handle->setURI("/drydock/lease/{$id}/"); } } diff --git a/src/applications/drydock/phid/DrydockResourcePHIDType.php b/src/applications/drydock/phid/DrydockResourcePHIDType.php index 36f9a8bbbd..1bfcc68597 100644 --- a/src/applications/drydock/phid/DrydockResourcePHIDType.php +++ b/src/applications/drydock/phid/DrydockResourcePHIDType.php @@ -29,6 +29,7 @@ final class DrydockResourcePHIDType extends PhabricatorPHIDType { $resource = $objects[$phid]; $id = $resource->getID(); + $handle->setName($resource->getName()); $handle->setURI("/drydock/resource/{$id}/"); } } diff --git a/src/applications/drydock/query/DrydockBlueprintQuery.php b/src/applications/drydock/query/DrydockBlueprintQuery.php index b698855bcf..23081c8910 100644 --- a/src/applications/drydock/query/DrydockBlueprintQuery.php +++ b/src/applications/drydock/query/DrydockBlueprintQuery.php @@ -4,6 +4,7 @@ final class DrydockBlueprintQuery extends DrydockQuery { private $ids; private $phids; + private $datasourceQuery; public function withIDs(array $ids) { $this->ids = $ids; @@ -15,6 +16,11 @@ final class DrydockBlueprintQuery extends DrydockQuery { return $this; } + public function withDatasourceQuery($query) { + $this->datasourceQuery = $query; + return $this; + } + protected function loadPage() { $table = new DrydockBlueprint(); $conn_r = $table->establishConnection('r'); @@ -59,6 +65,13 @@ final class DrydockBlueprintQuery extends DrydockQuery { $this->phids); } + if ($this->datasourceQuery) { + $where[] = qsprintf( + $conn_r, + 'blueprintName LIKE %>', + $this->datasourceQuery); + } + return $this->formatWhereClause($where); } diff --git a/src/applications/drydock/query/DrydockLeaseQuery.php b/src/applications/drydock/query/DrydockLeaseQuery.php index 23cad90e9c..b2bbfe9d66 100644 --- a/src/applications/drydock/query/DrydockLeaseQuery.php +++ b/src/applications/drydock/query/DrydockLeaseQuery.php @@ -6,6 +6,7 @@ final class DrydockLeaseQuery extends DrydockQuery { private $phids; private $resourceIDs; private $statuses; + private $datasourceQuery; public function withIDs(array $ids) { $this->ids = $ids; @@ -31,6 +32,11 @@ final class DrydockLeaseQuery extends DrydockQuery { return new DrydockLease(); } + public function withDatasourceQuery($query) { + $this->datasourceQuery = $query; + return $this; + } + protected function loadPage() { return $this->loadStandardPage($this->newResultObject()); } @@ -65,34 +71,41 @@ final class DrydockLeaseQuery extends DrydockQuery { protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { $where = parent::buildWhereClauseParts($conn); - if ($this->resourceIDs !== null) { + if ($this->resourceIDs) { $where[] = qsprintf( $conn, 'resourceID IN (%Ld)', $this->resourceIDs); } - if ($this->ids !== null) { + if ($this->ids) { $where[] = qsprintf( $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids !== null) { + if ($this->phids) { $where[] = qsprintf( $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->statuses !== null) { + if ($this->statuses) { $where[] = qsprintf( $conn, 'status IN (%Ld)', $this->statuses); } + if ($this->datasourceQuery) { + $where[] = qsprintf( + $conn, + 'id = %d', + (int)$this->datasourceQuery); + } + return $where; } diff --git a/src/applications/drydock/query/DrydockLogSearchEngine.php b/src/applications/drydock/query/DrydockLogSearchEngine.php index e7233d086f..85114394e8 100644 --- a/src/applications/drydock/query/DrydockLogSearchEngine.php +++ b/src/applications/drydock/query/DrydockLogSearchEngine.php @@ -11,16 +11,55 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine { } public function buildSavedQueryFromRequest(AphrontRequest $request) { - return new PhabricatorSavedQuery(); + $query = new PhabricatorSavedQuery(); + + $query->setParameter( + 'resourcePHIDs', + $this->readListFromRequest($request, 'resources')); + $query->setParameter( + 'leasePHIDs', + $this->readListFromRequest($request, 'leases')); + + return $query; } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - return new DrydockLogQuery(); + + // TODO: Change logs to use PHIDs instead of IDs. + $resource_ids = id(new DrydockResourceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($saved->getParameter('resourcePHIDs', array())) + ->execute(); + $resource_ids = mpull($resource_ids, 'getID'); + $lease_ids = id(new DrydockLeaseQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($saved->getParameter('leasePHIDs', array())) + ->execute(); + $lease_ids = mpull($lease_ids, 'getID'); + + return id(new DrydockLogQuery()) + ->withResourceIDs($resource_ids) + ->withLeaseIDs($lease_ids); } public function buildSearchForm( AphrontFormView $form, - PhabricatorSavedQuery $saved) {} + PhabricatorSavedQuery $saved) { + + $form + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setDatasource(new DrydockResourceDatasource()) + ->setName('resources') + ->setLabel(pht('Resources')) + ->setValue($saved->getParameter('resourcePHIDs', array()))) + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setDatasource(new DrydockLeaseDatasource()) + ->setName('leases') + ->setLabel(pht('Leases')) + ->setValue($saved->getParameter('leasePHIDs', array()))); + } protected function getURI($path) { return '/drydock/log/'.$path; diff --git a/src/applications/drydock/query/DrydockResourceQuery.php b/src/applications/drydock/query/DrydockResourceQuery.php index af84fefa4a..cdc465b93a 100644 --- a/src/applications/drydock/query/DrydockResourceQuery.php +++ b/src/applications/drydock/query/DrydockResourceQuery.php @@ -7,6 +7,7 @@ final class DrydockResourceQuery extends DrydockQuery { private $statuses; private $types; private $blueprintPHIDs; + private $datasourceQuery; public function withIDs(array $ids) { $this->ids = $ids; @@ -33,6 +34,11 @@ final class DrydockResourceQuery extends DrydockQuery { return $this; } + public function withDatasourceQuery($query) { + $this->datasourceQuery = $query; + return $this; + } + protected function loadPage() { $table = new DrydockResource(); $conn_r = $table->establishConnection('r'); @@ -88,6 +94,13 @@ final class DrydockResourceQuery extends DrydockQuery { $this->blueprintPHIDs); } + if ($this->datasourceQuery) { + $where[] = qsprintf( + $conn_r, + 'name LIKE %>', + $this->datasourceQuery); + } + $where[] = $this->buildPagingClause($conn_r); return $this->formatWhereClause($where); diff --git a/src/applications/drydock/storage/DrydockBlueprint.php b/src/applications/drydock/storage/DrydockBlueprint.php index bad3f9a2dc..8185d35388 100644 --- a/src/applications/drydock/storage/DrydockBlueprint.php +++ b/src/applications/drydock/storage/DrydockBlueprint.php @@ -40,7 +40,7 @@ final class DrydockBlueprint extends DrydockDAO ), self::CONFIG_COLUMN_SCHEMA => array( 'className' => 'text255', - 'blueprintName' => 'text255', + 'blueprintName' => 'sort255', ), ) + parent::getConfiguration(); } diff --git a/src/applications/drydock/typeahead/DrydockBlueprintDatasource.php b/src/applications/drydock/typeahead/DrydockBlueprintDatasource.php new file mode 100644 index 0000000000..8b838cd2b6 --- /dev/null +++ b/src/applications/drydock/typeahead/DrydockBlueprintDatasource.php @@ -0,0 +1,36 @@ +getViewer(); + $raw_query = $this->getRawQuery(); + + $blueprints = id(new DrydockBlueprintQuery()) + ->setViewer($viewer) + ->withDatasourceQuery($raw_query) + ->execute(); + + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(mpull($blueprints, 'getPHID')) + ->execute(); + + $results = array(); + foreach ($handles as $handle) { + $results[] = id(new PhabricatorTypeaheadResult()) + ->setName($handle->getName()) + ->setPHID($handle->getPHID()); + } + return $results; + } +} diff --git a/src/applications/drydock/typeahead/DrydockLeaseDatasource.php b/src/applications/drydock/typeahead/DrydockLeaseDatasource.php new file mode 100644 index 0000000000..feb6bd9368 --- /dev/null +++ b/src/applications/drydock/typeahead/DrydockLeaseDatasource.php @@ -0,0 +1,36 @@ +getViewer(); + $raw_query = $this->getRawQuery(); + + $leases = id(new DrydockLeaseQuery()) + ->setViewer($viewer) + ->withDatasourceQuery($raw_query) + ->execute(); + + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(mpull($leases, 'getPHID')) + ->execute(); + + $results = array(); + foreach ($handles as $handle) { + $results[] = id(new PhabricatorTypeaheadResult()) + ->setName($handle->getName()) + ->setPHID($handle->getPHID()); + } + return $results; + } +} diff --git a/src/applications/drydock/typeahead/DrydockResourceDatasource.php b/src/applications/drydock/typeahead/DrydockResourceDatasource.php new file mode 100644 index 0000000000..6e0b8f7116 --- /dev/null +++ b/src/applications/drydock/typeahead/DrydockResourceDatasource.php @@ -0,0 +1,36 @@ +getViewer(); + $raw_query = $this->getRawQuery(); + + $resources = id(new DrydockResourceQuery()) + ->setViewer($viewer) + ->withDatasourceQuery($raw_query) + ->execute(); + + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(mpull($resources, 'getPHID')) + ->execute(); + + $results = array(); + foreach ($handles as $handle) { + $results[] = id(new PhabricatorTypeaheadResult()) + ->setName($handle->getName()) + ->setPHID($handle->getPHID()); + } + return $results; + } +} diff --git a/src/applications/drydock/view/DrydockLogListView.php b/src/applications/drydock/view/DrydockLogListView.php index 51a385fbb2..22e280939b 100644 --- a/src/applications/drydock/view/DrydockLogListView.php +++ b/src/applications/drydock/view/DrydockLogListView.php @@ -21,13 +21,18 @@ final class DrydockLogListView extends AphrontView { $resource_uri = '/drydock/resource/'.$log->getResourceID().'/'; $lease_uri = '/drydock/lease/'.$log->getLeaseID().'/'; + $resource_name = $log->getResourceID(); + if ($log->getResourceID() !== null) { + $resource_name = $log->getResource()->getName(); + } + $rows[] = array( phutil_tag( 'a', array( 'href' => $resource_uri, ), - $log->getResourceID()), + $resource_name), phutil_tag( 'a', array( From e55a197dd6833d658a6065e676e6246c5b4ce69f Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Mon, 24 Aug 2015 21:23:04 +1000 Subject: [PATCH 10/18] Fix issues where Drydock queries didn't work correctly with empty arrays Summary: Ref T2015. This fixes issues where the Drydock queries wouldn't filter (or throw an exception) when passed empty arrays for their `with` methods. In addition, this also adds `array_unique` to the resource and lease subqueries so that we don't pull in a bunch of stuff if logs or leases have the same related objects. Test Plan: Tested it by using DarkConsole on the log controller. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: joshuaspence, Korvin, epriestley Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D10879 --- .../drydock/query/DrydockBlueprintQuery.php | 6 +-- .../drydock/query/DrydockLeaseQuery.php | 12 ++--- .../drydock/query/DrydockLogQuery.php | 8 ++-- .../drydock/query/DrydockLogSearchEngine.php | 44 +++++++++++++------ .../drydock/query/DrydockResourceQuery.php | 12 ++--- 5 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/applications/drydock/query/DrydockBlueprintQuery.php b/src/applications/drydock/query/DrydockBlueprintQuery.php index 23081c8910..abad714d8f 100644 --- a/src/applications/drydock/query/DrydockBlueprintQuery.php +++ b/src/applications/drydock/query/DrydockBlueprintQuery.php @@ -51,21 +51,21 @@ final class DrydockBlueprintQuery extends DrydockQuery { protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn_r, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( $conn_r, 'phid IN (%Ls)', $this->phids); } - if ($this->datasourceQuery) { + if ($this->datasourceQuery !== null) { $where[] = qsprintf( $conn_r, 'blueprintName LIKE %>', diff --git a/src/applications/drydock/query/DrydockLeaseQuery.php b/src/applications/drydock/query/DrydockLeaseQuery.php index b2bbfe9d66..37f02bd748 100644 --- a/src/applications/drydock/query/DrydockLeaseQuery.php +++ b/src/applications/drydock/query/DrydockLeaseQuery.php @@ -47,7 +47,7 @@ final class DrydockLeaseQuery extends DrydockQuery { $resources = id(new DrydockResourceQuery()) ->setParentQuery($this) ->setViewer($this->getViewer()) - ->withIDs($resource_ids) + ->withIDs(array_unique($resource_ids)) ->execute(); } else { $resources = array(); @@ -71,35 +71,35 @@ final class DrydockLeaseQuery extends DrydockQuery { protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { $where = parent::buildWhereClauseParts($conn); - if ($this->resourceIDs) { + if ($this->resourceIDs !== null) { $where[] = qsprintf( $conn, 'resourceID IN (%Ld)', $this->resourceIDs); } - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->statuses) { + if ($this->statuses !== null) { $where[] = qsprintf( $conn, 'status IN (%Ld)', $this->statuses); } - if ($this->datasourceQuery) { + if ($this->datasourceQuery !== null) { $where[] = qsprintf( $conn, 'id = %d', diff --git a/src/applications/drydock/query/DrydockLogQuery.php b/src/applications/drydock/query/DrydockLogQuery.php index 1e21864f10..47a6795463 100644 --- a/src/applications/drydock/query/DrydockLogQuery.php +++ b/src/applications/drydock/query/DrydockLogQuery.php @@ -36,7 +36,7 @@ final class DrydockLogQuery extends DrydockQuery { $resources = id(new DrydockResourceQuery()) ->setParentQuery($this) ->setViewer($this->getViewer()) - ->withIDs($resource_ids) + ->withIDs(array_unique($resource_ids)) ->execute(); } else { $resources = array(); @@ -59,7 +59,7 @@ final class DrydockLogQuery extends DrydockQuery { $leases = id(new DrydockLeaseQuery()) ->setParentQuery($this) ->setViewer($this->getViewer()) - ->withIDs($lease_ids) + ->withIDs(array_unique($lease_ids)) ->execute(); } else { $leases = array(); @@ -91,14 +91,14 @@ final class DrydockLogQuery extends DrydockQuery { protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); - if ($this->resourceIDs) { + if ($this->resourceIDs !== null) { $where[] = qsprintf( $conn_r, 'resourceID IN (%Ld)', $this->resourceIDs); } - if ($this->leaseIDs) { + if ($this->leaseIDs !== null) { $where[] = qsprintf( $conn_r, 'leaseID IN (%Ld)', diff --git a/src/applications/drydock/query/DrydockLogSearchEngine.php b/src/applications/drydock/query/DrydockLogSearchEngine.php index 85114394e8..13777031d6 100644 --- a/src/applications/drydock/query/DrydockLogSearchEngine.php +++ b/src/applications/drydock/query/DrydockLogSearchEngine.php @@ -24,22 +24,38 @@ final class DrydockLogSearchEngine extends PhabricatorApplicationSearchEngine { } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { + $resource_phids = $saved->getParameter('resourcePHIDs', array()); + $lease_phids = $saved->getParameter('leasePHIDs', array()); - // TODO: Change logs to use PHIDs instead of IDs. - $resource_ids = id(new DrydockResourceQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($saved->getParameter('resourcePHIDs', array())) - ->execute(); - $resource_ids = mpull($resource_ids, 'getID'); - $lease_ids = id(new DrydockLeaseQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($saved->getParameter('leasePHIDs', array())) - ->execute(); - $lease_ids = mpull($lease_ids, 'getID'); + // TODO: Change logs to use PHIDs instead of IDs. + $resource_ids = array(); + $lease_ids = array(); - return id(new DrydockLogQuery()) - ->withResourceIDs($resource_ids) - ->withLeaseIDs($lease_ids); + if ($resource_phids) { + $resource_ids = id(new DrydockResourceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($resource_phids) + ->execute(); + $resource_ids = mpull($resource_ids, 'getID'); + } + + if ($lease_phids) { + $lease_ids = id(new DrydockLeaseQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($lease_phids) + ->execute(); + $lease_ids = mpull($lease_ids, 'getID'); + } + + $query = new DrydockLogQuery(); + if ($resource_ids) { + $query->withResourceIDs($resource_ids); + } + if ($lease_ids) { + $query->withLeaseIDs($lease_ids); + } + + return $query; } public function buildSearchForm( diff --git a/src/applications/drydock/query/DrydockResourceQuery.php b/src/applications/drydock/query/DrydockResourceQuery.php index cdc465b93a..d07e729276 100644 --- a/src/applications/drydock/query/DrydockResourceQuery.php +++ b/src/applications/drydock/query/DrydockResourceQuery.php @@ -59,42 +59,42 @@ final class DrydockResourceQuery extends DrydockQuery { protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn_r, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( $conn_r, 'phid IN (%Ls)', $this->phids); } - if ($this->types) { + if ($this->types !== null) { $where[] = qsprintf( $conn_r, 'type IN (%Ls)', $this->types); } - if ($this->statuses) { + if ($this->statuses !== null) { $where[] = qsprintf( $conn_r, 'status IN (%Ls)', $this->statuses); } - if ($this->blueprintPHIDs) { + if ($this->blueprintPHIDs !== null) { $where[] = qsprintf( $conn_r, 'blueprintPHID IN (%Ls)', $this->blueprintPHIDs); } - if ($this->datasourceQuery) { + if ($this->datasourceQuery !== null) { $where[] = qsprintf( $conn_r, 'name LIKE %>', From c6125798542deffa6cb044c4f98485b6719523e3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 24 Aug 2015 09:21:56 -0700 Subject: [PATCH 11/18] Add very basic routing to Nuance Summary: Ref T8783. Sort out some relationships and fields: - Make Items 1:1 with Queues: each item is always in exactly one queue. Minor discussion on T8783. I think this is easier to understand and reason about (and implement!) and can't come up with any real cases where it isn't powerful enough. - Remove "QueueItem", which allowed items to be in multiple queues at once. - Remove "dateNuanced", which is equivalent to "dateCreated" in all cases. Then add really basic routing: - Add "Default Queue" for Sources. New items from the source route into that queue. - (Some day there will be routing rules, but for now the rule is "always route into the default queue".) - Show queue on items. - Show more / more useful edit history and transactions in several UIs. Test Plan: {F749445} {F749446} {F749447} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8783 Differential Revision: https://secure.phabricator.com/D13988 --- .../autopatches/20150823.nuance.queue.1.sql | 1 + .../autopatches/20150823.nuance.queue.2.sql | 2 + .../autopatches/20150823.nuance.queue.3.sql | 2 + .../autopatches/20150823.nuance.queue.4.sql | 2 + src/__phutil_library_map__.php | 4 +- .../controller/NuanceItemEditController.php | 11 ++++ .../controller/NuanceSourceViewController.php | 21 +++++- .../nuance/editor/NuanceItemEditor.php | 8 +++ .../nuance/editor/NuanceSourceEditor.php | 21 ++++++ .../nuance/source/NuanceSourceDefinition.php | 52 +++++++++++---- .../nuance/storage/NuanceItem.php | 16 ++--- .../nuance/storage/NuanceItemTransaction.php | 52 +++++++++++++++ .../nuance/storage/NuanceQueueItem.php | 34 ---------- .../nuance/storage/NuanceQueueTransaction.php | 30 +++++++++ .../nuance/storage/NuanceSource.php | 1 + .../storage/NuanceSourceTransaction.php | 65 +++++++++++++++---- .../typeahead/NuanceQueueDatasource.php | 38 +++++++++++ 17 files changed, 288 insertions(+), 72 deletions(-) create mode 100644 resources/sql/autopatches/20150823.nuance.queue.1.sql create mode 100644 resources/sql/autopatches/20150823.nuance.queue.2.sql create mode 100644 resources/sql/autopatches/20150823.nuance.queue.3.sql create mode 100644 resources/sql/autopatches/20150823.nuance.queue.4.sql delete mode 100644 src/applications/nuance/storage/NuanceQueueItem.php create mode 100644 src/applications/nuance/typeahead/NuanceQueueDatasource.php diff --git a/resources/sql/autopatches/20150823.nuance.queue.1.sql b/resources/sql/autopatches/20150823.nuance.queue.1.sql new file mode 100644 index 0000000000..bb7dbba88b --- /dev/null +++ b/resources/sql/autopatches/20150823.nuance.queue.1.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_nuance.nuance_queueitem; diff --git a/resources/sql/autopatches/20150823.nuance.queue.2.sql b/resources/sql/autopatches/20150823.nuance.queue.2.sql new file mode 100644 index 0000000000..ed906d42fe --- /dev/null +++ b/resources/sql/autopatches/20150823.nuance.queue.2.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_nuance.nuance_item + ADD queuePHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20150823.nuance.queue.3.sql b/resources/sql/autopatches/20150823.nuance.queue.3.sql new file mode 100644 index 0000000000..deb343a031 --- /dev/null +++ b/resources/sql/autopatches/20150823.nuance.queue.3.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_nuance.nuance_source + ADD defaultQueuePHID VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20150823.nuance.queue.4.sql b/resources/sql/autopatches/20150823.nuance.queue.4.sql new file mode 100644 index 0000000000..d782b9de98 --- /dev/null +++ b/resources/sql/autopatches/20150823.nuance.queue.4.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_nuance.nuance_item + DROP dateNuanced; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c70fc08ca0..697a5ad11b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1288,9 +1288,9 @@ phutil_register_library_map(array( 'NuancePhabricatorFormSourceDefinition' => 'applications/nuance/source/NuancePhabricatorFormSourceDefinition.php', 'NuanceQuery' => 'applications/nuance/query/NuanceQuery.php', 'NuanceQueue' => 'applications/nuance/storage/NuanceQueue.php', + 'NuanceQueueDatasource' => 'applications/nuance/typeahead/NuanceQueueDatasource.php', 'NuanceQueueEditController' => 'applications/nuance/controller/NuanceQueueEditController.php', 'NuanceQueueEditor' => 'applications/nuance/editor/NuanceQueueEditor.php', - 'NuanceQueueItem' => 'applications/nuance/storage/NuanceQueueItem.php', 'NuanceQueueListController' => 'applications/nuance/controller/NuanceQueueListController.php', 'NuanceQueuePHIDType' => 'applications/nuance/phid/NuanceQueuePHIDType.php', 'NuanceQueueQuery' => 'applications/nuance/query/NuanceQueueQuery.php', @@ -5080,9 +5080,9 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', 'PhabricatorApplicationTransactionInterface', ), + 'NuanceQueueDatasource' => 'PhabricatorTypeaheadDatasource', 'NuanceQueueEditController' => 'NuanceController', 'NuanceQueueEditor' => 'PhabricatorApplicationTransactionEditor', - 'NuanceQueueItem' => 'NuanceDAO', 'NuanceQueueListController' => 'NuanceController', 'NuanceQueuePHIDType' => 'PhabricatorPHIDType', 'NuanceQueueQuery' => 'NuanceQuery', diff --git a/src/applications/nuance/controller/NuanceItemEditController.php b/src/applications/nuance/controller/NuanceItemEditController.php index bad409a801..afb84e6471 100644 --- a/src/applications/nuance/controller/NuanceItemEditController.php +++ b/src/applications/nuance/controller/NuanceItemEditController.php @@ -33,10 +33,17 @@ final class NuanceItemEditController extends NuanceController { ->setHeaderText($title) ->addPropertyList($properties); + $timeline = $this->buildTransactionTimeline( + $item, + new NuanceItemTransactionQuery()); + + $timeline->setShouldTerminate(true); + return $this->buildApplicationPage( array( $crumbs, $box, + $timeline, ), array( 'title' => $title, @@ -62,6 +69,10 @@ final class NuanceItemEditController extends NuanceController { pht('Source'), $viewer->renderHandle($item->getSourcePHID())); + $properties->addProperty( + pht('Queue'), + $viewer->renderHandle($item->getQueuePHID())); + $source = $item->getSource(); $definition = $source->requireDefinition(); diff --git a/src/applications/nuance/controller/NuanceSourceViewController.php b/src/applications/nuance/controller/NuanceSourceViewController.php index e902029b11..7b8d9e8251 100644 --- a/src/applications/nuance/controller/NuanceSourceViewController.php +++ b/src/applications/nuance/controller/NuanceSourceViewController.php @@ -13,7 +13,7 @@ final class NuanceSourceViewController extends NuanceController { return new Aphront404Response(); } - $source_phid = $source->getPHID(); + $source_id = $source->getID(); $timeline = $this->buildTransactionTimeline( $source, @@ -34,10 +34,29 @@ final class NuanceSourceViewController extends NuanceController { $crumbs->addTextCrumb($title); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $source, + PhabricatorPolicyCapability::CAN_EDIT); + + $routing_list = id(new PHUIPropertyListView()) + ->addProperty( + pht('Default Queue'), + $viewer->renderHandle($source->getDefaultQueuePHID())); + + $routing_header = id(new PHUIHeaderView()) + ->setHeader(pht('Routing Rules')); + + $routing = id(new PHUIObjectBoxView()) + ->setHeader($routing_header) + ->addPropertyList($routing_list); + return $this->buildApplicationPage( array( $crumbs, $box, + $routing, $timeline, ), array( diff --git a/src/applications/nuance/editor/NuanceItemEditor.php b/src/applications/nuance/editor/NuanceItemEditor.php index 49382f5350..a331a65196 100644 --- a/src/applications/nuance/editor/NuanceItemEditor.php +++ b/src/applications/nuance/editor/NuanceItemEditor.php @@ -18,6 +18,7 @@ final class NuanceItemEditor $types[] = NuanceItemTransaction::TYPE_SOURCE; $types[] = NuanceItemTransaction::TYPE_REQUESTOR; $types[] = NuanceItemTransaction::TYPE_PROPERTY; + $types[] = NuanceItemTransaction::TYPE_QUEUE; $types[] = PhabricatorTransactions::TYPE_EDGE; $types[] = PhabricatorTransactions::TYPE_COMMENT; @@ -38,6 +39,8 @@ final class NuanceItemEditor return $object->getSourcePHID(); case NuanceItemTransaction::TYPE_OWNER: return $object->getOwnerPHID(); + case NuanceItemTransaction::TYPE_QUEUE: + return $object->getQueuePHID(); case NuanceItemTransaction::TYPE_PROPERTY: $key = $xaction->getMetadataValue( NuanceItemTransaction::PROPERTY_KEY); @@ -56,6 +59,7 @@ final class NuanceItemEditor case NuanceItemTransaction::TYPE_SOURCE: case NuanceItemTransaction::TYPE_OWNER: case NuanceItemTransaction::TYPE_PROPERTY: + case NuanceItemTransaction::TYPE_QUEUE: return $xaction->getNewValue(); } @@ -76,6 +80,9 @@ final class NuanceItemEditor case NuanceItemTransaction::TYPE_OWNER: $object->setOwnerPHID($xaction->getNewValue()); break; + case NuanceItemTransaction::TYPE_QUEUE: + $object->setQueuePHID($xaction->getNewValue()); + break; case NuanceItemTransaction::TYPE_PROPERTY: $key = $xaction->getMetadataValue( NuanceItemTransaction::PROPERTY_KEY); @@ -93,6 +100,7 @@ final class NuanceItemEditor case NuanceItemTransaction::TYPE_SOURCE: case NuanceItemTransaction::TYPE_OWNER: case NuanceItemTransaction::TYPE_PROPERTY: + case NuanceItemTransaction::TYPE_QUEUE: return; } diff --git a/src/applications/nuance/editor/NuanceSourceEditor.php b/src/applications/nuance/editor/NuanceSourceEditor.php index 1e913b33a4..233b2ae163 100644 --- a/src/applications/nuance/editor/NuanceSourceEditor.php +++ b/src/applications/nuance/editor/NuanceSourceEditor.php @@ -15,6 +15,7 @@ final class NuanceSourceEditor $types = parent::getTransactionTypes(); $types[] = NuanceSourceTransaction::TYPE_NAME; + $types[] = NuanceSourceTransaction::TYPE_DEFAULT_QUEUE; $types[] = PhabricatorTransactions::TYPE_EDGE; $types[] = PhabricatorTransactions::TYPE_COMMENT; @@ -31,6 +32,8 @@ final class NuanceSourceEditor switch ($xaction->getTransactionType()) { case NuanceSourceTransaction::TYPE_NAME: return $object->getName(); + case NuanceSourceTransaction::TYPE_DEFAULT_QUEUE: + return $object->getDefaultQueuePHID(); } return parent::getCustomTransactionOldValue($object, $xaction); @@ -42,6 +45,7 @@ final class NuanceSourceEditor switch ($xaction->getTransactionType()) { case NuanceSourceTransaction::TYPE_NAME: + case NuanceSourceTransaction::TYPE_DEFAULT_QUEUE: return $xaction->getNewValue(); } @@ -56,6 +60,9 @@ final class NuanceSourceEditor case NuanceSourceTransaction::TYPE_NAME: $object->setName($xaction->getNewValue()); break; + case NuanceSourceTransaction::TYPE_DEFAULT_QUEUE: + $object->setDefaultQueuePHID($xaction->getNewValue()); + break; } } @@ -65,6 +72,7 @@ final class NuanceSourceEditor switch ($xaction->getTransactionType()) { case NuanceSourceTransaction::TYPE_NAME: + case NuanceSourceTransaction::TYPE_DEFAULT_QUEUE: return; } @@ -95,6 +103,19 @@ final class NuanceSourceEditor $errors[] = $error; } break; + case NuanceSourceTransaction::TYPE_DEFAULT_QUEUE: + foreach ($xactions as $xaction) { + if (!$xaction->getNewValue()) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('Sources must have a default queue.'), + $xaction); + $error->setIsMissingFieldError(true); + $errors[] = $error; + } + } + break; } return $errors; diff --git a/src/applications/nuance/source/NuanceSourceDefinition.php b/src/applications/nuance/source/NuanceSourceDefinition.php index 06d302f587..4b37e39845 100644 --- a/src/applications/nuance/source/NuanceSourceDefinition.php +++ b/src/applications/nuance/source/NuanceSourceDefinition.php @@ -169,25 +169,39 @@ abstract class NuanceSourceDefinition extends Phobject { $form = $this->augmentEditForm($form, $ex); + $default_phid = $source->getDefaultQueuePHID(); + if ($default_phid) { + $default_queues = array($default_phid); + } else { + $default_queues = array(); + } + $form + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setLabel(pht('Default Queue')) + ->setName('defaultQueuePHIDs') + ->setLimit(1) + ->setDatasource(new NuanceQueueDatasource()) + ->setValue($default_queues)) ->appendChild( id(new AphrontFormPolicyControl()) - ->setUser($user) - ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) - ->setPolicyObject($source) - ->setPolicies($policies) - ->setName('viewPolicy')) + ->setUser($user) + ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) + ->setPolicyObject($source) + ->setPolicies($policies) + ->setName('viewPolicy')) ->appendChild( id(new AphrontFormPolicyControl()) - ->setUser($user) - ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) - ->setPolicyObject($source) - ->setPolicies($policies) - ->setName('editPolicy')) + ->setUser($user) + ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) + ->setPolicyObject($source) + ->setPolicies($policies) + ->setName('editPolicy')) ->appendChild( id(new AphrontFormSubmitControl()) - ->addCancelButton($source->getURI()) - ->setValue(pht('Save'))); + ->addCancelButton($source->getURI()) + ->setValue(pht('Save'))); return $form; } @@ -213,13 +227,19 @@ abstract class NuanceSourceDefinition extends Phobject { $transactions[] = id(new NuanceSourceTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY) ->setNewValue($request->getStr('editPolicy')); + $transactions[] = id(new NuanceSourceTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY) ->setNewValue($request->getStr('viewPolicy')); - $transactions[] = id(new NuanceSourceTransaction()) + + $transactions[] = id(new NuanceSourceTransaction()) ->setTransactionType(NuanceSourceTransaction::TYPE_NAME) ->setNewvalue($request->getStr('name')); + $transactions[] = id(new NuanceSourceTransaction()) + ->setTransactionType(NuanceSourceTransaction::TYPE_DEFAULT_QUEUE) + ->setNewvalue(head($request->getArr('defaultQueuePHIDs'))); + return $transactions; } @@ -251,6 +271,12 @@ abstract class NuanceSourceDefinition extends Phobject { ->setTransactionType(NuanceItemTransaction::TYPE_REQUESTOR) ->setNewValue($requestor->getPHID()); + // TODO: Eventually, apply real routing rules. For now, just put everything + // in the default queue for the source. + $xactions[] = id(new NuanceItemTransaction()) + ->setTransactionType(NuanceItemTransaction::TYPE_QUEUE) + ->setNewValue($source->getDefaultQueuePHID()); + foreach ($properties as $key => $property) { $xactions[] = id(new NuanceItemTransaction()) ->setTransactionType(NuanceItemTransaction::TYPE_PROPERTY) diff --git a/src/applications/nuance/storage/NuanceItem.php b/src/applications/nuance/storage/NuanceItem.php index 2335cd4d42..7ce517333f 100644 --- a/src/applications/nuance/storage/NuanceItem.php +++ b/src/applications/nuance/storage/NuanceItem.php @@ -17,13 +17,12 @@ final class NuanceItem protected $sourceLabel; protected $data = array(); protected $mailKey; - protected $dateNuanced; + protected $queuePHID; private $source = self::ATTACHABLE; public static function initializeNewItem() { return id(new NuanceItem()) - ->setDateNuanced(time()) ->setStatus(self::STATUS_OPEN); } @@ -38,17 +37,19 @@ final class NuanceItem 'sourceLabel' => 'text255?', 'status' => 'uint32', 'mailKey' => 'bytes20', - 'dateNuanced' => 'epoch', ), self::CONFIG_KEY_SCHEMA => array( 'key_source' => array( - 'columns' => array('sourcePHID', 'status', 'dateNuanced', 'id'), + 'columns' => array('sourcePHID', 'status'), ), 'key_owner' => array( - 'columns' => array('ownerPHID', 'status', 'dateNuanced', 'id'), + 'columns' => array('ownerPHID', 'status'), ), - 'key_contacter' => array( - 'columns' => array('requestorPHID', 'status', 'dateNuanced', 'id'), + 'key_requestor' => array( + 'columns' => array('requestorPHID', 'status'), + ), + 'key_queue' => array( + 'columns' => array('queuePHID', 'status'), ), ), ) + parent::getConfiguration(); @@ -144,7 +145,6 @@ final class NuanceItem 'sourceLabel' => $this->getSourceLabel(), 'dateCreated' => $this->getDateCreated(), 'dateModified' => $this->getDateModified(), - 'dateNuanced' => $this->getDateNuanced(), ); } diff --git a/src/applications/nuance/storage/NuanceItemTransaction.php b/src/applications/nuance/storage/NuanceItemTransaction.php index e6fac7cde7..183596402f 100644 --- a/src/applications/nuance/storage/NuanceItemTransaction.php +++ b/src/applications/nuance/storage/NuanceItemTransaction.php @@ -9,6 +9,7 @@ final class NuanceItemTransaction const TYPE_REQUESTOR = 'nuance.item.requestor'; const TYPE_SOURCE = 'nuance.item.source'; const TYPE_PROPERTY = 'nuance.item.property'; + const TYPE_QUEUE = 'nuance.item.queue'; public function getApplicationTransactionType() { return NuanceItemPHIDType::TYPECONST; @@ -18,4 +19,55 @@ final class NuanceItemTransaction return new NuanceItemTransactionComment(); } + public function shouldHide() { + $old = $this->getOldValue(); + $type = $this->getTransactionType(); + + switch ($type) { + case self::TYPE_REQUESTOR: + case self::TYPE_SOURCE: + return ($old === null); + } + + return parent::shouldHide(); + } + + public function getRequiredHandlePHIDs() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $type = $this->getTransactionType(); + + $phids = parent::getRequiredHandlePHIDs(); + switch ($type) { + case self::TYPE_QUEUE: + if ($old) { + $phids[] = $old; + } + if ($new) { + $phids[] = $new; + } + break; + } + + return $phids; + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $type = $this->getTransactionType(); + + $author_phid = $this->getAuthorPHID(); + + switch ($type) { + case self::TYPE_QUEUE: + return pht( + '%s routed this item to the %s queue.', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($new)); + } + + return parent::getTitle(); + } + } diff --git a/src/applications/nuance/storage/NuanceQueueItem.php b/src/applications/nuance/storage/NuanceQueueItem.php deleted file mode 100644 index 77a80ff181..0000000000 --- a/src/applications/nuance/storage/NuanceQueueItem.php +++ /dev/null @@ -1,34 +0,0 @@ - array( - 'itemStatus' => 'uint32', - 'itemDateNuanced' => 'epoch', - ), - self::CONFIG_KEY_SCHEMA => array( - 'key_one_per_queue' => array( - 'columns' => array('itemPHID', 'queuePHID'), - 'unique' => true, - ), - 'key_queue' => array( - 'columns' => array( - 'queuePHID', - 'itemStatus', - 'itemDateNuanced', - 'id', - ), - ), - ), - ) + parent::getConfiguration(); - } - -} diff --git a/src/applications/nuance/storage/NuanceQueueTransaction.php b/src/applications/nuance/storage/NuanceQueueTransaction.php index 46ace23cd1..c1630cdbd8 100644 --- a/src/applications/nuance/storage/NuanceQueueTransaction.php +++ b/src/applications/nuance/storage/NuanceQueueTransaction.php @@ -12,4 +12,34 @@ final class NuanceQueueTransaction extends NuanceTransaction { return new NuanceQueueTransactionComment(); } + public function shouldHide() { + $old = $this->getOldValue(); + $type = $this->getTransactionType(); + + switch ($type) { + case self::TYPE_NAME: + return ($old === null); + } + + return parent::shouldHide(); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $type = $this->getTransactionType(); + + $author_phid = $this->getAuthorPHID(); + + switch ($type) { + case self::TYPE_NAME: + return pht( + '%s renamed this queue from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); + } + + return parent::getTitle(); + } } diff --git a/src/applications/nuance/storage/NuanceSource.php b/src/applications/nuance/storage/NuanceSource.php index 6ac04ba33a..4ce61289f9 100644 --- a/src/applications/nuance/storage/NuanceSource.php +++ b/src/applications/nuance/storage/NuanceSource.php @@ -11,6 +11,7 @@ final class NuanceSource extends NuanceDAO protected $mailKey; protected $viewPolicy; protected $editPolicy; + protected $defaultQueuePHID; private $definition; diff --git a/src/applications/nuance/storage/NuanceSourceTransaction.php b/src/applications/nuance/storage/NuanceSourceTransaction.php index 86f52104d7..0b18c81184 100644 --- a/src/applications/nuance/storage/NuanceSourceTransaction.php +++ b/src/applications/nuance/storage/NuanceSourceTransaction.php @@ -3,7 +3,8 @@ final class NuanceSourceTransaction extends NuanceTransaction { - const TYPE_NAME = 'name-source'; + const TYPE_NAME = 'source.name'; + const TYPE_DEFAULT_QUEUE = 'source.queue.default'; public function getApplicationTransactionType() { return NuanceSourcePHIDType::TYPECONST; @@ -13,27 +14,63 @@ final class NuanceSourceTransaction return new NuanceSourceTransactionComment(); } - public function getTitle() { + public function shouldHide() { $old = $this->getOldValue(); $new = $this->getNewValue(); - $author_phid = $this->getAuthorPHID(); + $type = $this->getTransactionType(); - switch ($this->getTransactionType()) { + switch ($type) { + case self::TYPE_DEFAULT_QUEUE: + return !$old; case self::TYPE_NAME: - if ($old === null) { - return pht( - '%s created this source.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s renamed this source from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $old, - $new); + return ($old === null); + } + + return parent::shouldHide(); + } + + public function getRequiredHandlePHIDs() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $type = $this->getTransactionType(); + + $phids = parent::getRequiredHandlePHIDs(); + switch ($type) { + case self::TYPE_DEFAULT_QUEUE: + if ($old) { + $phids[] = $old; + } + if ($new) { + $phids[] = $new; } break; } + return $phids; + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + $type = $this->getTransactionType(); + $author_phid = $this->getAuthorPHID(); + + switch ($type) { + case self::TYPE_DEFAULT_QUEUE: + return pht( + '%s changed the default queue from %s to %s.', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($old), + $this->renderHandleLink($new)); + case self::TYPE_NAME: + return pht( + '%s renamed this source from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); + } + + return parent::getTitle(); } } diff --git a/src/applications/nuance/typeahead/NuanceQueueDatasource.php b/src/applications/nuance/typeahead/NuanceQueueDatasource.php new file mode 100644 index 0000000000..15b01fcecd --- /dev/null +++ b/src/applications/nuance/typeahead/NuanceQueueDatasource.php @@ -0,0 +1,38 @@ +getViewer(); + $raw_query = $this->getRawQuery(); + + $results = array(); + + // TODO: Make this use real typeahead logic. + $query = new NuanceQueueQuery(); + $queues = $this->executeQuery($query); + + foreach ($queues as $queue) { + $results[] = id(new PhabricatorTypeaheadResult()) + ->setName($queue->getName()) + ->setURI('/nuance/queue/'.$queue->getID().'/') + ->setPHID($queue->getPHID()); + } + + return $this->filterResultsAgainstTokens($results); + } + +} From 779a612e417261686be63f65d099a42f5bfcc0c3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 24 Aug 2015 09:37:48 -0700 Subject: [PATCH 12/18] Fix mail parameter error with old migrations Summary: Fixes T9251. Old mail could get saved with bad parameters for two reasons that I can come up with: - Nothing ever set a parameter on it -- not sure this could ever actually happen; or - some field contained non-UTF8 data prior to D13939 and we silently failed to encode it. My guess is that the second case is probably the culprit here. In any case, recover from this so `20150622.metamta.5.actor-phid-mig.php` can proceed. Test Plan: Same effective patch as user patch in T9251; looked at some mail to make sure it was still pulling parameters properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9251 Differential Revision: https://secure.phabricator.com/D13990 --- .../metamta/storage/PhabricatorMetaMTAMail.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index abcbb7a082..c4793ed918 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -10,7 +10,7 @@ final class PhabricatorMetaMTAMail const RETRY_DELAY = 5; protected $actorPHID; - protected $parameters; + protected $parameters = array(); protected $status; protected $message; protected $relatedPHID; @@ -69,6 +69,13 @@ final class PhabricatorMetaMTAMail } protected function getParam($param, $default = null) { + // Some old mail was saved without parameters because no parameters were + // set or encoding failed. Recover in these cases so we can perform + // mail migrations, see T9251. + if (!is_array($this->parameters)) { + $this->parameters = array(); + } + return idx($this->parameters, $param, $default); } From 10966519e2ebf9e12322b3d73d3a16305e5efb47 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Aug 2015 09:35:23 -0700 Subject: [PATCH 13/18] Prevent "commit message magic words" parser from exploding on "reverts aaaa.....aaz" Summary: Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up. However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches. That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of: aaa aa a a aa a a a All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings. Instead, require a word boundary or EOL after the match, so this is the only valid match: aaa Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations. Test Plan: Added a failing unit test, applied patch, clean test. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9268 Differential Revision: https://secure.phabricator.com/D13997 --- .../DifferentialCustomFieldRevertsParserTestCase.php | 3 +++ .../parser/PhabricatorCustomFieldMonogramParser.php | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/parser/__tests__/DifferentialCustomFieldRevertsParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialCustomFieldRevertsParserTestCase.php index ff8d1c1bf7..0061273ac7 100644 --- a/src/applications/differential/parser/__tests__/DifferentialCustomFieldRevertsParserTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialCustomFieldRevertsParserTestCase.php @@ -76,6 +76,9 @@ final class DifferentialCustomFieldRevertsParserTestCase ), ), + // This tests a degenerate regex behavior, see T9268. + 'Reverts aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaz' => array(), + "This doesn't revert anything" => array(), 'nonrevert of r11' => array(), 'fixed a bug' => array(), diff --git a/src/infrastructure/customfield/parser/PhabricatorCustomFieldMonogramParser.php b/src/infrastructure/customfield/parser/PhabricatorCustomFieldMonogramParser.php index 48e922dc62..9c6f33a855 100644 --- a/src/infrastructure/customfield/parser/PhabricatorCustomFieldMonogramParser.php +++ b/src/infrastructure/customfield/parser/PhabricatorCustomFieldMonogramParser.php @@ -24,8 +24,8 @@ abstract class PhabricatorCustomFieldMonogramParser '(?:^|\b)'. $prefix_regex. $infix_regex. - '((?:'.$monogram_pattern.'[,\s]*)+)'. - '(?:\band\s+('.$monogram_pattern.'))?'. + '((?:'.$monogram_pattern.'(?:\b|$)[,\s]*)+)'. + '(?:\band\s+('.$monogram_pattern.'(?:\b|$)))?'. $suffix_regex. '(?:$|\b)'. '/'; From 55b44f53f87f911d64268751df7d92b208206ba6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Aug 2015 17:59:47 -0700 Subject: [PATCH 14/18] Fix possible recursive embeds in Dashboard text panels Summary: We currently detect tab panels embedding themselves, but do not detect text panels embedding themselves with `{Wxx}`. Detect these self-embedding panels. I had to add a bit of a hack to pass the parent panel PHIDs to the rule. Generally, I got the Markup API kind of wrong and want to update it, I'll file a followup with details about how I'd like to move forward. Test Plan: Created a text panel embedding itself, a tab panel embedding a text panel embedding itself, a tab panel embedding a text panel embedding the tab panel, etc. Rendered all panels standalone and as `{Wxx}` from a different context. {F761158} {F761159} {F761160} {F761161} {F761162} Reviewers: chad, jbeta Reviewed By: chad, jbeta Differential Revision: https://secure.phabricator.com/D13999 --- .../PhabricatorDashboardTextPanelType.php | 22 +++++++++++++++---- .../PhabricatorDashboardRemarkupRule.php | 10 +++++++-- .../markup/PhabricatorMarkupEngine.php | 11 ++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/applications/dashboard/paneltype/PhabricatorDashboardTextPanelType.php b/src/applications/dashboard/paneltype/PhabricatorDashboardTextPanelType.php index e9716f0868..413d3dc856 100644 --- a/src/applications/dashboard/paneltype/PhabricatorDashboardTextPanelType.php +++ b/src/applications/dashboard/paneltype/PhabricatorDashboardTextPanelType.php @@ -37,11 +37,25 @@ final class PhabricatorDashboardTextPanelType PhabricatorDashboardPanelRenderingEngine $engine) { $text = $panel->getProperty('text', ''); + $oneoff = id(new PhabricatorMarkupOneOff())->setContent($text); + $field = 'default'; - $text_content = PhabricatorMarkupEngine::renderOneObject( - id(new PhabricatorMarkupOneOff())->setContent($text), - 'default', - $viewer); + // NOTE: We're taking extra steps here to prevent creation of a text panel + // which embeds itself using `{Wnnn}`, recursing indefinitely. + + $parent_key = PhabricatorDashboardRemarkupRule::KEY_PARENT_PANEL_PHIDS; + $parent_phids = $engine->getParentPanelPHIDs(); + $parent_phids[] = $panel->getPHID(); + + $markup_engine = id(new PhabricatorMarkupEngine()) + ->setViewer($viewer) + ->setContextObject($panel) + ->setAuxiliaryConfig($parent_key, $parent_phids); + + $text_content = $markup_engine + ->addObject($oneoff, $field) + ->process() + ->getOutput($oneoff, $field); return id(new PHUIPropertyListView()) ->addTextContent($text_content); diff --git a/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php b/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php index 9870795f25..6b0a48dc01 100644 --- a/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php +++ b/src/applications/dashboard/remarkup/PhabricatorDashboardRemarkupRule.php @@ -3,6 +3,8 @@ final class PhabricatorDashboardRemarkupRule extends PhabricatorObjectRemarkupRule { + const KEY_PARENT_PANEL_PHIDS = 'dashboard.parentPanelPHIDs'; + protected function getObjectNamePrefix() { return 'W'; } @@ -21,12 +23,16 @@ final class PhabricatorDashboardRemarkupRule PhabricatorObjectHandle $handle, $options) { - $viewer = $this->getEngine()->getConfig('viewer'); + $engine = $this->getEngine(); + $viewer = $engine->getConfig('viewer'); + + $parent_key = self::KEY_PARENT_PANEL_PHIDS; + $parent_phids = $engine->getConfig($parent_key, array()); return id(new PhabricatorDashboardPanelRenderingEngine()) ->setViewer($viewer) ->setPanel($object) - ->setParentPanelPHIDs(array()) + ->setParentPanelPHIDs($parent_phids) ->renderPanel(); } diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index 933a21b163..c6babe99e0 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -44,6 +44,7 @@ final class PhabricatorMarkupEngine extends Phobject { private $contextObject; private $version = 15; private $engineCaches = array(); + private $auxiliaryConfig = array(); /* -( Markup Pipeline )---------------------------------------------------- */ @@ -122,6 +123,10 @@ final class PhabricatorMarkupEngine extends Phobject { $engines[$key] = $info['object']->newMarkupEngine($info['field']); $engines[$key]->setConfig('viewer', $this->viewer); $engines[$key]->setConfig('contextObject', $this->contextObject); + + foreach ($this->auxiliaryConfig as $aux_key => $aux_value) { + $engines[$key]->setConfig($aux_key, $aux_value); + } } // Load or build the preprocessor caches. @@ -310,6 +315,12 @@ final class PhabricatorMarkupEngine extends Phobject { return $this; } + public function setAuxiliaryConfig($key, $value) { + // TODO: This is gross and should be removed. Avoid use. + $this->auxiliaryConfig[$key] = $value; + return $this; + } + /* -( Engine Construction )------------------------------------------------ */ From 960a574dd538363da2bcecd06f8f13e45c170428 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Thu, 27 Aug 2015 04:15:40 -0700 Subject: [PATCH 15/18] lose help cursor on blur Summary: Fixes T8501. When losing focus while holding ctrl, we never get a key-up event; ctrl-f/d/tab make the browser tab lose focus. So treat 'blur' (unfocus) as if the user released ctrl. Test Plan: ctrl-f/ctrl-d/ctrl-tab, ctrl-click-outside-of-window, and move mouse over the content - see no help suggestions. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Maniphest Tasks: T8501 Differential Revision: https://secure.phabricator.com/D13260 --- resources/celerity/map.php | 18 +++++----- .../repository/repository-crossreference.js | 34 ++++++++++++++----- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 446ce87a89..8008377f6e 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'core.pkg.js' => 'a590b451', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', - 'differential.pkg.js' => 'ebef29b1', + 'differential.pkg.js' => 'fd95c208', 'diffusion.pkg.css' => '385e85b3', 'diffusion.pkg.js' => '0115b37c', 'maniphest.pkg.css' => '4845691a', @@ -407,7 +407,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => 'b2b4fbaf', 'rsrc/js/application/releeph/releeph-request-state-change.js' => 'a0b57eb8', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'de2e896f', - 'rsrc/js/application/repository/repository-crossreference.js' => 'bea81850', + 'rsrc/js/application/repository/repository-crossreference.js' => 'e9eeb416', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => 'dbbf48b6', @@ -643,7 +643,7 @@ return array( 'javelin-behavior-remarkup-preview' => 'f7379f45', 'javelin-behavior-reorder-applications' => '76b9fc3e', 'javelin-behavior-reorder-columns' => 'e1d25dfb', - 'javelin-behavior-repository-crossreference' => 'bea81850', + 'javelin-behavior-repository-crossreference' => 'e9eeb416', 'javelin-behavior-scrollbar' => '834a1173', 'javelin-behavior-search-reorder-queries' => 'e9581f08', 'javelin-behavior-select-on-click' => '4e3e79a6', @@ -1745,12 +1745,6 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), - 'bea81850' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), 'c1700f6f' => array( 'javelin-install', 'javelin-util', @@ -1913,6 +1907,12 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), + 'e9eeb416' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), 'ea681761' => array( 'javelin-behavior', 'javelin-aphlict', diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index 710d2e4e00..982d83c53d 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -40,8 +40,7 @@ JX.behavior('repository-crossreference', function(config, statics) { 'tag:span', function(e) { if (e.getType() === 'mouseout') { - highlighted && JX.DOM.alterClass(highlighted, classHighlight, false); - highlighted = null; + unhighlight(); return; } if (!isSignalkey(e)) { @@ -63,6 +62,10 @@ JX.behavior('repository-crossreference', function(config, statics) { } }); } + function unhighlight() { + highlighted && JX.DOM.alterClass(highlighted, classHighlight, false); + highlighted = null; + } function openSearch(target, lang) { var symbol = target.textContent || target.innerText; @@ -110,6 +113,7 @@ JX.behavior('repository-crossreference', function(config, statics) { linkAll(e.getData().container); }); + JX.Stratcom.listen( ['keydown', 'keyup'], null, @@ -117,14 +121,28 @@ JX.behavior('repository-crossreference', function(config, statics) { if (e.getRawEvent().keyCode !== signalKey) { return; } - statics.active = (e.getType() === 'keydown'); - linked.forEach(function(element) { - JX.DOM.alterClass(element, classMouseCursor, statics.active); - }); + setCursorMode(e.getType() === 'keydown'); if (!statics.active) { - highlighted && JX.DOM.alterClass(highlighted, classHighlight, false); - highlighted = null; + unhighlight(); } }); + + JX.Stratcom.listen( + 'blur', + null, + function(e) { + if (e.getTarget()) { + return; + } + unhighlight(); + setCursorMode(false); + }); + + function setCursorMode(active) { + statics.active = active; + linked.forEach(function(element) { + JX.DOM.alterClass(element, classMouseCursor, statics.active); + }); + } }); From 328d336c8bb366e70d8174573095cc07586aff8d Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Thu, 27 Aug 2015 04:17:33 -0700 Subject: [PATCH 16/18] Symbol Search: Allow ctrl-click with no hover Summary: Fix T8710. I had hopes of doing something cleaver with `highlighted` (Like trying to understand `foo.bar` when clicking `bar`, but I obviously didn't do it. Test Plan: ctrl-click. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: joshuaspence, epriestley, gena2x, Korvin Maniphest Tasks: T8710 Differential Revision: https://secure.phabricator.com/D13550 --- resources/celerity/map.php | 18 +++++++++--------- .../repository/repository-crossreference.js | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8008377f6e..562aef2bd7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'core.pkg.js' => 'a590b451', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', - 'differential.pkg.js' => 'fd95c208', + 'differential.pkg.js' => '813c1633', 'diffusion.pkg.css' => '385e85b3', 'diffusion.pkg.js' => '0115b37c', 'maniphest.pkg.css' => '4845691a', @@ -407,7 +407,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => 'b2b4fbaf', 'rsrc/js/application/releeph/releeph-request-state-change.js' => 'a0b57eb8', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'de2e896f', - 'rsrc/js/application/repository/repository-crossreference.js' => 'e9eeb416', + 'rsrc/js/application/repository/repository-crossreference.js' => 'e5339c43', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => 'dbbf48b6', @@ -643,7 +643,7 @@ return array( 'javelin-behavior-remarkup-preview' => 'f7379f45', 'javelin-behavior-reorder-applications' => '76b9fc3e', 'javelin-behavior-reorder-columns' => 'e1d25dfb', - 'javelin-behavior-repository-crossreference' => 'e9eeb416', + 'javelin-behavior-repository-crossreference' => 'e5339c43', 'javelin-behavior-scrollbar' => '834a1173', 'javelin-behavior-search-reorder-queries' => 'e9581f08', 'javelin-behavior-select-on-click' => '4e3e79a6', @@ -1890,6 +1890,12 @@ return array( 'javelin-behavior', 'javelin-dom', ), + 'e5339c43' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), 'e5822781' => array( 'javelin-behavior', 'javelin-dom', @@ -1907,12 +1913,6 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), - 'e9eeb416' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), 'ea681761' => array( 'javelin-behavior', 'javelin-aphlict', diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index 982d83c53d..bfb849c759 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -58,7 +58,7 @@ JX.behavior('repository-crossreference', function(config, statics) { target = target.parentNode; } } else if (e.getType() === 'click') { - openSearch(highlighted, lang); + openSearch(e.getTarget(), lang); } }); } From 786b135a662c41ebba44f1eb3169f98771fd49c4 Mon Sep 17 00:00:00 2001 From: cburroughs Date: Thu, 27 Aug 2015 04:38:10 -0700 Subject: [PATCH 17/18] variable days back for `bin/mail volume` Summary: Email is so exciting I can't wait 30 days for initial results. ref T9161 Test Plan: * `./bin/mail volume --days 60` took longer and gave plausibly larger results. * `./bin/mail volume --days 0` quickly told me no mail had been sent. * `./bin/mail volume` Said it was still looking 30 days back. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Maniphest Tasks: T9161 Differential Revision: https://secure.phabricator.com/D13901 --- ...habricatorMailManagementVolumeWorkflow.php | 24 ++++++++++++++++--- .../PhabricatorUSEnglishTranslation.php | 5 ++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/applications/metamta/management/PhabricatorMailManagementVolumeWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementVolumeWorkflow.php index dcc548e0f5..f0b0254f84 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementVolumeWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementVolumeWorkflow.php @@ -7,11 +7,18 @@ final class PhabricatorMailManagementVolumeWorkflow $this ->setName('volume') ->setSynopsis( - pht('Show how much mail users have received in the last 30 days.')) + pht('Show how much mail users have received recently.')) ->setExamples( '**volume**') ->setArguments( array( + array( + 'name' => 'days', + 'param' => 'days', + 'default' => 30, + 'help' => pht( + 'Number of days back (default 30).'), + ), )); } @@ -19,7 +26,16 @@ final class PhabricatorMailManagementVolumeWorkflow $console = PhutilConsole::getConsole(); $viewer = $this->getViewer(); - $since = (PhabricatorTime::getNow() - phutil_units('30 days in seconds')); + $days = (int)$args->getArg('days'); + if ($days < 1) { + throw new PhutilArgumentUsageException( + pht( + 'Period specified with --days must be at least 1.')); + } + + $duration = phutil_units("{$days} days in seconds"); + + $since = (PhabricatorTime::getNow() - $duration); $until = PhabricatorTime::getNow(); $mails = id(new PhabricatorMetaMTAMailQuery()) @@ -95,7 +111,9 @@ final class PhabricatorMailManagementVolumeWorkflow $table->draw(); echo "\n"; - echo pht('Mail sent in the last 30 days.')."\n"; + echo pht( + 'Mail sent in the last %s day(s).', + new PhutilNumber($days))."\n"; echo pht( '"Unfiltered" is raw volume before rules applied.')."\n"; echo pht( diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 9ac0d0b591..d9397d80c8 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -1369,6 +1369,11 @@ final class PhabricatorUSEnglishTranslation 'This action has no effect on targets: %2$s.', ), + 'Mail sent in the last %s day(s).' => array( + 'Mail sent in the last day.', + 'Mail sent in the last %s days.', + ), + ); } From fd189f14fb2621dd69c0c2ce96813edc39bf9768 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 28 Aug 2015 15:05:05 -0700 Subject: [PATCH 18/18] Fix diviner symbols documentation Summary: Fixes T9267. Removes preceeding r. Test Plan: Ran Sample, did not get error. Reviewers: epriestley, joshuaspence Reviewed By: epriestley, joshuaspence Subscribers: Korvin Maniphest Tasks: T9267 Differential Revision: https://secure.phabricator.com/D14000 --- src/docs/user/userguide/diffusion_symbols.diviner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/user/userguide/diffusion_symbols.diviner b/src/docs/user/userguide/diffusion_symbols.diviner index b609841812..c58ca72e1d 100644 --- a/src/docs/user/userguide/diffusion_symbols.diviner +++ b/src/docs/user/userguide/diffusion_symbols.diviner @@ -65,7 +65,7 @@ write such a script, and run this command to see its output: To actually build the symbol index, pipe this data to the `import_repository_symbols.php` script, providing the repository callsign: - $ ./scripts/symbols/import_repository_symbols.php rREPO < symbols_data + $ ./scripts/symbols/import_repository_symbols.php REPO < symbols_data Then just set up a cronjob to run that however often you like.