mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-29 18:22:41 +01:00
Fix an issue where internal paging of notifications could fail if some notifications are not visible
Summary: Ref T13266. See <https://discourse.phabricator-community.org/t/notification-page-throws-unrecoverable-fatal-error/2651/>. The "notifications" query currently uses offset paging for no apparent reason (just a legacy issue?), so some of the paging code is only reachable internally. - Stop it from using offset paging, since modern cursor paging is fine here (and Feed has used cursor paging for a long time). - Fix the non-offset paging to work like Feed. Also: - Remove a couple of stub methods with no callsites after cursor refactoring. Test Plan: - Set things up so I had more than 100 notifications and some in the first 100 were policy filtered, to reproduce the issue (I just made `FeedStory` return `NO_ONE` as a visibility policy). - Applied the patch, notifications now page cleanly. - Verified that "Next Page" took me to the right place in the result list. Reviewers: amckinley Reviewed By: amckinley Subscribers: hskiba Maniphest Tasks: T13266 Differential Revision: https://secure.phabricator.com/D20455
This commit is contained in:
parent
a82a2b8459
commit
a4a1143b18
4 changed files with 53 additions and 22 deletions
|
@ -170,10 +170,4 @@ final class PhabricatorApplicationQuery
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
protected function getResultCursor($object) {
|
|
||||||
// TODO: This won't work, but doesn't matter until we write more than 100
|
|
||||||
// applications. Since we only have about 70, just avoid fataling for now.
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -53,10 +53,10 @@ final class PhabricatorNotificationQuery
|
||||||
|
|
||||||
$data = queryfx_all(
|
$data = queryfx_all(
|
||||||
$conn,
|
$conn,
|
||||||
'SELECT story.*, notif.hasViewed FROM %R notif
|
'SELECT story.*, notification.hasViewed FROM %R notification
|
||||||
JOIN %R story ON notif.chronologicalKey = story.chronologicalKey
|
JOIN %R story ON notification.chronologicalKey = story.chronologicalKey
|
||||||
%Q
|
%Q
|
||||||
ORDER BY notif.chronologicalKey DESC
|
ORDER BY notification.chronologicalKey DESC
|
||||||
%Q',
|
%Q',
|
||||||
$notification_table,
|
$notification_table,
|
||||||
$story_table,
|
$story_table,
|
||||||
|
@ -82,21 +82,21 @@ final class PhabricatorNotificationQuery
|
||||||
if ($this->userPHIDs !== null) {
|
if ($this->userPHIDs !== null) {
|
||||||
$where[] = qsprintf(
|
$where[] = qsprintf(
|
||||||
$conn,
|
$conn,
|
||||||
'notif.userPHID IN (%Ls)',
|
'notification.userPHID IN (%Ls)',
|
||||||
$this->userPHIDs);
|
$this->userPHIDs);
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($this->unread !== null) {
|
if ($this->unread !== null) {
|
||||||
$where[] = qsprintf(
|
$where[] = qsprintf(
|
||||||
$conn,
|
$conn,
|
||||||
'notif.hasViewed = %d',
|
'notification.hasViewed = %d',
|
||||||
(int)!$this->unread);
|
(int)!$this->unread);
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($this->keys !== null) {
|
if ($this->keys !== null) {
|
||||||
$where[] = qsprintf(
|
$where[] = qsprintf(
|
||||||
$conn,
|
$conn,
|
||||||
'notif.chronologicalKey IN (%Ls)',
|
'notification.chronologicalKey IN (%Ls)',
|
||||||
$this->keys);
|
$this->keys);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -113,8 +113,53 @@ final class PhabricatorNotificationQuery
|
||||||
return $stories;
|
return $stories;
|
||||||
}
|
}
|
||||||
|
|
||||||
protected function getResultCursor($item) {
|
protected function getDefaultOrderVector() {
|
||||||
return $item->getChronologicalKey();
|
return array('key');
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getBuiltinOrders() {
|
||||||
|
return array(
|
||||||
|
'newest' => array(
|
||||||
|
'vector' => array('key'),
|
||||||
|
'name' => pht('Creation (Newest First)'),
|
||||||
|
'aliases' => array('created'),
|
||||||
|
),
|
||||||
|
'oldest' => array(
|
||||||
|
'vector' => array('-key'),
|
||||||
|
'name' => pht('Creation (Oldest First)'),
|
||||||
|
),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getOrderableColumns() {
|
||||||
|
return array(
|
||||||
|
'key' => array(
|
||||||
|
'table' => 'notification',
|
||||||
|
'column' => 'chronologicalKey',
|
||||||
|
'type' => 'string',
|
||||||
|
'unique' => true,
|
||||||
|
),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
protected function applyExternalCursorConstraintsToQuery(
|
||||||
|
PhabricatorCursorPagedPolicyAwareQuery $subquery,
|
||||||
|
$cursor) {
|
||||||
|
$subquery->withKeys(array($cursor));
|
||||||
|
}
|
||||||
|
|
||||||
|
protected function newExternalCursorStringForResult($object) {
|
||||||
|
return $object->getChronologicalKey();
|
||||||
|
}
|
||||||
|
|
||||||
|
protected function newPagingMapFromPartialObject($object) {
|
||||||
|
return array(
|
||||||
|
'key' => $object->getChronologicalKey(),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
protected function getPrimaryTableAlias() {
|
||||||
|
return 'notification';
|
||||||
}
|
}
|
||||||
|
|
||||||
public function getQueryApplicationClass() {
|
public function getQueryApplicationClass() {
|
||||||
|
|
|
@ -134,8 +134,4 @@ final class PhabricatorNotificationSearchEngine
|
||||||
return $result;
|
return $result;
|
||||||
}
|
}
|
||||||
|
|
||||||
public function shouldUseOffsetPaging() {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -46,8 +46,4 @@ final class PhabricatorEditEngineQuery
|
||||||
return 'PhabricatorTransactionsApplication';
|
return 'PhabricatorTransactionsApplication';
|
||||||
}
|
}
|
||||||
|
|
||||||
protected function getResultCursor($object) {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue