Page MenuHomePhabricator
Create Task
Maniphest T6693

Unable to view 'older changes'.
Closed, ResolvedPublic
Actions

Description

If we go to a differential like https://phabricator.khanacademy.org/D15010, it says 'older changes are hidden.' When we click on https://phabricator.khanacademy.org/transactions/showolder/PHID-DREV-ozvnqvasqqxgbwiv7t3a/?after=219387 it says

Argument 2 passed to DifferentialTransactionComment::sortAndGroupInlines() must be an array, null given, called in /home/ubuntu/internal-webserver/phabricator/src/applications/differential/view/DifferentialTransactionView.php on line 118 and defined

We expected it to show older comments instead. :-)

Revisions and Commits

rP Phabricator
AbandonedD10929 transactionTimelineCallback a good idea?
D10931rP6ab3f06b6ec6 Transactions - adding willRenderTimeline to handle tricky cases

Event Timeline

csilvers created this task.Dec 4 2014, 7:11 PM
csilvers assigned this task to btrahan.
csilvers raised the priority of this task from to High.
csilvers updated the task description. (Show Details)
csilvers added a project: Differential.
csilvers added a subscriber: csilvers.
btrahan added a comment.Dec 4 2014, 7:53 PM

Code wise, the issue is both Differential and Pholio need to give additional data to their respective timeline objects to render these timelines.

  • Differential
    • current revision
    • left side diff
    • right side diff
    • changesets as grabbed by loadChangesetsAndVsMap
  • Pholio
    • current mock

I think the Differential side of things is significantly complex such that I should revert the change for Differential, and while I'm reverting stuff might as well revert Pholio too. Sound good?

I think the fix might be to have some sort of transactionTimelineCallback($timeline, $request) in PhabricatorApplicationTransactionInterface. It will mostly do nothing (I think setShouldTerminate() calls should stay in the controller?) but for these two bad boys it will be useful. I'm hopeful that I can get the data I need out of $request for the Differential case...? I think for the Differential case there'll be lots of refactoring in DifferentialRevisionViewController to make some of that logic easily in the callback.

btrahan added a revision: D10929: transactionTimelineCallback a good idea?.Dec 4 2014, 8:12 PM
btrahan added a revision: D10931: Transactions - adding willRenderTimeline to handle tricky cases.Dec 4 2014, 9:18 PM
joshuaspence added a subscriber: joshuaspence.EditedDec 4 2014, 9:41 PM

We are also seeing this issue.

btrahan added a comment.Dec 4 2014, 9:44 PM

just like 60 more callsites to go

btrahan closed this task as Resolved.Dec 4 2014, 9:58 PM
btrahan added a commit: rP6ab3f06b6ec6: Transactions - adding willRenderTimeline to handle tricky cases.

Closed by commit rP6ab3f06b6ec6.

btrahan added a comment.Dec 4 2014, 10:00 PM

This should be fixed the fancy way now in HEAD. Please do let us know if you are seeing any issues after updating. :D

Phabricator · Built by Phacility