1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00
Commit graph

14888 commits

Author SHA1 Message Date
epriestley
5b22412f24 Support data export on push logs
Summary: Depends on D18967. Ref T13049. Nothing too fancy going on here.

Test Plan: Exported push logs, looked at the export, seemed sensible.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18968
2018-01-30 11:19:20 -08:00
epriestley
a5b8be0316 Support export of user activity logs
Summary:
Depends on D18966. Ref T13049. Adds export support to user activity logs.

These don't have PHIDs. We could add them, but just make the "phid" column test if the objects have PHIDs or not for now.

Test Plan:
  - Exported user activity logs, got sensible output (with no PHIDs).
  - Exported some users to make sure I didn't break PHIDs, got an export with PHIDs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18967
2018-01-30 11:12:32 -08:00
epriestley
91108cf838 Upgrade user account activity logs to modern construction
Summary: Depends on D18965. Ref T13049. Move this Query and SearchEngine to be a little more modern, to prepare for Export support.

Test Plan:
  - Used all the query fields, viewed activity logs via People and Settings.
  - I'm not sure the "Session" query is used/useful and may remove it before I'm done here, but I just left it in place for now.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18966
2018-01-30 11:11:47 -08:00
epriestley
b27fd05eef Add a bin/bulk export CLI tool to make debugging and profiling large exports easier
Summary:
Ref T13049. When stuff executes asynchronously on the bulk workflow it can be hard to inspect directly, and/or a pain to test because you have to go through a bunch of steps to run it again.

Make future work here easier by making export triggerable from the CLI. This makes it easy to repeat, inspect with `--trace`, profile with `--xprofile`, etc.

Test Plan:
  - Ran several invalid commands, got sensible error messages.
  - Ran some valid commands, got exported data.
  - Used `--xprofile` to look at the profile for a 300MB dump of 100K tasks which took about 40 seconds to export. Nothing jumped out as sketchy to me -- CustomField wrangling is a little slow but most of the time looked like it was being spent legitimately.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18965
2018-01-30 11:11:13 -08:00
epriestley
84df122085 When exporting more than 1,000 records, export in the background
Summary:
Depends on D18961. Ref T13049. Currently, longer exports don't give the user any feedback, and exports that take longer than 30 seconds are likely to timeout.

For small exports (up to 1,000 rows) continue doing the export in the web process.

For large exports, queue a bulk job and do them in the workers instead. This sends the user through the bulk operation UI and is similar to bulk edits. It's a little clunky for now, but you get your data at the end, which is far better than hanging for 30 seconds and then fataling.

Test Plan: Exported small result sets, got the same workflow as before. Exported very large result sets, went through the bulk flow, got reasonable results out.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18962
2018-01-29 16:08:02 -08:00
epriestley
ea58b6acea Remove the old, non-modular Excel export workflow from Maniphest
Summary:
Depends on D18960. Ref T13049. Now that Maniphest fully supports "Export Data", remove the old hard-coded version.

This is a backward compatibility break with the handful of installs that might have defined a custom export by subclassing `ManiphestExcelFormat`. I suspect this is almost zero installs, and that the additional data in the new format may serve most of the needs of this tiny number of installs. They can upgrade to `ExportEngineExtensions` fairly easily if this isn't true.

Test Plan:
  - Viewed Maniphest, no longer saw the old export workflow.
  - Grepped for `export` and similar strings to try to hunt everything down.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18961
2018-01-29 16:06:13 -08:00
epriestley
c00838878a Implement common infrastructure fields as export extensions
Summary:
Depends on D18959. Ref T13049. Provide tags, subscribers, spaces, and created/modified as automatic extensions for all objects which support them.

(Also, for JSON export, be a little more consistent about exporting `null` instead of empty string when there's no value in a text field.)

Test Plan: Exported users and tasks, saw relevant fields in the export.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18960
2018-01-29 16:05:32 -08:00
epriestley
2ac4e1991b Support new data export infrastructure in Maniphest
Summary: Depends on D18958. Ref T13049. Support the new stuff. There are a couple more fields this needs to strictly improve on the old export, but I'll add them as extensions shortly.

Test Plan: Exported tasks to Excel, saw reasonble-looking data in the export.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18959
2018-01-29 16:04:39 -08:00
epriestley
00b4eae1f4 When PHPExcel is not installed, detect it and provide install instructions
Summary:
Depends on D18957. Ref T13049. To do Excel exports, PHPExcel needs to be installed on the system somewhere.

This library is enormous (1K files, ~100K SLOC), which is why we don't just include it in `externals/`. This install process is a little weird and we could improve it, but users don't seem to have too much difficulty with it. This shouldn't be worse than the existing workflow in Maniphest, and I tried to make it at least slightly more clear.

Test Plan: Uninstalled PHPExcel, got it marked "Unavailable" and got reasonably-helpful-ish guidance on how to get it to work. Reinstalled, exported, got a sheet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18958
2018-01-29 16:03:34 -08:00
epriestley
61b8c12970 Make the data export format selector remember your last setting
Summary:
Depends on D18956. Ref T13049. Make the "Export Format" selector sticky.

This is partly selfish, since it makes testing format changes a bit easier.

It also seems like it's probably a good behavior in general: if you export to Excel once, that's probably what you're going to pick next time.

Test Plan: Exported to excel. Exported again, got excel as the default option.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18957
2018-01-29 16:01:54 -08:00
epriestley
5b61f863fd Organize the export code into subdirectories
Summary:
Depends on D18955. Ref T13049. This directory was getting a little cluttered with different kinds of code.

Put the formats (csv, json, ...), the field types (int, string, epoch, ...) and the engine-related stuff in subdirectories.

Test Plan: wow so aesthetic

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18956
2018-01-29 16:01:02 -08:00
epriestley
0409279595 Support Excel as a data export format
Summary:
Depends on D18954. Ref T13049. This brings over the existing Maniphest Excel export pipeline in a generic way.

The `<Type>ExportField` classes know directly that `PHPExcel` exists, which is a little sketchy, but writing an Excel indirection layer sounds like a lot of work and I don't anticipate us changing Excel backends anytime soon, so trying to abstract this feels YAGNI.

This doesn't bring over the install instructions for PHPExcel or the detection of whether or not it exists. I'll bring that over in a future change.

Test Plan: Exported users as Excel, opened them up, got a sensible-looking Excel sheet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18955
2018-01-29 16:00:41 -08:00
epriestley
a067f64ebb Support export engine extensions and implement an extension for custom fields
Summary:
Depends on D18953. Ref T13049. Allow applications and infrastructure to supplement exportable fields for objects.

Then, implement an extension for custom fields. Only a couple field types (int, string) are supported for now.

Test Plan: Added some custom fields to Users, populated them, exported users. Saw custom fields in the export.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18954
2018-01-29 15:59:58 -08:00
epriestley
8b8a3142b3 Support export of data in files larger than 8MB
Summary:
Depends on D18952. Ref T13049. For files larger than 8MB, we need to engage the chunk storage engine. `PhabricatorFile::newFromFileData()` always writes a single chunk, and can't handle files larger than the mandatory chunk threshold (8MB).

Use `IteratorUploadSource`, which can, and "stream" the data into it. This should raise the limit from 8MB to 2GB (maximum size of a string in PHP).

If we need to go above 2GB we could stream CSV and text pretty easily, and JSON without too much trouble, but Excel might be trickier. Hopefully no one is trying to export 2GB+ datafiles, though.

Test Plan:
  - Changed the JSON exporter to just export 8MB of the letter "q": `return str_repeat('q', 1024 * 1024 * 9);`.
  - Before change: fatal, "no storage engine can store this file".
  - After change: export works cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18953
2018-01-29 15:58:34 -08:00
epriestley
0de6210808 Give data exporters a header row
Summary:
Depends on D18951. Ref T13049. When we export to CSV or plain text, add a header row in the first line of the file to explain what each column means. This often isn't obvious with PHIDs, etc.

JSON has keys and is essentially self-labeling, so don't do anything special.

Test Plan: Exported CSV and text, saw new headers. Exported JSON, no changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18952
2018-01-29 15:17:30 -08:00
epriestley
213eb8e93d Define common ID and PHID export fields in SearchEngine
Summary:
Ref T13049. All exportable objects should always have these fields, so make them builtins.

This also sets things up for extensions (like custom fields).

Test Plan: Exported user data, got the same export as before.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18951
2018-01-29 15:17:00 -08:00
epriestley
1db281bcd1 Fix a possible count(null) in PHUIInfoView
Summary:
See <https://discourse.phabricator-community.org/t/php7-2-exception-count-parameter-must-be-an-array-or-an-object-that-implements-countable/1031>.

PHP7.2 raises a warning about `count(scalar)` (GREAT!) and we have one here if the caller doesn't `setErrors(...)`.

Test Plan: Sanity-checked usage of `$this->errors`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18964
2018-01-29 15:16:41 -08:00
epriestley
40e9806e3c Remove the caret dropdown from transaction lists when no actions are available
Summary:
See PHI325. When a transaction group in Differential (or Pholio) only has an inline comment, it renders with a "V" caret but no actual dropdown menu.

This caret renders in a "disabled" color, but the color is "kinda grey". The "active" color is "kinda grey with a dab of blue". Here's what they look like today:

{F5401581}

Just remove it.

Test Plan: Viewed one of these, no longer saw the inactive caret.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18963
2018-01-29 15:14:59 -08:00
epriestley
d8f51dff6e Use the configured viewer more consistently in the Herald commit adapter
Summary: See PHI276. Ref T13048. The fix in D18933 got one callsite, but missed the one in the `callConduit()` method, so the issue isn't fully fixed in production. Convert this adapter to use a real viewer (if one is available) more thoroughly.

Test Plan: Ran rules in test console, saw field values. Will test in production again.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18950
2018-01-29 15:00:26 -08:00
epriestley
98402b885b Add a bit of test coverage for bulky vs compact edge data representations
Summary: Depends on D18948. Ref T13051. The actual logic ended up so simple that this doesn't really feel terribly valuable, but maybe it'll catch something later on.

Test Plan: Ran test.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13051

Differential Revision: https://secure.phabricator.com/D18949
2018-01-29 11:34:57 -08:00
epriestley
6d2d1d3a97 Add bin/garbage compact-edges to compact edges into the new format
Summary:
Depends on D18947. Ref T13051. This goes through transaction tables and compacts the edge storage into the slim format.

I put this on `bin/garbage` instead of `bin/storage` because `bin/storage` has a lot of weird stuff about how it manages databases so that it can run before configuration (e.g., all the `--user`, `--password` type flags for configuring DB connections).

Test Plan:
Loaded an object with a bunch of transactions. Ran migration. Spot checked table for sanity. Loaded another copy of the object in the web UI, compared the two pages, saw no user-visible changes.

Here's a concrete example of the migration effect -- old row:

```
*************************** 44. row ***************************
             id: 757
           phid: PHID-XACT-PSTE-5gnaaway2vnyen5
     authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
     objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
     viewPolicy: public
     editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
    commentPHID: NULL
 commentVersion: 0
transactionType: core:edge
       oldValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]}}
       newValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]},"PHID-PROJ-tbowhnwinujwhb346q36":{"dst":"PHID-PROJ-tbowhnwinujwhb346q36","type":41,"data":[]},"PHID-PROJ-izrto7uflimduo6uw2tp":{"dst":"PHID-PROJ-izrto7uflimduo6uw2tp","type":41,"data":[]}}
  contentSource: {"source":"web","params":[]}
       metadata: {"edge:type":41}
    dateCreated: 1450197571
   dateModified: 1450197571
```

New row:

```
*************************** 44. row ***************************
             id: 757
           phid: PHID-XACT-PSTE-5gnaaway2vnyen5
     authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
     objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
     viewPolicy: public
     editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
    commentPHID: NULL
 commentVersion: 0
transactionType: core:edge
       oldValue: []
       newValue: ["PHID-PROJ-tbowhnwinujwhb346q36","PHID-PROJ-izrto7uflimduo6uw2tp"]
  contentSource: {"source":"web","params":[]}
       metadata: {"edge:type":41}
    dateCreated: 1450197571
   dateModified: 1450197571
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13051

Differential Revision: https://secure.phabricator.com/D18948
2018-01-29 11:34:40 -08:00
epriestley
e5639a8ed9 Write edge transactions in a more compact way
Summary: Depends on D18946. Ref T13051. Begins writing edge transactions as just a list of changed PHIDs.

Test Plan: Added, edited, and removed projects. Reviewed transaction record and database. Saw no user-facing changes but a far more compact database representation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13051

Differential Revision: https://secure.phabricator.com/D18947
2018-01-29 11:33:58 -08:00
epriestley
de7f836f03 Wrap edge transaction readers in a translation layer
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.

The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.

Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13051

Differential Revision: https://secure.phabricator.com/D18946
2018-01-29 11:33:41 -08:00
epriestley
162563d40b Move the fix for Git 2.16.0 from the "Mercurial" part of the code to the "Git" part of the code
Summary: Ref T13050. Oh boy. Both of them run `grep`!

Test Plan: Will push again.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13050

Differential Revision: https://secure.phabricator.com/D18945
2018-01-26 13:48:35 -08:00
epriestley
a80d1e7e7d Pass "." to git grep to satisfy "all paths" for Git 2.16.0
Summary:
Ref T13050. See <https://discourse.phabricator-community.org/t/issues-with-git-2-16-0/1004/2>.

`secure` picked up 2.16.0 so this reproduces now: <https://secure.phabricator.com/source/phabricator/browse/master/?grep=dog>

Test Plan: Will push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13050

Differential Revision: https://secure.phabricator.com/D18944
2018-01-26 13:38:18 -08:00
epriestley
4b5a78e343 Add "you can only enter one value" UI limits to Herald "set status" and "set priority" actions
Summary:
Ref T13048. Fixes T11112. I mostly fixed this before in D18887, but missed that these other actions are also affected. T11112 had a more complete list of missing limits.

(It's possible there are some others somewhere, but this is everything we know about, I think.)

Test Plan: Created rules using these actions, typed stuff into the box, was only able to enter one value.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048, T11112

Differential Revision: https://secure.phabricator.com/D18943
2018-01-26 13:22:26 -08:00
epriestley
7e7720803c Set GIT_SSH_VARIANT alongside GIT_SSH
A recent version of Git has changed some piece of behavior here and we
now get "fatal: ssh variant 'simple' does not support setting port"
when using a port. Explicitly setting GIT_SSH_VARIANT to `ssh` likely
fixes this.
2018-01-26 13:21:10 -08:00
epriestley
fd49acd033 Fix Herald repetition policy migration for NULL
When we change a nullable column to a non-nullable column, we can get a
data truncation error if any value was "NULL".

This is exceptionally unusual, but our two very oldest Herald rules have
a "NULL" policy on `secure`.
2018-01-26 13:17:15 -08:00
epriestley
9d69118664 Add a discovery format hint for date fields in SearchEngine UIs
Summary:
See PHI316. Maniphest and other applications currently have controls like `Created After: [_____]` where you just get an empty text field.

Although most formats work -- including relative formats like "3 days ago" -- and we validate inputs so you get an error if you enter something nonsensical, this still isn't very user friendly.

T8060 or some other approach is likely the long term of this control.

In the meantime, add placeholder text to suggest that `YYYY-MM-DD` or `X days ago` will work.

Test Plan: Viewed date inputs, saw placeholder text.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18942
2018-01-26 13:11:10 -08:00
epriestley
fd66ab5579 Always use the same set of transactions to generate mail and mail tags
Summary:
See PHI307. Currently, when reviews undraft, we retroactively add in older activity to the mail ("alice created this revision...").

However, we don't add that activity to the mail tags, so the relevant tags (like "revision created") are dropped forever.

Instead, use the same set of transactions for both mail body and mail tag construction.

This should be obsoleted in the relatively near future by T10448, but it's a better/more correct behavior in general and we probably can't get rid of tags completely for a while.

Test Plan:
Applied patch, created a revision with builds, saw it auto-undraft after builds finished. Used `bin/mail list-outbound` and `bin/mail show-outbound` to see the mail. Verified that it included retroactive text ("created this revision") AND retroactive tags.

Note that the tag for "A new revision is created" is `DifferentialTransaction::MAILTAG_REVIEW_REQUEST` with literal value `differential-review-request`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18941
2018-01-26 13:09:04 -08:00
epriestley
ad7755d9a9 Fix an issue with symbol lookup identifying path names in Diffusion
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.

In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.

Test Plan:
  - Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
  - Command-clicked symbols in Differential diff view to check I didn't break anything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13047

Differential Revision: https://secure.phabricator.com/D18940
2018-01-26 13:02:20 -08:00
epriestley
fdc36677ba Provide character position information to symbol queries
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.

Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13047

Differential Revision: https://secure.phabricator.com/D18939
2018-01-26 13:01:57 -08:00
epriestley
3a2d337679 Add a "Revision status" field to Herald for Differential revisions
Summary: See PHI280. We have a similar field for tasks already, this is generally a reasonable sort of thing to support, and the addition of "draft" states means there are some pretty reasonable use cases.

Test Plan:
  - Wrote a status-based ("status is needs revision") Herald rule.
  - Tested it against a "Needs Revision" revision (passed) and a "Changes Planned" revision (failed).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18938
2018-01-26 13:01:36 -08:00
epriestley
d606eb1c38 When available, pass path, line and repository hints to external symbol queries
Summary:
Depends on D18936. Ref T13047. Third parties can define external symbol sources that let users jump to PHP or Python documentation or query some server.

Give these queries more information so they can try to get better results: the path and line where the symbol appeared, and any known repository scope.

Test Plan: Wrote a fake external source that used this data, command-clicked a symbol in Differential, saw a fake external symbol result.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13047

Differential Revision: https://secure.phabricator.com/D18937
2018-01-26 12:00:44 -08:00
epriestley
c37b6c6633 When users click a symbol in Differential to jump to the definition, include path/line context
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.

Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.

We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).

Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13047

Differential Revision: https://secure.phabricator.com/D18936
2018-01-26 11:59:48 -08:00
epriestley
0ec83132a8 Support basic export of user accounts
Summary:
Depends on D18934. Ref T13046. Add support for the new export flow to a second application.

My goal here is mostly just to make sure that this is general enough to work in more than one place, and exporting user accounts seems plausible as a useful feature, although we do see occasional requests for this feature exactly (like <https://discourse.phabricator-community.org/t/users-export-to-csv/968>).

The exported data may not truly be useful for much (no disabled/admin/verified/MFA flags, no external account data, no email addresses for policy reasons) but we can expand it as use cases arise.

Test Plan: Exported user accounts in several formats.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

Differential Revision: https://secure.phabricator.com/D18935
2018-01-26 11:17:44 -08:00
epriestley
a79bb55f3f Support CSV, JSON, and tab-separated text as export formats
Summary: Depends on D18919. Ref T13046. Adds some simple modular exporters.

Test Plan: Exported pull logs in each format.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

Differential Revision: https://secure.phabricator.com/D18934
2018-01-26 11:16:52 -08:00
epriestley
c0b8e4784b Add a basic, general-purpose export workflow for all objects with SearchEngine support
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.

Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.

In the future, this can replace the existing "Export to Excel" feature in Maniphest.

For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.

For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).

Test Plan: Downloaded pull logs in JSON format.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046, T5954

Differential Revision: https://secure.phabricator.com/D18919
2018-01-26 11:15:59 -08:00
epriestley
5058cfb972 Pass a real viewer to HeraldAdapter when doing test console runs
Summary:
Depends on D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.

However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.

Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.

Test Plan:
Wrote and ran a commit content rule locally, no issues.

This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18933
2018-01-26 11:08:12 -08:00
epriestley
a9f87857af Mark the "Reviewer" field for Commits as deprecated
Summary:
Depends on D18931. Ref T13048. Ref T13041. This field means "the first accepting reviewer, where order is mostly arbitrary". Modern rules should almost certainly use "Accepting Reviewers" instead.

Getting rid of this completely is a pain, but we can at least reduce confusion by marking it as not-the-new-hotness. Add a "Deprecated" group, move it there, and mark it for exile.

Test Plan:
Edited a commit rule, saw it in "Deprecated" group at the bottom of the list:

{F5395001}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048, T13041

Differential Revision: https://secure.phabricator.com/D18932
2018-01-26 11:07:02 -08:00
epriestley
5c762d8957 Document the new "only if this didn't match last time" Herald action setting
Summary: Depends on D18930. Ref T13048. Try to explain what this does and give an example since I think it's probably not very obvious from the name.

Test Plan: Read the text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18931
2018-01-26 11:06:43 -08:00
epriestley
a34b6bdd06 Implement an "only if the rule did not match last time" policy for Herald rules
Summary: Depends on D18927. Ref T13048. This implements a new policy which allows Herald rules to fire on some kinds of state changes.

Test Plan:
Wrote and tested rules with the new policy:

{F5394971}

{F5394972}

Also wrote and tested rules with the old policies:

{F5394973}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18930
2018-01-26 11:06:13 -08:00
epriestley
204d1de683 Convert storage for Herald repetition policy to "text32"
Summary:
Depends on D18926. Ref T6203. Ref T13048. Herald rule repetition policies are stored as integers but treated as strings in most contexts.

After D18926, the integer stuff is almost totally hidden inside `HeraldRule` and getting rid of it completely isn't too tricky.

Do so now.

Test Plan:
  - Created "only the first time" and "every time" rules. Did a SELECT on their rows in the database.
  - Ran migrations, got a clean bill of health from `storage adjust`.
  - Did another SELECT on the rows, saw a faithful conversion to strings "every" and "first".
  - Edited and reviewed rules, swapping them between "every" and "first".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048, T6203

Differential Revision: https://secure.phabricator.com/D18927
2018-01-26 11:05:37 -08:00
epriestley
1dd1237c92 Remove "HeraldRepetitionPolicyConfig" and hide storage details inside HeraldRule
Summary:
Depends on D18925. Ref T13048. Currently, HeraldRule stores policies as integers (0 or 1) in the database.

The application tries to mostly use strings ("first", "every"), but doesn't do a good job of hiding the fact that the values are integers deeper in the stack. So we end up with a lot of code like this:

```lang=php
$stored_int_value = $rule->getRepetitionPolicy();
$equivalent_string = HeraldRepetitionPolicyConfig::getString($stored_int_value);
$is_first = ($equivalent_string === HeraldRepetitionPolicyConfig::FIRST);
```

This happens in several places and is generally awful. Replace it with:

```lang=php
$is_first = $rule->isRepeatFirst();
```

To do this, merge `HeraldRepetitionPolicyConfig` into `HeraldRule` and hide all the mess inside the methods.

(This may let us just get rid of the integers in a later change, although I'm not sure I want to commit to that.)

Test Plan:
  - Grepped for `HeraldRepetitionPolicyConfig`, no more hits.
  - Grepped for `setRepetitionPolicy(...)` and `getRepetitionPolicy(...)`. There are no remaining callers outside of `HeraldRule`.
  - Browed and edited several rules. I'll vet this more convincingly after adding the new repetition rule.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18926
2018-01-26 11:03:29 -08:00
epriestley
a90b16e83a Define available Herald rule repetition options in terms of "isSingleEventAdapter()"
Summary:
Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.

Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):

> You can pick "only the first time" if this adapter fires more than once on the same object.

Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.

This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.

This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.

Test Plan:
  - Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
  - Checked mocks and wiki docs, saw that you can now write "only the first time" rules.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18925
2018-01-26 11:02:35 -08:00
epriestley
042c43d6d8 Remove a very old Herald garbage collection migration
Summary:
Ref T13048. This migration is from January 2012 and probably only impacted Facebook.

It references `HeraldRepetitionPolicyConfig`, which I'd like to change significantly. I initially just replaced the constant with a literal `0`, but I don't think there's any actual value in retaining this migration nowadays.

The cost of removing this migration is: if you installed Phabricator before January 2012 and haven't upgraded since then, you'll have a few more rows in the `APPLIED` table than necessary. Herald will still work correctly.

Test Plan: Reading.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18924
2018-01-26 10:54:37 -08:00
epriestley
5529458e14 Add test coverage for SSH key revocation
Summary: Depends on D18928. Ref T13043. Add some automated test coverage for SSH revocation rules.

Test Plan: Ran tests, got a clean bill of health.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18929
2018-01-25 19:47:20 -08:00
epriestley
deb754dfe1 Make SSH key revocation actually prevent adding the same key back
Summary:
Ref T13043. In an earlier change I updated this langauge from "Deactivate" to "Revoke", but the behavior doesn't quite match.

This table has a unique key on `<isActive, keyBody>`, which enforces the rule that "a key can only be active for one unique user".

However, we set `isActive` to `null` when we revoke a key, and multiple rows are allowed to have the value `<null, "asdf">` (since a `null` column in a unique key basically means "don't enforce this unique key").

This is intentional, to support this workflow:

  - You add key X to bot A.
  - Whoops, wrong account.
  - You revoke key X from bot A.
  - You add key X to bot B.

This isn't necessarily a great workflow -- ideally, you'd throw key X away and go generate a new key after you realize you made a mistake -- but it's the sort of practical workflow that users are likely to expect and want to see work ("I don't want to generate a new key, it's already being used by 5 other services and cycling it is a ton of work and this is just a test install for my dog anyway."), and there's no technical reason we can't support it.

To prevent users from adding keys on the revocation list back to their account, just check explicitly.

(This is probably better in general anyway, because "cert-authority" support from PHI269 may mean that two keys are "equivalent" even if their text differs, and we may not be able to rely on a database test anyway.)

Test Plan:
  - Added the key `ssh-rsa asdf` to my account.
  - Revoked it.
  - Tried to add it again.
    - Before patch: worked.
    - After patch: error, "this key has been revoked".
  - Added it to a different account (the "I put it on the wrong bot" workflow).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18928
2018-01-25 19:43:37 -08:00
epriestley
1059fae6c9 Set an explicit default value for the bulk action control
Summary:
Ref T13025. See <https://discourse.phabricator-community.org/t/bulk-edit-no-actions-available/1011/1>.

I'm not sure if this is what the user is seeing, but in Chrome, the `<select />` does not automatically get set to the first valid value like it does in Safari.

Set it to the first valid value explicitly.

Test Plan: In Chrome, bulk editor previously hit a JS error when trying to read a bad action off the `<select />`. After patch, bulk edits go through cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18923
2018-01-25 10:42:42 -08:00
epriestley
d28ddc21a5 Don't error when trying to mirror or observe an empty repository
Summary:
Fixes T5965.

Fixes two issues:

  - Observing an empty repository could write a warning to the log.
  - Mirroring an empty repository to a remote could fail.

For observing:

If newly-created with `git init --bare`, `git ls-remote` will
return the empty string.  Properly return an empty set of refs, rather
than attempting to parse the single "line" that is produced by
splitting that on newlines:

```
[2018-01-23 18:47:00] ERROR 8: Undefined offset: 1 at [/phab_path/phabricator/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:405]
arcanist(head=master, ref.master=5634f8410176), phabricator(head=master, ref.master=12551a1055ce), phutil(head=master, ref.master=4755785517cf)
  #0 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
  #1 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
  #2 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
  #3 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
  ...
```

For mirroring:

`git` treats `git push --mirror` specially when a repository is empty. Detect this case by seeing if `git for-each-ref --count 1` does anything. If the repository is empty, just bail.

Test Plan:
  - Observed an empty and non-empty repository.
  - Mirrored an empty and non-empty repository.

Reviewers: alexmv, amckinley

Reviewed By: alexmv

Subscribers: Korvin, epriestley

Maniphest Tasks: T5965

Differential Revision: https://secure.phabricator.com/D18920
2018-01-24 15:50:30 -08:00