1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00

Disable mentions in nonpublishing repositories

Summary:
Ref T6516. Although this behavior is somewhat-arguable as desirable, I think it's less surprising and more consistent to disable mentions when a repository is publishing.

In particular, if you import a repository developed on another Phabricator install, this stops all the `T123` in commit messages from creating mentions on your unrelated `T123` tasks.

We already disable autoclose, so `Closes T123` and `Ref T123` already have no effect, but a bare `T123` would generate a mention. Likewise, `@epriestley` would generate a mention.

If you import such a repository and then update it periodically, updates will activate autoclose and publishing (if you didn't disable them), but presumably this will hit a couple of tasks and you'll go change the settings if you forgot.

At some point, we may have some kind of use case for separating the "publish" setting into a "publish" setting and a "this is a local repository" setting. For example, if you work at Widget Corp, want to import Phabricator locally, //and// want to write Herald rules against it, you can't currently configure the repository to let you do all of this. But we haven't actually seen a use case for this yet.

Test Plan:
  - Pushed some commits with bare `T11`, saw mentions.
  - Disabled publishing for the repository, pushed some commits with
  - Imported a bunch of commits without seeing pipeline failures.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6516

Differential Revision: https://secure.phabricator.com/D11966
This commit is contained in:
epriestley 2015-03-04 10:36:38 -08:00
parent 46e5e79c4f
commit 174dd220df
2 changed files with 68 additions and 44 deletions

View file

@ -519,22 +519,6 @@ final class PhabricatorAuditEditor
}
protected function shouldSendMail(
PhabricatorLiskDAO $object,
array $xactions) {
// not every code path loads the repository so tread carefully
// TODO: They should, and then we should simplify this.
if ($object->getRepository($assert_attached = false)) {
$repository = $object->getRepository();
if (!$repository->shouldPublish()) {
return false;
}
}
return $this->isCommitMostlyImported($object);
}
protected function buildReplyHandler(PhabricatorLiskDAO $object) {
$reply_handler = PhabricatorEnv::newObjectFromConfig(
'metamta.diffusion.reply-handler');
@ -808,14 +792,6 @@ final class PhabricatorAuditEditor
);
}
protected function shouldPublishFeedStory(
PhabricatorLiskDAO $object,
array $xactions) {
return $this->shouldSendMail($object, $xactions);
}
protected function shouldApplyHeraldRules(
PhabricatorLiskDAO $object,
array $xactions) {
@ -916,4 +892,39 @@ final class PhabricatorAuditEditor
return $object->isPartiallyImported($mask);
}
private function shouldPublishRepositoryActivity(
PhabricatorLiskDAO $object,
array $xactions) {
// not every code path loads the repository so tread carefully
// TODO: They should, and then we should simplify this.
if ($object->getRepository($assert_attached = false)) {
$repository = $object->getRepository();
if (!$repository->shouldPublish()) {
return false;
}
}
return $this->isCommitMostlyImported($object);
}
protected function shouldSendMail(
PhabricatorLiskDAO $object,
array $xactions) {
return $this->shouldPublishRepositoryActivity($object, $xactions);
}
protected function shouldEnableMentions(
PhabricatorLiskDAO $object,
array $xactions) {
return $this->shouldPublishRepositoryActivity($object, $xactions);
}
protected function shouldPublishFeedStory(
PhabricatorLiskDAO $object,
array $xactions) {
return $this->shouldPublishRepositoryActivity($object, $xactions);
}
}

View file

@ -186,6 +186,10 @@ abstract class PhabricatorApplicationTransactionEditor
return $this->unmentionablePHIDMap;
}
protected function shouldEnableMentions() {
return true;
}
public function setApplicationEmail(
PhabricatorMetaMTAApplicationEmail $email) {
$this->applicationEmail = $email;
@ -1052,10 +1056,14 @@ abstract class PhabricatorApplicationTransactionEditor
return null;
}
$texts = array_mergev($blocks);
$phids = PhabricatorMarkupEngine::extractPHIDsFromMentions(
$this->getActor(),
$texts);
if ($this->shouldEnableMentions($object, $xactions)) {
$texts = array_mergev($blocks);
$phids = PhabricatorMarkupEngine::extractPHIDsFromMentions(
$this->getActor(),
$texts);
} else {
$phids = array();
}
$this->mentionedPHIDs = $phids;
@ -1229,12 +1237,14 @@ abstract class PhabricatorApplicationTransactionEditor
$engine);
$mentioned_phids = array();
foreach ($blocks as $key => $xaction_blocks) {
foreach ($xaction_blocks as $block) {
$engine->markupText($block);
$mentioned_phids += $engine->getTextMetadata(
PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS,
array());
if ($this->shouldEnableMentions($object, $xactions)) {
foreach ($blocks as $key => $xaction_blocks) {
foreach ($xaction_blocks as $block) {
$engine->markupText($block);
$mentioned_phids += $engine->getTextMetadata(
PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS,
array());
}
}
}
@ -1248,19 +1258,22 @@ abstract class PhabricatorApplicationTransactionEditor
->execute();
$mentionable_phids = array();
foreach ($mentioned_objects as $mentioned_object) {
if ($mentioned_object instanceof PhabricatorMentionableInterface) {
$mentioned_phid = $mentioned_object->getPHID();
if (idx($this->getUnmentionablePHIDMap(), $mentioned_phid)) {
continue;
if ($this->shouldEnableMentions($object, $xactions)) {
foreach ($mentioned_objects as $mentioned_object) {
if ($mentioned_object instanceof PhabricatorMentionableInterface) {
$mentioned_phid = $mentioned_object->getPHID();
if (idx($this->getUnmentionablePHIDMap(), $mentioned_phid)) {
continue;
}
// don't let objects mention themselves
if ($object->getPHID() && $mentioned_phid == $object->getPHID()) {
continue;
}
$mentionable_phids[$mentioned_phid] = $mentioned_phid;
}
// don't let objects mention themselves
if ($object->getPHID() && $mentioned_phid == $object->getPHID()) {
continue;
}
$mentionable_phids[$mentioned_phid] = $mentioned_phid;
}
}
if ($mentionable_phids) {
$edge_type = PhabricatorObjectMentionsObjectEdgeType::EDGECONST;
$block_xactions[] = newv(get_class(head($xactions)), array())