Page MenuHomePhabricator

Transactions - add pagination to application transactions
ClosedPublic

Authored by btrahan on Nov 20 2014, 8:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 10:39 PM
Unknown Object (File)
Mon, Mar 25, 10:39 PM
Unknown Object (File)
Mon, Mar 25, 10:39 PM
Unknown Object (File)
Mon, Mar 25, 10:38 PM
Unknown Object (File)
Mon, Mar 25, 10:38 PM
Unknown Object (File)
Fri, Mar 22, 2:16 PM
Unknown Object (File)
Sun, Mar 10, 11:14 PM
Unknown Object (File)
Feb 22 2024, 8:09 AM
Subscribers

Details

Summary

Ref T4712. This adds pagination. Future diffs will need to deploy buildTransactionTimeline everywhere and massage this stuff as necessary if we hit any special cases.

Test Plan

Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Checkpointing - add pagination to application transactions.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

This looks good so far.

Since we don't actually care about the individual nodes in the response, I think you could just send back a giant block of HTML (e.g., use hsprintf('%s', $events) to produce one) and then dump it in with something like:

JX.DOM.replace(swap, JX.$H(r.timeline).getFragment());

That might make it a little easier?

btrahan edited edge metadata.

works now. thanks for the js tip!

btrahan retitled this revision from Checkpointing - add pagination to application transactions to Transactions - add pagination to application transactions.Nov 21 2014, 12:38 AM
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.

We can probably clean all that reverse paging stuff up once everything moves over to this, I think it was just a lazy way for me to not have to array_reverse() a dozen times in every application.

src/applications/transactions/controller/PhabricatorApplicationTransactionShowOlderController.php
7–17

Oh, you could do the slightly more modern handleRequest(AphrontRequest $request) + $request->getURIData('phid') thing here if you want.

28

Maybe 404 if the object doesn't implement PhabricatorApplicationTransactionInterface?

This revision is now accepted and ready to land.Nov 21 2014, 12:39 AM

This isn't quite ready yet. I forgot about the case where you have some transaction ID in the URI and that transaction ID is in the middle such that you have some "show newer" UI at the bottom. This theoretically breaks without this so I am going to get this part right before checkin. I also want to restore the highlight feature which I currently broke with my if (pager) new_js else old_js code.

btrahan edited edge metadata.
  • changes as suggested
  • handle the #transactionID case with some javascript wizardry. We'll keep loading prior transactions until we find what we're looking for
This revision is now accepted and ready to land.Nov 21 2014, 7:50 PM
  • fix a small JS race condition bug maybe and return even if we aren't "loading"
  • make sure to specify "with hiding" as false in new ajax endpoint. "with hiding" as true may hide a bunch of transactions based on assumptions about showing the author when they commented most recently... this should be off for this end point.

    (this is really, really, ready now...! :D )

I've been holding this because family is in town and I want to support it aggressively if it breaks anything. I'll probably land it this coming Sunday evening or Monday morning.

Also, @epriestley - if you want to take a look again given the javascript I added since last firm review please do. :D

Yeah, my family stuff is turning out to take like 100% of my time. I'll try to give this another look, but may not really be back in action until late Monday.

This revision was automatically updated to reflect the committed changes.