Page MenuHomePhabricator

In Herald, save applied transaction PHIDs in the transcript and display them in the UI
ClosedPublic

Authored by epriestley on May 13 2019, 5:02 PM.

Details

Summary

Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.

Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.

Test Plan
  • Viewed a modern transcript, saw a list of transactions.
  • Viewed an older transcript, saw nothing (since there were no transactions in the transcript).

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.May 13 2019, 5:02 PM
epriestley requested review of this revision.May 13 2019, 5:04 PM
amckinley accepted this revision.May 15 2019, 5:37 PM
amckinley added inline comments.
src/applications/herald/controller/HeraldTranscriptController.php
30

Shouldn't this just be $object = null? I guess it's not really a behavior change since this behavior is new, but it seems weird that I suddenly won't be able to see a Herald transcript if the view policy on the affected objects changes out from under me. Maybe it doesn't matter because no one is looking at old Herald transcripts and they get GC'd anyway.

This revision is now accepted and ready to land.May 15 2019, 5:37 PM

HeraldTranscriptQuery already requires that you must be able to see the object, it just doesn't actually attach it anywhere. Possibly a cleaner fix is to make it do that.

(Although losing access to an older transcript because you lost access to an object is slightly weird, being able to take any object, go find transcripts for it in Herald, and see some of its fields would be super bad and fairly strongly violate policy controls if anyone had written a rule like "task summary matches regexp ..." so the field value was exposed, so I think it's definitely right that we err on the side of locking you out of things.)

epriestley updated this revision to Diff 48930.May 15 2019, 6:41 PM
  • In HeraldTranscriptQuery, attach the object we load (if the transcript has an object).
  • Later, just getObject() it.

Test plan:

  • Viewed a transcript for an object I can see.
  • Viewed a transcript for an object I can't see, got a policy error.