From 4fd81150bec2c73bff38c5af58c21fd966c19557 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Dec 2011 11:13:30 -0800 Subject: [PATCH] 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 --- resources/sql/patches/083.dxviewtime.sql | 1 + src/__phutil_library_map__.php | 2 - .../DifferentialRevisionListController.php | 10 ----- .../DifferentialRevisionViewController.php | 12 ------ .../controller/revisionview/__init__.php | 2 - .../DifferentialRevisionListData.php | 42 ------------------ .../data/revisionlist/__init__.php | 1 - .../storage/revision/DifferentialRevision.php | 1 - .../storage/viewtime/DifferentialViewTime.php | 43 ------------------- .../storage/viewtime/__init__.php | 13 ------ 10 files changed, 1 insertion(+), 126 deletions(-) create mode 100644 resources/sql/patches/083.dxviewtime.sql delete mode 100644 src/applications/differential/storage/viewtime/DifferentialViewTime.php delete mode 100644 src/applications/differential/storage/viewtime/__init__.php diff --git a/resources/sql/patches/083.dxviewtime.sql b/resources/sql/patches/083.dxviewtime.sql new file mode 100644 index 0000000000..b33b74eb80 --- /dev/null +++ b/resources/sql/patches/083.dxviewtime.sql @@ -0,0 +1 @@ +DROP TABLE phabricator_differential.differential_viewtime; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 13a9920002..5c8901836b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -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', diff --git a/src/applications/differential/controller/revisionlist/DifferentialRevisionListController.php b/src/applications/differential/controller/revisionlist/DifferentialRevisionListController.php index 3d4332664c..d6b3e48d47 100644 --- a/src/applications/differential/controller/revisionlist/DifferentialRevisionListController.php +++ b/src/applications/differential/controller/revisionlist/DifferentialRevisionListController.php @@ -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', - ), - ), - ), '
' ); } diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 3daa315afe..6fb4f33d24 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -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, diff --git a/src/applications/differential/controller/revisionview/__init__.php b/src/applications/differential/controller/revisionview/__init__.php index 0218e5abf0..90cd2eb579 100644 --- a/src/applications/differential/controller/revisionview/__init__.php +++ b/src/applications/differential/controller/revisionview/__init__.php @@ -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'); diff --git a/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php b/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php index 2b81a817f3..cac9b86ed2 100644 --- a/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php +++ b/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php @@ -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()); } diff --git a/src/applications/differential/data/revisionlist/__init__.php b/src/applications/differential/data/revisionlist/__init__.php index 846363efa2..166a7864cf 100644 --- a/src/applications/differential/data/revisionlist/__init__.php +++ b/src/applications/differential/data/revisionlist/__init__.php @@ -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'); diff --git a/src/applications/differential/storage/revision/DifferentialRevision.php b/src/applications/differential/storage/revision/DifferentialRevision.php index e6c12c4ef8..d16452ae65 100644 --- a/src/applications/differential/storage/revision/DifferentialRevision.php +++ b/src/applications/differential/storage/revision/DifferentialRevision.php @@ -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'; diff --git a/src/applications/differential/storage/viewtime/DifferentialViewTime.php b/src/applications/differential/storage/viewtime/DifferentialViewTime.php deleted file mode 100644 index 2df8075781..0000000000 --- a/src/applications/differential/storage/viewtime/DifferentialViewTime.php +++ /dev/null @@ -1,43 +0,0 @@ - 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; - } -} - diff --git a/src/applications/differential/storage/viewtime/__init__.php b/src/applications/differential/storage/viewtime/__init__.php deleted file mode 100644 index 905e57f5c9..0000000000 --- a/src/applications/differential/storage/viewtime/__init__.php +++ /dev/null @@ -1,13 +0,0 @@ -