phabricator.git
25 hours agoImprove rendering of "default value changed" custom form transactions to at least... master
epriestley [Wed, 19 Jun 2019 20:17:00 +0000 (13:17 -0700)]
Improve rendering of "default value changed" custom form transactions to at least have all the information

Summary:
Ref T13319. Currently, transactions about changes to a default form value use a raw internal key for the affected field and don't show the actual value change.

An ideal implementation will likely require us to specialize a great deal of rendering, but we can do much better than we currently do without too much work:

  - Try to pull the actual `EditField` object for the key so we can `getLabel()` it and get a human-readable label (like `Visible To` instead of `policy.view`).
  - Add a "(Show Changes)" action that dumps the raw values as more-or-less JSON, so you can at least figure out what happened if you're sophisticated enough.

Test Plan:
Before:

{F6516640}

After:

{F6516642}

The quality of "Show Details" varies a lot. For some fields, like "Description", it's pretty good:

{F6516645}

For others, like "Assigned To", it's better than nothing but pretty technical:

{F6516647}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13319

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

26 hours agoDon't count "Cc: x@y.com" as a legitimate recipient if the user who has "x@y.com...
epriestley [Wed, 19 Jun 2019 17:57:55 +0000 (10:57 -0700)]
Don't count "Cc: x@y.com" as a legitimate recipient if the user who has "x@y.com" attached to their account has not verified the address

Summary:
Fixes T13317. On `admin.phacility.com`, an enterprising user added `noreply@admin.phacility.com` to their account. This caused them to become CC'd on several support issues over the last year, because we send mail "From" this address and it can get CC'd via reply/reply all/whatever else.

The original driving goal here is that if I reply to a task email and CC you on my reply, that should count as a CC in Phabricator, since this aligns with user intent and keeps them in the loop.

This misfire on `noreply@` is ultimately harmless (being CC'd does not grant the user access permission, see T4411), but confusing and undesirable. Instead:

  - Don't allow reserved addresses ("noreply@", "ssladmin@", etc) to trigger this subscribe-via-CC behavior.
  - Only count verified addresses as legitimate user recipients.

Test Plan:
  - Added a `bin/mail receive-test --cc ...` flag to make this easier to test.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc bailey@verified.com`. Bailey was CC'd both before and after the change.
  - Sent mail as `bin/mail receive-test --to X --as alice --cc unverified@imaginary.com`, an address which Bailey has added to her account but not verified.
    - Before change: Bailey was CC'd on the task anyway.
    - After change: Bailey is not CC'd on the task.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13317

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

28 hours agoUse "link.getAttribute('href')", not "link.href", to bypass dark browser magic
epriestley [Wed, 19 Jun 2019 17:19:07 +0000 (10:19 -0700)]
Use "link.getAttribute('href')", not "link.href", to bypass dark browser magic

Summary:
Ref T13302. In at least some browsers (including Safari and Chrome), when you write this:

```
<a href="#">...</a>
```

...and then access `<that node>.href`, you get `http://local-domain-whatever.com/path/to/current/page#` back.

This is wonderful, but not what we want. Access the raw attribute value instead, which is `#` in all browsers.

Test Plan:
  - In Safari, Chrome, and Firefox:
  - Clicked "Edit Subtasks" from a task.
  - Clicked "Select" buttons to select several tasks.
  - Before: Clicking these button incorrectly closed the dialog (because of D20573).
  - After: Clicking these buttons now selects tasks without closing the dialog.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

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

47 hours agoReplace weird, redundant list of branches in Diffusion "Manage" UI with a link to...
epriestley [Mon, 17 Jun 2019 17:49:31 +0000 (10:49 -0700)]
Replace weird, redundant list of branches in Diffusion "Manage" UI with a link to the main branch list

Summary:
Fixes T13312. Currently, {nav Manage > Branches} has a list of branches on the same page. This has a few minor issues:

  - Pager is at the top (see T13312), which is weird.
  - "Default" icon is mystery meat.
  - Table is kind of pointless/redundant in general?

Previously, this table had more information about technical status of each branch (autoclose/track/publish) but most of these details have been simplified/eliminated, and the main "Branches" view now has more information than it did before.

Get rid of this and just link to the main view.

Test Plan: Viewed "Branches" in UI, saw a link to the main view instead of a weird table.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13312

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

47 hours agoWhen a user clicks a navigation link in a dialog, close the dialog
epriestley [Thu, 6 Jun 2019 01:42:42 +0000 (18:42 -0700)]
When a user clicks a navigation link in a dialog, close the dialog

Summary:
Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground.

Fix this by closing dialogs when users click navigation links inside them.

Test Plan:
With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog.

  - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs.
  - After, Quicksand Disabled: Dialog vanishes, then navigation occurs.
  - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it.
  - After, Quicksand Enabled: Dialog vanishes, then navigation occurs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

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

47 hours agoParse "shallow" frames in the Git "upload-pack" wire protocol parser
epriestley [Thu, 13 Jun 2019 23:28:30 +0000 (16:28 -0700)]
Parse "shallow" frames in the Git "upload-pack" wire protocol parser

Summary:
Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it.

We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way.

Test Plan:
  - Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk.
  - Cloned it with `git clone <uri>`.
  - Deleted all the refs inside it so the wire only has "shallow" frames; cloned it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13309

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

2 days agoFix missing link targets for "View Object" header buttons in HTML email
epriestley [Tue, 18 Jun 2019 14:22:15 +0000 (07:22 -0700)]
Fix missing link targets for "View Object" header buttons in HTML email

Summary:
See <https://discourse.phabricator-community.org/t/view-task-from-maniphest-e-mail-doesnt-have-url/2827>.

I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up.

Test Plan:
  - Commented on a task.
  - Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML.
  - Previewed the HTML in a browser.
  - This time, actually clicked the button to go to the task.

Reviewers: amckinley

Reviewed By: amckinley

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

2 days agoRecommend dumping database backups with "--compress --output X" instead of "| gzip...
epriestley [Wed, 5 Jun 2019 21:56:10 +0000 (14:56 -0700)]
Recommend dumping database backups with "--compress --output X" instead of "| gzip > X"

Summary:
Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use.

  - Recommend their use, since their error handling behavior is more robust in the face of issues like full disks.
  - If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this.
  - Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command.

Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13304

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

2 days agoFix more "msort()" vs "msortv()" callsites
epriestley [Tue, 18 Jun 2019 14:27:57 +0000 (07:27 -0700)]
Fix more "msort()" vs "msortv()" callsites

Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes.

These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`.

Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up.

Reviewers: amckinley

Reviewed By: amckinley

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

2 days agoIn "bin/repository reparse", continue on "PhabricatorWorkerPermanentFailureException"
epriestley [Tue, 18 Jun 2019 15:00:07 +0000 (08:00 -0700)]
In "bin/repository reparse", continue on "PhabricatorWorkerPermanentFailureException"

Summary:
Fixes T13315. See that task for discussion.

Without `--background`, we currently treat this as a catastrophic failure, but it's relatively routine for some repository states. We can safely continue reparsing other steps.

Test Plan: Ran `bin/repository reparse --all X --message` with commits faked to all be unreachable. Got warnings instead of a hard failure on first problem.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13315

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

3 days agoSkip loading transaction handles in an old migration
epriestley [Wed, 5 Jun 2019 18:41:36 +0000 (11:41 -0700)]
Skip loading transaction handles in an old migration

Summary:
Ref T13305. See that task for discussion.

This old migration may indirectly cause search index worker tasks to queue by loading handles. They'll fail since we later added `dateCreated` to the worker task table.

Use `needHandles(false)` (since we don't use them) to disable loading handles and avoid the problem.

Test Plan:
  - Ran `bin/storage upgrade -f` on an older instance (late 2016) and hit this issue.
  - Applied the patch, got a clean migration to modernity.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13305

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

3 days agoRespect repository identities when selecting author vs auditor actions
epriestley [Mon, 17 Jun 2019 17:08:08 +0000 (10:08 -0700)]
Respect repository identities when selecting author vs auditor actions

Summary:
Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification").

Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist.

(Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.)

Test Plan:
See D20580. As `@alice`, viewed the commit in the UI.

  - Before: got auditor actions presented to me.
  - After: got author actions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13311

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

3 days agoWhen triggering audits, respect committer identities when importing commits
epriestley [Mon, 17 Jun 2019 17:00:21 +0000 (10:00 -0700)]
When triggering audits, respect committer identities when importing commits

Summary:
Ref T13311. We currently don't use committer identity mappings when triggering audits, so if a user is only associated with an identity via manual mapping we won't treat them as the author.

Instead, use the identity and manual mapping if they're available.

Test Plan:
  - Pushed a commit as `xyz <xyz@example.org>`, an address with no corresponding user.
  - In the UI, manually associated that identity with user `@alice`.
  - Ran `bin/repository reparse --publish <hash>` to trigger audits and publishing for the commit.
  - Before: observed the `$author_phid` was `null`.
  - After: observed the `$author_phid` is Alice.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13311

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

3 days agoDon't require users be logged in to access the Logout controller, so users with no...
epriestley [Thu, 13 Jun 2019 23:55:57 +0000 (16:55 -0700)]
Don't require users be logged in to access the Logout controller, so users with no Spaces can log out

Summary:
Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support.

Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out.

For users who are already logged out, this just redirects home with no effect.

Test Plan:
  - As a user with access to no Spaces, logged out. (Before: error; after: worked).
  - As a logged-out user, logged out (was redirected).
  - As a normal user, logged out (normal logout).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13310

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

3 days agoIn "Download Raw Diff", engage the chunk engine to handle 8MB+ changes
epriestley [Mon, 17 Jun 2019 16:47:19 +0000 (09:47 -0700)]
In "Download Raw Diff", engage the chunk engine to handle 8MB+ changes

Summary:
Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files.

Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked.

This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now.

Test Plan:
  - Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff.
  - Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably").
  - Clicked "Download Raw Diff".
  - Before: misleading file storage engine error ("no engine can store this file").
  - After: large, raw diff response.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13313

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

3 days agoFix another stray "msort()/msortv()" issue
epriestley [Mon, 17 Jun 2019 20:10:07 +0000 (13:10 -0700)]
Fix another stray "msort()/msortv()" issue

Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly.

Test Plan: The system works!

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

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

3 days agoCorrect "msort()" vs "msortv()" to more fully stabilize transaction sorts after recen...
epriestley [Mon, 17 Jun 2019 17:36:59 +0000 (10:36 -0700)]
Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after recent changes

Summary: Ref T13303. I upgraded this to a vector-based sort but forgot to type a "v", which means the sort has different stability under PHP 5.5. See D20582 for a root cause fix.

Test Plan: Locally, on PHP7, not much changes. I expect this to fix the odd selection of title stories in mail and notification stories on `secure`, which is running PHP 5.5.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

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

3 days agoAllow "Sign with MFA" to be applied as a comment action without requiring "CAN_EDIT"
epriestley [Thu, 6 Jun 2019 02:21:13 +0000 (19:21 -0700)]
Allow "Sign with MFA" to be applied as a comment action without requiring "CAN_EDIT"

Summary:
Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT".

Since the actions like "Accept Revision" already work like this, the fix is one line.

Test Plan: {F6488135}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13307

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

5 days agoAdd "bin/file migrate" options to support import of a local-disk backup for Phacility...
epriestley [Wed, 5 Jun 2019 21:33:18 +0000 (14:33 -0700)]
Add "bin/file migrate" options to support import of a local-disk backup for Phacility instances

Summary:
Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance.

Add some more options to `bin/files migrate` to support this. In particular, this enables:

```
$ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup
```

...to import these files into S3 directly.

These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare.

Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports.

Reviewers: amckinley

Maniphest Tasks: T13306

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

2 weeks agoAdd a basic profiler to Herald transcripts
epriestley [Fri, 31 May 2019 14:22:10 +0000 (07:22 -0700)]
Add a basic profiler to Herald transcripts

Summary:
Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.

This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.

This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.

Test Plan: {F6473407}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13298

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

2 weeks agoDifferentiate Remarkup header sizes more clearly
epriestley [Tue, 4 Jun 2019 18:58:31 +0000 (11:58 -0700)]
Differentiate Remarkup header sizes more clearly

Summary:
Ref PHI1275. Previously, see T591. See also T7963. Headers are currently very visually similar to one another, and similar to the text size:

{F6485441}

I think the design intent was to make it hard to make bad-looking documents, but all the headers end up being very samey.

Differentiate the sizes of the headers better so they're much more obvious (e.g., when scrolling through a document) and the different levels are more distinct.

This might be a little overboard, but we can always pull it back a bit if it's too much, and I think giving users more control in Remarkup (in cases where it doesn't create some weird syntax/parsing nightmare) is generally a good thing.

Test Plan: {F6485447}

Reviewers: amckinley

Reviewed By: amckinley

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

2 weeks agoAdd "bin/herald rule ..." to modify Herald rules from the CLI
epriestley [Fri, 31 May 2019 14:56:59 +0000 (07:56 -0700)]
Add "bin/herald rule ..." to modify Herald rules from the CLI

Summary:
Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide.

Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`.

Test Plan: Used the CLI to disable and enable a personal rule.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13298

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

2 weeks agoWhen a revision is closed, always promote it out of draft
epriestley [Fri, 31 May 2019 13:15:28 +0000 (06:15 -0700)]
When a revision is closed, always promote it out of draft

Summary:
Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever.

Instead, count landing changes like this as a publishing action.

Test Plan:
  - Used `arc diff --hold` to create a revision, then pushed the commit immediately.
  - Before change: revision closed, but was stuck in draft.
  - After change: revision closed and was promoted out of draft.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13300

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

2 weeks agoWhen triggering audits, count "Accepted" revisions as successfully reviewed
epriestley [Thu, 30 May 2019 22:58:55 +0000 (15:58 -0700)]
When triggering audits, count "Accepted" revisions as successfully reviewed

Summary:
See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem.

Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes.

Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed".

Test Plan:
  - With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish <hash>` to republish the commit.
  - Before change: audit by package P triggered.
  - After change: audit by package P no longer triggered.

Reviewers: amckinley

Reviewed By: amckinley

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

2 weeks agoAfter reloading transactions for the recipient while building transaction mail, put...
epriestley [Thu, 30 May 2019 22:34:54 +0000 (15:34 -0700)]
After reloading transactions for the recipient while building transaction mail, put them in the input order

Summary:
Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions.

However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent.

Instead, reorder the transactions before continuing so mail transaction order is consistent.

Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

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

2 weeks agoAdd a "View Task" button to HTML mail from Maniphest
epriestley [Thu, 30 May 2019 17:11:54 +0000 (10:11 -0700)]
Add a "View Task" button to HTML mail from Maniphest

Summary:
See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.

It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.

Test Plan:
Used `bin/mail show-outbound --id <x> --dump-html`:

{F6470461}

Reviewers: amckinley

Reviewed By: amckinley

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

2 weeks agoRequire valid comments to contain at least one non-whitespace character
epriestley [Thu, 30 May 2019 19:23:51 +0000 (12:23 -0700)]
Require valid comments to contain at least one non-whitespace character

Summary:
See downstream <https://phabricator.wikimedia.org/T88655>. This is very marginal, but we currently allow comments consisting of //only// whitespace.

These are probably always mistakes, so treat them like completely empty comments.

(We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.)

Test Plan:
  - Posted empty, nonempty, and whitespace-only comments.
  - Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction).

Reviewers: amckinley

Reviewed By: amckinley

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

2 weeks agoClean up "phabricator.timezone" configuration instructions a little bit
epriestley [Thu, 30 May 2019 15:15:08 +0000 (08:15 -0700)]
Clean up "phabricator.timezone" configuration instructions a little bit

Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little.

Test Plan: d( O_o )b

Reviewers: amckinley

Reviewed By: amckinley

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

2 weeks agoRender timezone names more readably, with spaces rather than underscores ("America...
epriestley [Thu, 30 May 2019 15:02:39 +0000 (08:02 -0700)]
Render timezone names more readably, with spaces rather than underscores ("America/Los Angeles", not "America/Los_Angeles").

Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.

Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).

There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.

Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".

Reviewers: amckinley

Reviewed By: amckinley

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

3 weeks agoTest for "CAN_INTERACT" on comment edits in a way that survives objects which only...
epriestley [Tue, 28 May 2019 17:11:09 +0000 (10:11 -0700)]
Test for "CAN_INTERACT" on comment edits in a way that survives objects which only implement "CAN_VIEW"

Summary:
Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.

Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.

Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).

Reviewers: amckinley

Maniphest Tasks: T13289

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

3 weeks agoFix a looping workflow when trying to submit a partially-effectless transaction group
epriestley [Thu, 23 May 2019 18:41:14 +0000 (11:41 -0700)]
Fix a looping workflow when trying to submit a partially-effectless transaction group

Summary:
Ref T13289. If you do this:

  - Subscribe to a task (so we don't generate a subscribe side-effect later).
  - Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
  - Submit the transaction group.

...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".

If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.

Instead, retain the "continue" bit through the MFA.

Also, don't show "You can't sign an empty transaction group" if there's a comment.

See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.

Test Plan:
  - Went through the workflow above.
  - Before: looping workflow.
  - After: "Continue" carries through the MFA gate.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

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

3 weeks agoRemove the "search_documentfield" table
epriestley [Thu, 23 May 2019 16:51:23 +0000 (09:51 -0700)]
Remove the "search_documentfield" table

Summary: Ref T11741. See PHI1276. After the switch to "Ferret", this table has no remaining readers or writers.

Test Plan:
  - Ran `bin/storage upgrade -f`, no warnings.
  - Grepped for class name, table name, `stemmedCorpus` column; got no relevant hits.
  - Did a fulltext search.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11741

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

3 weeks agoPrevent editing and deleting comments in locked conversations
epriestley [Thu, 23 May 2019 17:55:55 +0000 (10:55 -0700)]
Prevent editing and deleting comments in locked conversations

Summary:
Ref T13289. This tightens up a couple of corner cases around locked threads.

Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.

Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.

Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.

Test Plan:
  - On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
  - On a locked task, tried to remove another user's comments as an administrator. This works.
  - On a normal task, edited comments normally.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

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

3 weeks agoFix two straggling pagination issues in Drydock
epriestley [Thu, 23 May 2019 18:52:43 +0000 (11:52 -0700)]
Fix two straggling pagination issues in Drydock

Summary:
Ref T13289. See <https://discourse.phabricator-community.org/t/fatal-error-in-pagination-in-drydock-resources-host-logs-all-logs/2735>.

`bin/drydock lease` and the web UI for reviewing all object logs when there is more than one page of logs didn't get fully updated to the new cursors.

  - Use a cursor pager in `bin/drydock lease`.
  - Implement `withIDs()` in `LeaseQuery` so the default paging works properly.

Test Plan:
  - Ran `bin/drydock lease`, got a lease with log output along the way.
  - Set page size to 2, viewed host logs with multiple pages, paged to page 2.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13289

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

4 weeks agoIn the Herald test console, don't consider transactions that Herald rules applied
epriestley [Wed, 22 May 2019 21:44:49 +0000 (14:44 -0700)]
In the Herald test console, don't consider transactions that Herald rules applied

Summary:
Depends on D20546. Ref T13283. Currently, if you do something (transactions "A", "B") and Herald does some things in response (transaction "C"), Herald acts only on the things you did ("A", "B") since the thing it did ("C") didn't exist yet, until it ran.

However, if you use the test console to test rules against the object we'll pick up all three transactions since they're all part of the same group. This isn't ideal.

To fix this, skip transactions which Herald applied, since it obviously didn't consider them when it was evaluating.

Test Plan:
  - Created a revision, in the presence of a Herald rule that adds reviewers.
  - Then, ran the revision through the test console.
  - Before: saw the "Herald added reviewers: ..." transaction in the transaction group Herald evaluated.
  - After: saw only authentic human transactions.

{F6464064}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

4 weeks agoUse the same transaction group ID for transactions applied indirectly by a sub-editor
epriestley [Wed, 22 May 2019 21:38:20 +0000 (14:38 -0700)]
Use the same transaction group ID for transactions applied indirectly by a sub-editor

Summary:
Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.

This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.

Test Plan:
  - Created a revision. Herald applied one transaction.
  - Used the test console.
  - Before: The test console only picked up the single most recent Herald transaction.
  - After: The test console picked up the whole transaction group.

{F6464059}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

4 weeks agoWhen creating a Phriction document, mark initial transactions as "create" transaction...
epriestley [Wed, 22 May 2019 22:10:19 +0000 (15:10 -0700)]
When creating a Phriction document, mark initial transactions as "create" transactions to fix weird email

Summary:
Ref T13289. When you create a Phriction document, you currently get an email with the whole new content as a "diff".

You also get extra transactions in the email and on the page.

This is because Phriction isn't on EditEngine and doesn't mark "create" transactions in a modern way. Get them marked properly to fix these obviously-broken behaviors. This can all go away once Phriction switches to EditEngine, although I don't have any particular plans to do that in the immediate future.

Test Plan:
  - Created a new document, viewed email, no longer saw redundant "edited content" transaction or "CHANGES TO CONTENT" diff.
  - Updated a document, viewed email, got interdiff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

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

4 weeks agoDrop the "update revision with commit diff" transaction if the revision is already...
epriestley [Wed, 22 May 2019 21:25:03 +0000 (14:25 -0700)]
Drop the "update revision with commit diff" transaction if the revision is already closed

Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.

We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)

To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.

This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.

Test Plan:
  - Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
  - Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
  - After: repeated runs had no effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13290

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

4 weeks agoPrevent "Differential Revision: ..." from counting as a mention in commit messages
epriestley [Wed, 22 May 2019 21:04:15 +0000 (14:04 -0700)]
Prevent "Differential Revision: ..." from counting as a mention in commit messages

Summary:
Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.

Stop it from doing that, since these mentions are silly/redundant/unintended.

The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".

Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291, T13290

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

4 weeks agoStabilize sorting of feed stories with similar strength
epriestley [Tue, 21 May 2019 23:57:35 +0000 (16:57 -0700)]
Stabilize sorting of feed stories with similar strength

Summary:
See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.

This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:

{F6463721}

Switch to `msortv()`, which is a stable sort. Previously, see also T6861.

If all transactions have the same strength, we'll now consistently pick the first one.

This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.

Test Plan:
Top story was published after this change and uses the chronologically first transaction as the title story.

Bottom story was published before this change and uses the chronologically second transaction as the title story.

Both stories have two transactions with the same strength ("create" + "add reviewer").

{F6463722}

Reviewers: amckinley

Reviewed By: amckinley

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

4 weeks agoStack chart functions in a more physical way
epriestley [Wed, 8 May 2019 19:43:55 +0000 (12:43 -0700)]
Stack chart functions in a more physical way

Summary:
Ref T13279. See that task for some discussion.

The accumulations of some of the datasets may be negative (e.g., if more tasks are moved out of a project than into it) which can lead to negative area in the stacked chart.

Introduce `min(...)` and `max(...)` to separate a function into points above or below some line, then mangle the areas to pick the negative and positive regions apart so they at least have a plausible physical interpretation and none of the areas are negative.

This is presumably not a final version, I'm just trying to produce a chart that isn't a sequence of overlapping regions with negative areas that is "technically" correct but not really possible to interpret.

Test Plan: {F6439195}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoStore charts earlier and build them out a little later
epriestley [Wed, 8 May 2019 16:29:28 +0000 (09:29 -0700)]
Store charts earlier and build them out a little later

Summary:
Ref T13279. Currently, we store a fairly low-level description of functions and datasets in a chart. This will create problems with (for example) translating function labels.

If you view a chart someone links you, it should say "El Charto" if you speak Spanish, not "The Chart" if the original viewer speaks English.

To support this, store a slightly higher level version of the chart: the chart engine key, plus configuration parameters. This is very similar to how SearchEngine works.

For example, the burndown chart now stores a list of project PHIDs, instead of a list of `[accumulate [sum [fact task.open <project-phid>]]]` functions.

(This leaves some serialization code with no callsites, but we may eventually have a "CustomChartEngine" which stores raw functions, so I'm leaving it for now.)

As a result, function labels provided by the chart engine are now translatable.

(Note that the actual chart is meaningless since the underlying facts can't be stacked like they're being stacked, as some are negative in some areas of their accumulation.)

Test Plan: {F6439121}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoAutomatically select the range for charts in a general way
epriestley [Wed, 8 May 2019 15:53:35 +0000 (08:53 -0700)]
Automatically select the range for charts in a general way

Summary:
Ref T13279. Replace the hard-coded default range with a range computed by examining the chart data.

Instead of having a "Dataset" return a blob of wire data, "Dataset" now returns a structure with raw wire data plus a range. I expect to add more structured data here in future changes (tooltip/hover event data, maybe function labels).

Test Plan: {F6439101}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoWrap "<min, max>" chart domain pairs in an "Interval" class
epriestley [Wed, 8 May 2019 15:40:30 +0000 (08:40 -0700)]
Wrap "<min, max>" chart domain pairs in an "Interval" class

Summary: Ref T13279. Slightly simplify domain handling by putting all the "[x, y]" stuff in an Interval class. I'm planning to do something similar for ranges next, so this should make that easierr.

Test Plan: Viewed chart, saw same chart as before.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoProvide chart function labels over the wire instead of making them up
epriestley [Wed, 8 May 2019 14:06:14 +0000 (07:06 -0700)]
Provide chart function labels over the wire instead of making them up

Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.

Test Plan: {F6438872}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoLabel important data on charts
epriestley [Wed, 8 May 2019 13:50:23 +0000 (06:50 -0700)]
Label important data on charts

Summary:
Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.

Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.

Test Plan: {F6438860}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoRoughly support stacked area charts
epriestley [Fri, 3 May 2019 19:26:35 +0000 (12:26 -0700)]
Roughly support stacked area charts

Summary:
Ref T13279. This adds support for:

  - Datasets can have types, like "stacked area".
  - Datasets can have multiple functions.
  - Charts can store dataset types and datasets with multiple functions.
  - Adds a "stacked area" dataset.
  - Makes D3 actually draw a stacked area chart.

Lots of rough edges here still, but the result looks slightly more like it's supposed to look.

D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.

Test Plan: {F6427780}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoUpdate D3 to the current version
epriestley [Fri, 3 May 2019 21:40:21 +0000 (14:40 -0700)]
Update D3 to the current version

Summary:
Ref T13279. Old D3 seems perfectly fine, but most of the good references seem to have been written by people who update D3 more than once every 10 years (???).

This requires some minor API changes, see next diff.

Test Plan: See next diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

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

4 weeks agoConsolidate burndown logic into a "BurndownChartEngine"
epriestley [Fri, 3 May 2019 19:14:10 +0000 (12:14 -0700)]
Consolidate burndown logic into a "BurndownChartEngine"

Summary:
Ref T13279. For now, we need to render burndowns from both Maniphest (legacy) and Projects (new prototype).

Consolidate this logic into a "BurndownChartEngine". I plan to expand this to work a bit like a "SearchEngine", and serve as a UI layer on top of the raw chart features.

The old "ChartEngine" is now "ChartRenderingEngine".

Test Plan:
  - Viewed burndowns ("burnups") in Maniphest.
  - Viewed burndowns in Projects.
  - Saw the same chart.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoAdd a "Reports" menu item to Projects
epriestley [Fri, 3 May 2019 18:44:05 +0000 (11:44 -0700)]
Add a "Reports" menu item to Projects

Summary:
Ref T13279. Since the use cases that have made it upstream are all for relatively complex charts (e.g., requiring aggregation and composition of multiple data series in nontrivial ways) I'm currently looking at an overall approach like this:

  - At least for now, Charts provides a low-level internal-only API for composing charts from raw datasets.
  - This is exposed to users through pre-built `SearchEngine`-like interfaces that provide a small number of more manageable controls (show chart from date X to date Y, show projects A, B, C), but not the full set of composition features (`compose(scale(2), cos())` and such).
  - Eventually, we may put more UI on the raw chart composition stuff and let you build your own fully custom charts by gluing together datasets and functions.
  - Or we may add this stuff in piecemeal to the higher-level UI as tools like "add goal line" or "add trend line" or whatever.

This will let the low-level API mature/evolve a bit before users get hold of it directly, if they ever do. Most requests today are likely satisfiable with a small number of chart engines plus raw API data access, so maybe UI access to flexible charting is far away.

Step toward this by adding a "Reports" section to projects. For now, this just renders a basic burnup for the current project. Followups will add an "Engine" layer above this and make the chart it produces more useful.

Test Plan: {F6426984}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

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

4 weeks agoStart the fact daemon in "bin/phd start"
epriestley [Tue, 30 Apr 2019 13:49:28 +0000 (06:49 -0700)]
Start the fact daemon in "bin/phd start"

Summary:
Depends on D20488. Ref T13279. When installs run `bin/phd start`, start the fact daemon alongside other daemons.

Since "Reports" in Maniphest now relies on Facts data, populate it.

Test Plan: Ran `bin/phd start`, saw the Fact daemon start.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

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

4 weeks agoFix handling of "null" domain values in Charts
epriestley [Tue, 30 Apr 2019 13:49:28 +0000 (06:49 -0700)]
Fix handling of "null" domain values in Charts

Summary: Depends on D20487. If you `min(1, 2, null)`, you get `null`. We want `1`.

Test Plan: Viewed a "burnup for project X" chart where one dataseries had no datapoints. Saw a sensible domain selected automatically.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

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

4 weeks agoRemove the legacy chart behavior from Maniphest
epriestley [Mon, 29 Apr 2019 19:00:58 +0000 (12:00 -0700)]
Remove the legacy chart behavior from Maniphest

Summary: Depends on D20486. Ref T13279. Now that the "Reports" UI uses a panel to draw a real chart from Facts, throw away the copy of the old code.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

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

4 weeks agoReplace the chart in Maniphest Reports with a chart driven by Facts
epriestley [Mon, 29 Apr 2019 18:55:22 +0000 (11:55 -0700)]
Replace the chart in Maniphest Reports with a chart driven by Facts

Summary:
Depends on D20485. Ref T13279. This removes the ad-hoc charting in Maniphest and replaces it with a Facts-based chart.

(To do this, we build a dashboard panel inline and render it.)

Test Plan: {F6412720}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoAdd a rough "Chart" Dashboard Panel
epriestley [Mon, 29 Apr 2019 18:23:38 +0000 (11:23 -0700)]
Add a rough "Chart" Dashboard Panel

Summary:
Depends on D20484. Ref T13279. Allows a chart to render as a panel.

Configuring these is currently quite low-level (you have to manually copy/paste a chart key in), but works well enough.

Test Plan: {F6412708}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoRender charts from storage instead of just one ad-hoc hard-coded chart
epriestley [Mon, 29 Apr 2019 18:18:42 +0000 (11:18 -0700)]
Render charts from storage instead of just one ad-hoc hard-coded chart

Summary:
Ref T13279. This changes the chart controller:

  - if we have no arguments, build a demo chart and redirect to it;
  - otherwise, load the specified chart from storage and render it.

This mostly prepares for "Chart" panels on dashboards.

Test Plan: Visited `/fact/chart/`, got redirected to a chart from storage.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

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

4 weeks agoAdd a "{src ...}" Remarkup rule to provide a more flexible way to reference source...
epriestley [Tue, 21 May 2019 17:19:49 +0000 (10:19 -0700)]
Add a "{src ...}" Remarkup rule to provide a more flexible way to reference source files in Diffusion

Summary: Depends on D20538. Ref T13291. We now recognize full source URIs, but encoding full URIs isn't super human-friendly and we can't do stuff like relative links with them. Add `{src ...}` as a way to get to this behavior that supports options and more flexible syntax.

Test Plan: {F6463607}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

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

4 weeks agoRecognize self-URI links to Diffusion files and give them special rendering behavior
epriestley [Tue, 21 May 2019 16:41:23 +0000 (09:41 -0700)]
Recognize self-URI links to Diffusion files and give them special rendering behavior

Summary:
Depends on D20530. Ref T13291. When users paste links to files in Diffusion into remarkup contexts, identify them and specialize the rendering.

When the URIs are embedded with `{...}`, parse them in more detail.

This is a lead-up to a `{src ...}` rule which will use the same `View` but give users more options to customize presentation.

Test Plan: {F6463580}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

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

4 weeks agoSupport "none()" in Differential to find revisions with no (un-resigned) reviewers
epriestley [Tue, 21 May 2019 14:22:38 +0000 (07:22 -0700)]
Support "none()" in Differential to find revisions with no (un-resigned) reviewers

Summary:
Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now.

Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche.

Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: <no constraint>" and got sensible results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

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

4 weeks agoSupport export of feed transactions to CSV/Excel/etc
epriestley [Tue, 21 May 2019 13:04:37 +0000 (06:04 -0700)]
Support export of feed transactions to CSV/Excel/etc

Summary: Depends on D20534. Ref T13294. Add export support so you can dump these out, print them on paper, notarize them, and store them in a box under a tree or whatever.

Test Plan: Exported transactions to a flat file, read the file.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13294

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

4 weeks agoSupport filtering feed transactions by object type
epriestley [Tue, 21 May 2019 12:40:11 +0000 (05:40 -0700)]
Support filtering feed transactions by object type

Summary: Depends on D20533. Allow querying for transactions of a specific object type, so you can run queries like "Show all edits to Herald rules between date X and Y".

Test Plan: {F6463478}

Reviewers: amckinley

Reviewed By: amckinley

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

4 weeks agoAdd support for querying feed transactions by author and date range
epriestley [Tue, 21 May 2019 12:23:31 +0000 (05:23 -0700)]
Add support for querying feed transactions by author and date range

Summary: Depends on D20531. Ref T13294. Enable finding raw transactions in particular date ranges or with particular authors.

Test Plan: {F6463471}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13294

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

4 weeks agoBuild a rough transaction-level view of Feed
epriestley [Mon, 20 May 2019 21:02:38 +0000 (14:02 -0700)]
Build a rough transaction-level view of Feed

Summary:
Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".

We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.

This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.

To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.

To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.

Test Plan: {F6463076}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13294

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

4 weeks agoTo improve wrapping behavior of rendered README files, don't use "PHUIDocumentView...
epriestley [Tue, 21 May 2019 13:31:24 +0000 (06:31 -0700)]
To improve wrapping behavior of rendered README files, don't use "PHUIDocumentView" in Diffusion

Summary:
See PHI1268. We currently do some weird width handling when rendering Diffusion readmes in a document directory view.

I think this came from D12330, which used `PHUIDocumentViewPro` to change the font, but we later reverted the font and were left with the `DocumentView`. Other changes after that modified `DocumentView` to have fixed-width behavior, but it doesn't make much sense here since the content panel is clearly rendered full-width.

Today, the `DocumentView` is a more structural element with methods like `setCurtain()`. Just get rid of it to simplify things, at least as a first step.

Test Plan:
Before:

{F6463493}

After:

{F6463492}

Reviewers: amckinley

Reviewed By: amckinley

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

4 weeks agoAdd a 15-second timeout to external service calls to fill Doorkeeper link tags
epriestley [Mon, 20 May 2019 18:40:05 +0000 (11:40 -0700)]
Add a 15-second timeout to external service calls to fill Doorkeeper link tags

Summary:
Depends on D20528. Ref T13291. Ref T13285. Currently, we don't put a timeout on external service calls when enriching URIs for external Asana/JIRA tasks.

Add a 15-second timeout so we'll do something reasonable-ish in the face of a downed service provider. Later, I plan to healthcheck Asana/JIRA providers in a generic way (see T13287) so we can stop making calls if they time out / fail too frequently.

Test Plan:
  - Linked to JIRA and Asana tasks in comments.
  - Set timeout to 0.0001 seconds, saw requests time out.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291, T13285

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

4 weeks agoImplement Asana and JIRA external links via HyperlinkEngineExtension, not separate...
epriestley [Mon, 20 May 2019 17:33:55 +0000 (10:33 -0700)]
Implement Asana and JIRA external links via HyperlinkEngineExtension, not separate Remarkup rules

Summary:
Depends on D20527. Ref T13291. Now that we have more flexible support for URI rewriting, use it for Doorkeeper URIs.

These are used when you set up Asana or JIRA and include the URI to an Asana task or a JIRA issue in a comment.

Test Plan:
  - Linked up to Asana and JIRA.
  - Put Asana and JIRA URIs in comments.
  - Saw the UI update to pull task titles from Asana / JIRA using my OAuth credentials.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

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

4 weeks agoWhen an object is referenced by URI, treat it like a mention
epriestley [Mon, 20 May 2019 16:57:31 +0000 (09:57 -0700)]
When an object is referenced by URI, treat it like a mention

Summary:
Ref T13291. Currently, `T123` is a mention and adds an "alice mentioned this on Txxx." to `T123`, but `https://install.com/T123` is not a mention.

Make the full URI a mention.

Test Plan: Commented a full URI, saw the target object get a mention story in its timeline.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291

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

4 weeks agoFix an issue where handles could load with the incorrect viewer when building mail...
epriestley [Sun, 19 May 2019 22:55:59 +0000 (15:55 -0700)]
Fix an issue where handles could load with the incorrect viewer when building mail about changes to related objects

Summary:
See <https://phabricator.wikimedia.org/T179591>. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.

Later, we introduced `HandlePool` and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.

When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.

Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of `setViewer()` and `clone` games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.

Test Plan:
  - Created task A (public) with subtask B (private).
  - Closed subtask B as a user with access to it.
  - Viewed mail sent to subscribers of task A who can not see subtask B.
    - Before change: mail discloses title of subtask B.
    - After change: mail properly labels subtask B as "Restricted Task".

Reviewers: amckinley

Reviewed By: amckinley

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

4 weeks agoSeparate the "configuration" and "evaluation" phases of chart functions
epriestley [Thu, 18 Apr 2019 23:42:17 +0000 (16:42 -0700)]
Separate the "configuration" and "evaluation" phases of chart functions

Summary:
Depends on D20446. Currently, chart functions are both configured through arguments and evaluated through arguments. This sort of conflates things and makes some logic more difficult than it should be.

Instead:

  - Function arguments are used to configure function behavior. For example, `scale(2)` configures a function which does `f(x) => 2 * x`.
  - Evaluation is now separate, after configuration.

We can get rid of "sourceFunction" (which was basically marking one argument as "this is the thing that gets piped in" in a weird magical way) and "canEvaluate()" and "impulse".

Sequences of functions are achieved with `compose(u, v, w)`, which configures a function `f(x) => w(v(u(x)))` (note order is left-to right, like piping `x | u | v | w` to produce `y`).

The new flow is:

  - Every chartable function is `compose(...)` at top level, and composes one or more functions. `compose(x)` is longhand for `id(x)`. This just gives us a root/anchor node.
  - Figure out a domain, through various means.
  - Ask the function for a list of good input X values in that domain. This lets function chains which include a "fact" with distinct datapoints tell us that we should evaluate those datapoints.
  - Pipe those X values through the function.
  - We get Y values out.
  - Draw those points.

Also:

  - Adds `accumluate()`.
  - Adds `sum()`, which is now easy to implement.
  - Adds `compose()`.
  - All functions can now always evaluate everywhere, they just return `null` if they are not defined at a given X.
  - Adds repeatable arguments for `compose(f, g, ...)` and `sum(f, g, ...)`.

Test Plan: {F6409890}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

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

4 weeks agoRemove obsolete Dashboard panel methods with no callsites
epriestley [Mon, 29 Apr 2019 18:23:33 +0000 (11:23 -0700)]
Remove obsolete Dashboard panel methods with no callsites

Summary: Ref T13272. Since the move to EditEngine, these methods have no callsites.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

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

4 weeks agoLabel transaction groups with a "group ID" so Herald can reconstruct them faithfully
epriestley [Thu, 16 May 2019 19:56:58 +0000 (12:56 -0700)]
Label transaction groups with a "group ID" so Herald can reconstruct them faithfully

Summary:
Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".

This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.

Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.

Test Plan:
  - Ran Herald Test Console, saw faithful selection of recent transactions.
  - Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
  - Called `transaction.search`, saw group ID information available.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

5 weeks agoImprove the performance of tab replacement in common cases
epriestley [Wed, 24 Apr 2019 03:26:13 +0000 (20:26 -0700)]
Improve the performance of tab replacement in common cases

Summary:
See PHI1210. For certain large inputs, we spend more time than we need to replacing tabs with spaces. Add some fast paths:

  - When a line only has tabs at the beginning of the line, we don't need to do as much work parsing the rest of the line.
  - When a line has no unicode characters, we don't need to vectorize it to get the right result.

Test Plan:
  - Added test coverage.
  - Profiled this, got a ~60x performance increase on a 36,000 line 3MB text file.

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoMake branch status more clear on Diffusion branches view
epriestley [Fri, 3 May 2019 17:10:25 +0000 (10:10 -0700)]
Make branch status more clear on Diffusion branches view

Summary:
See PHI1225. Ref T13277. In Diffusion, show "default", "permanent", or "not permanent" when looking at branches.

For repositories with 100 or fewer branches, put default and permanent branches on top.

Test Plan: {F6426814}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: leoluk

Maniphest Tasks: T13277

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

5 weeks agoExpose subscribers transaction data via Conduit "transaction.search"
epriestley [Thu, 9 May 2019 16:19:23 +0000 (09:19 -0700)]
Expose subscribers transaction data via Conduit "transaction.search"

Summary:
Depends on D20507. See PHI1232. Previously, see T13255 and D20209.

Since nothing seems to have exploded after "projects" was exposed, give "subscribers" the same treatment.

Test Plan: Added, removed, and modified subscribers. Queried transactions with "transaction.search", saw sensible "type" and data.

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoSpecialize rendering of self-URIs in the form "/X123"
epriestley [Thu, 9 May 2019 20:01:09 +0000 (13:01 -0700)]
Specialize rendering of self-URIs in the form "/X123"

Summary:
Depends on D20510. Ref T5378. When remarkup includes a hyperlink to the current install in the form "/X123" (which is common), load the corresponding object and specialize the rendering.

This doesn't cover everything (notably, no handling for Diffusion paths yet), but does cover a lot of the most common cases.

The "uri" form preserves the URI as written, but adds an icon, tag, and hovercard.

The "{uri}" form is more similar to `{T123}` and shows the object name.

Test Plan: {F6440367}

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Subscribers: joshuaspence

Maniphest Tasks: T5378

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

5 weeks agoWhen a user pastes a Phabricator URI into the search box, redirect to the URI
epriestley [Thu, 9 May 2019 18:20:09 +0000 (11:20 -0700)]
When a user pastes a Phabricator URI into the search box, redirect to the URI

Summary:
Depends on D20509. See PHI1224. Ref T5378. With some frequency, I paste URIs into the global search input (I am dumb).

When I do this dumb thing, redirect to the URI as though the global search was a URI bar.

Maybe only I am dumb like this, but I don't think it'll hurt anything.

Test Plan: pasted a URI and hit return; tried to eat a rock

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Maniphest Tasks: T5378

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

5 weeks agoRemove ancient "PhabricatorQuickSearchEngineExtension" compatibility class
epriestley [Thu, 9 May 2019 18:12:27 +0000 (11:12 -0700)]
Remove ancient "PhabricatorQuickSearchEngineExtension" compatibility class

Summary: Ref T5378. This class was renamed more than a year ago, in D19087. Remove the leftover compatiblity layer.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5378

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

5 weeks agoEnrich "draft", "summary", and "testPlan" transactions from Differential in "transact...
epriestley [Thu, 9 May 2019 16:04:45 +0000 (09:04 -0700)]
Enrich "draft", "summary", and "testPlan" transactions from Differential in "transaction.search"

Summary:
See PHI1232, which describes a reasonable use case for wanting information about the "draft" ("Hold as Draft / Do Not Auto-Promote") flag.

Also, flesh out "testPlan" and "summary". It's possible these "blob of remarkup" fields might have metadata some day (e.g., a rendered version or a list of PHIDs or something), but we could add more keys, and we already have some other transactions which work like this.

Test Plan: Used "transaction.search" to fetch these transaction types, saw type information and metadata.

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoAdd support for custom "Wait for Approval" instructions
epriestley [Fri, 3 May 2019 15:26:23 +0000 (08:26 -0700)]
Add support for custom "Wait for Approval" instructions

Summary:
See PHI1229. An install has a somewhat duct-taped registration flow which can dump users on the "Wait for Approval" screen without clear guidance. The desired guidance is something like "this is totally normal, just wait a bit for a bot to approve you".

Adding guidance here is generally reasonable and consistent with the intent of this feature.

Test Plan: {F6426583}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: kylec

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

5 weeks agoAdd a "Published document changed" rule to Herald for Phriction documents
epriestley [Mon, 13 May 2019 17:26:32 +0000 (10:26 -0700)]
Add a "Published document changed" rule to Herald for Phriction documents

Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.

Test Plan:
  - Wrote a Herald rule using this field.
  - Created a document (rule matched).
  - Edited a document (rule matched).
  - Edited a document, saving as a draft (no match).
  - Edited a draft, updating it (no match).
  - Published a draft docuemnt (rule matched).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

5 weeks agoWhen using the Herald test console on a transactional object, guess a reasonable...
epriestley [Mon, 13 May 2019 17:12:20 +0000 (10:12 -0700)]
When using the Herald test console on a transactional object, guess a reasonable set of transactions to simulate

Summary:
Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.

This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.

The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.

(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)

Test Plan: Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

5 weeks agoIn Herald, save applied transaction PHIDs in the transcript and display them in the UI
epriestley [Mon, 13 May 2019 16:53:49 +0000 (09:53 -0700)]
In Herald, save applied transaction PHIDs in the transcript and display them in the UI

Summary:
Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.

Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.

Test Plan:
  - Viewed a modern transcript, saw a list of transactions.
  - Viewed an older transcript, saw nothing (since there were no transactions in the transcript).

{F6456431}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

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

5 weeks agoWhen viewing a workboard, only link to parent workboards in crumbs if parents have...
epriestley [Mon, 13 May 2019 14:18:58 +0000 (07:18 -0700)]
When viewing a workboard, only link to parent workboards in crumbs if parents have workboards

Summary:
Depends on D20516. See PHI1247. In D20331, I made the crumbs on workboards point at ancestor workboards.

However, this isn't a great destination if an ancestor doesn't actually have a workboard. In this case, point at the normal profile URI instead.

Test Plan:
  - Viewed a milestone workboard with a parent that had no workboard. Saw a profile link instead of a workboard link (new behavior).
  - Viewed a milestone workboard with a parent that also had a workboard. Saw a workboard link (existing old behavior still works).

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoHide the "added a commit/revision" stories from feed and mail
epriestley [Fri, 3 May 2019 14:58:26 +0000 (07:58 -0700)]
Hide the "added a commit/revision" stories from feed and mail

Summary:
Ref T13276. Previously, these edges were added directly with an `EdgeEditor`, so they did not generate transaction stories.

Now, they're added properly, but they aren't terribly useful in feed/mail. Hide them in those contexts, like we already do with other types of similar edges.

Test Plan: Will verify behavior on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

5 weeks agoDon't mark subscribes/mentions as "Lock Overridden"
epriestley [Fri, 10 May 2019 18:41:43 +0000 (11:41 -0700)]
Don't mark subscribes/mentions as "Lock Overridden"

Summary:
See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:

  - You can subscribe/unsubscribe.
  - You can mention it on another object.
  - You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).

Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.

For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.

Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.

Test Plan:
  - Hard-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
  - Soft-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
    - Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
  - Added some comments and stuff to a normal non-locked task, no emblems.

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoFix a fatal when navigating to a Workboard after removing the Workboard menu item
epriestley [Mon, 13 May 2019 14:15:23 +0000 (07:15 -0700)]
Fix a fatal when navigating to a Workboard after removing the Workboard menu item

Summary:
See PHI1247. If you remove the Workboard from a project profile menu, then navigate to the URI, we currently fatal when trying to select the "Workboard" item.

Instead, only try to select the item if some matching item is present.

Test Plan:
  - Disabled the workboard on a project, navigated to `/board/`, got a sensible page with no navigation item selected instead of a fatal.
  - Viewed a normal workboard, saw the correct selection.

Reviewers: amckinley

Reviewed By: amckinley

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

5 weeks agoRemove WePay support from Phortune, and Restful/Httpful dependencies
epriestley [Mon, 13 May 2019 19:04:21 +0000 (12:04 -0700)]
Remove WePay support from Phortune, and Restful/Httpful dependencies

Summary: Ref PHI1166. I'm documenting our dependencies, and we have approximately 5,000 lines of external code to support WePay as a Phortune provider. We don't use it, I'm almost certain it doesn't work, and we have no plans to use it in the near future. If we did pursue it, I'd probably just wrap the API in a 100-line `WePayFuture` anyway since 5K lines of dependencies to make a couple method calls is ridiculous.

Test Plan: Grepped for `wepay`, `httpful`, `restful`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aurelijus

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

6 weeks agoFix a HunkParser typo, "identcal" -> "identical"
epriestley [Tue, 7 May 2019 13:28:58 +0000 (06:28 -0700)]
Fix a HunkParser typo, "identcal" -> "identical"

Summary: See rP3f8eccdaec8f85933bfea6ce5348bde25ced0489.

Test Plan: Read carefully.

Reviewers: amckinley, avivey

Reviewed By: avivey

Subscribers: avivey

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

7 weeks agoFix the direction of the commit/revision edge
epriestley [Wed, 1 May 2019 15:46:44 +0000 (08:46 -0700)]
Fix the direction of the commit/revision edge

Summary: Ref T13276. This edge is pointed the wrong way. Point it the right way.

Test Plan: Will verify production works better.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

7 weeks agoReally fix movable panels inside tab panels by changing the JX.Request serializer
epriestley [Thu, 25 Apr 2019 18:57:55 +0000 (11:57 -0700)]
Really fix movable panels inside tab panels by changing the JX.Request serializer

Summary:
Depends on D20475. Ref T13272. Currently, if you `JX.Request` with `data` like `{x: null}`, we submit that as `?x=null`, i.e. as though `null` was the string `"null"`.

This is weird and almost certainly never intended/desiarable. In particular, it causes a bug where panels embedded inside tab panels are incorrectly draggable.

It's possible this breaks something which relied on the buggy behavior, but that seems unlikely.

Test Plan: Tried to drag a panel inside a tab panel, it really truly didn't work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

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

7 weeks agoAdd "Move Left" and "Move Right" to dashboard tab panels
epriestley [Thu, 25 Apr 2019 18:36:40 +0000 (11:36 -0700)]
Add "Move Left" and "Move Right" to dashboard tab panels

Summary: Depends on D20474. Ref T13272. Provide an easy way to rearrange tabs on a tab panel, by moving them left or right from the context menu.

Test Plan: Moved tabs left and right. Tried to move them off the end of the tab list, no luck.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

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

7 weeks agoAdd a "Customize Query" action to query panels to make it easier to make minor query...
epriestley [Thu, 25 Apr 2019 18:03:17 +0000 (11:03 -0700)]
Add a "Customize Query" action to query panels to make it easier to make minor query adjustments

Summary:
Depends on D20473. Ref T13272. Fixes T7216. If you want to tweak the query a panel uses, you currently have to complete 7 Great Labors.

Instead, add a "Customize Query" action which lets you update the query inline.

Test Plan: {F6402171}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272, T7216

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

7 weeks agoAdd "Revision has passing builds" Herald rules for commit content (pushes) and commit...
epriestley [Tue, 23 Apr 2019 18:20:29 +0000 (11:20 -0700)]
Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery)

Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.

Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.

To accomplish this:

  - When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
  - If the revision is closed when we hit Herald, look for the flag.
  - If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.

Test Plan:

  - Wrote a "Require Green" rule.
  - Ran it against various commits with various build states (good, not good).
  - Fiddled with "Warn on Landing" and saw the effect in rule evaluation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

7 weeks agoUnify code for parsing "Reverts X" magic, and when something "reverts <hash>", also...
epriestley [Tue, 23 Apr 2019 17:57:51 +0000 (10:57 -0700)]
Unify code for parsing "Reverts X" magic, and when something "reverts <hash>", also revert associated revisions

Summary:
Depends on D20468. Ref T13276. See PHI1008.

When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.

Test Plan:
Created "reverts X" objects for:

  - a revision;
  - a commit;
  - a commit with a revision (both got marked properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

7 weeks agoConsolidate readers of "differential.revisionID" property
epriestley [Tue, 23 Apr 2019 17:15:34 +0000 (10:15 -0700)]
Consolidate readers of "differential.revisionID" property

Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.

Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.

Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:

  - Just write the edges (in "MessageParserWorker").
  - Hide the edges from mail.

However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.

For now, just reduce the amount of harm/badness here.

Test Plan:
  - Ran `bin/repository reparse --publish ...`.
  - Ran a Herald "Audit" rule using the "Accepted Differential revision" field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

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

7 weeks agoRestore "Limit" to dashboard Query panels
epriestley [Thu, 25 Apr 2019 17:28:41 +0000 (10:28 -0700)]
Restore "Limit" to dashboard Query panels

Summary: See PHI1220. Ref T13272. I accidentally left the ability to set a query limit behind when updating this.

Test Plan: Edited a query panel, set/removed the limit, tried to set an invalid limit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

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

7 weeks agoFix an issue where editing a dashboard panel from a dashboard could duplicate the...
epriestley [Thu, 25 Apr 2019 17:35:35 +0000 (10:35 -0700)]
Fix an issue where editing a dashboard panel from a dashboard could duplicate the panel

Summary:
Depends on D20472. Ref T13272. Currently, when you edit a panel from a dashboard, we try to add the panel to the dashboard. This always works since dashboards no longer enforce panel uniqueness, and you can end up with duplicate panels.

Instead, only add panels if we're creating them.

Test Plan:
  - Edited an existing panel, no duplication.
  - Created a new panel, saw it added to the dashboard.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

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

7 weeks agoUpdate Quickstart SQL
epriestley [Thu, 25 Apr 2019 22:34:42 +0000 (15:34 -0700)]
Update Quickstart SQL

Summary:
This hasn't been updated in a bit more than a year (last updated in D18594) and we've accumulated a fair number of SQL patches. Update it.

This is mostly automatic (with `bin/storage quickstart`), except:

  - Manual edit to one migration for a missed callsite to `DashboardInstall`.
  - Replaced two InnoDB tables that still have FULLTEXT indexes with MyISAM (see rP6cedd4a95cfc).

This is not really possible to review and more for reference than examination. `bin/storage quickstart` has historically worked correctly.

Test Plan: I have great faith that `bin/storage quickstart` is a script which creates a big `.sql` file.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

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

7 weeks agoSet a URI on Auth Messages, so the "Change Details" dialog from the transaction recor...
epriestley [Thu, 25 Apr 2019 15:08:56 +0000 (08:08 -0700)]
Set a URI on Auth Messages, so the "Change Details" dialog from the transaction record has a cancel button

Summary:
If you edit an auth message in Auth > Customize Messages, then click "Show Details" in the transaction record, the resulting dialog uses the object's handle's URI to generate a "cancel" button.

Since these handles currently have no URI, the dialog currently has no cancel/done button to close it.

Test Plan: Edited an auth message, clicked "Show Details", was now able to click "Done" to close the dialog.

Reviewers: amckinley

Reviewed By: amckinley

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

7 weeks agoMake very minor generality improvements to the scope selector
epriestley [Thu, 25 Apr 2019 22:26:48 +0000 (15:26 -0700)]
Make very minor generality improvements to the scope selector

Summary:
See PHI985. I think we pretty much need to start applying language-specific rules, but we can apply at least one more relatively language-agnostic rule: don't match lines which are indented 3+ levels.

In C++, we may have symbols like this:

```
class X {
  public:
    int m() { ... }
}
```

..but I believe no mainstream language puts symbol definitions 3+ levels deep.

Also clean up some of the tab handling very slightly.

Test Plan: Tests pass, looked at some C++ code and got slightly better (but still not great) matches.

Reviewers: amckinley

Reviewed By: amckinley

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