diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 79d9e93afd..a9b28c2db0 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -153,7 +153,7 @@ final class PhabricatorAuditEditor $status_accepted = PhabricatorAuditStatusConstants::ACCEPTED; $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; - $actor_phid = $this->requireActor()->getPHID(); + $actor_phid = $this->getActingAsPHID(); $actor_is_author = ($object->getAuthorPHID()) && ($actor_phid == $object->getAuthorPHID()); @@ -317,7 +317,7 @@ final class PhabricatorAuditEditor $can_author_close = PhabricatorEnv::getEnvConfig($can_author_close_key); $actor_is_author = ($object->getAuthorPHID()) && - ($object->getAuthorPHID() == $this->requireActor()->getPHID()); + ($object->getAuthorPHID() == $this->getActingAsPHID()); switch ($action) { case PhabricatorAuditActionConstants::CLOSE: @@ -348,7 +348,7 @@ final class PhabricatorAuditEditor protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { - return true; + return $this->isCommitMostlyImported($object); } protected function buildReplyHandler(PhabricatorLiskDAO $object) { @@ -467,7 +467,20 @@ final class PhabricatorAuditEditor protected function shouldPublishFeedStory( PhabricatorLiskDAO $object, array $xactions) { - return true; + return $this->isCommitMostlyImported($object); + } + + private function isCommitMostlyImported(PhabricatorLiskDAO $object) { + $has_message = PhabricatorRepositoryCommit::IMPORTED_MESSAGE; + $has_changes = PhabricatorRepositoryCommit::IMPORTED_CHANGE; + + // Don't publish feed stories or email about events which occur during + // import. In particular, this affects tasks being attached when they are + // closed by "Fixes Txxxx" in a commit message. See T5851. + + $mask = ($has_message | $has_changes); + + return $object->isPartiallyImported($mask); } } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 0fb2c53176..8bfc106bf4 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -98,6 +98,8 @@ final class DifferentialTransactionEditor PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { + $actor_phid = $this->getActingAsPHID(); + switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_INLINE: return $xaction->hasComment(); @@ -119,7 +121,6 @@ final class DifferentialTransactionEditor } $actor = $this->getActor(); - $actor_phid = $actor->getPHID(); // These transactions can cause effects in two ways: by altering the // status of an existing reviewer; or by adding the actor as a new @@ -151,7 +152,6 @@ final class DifferentialTransactionEditor case DifferentialAction::ACTION_REQUEST: return ($object->getStatus() != $status_review); case DifferentialAction::ACTION_RESIGN: - $actor_phid = $this->getActor()->getPHID(); foreach ($object->getReviewerStatus() as $reviewer) { if ($reviewer->getReviewerPHID() == $actor_phid) { return true; @@ -159,7 +159,6 @@ final class DifferentialTransactionEditor } return false; case DifferentialAction::ACTION_CLAIM: - $actor_phid = $this->getActor()->getPHID(); return ($actor_phid != $object->getAuthorPHID()); } } @@ -231,7 +230,7 @@ final class DifferentialTransactionEditor $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED); return; case DifferentialAction::ACTION_CLAIM: - $object->setAuthorPHID($this->getActor()->getPHID()); + $object->setAuthorPHID($this->getActingAsPHID()); return; } break; @@ -247,7 +246,7 @@ final class DifferentialTransactionEditor $results = parent::expandTransaction($object, $xaction); $actor = $this->getActor(); - $actor_phid = $actor->getPHID(); + $actor_phid = $this->getActingAsPHID(); $type_edge = PhabricatorTransactions::TYPE_EDGE; $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; @@ -782,7 +781,7 @@ final class DifferentialTransactionEditor $action) { $author_phid = $revision->getAuthorPHID(); - $actor_phid = $this->getActor()->getPHID(); + $actor_phid = $this->getActingAsPHID(); $actor_is_author = ($author_phid == $actor_phid); $config_abandon_key = 'differential.always-allow-abandon'; diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index aa3e07b4f6..7edff793ff 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -410,7 +410,7 @@ final class ManiphestTransactionEditor protected function getMailTo(PhabricatorLiskDAO $object) { return array( $object->getOwnerPHID(), - $this->requireActor()->getPHID(), + $this->getActingAsPHID(), ); } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 4d03999019..637c5ac179 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -43,20 +43,24 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $author_phid); } - $field_values = id(new DiffusionLowLevelCommitFieldsQuery()) - ->setRepository($repository) - ->withCommitRef($ref) - ->execute(); - $revision_id = idx($field_values, 'revisionID'); + $differential_app = 'PhabricatorDifferentialApplication'; + $revision_id = null; + if (PhabricatorApplication::isClassInstalled($differential_app)) { + $field_values = id(new DiffusionLowLevelCommitFieldsQuery()) + ->setRepository($repository) + ->withCommitRef($ref) + ->execute(); + $revision_id = idx($field_values, 'revisionID'); - if (!empty($field_values['reviewedByPHIDs'])) { - $data->setCommitDetail( - 'reviewerPHID', - reset($field_values['reviewedByPHIDs'])); + if (!empty($field_values['reviewedByPHIDs'])) { + $data->setCommitDetail( + 'reviewerPHID', + reset($field_values['reviewedByPHIDs'])); + } + + $data->setCommitDetail('differential.revisionID', $revision_id); } - $data->setCommitDetail('differential.revisionID', $revision_id); - if ($author_phid != $commit->getAuthorPHID()) { $commit->setAuthorPHID($author_phid); } @@ -64,6 +68,17 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $commit->setSummary($data->getSummary()); $commit->save(); + // When updating related objects, we'll act under an omnipotent user to + // ensure we can see them, but take actions as either the committer or + // author (if we recognize their accounts) or the Diffusion application + // (if we do not). + + $actor = PhabricatorUser::getOmnipotentUser(); + $acting_as_phid = nonempty( + $committer_phid, + $author_phid, + id(new PhabricatorDiffusionApplication())->getPHID()); + $conn_w = id(new DifferentialRevision())->establishConnection('w'); // NOTE: The `differential_commit` table has a unique ID on `commitPHID`, @@ -78,10 +93,9 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $should_autoclose = $repository->shouldAutocloseCommit($commit, $data); if ($revision_id) { - // TODO: Check if a more restrictive viewer could be set here $revision_query = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($actor) ->needReviewerStatus(true) ->needActiveDiffs(true); @@ -105,14 +119,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $should_autoclose; if ($should_close) { - $actor_phid = nonempty( - $committer_phid, - $author_phid, - $revision->getAuthorPHID()); - - $actor = id(new PhabricatorUser()) - ->loadOneWhere('phid = %s', $actor_phid); - $commit_name = $repository->formatCommitName( $commit->getCommitIdentifier()); @@ -139,7 +145,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $author_name); } - $diff = $this->generateFinalDiff($revision, $actor_phid); + $diff = $this->generateFinalDiff($revision, $acting_as_phid); $vs_diff = $this->loadChangedByCommit($revision, $diff); $changed_uri = null; @@ -177,6 +183,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $editor = id(new DifferentialTransactionEditor()) ->setActor($actor) + ->setActingAsPHID($acting_as_phid) ->setContinueOnMissingFields(true) ->setContentSource($content_source) ->setChangedPriorToCommitURI($changed_uri) @@ -195,10 +202,12 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker } if ($should_autoclose) { - // TODO: This isn't as general as it could be. - if ($user->getPHID()) { - $this->closeTasks($user, $repository, $commit, $message); - } + $this->closeTasks( + $actor, + $acting_as_phid, + $repository, + $commit, + $message); } $data->save(); @@ -405,6 +414,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker private function closeTasks( PhabricatorUser $actor, + $acting_as, PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit, $message) { @@ -487,6 +497,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $editor = id(new ManiphestTransactionEditor()) ->setActor($actor) + ->setActingAsPHID($acting_as) ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) ->setContentSource($content_source); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php index 47eb66f5e9..3ba62978e2 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php @@ -4,6 +4,19 @@ final class PhabricatorApplicationTransactionCommentEditor extends PhabricatorEditor { private $contentSource; + private $actingAsPHID; + + public function setActingAsPHID($acting_as_phid) { + $this->actingAsPHID = $acting_as_phid; + return $this; + } + + public function getActingAsPHID() { + if ($this->actingAsPHID) { + return $this->actingAsPHID; + } + return $this->getActor()->getPHID(); + } public function setContentSource(PhabricatorContentSource $content_source) { $this->contentSource = $content_source; @@ -27,11 +40,11 @@ final class PhabricatorApplicationTransactionCommentEditor $actor = $this->requireActor(); $comment->setContentSource($this->getContentSource()); - $comment->setAuthorPHID($actor->getPHID()); + $comment->setAuthorPHID($this->getActingAsPHID()); // TODO: This needs to be more sophisticated once we have meta-policies. $comment->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC); - $comment->setEditPolicy($actor->getPHID()); + $comment->setEditPolicy($this->getActingAsPHID()); $xaction->openTransaction(); $xaction->beginReadLocking(); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 471e0f304f..ac6a1d6f9c 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -479,7 +479,7 @@ abstract class PhabricatorApplicationTransactionEditor if ($actor->isOmnipotent()) { $xaction->setEditPolicy(PhabricatorPolicies::POLICY_NOONE); } else { - $xaction->setEditPolicy($actor->getPHID()); + $xaction->setEditPolicy($this->getActingAsPHID()); } $xaction->setAuthorPHID($this->getActingAsPHID()); @@ -644,6 +644,7 @@ abstract class PhabricatorApplicationTransactionEditor $comment_editor = id(new PhabricatorApplicationTransactionCommentEditor()) ->setActor($actor) + ->setActingAsPHID($this->getActingAsPHID()) ->setContentSource($this->getContentSource()); if (!$transaction_open) { @@ -1717,7 +1718,15 @@ abstract class PhabricatorApplicationTransactionEditor return $xactions; } - $actor_phid = $this->requireActor()->getPHID(); + $actor_phid = $this->getActingAsPHID(); + + $type_user = PhabricatorPeopleUserPHIDType::TYPECONST; + if (phid_get_type($actor_phid) != $type_user) { + // Transactions by application actors like Herald, Harbormaster and + // Diffusion should not CC the applications. + return $xactions; + } + if ($object->isAutomaticallySubscribed($actor_phid)) { // If they're auto-subscribed, don't CC them. return $xactions; @@ -1827,7 +1836,7 @@ abstract class PhabricatorApplicationTransactionEditor } $template - ->setFrom($this->requireActor()->getPHID()) + ->setFrom($this->getActingAsPHID()) ->setSubjectPrefix($this->getMailSubjectPrefix()) ->setVarySubjectPrefix('['.$action.']') ->setThreadID($this->getMailThreadID($object), $this->getIsNewObject()) @@ -2089,7 +2098,7 @@ abstract class PhabricatorApplicationTransactionEditor return array( $object->getPHID(), - $this->requireActor()->getPHID(), + $this->getActingAsPHID(), ); } @@ -2148,7 +2157,7 @@ abstract class PhabricatorApplicationTransactionEditor ->setStoryType($story_type) ->setStoryData($story_data) ->setStoryTime(time()) - ->setStoryAuthorPHID($this->requireActor()->getPHID()) + ->setStoryAuthorPHID($this->getActingAsPHID()) ->setRelatedPHIDs($related_phids) ->setPrimaryObjectPHID($object->getPHID()) ->setSubscribedPHIDs($subscribed_phids) @@ -2394,6 +2403,7 @@ abstract class PhabricatorApplicationTransactionEditor ->setParentMessageID($this->getParentMessageID()) ->setIsInverseEdgeEditor(true) ->setActor($this->requireActor()) + ->setActingAsPHID($this->getActingAsPHID()) ->setContentSource($this->getContentSource()); $editor->applyTransactions($target, array($template));