From 15b41f56393490f526053f0828c64dc7a9a9b190 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 11 Apr 2015 08:50:50 -0700 Subject: [PATCH] Remove Herald rule edit log Summary: Fixes T7601. Ref T7803, weakly (this removes a Query subclass with ad-hoc paging). Herald has a very old edit log which predates transactions and is essentially useless and not really policy-aware. I think it's doing more harm than good; remove it. Herald rules have proper transactions, but rule edits don't currently render something nice into the transaction log. This is definitely the way forward, but we haven't seen requests for this so don't bother building it for now. I did put a nice end-cap on the transaction log, though. Test Plan: - Viewed Herald UI. - Grepped for removed classes and methods. - Edited a rule. - Viewed rule transaction log. Reviewers: btrahan Reviewed By: btrahan Subscribers: cburroughs, chad, epriestley Maniphest Tasks: T7601, T7803 Differential Revision: https://secure.phabricator.com/D12346 --- .../sql/autopatches/20150410.nukeruleedit.sql | 1 + src/__phutil_library_map__.php | 8 --- .../PhabricatorHeraldApplication.php | 1 - .../herald/controller/HeraldController.php | 5 +- .../controller/HeraldRuleController.php | 1 - .../HeraldRuleEditHistoryController.php | 55 ---------------- .../controller/HeraldRuleViewController.php | 1 + .../herald/query/HeraldEditLogQuery.php | 48 -------------- .../herald/storage/HeraldRule.php | 20 ------ .../herald/storage/HeraldRuleEdit.php | 24 ------- .../herald/view/HeraldRuleEditHistoryView.php | 63 ------------------- 11 files changed, 4 insertions(+), 223 deletions(-) create mode 100644 resources/sql/autopatches/20150410.nukeruleedit.sql delete mode 100644 src/applications/herald/controller/HeraldRuleEditHistoryController.php delete mode 100644 src/applications/herald/query/HeraldEditLogQuery.php delete mode 100644 src/applications/herald/storage/HeraldRuleEdit.php delete mode 100644 src/applications/herald/view/HeraldRuleEditHistoryView.php diff --git a/resources/sql/autopatches/20150410.nukeruleedit.sql b/resources/sql/autopatches/20150410.nukeruleedit.sql new file mode 100644 index 0000000000..29302c75b6 --- /dev/null +++ b/resources/sql/autopatches/20150410.nukeruleedit.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_herald.herald_ruleedit; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 74f5b931f6..0fedc28f14 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -902,7 +902,6 @@ phutil_register_library_map(array( 'HeraldDifferentialDiffAdapter' => 'applications/herald/adapter/HeraldDifferentialDiffAdapter.php', 'HeraldDifferentialRevisionAdapter' => 'applications/herald/adapter/HeraldDifferentialRevisionAdapter.php', 'HeraldDisableController' => 'applications/herald/controller/HeraldDisableController.php', - 'HeraldEditLogQuery' => 'applications/herald/query/HeraldEditLogQuery.php', 'HeraldEffect' => 'applications/herald/engine/HeraldEffect.php', 'HeraldEngine' => 'applications/herald/engine/HeraldEngine.php', 'HeraldInvalidActionException' => 'applications/herald/engine/exception/HeraldInvalidActionException.php', @@ -920,9 +919,6 @@ phutil_register_library_map(array( 'HeraldRepetitionPolicyConfig' => 'applications/herald/config/HeraldRepetitionPolicyConfig.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', - 'HeraldRuleEdit' => 'applications/herald/storage/HeraldRuleEdit.php', - 'HeraldRuleEditHistoryController' => 'applications/herald/controller/HeraldRuleEditHistoryController.php', - 'HeraldRuleEditHistoryView' => 'applications/herald/view/HeraldRuleEditHistoryView.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', @@ -4146,7 +4142,6 @@ phutil_register_library_map(array( 'HeraldDifferentialDiffAdapter' => 'HeraldDifferentialAdapter', 'HeraldDifferentialRevisionAdapter' => 'HeraldDifferentialAdapter', 'HeraldDisableController' => 'HeraldController', - 'HeraldEditLogQuery' => 'PhabricatorOffsetPagedQuery', 'HeraldInvalidActionException' => 'Exception', 'HeraldInvalidConditionException' => 'Exception', 'HeraldManageGlobalRulesCapability' => 'PhabricatorPolicyCapability', @@ -4166,9 +4161,6 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', ), 'HeraldRuleController' => 'HeraldController', - 'HeraldRuleEdit' => 'HeraldDAO', - 'HeraldRuleEditHistoryController' => 'HeraldController', - 'HeraldRuleEditHistoryView' => 'AphrontView', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleListController' => 'HeraldController', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', diff --git a/src/applications/herald/application/PhabricatorHeraldApplication.php b/src/applications/herald/application/PhabricatorHeraldApplication.php index f364117fba..b74a27d9f2 100644 --- a/src/applications/herald/application/PhabricatorHeraldApplication.php +++ b/src/applications/herald/application/PhabricatorHeraldApplication.php @@ -54,7 +54,6 @@ final class PhabricatorHeraldApplication extends PhabricatorApplication { 'edit/(?:(?P[1-9]\d*)/)?' => 'HeraldRuleController', 'disable/(?P[1-9]\d*)/(?P\w+)/' => 'HeraldDisableController', - 'history/(?:(?P[1-9]\d*)/)?' => 'HeraldRuleEditHistoryController', 'test/' => 'HeraldTestConsoleController', 'transcript/' => array( '' => 'HeraldTranscriptListController', diff --git a/src/applications/herald/controller/HeraldController.php b/src/applications/herald/controller/HeraldController.php index 71d7c3b3b9..b363232ca5 100644 --- a/src/applications/herald/controller/HeraldController.php +++ b/src/applications/herald/controller/HeraldController.php @@ -48,9 +48,8 @@ abstract class HeraldController extends PhabricatorController { $nav ->addLabel(pht('Utilities')) - ->addFilter('test', pht('Test Console')) - ->addFilter('transcript', pht('Transcripts')) - ->addFilter('history', pht('Edit Log')); + ->addFilter('test', pht('Test Console')) + ->addFilter('transcript', pht('Transcripts')); $nav->selectFilter(null); diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 27ace9dd2f..c16a45b47e 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -328,7 +328,6 @@ final class HeraldRuleController extends HeraldController { $rule->save(); $rule->saveConditions($conditions); $rule->saveActions($actions); - $rule->logEdit($request->getUser()->getPHID(), $edit_action); $rule->saveTransaction(); } diff --git a/src/applications/herald/controller/HeraldRuleEditHistoryController.php b/src/applications/herald/controller/HeraldRuleEditHistoryController.php deleted file mode 100644 index 9b816cee28..0000000000 --- a/src/applications/herald/controller/HeraldRuleEditHistoryController.php +++ /dev/null @@ -1,55 +0,0 @@ -id = idx($data, 'id'); - } - - public function processRequest() { - $request = $this->getRequest(); - - $edit_query = new HeraldEditLogQuery(); - if ($this->id) { - $edit_query->withRuleIDs(array($this->id)); - } - - $pager = new AphrontPagerView(); - $pager->setURI($request->getRequestURI(), 'offset'); - $pager->setOffset($request->getStr('offset')); - - $edits = $edit_query->executeWithOffsetPager($pager); - - $need_phids = mpull($edits, 'getEditorPHID'); - $handles = $this->loadViewerHandles($need_phids); - - $list_view = id(new HeraldRuleEditHistoryView()) - ->setEdits($edits) - ->setHandles($handles) - ->setUser($this->getRequest()->getUser()); - - $panel = new PHUIObjectBoxView(); - $panel->setHeaderText(pht('Edit History')); - $panel->appendChild($list_view); - - $crumbs = $this - ->buildApplicationCrumbs($can_create = false) - ->addTextCrumb( - pht('Edit History'), - $this->getApplicationURI('herald/history')); - - $nav = $this->buildSideNavView(); - $nav->selectFilter('history'); - $nav->appendChild($panel); - $nav->setCrumbs($crumbs); - - return $this->buildApplicationPage( - $nav, - array( - 'title' => pht('Rule Edit History'), - )); - } - -} diff --git a/src/applications/herald/controller/HeraldRuleViewController.php b/src/applications/herald/controller/HeraldRuleViewController.php index 636650ad2d..20f293b67d 100644 --- a/src/applications/herald/controller/HeraldRuleViewController.php +++ b/src/applications/herald/controller/HeraldRuleViewController.php @@ -53,6 +53,7 @@ final class HeraldRuleViewController extends HeraldController { $timeline = $this->buildTransactionTimeline( $rule, new HeraldTransactionQuery()); + $timeline->setShouldTerminate(true); return $this->buildApplicationPage( array( diff --git a/src/applications/herald/query/HeraldEditLogQuery.php b/src/applications/herald/query/HeraldEditLogQuery.php deleted file mode 100644 index 42db5f4db2..0000000000 --- a/src/applications/herald/query/HeraldEditLogQuery.php +++ /dev/null @@ -1,48 +0,0 @@ -ruleIDs = $rule_ids; - return $this; - } - - public function execute() { - $table = new HeraldRuleEdit(); - $conn_r = $table->establishConnection('r'); - - $where = $this->buildWhereClause($conn_r); - $order = $this->buildOrderClause($conn_r); - $limit = $this->buildLimitClause($conn_r); - - $data = queryfx_all( - $conn_r, - 'SELECT log.* FROM %T log %Q %Q %Q', - $table->getTableName(), - $where, - $order, - $limit); - - return $table->loadAllFromArray($data); - } - - private function buildWhereClause($conn_r) { - $where = array(); - - if ($this->ruleIDs) { - $where[] = qsprintf( - $conn_r, - 'ruleID IN (%Ld)', - $this->ruleIDs); - } - - return $this->formatWhereClause($where); - } - - private function buildOrderClause($conn_r) { - return 'ORDER BY id DESC'; - } - -} diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 1c1623c5bf..2d19bc5356 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -115,26 +115,6 @@ final class HeraldRule extends HeraldDAO return $this->actions; } - public function loadEdits() { - if (!$this->getID()) { - return array(); - } - $edits = id(new HeraldRuleEdit())->loadAllWhere( - 'ruleID = %d ORDER BY dateCreated DESC', - $this->getID()); - - return $edits; - } - - public function logEdit($editor_phid, $action) { - id(new HeraldRuleEdit()) - ->setRuleID($this->getID()) - ->setRuleName($this->getName()) - ->setEditorPHID($editor_phid) - ->setAction($action) - ->save(); - } - public function saveConditions(array $conditions) { assert_instances_of($conditions, 'HeraldCondition'); return $this->saveChildren( diff --git a/src/applications/herald/storage/HeraldRuleEdit.php b/src/applications/herald/storage/HeraldRuleEdit.php deleted file mode 100644 index 735ae145ac..0000000000 --- a/src/applications/herald/storage/HeraldRuleEdit.php +++ /dev/null @@ -1,24 +0,0 @@ - array( - 'ruleName' => 'text255', - 'action' => 'text32', - ), - self::CONFIG_KEY_SCHEMA => array( - 'ruleID' => array( - 'columns' => array('ruleID', 'dateCreated'), - ), - ), - ) + parent::getConfiguration(); - } - -} diff --git a/src/applications/herald/view/HeraldRuleEditHistoryView.php b/src/applications/herald/view/HeraldRuleEditHistoryView.php deleted file mode 100644 index 0ca5a6fc5a..0000000000 --- a/src/applications/herald/view/HeraldRuleEditHistoryView.php +++ /dev/null @@ -1,63 +0,0 @@ -edits = $edits; - return $this; - } - - public function getEdits() { - return $this->edits; - } - - public function setHandles(array $handles) { - assert_instances_of($handles, 'PhabricatorObjectHandle'); - $this->handles = $handles; - return $this; - } - - public function render() { - $list = new PHUIObjectItemListView(); - $list->setFlush(true); - - foreach ($this->edits as $edit) { - $name = nonempty($edit->getRuleName(), 'Unknown Rule'); - $rule_name = phutil_tag( - 'strong', - array(), - $name); - - switch ($edit->getAction()) { - case 'create': - $details = pht("Created rule '%s'.", $rule_name); - break; - case 'delete': - $details = pht("Deleted rule '%s'.", $rule_name); - break; - case 'edit': - default: - $details = pht("Edited rule '%s'.", $rule_name); - break; - } - - $editor = $this->handles[$edit->getEditorPHID()]->renderLink(); - $date = phabricator_datetime($edit->getDateCreated(), $this->user); - - $item = id(new PHUIObjectItemView()) - ->setObjectName(pht('Rule %d', $edit->getRuleID())) - ->setSubHead($details) - ->addIcon('none', $date) - ->addByLine(pht('Editor: %s', $editor)); - - $list->addItem($item); - } - - $list->setNoDataString(pht('No edits for rule.')); - - return $list; - } -}