1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-20 13:52:40 +01:00

Remove PhabricatorProjectEditor

Summary:
Ref T4379. Perform all editing with modern transaction infrastructure. A few practical changes here:

  - Message for "project name required" should be a little nicer. I'll deal with this once more stuff gets straightened out. You get a reasonable message now, it's just not nicely handled as part of the form.
  - Message for "project name is not unique" should be a little nicer. Same as above.
  - Previously, we would automatically archive a project when the last member left or was removed. I'll probably restore this in a bit but am omitting it for the moment for simplicity.
  - Previously, we would create projects with goofy nonsensical permissions. Now we create them with reasonable permissions.

Test Plan:
  - Created project.
  - Edited project.
  - Ran unit tests.
  - Viewed project edit history.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8168
This commit is contained in:
epriestley 2014-02-10 14:30:17 -08:00
parent 8544d0d00f
commit a035d3d528
9 changed files with 155 additions and 319 deletions

View file

@ -1836,7 +1836,6 @@ phutil_register_library_map(array(
'PhabricatorProjectCustomFieldStorage' => 'applications/project/storage/PhabricatorProjectCustomFieldStorage.php',
'PhabricatorProjectCustomFieldStringIndex' => 'applications/project/storage/PhabricatorProjectCustomFieldStringIndex.php',
'PhabricatorProjectDAO' => 'applications/project/storage/PhabricatorProjectDAO.php',
'PhabricatorProjectEditor' => 'applications/project/editor/PhabricatorProjectEditor.php',
'PhabricatorProjectEditorTestCase' => 'applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php',
'PhabricatorProjectHistoryController' => 'applications/project/controller/PhabricatorProjectHistoryController.php',
'PhabricatorProjectListController' => 'applications/project/controller/PhabricatorProjectListController.php',
@ -4572,7 +4571,6 @@ phutil_register_library_map(array(
'PhabricatorProjectCustomFieldStorage' => 'PhabricatorCustomFieldStorage',
'PhabricatorProjectCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage',
'PhabricatorProjectDAO' => 'PhabricatorLiskDAO',
'PhabricatorProjectEditor' => 'PhabricatorEditor',
'PhabricatorProjectEditorTestCase' => 'PhabricatorTestCase',
'PhabricatorProjectHistoryController' => 'PhabricatorProjectController',
'PhabricatorProjectListController' =>

View file

@ -12,37 +12,33 @@ final class PhabricatorProjectCreateController
$this->requireApplicationCapability(
ProjectCapabilityCreateProjects::CAPABILITY);
$project = new PhabricatorProject();
$project->setAuthorPHID($user->getPHID());
$project->attachMemberPHIDs(array());
$project = PhabricatorProject::initializeNewProject($user);
$profile = new PhabricatorProjectProfile();
$e_name = true;
$errors = array();
if ($request->isFormPost()) {
$xactions = array();
try {
$xactions = array();
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
->setNewValue($request->getStr('name'));
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorProjectTransaction::TYPE_NAME);
$xaction->setNewValue($request->getStr('name'));
$xactions[] = $xaction;
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
->setMetadataValue('edge:type', PhabricatorEdgeConfig::TYPE_PROJ_MEMBER)
->setNewValue(
array(
'+' => array($user->getPHID() => $user->getPHID()),
));
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorProjectTransaction::TYPE_MEMBERS);
$xaction->setNewValue(array($user->getPHID()));
$xactions[] = $xaction;
$editor = id(new PhabricatorProjectTransactionEditor())
->setActor($user)
->setContinueOnNoEffect(true)
->setContentSourceFromRequest($request)
->applyTransactions($project, $xactions);
$editor = new PhabricatorProjectEditor($project);
$editor->setActor($user);
$editor->applyTransactions($xactions);
} catch (PhabricatorProjectNameCollisionException $ex) {
$e_name = 'Not Unique';
$errors[] = $ex->getMessage();
}
// TODO: Deal with name collision exceptions more gracefully.
$profile->setBlurb($request->getStr('blurb'));
@ -103,7 +99,6 @@ final class PhabricatorProjectCreateController
return id(new AphrontDialogResponse())->setDialog($dialog);
} else {
$form
->appendChild(
id(new AphrontFormSubmitControl())

View file

@ -35,45 +35,33 @@ final class PhabricatorProjectProfileEditController
$errors = array();
if ($request->isFormPost()) {
try {
$xactions = array();
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorProjectTransaction::TYPE_NAME);
$xaction->setNewValue($request->getStr('name'));
$xactions[] = $xaction;
$xactions = array();
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorProjectTransaction::TYPE_STATUS);
$xaction->setNewValue($request->getStr('status'));
$xactions[] = $xaction;
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
->setNewValue($request->getStr('name'));
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorTransactions::TYPE_VIEW_POLICY);
$xaction->setNewValue($request->getStr('can_view'));
$xactions[] = $xaction;
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorProjectTransaction::TYPE_STATUS)
->setNewValue($request->getStr('status'));
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorTransactions::TYPE_EDIT_POLICY);
$xaction->setNewValue($request->getStr('can_edit'));
$xactions[] = $xaction;
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
->setNewValue($request->getStr('can_view'));
$xaction = new PhabricatorProjectTransaction();
$xaction->setTransactionType(
PhabricatorTransactions::TYPE_JOIN_POLICY);
$xaction->setNewValue($request->getStr('can_join'));
$xactions[] = $xaction;
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY)
->setNewValue($request->getStr('can_edit'));
$editor = new PhabricatorProjectEditor($project);
$editor->setActor($user);
$editor->applyTransactions($xactions);
} catch (PhabricatorProjectNameCollisionException $ex) {
$e_name = pht('Not Unique');
$errors[] = $ex->getMessage();
}
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_JOIN_POLICY)
->setNewValue($request->getStr('can_join'));
$editor = id(new PhabricatorProjectTransactionEditor())
->setActor($user)
->setContentSourceFromRequest($request)
->setContinueOnNoEffect(true)
->applyTransactions($project, $xactions);
$profile->setBlurb($request->getStr('blurb'));

View file

@ -1,255 +0,0 @@
<?php
final class PhabricatorProjectEditor extends PhabricatorEditor {
private $project;
private $projectName;
private $addEdges = array();
private $remEdges = array();
private $shouldArchive = false;
private function setShouldArchive($should_archive) {
$this->shouldArchive = $should_archive;
return $this;
}
private function shouldArchive() {
return $this->shouldArchive;
}
public function __construct(PhabricatorProject $project) {
$this->project = $project;
}
public function applyTransactions(array $transactions) {
assert_instances_of($transactions, 'PhabricatorProjectTransaction');
$actor = $this->requireActor();
$project = $this->project;
$is_new = !$project->getID();
if ($is_new) {
$project->setAuthorPHID($actor->getPHID());
}
foreach ($transactions as $key => $xaction) {
$this->setTransactionOldValue($project, $xaction);
if (!$this->transactionHasEffect($xaction)) {
unset($transactions[$key]);
continue;
}
}
if (!$is_new) {
// You must be able to view a project in order to edit it in any capacity.
PhabricatorPolicyFilter::requireCapability(
$actor,
$project,
PhabricatorPolicyCapability::CAN_VIEW);
PhabricatorPolicyFilter::requireCapability(
$actor,
$project,
PhabricatorPolicyCapability::CAN_EDIT);
}
if (!$transactions) {
return $this;
}
foreach ($transactions as $xaction) {
$this->applyTransactionEffect($project, $xaction);
}
try {
$project->openTransaction();
if ($this->shouldArchive()) {
$project->setStatus(PhabricatorProjectStatus::STATUS_ARCHIVED);
}
$project->save();
$edge_type = PhabricatorEdgeConfig::TYPE_PROJ_MEMBER;
$editor = new PhabricatorEdgeEditor();
$editor->setActor($actor);
foreach ($this->remEdges as $phid) {
$editor->removeEdge($project->getPHID(), $edge_type, $phid);
}
foreach ($this->addEdges as $phid) {
$editor->addEdge($project->getPHID(), $edge_type, $phid);
}
$editor->save();
foreach ($transactions as $xaction) {
$xaction->setAuthorPHID($actor->getPHID());
$xaction->setObjectPHID($project->getPHID());
$xaction->setViewPolicy('public');
$xaction->setEditPolicy($actor->getPHID());
$xaction->setContentSource(
PhabricatorContentSource::newForSource(
PhabricatorContentSource::SOURCE_LEGACY,
array()));
$xaction->save();
}
$project->saveTransaction();
} catch (AphrontQueryDuplicateKeyException $ex) {
// We already validated the slug, but might race. Try again to see if
// that's the issue. If it is, we'll throw a more specific exception. If
// not, throw the original exception.
$this->validateName($project);
throw $ex;
}
id(new PhabricatorSearchIndexer())
->queueDocumentForIndexing($project->getPHID());
return $this;
}
private function validateName(PhabricatorProject $project) {
$slug = $project->getPhrictionSlug();
$name = $project->getName();
if ($slug == '/') {
throw new PhabricatorProjectNameCollisionException(
pht("Project names must be unique and contain some ".
"letters or numbers."));
}
$id = $project->getID();
$collision = id(new PhabricatorProject())->loadOneWhere(
'(name = %s OR phrictionSlug = %s) AND id %Q %nd',
$name,
$slug,
$id ? '!=' : 'IS NOT',
$id ? $id : null);
if ($collision) {
$other_name = $collision->getName();
$other_id = $collision->getID();
throw new PhabricatorProjectNameCollisionException(
pht("Project names must be unique. The name '%s' is too similar to ".
"the name of another project, '%s' (Project ID: ".
"%d). Choose a unique name.", $name, $other_name, $other_id));
}
}
private function setTransactionOldValue(
PhabricatorProject $project,
PhabricatorProjectTransaction $xaction) {
$type = $xaction->getTransactionType();
switch ($type) {
case PhabricatorProjectTransaction::TYPE_NAME:
$xaction->setOldValue($project->getName());
break;
case PhabricatorProjectTransaction::TYPE_STATUS:
$xaction->setOldValue($project->getStatus());
break;
case PhabricatorProjectTransaction::TYPE_MEMBERS:
$member_phids = $project->getMemberPHIDs();
$old_value = array_values($member_phids);
$xaction->setOldValue($old_value);
$new_value = $xaction->getNewValue();
$new_value = array_filter($new_value);
$new_value = array_unique($new_value);
$new_value = array_values($new_value);
$xaction->setNewValue($new_value);
break;
case PhabricatorTransactions::TYPE_VIEW_POLICY:
$xaction->setOldValue($project->getViewPolicy());
break;
case PhabricatorTransactions::TYPE_EDIT_POLICY:
$xaction->setOldValue($project->getEditPolicy());
break;
case PhabricatorTransactions::TYPE_JOIN_POLICY:
$xaction->setOldValue($project->getJoinPolicy());
break;
default:
throw new Exception("Unknown transaction type '{$type}'!");
}
return $this;
}
private function applyTransactionEffect(
PhabricatorProject $project,
PhabricatorProjectTransaction $xaction) {
$type = $xaction->getTransactionType();
switch ($type) {
case PhabricatorProjectTransaction::TYPE_NAME:
$old_slug = $project->getFullPhrictionSlug();
$project->setName($xaction->getNewValue());
$project->setPhrictionSlug($xaction->getNewValue());
$changed_slug = $old_slug != $project->getFullPhrictionSlug();
if ($xaction->getOldValue() && $changed_slug) {
$old_document = id(new PhrictionDocument())
->loadOneWhere(
'slug = %s',
$old_slug);
if ($old_document && $old_document->getStatus() ==
PhrictionDocumentStatus::STATUS_EXISTS) {
$content = id(new PhrictionContent())
->load($old_document->getContentID());
$from_editor = id(PhrictionDocumentEditor::newForSlug($old_slug))
->setActor($this->getActor())
->setTitle($content->getTitle())
->setContent($content->getContent())
->setDescription($content->getDescription());
$target_editor = id(PhrictionDocumentEditor::newForSlug(
$project->getFullPhrictionSlug()))
->setActor($this->getActor())
->setTitle($content->getTitle())
->setContent($content->getContent())
->setDescription($content->getDescription())
->moveHere($old_document->getID(), $old_document->getPHID());
$target_document = $target_editor->getDocument();
$from_editor->moveAway($target_document->getID());
}
}
$this->validateName($project);
break;
case PhabricatorProjectTransaction::TYPE_STATUS:
$project->setStatus($xaction->getNewValue());
break;
case PhabricatorProjectTransaction::TYPE_MEMBERS:
$old = array_fill_keys($xaction->getOldValue(), true);
$new = array_fill_keys($xaction->getNewValue(), true);
$this->addEdges = array_keys(array_diff_key($new, $old));
$this->remEdges = array_keys(array_diff_key($old, $new));
if ($new === array()) {
$this->setShouldArchive(true);
}
break;
case PhabricatorTransactions::TYPE_VIEW_POLICY:
$project->setViewPolicy($xaction->getNewValue());
break;
case PhabricatorTransactions::TYPE_EDIT_POLICY:
$project->setEditPolicy($xaction->getNewValue());
// You can't edit away your ability to edit the project.
PhabricatorPolicyFilter::mustRetainCapability(
$this->getActor(),
$project,
PhabricatorPolicyCapability::CAN_EDIT);
break;
case PhabricatorTransactions::TYPE_JOIN_POLICY:
$project->setJoinPolicy($xaction->getNewValue());
break;
default:
throw new Exception("Unknown transaction type '{$type}'!");
}
}
private function transactionHasEffect(
PhabricatorProjectTransaction $xaction) {
return ($xaction->getOldValue() !== $xaction->getNewValue());
}
}

View file

@ -7,6 +7,12 @@ final class PhabricatorProjectTransactionEditor
$types = parent::getTransactionTypes();
$types[] = PhabricatorTransactions::TYPE_EDGE;
$types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
$types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
$types[] = PhabricatorTransactions::TYPE_JOIN_POLICY;
$types[] = PhabricatorProjectTransaction::TYPE_NAME;
$types[] = PhabricatorProjectTransaction::TYPE_STATUS;
return $types;
}
@ -16,6 +22,10 @@ final class PhabricatorProjectTransactionEditor
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorProjectTransaction::TYPE_NAME:
return $object->getName();
case PhabricatorProjectTransaction::TYPE_STATUS:
return $object->getStatus();
}
return parent::getCustomTransactionOldValue($object, $xaction);
@ -26,6 +36,9 @@ final class PhabricatorProjectTransactionEditor
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorProjectTransaction::TYPE_NAME:
case PhabricatorProjectTransaction::TYPE_STATUS:
return $xaction->getNewValue();
}
return parent::getCustomTransactionNewValue($object, $xaction);
@ -36,8 +49,23 @@ final class PhabricatorProjectTransactionEditor
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorProjectTransaction::TYPE_NAME:
$object->setName($xaction->getNewValue());
return;
case PhabricatorProjectTransaction::TYPE_STATUS:
$object->setStatus($xaction->getNewValue());
return;
case PhabricatorTransactions::TYPE_EDGE:
return;
case PhabricatorTransactions::TYPE_VIEW_POLICY:
$object->setViewPolicy($xaction->getNewValue());
return;
case PhabricatorTransactions::TYPE_EDIT_POLICY:
$object->setEditPolicy($xaction->getNewValue());
return;
case PhabricatorTransactions::TYPE_JOIN_POLICY:
$object->setJoinPolicy($xaction->getNewValue());
return;
}
return parent::applyCustomInternalTransaction($object, $xaction);
@ -48,7 +76,43 @@ final class PhabricatorProjectTransactionEditor
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorProjectTransaction::TYPE_NAME:
$old_slug = $object->getFullPhrictionSlug();
$object->setPhrictionSlug($xaction->getNewValue());
$changed_slug = $old_slug != $object->getFullPhrictionSlug();
if ($xaction->getOldValue() && $changed_slug) {
$old_document = id(new PhrictionDocument())
->loadOneWhere(
'slug = %s',
$old_slug);
if ($old_document && $old_document->getStatus() ==
PhrictionDocumentStatus::STATUS_EXISTS) {
$content = id(new PhrictionContent())
->load($old_document->getContentID());
$from_editor = id(PhrictionDocumentEditor::newForSlug($old_slug))
->setActor($this->getActor())
->setTitle($content->getTitle())
->setContent($content->getContent())
->setDescription($content->getDescription());
$target_editor = id(PhrictionDocumentEditor::newForSlug(
$object->getFullPhrictionSlug()))
->setActor($this->getActor())
->setTitle($content->getTitle())
->setContent($content->getContent())
->setDescription($content->getDescription())
->moveHere($old_document->getID(), $old_document->getPHID());
$target_document = $target_editor->getDocument();
$from_editor->moveAway($target_document->getID());
}
}
return;
case PhabricatorTransactions::TYPE_VIEW_POLICY:
case PhabricatorTransactions::TYPE_EDIT_POLICY:
case PhabricatorTransactions::TYPE_JOIN_POLICY:
case PhabricatorTransactions::TYPE_EDGE:
case PhabricatorProjectTransaction::TYPE_STATUS:
return;
}
@ -63,16 +127,40 @@ final class PhabricatorProjectTransactionEditor
$errors = parent::validateTransaction($object, $type, $xactions);
switch ($type) {
case PhabricatorProjectTransaction::TYPE_NAME:
$missing = $this->validateIsEmptyTextField(
$object->getName(),
$xactions);
if ($missing) {
$error = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Required'),
pht('Project name is required.'),
nonempty(last($xactions), null));
$error->setIsMissingFieldError(true);
$errors[] = $error;
}
break;
}
return $errors;
}
protected function requireCapabilities(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorProjectTransaction::TYPE_NAME:
case PhabricatorProjectTransaction::TYPE_STATUS:
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_EDIT);
return;
case PhabricatorTransactions::TYPE_EDGE:
switch ($xaction->getMetadataValue('edge:type')) {
case PhabricatorEdgeConfig::TYPE_PROJ_MEMBER:
@ -107,7 +195,11 @@ final class PhabricatorProjectTransactionEditor
break;
}
return parent::requireCapabilities();
return parent::requireCapabilities($object, $xaction);
}
protected function supportsSearch() {
return true;
}
}

View file

@ -97,9 +97,10 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
$xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME);
$xaction->setNewValue($new_name);
$editor = new PhabricatorProjectEditor($proj);
$editor = new PhabricatorProjectTransactionEditor();
$editor->setActor($user);
$editor->applyTransactions(array($xaction));
$editor->setContentSource(PhabricatorContentSource::newConsoleSource());
$editor->applyTransactions($proj, array($xaction));
return true;
}

View file

@ -32,9 +32,10 @@ final class PhabricatorProjectTestDataGenerator
PhabricatorTransactions::TYPE_JOIN_POLICY,
PhabricatorPolicies::POLICY_PUBLIC);
$editor = id(new PhabricatorProjectEditor($project))
$editor = id(new PhabricatorProjectTransactionEditor())
->setActor($author)
->applyTransactions($this->xactions);
->setContentSource(PhabricatorContentSource::newConsoleSource())
->applyTransactions($project, $this->xactions);
$profile = id(new PhabricatorProjectProfile())
->setBlurb($this->generateDescription())

View file

@ -20,6 +20,16 @@ final class PhabricatorProject extends PhabricatorProjectDAO
private $sparseMembers = self::ATTACHABLE;
private $profile = self::ATTACHABLE;
public static function initializeNewProject(PhabricatorUser $actor) {
return id(new PhabricatorProject())
->setName('')
->setAuthorPHID($actor->getPHID())
->setViewPolicy(PhabricatorPolicies::POLICY_USER)
->setEditPolicy(PhabricatorPolicies::POLICY_USER)
->setJoinPolicy(PhabricatorPolicies::POLICY_USER)
->attachMemberPHIDs(array());
}
public function getCapabilities() {
return array(
PhabricatorPolicyCapability::CAN_VIEW,
@ -73,6 +83,9 @@ final class PhabricatorProject extends PhabricatorProjectDAO
}
public function isUserMember($user_phid) {
if ($this->memberPHIDs !== self::ATTACHABLE) {
return in_array($user_phid, $this->memberPHIDs);
}
return $this->assertAttachedKey($this->sparseMembers, $user_phid);
}

View file

@ -139,6 +139,8 @@ abstract class PhabricatorApplicationTransactionEditor
return $object->getViewPolicy();
case PhabricatorTransactions::TYPE_EDIT_POLICY:
return $object->getEditPolicy();
case PhabricatorTransactions::TYPE_JOIN_POLICY:
return $object->getJoinPolicy();
case PhabricatorTransactions::TYPE_EDGE:
$edge_type = $xaction->getMetadataValue('edge:type');
if (!$edge_type) {
@ -175,6 +177,7 @@ abstract class PhabricatorApplicationTransactionEditor
return $this->getPHIDTransactionNewValue($xaction);
case PhabricatorTransactions::TYPE_VIEW_POLICY:
case PhabricatorTransactions::TYPE_EDIT_POLICY:
case PhabricatorTransactions::TYPE_JOIN_POLICY:
return $xaction->getNewValue();
case PhabricatorTransactions::TYPE_EDGE:
return $this->getEdgeTransactionNewValue($xaction);