revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-28 08:20:57 +01:00
Code Issues Releases Wiki Activity

Move Differential to proper subscriptions

Browse source 
Summary:
Ref T2222. Ref T4415. We're still writing Differential subscription stuff into this weird legacy `differential_relationship` table, which is like an edge table but extremely ancient.

Move it into a proper table.

I've removed `withSubscriptions()` from `DifferentialRevisionQuery`. It was weird, doesn't work consistently with other similar filters, and was only used by the API. Now it means "ccs", which is consistent with the ApplicationSearch UI and with Maniphest.

Test Plan:
Without migrating, added and removed subscribers via various workflows. Queried for subscribers. Everything worked as expected.

Ran the migration, verified data survived.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222, T4415

Differential Revision: https://secure.phabricator.com/D8202
This commit is contained in:
epriestley 2014-02-12 08:53:40 -08:00
parent 1dbfc56d35
commit 8f9b7f4196
6 changed files with 84 additions and 193 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
resources/sql/autopatches/20140211.dx.3.migsubscriptions.sql Normal file
View file

@ -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';

1
src/__phutil_library_map__.php
View file

@ -2953,6 +2953,7 @@ phutil_register_library_map(array(
3 => 'PhabricatorFlaggableInterface',
4 => 'PhrequentTrackableInterface',
5 => 'HarbormasterBuildableInterface',
6 => 'PhabricatorSubscribableInterface',
),
'DifferentialRevisionCommentListView' => 'AphrontView',
'DifferentialRevisionCommentView' => 'AphrontView',

2
src/applications/differential/conduit/ConduitAPI_differential_query_Method.php
View file

@ -164,7 +164,7 @@ final class ConduitAPI_differential_query_Method
$query->withResponsibleUsers($responsible_users);
}
if ($subscribers) {
$query->withSubscribers($subscribers);
$query->withCCs($subscribers);
}
if ($branches) {
$query->withBranches($branches);

110
src/applications/differential/editor/DifferentialRevisionEditor.php
View file

@ -250,12 +250,16 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
$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 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
$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 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
->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())

93
src/applications/differential/query/DifferentialRevisionQuery.php
View file

@ -33,7 +33,6 @@ final class DifferentialRevisionQuery
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 @@ final class DifferentialRevisionQuery
* 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
*/
@ -220,20 +219,6 @@ final class DifferentialRevisionQuery
}
/**
* 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 @@ final class DifferentialRevisionQuery
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 @@ final class DifferentialRevisionQuery
$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 @@ final class DifferentialRevisionQuery
$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 @@ final class DifferentialRevisionQuery
$join_triggers = array_merge(
$this->pathIDs,
$this->ccs,
$this->reviewers,
$this->subscribers);
$this->reviewers);
$needs_distinct = (count($join_triggers) > 1);
@ -929,31 +885,32 @@ final class DifferentialRevisionQuery
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);

61
src/applications/differential/storage/DifferentialRevision.php
View file

@ -6,7 +6,8 @@ final class DifferentialRevision extends DifferentialDAO
PhabricatorPolicyInterface,
PhabricatorFlaggableInterface,
PhrequentTrackableInterface,
HarbormasterBuildableInterface {
HarbormasterBuildableInterface,
PhabricatorSubscribableInterface {
protected $title = '';
protected $originalTitle;
@ -39,7 +40,6 @@ final class DifferentialRevision extends DifferentialDAO
private $reviewerStatus = self::ATTACHABLE;
const RELATIONSHIP_TABLE = 'differential_relationship';
const TABLE_COMMIT = 'differential_commit';
const RELATION_REVIEWER = 'revw';
@ -187,12 +187,6 @@ final class DifferentialRevision extends DifferentialDAO
$conn_w = $this->establishConnection('w');
queryfx(
$conn_w,
'DELETE FROM %T WHERE revisionID = %d',
self::RELATIONSHIP_TABLE,
$this->getID());
queryfx(
$conn_w,
'DELETE FROM %T WHERE revisionID = %d',
@ -240,22 +234,24 @@ final class DifferentialRevision extends DifferentialDAO
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 @@ final class DifferentialRevision extends DifferentialDAO
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 @@ final class DifferentialRevision extends DifferentialDAO
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;
}
}