Index: resources/sql/autopatches/20140211.dx.3.migsubscriptions.sql =================================================================== --- /dev/null +++ resources/sql/autopatches/20140211.dx.3.migsubscriptions.sql @@ -0,0 +1,10 @@ +/* For `grep`: */ + +/* PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER = 21 */ + +INSERT IGNORE INTO {$NAMESPACE}_differential.edge (src, type, dst, seq) + SELECT rev.phid, 21, rel.objectPHID, rel.sequence + FROM {$NAMESPACE}_differential.differential_revision rev + JOIN {$NAMESPACE}_differential.differential_relationship rel + ON rev.id = rel.revisionID + WHERE relation = 'subd'; Index: src/__phutil_library_map__.php =================================================================== --- src/__phutil_library_map__.php +++ src/__phutil_library_map__.php @@ -2953,6 +2953,7 @@ 3 => 'PhabricatorFlaggableInterface', 4 => 'PhrequentTrackableInterface', 5 => 'HarbormasterBuildableInterface', + 6 => 'PhabricatorSubscribableInterface', ), 'DifferentialRevisionCommentListView' => 'AphrontView', 'DifferentialRevisionCommentView' => 'AphrontView', Index: src/applications/differential/conduit/ConduitAPI_differential_query_Method.php =================================================================== --- src/applications/differential/conduit/ConduitAPI_differential_query_Method.php +++ src/applications/differential/conduit/ConduitAPI_differential_query_Method.php @@ -164,7 +164,7 @@ $query->withResponsibleUsers($responsible_users); } if ($subscribers) { - $query->withSubscribers($subscribers); + $query->withCCs($subscribers); } if ($branches) { $query->withBranches($branches); Index: src/applications/differential/editor/DifferentialRevisionEditor.php =================================================================== --- src/applications/differential/editor/DifferentialRevisionEditor.php +++ src/applications/differential/editor/DifferentialRevisionEditor.php @@ -250,12 +250,16 @@ $rem_ccs = array(); $xscript_phid = null; if ($diff) { + $unsubscribed_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $revision->getPHID(), + PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER); + $adapter = HeraldDifferentialRevisionAdapter::newLegacyAdapter( $revision, $diff); $adapter->setExplicitCCs($new['ccs']); $adapter->setExplicitReviewers($new['rev']); - $adapter->setForbiddenCCs($revision->loadUnsubscribedPHIDs()); + $adapter->setForbiddenCCs($unsubscribed_phids); $adapter->setIsNewObject($is_new); $xscript = HeraldEngine::loadAndApplyRules($adapter); @@ -597,13 +601,12 @@ $dont_add = self::getImpliedCCs($revision); $add_phids = array_diff($add_phids, $dont_add); - return self::alterRelationships( - $revision, - $stable_phids, - $rem_phids, - $add_phids, - $reason_phid, - DifferentialRevision::RELATION_SUBSCRIBED); + id(new PhabricatorSubscriptionsEditor()) + ->setActor(PhabricatorUser::getOmnipotentUser()) + ->setObject($revision) + ->subscribeExplicit($add_phids) + ->unsubscribe($rem_phids) + ->save(); } private static function getImpliedCCs(DifferentialRevision $revision) { @@ -694,97 +697,6 @@ ->save(); } - private static function alterRelationships( - DifferentialRevision $revision, - array $stable_phids, - array $rem_phids, - array $add_phids, - $reason_phid, - $relation_type) { - - $rem_map = array_fill_keys($rem_phids, true); - $add_map = array_fill_keys($add_phids, true); - - $seq_map = array_values($stable_phids); - $seq_map = array_flip($seq_map); - foreach ($rem_map as $phid => $ignored) { - if (!isset($seq_map[$phid])) { - $seq_map[$phid] = count($seq_map); - } - } - foreach ($add_map as $phid => $ignored) { - if (!isset($seq_map[$phid])) { - $seq_map[$phid] = count($seq_map); - } - } - - $raw = $revision->getRawRelations($relation_type); - $raw = ipull($raw, null, 'objectPHID'); - - $sequence = count($seq_map); - foreach ($raw as $phid => $ignored) { - if (isset($seq_map[$phid])) { - $raw[$phid]['sequence'] = $seq_map[$phid]; - } else { - $raw[$phid]['sequence'] = $sequence++; - } - } - $raw = isort($raw, 'sequence'); - - foreach ($raw as $phid => $ignored) { - if (isset($rem_map[$phid])) { - unset($raw[$phid]); - } - } - - foreach ($add_phids as $add) { - $reason = is_array($reason_phid) - ? idx($reason_phid, $add) - : $reason_phid; - - $raw[$add] = array( - 'objectPHID' => $add, - 'sequence' => idx($seq_map, $add, $sequence++), - 'reasonPHID' => $reason, - ); - } - - $conn_w = $revision->establishConnection('w'); - - $sql = array(); - foreach ($raw as $relation) { - $sql[] = qsprintf( - $conn_w, - '(%d, %s, %s, %d, %s)', - $revision->getID(), - $relation_type, - $relation['objectPHID'], - $relation['sequence'], - $relation['reasonPHID']); - } - - $conn_w->openTransaction(); - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d AND relation = %s', - DifferentialRevision::RELATIONSHIP_TABLE, - $revision->getID(), - $relation_type); - if ($sql) { - queryfx( - $conn_w, - 'INSERT INTO %T - (revisionID, relation, objectPHID, sequence, reasonPHID) - VALUES %Q', - DifferentialRevision::RELATIONSHIP_TABLE, - implode(', ', $sql)); - } - $conn_w->saveTransaction(); - - $revision->loadRelationships(); - } - - private function createComment() { $template = id(new DifferentialComment()) ->setAuthorPHID($this->getActorPHID()) Index: src/applications/differential/query/DifferentialRevisionQuery.php =================================================================== --- src/applications/differential/query/DifferentialRevisionQuery.php +++ src/applications/differential/query/DifferentialRevisionQuery.php @@ -33,7 +33,6 @@ private $revIDs = array(); private $commitHashes = array(); private $phids = array(); - private $subscribers = array(); private $responsibles = array(); private $branches = array(); private $arcanistProjectPHIDs = array(); @@ -114,7 +113,7 @@ * Filter results to revisions which CC one of the listed people. Calling this * function will clear anything set by previous calls to @{method:withCCs}. * - * @param array List of PHIDs of subscribers + * @param array List of PHIDs of subscribers. * @return this * @task config */ @@ -221,20 +220,6 @@ /** - * Filter results to only return revisions with a given set of subscribers - * (i.e., they are authors, reviewers or CC'd). - * - * @param array List of user PHIDs. - * @return this - * @task config - */ - public function withSubscribers(array $subscriber_phids) { - $this->subscribers = $subscriber_phids; - return $this; - } - - - /** * Filter results to only return revisions with a given set of arcanist * projects. * @@ -628,11 +613,11 @@ if ($this->ccs) { $joins[] = qsprintf( $conn_r, - 'JOIN %T cc_rel ON cc_rel.revisionID = r.id '. - 'AND cc_rel.relation = %s '. - 'AND cc_rel.objectPHID in (%Ls)', - DifferentialRevision::RELATIONSHIP_TABLE, - DifferentialRevision::RELATION_SUBSCRIBED, + 'JOIN %T e_ccs ON e_ccs.src = r.phid '. + 'AND e_ccs.type = %s '. + 'AND e_ccs.dst in (%Ls)', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER, $this->ccs); } @@ -647,27 +632,6 @@ $this->reviewers); } - if ($this->subscribers) { - // TODO: These can be expressed as a JOIN again (and the corresponding - // WHERE clause removed) once subscribers move to edges. - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T sub_rel_cc ON sub_rel_cc.revisionID = r.id '. - 'AND sub_rel_cc.relation = %s '. - 'AND sub_rel_cc.objectPHID in (%Ls)', - DifferentialRevision::RELATIONSHIP_TABLE, - DifferentialRevision::RELATION_SUBSCRIBED, - $this->subscribers); - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T sub_rel_reviewer ON sub_rel_reviewer.src = r.phid '. - 'AND sub_rel_reviewer.type = %s '. - 'AND sub_rel_reviewer.dst in (%Ls)', - PhabricatorEdgeConfig::TABLE_NAME_EDGE, - PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, - $this->subscribers); - } - $joins = implode(' ', $joins); return $joins; @@ -757,13 +721,6 @@ $this->arcanistProjectPHIDs); } - if ($this->subscribers) { - $where[] = qsprintf( - $conn_r, - '(sub_rel_cc.objectPHID IS NOT NULL) - OR (sub_rel_reviewer.dst IS NOT NULL)'); - } - switch ($this->status) { case self::STATUS_ANY: break; @@ -828,8 +785,7 @@ $join_triggers = array_merge( $this->pathIDs, $this->ccs, - $this->reviewers, - $this->subscribers); + $this->reviewers); $needs_distinct = (count($join_triggers) > 1); @@ -929,31 +885,32 @@ private function loadRelationships($conn_r, array $revisions) { assert_instances_of($revisions, 'DifferentialRevision'); - $relationships = queryfx_all( - $conn_r, - 'SELECT * FROM %T WHERE revisionID in (%Ld) - AND relation != %s ORDER BY sequence', - DifferentialRevision::RELATIONSHIP_TABLE, - mpull($revisions, 'getID'), - DifferentialRevision::RELATION_REVIEWER); - $relationships = igroup($relationships, 'revisionID'); $type_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER; + $type_subscriber = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER; + $edges = id(new PhabricatorEdgeQuery()) ->withSourcePHIDs(mpull($revisions, 'getPHID')) - ->withEdgeTypes(array($type_reviewer)) + ->withEdgeTypes(array($type_reviewer, $type_subscriber)) ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST) ->execute(); + $type_map = array( + DifferentialRevision::RELATION_REVIEWER => $type_reviewer, + DifferentialRevision::RELATION_SUBSCRIBED => $type_subscriber, + ); + foreach ($revisions as $revision) { - $data = idx($relationships, $revision->getID(), array()); - $revision_edges = $edges[$revision->getPHID()][$type_reviewer]; - foreach ($revision_edges as $dst_phid => $edge_data) { - $data[] = array( - 'relation' => DifferentialRevision::RELATION_REVIEWER, - 'objectPHID' => $dst_phid, - 'reasonPHID' => null, - ); + $data = array(); + foreach ($type_map as $rel_type => $edge_type) { + $revision_edges = $edges[$revision->getPHID()][$edge_type]; + foreach ($revision_edges as $dst_phid => $edge_data) { + $data[] = array( + 'relation' => $rel_type, + 'objectPHID' => $dst_phid, + 'reasonPHID' => null, + ); + } } $revision->attachRelationships($data); Index: src/applications/differential/storage/DifferentialRevision.php =================================================================== --- src/applications/differential/storage/DifferentialRevision.php +++ src/applications/differential/storage/DifferentialRevision.php @@ -6,7 +6,8 @@ PhabricatorPolicyInterface, PhabricatorFlaggableInterface, PhrequentTrackableInterface, - HarbormasterBuildableInterface { + HarbormasterBuildableInterface, + PhabricatorSubscribableInterface { protected $title = ''; protected $originalTitle; @@ -39,7 +40,6 @@ private $reviewerStatus = self::ATTACHABLE; - const RELATIONSHIP_TABLE = 'differential_relationship'; const TABLE_COMMIT = 'differential_commit'; const RELATION_REVIEWER = 'revw'; @@ -190,12 +190,6 @@ queryfx( $conn_w, 'DELETE FROM %T WHERE revisionID = %d', - self::RELATIONSHIP_TABLE, - $this->getID()); - - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d', self::TABLE_COMMIT, $this->getID()); @@ -240,22 +234,24 @@ return; } - // Read "subscribed" and "unsubscribed" data out of the old relationship - // table. - $data = queryfx_all( - $this->establishConnection('r'), - 'SELECT * FROM %T WHERE revisionID = %d - AND relation != %s ORDER BY sequence', - self::RELATIONSHIP_TABLE, - $this->getID(), - self::RELATION_REVIEWER); + $data = array(); + + $subscriber_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $this->getPHID(), + PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER); + $subscriber_phids = array_reverse($subscriber_phids); + foreach ($subscriber_phids as $phid) { + $data[] = array( + 'relation' => self::RELATION_SUBSCRIBED, + 'objectPHID' => $phid, + 'reasonPHID' => null, + ); + } - // Read "reviewer" data out of the new table. $reviewer_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( $this->getPHID(), PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER); $reviewer_phids = array_reverse($reviewer_phids); - foreach ($reviewer_phids as $phid) { $data[] = array( 'relation' => self::RELATION_REVIEWER, @@ -290,12 +286,6 @@ return idx($this->relationships, $relation, array()); } - public function loadUnsubscribedPHIDs() { - return PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->phid, - PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER); - } - public function getPrimaryReviewer() { $reviewers = $this->getReviewers(); $last = $this->lastReviewerPHID; @@ -425,4 +415,25 @@ return $this->getPHID(); } + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + + + public function isAutomaticallySubscribed($phid) { + // TODO: Reviewers are also automatically subscribed, but that data may + // not always be loaded, so be conservative for now. All the editing code + // has checks around this already. + return ($phid == $this->getAuthorPHID()); + } + + public function shouldShowSubscribersProperty() { + // TODO: For now, Differential has its own stuff. + return false; + } + + public function shouldAllowSubscription($phid) { + // TODO: For now, Differential has its own stuff. + return false; + } + }