Skip to content

Commit a246c85

Browse files
author
epriestley
committed
Use ApplicationTransactions and CustomField to implement build steps
Summary: Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits. This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future. All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types. Test Plan: Made a bunch of step edits. Here's an example: {F133694} Note that: - "Required" fields work correctly. - the transaction record is shown at the bottom of the page. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4602, T1049 Differential Revision: https://secure.phabricator.com/D8600
1 parent 72337de commit a246c85

24 files changed

+274
-318
lines changed

‎resources/sql/autopatches/20140321.harbor.1.bxaction.sql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_buildsteptransaction (
2+
id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT,
3+
phid VARCHAR(64) NOT NULL COLLATE utf8_bin,
4+
authorPHID VARCHAR(64) NOT NULL COLLATE utf8_bin,
5+
objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin,
6+
viewPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin,
7+
editPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin,
8+
commentPHID VARCHAR(64) COLLATE utf8_bin,
9+
commentVersion INT UNSIGNED NOT NULL,
10+
transactionType VARCHAR(32) NOT NULL COLLATE utf8_bin,
11+
oldValue LONGTEXT NOT NULL COLLATE utf8_bin,
12+
newValue LONGTEXT NOT NULL COLLATE utf8_bin,
13+
contentSource LONGTEXT NOT NULL COLLATE utf8_bin,
14+
metadata LONGTEXT NOT NULL COLLATE utf8_bin,
15+
dateCreated INT UNSIGNED NOT NULL,
16+
dateModified INT UNSIGNED NOT NULL,
17+
18+
UNIQUE KEY `key_phid` (phid),
19+
KEY `key_object` (objectPHID)
20+
21+
) ENGINE=InnoDB, COLLATE utf8_general_ci;

‎src/__phutil_library_map__.php

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,7 +712,12 @@
712712
'HarbormasterBuildPlanTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanTransactionQuery.php',
713713
'HarbormasterBuildQuery' => 'applications/harbormaster/query/HarbormasterBuildQuery.php',
714714
'HarbormasterBuildStep' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStep.php',
715+
'HarbormasterBuildStepCoreCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php',
716+
'HarbormasterBuildStepCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php',
717+
'HarbormasterBuildStepEditor' => 'applications/harbormaster/editor/HarbormasterBuildStepEditor.php',
715718
'HarbormasterBuildStepQuery' => 'applications/harbormaster/query/HarbormasterBuildStepQuery.php',
719+
'HarbormasterBuildStepTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php',
720+
'HarbormasterBuildStepTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php',
716721
'HarbormasterBuildTarget' => 'applications/harbormaster/storage/build/HarbormasterBuildTarget.php',
717722
'HarbormasterBuildTargetQuery' => 'applications/harbormaster/query/HarbormasterBuildTargetQuery.php',
718723
'HarbormasterBuildViewController' => 'applications/harbormaster/controller/HarbormasterBuildViewController.php',
@@ -3310,8 +3315,18 @@
33103315
array(
33113316
0 => 'HarbormasterDAO',
33123317
1 => 'PhabricatorPolicyInterface',
3318+
2 => 'PhabricatorCustomFieldInterface',
3319+
),
3320+
'HarbormasterBuildStepCoreCustomField' =>
3321+
array(
3322+
0 => 'HarbormasterBuildStepCustomField',
3323+
1 => 'PhabricatorStandardCustomFieldInterface',
33133324
),
3325+
'HarbormasterBuildStepCustomField' => 'PhabricatorCustomField',
3326+
'HarbormasterBuildStepEditor' => 'PhabricatorApplicationTransactionEditor',
33143327
'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
3328+
'HarbormasterBuildStepTransaction' => 'PhabricatorApplicationTransaction',
3329+
'HarbormasterBuildStepTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
33153330
'HarbormasterBuildTarget' =>
33163331
array(
33173332
0 => 'HarbormasterDAO',

‎src/applications/harbormaster/controller/HarbormasterStepEditController.php

Lines changed: 34 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -27,85 +27,39 @@ public function processRequest() {
2727
$plan = $step->getBuildPlan();
2828

2929
$implementation = $step->getStepImplementation();
30-
$implementation->validateSettingDefinitions();
31-
$settings = $implementation->getSettings();
30+
31+
$field_list = PhabricatorCustomField::getObjectFields(
32+
$step,
33+
PhabricatorCustomField::ROLE_EDIT);
34+
$field_list
35+
->setViewer($viewer)
36+
->readFieldsFromStorage($step);
3237

3338
$errors = array();
39+
$validation_exception = null;
3440
if ($request->isFormPost()) {
35-
foreach ($implementation->getSettingDefinitions() as $name => $opt) {
36-
$readable_name = $this->getReadableName($name, $opt);
37-
$value = $this->getValueFromRequest($request, $name, $opt['type']);
38-
39-
// TODO: This won't catch any validation issues unless the field
40-
// is missing completely. How should we check if the user is
41-
// required to enter an integer?
42-
if ($value === null) {
43-
$errors[] = $readable_name.' is not valid.';
44-
} else {
45-
$step->setDetail($name, $value);
46-
}
47-
}
41+
$xactions = $field_list->buildFieldTransactionsFromRequest(
42+
new HarbormasterBuildStepTransaction(),
43+
$request);
4844

49-
if (!$errors) {
50-
$step->save();
45+
$editor = id(new HarbormasterBuildStepEditor())
46+
->setActor($viewer)
47+
->setContinueOnNoEffect(true)
48+
->setContentSourceFromRequest($request);
49+
50+
try {
51+
$editor->applyTransactions($step, $xactions);
5152
return id(new AphrontRedirectResponse())
5253
->setURI($this->getApplicationURI('plan/'.$plan->getID().'/'));
54+
} catch (PhabricatorApplicationTransactionValidationException $ex) {
55+
$validation_exception = $ex;
5356
}
5457
}
5558

5659
$form = id(new AphrontFormView())
5760
->setUser($viewer);
5861

59-
// We need to render out all of the fields for the settings that
60-
// the implementation has.
61-
foreach ($implementation->getSettingDefinitions() as $name => $opt) {
62-
if ($request->isFormPost()) {
63-
$value = $this->getValueFromRequest($request, $name, $opt['type']);
64-
} else {
65-
$value = $settings[$name];
66-
}
67-
68-
switch ($opt['type']) {
69-
case BuildStepImplementation::SETTING_TYPE_STRING:
70-
case BuildStepImplementation::SETTING_TYPE_INTEGER:
71-
$control = id(new AphrontFormTextControl())
72-
->setLabel($this->getReadableName($name, $opt))
73-
->setName($name)
74-
->setValue($value);
75-
break;
76-
case BuildStepImplementation::SETTING_TYPE_BOOLEAN:
77-
$control = id(new AphrontFormCheckboxControl())
78-
->setLabel($this->getReadableName($name, $opt))
79-
->setName($name)
80-
->setValue($value);
81-
break;
82-
case BuildStepImplementation::SETTING_TYPE_ARTIFACT:
83-
$filter = $opt['artifact_type'];
84-
$available_artifacts =
85-
BuildStepImplementation::loadAvailableArtifacts(
86-
$plan,
87-
$step,
88-
$filter);
89-
$options = array();
90-
foreach ($available_artifacts as $key => $type) {
91-
$options[$key] = $key;
92-
}
93-
$control = id(new AphrontFormSelectControl())
94-
->setLabel($this->getReadableName($name, $opt))
95-
->setName($name)
96-
->setValue($value)
97-
->setOptions($options);
98-
break;
99-
default:
100-
throw new Exception("Unable to render field with unknown type.");
101-
}
102-
103-
if (isset($opt['description'])) {
104-
$control->setCaption($opt['description']);
105-
}
106-
107-
$form->appendChild($control);
108-
}
62+
$field_list->appendFieldsToForm($form);
10963

11064
$form->appendChild(
11165
id(new AphrontFormSubmitControl())
@@ -115,7 +69,7 @@ public function processRequest() {
11569

11670
$box = id(new PHUIObjectBoxView())
11771
->setHeaderText('Edit Step: '.$implementation->getName())
118-
->setValidationException(null)
72+
->setValidationException($validation_exception)
11973
->setForm($form);
12074

12175
$crumbs = $this->buildApplicationCrumbs();
@@ -127,43 +81,30 @@ public function processRequest() {
12781

12882
$variables = $this->renderBuildVariablesTable();
12983

84+
$xactions = id(new HarbormasterBuildStepTransactionQuery())
85+
->setViewer($viewer)
86+
->withObjectPHIDs(array($step->getPHID()))
87+
->execute();
88+
89+
$xaction_view = id(new PhabricatorApplicationTransactionView())
90+
->setUser($viewer)
91+
->setObjectPHID($step->getPHID())
92+
->setTransactions($xactions)
93+
->setShouldTerminate(true);
94+
13095
return $this->buildApplicationPage(
13196
array(
13297
$crumbs,
13398
$box,
13499
$variables,
100+
$xaction_view,
135101
),
136102
array(
137103
'title' => $implementation->getName(),
138104
'device' => true,
139105
));
140106
}
141107

142-
public function getReadableName($name, $opt) {
143-
$readable_name = $name;
144-
if (isset($opt['name'])) {
145-
$readable_name = $opt['name'];
146-
}
147-
return $readable_name;
148-
}
149-
150-
public function getValueFromRequest(AphrontRequest $request, $name, $type) {
151-
switch ($type) {
152-
case BuildStepImplementation::SETTING_TYPE_STRING:
153-
case BuildStepImplementation::SETTING_TYPE_ARTIFACT:
154-
return $request->getStr($name);
155-
break;
156-
case BuildStepImplementation::SETTING_TYPE_INTEGER:
157-
return $request->getInt($name);
158-
break;
159-
case BuildStepImplementation::SETTING_TYPE_BOOLEAN:
160-
return $request->getBool($name);
161-
break;
162-
default:
163-
throw new Exception("Unsupported setting type '".$type."'.");
164-
}
165-
}
166-
167108
private function renderBuildVariablesTable() {
168109
$viewer = $this->getRequest()->getUser();
169110

‎src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?php
2+
3+
final class HarbormasterBuildStepCoreCustomField
4+
extends HarbormasterBuildStepCustomField
5+
implements PhabricatorStandardCustomFieldInterface {
6+
7+
public function getStandardCustomFieldNamespace() {
8+
return 'harbormaster:core';
9+
}
10+
11+
public function createFields($object) {
12+
$specs = $object->getStepImplementation()->getFieldSpecifications();
13+
return PhabricatorStandardCustomField::buildStandardFields($this, $specs);
14+
}
15+
16+
public function shouldUseStorage() {
17+
return false;
18+
}
19+
20+
public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
21+
$key = $this->getProxy()->getRawStandardFieldKey();
22+
$this->setValueFromStorage($object->getDetail($key));
23+
}
24+
25+
public function applyApplicationTransactionInternalEffects(
26+
PhabricatorApplicationTransaction $xaction) {
27+
$object = $this->getObject();
28+
$key = $this->getProxy()->getRawStandardFieldKey();
29+
30+
$this->setValueFromApplicationTransactions($xaction->getNewValue());
31+
$value = $this->getValueForStorage();
32+
33+
$object->setDetail($key, $value);
34+
}
35+
36+
public function applyApplicationTransactionExternalEffects(
37+
PhabricatorApplicationTransaction $xaction) {
38+
return;
39+
}
40+
41+
}

‎src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
3+
abstract class HarbormasterBuildStepCustomField
4+
extends PhabricatorCustomField {
5+
6+
}

‎src/applications/harbormaster/editor/HarbormasterBuildStepEditor.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
3+
final class HarbormasterBuildStepEditor
4+
extends PhabricatorApplicationTransactionEditor {
5+
6+
}

‎src/applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
final class HarbormasterBuildStepTransactionQuery
4+
extends PhabricatorApplicationTransactionQuery {
5+
6+
public function getTemplateApplicationTransaction() {
7+
return new HarbormasterBuildStepTransaction();
8+
}
9+
10+
}

‎src/applications/harbormaster/step/BuildStepImplementation.php

Lines changed: 8 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,9 @@
22

33
abstract class BuildStepImplementation {
44

5-
private $settings;
6-
7-
const SETTING_TYPE_STRING = 'string';
8-
const SETTING_TYPE_INTEGER = 'integer';
9-
const SETTING_TYPE_BOOLEAN = 'boolean';
10-
const SETTING_TYPE_ARTIFACT = 'artifact';
11-
125
public static function getImplementations() {
136
$symbols = id(new PhutilSymbolLoader())
14-
->setAncestorClass("BuildStepImplementation")
7+
->setAncestorClass('BuildStepImplementation')
158
->setConcreteOnly(true)
169
->selectAndLoadSymbols();
1710
return ipull($symbols, 'name');
@@ -33,7 +26,7 @@ public function getGenericDescription() {
3326
* The description of the implementation, based on the current settings.
3427
*/
3528
public function getDescription() {
36-
return '';
29+
return $this->getGenericDescription();
3730
}
3831

3932
/**
@@ -54,44 +47,13 @@ public function getSetting($key, $default = null) {
5447
return idx($this->settings, $key, $default);
5548
}
5649

57-
/**
58-
* Validate the current settings of this build step.
59-
*/
60-
public function validateSettings() {
61-
return true;
62-
}
63-
6450
/**
6551
* Loads the settings for this build step implementation from a build
6652
* step or target.
6753
*/
6854
public final function loadSettings($build_object) {
69-
$this->settings = array();
70-
$this->validateSettingDefinitions();
71-
foreach ($this->getSettingDefinitions() as $name => $opt) {
72-
$this->settings[$name] = $build_object->getDetail($name);
73-
}
74-
return $this->settings;
75-
}
76-
77-
/**
78-
* Validates that the setting definitions for this implementation are valid.
79-
*/
80-
public final function validateSettingDefinitions() {
81-
foreach ($this->getSettingDefinitions() as $name => $opt) {
82-
if (!isset($opt['type'])) {
83-
throw new Exception(
84-
'Setting definition \''.$name.
85-
'\' is missing type requirement.');
86-
}
87-
}
88-
}
89-
90-
/**
91-
* Return an array of settings for this step implementation.
92-
*/
93-
public function getSettingDefinitions() {
94-
return array();
55+
$this->settings = $build_object->getDetails();
56+
return $this;
9557
}
9658

9759
/**
@@ -197,4 +159,8 @@ protected function mergeVariables($function, $pattern, array $variables) {
197159
return call_user_func($function, $pattern, $argv);
198160
}
199161

162+
public function getFieldSpecifications() {
163+
return array();
164+
}
165+
200166
}

0 commit comments

Comments
 (0)