Page MenuHomePhabricator

Fix an issue where internal paging of notifications could fail if some notifications are not visible
ClosedPublic

Authored by epriestley on Apr 22 2019, 2:27 PM.
Tags
None
Referenced Files
F13082159: D20455.diff
Wed, Apr 24, 10:01 PM
Unknown Object (File)
Sun, Apr 21, 4:07 PM
Unknown Object (File)
Sat, Apr 20, 4:47 PM
Unknown Object (File)
Wed, Apr 17, 3:08 PM
Unknown Object (File)
Thu, Apr 11, 7:18 AM
Unknown Object (File)
Mon, Apr 1, 7:21 PM
Unknown Object (File)
Sun, Mar 31, 11:58 PM
Unknown Object (File)
Mar 19 2024, 3:16 PM
Subscribers

Details

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.

Diff Detail

Repository
rP Phabricator
Branch
nquery1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22681
Build 31082: Run Core Tests
Build 31081: arc lint + arc unit