1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-19 21:32:43 +01:00

Allow monogrammed objects to be parsed from the arc command line in "Reviewers" and similar fields

Summary:
Ref T10939. This allows the CLI to parse reviewers and subscribers like this:

```Reviewers: epriestley, O123 Some Package Name```

The rule goes:

  - If a reviewer or subscriber starts with a monogram (like `X111`), just look that up and ignore everything until the next comma.
  - Otherwise, split it on spaces and look up each part.

This means that these are valid:

```
alincoln htaft
alincoln, htaft
#a #b epriestley
O123 Some Package, epriestley, #b
```

I think the only real downside is that this:

```
O123 Some Package epriestley
```

...ignores the "epriestley" part. However, I don't expect users to be typing package monograms manually -- they just need to be representable by `arc land` and `arc diff --edit` and such. Those flows will always add commas and make the parse unambiguous.

Test Plan:
  - Added test coverage.
  - `amend --show`'d a revision with a package subscriber (this isn't currently possible to produce using the web UI, it came from a future change) and saw `Subscribers: O123 package name, usera, userb`.
  - Updated a revision with a package subscriber.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10939

Differential Revision: https://secure.phabricator.com/D15911
This commit is contained in:
epriestley 2016-05-13 07:56:52 -07:00
parent 9abc16df4d
commit 3ea47d967a
8 changed files with 96 additions and 9 deletions

View file

@ -54,7 +54,7 @@ abstract class DifferentialCustomField
if ($handle->getPolicyFiltered()) { if ($handle->getPolicyFiltered()) {
$out[] = $handle->getPHID(); $out[] = $handle->getPHID();
} else if ($handle->isComplete()) { } else if ($handle->isComplete()) {
$out[] = $handle->getObjectName(); $out[] = $handle->getCommandLineObjectName();
} }
} }

View file

@ -146,6 +146,7 @@ final class DifferentialReviewersField
array( array(
PhabricatorPeopleUserPHIDType::TYPECONST, PhabricatorPeopleUserPHIDType::TYPECONST,
PhabricatorProjectProjectPHIDType::TYPECONST, PhabricatorProjectProjectPHIDType::TYPECONST,
PhabricatorOwnersPackagePHIDType::TYPECONST,
)); ));
} }

View file

@ -78,6 +78,7 @@ final class DifferentialSubscribersField
array( array(
PhabricatorPeopleUserPHIDType::TYPECONST, PhabricatorPeopleUserPHIDType::TYPECONST,
PhabricatorProjectProjectPHIDType::TYPECONST, PhabricatorProjectProjectPHIDType::TYPECONST,
PhabricatorOwnersPackagePHIDType::TYPECONST,
)); ));
} }

View file

@ -205,6 +205,21 @@ final class PhabricatorOwnersPackageTransactionEditor
$error->setIsMissingFieldError(true); $error->setIsMissingFieldError(true);
$errors[] = $error; $errors[] = $error;
} }
foreach ($xactions as $xaction) {
$new = $xaction->getNewValue();
if (preg_match('([,!])', $new)) {
$errors[] = new PhabricatorApplicationTransactionValidationError(
$type,
pht('Invalid'),
pht(
'Package names may not contain commas (",") or exclamation '.
'marks ("!"). These characters are ambiguous when package '.
'names are parsed from the command line.'),
$xaction);
}
}
break; break;
case PhabricatorOwnersPackageTransaction::TYPE_PATHS: case PhabricatorOwnersPackageTransaction::TYPE_PATHS:
if (!$xactions) { if (!$xactions) {

View file

@ -40,9 +40,11 @@ final class PhabricatorOwnersPackagePHIDType extends PhabricatorPHIDType {
$name = $package->getName(); $name = $package->getName();
$id = $package->getID(); $id = $package->getID();
$handle->setName($monogram); $handle
$handle->setFullName("{$monogram}: {$name}"); ->setName($monogram)
$handle->setURI("/owners/package/{$id}/"); ->setFullName("{$monogram}: {$name}")
->setCommandLineObjectName("{$monogram} {$name}")
->setURI("/owners/package/{$id}/");
if ($package->isArchived()) { if ($package->isArchived()) {
$handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED); $handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED);

View file

@ -29,6 +29,7 @@ final class PhabricatorObjectHandle
private $policyFiltered; private $policyFiltered;
private $subtitle; private $subtitle;
private $tokenIcon; private $tokenIcon;
private $commandLineObjectName;
public function setIcon($icon) { public function setIcon($icon) {
$this->icon = $icon; $this->icon = $icon;
@ -196,6 +197,19 @@ final class PhabricatorObjectHandle
return $this->getName(); return $this->getName();
} }
public function setCommandLineObjectName($command_line_object_name) {
$this->commandLineObjectName = $command_line_object_name;
return $this;
}
public function getCommandLineObjectName() {
if ($this->commandLineObjectName !== null) {
return $this->commandLineObjectName;
}
return $this->getObjectName();
}
public function setTitle($title) { public function setTitle($title) {
$this->title = $title; $this->title = $title;
return $this; return $this;

View file

@ -45,9 +45,49 @@ final class PhabricatorObjectListQuery extends Phobject {
public function execute() { public function execute() {
$names = $this->getObjectList(); $names = $this->getObjectList();
$names = array_unique(array_filter(preg_split('/[\s,]+/', $names)));
$objects = $this->loadObjects($names); // First, normalize any internal whitespace so we don't get weird results
// if linebreaks hit in weird spots.
$names = preg_replace('/\s+/', ' ', $names);
// Split the list on commas.
$names = explode(',', $names);
// Trim and remove empty tokens.
foreach ($names as $key => $name) {
$name = trim($name);
if (!strlen($name)) {
unset($names[$key]);
continue;
}
$names[$key] = $name;
}
// Remove duplicates.
$names = array_unique($names);
$name_map = array();
foreach ($names as $name) {
$parts = explode(' ', $name);
// If this looks like a monogram, ignore anything after the first token.
// This allows us to parse "O123 Package Name" as though it was "O123",
// which we can look up.
if (preg_match('/^[A-Z]\d+\z/', $parts[0])) {
$name_map[$parts[0]] = $name;
} else {
// For anything else, split it on spaces and use each token as a
// value. This means "alincoln htaft", separated with a space instead
// of with a comma, is two different users.
foreach ($parts as $part) {
$name_map[$part] = $part;
}
}
}
$objects = $this->loadObjects(array_keys($name_map));
$types = array(); $types = array();
foreach ($objects as $name => $object) { foreach ($objects as $name => $object) {
@ -66,8 +106,8 @@ final class PhabricatorObjectListQuery extends Phobject {
$invalid = array_mergev($invalid); $invalid = array_mergev($invalid);
$missing = array(); $missing = array();
foreach ($names as $name) { foreach ($name_map as $key => $name) {
if (empty($objects[$name])) { if (empty($objects[$key])) {
$missing[] = $name; $missing[] = $name;
} }
} }

View file

@ -13,7 +13,6 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
$name = $user->getUsername(); $name = $user->getUsername();
$phid = $user->getPHID(); $phid = $user->getPHID();
$result = $this->parseObjectList("@{$name}"); $result = $this->parseObjectList("@{$name}");
$this->assertEqual(array($phid), $result); $this->assertEqual(array($phid), $result);
@ -29,6 +28,21 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
$result = $this->parseObjectList(''); $result = $this->parseObjectList('');
$this->assertEqual(array(), $result); $this->assertEqual(array(), $result);
$package = PhabricatorOwnersPackage::initializeNewPackage($user)
->setName(pht('Query Test Package'))
->save();
$package_phid = $package->getPHID();
$package_mono = $package->getMonogram();
$result = $this->parseObjectList("{$package_mono} Any Ignored Text");
$this->assertEqual(array($package_phid), $result);
$result = $this->parseObjectList("{$package_mono} Any Text, {$name}");
$this->assertEqual(array($package_phid, $phid), $result);
// Expect failure when loading a user if objects must be of type "DUCK". // Expect failure when loading a user if objects must be of type "DUCK".
$caught = null; $caught = null;
try { try {