Page MenuHomePhabricator

Link Herald rules to rule detail pages in Herald transcripts
ClosedPublic

Authored by epriestley on Aug 10 2016, 1:25 AM.
Tags
None
Referenced Files
F13091238: D16383.diff
Thu, Apr 25, 2:49 AM
Unknown Object (File)
Fri, Apr 19, 7:54 PM
Unknown Object (File)
Thu, Apr 11, 10:08 AM
Unknown Object (File)
Mon, Apr 1, 3:52 PM
Unknown Object (File)
Mar 12 2024, 7:24 AM
Unknown Object (File)
Feb 19 2024, 12:55 PM
Unknown Object (File)
Feb 15 2024, 6:53 PM
Unknown Object (File)
Feb 13 2024, 5:54 AM
Subscribers
None

Details

Summary

Fixes T9410. Depends on D16382. Since all users can now view all Herald rules, we can link them in the transcripts.

Test Plan

Viewed a transcript, clicked rule names, reviewed rules.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Link Herald rules to rule detail pages in Herald transcripts.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
chad added inline comments.
src/applications/herald/controller/HeraldTranscriptController.php
252

Is there a need to pht this?

This revision is now accepted and ready to land.Aug 10 2016, 1:47 AM
This revision was automatically updated to reflect the committed changes.

It probably shouldn't be pht()'d.

We used to do more pht('D%d: %s', $id, $title) stuff, particularly in Handle queries IIRC, which probably wasn't right either, but is at least sort of theoretically valid if : isn't a great separator character in some language.

Now that objects usually have a getMonogram(), this looks more like pht('%s: %s', $monogram, $title). Still probably a little silly, but maybe that's more conventional as - or ~ or something in Chinese. But I don't think there's any reason for the monogram construction itself to be translatable.

I figure we'll just clean this stuff up more generally in the future -- this code still really isn't "right", and it should probably load the rule and just call getMonogram() on it mooting the whole issue. At some point Herald needs a cleanup pass for transaction rendering and I imagine just cleaning up all this little stuff then.