Differential D11049 Diff 26715 src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

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())
		->setViewer($this->getActor())
		->withPHIDs($phids)
		epriestleyUnsubmitted Not Done Inline Actions 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: $users = id(new PhabricatorPeopleQuery()) ->setViewer($this->getActor()) ->withPHIDs($phids) ->execute(); epriestley: Oh, `loadPage()` is supposed to be `protected`, it just isn't in this class for some reason.
		->execute();
		$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
		epriestleyUnsubmitted Not Done Inline Actions To 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…
		if ($object instanceof PhabricatorPolicyInterface
		&& !PhabricatorPolicyFilter::hasCapability(
		$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 Actions Specifically, 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