Changeset View
Standalone View
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
Show First 20 Lines • Show All 1,047 Lines • ▼ Show 20 Lines | private function buildSubscribeTransaction( | ||||
if ($object->getPHID()) { | if ($object->getPHID()) { | ||||
// Don't try to subscribe already-subscribed mentions: we want to generate | // Don't try to subscribe already-subscribed mentions: we want to generate | ||||
// a dialog about an action having no effect if the user explicitly adds | // a dialog about an action having no effect if the user explicitly adds | ||||
// existing CCs, but not if they merely mention existing subscribers. | // existing CCs, but not if they merely mention existing subscribers. | ||||
$phids = array_diff($phids, $this->subscribers); | $phids = array_diff($phids, $this->subscribers); | ||||
} | } | ||||
if ($phids) { | |||||
$users = id(new PhabricatorPeopleQuery()) | |||||
->withPHIDs($phids) | |||||
->loadPage(); | |||||
epriestley: Oh, `loadPage()` is supposed to be `protected`, it just isn't in this class for some reason. | |||||
$users = mpull($users, null, 'getPHID'); | |||||
foreach ($phids as $key => $phid) { | foreach ($phids as $key => $phid) { | ||||
// Do not subscribe mentioned users | |||||
// who do not have VIEW Permissions | |||||
if (!PhabricatorPolicyFilter::hasCapability( | |||||
epriestleyUnsubmitted Not Done Inline ActionsTo be completely correct, we should probably test: if ($object instanceof PhabricatorPolicyInterface) ...before doing the hasCapability() check. I think there are no PhabricatorSubscribableInterface objects which aren't also PhabricatorPolicyInterface objects right now, but I might be forgetting something and we (or a third party application) might have such an object in the future. If the object doesn't implement PhabricatorPolicyInterface, we can assume all users can get mentioned. epriestley: To be completely correct, we should probably test:
if ($object instanceof… | |||||
$users[$phid], | |||||
$object, | |||||
PhabricatorPolicyCapability::CAN_VIEW) | |||||
) { | |||||
unset($phids[$key]); | |||||
} else { | |||||
if ($object->isAutomaticallySubscribed($phid)) { | if ($object->isAutomaticallySubscribed($phid)) { | ||||
unset($phids[$key]); | unset($phids[$key]); | ||||
} | } | ||||
} | } | ||||
} | |||||
$phids = array_values($phids); | $phids = array_values($phids); | ||||
} | |||||
// No else here to properly return null should we unset all subscriber | |||||
if (!$phids) { | if (!$phids) { | ||||
return null; | return null; | ||||
} | } | ||||
$xaction = newv(get_class(head($xactions)), array()); | $xaction = newv(get_class(head($xactions)), array()); | ||||
$xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); | $xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); | ||||
$xaction->setNewValue(array('+' => $phids)); | $xaction->setNewValue(array('+' => $phids)); | ||||
▲ Show 20 Lines • Show All 617 Lines • ▼ Show 20 Lines | switch ($type) { | ||||
if (!$field->shouldEnableForRole($role_xactions)) { | if (!$field->shouldEnableForRole($role_xactions)) { | ||||
continue; | continue; | ||||
} | } | ||||
$errors[] = $field->validateApplicationTransactions( | $errors[] = $field->validateApplicationTransactions( | ||||
$this, | $this, | ||||
$type, | $type, | ||||
idx($groups, $field->getFieldKey(), array())); | idx($groups, $field->getFieldKey(), array())); | ||||
} | } | ||||
break; | break; | ||||
} | } | ||||
epriestleyUnsubmitted Not Done Inline ActionsSpecifically, I would expect the explicit CC case to go here, in a new TYPE_SUBSCRIBERS block. It would check visibility and then add an error to the $errors[] array, like "You can not add users X, Y or Z as subscribers because they can not see this object.". That should give a clean piece of feedback on explicit CCs, no matter how they were added (e.g., "Add CCs" or "Edit -> change the CCs field", or Differential commit message stuff). Once that works, we can grey out the username tokens in the tokenizer when the user types them, to provide similar feedback to greying out mentions. epriestley: Specifically, I would expect the //explicit// CC case to go here, in a new TYPE_SUBSCRIBERS… | |||||
return array_mergev($errors); | return array_mergev($errors); | ||||
} | } | ||||
private function validatePolicyTransaction( | private function validatePolicyTransaction( | ||||
PhabricatorLiskDAO $object, | PhabricatorLiskDAO $object, | ||||
array $xactions, | array $xactions, | ||||
$transaction_type, | $transaction_type, | ||||
▲ Show 20 Lines • Show All 868 Lines • Show Last 20 Lines |
Oh, loadPage() is supposed to be protected, it just isn't in this class for some reason. Maybe we can add a reflection-based unit test to catch this stuff: when overloading methods, we always want to retain the same visibility level (in this codebase, at least).
You should write this as: