Page MenuHomePhabricator

Build a rough transaction-level view of Feed
ClosedPublic

Authored by epriestley on Mon, May 20, 9:23 PM.

Details

Summary

Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".

We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.

This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.

To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.

To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.

Test Plan

Diff Detail

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

Event Timeline

epriestley created this revision.Mon, May 20, 9:23 PM
epriestley requested review of this revision.Mon, May 20, 9:24 PM
amckinley accepted this revision.Tue, May 21, 6:37 PM
amckinley added inline comments.
src/applications/feed/controller/PhabricatorFeedController.php
11–13

Weird that we instantiate this object and immediately drop a reference to it. Also, I think the important part of this got moved faithfully to PhabricatorFeedListController, but just wanted to make sure it was intentional to lose this SideNavFilterView part.

This revision is now accepted and ready to land.Tue, May 21, 6:37 PM
epriestley added inline comments.Tue, May 21, 7:27 PM
src/applications/feed/controller/PhabricatorFeedController.php
11–13

The new behavior should be faithful to the old behavior, just with a simpler implementation.

The old behavior, with delegateToController(), required more boilerplate and this weird instantiation just to call addNavigationItems(), which is counterintuitively "take the navigation items from THIS object and add them to some other menu".

The new behavior, with SearchEngine->buildResponse(), achieves the same effect with a simpler API, and changes to the "Hamburger" menu connected to T13247 mean that we don't need to do buildApplicationMenu() gymnastics anymore to get a proper mobile menu.

Possible I missed something since this code is pretty old and this upgrade isn't a routine upgrade that I'm doing regularly, but I'm nearly sure the new code is the old code but better.

Stuff I tested:

  • /feed/ has the same menu.
  • /feed/ has the right hamburger menu on mobile.
  • No other page in Feed has any menus (same behavior before/after).
This revision was automatically updated to reflect the committed changes.