Page MenuHomePhabricator

Reduce the severity of policy fatals when building the Harbormaster "build status" element
ClosedPublic

Authored by epriestley on Mar 16 2018, 8:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 23, 5:01 AM
Unknown Object (File)
Oct 17 2024, 2:23 AM
Unknown Object (File)
Oct 9 2024, 9:13 AM
Unknown Object (File)
Oct 9 2024, 9:13 AM
Unknown Object (File)
Oct 9 2024, 9:12 AM
Unknown Object (File)
Oct 7 2024, 9:13 PM
Unknown Object (File)
Sep 16 2024, 8:47 AM
Unknown Object (File)
Sep 10 2024, 4:37 PM
Subscribers
Restricted Owners Package

Details

Summary

See PHI430. Ref T13102. When the "Build Status" element raises a policy exception, we currently fatal the whole page rather than raising a normal policy error.

This is because the policy check happens very late in page construction, long after we've made the decision to show the page instead of a policy error, and gets treated as a rendering error.

In turn, this is because the rendering is event-based rather than using a more modern Engine + EngineExtension sort of construct, so some of the actual logic runs way later than it should.

Since unwinding all of this isn't trivial and the current behavior is materially bad, limit the damage here for now by just hiding the element. See T13088 for notes on handling this in a more nuanced way in the future.

Test Plan
  • Created a revision visible to "Public".
  • Ran a build against it with a build plan visible to "All Users".
  • Viewed revision in an incognito window.
    • Before patch: Policy fatal with a red "rendering phase" error box.
    • After patch: Mostly-functional page with a missing "Build Status" element.
  • Viewed revision as a user with a normal session, saw the same UI before and after the change.

Diff Detail

Repository
rP Phabricator
Branch
policyx1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/harbormaster/event/HarbormasterUIEventListener.php:63XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 19868
Build 26925: Run Core Tests
Build 26924: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Mar 16 2018, 8:26 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 16 2018, 8:27 PM
epriestley requested review of this revision.
This revision was automatically updated to reflect the committed changes.