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.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 12:48 AM
Unknown Object (File)
Sat, Mar 23, 12:47 AM
Unknown Object (File)
Sat, Mar 23, 12:47 AM
Unknown Object (File)
Sun, Mar 10, 2:44 AM
Unknown Object (File)
Jan 28 2024, 9:18 PM
Unknown Object (File)
Jan 26 2024, 3:47 AM
Unknown Object (File)
Dec 30 2023, 4:38 PM
Unknown Object (File)
Dec 30 2023, 12:01 PM
Subscribers
None

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).

Screen Shot 2019-05-13 at 9.56.31 AM.png (730×1 px, 174 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.)

  • 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.