Ref T12264. This allows users to install a dashboard they are viewing to their personal home menu or as a global home menu item. Has some basic ability to be extended later for maybe projects.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T10390: Dashboards v2 (UX)
T12264: Install Dashboard to Home / Favorites / Project - Commits
- rPa72d18765f85: Basic "Install Dashboard" workflow
Build a dashboard, click "Install Dashboard".
- As user only get personal option
- As HomeApp edit person, see both options
- Try installation as either, with and without label set
- Fake "global" form as user, get error
- Don't set anything, get error
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- install-dashboard (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 15982 Build 21179: Run Core Tests Build 21178: arc lint + arc unit
Event Timeline
Comment Actions
- add a basic global/personal radio button
- move install to view only, add more copy post install
Comment Actions
Not sure how much of this is just because this is testing direction vs actual issues, but:
This allows any user to install a global dashboard, by doing this:
- Open the dialog.
- Control-click near the radio button to activate the web inspector.
- Change the value of the radio button from "personal" to "global".
- Submit the form.
The edit logic -- not just the display logic -- needs to validate that the user has permission to make the edit. Sometimes, a good way to test this is to do the edit logic first (before fixing the display logic), and try to do all the invalid edits to make sure you get appropriate policy/permissions errors, then only fix up the display logic once you're sure the options you're hiding are properly prevented.
The "admin" flag is also "incorrect", in the sense that it gets the right result but in the wrong way. The way the policies work internally is that you can do global edits if you can edit the object (in this case, the HomeApplication). So a cleaner fix would be something like this:
- Change 'admin' to 'needsEditPermission' or similar.
- Add a 'phid' or similar.
- For this edit, the phid is the Home application PHID.
- To determine if a user can make an edit: load the object identified by phid:
- If needsEditPermission is set, require both VIEW and EDIT.
- Otherwise, require only VIEW.
- Actually, needsEditPermission is really just the opposite of personal, which would also control whether you set the customPHID. So maybe personal (or custom) is a better name for this flag (with inverted meaning), or global (with the same meaning).
This will fix the policy issue and handle these two cases (more) correctly, I think:
- In the future, we may make "Can Manage Application" configurable, so you can let some set of users other than admins configure "Home". If we do, the admin code will not enforce policies properly, but the needsEditPermission code will.
- Today, you can uninstall Home, which should probably prevent you from installing dashboards to it. I think testing VIEW permission won't actually get this right, since you can still view applications you can't use. Possibly, we could use the new INTERACT permission to fix this (sort of treat applications as "locked" if they aren't installed for you) down the road. For now this doesn't matter much because uninstalling "Home" is rare/pointless, but if this is extended to Favorites or Projects it will be more relevant since those applications can be meaningfully restricted/uninstalled.
In this diff, we should at least prevent users from installing global dashboards, even if the rest of this stuff doesn't get cleaned up yet. The code currently always gets the right result in reasonable cases other than allowing unprivileged users to do global dashboard installs, it's just a bit fragile to reasonable/likely future changes here and elsewhere in the codebase.
This isn't available on "Arrange", is that intentional? I could imagine a workflow like "create dashboard, add some panels, don't see an install button, not sure how to proceed". As a new user, I don't think it would be obvious to me that I need to return to the "view" mode to install the dashboard after I'm satisfied with my edits, since the edit mode isn't especially modal and provides a reasonable preview of the result on its own.
Maybe take the user directly to the dashboard after the install? That makes it easier for them to make further adjustments. Maybe just do this, and drop "name"? If the flow is like "install, end up on the dashboard, do any edits you want from there" then we can probably just let users do order/name/icon/etc stuff after this flow, and not need to duplicate the logic.
If you want to duplicate the logic anyway, it should apply transactions -- not directly call setMenuItemProperty(). For example, if we put a length restriction on names in the future (maybe to index them for a typeahead) this would bypass that restriction and fatal.
Overall seems generally reasonable to me, do you have thoughts on extending it to favorites/projects/other things which we might use this for in the future?
| src/applications/dashboard/controller/PhabricatorDashboardInstallController.php | ||
|---|---|---|
| 14 | Does this actually need panels? | |
| 33–34 | "as the a" -> "as a" | |
| 35 | "You can change this after by editing..." reads awkwardly to me, although I'm not sure it's really grammatically incorrect. This seems pretty much impossible to Google because the word "after" is used frequently when discussing grammar in general. I think it's because "after" is a preposition, not an adverb? Maybe consider:
| |
| src/applications/dashboard/controller/PhabricatorDashboardViewController.php | ||
| 49 | "to the your" -> "to your" | |
Comment Actions
Maybe take the user directly to the dashboard after the install?
That is, take them to Home or the project or whatever, with the dashboard selected, so they can see it in context and use the normal edit workflow to undo it or adjust it if they want.
Comment Actions
My assumption was setting of Global/Personal items was checked at the transaction level in MenuItemConfiguration. It's easy to add it to my code, but seems like the check should be at the transaction level?
Comment Actions
If you use an Editor directly, you get only a limited (and, currently, somewhat inconsistent) set of checks. Some of the reasons for this are:
- Many transactional edits don't actually require edit permission (awarding tokens, subscribing, commenting, lots of other edge stuff, pretty much everything else until D17452).
- All of the higher-level abstractions (nowadays, mostly EditEngine, but historically EditControllers) do the checks anyway.
- Performing the checks in the Editor is kind of a mess sometimes because edits may change the policies.
- Historically, I think some newly initialized objects may not have been policy-check-able?
So, in practice, the policy checks you actually get are pretty inconsistent. In theory, PhabricatorApplicationTransactionEditor->requireCapabilities() does policy checks, but in practice you get:
- No checks for new objects (due to historical initialization issues, I think).
- A View check for commenting.
- Edit checks for policy and space changes.
- By default, no other checks.
Some subclasses make an effort to implement requireCapabilities(), but most do not.
This state of affairs isn't great, but I think the future probably involves removing these checks rather than fixing them. They're basically happening at the wrong level (in the overwhelming majority of cases, we want to raise policy errors long before we get to an Editor) and EditEngine implements a more cohesive approach to transaction policies than Editor can.
We could probably also fix this instead, and get an extra layer of checks, but there's normally no way to ever hit them: you get stopped way before you get to Editor on normal pathways (edit, comment, API) via EditEngine. Since they're usually impossible to hit they're also hard to test or maintain.
Comment Actions
removing these checks rather than fixing them
Probably with the exception of the View/Edit policy checks, which check that you'll still be able to view or edit an object after making a set of policy changes. These are happening at the right level and seem correct/useful, and are actually reachable where the other checks normally aren't.
Comment Actions
On the workflow, I expect people to want to rename this. That is, I make a dashboard (Chad's Dashboard), click install, name it "Home", then install to Personal. I'm not sure that's entirely clear for the first time user, but I expect admins building new "Home" dashboards will like being able to rename + install in one dialog.
Comment Actions
is there a way to have the editor return an id of the new item??
| src/applications/dashboard/controller/PhabricatorDashboardInstallController.php | ||
|---|---|---|
| 130–137 | This is causing permission errors if you're not an admin. Is there a way to "check" permissions, and not require them? | |
| src/applications/dashboard/controller/PhabricatorDashboardInstallController.php | ||
|---|---|---|
| 130–137 | After loading the object, you can PhabricatorPolicyFilter::hasCapability(...) to test for edit permission without raising exceptions. | |
Comment Actions
is there a way to have the editor return an id of the new item??
The object itself should be updated, so just $thing->getID() should work.
| src/applications/dashboard/controller/PhabricatorDashboardInstallController.php | ||
|---|---|---|
| 13–16 | You can drop this now since it's the default behavior. | |