Page MenuHomePhabricator

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

Authored by epriestley on Apr 22 2019, 2:27 PM.



Ref T13266. See

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.


  • 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

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 22 2019, 2:27 PM
epriestley requested review of this revision.Apr 22 2019, 2:28 PM
hskiba added a subscriber: hskiba.Apr 23 2019, 1:15 AM
amckinley accepted this revision.Apr 23 2019, 4:28 PM
This revision is now accepted and ready to land.Apr 23 2019, 4:28 PM
This revision was automatically updated to reflect the committed changes.