Page MenuHomePhabricator

Allow searching Dashboards by Editable
ClosedPublic

Authored by chad on Mar 21 2017, 2:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 4:08 PM
Unknown Object (File)
Thu, Dec 12, 9:41 PM
Unknown Object (File)
Thu, Dec 12, 4:28 AM
Unknown Object (File)
Sun, Dec 8, 8:07 PM
Unknown Object (File)
Fri, Dec 6, 8:00 PM
Unknown Object (File)
Thu, Dec 5, 7:56 AM
Unknown Object (File)
Fri, Nov 29, 4:48 AM
Unknown Object (File)
Thu, Nov 28, 7:25 PM
Subscribers

Details

Summary

Ref T10390. I find myself wanting to find dashboards I can edit, even if I am not the author. I think this is useful for larger installs with multiple admins. Also make disabled Dashboards more grey in UI results.

Test Plan

Log in a test user, create a dashboard with I cannot edit. Log into my account, search for editable dashboards and only see mine. Set dashboard to all users, search under test account and see editable dashboards.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Performance on this kind of application-level policy filtered query can be pretty bad, but I don't really see a way to provide this feature otherwise.

The "overheat" limit is currently 10x the page size, so if an install has 1,000 dashboards you can not edit before any you can, this UI will become useless (i.e., take a moderately long time to give you an "overheated" error with no results). If it has 1,000 dashboards you can not edit and all the dashboards you can edit do not appear in the first 1,000 results, you will not be able to find the other dashboards with this UI. (These issues also apply to the <select /> input on the "Add Query to Dashboard" workflow.)

We could change the edit policy to an explicit list of users or something, but that doesn't seem like a very good way forward.

I'm not quite sure why/when you want to find dashboards you can edit offhand, other than for the "Add Query to Dashboard" flow. Maybe first we should try destroying every dashboard on this install which isn't installed on any user's homepage, and see where that leaves us? I think there's so much "Copy of Copy of New Simple Dashboard Test" on this install that it's hard to get a great sense of things.

I'm fine with this regardless since it's well-contained and small, but it feels like it might not be the best solution to whatever problems it solves.

src/applications/dashboard/query/PhabricatorDashboardQuery.php
69–78

This should do this instead, to batch the policy checks:

$dashboards = id(new PhabricatorPolicyFilter())
  ->setViewer(...)
  ->requireCapabilities(...)
  ->apply($dashboards);

If you loop instead of passing a list to PolicyFilter, we can end up with a bunch of single-gets for project policies.

This revision is now accepted and ready to land.Mar 21 2017, 3:01 PM

I think the core problem is I want to find all dashboards I'm responsible for. As a user this doesn't mean much but with admins, facts, projects I think there are cases where I'll be responsible for a small handful.

If we had like 10 employees and facts / project dashboards, "stuff you can edit" (~every project/fact dashboard) would probably be a huge superset of "stuff you're responsible for" (only a couple of them).

We could let you subscribe to dashboards, or improve Flag support, or add some kind of explicit "I care about this" action to dashboards, maybe.

This revision was automatically updated to reflect the committed changes.