When editing a tab panel from a dashboard, redirect back to the dashboard
ClosedPublic
Actions

Authored by epriestley on Apr 11 2019, 4:27 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13272: Remove the hard-coded homepage, and other Dashboards improvements

Commits

rPcbe13b3065a1: When editing a tab panel from a dashboard, redirect back to the dashboard

Summary

Depends on D20396. Ref T13272. Currently, using the dropdowns to edit a tab panel from a dashboard redirects you to the tab panel page.

Instead, redirect back to the context page (usually, a dashboard -- but theoretically a containing tab panel, since you can put tab panels inside tab panels).

Also, fix some JS issues with non-integer panel keys. I've moved panel keys from "0, 1, 2, ..." to having arbitrary keys to make some operations less flimsy/error-prone, but this needs some JS tweaks.

Test Plan

Edited a tab panel from a dashboard, got sent sensibly back to the dashboard.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Apr 11 2019, 4:27 PM

Harbormaster completed remote builds in B22549: Diff 48676.Apr 11 2019, 4:29 PM

epriestley requested review of this revision.Apr 11 2019, 4:29 PM

epriestley added a child revision: D20398: Index and surface usage sites for Dashboards.Apr 11 2019, 6:10 PM

amckinley accepted this revision.Apr 11 2019, 6:52 PM

amckinley added inline comments.

src/applications/dashboard/controller/panel/PhabricatorDashboardPanelTabsController.php
127–129	Maybe `throw` this in `setContextObject()` to make the resulting stack trace more clear? (And this should probably include the `phid_get_type` of the phid to skip one extra step).

This revision is now accepted and ready to land.Apr 11 2019, 6:52 PM

epriestley added inline comments.Apr 11 2019, 7:10 PM

src/applications/dashboard/controller/panel/PhabricatorDashboardPanelTabsController.php
127–129	If we throw, we either have to catch the exception and turn it back into a dialog, or sacrifice the UX somewhat (produce a response which says "Exception!" and doesn't have a title), since we don't currently have a general-purpose way to throw an exception which has "here's how to render a nice dialog out of this" information. It's easier to just throw a little deeper, but I'm trying to make this a little nicer if users fiddle with `contextPHID` for some reason, since it's technically user-editable. Possibly we should have a user-facing sort of exception (and `PhortuneDisplayException` is a small step toward this), but then the signature needs to be `setContextObject($context, $cancel_uri)`, which seems unintuitive. Maybe this should be: try { $this->setContextObject(); } catch (PhabricatorUserFacingException $ex) { $ex->setCancelURI($cancel_uri); throw $ex; } ...with: $developer_message = pht('contextPHID points at an invalid object'); $user_message = pht("oopsie woopsie OwO that's not a valid context ;)"); throw new id(new PhabricatorUserFacingException($developer_message)) ->setTitle(pht('Bad Context Object')) ->setMessage($user_message) ...but that sure feels like a lot of work and very easy to get wrong.

I would like to find some general-purpose approach for "users could possibly hit this legitimately and we can give them something user-friendly if they do, but it's fundamentally an actual exception that we don't really expect users to hit", but I'm not sure how to do that without making some kind of mess:

One kind of mess sends UI information (like $cancel_uri) very deep into the stack.
Another kind of mess is to throw a BadDashboardContextObjectException, catch it, then return a dialog. But this is a whole lot of code for one private method in one controller and feels a lot like exceptions for control flow. I think this is the "right" way for actual APIs, but feels very heavy for one-offs. It also doesn't handle a few cases, like PhortuneDisplayException, where the Stripe connector wanted to return a whole table of drawings explaining what went wrong.
Another kind of mess is to return a list or instanceof Response the result. We do this sometimes but it feels pretty bad most of the time and has all the same problems.
Another kind of mess is to throw a DisplayException, catch it, add extra data, and re-throw it (as above) but this means the code is wrong by default (until you write try/catch and add context information) and this isn't obvious.

None of these feel particularly great to me.

Another fix is just throw new Exception and adopt an attitude of "users get what's coming to them if they hit this". The cost for that is that our error logs will never be clean.

Another fix is to return 400 or return 500 when anything goes wrong, but inevitably users figure out how to hit a significant fraction of these and the debugging cost is way higher than if the endpoint had returned a human-readable message.

Another fix is to throw new ExceptionThatShouldNotBeLogged + "users get what's coming to them if they hit this". We do this to some degree with AphrontMalformedRequestException, and it has a getTitle() to sort of render a dialog, but the whole pattern there doesn't feel great to me (and it can't render a "cancel/continue" button pointed anywhere useful).