Summary:
Depends on D20411. Ref T13272. Dashboards and panels have new indexes (Ferret and usage edges) that need a rebuild.
For large datasets like commits we have the "activity" flow in T11932, but realistically these rebuilds won't take more than a few minutes on any realistic install so we should be able to just queue them up as migrations.
Let migrations insert a job to basically run `bin/search index --type SomeObjectType`, then do that for dashboards and panels.
(I'll do Herald rules in a followup too, but I want to tweak one indexing thing there.)
Test Plan: Ran the migration, ran `bin/phd debug task`, saw everything get indexed with no manual intervention.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20412
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:
* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.
Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20400
Summary:
Depends on D20410. Ref T13272. Dashboards/Panels currently use older "ngram" indexing, which is a less-powerful precursor to Ferret. Throw away the ngram index and provide a Ferret index instead. Also:
- Remove the NUX state, which links to the wrong place now and doesn't seem terribly important.
- Add project tags to the search result list.
- Make the "No Tags" tag a little less conspicious.
Test Plan:
- Indexed dashboards and panels.
- Searched for dashboards and panels via SearchEngine using Ferret "query" field.
- Searched for panels via "Add Existing Panel" datasource typeahead.
- Searched for dashboards via "Add Menu Item > Dashboard" on a ProfileMenu via typeahead.
- Viewed dashboard NUX state (no special state, but no more bad link to "/create/").
- Viewed dashboard list, saw project tags.
- Viewed dashboards with no project tags ("No Tags" is now displayed but less visible).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20411
Summary: Depends on D20409. Ref T13272. Before "ProfileMenu", dashboards were installed on specific objects using this table. Installs are now handled via ProfileMenu and this table no longer has any meaningful readers. Remove references to the table and destroy it.
Test Plan: Grepped for `DashboardInstall`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20410
Summary: Depends on D20408. Ref T13272. The actual JS is still a little bit iffy, but this makes the server side "move" operation work correctly by updating it to use the same code as everything else.
Test Plan: Moved panels around on single-column and multi-column dashboards, saw them move to reasonable places and stay there when I reloaded the page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20409
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.
- We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
- We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.
The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.
Test Plan:
- Added panels to a dashboard with "Create New Panel".
- Added panels to a dashboard with "Add Existing Panel".
- Removed panels from a dashboard.
- Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20408
Summary:
Depends on D20406. Ref T13272. This gets about half of Dashboards working with the new "duplicate panel friendly" storage format. Followups will fix the write pathways.
Collateral damage here includes:
- Remove the old Dashboard/Panel edge type. We have a new, more general edge type for "container X uses panel Y", and we don't need this edge type for anything else.
- Remove "attachPanels()" from Dashboard. Only rendering actually needs this, and it can just load the panels.
- Remove "attachPanelPHIDs()" from Dashboard. We can look at the panel refs to figure this out.
- Remove "attachProjects()" from Dashboard. Nothing uses this and it's not a very modern approach.
- `getPanelPHIDs()` just looks at the config now.
- Deleted some `LayoutConfig`-related code which is broken/obsolete.
Test Plan:
- Viewed various dashboards which were created before the changes, saw them render correctly.
- Viewed a dashboard with two of the same panel! AMAZING!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20407
Summary:
Depends on D20405. Ref T13272. Currently, the `PhabricatorDashboardLayoutConfig` class uses a lot of `switch()` statements to define layout modes.
Although I'm not planning to add thousands of new layout modes, this (and upcoming changes) can be made substantially cleaner by using a standard modular approach.
(This doesn't fully remove `PhabricatorDashboardLayoutConfig` yet, but that will happen soon.)
Test Plan: Edited a dashboard, saw the same layout modes as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20406
Summary:
Depends on D20402. Ref T13272. Replaces an old-school hard-coded "EditController" with a more modern one.
The actual panel stuff is still using a weird mix of legacy manual `save()` calls, but that's up next.
Test Plan: Created Dashboards, edited all dashboard fields via "Edit Dashboard".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20403
Summary:
Depends on D20399. Ref T13272. I'm moving toward fixing all the "moving panels around on Dashboards breaks the entire world" problems.
On the way there, modularize Dashboard transactions.
Test Plan:
- Created a new Dashboard.
- Edited all fiedls of a dashboard.
- Archived/restored a dashboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20402
Summary:
Depends on D20398. Ref T13272. Fixes T6018. Previously, panels showed "used on dashboards: x, y", but this did not include cases where a panel was used by another container panel (today, a tab panel).
Do edge indexing when a dashboard or panel is saved, then pull the edges on the Panel page so we can provide a full list of uses.
Test Plan: {F6369289}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T6018
Differential Revision: https://secure.phabricator.com/D20399
Summary:
Depends on D20397. Ref T13272. Similar to the recent "where are Herald rules used" stuff, show which menus Dashboards are installed in.
This is mostly straightforward, except that I pulled some of the Herald logic into a parent class so it could be shared.
Test Plan: {F6369164}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20398
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.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20397
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.
Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.
This is more work than it appears because:
- We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
- In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
- The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.
..but I think I fixed everything and didn't break anything.
Test Plan:
- Clicked "select tab" and "open dropdown menu" as separate actions.
- Viewed Diffusion, Maniphest with multiple create forms, Instances.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20396
Summary:
Ref T13276. This was introduced in D2586 to power a "trigger audits when the committed change does not match the reviewed change" feature. It was removed without ceremony in D15939. Broadly, rebases mean that this sort of feature can't really work like this and this approach is inherently unreliable; see also T182.
This property no longer has readers, and is unlikely to get any in the future since my planned pathway for "committed code must match reviewed code, modulo an automated rebase" is automating the rebase via "Land Revision", not comparing the diff text.
Remove this to simplify the flow of data here so that things in T13276 can be fixed more easily.
Test Plan: Grepped for `vsDiff`, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20395
Summary:
Ref T7667. Adds new flows `bin/auth lock` and `bin/auth unlock` to prevent compromised administrator accounts from doing additional damage by altering the authentication provider configuration.
Note that this currently doesn't actually do anything because we aren't checking this config key in any of the edit controllers yet.
Test Plan: Ran `lock` and `unlock`, checked for correct DB state, observed expected setup warning.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20394
Summary:
Ref T13275. Add portals to the search index so that:
- they show up in fulltext global search; and
- the typeahead actually uses an index.
Also make them taggable with projects as an organizational aid.
Test Plan: Indexed portals with `bin/serach index`, searched for a portal with "Query", with fulltext search in main menu, with typehead on "Install Dashboard...", changed the name of a portal and searched again to check that the index updates properly.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20389
Summary: Ref T13269. Same as D20379 with the polarity reversed.
Test Plan: Added some triggers, removed some projects, observed expected results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20390
Summary: Ref T13269. This is mostly copying code from the similar Herald implementation. Note that the drop effect preview always renders because we don't have the infrastructure to compare lists of edge targets.
Test Plan: Created some triggers, dragged some tasks around, checked that tasks that already had project membership didn't write additional edges.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20379
Summary:
See PHI1175. An install would like to trigger some reminders/guidance if users don't link revisions to JIRA issues.
Expose "JIRA Issue URIs" as a field so Herald can act on the presence or absence of issues.
I'm exposing "JIRA Issue URIs", not a field like "[ Has Jira Issue ][ is true ]", since it's a bit more flexible: you can use a regexp to test against particular `PROJ-123` project prefixes in JIRA, for example.
Test Plan: {F6367696}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20386
Summary:
Depends on D20383. Ref T13272. Fixes T12363. See PHI997. This gets the edit flows for tab panels functional again. They aren't //nice//, and a lot of the workflows are fairly janky: for example, most of them end up with you on the tab panel's page, which isn't useful if you started on a dashboard page.
However, these flows were extremely janky before anyway (see T12363) and I suspect this is a net improvement even though it's a bit of a mess. I anticipate cleaning this up bit-by-bit in future diffs.
Test Plan: {F6366372}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T12363
Differential Revision: https://secure.phabricator.com/D20384
Summary: Depends on D20384. Ref T13275. A bunch of this code got converted but I missed some callsites that aren't reached directly from the menu.
Test Plan:
- Visited each controller, saw actual pages instead of menu construction fatals.
- Grepped for `getProfileMenu()`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20385
Summary: Depends on D20377. Ref T13272. In D20372, I temporarily removed the controls for actually editing Query panels. Restore them.
Test Plan:
- Viewed existing Query panels, saw them working like they did before.
- Created and edited Query panels.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20382
Summary:
Depends on D20376. Ref T8033. It's possible to put a bunch of secret panels on a public dashboard, and not obvious that the dashboard won't be very useful.
This was more of an issue long ago (when the dashboard broke or all the panels completely vanished or something?). Nowadays, the panels render "You don't have permission to view this" so it's likely easy to explain/fix. Still, we can warn about this.
But, for now, don't, since a lot of this works better now and it's not really clear that this is particularly valuable. We can revisit this after all the connected changes have more of a chance to settle.
Test Plan:
(Earlier behavior, not how things look in the final version.)
{F6335008}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8033
Differential Revision: https://secure.phabricator.com/D20377
Summary:
Depends on D20374. Panels may not be visible if they are restricted (no permission) or if they are invalid (e.g., the panel was deleted).
Render these as two separate states instead of one big combined state.
Test Plan: {F6334756}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20376
Summary:
Depends on D20373. Ref T13272. When you put Dashboards in Favorites, the render without a navigation menu, which is kind of weird.
Instead, make Favorites display a navigation menu. This effectively turns it into a weird cross between the home page and a portal, but so be it.
Also change the icon from "star" to "bookmark" since I think that a clearer hint about how it works.
Test Plan: Viewed dashboards in favorites, got a navigation menu. Edited items from portals, home, favorites.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20374
Summary:
Depends on D20372. Ref T13272.
- There's a very heavy dropshadow on panels right now that looks out of place. Reduce it a bit.
- Panels currently have unlabeled pencil and trash icons. Turn this into a menu. I'm likely planning to add options like "Change Query..." to this menu to make managing some types of panels easier.
Test Plan: {F6332838}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20373
Summary:
Depends on D20371. Ref T13272. Dashboard panels use CustomField to specify editable panel behavior. This is an older approach which was largely or entirely obsoleted by EditEngine.
Throw away all the CustomField edit stuff. Convert the "text" panel to EditEngine to prove this at least mostly works.
This breaks "query" panels and "tab" panels (they'll still work fine, but they can't be meaningfully edited). I'll restore those in a future change.
Test Plan: Created and edited a "text" panel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20372
Summary:
Depends on D20370. Ref T13272. This tries to get panel editing fully on the newer "Modular Transactions" + "EditEngine" flow.
This breaks tab panels a bit, but I'll fix that in a followup. And they weren't exactly in great shape before.
Also makes the flow prettier. :3
Test Plan: {F6332746}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20371
Summary: Depends on D20369. Ref T13272. Move toward a world where we can edit panels with just one controller, instead of separate "Edit" and "Editpro" controllers.
Test Plan: Created and edited panels. This will get vetted more thoroughly after additional changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20370
Summary:
Depends on D20368. Ref T13272. Dashboards have a lot of controllers, try to organize them a little better.
Note "EditController" vs "EditproController". Yikes.
Test Plan: Loaded dashboards. No code changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20369
Summary:
Depends on D20367. Ref T13272. When an edit action is disabled, we add "workflow" so that the "You can't do this" message renders in a dialog instead of a separate page.
These actions are implemented in a nonstandard way; standardize them.
Test Plan: Clicked both actions as a user who could take them (got normal behavior); and as a user who could not (got permissions dialog errors).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20368
Summary:
Depends on D20364. Ref T13272. When you create a dashboard, we currently give you a modal choice between an empty dashboard and a "simple template" dashboard.
Remove this choice, and create an empty dashboard in all cases instead.
I think this template dashboard flow isn't terribly useful, and is partly covering over other deficiencies in the workflow. I'm fixing many of those and suspect we can get away without this now. Users on this flow also may not really know what they want. This also contributes to having a lot of extra unmoored panels floating around.
If we did rebuild this, I'd like to address more specific use cases and probably build it as "Add a Template Panel..." or similar, as an action you can use to quickly update an existing workboard. This would be a lot more flexible than create-a-whole-template-board.
Test Plan: Created a new board, no more "template: yes or no?" gate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20367
Summary:
Depends on D20362. Ref T13272. Currently, Dashboards have an "Install Dashboard" flow which is pretty janky and only allows you to install things to the home page.
Instead, allow users to install things to any valid target (home, favorites, portals, projects). This also provides URIs like `dashboard/install/1/home/personal/` which allow you to link users to an "install a dashboard" page; this may or may not get used.
Test Plan: Installed dashboards on home, favorites, projects, and portals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20364
Summary:
Depends on D20361. Ref T13272. Currently, Dashboards have three separate modes: view, arrange, manage.
With the advent of Portals, I think we can simplify this, and make the dashboard view a combined view/edit/manage page. To view it in a cleaner standalone way, you can add it to a portal/home/project. I'll also improve the "Install" workflow.
Test Plan:
Viewed a dashboard page, clicked through all the actions, grepped for affected URIs.
{F6327027}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20362
Summary:
Ref T13269. Workboard triggers can now reassign tasks on column drop. Also sprinkles some `setViewer()` calls in places that needed them.
This mostly works, but a few issues:
* To set the owner to unassigned, you must explicitly put the "No Owner" token in the typeahead. Maybe this should just figure out you've put nothing in that field and set it for you?
* I'm pretty sure this was already broken, but if you change the rule type from a tokenizer to a different type, the default for the field doesn't populate correctly: {F6312227}
Also adds a new hook for trigger rules: `getValueForField($value)` which allows you to transform a value stored in the DB into a form suitable for setting on a form control.
Test Plan: Dragged tasks between columns and observed new owners as expected. Didn't try to get fancy to assign tasks to deleted users, users that the viewer can't see, bot users, etc etc. I'm relying on the underlying transaction to hopefully do the right thing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20329
Summary:
See <https://discourse.phabricator-community.org/t/call-to-undefined-method-phuipagerview-gethasmoreresults-in-2019-week-13/2586/>.
A small number of queries (including "Notifications" and (global) "Search") use offset-based pagers which have a slightly different API `PHUIPagerView` instead of `AphrontCursorPagerView`. This leads to a fatal in the new code for the "View All Results" buttons.
To fix this, just do an `instanceof` test. Some day we can unify the pagers.
Test Plan: Added a notifications panel, rendered it, saw it work instead of fataling on "getHasMoreResults()". Also rendered some normal panels.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20366
Summary:
See <https://discourse.phabricator-community.org/t/non-functional-actions-menu-on-live-phame-views/2593>. Several layers here:
The "Actions" button is broken because a menu behavior is failing, since we aren't rendering the menu.
When a behavior fails to initialize, catch and log the exception and continue. Previously, we stopped initializing behaviors if any failed, but behaviors are usually independent and continuing with an explicit exception seems reasonable.
Give "JX.log()" some "sprintf()" semantics to make logging the behavior failure easier. We can probably afford these extra 200 bytes now in 2019.
This fixes the button and gives us explicit errors in the log. So far, so good.
Then, when a page won't render chrome, don't try to render the main menu. This fixes the actual errors (we no longer try to initialize menu behaviors for nodes which don't exist).
Completely hide the "Actions" and "Comment" flows if the viewer isn't logged in. Although this isn't completely consistent with other applications, I think it's more appropriate for Phame. In applications like Maniphest, we show a full set of controls (but disable them) so that users who are not currently logged in have a clear path to interact with the content, under the assumption that this is a relatively common workflow. This is probably less common for Phame, where we expect most anonymous viewers not to log in or interact.
Finally, parametrize a one-off border color and add a border under the crumbs at the top of the page.
Test Plan:
- Viewed a "Live" Phame blog post page, clicked "Actions", got a dropdown.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20378
Summary:
Depends on D20360. Ref T13275. This makes the "Dashboards" application start on a Drydock-like console page where you pick portals, dashboards, or panels.
Probably the "Dashboards" application should either be renamed to "IntelliknowledgePro" or Portals should be split off into a separate application eventually, but let's see how things go like this for now, since restructuring probably breaks some URIs at least a little bit so I'd like more confidence that we're headed in the right direction before we do it.
Test Plan:
- Visited Dashboards via typeahead, got options for Dashboards/Portals/Panels.
- Visited Portals pages, got simplified crumbs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20361
Summary:
Depends on D20359. Fixes T12098. When you add a new "Form" item and pick "Create Revision", you currently get a bad link. This is because Differential is kind of special and the form isn't usable directly, even though Differential does use EditEngine.
Allow EditEngine to specify a different create URI, then specify the web UI paste-a-diff flow to fix this.
Test Plan:
- Added "Create Revision" to a portal, clicked it, was sensibly put on the diff flow.
- Grepped for `getCreateURI()`, the only other real use case is to render the "Create X" dropdowns in the upper right.
- Clicked one of those, still worked great.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12098
Differential Revision: https://secure.phabricator.com/D20360
Summary:
Depends on D20358. Fixes T12871. After refactoring, we can now tell when a "storage" menu item generated only disabled "display" menu items, and not pick any of them as the default rendering.
This means that if you're looking at a portal/menu with several dashboards, but can't see some at the top, you'll get the first one you can see.
Also clean up a lot of minor issues with less-common states.
Test Plan:
- Created a portal with two private dashboards and a public dashboard.
- Viewed it as another user, saw the default view show the dashboard I can actually see.
- Minor fix: Disabled and enabled the hard-coded "Home" item, now worked cleanly with the right menu state.
- Minor fix: added a motivator panel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12871
Differential Revision: https://secure.phabricator.com/D20359
Summary:
Depends on D20357. Ref T13275. Now that there's a stronger layer between "stuff in the database" and "stuff on the screen", these subclasses all need to emit intermediate objects instead of raw, HTML-producing view objects.
This update is mostly mechanical.
Test Plan:
- Viewed Home, Favorites, Portals, User Profiles, Project Profiles.
- Clicked each item on each menu/profile type.
- Added every (I think?) type of item to a menu and clicked them all.
- Grepped for obsolete symbols (`newNavigationMenuItems`, `willBuildNavigationItems`).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20358
Summary:
Depends on D20356. Ref T13275. See also T12871 and T12949.
Currently, the whole "ProfileMenu" API operates around //stored// items. However, stored items are allowed to produce zero or more //display// items, and we sometimes want to highlight display item X but render stored item Y (as is the case with "Link" items pointing at `?filter=xyz` on Workboards).
For the most part, this either: doesn't work; or works by chance; or is kind of glued together with hope and prayer (as in D20353).
Put an actual structural layer in place between "stored/configured item" and "display item" that can link them together more clearly. Now:
- The list of `ItemConfiguration` objects (stored/configured items) is used to build an `ItemViewList`.
- This handles the selection/highlighting/default state, and knows which display items are related to which stored items.
- When we're all done figuring out what we're going to select and what we're going to highlight, it pops out an actual View which can build the HTML.
This requires API changes which are not included in this change, see next change.
This doesn't really do anything on its own, but builds toward a more satisfying fix for T12871. I'd hoped to avoid doing this for now, but wasn't able to get a patch I felt good about for T12871 built without fixing this first.
Test Plan: See next change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20357
Summary:
Depends on D20355. Ref T13275. Ref T13247. Currently, "Hamburger" menus are not automatically built from navigation menus. However, this is (I'm almost completely sure?) a reasonable and appropriate default behavior, and saves us some code around profile menus.
With this rule in place, we can remove `setApplicationMenu()` and `getApplicationMenu()` from `StandardPageView`, since they have no callers.
This also updates a lot of profile menu callsites to a new API which is added in the next change.
Test Plan: See the next two changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275, T13247
Differential Revision: https://secure.phabricator.com/D20356
Summary:
Depends on D20353. Ref T13275. This is just some small quality-of-life fixes:
- When you add items to menus, they currently go below the "Edit Menu/Manage Menu" links by default. This isn't a very good place for them. Instead, lock "edit" items to the bottom of the menu.
- Lock profile pictures to the top of the menu. This just simplifies things a little.
- Show more iconography hints on the "edit menu items" UI.
- Add a "drag stuff to do things" hint if some stuff can be dragged.
Test Plan:
- Added new items to a Portal, they didn't go to the very bottom. Instead, they went above the "Edit/Manage" links; a sensible place for them.
- Viewed the "edit menu items" screen, saw more hints and visual richness.
- Viewed/edited Home, Projects, Portals, Favorites
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20355
Summary:
Depends on D20352. Fixes T12949. If a user adds a link (for example, to a workboard) that takes you to the same page but with some URI parameters, we'd prefer to highlight the "link" item instead of the default "workboard" item when you click it.
For example, you add a `/query/assigned/` "link" item to a workboard, called "Click This To Show Tasks Assigned To Me On This Workboard", i.e. filter the current view.
This is a pretty reasonable thing to want to do. When you click it, we'd like to highlight that item to show that you've activated the "Assigned to Me" filter you added.
However, we currently highlight the thing actually serving the content, i.e. the "Workboard" item.
Instead:
- When picking what to highlight, look through all the items for one with a link to the current request URI.
- If we find one or more, pick the one that would be the default.
- Otherwise, pick the first one.
This means that you can have several items like "?a=1", "?a=2", etc., and we will highlight them correctly when you click them.
This actual patch has some questionable bits (see some discussion in T13275), but I'd like to wait for stronger motivation to refactor it more extensively.
Test Plan:
- On a portal, added a `?a=1` link. Saw it highlight properly when clikced.
- On a workboard, added a link to the board itself with a different filter. Saw it highlight appropriately when clicked.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12949
Differential Revision: https://secure.phabricator.com/D20353
Summary:
Depends on D20349. Ref T13275. Currently, a default item is selected as a side effect of generating the full list of items, for absolutely no reason.
The logic to pick the currently selected item can also be separated out pretty easily.
(And fix a bug in with a weird edge case in projects.)
This doesn't really change anything, but it will probably make T12949 a bit easier to fix.
Test Plan: Viewed Home / projects / portals, clicked various links, got same default/selection behavior as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20352
Summary:
Depends on D20348. Ref T13275. Portals are mostly just a "ProfileMenuEngine" menu, and that code is already relatively modular/flexible, so set that up to start with.
The stuff it gets wrong right now is mostly around empty/no-permission states, since the original use cases (project menus) didn't have any of these states: it's not possible to have a project menu with no content.
Let the engine render an "empty" state (when there are no items that can render a content page) and try to make some of the empty behavior a little more user-friendly.
This mostly makes portals work, more or less.
Test Plan: {F6322284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20349
Summary:
Ref T13275. Today, you can build a custom page on the home page, on project pages, and in your favorites menu.
PHI374 would approximately like to build a completely standalone custom page, and this generally seems like a reasonable capability which we should support, and which should be easy to support if the "custom menu" stuff is built right.
In the near future, I'm planning to shore up some of the outstanding issues with profile menus and then build charts (which will have a big dashboard/panel component), so adding Portals now should let me double up on a lot of the testing and maybe make some of it a bit easier.
Test Plan:
Viewed the list of portals, created a new portal. Everything is currently a pure skeleton with no unique behavior.
Here's a glorious portal page:
{F6321846}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20348
Summary:
There are two issues here I was trying to fix:
* Viewing `/conpherence` by logged out users on `secure` would generate an overheated query on `ConpherenceThreadQuery` `secure` has a ton of wacky threads with bogus names.
* When a user views a specific thread that they don't have permission to see, we attempt to fetch the thread's transactions before applying policy filtering. If the thread has more than 1000 comments, that query will also overheat instead of returning a policy exception.
I fixed the first problem, but started trying to fix the second by moving the transaction fetch to `didFilterPage` but it broke in strange ways so I gave up.
Also fix a dangling `qsprintf` update.
Test Plan: Loaded threads and the Conpherence homepage with and without logged in users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20375
Summary: See PHI985. The layers above this may return `array()` to mean "one hunk with a line-1 offset". Accept either `array()` or `array(1 => ...)` to engage the scope engine.
Test Plan: See PHI985.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20363
Summary:
See PHI1182. Ref T13266. The recent fixes didn't quite cover the case where you have a query, but order by something other than relevance, and page forward.
Refine the tests around building/selecting these columns and paging values a little bit to be more specific about what they care about.
Test Plan:
Executed queries, then went to "Next Page", for:
- query text, non-relevance order.
- query text, relevance order.
- no query text, non-relevance order.
- no query text, relevance order.
Also, made an API call similar to the one in PHI1182.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13266
Differential Revision: https://secure.phabricator.com/D20354
Summary: See PHI1180. Currently, when we failover to a replica, we may not log the failure. Failovers are serious business and bad news, so emit a log even if we are able to connect to the replica.
Test Plan:
Configured a bogus master and a good replica:
```
$ ./bin/mail list-outbound
[2019-03-29 16:26:09] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:26:19] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:26:29] EXCEPTION: (PhutilProxyException) Failed to connect to master database ("local_config"), failing over into read-only mode. {>} (AphrontConnectionQueryException) Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out. at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:362]
<...snip backtrace...>
3945 Voided email rP04f9e72cbd10: Don't subscribe bots implicitly when they act on objects, or when they are…
3946 Voided email rPdf53d72e794c: Allow "Move Tasks to Column..." to prompt for MFA
3947 Voided email rP492b03628f19: Fix a typo in Drydock "Land" operations
3948 Voided email rPb469a5134ddd: Allow "SMTP" and "Sendmail" mailers to have "Message-ID" behavior configured in…
3949 Voided email rPa6fd8f04792d: When performing complex edits, pause sub-editors before they publish to…
...
```
Configured a bogus master and a bogus replica:
```
$ ./bin/mail list-outbound
[2019-03-29 16:26:57] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:07] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:27] PHLOG: 'Retrying (attempt 1) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.3 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:37] PHLOG: 'Retrying (attempt 2) after connection failure ("AphrontConnectionQueryException", #2002): Attempt to connect to root@127.0.0.3 failed with error #2002: Operation timed out.' at [/Users/epriestley/dev/core/lib/libphutil/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:124]
[2019-03-29 16:27:47] EXCEPTION: (PhabricatorClusterStrandedException) Unable to establish a connection to any database host (while trying "local_config"). All masters and replicas are completely unreachable.
AphrontConnectionQueryException: Attempt to connect to root@127.0.0.2 failed with error #2002: Operation timed out. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:177]
<...snip backtrace...>
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20351
Summary:
See <https://discourse.phabricator-community.org/t/duo-broken-in-2019-week-12/2580/>.
The "live update Duo status" endpoint currently requires full sessions, and doesn't work from the session upgrade gate on login.
Don't require a full session to check the status of an MFA challenge.
Test Plan: Went through Duo gate in a new session, got a live update.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20347
Summary:
Fixes T13273. This element is a bit weird, but I think I fixed it without breaking anything.
The CSS is used by project hovercards and user hovercards, but they each have a class which builds mostly-shared-but-not-really-identical CSS, instead of having a single `View` class with modes. So I'm not 100% sure I didn't break something obscure, but I couldn't find anything this breaks.
The major issue is that all the text content has "position: absolute". Instead, make the image "absolute" and the text actual positioned content. Then fix all the margins/padding/spacing/layout and add overflow. Seems to work?
Plus: hide availability for disabled users, for consistency with D20342.
Test Plan:
Before:
{F6320155}
After:
{F6320156}
I think this is pixel-exact except for the overflow behavior.
Also:
- Viewed some other user hovercards, including a disabled user. They all looked unchanged.
- Viewed some project hovercards. They all looked good, too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13273
Differential Revision: https://secure.phabricator.com/D20344
Summary:
See downstream <https://phabricator.wikimedia.org/T138723>. That suggestion is a little light on details, but I basically agree that showing "Availability: Available" on disabled user profiles is kind of questionable/misleading.
Just hide event information on disabled profiles, since this doesn't seem worth building a special "Availability: Who Knows, They Are Disabled, Good Luck" disabled state for.
Test Plan: Looked at disabled and non-disabled user profiles, saw Calendar stuff only on the former.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20342
Summary: Ref T13266. Caught one more of these "directly setting afterID" issues in the logs.
Test Plan: Ran `bin/search index --type ConpherenceThread` before and after changes. Before: fatal about a direct call. After: clean index rebuild.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13266
Differential Revision: https://secure.phabricator.com/D20341
Summary:
Ref PHI1173. Currently, you can edit an MFA'd comment without redoing MFA. This is inconsistent with the intent of the MFA badge, since it means an un-MFA'd comment may have an "MFA" badge on it.
Instead, implement these rules:
- If a comment was signed with MFA, you MUST MFA to edit it.
- When removing a comment, add an extra MFA prompt if the user has MFA. This one isn't strictly required, this action is just very hard to undo and seems reasonable to MFA.
Test Plan:
- Made normal comments and MFA comments.
- Edited normal comments and MFA comments (got prompted).
- Removed normal comments and MFA comments (prompted in both cases).
- Tried to edit an MFA comment without MFA on my account, got a hard "MFA absolutely required" failure.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20340
Summary:
See downstream <https://phabricator.wikimedia.org/T209449>.
The "Bulk Edit" flow works with `setContinueOnMissingFields(true)`, so `newRequiredError()` errors are ignored. This allows you to apply a transaction which changes the title to `""` (the empty string) without actually hitting any errors which the workflow respects.
(Normally, `setContinueOnMissingFields(...)` workflows only edit properties that can't be missing, like the status of an object, so this is an unusual flow.)
Instead, validate more narrowly:
- Transactions which would remove the title get an "invalid" error, which is respected even under "setContinueOnMissingFields()".
- Then, we try to raise a "missing/required" error if everything otherwise looks okay.
Test Plan:
- Edited a task title normally.
- Edited a task to remove the title (got an error).
- Created a task with no title (disallowed: got an error).
- Bulk edited a task to remove its title.
- Before change: allowed.
- After change: disallowed.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20339
Summary:
Depends on D20337. Fixes T12167. Ref T13272. On this page ("Favorites > Edit Favorites > Personal", for example) the curtain actions aren't available on mobile.
Normally, curtains are built with `Controller->newCurtainView()`, which sets an ID on the action list, which populates the header button. This curtain is built directly because there's no `Controller` handy.
To fix the issue, just set an ID. This could probably be cleaner, but that's likely a much more involved change.
Test Plan: Edited my favorites, narrowed the window, saw an "Actions" button.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T12167
Differential Revision: https://secure.phabricator.com/D20338
Summary: Depends on D20336. Ref T13272. Fixes T12745. If you uninstall Conpherence, "Edit Favorites" and other editable menu interfaces still allow you to add "Conpherence" items. Prevent this.
Test Plan:
- Installed Conpherence: Saw option to add a "Conpherence" link when editing favorites menu.
- Uninstalled Conpherence: No more option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T12745
Differential Revision: https://secure.phabricator.com/D20337
Summary:
Depends on D20335. Ref T13263. Ref T13272. See PHI854. Ref T9903.
Currently, we don't provide a clear indicator that a query panel is showing a partial result set (UI looks the same whether there are more results or not).
We also don't provide any way to get to the full result set (regardless of whether it is the same as the visible set or not) on tab panels, since they don't inherit the header buttons.
To (mostly) fix these problems, add a "View All Results" button at the bottom of the list if the panel shows only a subset of results.
Test Plan:
{F6314560}
{F6314562}
{F6314564}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T13263, T9903
Differential Revision: https://secure.phabricator.com/D20336
Summary:
Depends on D20334. Ref T13272. After recent changes to make overheating queries throw by default, dashboard panels now fail into an error state when they overheat.
This is a big step up from the hard-coded homepage panels removed by D20333, but can be improved. Let these panels render partial results when they overheat and show a human-readable warning.
Test Plan: {F6314114}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20335
Summary:
Depends on D20333. Ref T13272. Currently, dashboard query panels have an aesthetic but hard-to-see icon to view results, with no text label.
Instead, provide an easier-to-see button with a text label.
Test Plan: {F6314091}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20334
Summary:
Ref T13272. Currently, the hard-coded default homepage looks like a dashboard but is actually rendered completely manually.
This means that various panel rendering improvements we'd like to make (including better "Show More" behavior and better handling of overheated queries) won't work on the home page naturally: we'd have to make these changes twice, once for dashboards and once for the home page.
Instead, build the home page out of real panels. This turns out to be significantly simpler (I think the backend part of panels/dashboards is mostly on solid footing, the frontend just needs some work).
Test Plan:
Loaded the default home page, saw a thing which looked the same as the old thing. Changes I know about / expect:
- The headers for these panels are no longer linked, but they weren't colorized before so the links were hard to find. I plan to improve panel behavior for "find/more" in a followup.
- I've removed the "follow us on twitter" default NUX if feed is empty, since this seems like unnecessary incidental complexity.
- (Internal exception behavior should be better, now.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20333
Summary:
Ref T13269. Currently, if you're on a milestone workboard like this:
> Projects > Parent > Milestone > Workboard
The "Parent" link goes to the parent profile. More often, I want it to go to the parent workboard. Try doing that? This is kind of one-off but I suspect it's a better rule.
Also, consolidate one billion manual constructions of "/board/" URIs.
Test Plan: Viewed a milestone workboard, clicked the parent link, ended up on the parent workboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20331
Summary:
Ref T13269. These cards really have three states:
- Editable: shows a pencil icon edit button.
- You Do Not Have Permission To Edit This: shows a "no editing" icon in red.
- Hovecard: shouldn't show anything.
However, the "hovercard" and "no permission" states are currently the same state, so when I made the "no permission" state more obvious that made the hovercard go all weird.
Make these states explicitly separate states.
Test Plan:
Looked at a...
- Editable card on workboard: edit state.
- No permission card on workboard: no permission state.
- Any hovercard: "not editable, this is a hovercard" state.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20330
Summary: If "GD" doesn't support a particular image type, applying a cover image currently goes through but no-ops. Fail it earlier in the process with a more specific error.
Test Plan: Without PNG support locally, dropped a PNG onto a card on a workboard. Got a more useful error.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20328
Summary: This is a copy/paste/find-and-replace-all of the status rule added by D20288.
Test Plan: Made some triggers, moved some tasks, edited some triggers. Grepped for the word "status" in the new file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20325
Summary:
Depends on D20321. Fixes T12175. Ref T13074. Now that before/after PHIDs are suggestions, we can give the server a more complete view of what the client is trying to do so we're more likely to get a good outcome if the client view is out of date.
Instead of passing only the one directly adjacent card PHID, pass all the card PHIDs that the client thinks are in the same group.
(For gigantic columns with tens of thousands of tasks this might need some tweaking -- like, slice both lists down to 10 items -- but we can cross that bridge when we come to it.)
Test Plan:
- Dragged some cards around to top/bottom/middle positions, saw good positioning in all cases.
- In two windows, dragged stuff around on the same board. At least at first glance, conflicting simultaneous edits seemed to do reasonable things.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074, T12175
Differential Revision: https://secure.phabricator.com/D20322
Summary:
Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one `afterPHID` or `beforePHID` and moves the card before or after the specified card.
This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.
In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.
In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:
> You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.
...but this is pretty hostile and probably rarely/never what the user wants.
Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.
(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)
We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.
Test Plan:
- (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
- (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074, T12175
Differential Revision: https://secure.phabricator.com/D20321
Summary:
Fixes T13270. In Diffusion, the "Code" tab is linked in a weird way that isn't consistent with the other tabs.
Particularly, if you navigate to `x/y/z/` and toggle between the "Branches" and "History" tabs (or other tabs), you keep your path. If you click "Code", you lose your path.
Instead, retain the path, so you can navigate somewhere and then toggle to/from the "Code" tab to get different views of the same path.
Test Plan: Browed into a repository, clicked "History", clicked "Code", ended up back in the place I started.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13270
Differential Revision: https://secure.phabricator.com/D20323
Summary:
In some cases, we show a limited number of one type of object somewhere else, like "Recent Such-And-Such" or "Herald Rules Which Use This" or whatever.
We don't do a very good job of communicating that these are partial lists, or how to see all the results. Usually there's a button in the upper right, which is fine, but this could be better.
Add an explicit "more stuff" button that shows up where a pager would appear and makes it clear that (a) the list is partial; and (b) you can click the button to see everything.
Test Plan: {F6302793}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20315
Summary:
Ref T5474. In 99% of cases, a separate "archived/active" status for triggers probably doesn't make much sense: there's not much reason to ever disable/archive a trigger explcitly, and the archival rule is really just "is this trigger used by anything?".
(The one reason I can think of to disable a trigger manually is because you want to put something in a column and skip trigger rules, but you can already do this from the task detail page anyway, and disabling the trigger globally is a bad way to accomplish this if it's in use by other columns.)
Instead of adding a separate "status", just track how many columns a trigger is used by and consider it "inactive" if it is not used by any active columns.
Test Plan: This is slightly hard to test exhaustively since you can't share a trigger across multiple columns right now, but: rebuild indexes, poked around the trigger list and trigger details, added/removed triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20308
Summary:
Ref T5474. Allow columns to play a sound when tasks are dropped.
This is a little tricky because Safari has changed somewhat recently to require some gymnastics to play sounds when the user didn't explicitly click something. Preloading the sound on the first mouse interaction, then playing and immediately pausing it seems to work, though.
Test Plan: Added a trigger with 5 sounds. In Safari, Chrome, and Firefox, dropped a card into the column. In all browsers, heard a nice sequence of 5 sounds played one after the other.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20306
Summary:
Ref T5474. The first rough cut of triggers showed some of the trigger rules in a tooltip when you hover over the "add/remove" trigger menu.
This isn't great since we don't have much room and it's a bit finnicky / hard to read.
Since we have a better way to show effects now in the drop preview, just use that instead. When you hover over the trigger menu, preview the trigger in the "drop effect" element, with a "Trigger: such-and-such" header.
Test Plan:
- This is pretty tough to screenshot.
- Hovered over menu, got a sensible preview of the trigger effects.
- Dragged a card over the menu, no preview.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20304
Summary: Ref T5474. When you view the main page for a rule, show what the rule does before you actually edit it.
Test Plan:
Viewed a real trigger, then faked invalid/unknown rules:
{F6300211}
{F6300212}
{F6300213}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20303
Summary:
Ref T5474. This provides a Herald-like UI for editing workboard trigger rules.
This probably has some missing pieces and doesn't actually save anything to the database yet, but the basics at least roughly work.
Test Plan: {F6299886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20301
Summary:
Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing.
For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions.
Test Plan:
Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases.
Drags which actually cause effects still show the effects correctly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335
Differential Revision: https://secure.phabricator.com/D20300
Summary:
Ref T10335. Ref T5474. When you drag-and-drop a card on a workboard, show a UI hint which lists all the things that the operation will do.
This shows: column moves; changes because of dragging a card to a different header; and changes which will be caused by triggers.
Not implemented here:
- Actions are currently shown even if they have no effect. For example, if you drag a "Normal" task to a different column, it says "Change priority to Normal.". I plan to hide actions which have no effect, but figuring this out is a little bit tricky.
- I'd like to make "trigger effects" vs "non-trigger effects" a little more clear in the future, probably.
Test Plan:
Dragged stuff between columns and headers, and into columns with triggers. Got appropriate preview text hints previewing what the action would do in the UI.
(This is tricky to take a screenshot of since it only shows up while the mouse cursor is down.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335, T5474
Differential Revision: https://secure.phabricator.com/D20299
Summary: Depends on D20287. Ref T5474. This hard-codes a storage value for every trigger, with a "Change status to <default closed status>" rule and two bogus rules. Rules may now apply transactions when cards are dropped.
Test Plan: Dragged cards to a column with a trigger, saw them close.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20288
Summary:
Depends on D20286. Ref T5474. Attaches triggers to columns and makes "Remove Trigger" work.
(There's no "pick an existing named trigger from a list" UI yet, but I plan to add that at some point.)
Test Plan: Attached and removed triggers, saw column UI update appropriately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20287
Summary: Depends on D20279. Ref T5474. Modernize these transactions before I add a new "TriggerTransaction" for setting triggers.
Test Plan: Created a column. Edited a column name and point limit. Hid and un-hid a column. Grepped for removed symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20286
Summary:
Depends on D20278. Ref T5474. This change creates some new empty objects that do nothing, and some new views for looking at those objects. There's no actual useful behavior yet.
The "Edit" controller is custom instead of being driven by "EditEngine" because I expect it to be a Herald-style "add new rules" UI, and EditEngine isn't a clean match for those today (although maybe I'll try to move it over).
The general idea here is:
- Triggers are "real" objects with a real PHID.
- Each trigger has a name and a collection of rules, like "Change status to: X" or "Play sound: Y".
- Each column may be bound to a trigger.
- Multiple columns may share the same trigger.
- Later UI refinements will make the cases around "copy trigger" vs "reference the same trigger" vs "create a new ad-hoc trigger" more clear.
- Triggers have their own edit policy.
- Triggers are always world-visible, like Herald rules.
Test Plan: Poked around, created some empty trigger objects, and nothing exploded. This doesn't actually do anything useful yet since triggers can't have any rule behavior and columns can't actually be bound to triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20279
Summary:
When a TOS-like Legalpad document is marked "Require this document to use Phabricator", the login prompt shows a "Manage" button, but that button doesn't work.
When we're presenting a document as a session gate, don't show "Manage".
Test Plan: Viewed a required document during a session gate (no "Manage" button) and normally (saw "Manage" button).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20312
Summary:
See downstream <https://phabricator.wikimedia.org/T208254>.
I can't actually reproduce any issue here (we only show this field when creating a document, and only if the viewer is an administrator), so maybe this relied on some changes or was originally reported against older code.
Regardless, the validation isn't quite right: it requires administrator privileges to apply this transaction at all, but should only require administrator privileges to change the value.
Test Plan:
Edited Legalpad documents as an administrator and non-administrator before and after the change, with and without signatures being required.
Couldn't reproduce the original issue, but this version is generally more correct/robust.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20311
Summary: See downstream <https://phabricator.wikimedia.org/T182232>. We currently don't mark repository handles as closed.
Test Plan: Mentioned two repositories with `R1` (active) and `R2` (inactive). After patch, saw `R2` visually indicated as closed/inactive.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20309
Summary: See PHI1153. The "Runnable" and "Restartable" behaviors interact (to click "restart", you must be able to run the build AND it must be restartable). Make this more clear.
Test Plan: {F6301739}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20307
Summary: Ref T13266. This callsite is using the older API; swap it to use pagers.
Test Plan: Viewed a Phame blog post with siblings, saw the previous/next posts linked.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: nicolast
Maniphest Tasks: T13263, T13266
Differential Revision: https://secure.phabricator.com/D20319
Summary:
Ref T13263. Two minor issues:
- The "reconcile" dialog shows the wrong sign because JS signs differ from normal signs (for example, PST or PDT or whatever we're in right now is shown as "UTC+7", but should be "UTC-7").
- The big dropdown of possible timezones lumps "UTC+X:30" timezones into "UTC+X".
Test Plan:
- Reconciled "America/Nome", saw negative UTC offsets for "America/Nome" and "America/Los_Angeles" (previously: improperly positive).
- Viewed the big timzone list, saw ":30" and ":45" timezones grouped/labeled more accurately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13263
Differential Revision: https://secure.phabricator.com/D20314
Summary:
See <https://discourse.phabricator-community.org/t/error-when-sending-a-message-chat-room/2548>.
Conpherence calls `setAfterID()` and `setBeforeID()` directly on a subquery, but these methods no longer exist.
Use a pager instead. This code probably shouldn't exist (we should use some other approach to fetch this data in most cases) but that's a larger change.
Test Plan: Sent messages in a Conpherence thread. Before: fatal; after: success. Viewed the Conphrence menu, loaded threads, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20318
Summary:
Ref T13091. The Ferret "rank" column is a function of the query text and looks something like `SELECT ..., 2 + 2 AS rank, ...`.
You can't apply conditions to this kind of dynamic column with a WHERE clause: you get a slightly unhelpful error like "column rank unknown in where clause". You must use HAVING:
```
mysql> SELECT 2 + 2 AS x WHERE x = 4;
ERROR 1054 (42S22): Unknown column 'x' in 'where clause'
mysql> SELECT 2 + 2 AS x HAVING x = 4;
+---+
| x |
+---+
| 4 |
+---+
1 row in set (0.00 sec)
```
Add a flag to paging column definitions to let them specify that they must be applied with HAVING, then apply the whole paging clause with HAVING if any column requires HAVING.
Test Plan:
- In Maniphest, ran a fulltext search matching more than 100 results, ordered by "Relevance", then clicked "Next Page".
- Before patch: query with `... WHERE rank > 123 OR ...` caused MySQL error because `rank` is not a WHERE-able column.
- After patch: query builds as `... HAVING rank > 123 OR ...`, pages properly, no MySQL error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13091
Differential Revision: https://secure.phabricator.com/D20298
Summary:
See downstream <https://phabricator.wikimedia.org/T210482>.
On mobile, the task graph can take up most of the screen. Hide it on devices. Keep it on the standalone view if you're really dedicated and willing to rotate your phone or whatever to see the lines.
Test Plan: Dragged window real narrow, saw graph hide.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20313
