mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-10 00:42:41 +01:00
Remove "Updated" view from Differential
Summary: This landed during my review drama embargo and is a generally good idea but had some implementation issues. @elynde reports it has been broken for some time, although it still works on secure.phabricator.com so I'm guessing it's just taking a zillion years to run at Facebook. It's up to more than a second for me on secure.phabricator.com: https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/ The basic problem is that some of the data architecture around this implementation is hard to scale. I want to pursue a similar feature eventually, but drive it off notifications that we'll ship through real-time infrastructure too. I'm also trying to get rid of DifferentialRevisionListData and this simplifies that somewhat. Test Plan: - Grepped for table name, table constant, query constant, and class name; no hits. - Applied SQL patch. - Verified that Differential no longer shows "Updated". Reviewers: elynde, btrahan, jungejason Reviewed By: elynde CC: aran, elynde Differential Revision: 1178
This commit is contained in:
parent
74f710a437
commit
4fd81150be
10 changed files with 1 additions and 126 deletions
1
resources/sql/patches/083.dxviewtime.sql
Normal file
1
resources/sql/patches/083.dxviewtime.sql
Normal file
|
@ -0,0 +1 @@
|
|||
DROP TABLE phabricator_differential.differential_viewtime;
|
|
@ -239,7 +239,6 @@ phutil_register_library_map(array(
|
|||
'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/unit',
|
||||
'DifferentialUnitStatus' => 'applications/differential/constants/unitstatus',
|
||||
'DifferentialUnitTestResult' => 'applications/differential/constants/unittestresult',
|
||||
'DifferentialViewTime' => 'applications/differential/storage/viewtime',
|
||||
'DiffusionBranchInformation' => 'applications/diffusion/data/branch',
|
||||
'DiffusionBranchQuery' => 'applications/diffusion/query/branch/base',
|
||||
'DiffusionBranchTableView' => 'applications/diffusion/view/branchtable',
|
||||
|
@ -937,7 +936,6 @@ phutil_register_library_map(array(
|
|||
'DifferentialTestPlanFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialTitleFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialUnitFieldSpecification' => 'DifferentialFieldSpecification',
|
||||
'DifferentialViewTime' => 'DifferentialDAO',
|
||||
'DiffusionBranchTableView' => 'DiffusionView',
|
||||
'DiffusionBrowseController' => 'DiffusionController',
|
||||
'DiffusionBrowseFileController' => 'DiffusionController',
|
||||
|
|
|
@ -97,16 +97,6 @@ class DifferentialRevisionListController extends DifferentialController {
|
|||
),
|
||||
),
|
||||
),
|
||||
'updates' => array(
|
||||
'name' => 'Updates',
|
||||
'queries' => array(
|
||||
array(
|
||||
'query' => DifferentialRevisionListData::QUERY_UPDATED_SINCE,
|
||||
'header' =>
|
||||
'Diffs that have been updated since you\'ve last viewed them',
|
||||
),
|
||||
),
|
||||
),
|
||||
'<hr />'
|
||||
);
|
||||
}
|
||||
|
|
|
@ -244,8 +244,6 @@ class DifferentialRevisionViewController extends DifferentialController {
|
|||
$comment_form->setActionURI('/differential/comment/save/');
|
||||
$comment_form->setUser($user);
|
||||
$comment_form->setDraft($draft);
|
||||
|
||||
$this->updateViewTime($user->getPHID(), $revision->getPHID());
|
||||
}
|
||||
|
||||
$pane_id = celerity_generate_unique_node_id();
|
||||
|
@ -539,16 +537,6 @@ class DifferentialRevisionViewController extends DifferentialController {
|
|||
return array($changesets, $vs_map, $refs);
|
||||
}
|
||||
|
||||
private function updateViewTime($user_phid, $revision_phid) {
|
||||
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
|
||||
$view_time =
|
||||
id(new DifferentialViewTime())
|
||||
->setViewerPHID($user_phid)
|
||||
->setObjectPHID($revision_phid)
|
||||
->setViewTime(time())
|
||||
->replace();
|
||||
}
|
||||
|
||||
private function loadAuxiliaryFieldsAndProperties(
|
||||
DifferentialRevision $revision,
|
||||
DifferentialDiff $diff,
|
||||
|
|
|
@ -7,7 +7,6 @@
|
|||
|
||||
|
||||
phutil_require_module('phabricator', 'aphront/response/404');
|
||||
phutil_require_module('phabricator', 'aphront/writeguard');
|
||||
phutil_require_module('phabricator', 'applications/differential/constants/action');
|
||||
phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
|
||||
phutil_require_module('phabricator', 'applications/differential/controller/base');
|
||||
|
@ -19,7 +18,6 @@ phutil_require_module('phabricator', 'applications/differential/storage/comment'
|
|||
phutil_require_module('phabricator', 'applications/differential/storage/diffproperty');
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/revision');
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/viewtime');
|
||||
phutil_require_module('phabricator', 'applications/differential/view/addcomment');
|
||||
phutil_require_module('phabricator', 'applications/differential/view/changesetlistview');
|
||||
phutil_require_module('phabricator', 'applications/differential/view/difftableofcontents');
|
||||
|
|
|
@ -29,7 +29,6 @@ class DifferentialRevisionListData {
|
|||
const QUERY_PHIDS = 'phids';
|
||||
const QUERY_CC = 'cc';
|
||||
const QUERY_ALL_OPEN = 'all-open';
|
||||
const QUERY_UPDATED_SINCE = 'updated-since';
|
||||
|
||||
private $ids;
|
||||
private $filter;
|
||||
|
@ -194,9 +193,6 @@ class DifferentialRevisionListData {
|
|||
'revision.phid in (%Ls)',
|
||||
$this->ids);
|
||||
break;
|
||||
case self::QUERY_UPDATED_SINCE:
|
||||
$this->revisions = $this->loadAllUpdated();
|
||||
break;
|
||||
}
|
||||
|
||||
return $this->revisions;
|
||||
|
@ -210,44 +206,6 @@ class DifferentialRevisionListData {
|
|||
);
|
||||
}
|
||||
|
||||
private function loadAllUpdated() {
|
||||
$revision = new DifferentialRevision();
|
||||
$min_view_time = (int)PhabricatorEnv::getEnvConfig('updates.min-view-time');
|
||||
|
||||
$data = queryfx_all(
|
||||
$revision->establishConnection('r'),
|
||||
'SELECT revs.* FROM (
|
||||
(
|
||||
SELECT revision.*
|
||||
FROM %T revision
|
||||
WHERE revision.authorPHID in (%Ls)
|
||||
)
|
||||
UNION
|
||||
(
|
||||
SELECT revision.*
|
||||
FROM %T revision
|
||||
JOIN %T rel
|
||||
WHERE rel.revisionId = revision.Id AND rel.objectPHID in (%Ls)
|
||||
)
|
||||
) as revs
|
||||
LEFT JOIN %T viewtime ON
|
||||
viewtime.viewerPHID in (%Ls)
|
||||
AND viewtime.objectPHID = revs.phid
|
||||
WHERE GREATEST(%d, IFNULL(viewtime.viewTime, 0)) < revs.dateModified
|
||||
%Q',
|
||||
$revision->getTableName(),
|
||||
$this->ids,
|
||||
$revision->getTableName(),
|
||||
DifferentialRevision::RELATIONSHIP_TABLE,
|
||||
$this->ids,
|
||||
DifferentialRevision::TABLE_VIEW_TIME,
|
||||
$this->ids,
|
||||
$min_view_time,
|
||||
$this->getOrderClause());
|
||||
return $revision->loadAllFromArray($data);
|
||||
}
|
||||
|
||||
|
||||
private function loadAllOpen() {
|
||||
return $this->loadAllWhere('status in (%Ld)', $this->getOpenStatuses());
|
||||
}
|
||||
|
|
|
@ -8,7 +8,6 @@
|
|||
|
||||
phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/revision');
|
||||
phutil_require_module('phabricator', 'infrastructure/env');
|
||||
phutil_require_module('phabricator', 'storage/queryfx');
|
||||
|
||||
phutil_require_module('phutil', 'utils');
|
||||
|
|
|
@ -39,7 +39,6 @@ class DifferentialRevision extends DifferentialDAO {
|
|||
private $commits;
|
||||
|
||||
const RELATIONSHIP_TABLE = 'differential_relationship';
|
||||
const TABLE_VIEW_TIME = 'differential_viewtime';
|
||||
const TABLE_COMMIT = 'differential_commit';
|
||||
|
||||
const RELATION_REVIEWER = 'revw';
|
||||
|
|
|
@ -1,43 +0,0 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* Copyright 2011 Facebook, Inc.
|
||||
*
|
||||
* Licensed under the Apache License, Version 2.0 (the "License");
|
||||
* you may not use this file except in compliance with the License.
|
||||
* You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing, software
|
||||
* distributed under the License is distributed on an "AS IS" BASIS,
|
||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
* See the License for the specific language governing permissions and
|
||||
* limitations under the License.
|
||||
*/
|
||||
|
||||
class DifferentialViewTime extends DifferentialDAO {
|
||||
|
||||
protected
|
||||
$viewerPHID,
|
||||
$objectPHID,
|
||||
$viewTime;
|
||||
|
||||
protected function getConfiguration() {
|
||||
return array(
|
||||
self::CONFIG_OPTIMISTIC_LOCKS => false,
|
||||
self::CONFIG_IDS => self::IDS_MANUAL,
|
||||
self::CONFIG_TIMESTAMPS => false,
|
||||
);
|
||||
}
|
||||
|
||||
// Primary key is (viewerPHID, objectPHID)
|
||||
public function getIDKey() {
|
||||
return null;
|
||||
}
|
||||
|
||||
protected function shouldInsertWhenSaved() {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
|
@ -1,13 +0,0 @@
|
|||
<?php
|
||||
/**
|
||||
* This file is automatically generated. Lint this module to rebuild it.
|
||||
* @generated
|
||||
*/
|
||||
|
||||
|
||||
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/base');
|
||||
|
||||
|
||||
phutil_require_source('DifferentialViewTime.php');
|
||||
|
Loading…
Reference in a new issue