Degrade more gracefully when ProfileMenu dashboards fail to render
ClosedPublic
Actions

Authored by epriestley on Jun 23 2017, 7:11 PM.

Details

Reviewers

chad
amckinley

Maniphest Tasks

T12871: Setting a restricted dashboard as the topmost item in the global menu can lock some users out

Commits

rPa19859053300: Degrade more gracefully when ProfileMenu dashboards fail to render

Summary

Ref T12871. This replaces a dead end UI (user totally locked out) with one where the menu is still available, if the default menu item is one which generates a policy exception (e.g., because users can't see the dashboard).

Really, we should do better than this and not select this item as the default item if the viewer can't see it, but there is currently no reliable way to test for "can the viewer see this item?" so this is a more involved change. I'm thinking we get this minor improvement into the release, then pursue a more detailed fix afterward.

Test Plan

Added a dashboard as the top item in the global menu.
Changed the dashboard to be visible to only user B.
Viewed Home as user A.
Before patch: entire page is a policy exception dialog.
After patch, things are better:

Screen Shot 2017-06-23 at 12.06.40 PM.png (1×1 px, 134 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jun 23 2017, 7:11 PM

Harbormaster completed remote builds in B17562: Diff 43674.Jun 23 2017, 7:12 PM

epriestley mentioned this in T12871: Setting a restricted dashboard as the topmost item in the global menu can lock some users out.Jun 23 2017, 7:15 PM

Wording nit.

src/applications/search/engine/PhabricatorProfileMenuEngine.php
245	`/s/Render/Display/`? Also, as a general style question, what's the reason we always catch `Exception` instead of a more-specific subclass?

This revision is now accepted and ready to land.Jun 23 2017, 7:17 PM

epriestley added inline comments.Jun 23 2017, 7:28 PM

src/applications/search/engine/PhabricatorProfileMenuEngine.php
245	I think we generally use fairly specific, technical language like "render" elsewhere -- in theory, our users are very technical. I probably wouldn't use the word "render" if we were consumer-facing. Here, I think "render" is technically more accurate: we encountered a problem while trying to render a view from the dashboard object. My intent here isn't "We're Refusing To Show This To You", but "We Tried To Build This, It Didn't Work". I'm catching `Exception` here because my goal is to keep the navigation menu on screen if any problem occurs while rendering the dashboard. Prior to this change, the page looked like this: Specifically, the user is stranded without a menu. We could catch just `PhabricatorPolicyException` to solve exactly this policy issue, but I want to show the menu no matter why the dashboard failed, since losing the menu is pretty confusing. It's also (almost always) safe in this codebase to catch `Exception`: we do not (usually) use exceptions for control flow, so this can't (normally) accidentally catch some kind of "pseudo-exception" which should reach top level. This isn't always entirely true -- for example, the "High Security" MFA check stuff does use control flow exceptions through a couple of levels of the stack -- but is close enough to true in practice.