phabricator.git
30 hours agoRemove nearly all remaining references to "Autoclose" master
epriestley [Mon, 22 Apr 2019 23:25:37 +0000 (16:25 -0700)]
Remove nearly all remaining references to "Autoclose"

Summary:
Depends on D20464. Ref T13277. Broadly:

  - Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
  - Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").

Test Plan:
This could use some trial-by-fire on `secure`, but:

  - Grepped for all symbols.
  - Viewed various commits.
  - Reparsed commits.
  - Here's a commit with an explanation:

{F6394569}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20465

31 hours agoRemove "--force-autoclose" from "bin/repository reparse"
epriestley [Mon, 22 Apr 2019 23:08:37 +0000 (16:08 -0700)]
Remove "--force-autoclose" from "bin/repository reparse"

Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.

Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20464

32 hours agoMove "update related object after commit" to a separate worker in the task queue
epriestley [Wed, 10 Apr 2019 23:05:33 +0000 (16:05 -0700)]
Move "update related object after commit" to a separate worker in the task queue

Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.

For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.

Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.

I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.

Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20463

33 hours agoWhen applying transactions, acquire a read lock sooner
epriestley [Mon, 22 Apr 2019 22:13:36 +0000 (15:13 -0700)]
When applying transactions, acquire a read lock sooner

Summary:
Depends on D20461. Ref T13276. Ref T13054.

Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.

This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:

```
PROCESS A             PROCESS B
Old Value = Open      Old Value = Open
Transaction OK: Yes   Transaction OK: Yes
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      Apply Transactions
                      New Value = Closed
                      Release Lock
```

That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.

We actually want this:

```
PROCESS A             PROCESS B
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Old Value = Open      Wait...
Transaction OK: Yes   Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      >>> Old Value = Closed
                      >>> Transaction Has No Effect!
                      >>> Do Nothing / Abort
                      Release Lock
```

Move the "lock" part up so we do that.

This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.

Test Plan:
  - Added a `sleep(10)` before the lock.
  - Ran `bin/repository message --reparse X` in two console windows, where X is a commit that closes revision Y and Y is open.
    - Before patch: both windows closed the revision and added duplicate transactions.
    - After patch: only one of the processes had an effect.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13276, T13054

Differential Revision: https://secure.phabricator.com/D20462

33 hours agoRemove all remaining readers and writers for TABLE_COMMIT
epriestley [Mon, 22 Apr 2019 19:38:00 +0000 (12:38 -0700)]
Remove all remaining readers and writers for TABLE_COMMIT

Summary: Depends on D20459. Ref T13276. I'll file a followup to actually destroy the table.

Test Plan:
- Grepped for `TABLE_COMMIT`.
- Ran `bin/storage upgrade -f`, got a clean bill of health.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20461

33 hours agoAlign "RevisionQuery->needCommitPHIDs()" to use Edges, not the legacy table
epriestley [Mon, 22 Apr 2019 19:19:54 +0000 (12:19 -0700)]
Align "RevisionQuery->needCommitPHIDs()" to use Edges, not the legacy table

Summary:
Depends on D20458. Ref T13276. Although I'm not thrilled about "needCommitPHIDs()", it has a few callers, including custom fields. Allow "need + attach + get" to survive for now since they're reasonably modern, at least.

However, use edges instead of "TABLE_COMMIT" and require `need...()` + `get...()`, removing the direct `load...()`.

Also remove `RevisionQuery->withCommitPHIDs(...)`, which has no callers.

Test Plan:
- Grepped for `loadCommitPHIDs` (only two hits, the private `RevisionQuery` method).
- Called "differential.getrevision", got commits.
- Viewed a revision, saw "Commits: ...".
- Grepped for `withCommitPHIDs()`, no callers on `RevisionQuery` (some other query classes have methods with this name).
- Called "differential.query", got commits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20459

33 hours agoRemove all callers to "DifferentialRevision->loadIDsByCommitPHIDs()"
epriestley [Mon, 22 Apr 2019 18:54:42 +0000 (11:54 -0700)]
Remove all callers to "DifferentialRevision->loadIDsByCommitPHIDs()"

Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.

Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20458

33 hours agoMove BlameController away from ancient "TABLE_COMMIT"
epriestley [Mon, 22 Apr 2019 18:42:38 +0000 (11:42 -0700)]
Move BlameController away from ancient "TABLE_COMMIT"

Summary:
Ref T13276. Differential has a pre-edge "TABLE_COMMIT" with about a half-dozen weird callers I'd like to get rid of.

Move blame to use edges instead. (Bonus: this makes blame respect edge edits in the UI.)

Since there are some more callers to clean up this code may move into some "RelatedObjectQueryThing" class or something, but I'm taking it one step at a time for now.

Test Plan: {F6394106}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20457

2 days agoSimplify implementation of "pure" Chart functions
epriestley [Wed, 17 Apr 2019 21:40:36 +0000 (14:40 -0700)]
Simplify implementation of "pure" Chart functions

Summary:
Depends on D20445. Ref T13279. I'm not sure what the class tree of functions actually looks like, and I suspect it isn't really a tree, so I'm hesitant to start subclassing. Instead, try adding some `isSomethingSomething()` methods.

We have some different types of functions:

  # Some functions can be evaluated anywhere, like "constant(3)", which always evaluates to 3.
  # Some functions can't be evaluated anywhere, but have values everywhere in some domain. This is most interesting functions, like "number of open tasks". These functions also usually have a distinct set of interesting points, and are constant between those points (any count of anything, like "open points in project" or "tasks closed by alice", etc).
  # Some functions can be evaluated almost nowhere and have only discrete values. This is most of the data we actually store, which is just "+1" when a task is opened and "-1" when a task is closed.

Soon, I'd like to be able to show ("all tasks" - "open tasks") and draw a chart of closed tasks. This is somewhat tricky because the two datasets are of the second class of function (straight lines connecting dots) but their "interesting" x values won't be the same (users don't open and close tasks every second, or at the same time).

The "subtract X Y" function will need to be able to know that `subtract "all tasks" 3` and `subtract "all tasks" "closed tasks"` evaluate slightly differently.

To make this worse, the data we actually //store// is of the third class of function (just the "derivative" of the line chart), then we accumulate it in the application after we pull it out of the database. So the code will need to know that `subtract "derivative of all tasks" "derivative of closed tasks"` is meaningless, or the UI needs to make that clear, or it needs to interpret it to mean "accumulate the derivative into a line first".

Anyway, I'll sort that out in future changes. For now, simplify the easy case of functions in class (1), where they're just actual functions.

Add "shift(function, number)" and "scale(function, number)". These are probably like "mul" and "add" but they can't take two functions -- the second value must always be a constant. Maybe these will go away in the future and become `add(function, constant(3))` or something?

Test Plan: {F6382885}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20446

2 days agoMake chart function argument parsing modular/flexible with 900 pages of error messages
epriestley [Wed, 17 Apr 2019 17:45:27 +0000 (10:45 -0700)]
Make chart function argument parsing modular/flexible with 900 pages of error messages

Summary:
Depends on D20444. Ref T13279. Instead of ad-hoc parsing and messages, formalize chart function arguments.

Also, add a whole lot of extra type checking.

Test Plan: Built and charted various functions with various valid and invalid argument lists, got sensible-seeming errors and results.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20445

2 days agoChange "Revision Close" story to use commit identities only with no fallback to commi...
epriestley [Mon, 22 Apr 2019 19:42:39 +0000 (12:42 -0700)]
Change "Revision Close" story to use commit identities only with no fallback to commit data

Summary:
See PHI1213. If we don't have identities for "revision X closed by commit Y" stories, just do the plain non-attribution rendering rather than trying to fall back. Falling back won't work since we don't load the data, which should be OK now since identities seem like they're in generally good shape.

(We could probably just throw out the fallback behavior instead, but we can always clean things up later.)

Test Plan: Forced no commit identity on a revision, loaded it, saw reasonable story rendering.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20460

2 days agoFix "before/after" cursor paging for API call "feed.query"
epriestley [Mon, 22 Apr 2019 17:39:08 +0000 (10:39 -0700)]
Fix "before/after" cursor paging for API call "feed.query"

Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/undefined-method-setafterid-when-calling-feed-query/2653>.

This older API call needs an update to the newer paging/cursor API.

Test Plan: Called `feed.query` with an "after" parameter.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: Itms

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20456

2 days agoFix an issue where internal paging of notifications could fail if some notifications...
epriestley [Mon, 22 Apr 2019 14:22:00 +0000 (07:22 -0700)]
Fix an issue where internal paging of notifications could fail if some notifications are not visible

Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/notification-page-throws-unrecoverable-fatal-error/2651/>.

The "notifications" query currently uses offset paging for no apparent reason (just a legacy issue?), so some of the paging code is only reachable internally.

  - Stop it from using offset paging, since modern cursor paging is fine here (and Feed has used cursor paging for a long time).
  - Fix the non-offset paging to work like Feed.

Also:

  - Remove a couple of stub methods with no callsites after cursor refactoring.

Test Plan:
  - Set things up so I had more than 100 notifications and some in the first 100 were policy filtered, to reproduce the issue (I just made `FeedStory` return `NO_ONE` as a visibility policy).
  - Applied the patch, notifications now page cleanly.
  - Verified that "Next Page" took me to the right place in the result list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hskiba

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20455

6 days agoSimplify non-bare working copy rules for the new "git fetch" strategy
epriestley [Thu, 18 Apr 2019 13:52:15 +0000 (06:52 -0700)]
Simplify non-bare working copy rules for the new "git fetch" strategy

Summary:
Ref T13280. In D20421, I changed our observe strategy to `git fetch <uri>` in all cases.

This doesn't work in an ancient, non-bare repository if `master` is checked out and `master` is also fetch: `git` refuses to overwrite the local ref unless we pass `--update-head-ok`. Pass this flag.

Also, remove some code which examines branches and tags in a special way for non-bare working copies. The old `git fetch <origin>` code without explicit revsets meant that `refs/remotes/orgin/heads/xyz` got updated instead of `refs/heads/xyz`. We now update our local refs in all cases (bare and non-bare) so we can throw away this special casing.

Test Plan:
  - Replaced a modern bare working copy with a non-bare working copy by explicitly using `git clone` without `--bare`.
  - Ran `bin/repository update`, hit the `--update-head-ok` error. Applied the patch, got a clean update.
  - Used the "repository.branchquery" API method...
    - ...with "contains" to trigger the "git branch" case. Got identical results after removing the special casing.
    - ...without "contains" to trigger the "low level ref" case. Got identical results after removing the special casing.
  - Grepped for `isWorkingCopyBare()`. The only remaining callsites deal with hook paths, and genuinely need to be specialized.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13280

Differential Revision: https://secure.phabricator.com/D20450

6 days agoCopy the "line-chart" behavior to "line-chart-legacy" to keep "Maniphest > Reports...
epriestley [Thu, 18 Apr 2019 13:38:49 +0000 (06:38 -0700)]
Copy the "line-chart" behavior to "line-chart-legacy" to keep "Maniphest > Reports" working

Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.

Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.

Also fix a leftover typo from D20435.

Test Plan: Viewed a legacy Maniphest burnup rate report.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20449

6 days agoImprove timeline rendering for old macros
Austin McKinley [Fri, 19 Apr 2019 00:14:38 +0000 (17:14 -0700)]
Improve timeline rendering for old macros

Summary: This always bugs me when I'm going through `secure` looking for the spiciest macros.

Test Plan: Forced a date to be null, saw reasonable text.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20453

7 days agoAdd a "sin()" function to charts
epriestley [Wed, 17 Apr 2019 14:17:28 +0000 (07:17 -0700)]
Add a "sin()" function to charts

Summary:
Depends on D20443. Ref T13279. This is probably not terribly useful on its own, but is mostly a function which takes another function as an argument, and a step toward more useful functions like arithmetic and drawing a picture of an owl.

The only structural change here is that functions now read data parameters (domain, sample limit) using a more tailored "ChartDataQuery" instead of reading the actual axis. Mostly, I want a more cohesive representation of query state that can be easily passed to sub-functions, as here.

Test Plan: {F6382432}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20444

7 days agoAdd chart functions "x()" and "constant(3)"
epriestley [Wed, 17 Apr 2019 14:00:49 +0000 (07:00 -0700)]
Add chart functions "x()" and "constant(3)"

Summary:
Depends on D20442. Ref T13279. Add basic support for drawing chart functions that are not based on Facts first-party ETL datasets. Some general goals:

  - This might be useful to draw a line like "goal" or "profitability".
  - This might be useful to pull data from an external source.
  - For composable functions like "add" or "subtract", which are useful in manipulating ETL datasets, these value functions will make testing easier.

Test Plan:
Added a `constant(256)` function:

{F6382408}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20443

7 days agoDon't warn that workboard columns need a name when editing milestone columns
epriestley [Thu, 18 Apr 2019 12:30:07 +0000 (05:30 -0700)]
Don't warn that workboard columns need a name when editing milestone columns

Summary: See <https://discourse.phabricator-community.org/t/columns-must-have-a-name-while-editong-point-limit-on-milestone-column/2650>. This check doesn't make sense for proxy columns, including milestone columns.

Test Plan: Added a point limit to a milestone column.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20448

7 days agoSelect the domain (X-axis range) for charts before pulling data
epriestley [Wed, 17 Apr 2019 13:05:34 +0000 (06:05 -0700)]
Select the domain (X-axis range) for charts before pulling data

Summary:
Depends on D20441. Ref T13279. Currently, we pull all the data, then decide what the X-axis should look like.

Since users will reasonably want to do stuff like "show me march-april 2018" in the future, we need to move toward flipping this around so that we can support cases where the domain is specified by the user.

For actual chart functions (like "constant(3)" or "cos(x)"), we must also know the domain before we pull data, since there are an infinite number of places where we can evaluate the function "constant(3)".

See note in T13279 about continunity.

Test Plan: {F6382356}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20442

7 days agoSeparate chart functions into a class tree
epriestley [Wed, 17 Apr 2019 12:15:15 +0000 (05:15 -0700)]
Separate chart functions into a class tree

Summary:
Depends on D20440. Ref T13279. Create a class to represent a chartable function: something we can get some data points out of.

Then, make the chart chart two functions.

For now, the only supported function is "fact(key)", which pulls data from the Facts ETL pipeline, identified by "key", and takes no other arguments.

In future changes, I plan to support things like "fact(tasks.open.project, PHID-PROJ-xyz)", "constant(1000)" (e.g. to draw a goal line), "sum(fact(...), fact(...))" (to combine data from several projects), and so on.

The UI may not expose this level of power for a while (or maybe ever) but until we get close enough to the UI that these features are a ton of extra work I'm going to try to keep things fairly flexible/modular.

Test Plan: {F6382286}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20441

7 days agoFetch chart data via async request and redraw charts when the window is resized
epriestley [Wed, 17 Apr 2019 02:24:18 +0000 (19:24 -0700)]
Fetch chart data via async request and redraw charts when the window is resized

Summary:
Depends on D20439. Ref T13279. Some day, charts will probably need to reload themselves or do a bunch of defer/request-shaping magic when they're on a dashboard with 900 other charts.

Give the controller separate "HTML placeholder" and "actual data" modes, and make the placeholder fetch the data in a separate request.

Then, make the chart redraw if you resize the window instead of staying at whatever size it started as.

Test Plan:
  - Loaded a chart, saw it load data asynchronously.
  - Resized the window, saw the chart resize.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20440

7 days agoSupport drawing multiple functions on the same chart
epriestley [Wed, 17 Apr 2019 02:00:05 +0000 (19:00 -0700)]
Support drawing multiple functions on the same chart

Summary: Depends on D20438. Ref T13279. Widgets produced vs widgets sold, etc.

Test Plan: {F6381609}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20439

7 days agoFix negative chart values, add storage for charts
epriestley [Tue, 16 Apr 2019 18:08:59 +0000 (11:08 -0700)]
Fix negative chart values, add storage for charts

Summary:
Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:

The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:

    - I'll probably let users customize this.
    - We should likely select 0 as the automatic minimum for charts with no negative values.
    - For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.

We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.

Rename `fn()` to `css_function()`. This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, `fn()` was just particularly offensive.

Test Plan: Viewed a fact series with negative values. Ran `bin/storage upgrade`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20438

7 days agoIn Git repositories, use "git symbolic-ref HEAD ..." to select the default branch
epriestley [Tue, 16 Apr 2019 13:27:53 +0000 (06:27 -0700)]
In Git repositories, use "git symbolic-ref HEAD ..." to select the default branch

Summary:
Depends on D20434. Fixes T5963. Broadly, the issue here is that when:

  - You create a new, empty repository.
  - Then, you work on some branch other than `master`, without ever creating `master`.

...you get a warning on `git clone`:

> warning: remote HEAD refers to nonexistent ref, unable to checkout

To fix this, point the symbolic-ref HEAD at `refs/heads/<default-branch>` after installing commit hooks.

This fixes the warning, and also means that `git clone` will check out the repository default branch by default, which is nice.

There are a few caveats about this behavior (see T5963 for discussion) but nothing too substantial.

The only real issue is that Git prevents deletion of the default branch without a config setting. Just set that settting.

Test Plan:
See T5963.

In a repository, set `HEAD` to point somewhere invalid. Ran `bin/repository update ...`. Saw HEAD pointed back at the repository default branch.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5963

Differential Revision: https://secure.phabricator.com/D20435

7 days agoUpdate "Autoclose" documentation to focus on "Permanent Refs" instead
epriestley [Mon, 15 Apr 2019 19:44:37 +0000 (12:44 -0700)]
Update "Autoclose" documentation to focus on "Permanent Refs" instead

Summary:
Depends on D20433. Ref T13277. Since "Autoclose" no longer exists, update the documentation.

Currently, this documentation focuses a lot on troubleshooting because users historically had a lot of trouble with figuring out why things were or were not autoclosing. I haven't seen any real confusion about this in years, so I suspect we may have improved the import pipeline and/or UI to make this less of a problem.

It's also possible that this document "fixed" the problem, but usually I expect a documentation fix to not affect the frequency of reports, just make them easier to resolve, so I doubt it.

If unclear things remain //and// documentation really did fix it, maybe we can fix the issues. Or we can just put the troubleshooting documentation back.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20434

7 days agoDeprecate "Track Only" in the Diffusion UI
epriestley [Mon, 15 Apr 2019 19:26:25 +0000 (12:26 -0700)]
Deprecate "Track Only" in the Diffusion UI

Summary:
Depends on D20432. Ref T13277. Fixes T12967. Removes some "Track Only" hints and warns that the feature is deprecated in favor of "Permanent Refs" and "Fetch Only".

(This "fixes" T12967 by mooting it.)

Test Plan: Viewed "branches" sectino of the manage UI, edited "braches" section of a repository.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T12967

Differential Revision: https://secure.phabricator.com/D20433

7 days agoRename some internal "Autoclose" mentions to "Permanent Refs"
epriestley [Mon, 15 Apr 2019 19:18:46 +0000 (12:18 -0700)]
Rename some internal "Autoclose" mentions to "Permanent Refs"

Summary: Depends on D20428. Ref T13277.

Test Plan: Grep / reading.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20432

7 days agoWhen a commit appears as an ancestor of a permanent ref for the first time, run all...
epriestley [Mon, 15 Apr 2019 16:59:08 +0000 (09:59 -0700)]
When a commit appears as an ancestor of a permanent ref for the first time, run all import steps

Summary:
Depends on D20427. Ref T13277. As an optimization, when we discover that a commit which was previously only on a non-permanent ref ("tmp-epriestley-123") is now reachable from a permanent ref ("master"), we currently queue only a new "message" parse step.

This is an optimization because these commits previously got the full treatment (feed, publish, audit, etc) as soon as they were discovered. Now, those steps only happen once a commit is reachable from a permanent ref, so we need to run everything.

Test Plan:
  - Pushed local "tmp-123" branch to remote tag "tmp-123".
  - Updated repository with "bin/repository update", saw commit import as a "not on any permanent ref" commit, with no herald/audit/etc.
  - Merged "tmp-123" tag into "master".
  - Pushed new "master".
  - Updated repository with "bin/repository refs ... --trace --verbose", saw commit detected as now reachable from a permament ref.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20428

7 days agoDeprecate the "Commit is on autoclose/permanent branch" Herald "Commit" field
epriestley [Mon, 15 Apr 2019 16:44:39 +0000 (09:44 -0700)]
Deprecate the "Commit is on autoclose/permanent branch" Herald "Commit" field

Summary:
Depends on D20426. Ref T13277. The new behavior is to fire Herald only once a commit becomes reachable from a permanent ref (previously, an "Autoclose" branch).

That means that every "Commit" Herald rule implicitly has this field as a condition, and it no longer does anything.

Test Plan: Wrote a Herald rule, saw this as an option in the "Deprecated" section.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20427

7 days agoExpose repository ref rules via "diffusion.repository.search"
epriestley [Mon, 15 Apr 2019 13:36:17 +0000 (06:36 -0700)]
Expose repository ref rules via "diffusion.repository.search"

Summary:
Depends on D20425. Ref T13277. See PHI1067. There's currently no way to retrieve branch/ref rules over the API, which makes some management operations against a large number of repositories difficult.

Expose these rules to the API.

Test Plan: Called `diffusion.repository.search`, got rules in the result set.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20426

7 days agoMove the repository "Publishing" option to the "Basics" panel in repository management
epriestley [Mon, 15 Apr 2019 13:27:32 +0000 (06:27 -0700)]
Move the repository "Publishing" option to the "Basics" panel in repository management

Summary:
Depends on D20424. Ref T13277. Now that the "Actions" panel only has one item ("Publishing"), just move it to the "Basics" panel.

Update the UI to show active/publishing status more clearly and relate them to one another and importing state.

Test Plan: {F6378087}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20425

7 days agoIn repository settings, fold "Autoclose On/Off" into "Publishing On/Off"
epriestley [Sun, 14 Apr 2019 20:38:16 +0000 (13:38 -0700)]
In repository settings, fold "Autoclose On/Off" into "Publishing On/Off"

Summary:
Depends on D20423. Ref T13277. Repositories currently have separate toggles for "Autoclose" and "Publishing".

Merge the "Autoclose" toggle into the "Publishing" toggle. I'm unaware of any valid use case for enabling one but not the other.

(This doesn't fix all the documentation, yet.)

Test Plan: Edited a repository, saw only one publishing option.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20424

7 days agoRename most user-facing "Autoclose" strings to "Permanent Refs"
epriestley [Sun, 14 Apr 2019 20:20:23 +0000 (13:20 -0700)]
Rename most user-facing "Autoclose" strings to "Permanent Refs"

Summary:
Depends on D20422. Ref T13277. Currently, "track only", "publish", and "autoclose" are three separate ideas. I'd like to generally merge them into a more natural idea called "permanent refs".

Since "Autoclose" effectively now controls both "autoclose" and "publish", rename it.

This doesn't rename all the methods or internals, and the documentation needs an update, but it renames most of the UI-facing stuff.

(You also can only specify branches as "Permanent Refs" today, but we may let you specify tags and other arbitrary refs in the future.)

Test Plan: Grepped, poked around the UI, saw UI show "Permanent" / "Permanent Refs" more often and "Autoclose" less.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20423

7 days agoAdd "Fetch Rules" to observed Git repositories
epriestley [Sun, 14 Apr 2019 19:17:38 +0000 (12:17 -0700)]
Add "Fetch Rules" to observed Git repositories

Summary:
Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only".

Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote.

Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify:

```
refs/heads/master
```

If you only want to fetch branches and tags, you can use:

```
refs/heads/*
refs/tags/*
```

In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions.

Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20422

7 days agoWhen observing a repository in Git, just "fetch <url>" without worrying about the...
epriestley [Sun, 14 Apr 2019 19:21:48 +0000 (12:21 -0700)]
When observing a repository in Git, just "fetch <url>" without worrying about the "origin" remote

Summary:
Depends on D20420. Ref T13277. We currently spend substantial effort trying to detect and correct the URL of the "origin" remote in Git repositories.

I believe this is unnecessary, and we can always `git fetch <url> ...` to get the desired result instead of `git muck-with-origin + git fetch origin ...`. We already do this in the more recent parts of the codebase (e.g., intracluster sync) and it works correctly in every case I'm aware of.

Test Plan:

  - Grepped for `origin`, ` origin `.
  - Ran `bin/repository update ...` to fetch a mirrored repository.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20421

7 days agoDo not publish/notify about commits which are not reachable from any "Autoclose" ref
epriestley [Sun, 14 Apr 2019 18:13:42 +0000 (11:13 -0700)]
Do not publish/notify about commits which are not reachable from any "Autoclose" ref

Summary:
Depends on D20418. Ref T13277. Fixes T11314.

Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions.

However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.

  - Stop publishing these commits.
  - In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."

These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").

Test Plan:
  - Pushed a commit to `refs/tags/quack`.
  - Before: got a feed story.
  - After: no feed story, UI shows commit as "Not Permanent".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T11314

Differential Revision: https://secure.phabricator.com/D20419

7 days agoMake the "git upload-pack" proxy more robust
epriestley [Tue, 16 Apr 2019 16:08:39 +0000 (09:08 -0700)]
Make the "git upload-pack" proxy more robust

Summary:
Depends on D20381. Ref T8093. This makes minor improvements to the protocol proxy to handle cases where we add, remove, or replace refs and may need to move the "capabilities" section.

Rather than invoking a callback on every ref: parse the whole ref list into a data structure, mutate it if necessary (in a future diff), then dump it back into wire format.

This allows us to shift the capabilities data (which needs to be coupled with the first ref) around if we modify the first ref, and reorder the reflist alphabetically like git does.

When the server has no refs, Git sends no capabilities data. This is easy to emulate, just surprising.

Test Plan:
Tested the cases not covered by D20381:

  - Fetching where the fetch actually fetches data.
  - `ls-remote` when we hide the first ref (capabilities data properly moves to the first visible ref).
  - `ls-remote` when the remote is empty (we just drop the capabilities frame completely).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20436

7 days agoProxy the "git upload-pack" wire protocol
epriestley [Mon, 8 Apr 2019 15:46:47 +0000 (08:46 -0700)]
Proxy the "git upload-pack" wire protocol

Summary:
Depends on D20380. Ref T8093. When prototypes are enabled, inject a (hopefully?) no-op proxy into the Git wire protocol.

This proxy decodes "git upload-pack" and allows the list of references to be rewritten, in a similar way to how we already proxy the Subversion protocol to rewrite URIs and proxy the Mercurial protocol to distinguish between read and write operations.

The piece we care about comes at the beginning, and looks like this:

```
<frame-length><ref-hash> <ref-name>\0<server-capabilities>\n
<frame-length><ref-hash> <ref-name>\n
<frame-length><ref-hash> <ref-name>\n
...
<0000>
```

We can add, remove, or modify this section to make it appear that the server has different refs than the refs that exist on disk.

Things I have tried:

  - `git ls-remote`
  - `git ls-remote` where the server hides some refs.
  - `git fetch` where the fetch is a no-op.

Things I have not tried:

  - `git fetch` where the fetch is not a no-op.
  - Tricking things into doing protocol v2. Or: I tried this, I wasn't successful. In v2, additional "\0" tricks are used to hide data in the capabilities, I think?
  - `git ls-remote` where we rewrite/hide the first ref in the list, and need to move the capabilities frame elsewhere.
  - `git ls-remote` where the server has no refs at all, or we remove every ref.

So the "interesting" piece of this works, but it almost certainly needs some cleanup to survive interaction with the real world.

Test Plan: See above.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20381

7 days agoAdd an optional protocol log to `git` SSH workflows
epriestley [Sat, 6 Apr 2019 17:31:58 +0000 (10:31 -0700)]
Add an optional protocol log to `git` SSH workflows

Summary:
Ref T8093. Support dumping the protocol bytes to a side channel logfile, as a precursor to parsing the protocol and rewriting protocol frames to virtualize refs.

The protocol itself is mostly ASCII text so the raw protocol bytes are pretty comprehensible.

Test Plan:
{F6363221}

{F6363222}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8093

Differential Revision: https://secure.phabricator.com/D20380

7 days agoDon't warn about a locked database value after users run "bin/auth lock"
epriestley [Thu, 18 Apr 2019 00:23:19 +0000 (17:23 -0700)]
Don't warn about a locked database value after users run "bin/auth lock"

Summary: Ref T7667. Also one text fix.

Test Plan: Ran `bin/auth lock`, didn't get a replacement setup issue.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7667

Differential Revision: https://secure.phabricator.com/D20447

7 days agoHide revision dependency stories from feed/notifications
epriestley [Tue, 16 Apr 2019 19:52:11 +0000 (12:52 -0700)]
Hide revision dependency stories from feed/notifications

Summary:
See PHI1134. Generally, "alice added a dependent revision: ..." isn't a very interesting story. This relationship itself is valuable, but the creation of the relationship is usually pretty obvious from context.

In the specific case of PHI1134, various scripts are racing one another, but I don't think this story is of much value in the general case anyway.

Test Plan: Edited parent/child revisions, no more feed stories.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20437

8 days agoWhen an invalid "constraints" parameter is provided to a "*.search" method, raise...
epriestley [Mon, 15 Apr 2019 18:48:07 +0000 (11:48 -0700)]
When an invalid "constraints" parameter is provided to a "*.search" method, raise a more tailored error

Summary:
See <https://discourse.phabricator-community.org/t/constraints-parameter-type-handling-in-maniphest-search/2636>.

(The caller provided `constraints={...}` via cURL, but you can't mix JSON and HTTP parameters like that.)

Test Plan: Called `curl 'http://local.phacility.com/api/maniphest.search?api.token=...&constraints=X'` (with a valid API token), got a more sensible error.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20429

8 days agoAccept pushes with arbitrary Git refs
epriestley [Sun, 14 Apr 2019 18:50:20 +0000 (11:50 -0700)]
Accept pushes with arbitrary Git refs

Summary:
Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.

Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).

Allow these writes as affecting raw refs.

Test Plan:
  - Pushed an arbitrary ref.
  - Pushed some Git notes.
  - Wrote a Herald ref rule, saw "ref" in the dropdown.

{F6376492}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277, T8936, T9383, T12300

Differential Revision: https://secure.phabricator.com/D20420

8 days agoDistinguish between "bad record format" and "bad record value" when validating Trigge...
epriestley [Sat, 13 Apr 2019 16:29:37 +0000 (09:29 -0700)]
Distinguish between "bad record format" and "bad record value" when validating Trigger rules

Summary:
Depends on D20416. Ref T13269. See D20329.

If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.

Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.

Test Plan:
First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):

{F6374205}

Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:

{F6374211}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20417

8 days agoUse repository identities, not denormalized strings, to identify authors for "Revisio...
epriestley [Sun, 14 Apr 2019 17:46:20 +0000 (10:46 -0700)]
Use repository identities, not denormalized strings, to identify authors for "Revision closed by commit X" stories

Summary:
Ref T12164. Ref T13276. Currently, the parsing pipeline copies the author and committer names and PHIDs into the transcaction record as metadata. They are then rendered directly from the metadata.

This makes planned changes to the parsing pipeline (to prevent races when multiple commits matching a single revision are pushed simultaneously) more difficult, and generally won't work with repository identities.

Instead, load the commit and use its identities.

Test Plan: Loaded a revision, saw the same story rendering for a "Closed by commit" story.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276, T12164

Differential Revision: https://secure.phabricator.com/D20418

8 days agoImprove trigger editor behavior when switching to/from tokenizers
epriestley [Sat, 13 Apr 2019 16:02:30 +0000 (09:02 -0700)]
Improve trigger editor behavior when switching to/from tokenizers

Summary:
Ref T13269. See D20329. When we switch trigger rule control types, reset the rule value.

Also, pick slightly nicer defaults for status/priority.

Test Plan:
  - Created a "Change Status To: X" rule.
  - Saved it.
  - Edited it.
  - Selected "Assign to" for the existing action's dropdown.
  - Before: tokenizer filled with nonsense.
  - After: tokenizer cleared.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20416

8 days agoReduce drag-and-drop jank on dashboards
epriestley [Fri, 12 Apr 2019 23:50:59 +0000 (16:50 -0700)]
Reduce drag-and-drop jank on dashboards

Summary:
Depends on D20414. Ref T13272. Several minor things here:

  - Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
  - Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
  - Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
  - Add or remove some icons or something.

Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20415

8 days agoFix the "Add Query to Dashboard..." flow from "Use Results" on search result pages
epriestley [Fri, 12 Apr 2019 23:46:00 +0000 (16:46 -0700)]
Fix the "Add Query to Dashboard..." flow from "Use Results" on search result pages

Summary:
Depends on D20413. Ref T13272. When you search for stuff, you can "Use Results > Add to Dashboard" to generate a query panel.

This needs some updating after the recent refactoring. All the changes are pretty straightforward. Swaps a giant `<select />` for a tokenizer with a datasource.

Test Plan: Used the "Use Results > Add to Dashboard" flow to create a panel on a dashboard using a query.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20414

8 days agoIndex and show Owners packages affected by Herald rules
epriestley [Fri, 12 Apr 2019 23:09:38 +0000 (16:09 -0700)]
Index and show Owners packages affected by Herald rules

Summary:
Depends on D20412. See PHI1147.

  - Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
  - Add a "Related Herald Rules" panel to Owners Package pages.
  - Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.

Test Plan:
Ran migration, verified all rules reindexed.

{F6372695}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20413

8 days agoReindex dashboards and panels (allow migrations to queue a job to queue other indexin...
epriestley [Fri, 12 Apr 2019 21:17:58 +0000 (14:17 -0700)]
Reindex dashboards and panels (allow migrations to queue a job to queue other indexing jobs)

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

8 days agoSome formatting changes for showing auth provider config guidance
Austin McKinley [Thu, 11 Apr 2019 20:02:04 +0000 (13:02 -0700)]
Some formatting changes for showing auth provider config guidance

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

9 days agoShow a warning when "git" is too old to support filesize limits
epriestley [Mon, 15 Apr 2019 19:09:00 +0000 (12:09 -0700)]
Show a warning when "git" is too old to support filesize limits

Summary: See <https://discourse.phabricator-community.org/t/git-push-failed-with-filesize-limit-on/2635/2>

Test Plan: {F6378519}

Reviewers: amckinley, avivey

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D20431

9 days agoUpdate documentation for "uri.allowed-protocols"
epriestley [Mon, 15 Apr 2019 18:58:51 +0000 (11:58 -0700)]
Update documentation for "uri.allowed-protocols"

Summary: See <https://discourse.phabricator-community.org/t/download-tasks-and-others-as-excel-throw-exception/>.

Test Plan: Read config.

Reviewers: amckinley, avivey

Reviewed By: avivey

Subscribers: avivey

Differential Revision: https://secure.phabricator.com/D20430

11 days agoUpdate search indexes for Dashboards and Panels to Ferret, plus various minor fixes
epriestley [Fri, 12 Apr 2019 19:42:02 +0000 (12:42 -0700)]
Update search indexes for Dashboards and Panels to Ferret, plus various minor fixes

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

11 days agoRemove legacy "DashboardInstall" table
epriestley [Fri, 12 Apr 2019 19:22:20 +0000 (12:22 -0700)]
Remove legacy "DashboardInstall" table

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

11 days agoMake "Move Panel" on dashboards use the new storage and transactions
epriestley [Fri, 12 Apr 2019 19:06:11 +0000 (12:06 -0700)]
Make "Move Panel" on dashboards use the new storage and transactions

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

11 days agoUpdate "add panel" and "remove panel" Dashboard flows to the new panel storage format
epriestley [Fri, 12 Apr 2019 17:39:11 +0000 (10:39 -0700)]
Update "add panel" and "remove panel" Dashboard flows to the new panel storage format

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

11 days agoConvert dashboard read/display pathways to the new panel storage format
epriestley [Fri, 12 Apr 2019 15:34:48 +0000 (08:34 -0700)]
Convert dashboard read/display pathways to the new panel storage format

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

11 days agoModularize dashboard layout modes (one column, two columns, etc)
epriestley [Fri, 12 Apr 2019 15:34:43 +0000 (08:34 -0700)]
Modularize dashboard layout modes (one column, two columns, etc)

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

11 days agoConvert dashboard panel storage to a format which can handle duplicates
epriestley [Fri, 12 Apr 2019 15:03:06 +0000 (08:03 -0700)]
Convert dashboard panel storage to a format which can handle duplicates

Summary:
Ref T13272. See PHI945. Currently, dashboards tend to break when they have duplicate panels. Partly, this is because all the edit operations operate on a "panelPHID", so there's no way to say "remove the copy of panel X at the bottom of the right-hand column", since the operation is `remove(phid)` and that doesn't point at a specific copy of that panel.

In theory, the code is supposed to prevent duplicate panels, but (a) it doesn't always do this successfully and (b) there's no real reason you can't put duplicate panels on a dashboard if you want. There may even be good reason to do this if you have a "random cat picture" panel or something. Even if you aren't doing this on purpose, it's probably better to let you do it and then fix your mistake by removing the panel you don't want than to prevent the operation entirely.

To simplify this whole mess, I want to just support putting the same panel into multiple places on a dashboard. As a first step, change the storage format so each instance of a panel has a unique "panelKey".

Since each instance of each panel now has its own object, this will also let us give particular instances of panels things like "automatic refresh time" (T5514) or "custom name for this panel on this dashboard" later, if we want. Not clear these are valuable but having this capability can't hurt.

Test Plan:
  - `var_dump()`'d the migration, looked at all the results.
  - Ran the migration.

NOTE: This breaks dashboards on its own since none of the other code has been changed yet, see followups.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20405

13 days agoFix a missing pht() in Ponder
epriestley [Fri, 12 Apr 2019 13:38:57 +0000 (06:38 -0700)]
Fix a missing pht() in Ponder

Summary: See <https://discourse.phabricator-community.org/t/translation-in-ponder/2630>.

Test Plan: Viewed a Ponder question page with no answers yet.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20404

13 days agoMove Dashboards main edit flow to EditEngine
epriestley [Thu, 11 Apr 2019 22:28:08 +0000 (15:28 -0700)]
Move Dashboards main edit flow to EditEngine

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

13 days agoModularize Dashboard transactions
epriestley [Thu, 11 Apr 2019 20:56:24 +0000 (13:56 -0700)]
Modularize Dashboard transactions

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

13 days agoOn panel pages, show where panels are used
epriestley [Thu, 11 Apr 2019 18:35:34 +0000 (11:35 -0700)]
On panel pages, show where panels are used

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

13 days agoIndex and surface usage sites for Dashboards
epriestley [Thu, 11 Apr 2019 16:58:48 +0000 (09:58 -0700)]
Index and surface usage sites for Dashboards

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

13 days agoWhen editing a tab panel from a dashboard, redirect back to the dashboard
epriestley [Thu, 11 Apr 2019 16:13:43 +0000 (09:13 -0700)]
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.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20397

13 days agoOn Dashboard tab panels in edit mode, make the "Tab Name" and the "Dropdown Edit...
epriestley [Thu, 11 Apr 2019 14:12:13 +0000 (07:12 -0700)]
On Dashboard tab panels in edit mode, make the "Tab Name" and the "Dropdown Edit Caret" into different links

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

2 weeks agoRemove very old "vsDiff" data from commit update / diff extraction pipeline
epriestley [Thu, 11 Apr 2019 13:03:16 +0000 (06:03 -0700)]
Remove very old "vsDiff" data from commit update / diff extraction pipeline

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

2 weeks agoAdd a workflow and a new config option for locking authentication providers
Austin McKinley [Wed, 10 Apr 2019 22:26:04 +0000 (15:26 -0700)]
Add a workflow and a new config option for locking authentication providers

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

2 weeks agoMake Portals indexable with Ferret
epriestley [Wed, 10 Apr 2019 18:05:49 +0000 (11:05 -0700)]
Make Portals indexable with Ferret

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

2 weeks agoChange docs to be consistent with script
Austin McKinley [Wed, 10 Apr 2019 19:25:10 +0000 (12:25 -0700)]
Change docs to be consistent with script

Summary:
Ref https://discourse.phabricator-community.org/t/upgrade-article-process-doesnt-match-script/2598.

I personally prefer "stop the server and then pull" to reduce the risk of scary "served half the request from one version and half from another", but I think this is hard to pull off with correctly configured APC caches.

Test Plan: Mk.1 eyeball

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20391

2 weeks agoAdd trigger rule to remove projects from tasks
Austin McKinley [Wed, 10 Apr 2019 19:13:55 +0000 (12:13 -0700)]
Add trigger rule to remove projects from tasks

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

2 weeks agoAdd trigger rule for adding projects to a task
Austin McKinley [Fri, 5 Apr 2019 19:48:18 +0000 (12:48 -0700)]
Add trigger rule for adding projects to a task

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

2 weeks agoFix two minor main menu bar CSS issues
epriestley [Wed, 10 Apr 2019 16:59:13 +0000 (09:59 -0700)]
Fix two minor main menu bar CSS issues

Summary:
Ref T13263.

  - Make the user profile section of the "Profile" dropdown menu have a transparent background, not a white background. This is a pre-existing issue. This is normally hard to see, but visible on Workboards with custom background colors.
  - Fix an alignment issue with the little "V" caret in the search scope dropdown. This is a recent issue caused by some tab-caret CSS I added recently for tabbed dashboard panels.

Test Plan:
Before:

{F6367723}

After:

{F6367724}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13263

Differential Revision: https://secure.phabricator.com/D20388

2 weeks agoSurface custom form instructions as a "Remarkup" field for the transaction layer
epriestley [Wed, 10 Apr 2019 16:34:03 +0000 (09:34 -0700)]
Surface custom form instructions as a "Remarkup" field for the transaction layer

Summary: Ref T13263. See <https://discourse.phabricator-community.org/t/image-uploads-for-forms-too-restricted-by-default/2594>. Currently, when you add instructions to a custom form, we don't expose them as remarkup, so `{Fxxx}` references don't get attached correctly and won't get proper permissions.

Test Plan:
Dragged-and-dropped an image into form instructions, saw the file attach to the form:

{F6367710}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13263

Differential Revision: https://secure.phabricator.com/D20387

2 weeks agoSupport "JIRA Issue URIs" as a Herald field for revisions
epriestley [Wed, 10 Apr 2019 16:09:34 +0000 (09:09 -0700)]
Support "JIRA Issue URIs" as a Herald field for revisions

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

2 weeks agoRebuild Dashboards on EditEngine: v1 Major Jank Edition
epriestley [Mon, 8 Apr 2019 17:29:26 +0000 (10:29 -0700)]
Rebuild Dashboards on EditEngine: v1 Major Jank Edition

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

2 weeks agoFix some overlooked profile menu construction callsites
epriestley [Tue, 9 Apr 2019 21:21:29 +0000 (14:21 -0700)]
Fix some overlooked profile menu construction callsites

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

2 weeks agoWhen a dropdown menu would render in a way that hides it offscreen, try a different...
epriestley [Mon, 8 Apr 2019 17:29:05 +0000 (10:29 -0700)]
When a dropdown menu would render in a way that hides it offscreen, try a different alignment

Summary:
Depends on D20382. Ref T13272. When something near the edge of the screen has a dropdown menu, we currently may render the menu offscreen.

Instead, keep the menu onscreen.

(This is happening because I'm adding dropdown menus to tab query panels.)

Test Plan:
Before:

{F6363339}

After:

{F6363340}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20383

2 weeks agoRebuild query panels on top of EditEngine
epriestley [Mon, 8 Apr 2019 16:58:42 +0000 (09:58 -0700)]
Rebuild query panels on top of EditEngine

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

2 weeks agoWhen a dashboard has inconsistent panel policies: keep doing nothing
epriestley [Wed, 3 Apr 2019 16:20:20 +0000 (09:20 -0700)]
When a dashboard has inconsistent panel policies: keep doing nothing

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

2 weeks agoOn Dashboards, distinguish between invalid panels and restricted panels
epriestley [Wed, 3 Apr 2019 13:59:28 +0000 (06:59 -0700)]
On Dashboards, distinguish between invalid panels and restricted panels

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

2 weeks agoMake "Favorites" work more like other customizable menus
epriestley [Tue, 2 Apr 2019 20:37:57 +0000 (13:37 -0700)]
Make "Favorites" work more like other customizable menus

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

2 weeks agoMake the Dashboard dropshadow a little lighter and turn panel management into a menu
epriestley [Tue, 2 Apr 2019 19:08:05 +0000 (12:08 -0700)]
Make the Dashboard dropshadow a little lighter and turn panel management into a menu

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

2 weeks agoUse EditEngine, not CustomFields, to define Dashboard Panel edit behavior
epriestley [Tue, 2 Apr 2019 18:15:04 +0000 (11:15 -0700)]
Use EditEngine, not CustomFields, to define Dashboard Panel edit behavior

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

2 weeks agoMostly replace the old panel "Edit" controller with the new "Editpro" controller
epriestley [Tue, 2 Apr 2019 16:18:55 +0000 (09:18 -0700)]
Mostly replace the old panel "Edit" controller with the new "Editpro" controller

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

2 weeks agoModularize Dashboard Panel transactionns
epriestley [Tue, 2 Apr 2019 16:54:44 +0000 (09:54 -0700)]
Modularize Dashboard Panel transactionns

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

2 weeks agoMove Dashboard panel controllers into a "panel/" subdirectory
epriestley [Tue, 2 Apr 2019 16:14:42 +0000 (09:14 -0700)]
Move Dashboard panel controllers into a "panel/" subdirectory

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

2 weeks agoPut "workflow" on Dashboard edit links when they're disabled
epriestley [Tue, 2 Apr 2019 16:09:12 +0000 (09:09 -0700)]
Put "workflow" on Dashboard edit links when they're disabled

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

2 weeks agoRemove the dashboard "template" workflow
epriestley [Tue, 2 Apr 2019 16:01:08 +0000 (09:01 -0700)]
Remove the dashboard "template" workflow

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

2 weeks agoMake the "Install Dashboard" flow smoother
epriestley [Mon, 1 Apr 2019 17:25:24 +0000 (10:25 -0700)]
Make the "Install Dashboard" flow smoother

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

2 weeks agoCombine the "View", "Arrange", and "Manage" modes of Dashboards into a single mode
epriestley [Mon, 1 Apr 2019 15:34:34 +0000 (08:34 -0700)]
Combine the "View", "Arrange", and "Manage" modes of Dashboards into a single mode

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

2 weeks agoAdd a trigger rule to reassign a task
Austin McKinley [Thu, 4 Apr 2019 23:30:42 +0000 (16:30 -0700)]
Add a trigger rule to reassign a task

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

3 weeks agoFix rendering of offset-paged query panels including "Notifications"
epriestley [Tue, 2 Apr 2019 12:40:28 +0000 (05:40 -0700)]
Fix rendering of offset-paged query panels including "Notifications"

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

3 weeks agoFix "Actions" button on Phame standalone/live pages (bonus: JX.sprintf())
epriestley [Wed, 3 Apr 2019 16:56:49 +0000 (09:56 -0700)]
Fix "Actions" button on Phame standalone/live pages (bonus: JX.sprintf())

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

3 weeks agoMake Portals reachable without knowing the URI
epriestley [Sun, 31 Mar 2019 21:48:23 +0000 (14:48 -0700)]
Make Portals reachable without knowing the URI

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

3 weeks agoFix the incorrect link target for "Create Revision" as a Menu Item
epriestley [Sun, 31 Mar 2019 21:26:05 +0000 (14:26 -0700)]
Fix the incorrect link target for "Create Revision" as a Menu Item

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

3 weeks agoWhen picking a default menu item to render, don't pick disabled items
epriestley [Sun, 31 Mar 2019 20:58:30 +0000 (13:58 -0700)]
When picking a default menu item to render, don't pick disabled items

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

3 weeks agoUpdate all existing ProfileMenuItems for the more-structured API
epriestley [Sun, 31 Mar 2019 19:07:05 +0000 (12:07 -0700)]
Update all existing ProfileMenuItems for the more-structured API

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