1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-25 15:00:58 +01:00
Commit graph

15347 commits

Author SHA1 Message Date
epriestley
a529efa5b8 Fix an issue where inline comments with only edit suggestions are considered empty
Summary:
Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly.

Update the shared transaction code to be aware that application comments may have more complex emptiness rules.

Test Plan:
  - Posted an inline with only an edit suggestion, comment went through.
  - Tried to post a normal empty comment, got an appropriate warning.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21287
2020-05-23 08:24:57 -07:00
epriestley
959a835b95 When executing a repository passthru command via CommandEngine, don't set a timeout
Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.

Phabricator never reaches this code normally, but this code is reachable during debugging if you try to foreground a slow fetch to inspect it.

Passthru commands generally only make sense to run interactively, and the caller or control script can enforce their own timeouts (usually by pressing "^C" with their fingers).

Test Plan: Used a debugging script to run ref-by-ref fetches in the foreground.

Maniphest Tasks: T13541

Differential Revision: https://secure.phabricator.com/D21284
2020-05-22 11:54:36 -07:00
epriestley
4fd0628fae Fix two rendering issues with Jupyter notebooks
Summary:
See PHI1752.

  - Early exit of document layout can cause us to fail to populate available rows.
  - Some Jupyter documents have "markdown" cells with plain strings, apparently.

Test Plan: Successfully rendered example diff from PHI1752.

Differential Revision: https://secure.phabricator.com/D21285
2020-05-22 11:53:55 -07:00
epriestley
66566f878d Make "Open in Editor" use the simple line number of the current selected block
Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

Differential Revision: https://secure.phabricator.com/D21282
2020-05-21 15:31:16 -07:00
epriestley
d3d41324be Drop old "differential_commit" table
Summary: Ref T13276. Ref T13513. All readers and writers were removed more than a year ago; clean up the last remnants of this table.

Test Plan: Grepped for table references, found none.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513, T13276

Differential Revision: https://secure.phabricator.com/D21281
2020-05-20 14:30:39 -07:00
epriestley
6d0dbeb77f Use the changeset parse cache to cache suggestion changesets
Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.

Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.

Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21280
2020-05-20 14:29:27 -07:00
epriestley
5d0ae283a9 Put a readthrough cache in front of inline context construction
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279
2020-05-20 14:28:37 -07:00
epriestley
d2d7e7f5ff Clean up Diffusion behaviors for inline edit suggestions
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278
2020-05-20 14:28:12 -07:00
epriestley
10f241352d Render inline comment suggestions as real diffs
Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.

Test Plan: {F7495053}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21277
2020-05-20 14:27:40 -07:00
epriestley
846562158a Roughly support inline comment suggestions
Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.

Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.

Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21276
2020-05-20 14:26:37 -07:00
epriestley
00430fdbe1 Make server components of inline comment content handling state-oriented
Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21275
2020-05-20 14:25:59 -07:00
epriestley
4b2a447003 Allow "has draft inlines?" queries to overheat
Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.

For now, just let them overheat. This may create a false negative (incorrect "no draft" signal when the real condition is "drafts, but 10 most recent comments were deleted"). This could be sorted out later with a query mode like "executeAny()", perhaps.

Test Plan:
  - Created and deleted 10 inlines.
  - Submitted comments.
  - Before: overheating fatal during draft flag generation.
  - After: clean submission.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21274
2020-05-20 14:25:34 -07:00
epriestley
87bc30526b Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.

This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.

Test Plan: Created, edited, submitted, cancelled, etc., comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21273
2020-05-20 14:24:11 -07:00
epriestley
4257b26abc Treat PHP7 "Throwable" exceptions like other unhandled "Exception" cases in the worker queue
Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize.

Test Plan:
  - Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270).
  - Ran `bin/worker execute --id ...`.
  - Before: worker failed, but did not increment failure count.
  - After: worker fails and increments failure count as it would for other types of unknown error.

Differential Revision: https://secure.phabricator.com/D21271
2020-05-19 10:41:28 -07:00
epriestley
43a8d8763d Update out-of-date API calls when rendering diffs inline in email
Summary: See PHI1745. This callsite for "ChangesetParser" was not properly updated for recent changes.

Test Plan:
  - Set `metamta.differential.inline-patches` to 100.
  - Created a new revision with a small (<100 line) diff, with at least one reviewer.
  - Ran `bin/phd debug` and observed outbound mail queue with `bin/mail list-outbound`.
  - Before: fatal when trying to generate the inline changes for mail.
  - After: clean mail generation.

Differential Revision: https://secure.phabricator.com/D21270
2020-05-19 10:39:58 -07:00
epriestley
86d6abe9db Fix an issue where builds with no initiator failed to render in build plans
Summary: See PHI1743. If a build has no initiator PHID, the rendering pathway incorrectly tries to access a handle for it anyway.

Test Plan:
  - Set a build to have no initiator PHID.
  - Viewed the build plan for the build.
  - Before: fatal when trying to access the `null` handle.
  - After: clean build plan rendering.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21269
2020-05-19 09:46:18 -07:00
epriestley
6cf017d680 Fix an unusual issue with intradiff highlighting of files with uncommon end-of-file modifications
Summary:
Fixes T13539. See that task for discussion and a reproduction case.

This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs).

Instead, don't count "\" as a display line.

Test Plan:
  - See T13539 and PHI1740.
  - Before: got fatals on the "wild" diff and the synthetic simplified version.
  - After: clean intradiff rendering in both cases.

Maniphest Tasks: T13539

Differential Revision: https://secure.phabricator.com/D21267
2020-05-19 09:06:19 -07:00
epriestley
0c51885cf7 Survive importing Git commits with no commit message and/or no author
Summary:
Ref T13538. See PHI1739. Synthetic Git commits with no author and/or no commit message currently extract `null` and then fail to parse.

Ideally, we would carefully distinguish between `null` and empty string. In practice, that requires significant schema changes (these columns are non-nullable and have indexing requirements) and these cases are degenerate. These commits are challenging to build and can not normally be constructed with `git commit`.

At least for now, merge the `null` cases into the empty string cases so we can survive import.

Test Plan:
  - Constructed a commit with no author and no commit message using the approach described in T13538; pushed and parsed it.
  - Before: fatals during identity selection and storing the commit message (both roughly NULL inserts into non-null columns).
  - After: clean import.

This produces a less-than-ideal UI in Diffusion, but it doesn't break anything:

{F7492094}

Maniphest Tasks: T13538

Differential Revision: https://secure.phabricator.com/D21266
2020-05-18 20:02:21 -07:00
epriestley
f86d822a37 Update MySQL schema inspection code for deprecation of integer display widths
Summary:
Fixes T13536. See that task for discussion.

Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent.

Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions.

Maniphest Tasks: T13536

Differential Revision: https://secure.phabricator.com/D21265
2020-05-18 12:10:31 -07:00
epriestley
7b0db3eb54 Fix an email address validation UI feedback issue when creating new users
Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message.

Test Plan:
  - Before: Tried to create a new user with address "asdf". Got no specific guidance.
  - After: Got specific guidance about email address formatting and length.

Differential Revision: https://secure.phabricator.com/D21264
2020-05-18 07:35:02 -07:00
epriestley
b1351d0fdb Remove code which overrides "diffusion.ssh-username" when instanced
Summary:
Ref T13529. Now that instances can be renamed, an instance may have multiple valid SSH usernames and the preferred SSH username may not be the intenal instance name.

`PhacilitySiteSource` should already always set `diffusion.ssh-username` correctly, to the current preferred SSH username (which may be "new-name" after a rename from "old-name"), so we should never be able to reach this code without an accurate `diffusion.ssh-username` value available.

The code to resolve names into instances also already works for both "ssh old-name@..." and "ssh new-name@...".

So I believe this code has no beneficial effects and only causes harm: it may force us to return "old-name" when falling through would correctly return "new-name".

Test Plan:
  - Previously: renamed an instance, then SSH'd to it using both the old and new names. Both work.
  - Previously: verified that `diffusion.ssh-username` is set correctly after a rename.
  - Verified that Diffusion "Clone" UI now shows "new-name" after an instance rename.
  - The real question here is: does this break something I'm not thinking of? And the change probably has to go to production to answer that.

Maniphest Tasks: T13529

Differential Revision: https://secure.phabricator.com/D21259
2020-05-15 07:45:06 -07:00
epriestley
3ee6b5393c Improve offset/range inline behavior for rich diffs and unified diffs
Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.

However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.

Also, adjust unified diff behavior to work correctly with highlighting and range selection.

Test Plan:
  - Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
  - Used range selection in a unified diff, got equivalent behavior to 2-up diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21257
2020-05-14 16:02:32 -07:00
epriestley
fbd57ad832 Give selected inline comments are more obvious selected state
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.

Also fix a couple of minor issues with select interactions and offset comments.

Test Plan: Selected inlines, saw obvious visual cues.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21256
2020-05-14 14:35:55 -07:00
epriestley
2f5398796e Store inline comment offset information and show it when highlighting comments
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.

When hovering the comment, highlight the original range.

Test Plan: {F7480926, size=full}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21250
2020-05-13 17:21:53 -07:00
epriestley
42378ea393 Allow users to create inline comments by directly selecting text directly
Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.

Test Plan:
  - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
  - Caveats: no unified support, doesn't work across lines in Firefox.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21248
2020-05-13 17:15:18 -07:00
epriestley
c063e0e5ec Add "View Raw Remarkup" to inline comments
Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines.

Test Plan: Viewed raw remarkup on inlines.

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21246
2020-05-13 17:14:20 -07:00
epriestley
419b7ceebb Move inline comment actions into a dropdown menu
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.

Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21244
2020-05-13 17:13:18 -07:00
epriestley
3dea92081b Fix an issue where passphrase-protected private keys were stored without discarding passphrases
Summary:
Ref T13454. See <https://discourse.phabricator-community.org/t/newly-created-ssh-private-keys-with-passphrase-not-working-anymore/3883>.

After changes to distinguish between invalid and passphrase-protected keys, SSH private key management code incorrectly uses "-y ..." ("print public key") when it means "-p ..." ("modify input file, removing passphrase"). This results in the command having no effect, and Passphrase stores the raw input credential, not the stripped version.

We can't recover the keys because we don't store the passphrase, so no migration here is really possible. (We could add more code to detect this case, but it's presumably rare.)

Also, correct the behavior of the "Show Public Key" action: this is available for users who can see the credential and does not require edit permission.

Test Plan:
  - Created a new credential with a passphrase, then showed the public key.

Maniphest Tasks: T13006, T13454

Differential Revision: https://secure.phabricator.com/D21245
2020-05-13 08:14:37 -07:00
epriestley
df139f044b Render proper "Show Context" links in DocumentEngine diffs, not just bullets
Summary:
Ref T13513. Currently, viewing a Jupyter document, hidden context just gets a plain "* * *" facade with no way to expand it.

Support click-to-expand, like source changes.

Test Plan:
  - Clicked to expand various Jupyter diffs.
  - Clicked to expand normal source changes.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21243
2020-05-12 16:09:22 -07:00
epriestley
e8109e4a92 When an inline was left on a rendered DocumentEngine document, don't include an email context patch
Summary:
Ref T13513. If you leave an inline on line 20 of a Jupyter document, we currently render context around *raw* line 20, which is inevitably some unrelated piece of JSON.

Instead, drop this context. (Ideal behavior would be to render context around Jupyter block 20, but that's a whole lot of work.)

Test Plan:
  - On Jupyter changes and normal source changes, made and submitted inline comments, then viewed text and HTML mail.
  - Saw no context on Jupyter comments (instead of bad context), and unchanged behavior (useful context) on normal source changes.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21242
2020-05-12 14:34:33 -07:00
epriestley
acc1fa1655 Make "View as Document Type..." only show valid options
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.

This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.

Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21241
2020-05-12 14:25:37 -07:00
epriestley
0cca40db3b When creating an inline, save the current document engine
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.

This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.

The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.

Test Plan:
  - Created inlines and replies on normal soure code, saw no document engine annotated in the database.
  - Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
  - Swapped document engines between Jupyter and Source, etc.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21240
2020-05-12 14:25:09 -07:00
epriestley
6dc20d1e2e Fix an issue where storage inlines are fed to InlineAdjustmentEngine
Summary:
Ref T13513. If an intradiff has at least one unchanged file ("hasSameEffectAs()") or more than 100 files ("Large Change"), we hit this block and don't upcast storage inlines to runtime inlines. I missed this in testing.

Add the conversion step.

Test Plan: Viewed an intradiff with at least one unchanged file and at least one inline comment, saw correct rendering instead of fatal.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21239
2020-05-08 16:55:54 -07:00
epriestley
b804e8cffa Make "View" from inline comment previews correctly jump to "isEditing" inlines
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.

Update this behavior so it works on inlines in either "Viewing" or "Editing" states.

Test Plan:
  - Clicked "View" on a normal inline, got jumped/selected.
  - Clicked "View" on an editing inline, got jumped/selected.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21237
2020-05-08 08:52:42 -07:00
epriestley
24ba66f106 Persist "Show Changeset" and improve path text selection
Summary:
Ref T13513. Currently:

  - If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
  - It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.

Instead: persist the former event; make the actual path text not be part of the highlight hitbox.

Test Plan:
  - Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
  - Selected changeset path text without issues.
  - Clicked non-text header area to select/deselect changesets.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21236
2020-05-08 06:59:10 -07:00
epriestley
fa2d30ee36 Lift inline comment draft behaviors to "InlineController"
Summary:
Ref T13513. Currently, if you:

  - click a line to create an inline;
  - type some text;
  - wait a moment; and
  - close the page.

...you don't get an "Unsubmitted Draft" marker in the revision list.

Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.

Test Plan:
  - Took the steps described above, got a draft state marker.
  - Created, edited, submitted, etc., inlines in Diffusion and Differential.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21235
2020-05-08 06:52:29 -07:00
epriestley
94a95efa05 Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21234
2020-05-07 16:11:02 -07:00
epriestley
79107574a7 Remove "DifferentialInlineCommentQuery"
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".

Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21233
2020-05-07 16:07:55 -07:00
epriestley
983d77848b Move the "Inline List" view to "DiffInlineCommentQuery"
Summary: Ref T13513. Continue removing usage sites for the obsolete "DifferentialInlineCommentQuery".

Test Plan: Viewed the inline list in Differential, saw sensible inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21232
2020-05-07 16:07:23 -07:00
epriestley
af5b94b234 Lift most "InlineController" querying to the base class
Summary: Ref T13513. Move querying to "DiffInlineCommentQuery" classes and lift them into the base Controller.

Test Plan: In Differential and Diffusion, created, edited, and submitted inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21231
2020-05-07 16:04:51 -07:00
epriestley
949b9163d0 Replace remaining pseudo-query methods on AuditInlineComment
Summary: Ref T13513. Another step closer to the light.

Test Plan: Created, edited, deleted, replied to, and submitted inline comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21230
2020-05-07 16:04:27 -07:00
epriestley
6c59db20a3 Replace "loadDraftComments()" with a Query
Summary: Ref T13513. Take another step toward coherent query pathways for inlines.

Test Plan:
  - Created, previewed, and submitted inlines in Diffusion.
  - Got a (mostly) appropriate draft state.
  - Got proper comment peristence, preview behavior, and submission behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21229
2020-05-07 16:02:35 -07:00
epriestley
d0593a5a78 Replace "loadDraftAndPublishedComments()" with a Query
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments.

Test Plan:
  - Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user.
  - Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer).
  - Grepped for "loadDraftAndPublishedComments()".

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21228
2020-05-07 16:02:10 -07:00
epriestley
c1f1345cc0 Make InlineCommentQueries more robust/consistent
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227
2020-05-07 16:00:28 -07:00
epriestley
1656a2ff08 Allow inline comment storage objects to generate their own runtime objects
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").

Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.

Simplify an especially gross callsite in preview code.

Test Plan: Previewed inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21226
2020-05-07 15:57:49 -07:00
epriestley
397648855f Make the "attach_inlines" parameter to "differential.createcomment" a no-op
Summary: Ref T13513. See that task for some discussion. This prepares to lift "loadUnsubmittedInlineComments(...)" into shared code.

Test Plan: Grepped for callers, found none in the upstream. This is a backward compatibilty break. See T13513.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21225
2020-05-07 15:55:37 -07:00
epriestley
0067f1a521 Remove the obsolete "DiffusionInlineCommentPreviewController"
Summary: Ref T13513. This controller was obsoleted by EditEngine and appears unreachable without explicitly typing the URL.

Test Plan:
  - Grepped for the route, didn't find any hits.
  - Deleted the controller, successfully previewed comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21224
2020-05-06 10:13:28 -07:00
epriestley
a590db28b2 Fix an issue where non-ID changeset state keys were used as changeset IDs
Summary:
Ref T13519. This is a little fuzzy, but I think the workflow here is:

  - View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
  - Apply "hidden" state changes to the changeset.
  - View some other intradiff and/or diff view.
  - The code attempts to use "*" as a changset ID?

I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.

Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".

Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.

Maniphest Tasks: T13519

Differential Revision: https://secure.phabricator.com/D21223
2020-05-04 16:05:05 -07:00
epriestley
6430d6d638 Fix an intradiff error when the newer changeset does not exist
Summary: Ref T13523. If a file hasn't been touched in the newer changeset, we can currently hit an error in the interdiff.

Test Plan:
  - Touched "moo.txt" in Diff 1.
  - Reverted the changes to "moo.txt" in Diff 2.
  - Diffed 2 vs 1.
  - Before patch: fatal (call to getFilename() on null).
  - After patch: clean interdiff.

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21220
2020-05-04 15:42:34 -07:00
epriestley
07e160bde1 When cancelling an unsaved editing inline after a reload, don't cancel into an empty state
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.

Instead, provide the text when the editor opens. This fixes a cancel interaction.

Test Plan:
  - Create an inline, type text, don't save.
  - Reload page.
  - Cancel.
  - Before: cancelled into empty state.
  - After: cancelled into deleted+undo state.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21219
2020-05-04 15:20:31 -07:00
epriestley
7fa47408a3 When a user submits "isEditing" inlines and chooses to publish them, publish their current draft state as-shown
Summary: Ref T13513. When users choose to publish inlines, we want to publish the visible text, not the last "checkpointed" state.

Test Plan:
  - Created an inline ("AAA").
  - Edited it into "BBB", did not save.
  - Submitted.
  - Confirmed that I want to publish the unsaved inline.
  - Saw "BBB" publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21218
2020-05-04 15:16:18 -07:00
epriestley
fe501bd7f7 Save drafts for inline comments currently being edited
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.

This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.

Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21216
2020-05-04 13:19:42 -07:00
epriestley
27b7ba814a Don't consider empty inlines when considering whether a revision has draft comments or not
Summary: Ref T13513. When computing whether a revision has draft comments or not, ignore empty inlines.

Test Plan: Added empty inlines to a revision, no longer saw a yellow "draft" bubble in the list UI.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21215
2020-05-04 13:17:20 -07:00
epriestley
9307f57747 When rendering changesets, discard empty draft inline comments
Summary: Ref T13513. When you load a changeset, discard all empty inlines. This is likely a more desirable behavior than keeping empty editors around, even though the rest of the pipeline generally handles them fairly well now.

Test Plan:
  - Started an inline, didn't type any text or save, reloaded page.
    - Before: page restores empty editor in the same place.
    - After: we just discard this likely-pointless empty inline.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21214
2020-05-04 13:16:42 -07:00
epriestley
63bfad0ff4 Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.

Test Plan:
  - Edit a comment ("A"), type text ("AB"), cancel, edit.
    - Old behavior: edit and undo states (wrong, and undo does not function).
    - New behavior: edit state only.
  - Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
    - Old behavior: "AB" (wrong: you never submitted this text).
    - New behavior: "A".
  - Create a comment, type text, cancel.
    - Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
    - New behavior: no counter.
  - Cancel editing an empty comment with no text.
    - Old behavior: Something buggy -- undo, I think?
    - New behavior: it just vanishes (correct behavior).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 13:15:01 -07:00
epriestley
f5ef341c9e Don't publish "empty" inline comments
Summary:
Ref T13513. Currently, if you start an inline and then submit overall comments, we publish an empty inline. This is literally faithful to what you did, but almost certainly not the intent.

Instead, simply ignore empty inlines at publishing time (and ignore "done" state changes for those comments).

We could delete them outright, but if we do, they'll break if you have another window open with the empty inline (since the stored comment won't exist anymore). At least for now, leave them in place.

Test Plan: Created empty inlines, submitted comments, no longer saw them publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21211
2020-05-04 13:14:04 -07:00
epriestley
67da18e374 When users submit "editing" inlines, warn them that their inlines will be saved
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".

Test Plan:
  - Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
  - Got sensible warnings in the cases where warnings were appropriate.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21191
2020-05-04 13:13:15 -07:00
epriestley
468aabd4ef When draft inline comments are submitted, disengage the editor
Summary:
Ref T13513. If you submit top-level comments while an inline comment editor is open, kick the comment out of the editing state.

(An improvement to this behavior would be to warn the user that we're going to do this first, but this is currently less straightforward.)

Test Plan:
  - Clicked a line number to create an inline.
  - Type text, save, click edit.
  - (Optional: reload page.)
  - Save changes overall using the form at the bottom of the page.
  - Outcome: published inline is no longer in an "editing" state.

Weirdness:

  - If you click a line number (and, optionally, type text), then submit without using "Save", the server-side version of the inline has no content.
    - This gives you a no-effect warning. Instead, these inlines should probably just be marked as deleted somewhere in the pipeline.
  - This saves the last "Saved" copy of the inline. That's (probably?) desired, but somewhat destructive without a warning.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21188
2020-05-04 13:12:04 -07:00
epriestley
b2ce0844b6 When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.

This also serves to delete empty comments.

Test Plan:
  - Clicked a line number to create a new comment. Then:
    - Clicked "Cancel". Reloaded page, saw no more comment.
    - Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
    - Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21187
2020-05-04 13:11:23 -07:00
epriestley
b48a22bf50 Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.

In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.

---

Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.

On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.

Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).

To simplify this:

  - Make `PhabricatorInlineCommentInterface` an abstract base class instead.
  - Lift as much code out of the `Audit` and `Differential` subclasses as possible.
  - Delete methods which no longer have callers, or have only trivial callers.

---

Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.

Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.

These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.

The `EditView` can not fully render its content. Move the content rendering code into the view.

---

Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.

This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.

---

Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.

Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.

This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.

---

Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".

Test Plan:
  - Created comments on either side of a diff.
  - Edited a comment, reloaded, saw edit stick.
  - Saved comments, reloaded, saw save stick.
  - Edited a comment, typed text, cancelled, "unedited" to get state back.
  - Created a comment, typed text, cancelled, "unedited" to get state back.
  - Deleted a comment, "undeleted" to get state back.

Weirdness / known issues:

  - Drafts don't autosave yet.
  - Fixed in D21187:
    - When you create an empty comment then reload, you get an empty editor. This is a bit silly.
    - "Cancel" does not save state, but should, once drafts autosave.
  - Mostly fixed in D21188:
    - "Editing" comments aren't handled specially by the overall submission flow.
    - "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.

Subscribers: jmeador

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21186
2020-05-04 13:10:30 -07:00
epriestley
5ff0ae7d48 Add generic "attributes" storage to inline comment tables
Summary: Ref T13513. This plans for "currently editing", character range comments, code suggestions, document engine tracking. And absolutely nothing else.

Test Plan:
  - Ran `bin/storage upgrade -f`, got a clean upgrade.
  - Created and submitted some inline comments; nothing exploded.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21184
2020-05-04 13:09:55 -07:00
epriestley
54ec566281 Restore highlighting when jumping to transactions using URI anchors
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.

Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.

(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)

Test Plan:
  - Clicked a timeline story anchor, saw the story highlight.

Differential Revision: https://secure.phabricator.com/D21213
2020-05-04 10:04:04 -07:00
epriestley
17426a60f0 Fix an issue where text intradiff bodies may not render
Summary:
Ref T13523. In the caching layer, there's a tricky clause about filetypes that skips some body rendering behavior.

Provide file type information which at least has a better chance of representing all changes (e.g., an image file may be replaced with a text file, but this can not be represented by a single file type).

Formalize "hasSourceTextBody()", to mean the changeset parser should engage the change as source text.

Test Plan: Intradiffed text changes, saw the body render properly.

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21210
2020-05-04 07:40:47 -07:00
epriestley
dade977307 Provide a hint about how to quote search terms containing literal colons
Summary: See <https://phabricator.wikimedia.org/T243483>. Provide a more direct path forward if users hit the "unknown function" error but are trying to search for a term with a colon in it.

Test Plan:
{F7414068}

{F7414067}

Differential Revision: https://secure.phabricator.com/D21209
2020-05-03 10:14:47 -07:00
epriestley
6f09edeb91 Fix an issue where the "%%%" parser could match too many lines in unterminated blocks
Summary: Fixes T13530. The block parser could match too many lines in an unterminated "%%%" literal block. Adjust the logic to stop doing this (and hopefully be a little easier to read).

Test Plan: Added a failing test, made it pass.

Maniphest Tasks: T13530

Differential Revision: https://secure.phabricator.com/D21208
2020-05-03 09:25:41 -07:00
epriestley
b2cfcda114 Provide detailed information about reviewer changes in "transaction.search"
Summary:
See PHI1722, which requests transaction details about reviewer changes.

This adds them; they're structured to be similar to "projects" and "subscribers" transactions and the "reviewers" attachment on revisions.

Test Plan: {F7410675}

Differential Revision: https://secure.phabricator.com/D21207
2020-05-01 14:18:12 -07:00
epriestley
b89e6c0fa9 Add "idea://" to the upstream editor whitelist
Summary: This supports the IntelliJ IDEA editor.

Test Plan:
  - Looked at the editor settings panel, saw "idea://".
  - Set my editor pattern to "idea://a?b".
  - (Did not actually install IntelliJ IDEA.)

Differential Revision: https://secure.phabricator.com/D21206
2020-05-01 12:56:35 -07:00
epriestley
eab561bb87 Add "uri" to the API results for File objects
Summary: Ref T13528. This supports "arc upload --browse ...".

Test Plan: Called "file.search", saw URIs in results.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21204
2020-05-01 09:11:59 -07:00
epriestley
fbbd2e35cb Make content more prominent in Files and move some details to the curtain
Summary: Ref T13528. Now that we're hinting users into Files, put the content first and move the detail panel under it. Move the most-useful details (author, size, dimensions) into the curtain.

Test Plan: {F7409925}

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21201
2020-05-01 09:11:33 -07:00
epriestley
1edca1ee2a Put application curtain panels above extension curtain panels
Summary:
Ref T13528. The original rationale here was that it's easier to find items at the bottom of the curtain than somewhere in the middle, since they're in a more clearly predictable visual location.

This might be true in some sense, but user feedback about this has fairly consistently indicated that the layout is surprising. Try the other order. See also D20967 for some discussion.

In practice, this primarily moves "Author / Assigned" above other panel elements in Maniphest.

Test Plan: Looked at tasks, aw "Author / Assigned" above "Tags / Subscribers".

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21200
2020-05-01 09:11:04 -07:00
epriestley
d6928a3c26 When creating a File storage object for a Paste, try to give it the same name as the Paste
Summary:
Ref T13528. Paste data is stored in files, but the files are always named "raw.txt".

Now that Paste provides a hint to use Files for "DocumentEngine" rendering, try to use the same name as the paste instead.

Test Plan:
  - Created a paste named "staggering-insight.ipynb".
  - Clicked "View as Jupyter Notebook" from Paste.
  - Saw a file named "staggering-insight.ipynb", not "raw.txt".
  - Created a paste with no name, saw a file named "raw-paste-data.txt" get created.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21197
2020-05-01 09:10:31 -07:00
epriestley
3c1f393c81 When a Paste has a useful alternative rendering in Files, provide a hint
Summary: Ref T13528. When a file in Paste (like a Jupyter notebook) has a good/useful document engine, provide a link to Files.

Test Plan: {F7409881}

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21196
2020-05-01 09:09:42 -07:00
epriestley
65a2b5e219 Route hard-coded "/favicon.ico" requests to a favicon resource
Summary:
See PHI1719. User agents making hard-coded requests to "/favicon.ico" currently 404. This is a mild source of log noise, and we can reasonably route this request.

Limitations:

  - This only routes the "PlatformSite". Other sites (custom Phame blogs, third-party sites, Phurl redirectors) won't route here for now.
  - This returns a "Location:" redirect to the correct resource rather than icon data directly. This produces the right icon with the right caching behavior, and returning icon data directly is difficult in the general case. However, it won't perform/cache as well as a direct response would.

Test Plan:
  - Visted `/favicon.ico`.
  - Before: 404.
  - After: redirect to favicon.

Differential Revision: https://secure.phabricator.com/D21195
2020-05-01 05:23:00 -07:00
epriestley
304467feb2 Stabilize fatals when a build has a build plan the viewer can't see because of policy restrictions
Summary:
Ref T13526. Currently, if a build plan is restricted, viewers may fatal when trying to view related builds.

The old behavior allowed them to see the build even if they can not see the build plan. This is sort of incoherent, but try to stabilize things before fixing this.

Test Plan:
This is a muddy change.

  - Created a build with a build plan that Alice can't see.
  - As Alice, viewed the build page (restricted before, restricted after); the buildable page (fatal before, works after).
  - Also viewed a revision page (works before and after, but user-reported fatal).

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13526

Differential Revision: https://secure.phabricator.com/D21194
2020-04-30 07:57:23 -07:00
epriestley
186a12ef7f Replace nonexistent "withPHIDs()" in ChangesetQuery with "withIDs()"
Summary:
Ref T13519. See <https://discourse.phabricator-community.org/t/error-call-to-undefined-method-differentialchangesetquery-withphids/3816/>.

Changesets do not have PHIDs, and the Query has no "withPHIDs()" method. The keys in the viewstate storage are (usually) IDs.

Test Plan:
  - On a revision with Diff 1 and Diff 2 affecting the same file:
    - Viewed Diff 1.
    - Hid file A.
    - Viewed Diff 2.
  - Before patch: exception about call to "withPHIDs()", which does not exist for ChangesetQuery.
  - After patch: no exception. Also, file actually unhid, which is the correct behavior!

Maniphest Tasks: T13519

Differential Revision: https://secure.phabricator.com/D21189
2020-04-29 14:47:47 -07:00
epriestley
b648a85841 Fix an issue where the Maniphest burnup chart was trying to render a non-View object
Summary:
See PHI1714. This code is incorrectly rendering the chart panel twice, sort of, and passing a non-View object to rendering.

After D21044, this fatals by raising an exception in rendering.

Test Plan: Loaded page, no more exception.

Differential Revision: https://secure.phabricator.com/D21185
2020-04-29 05:06:03 -07:00
epriestley
f21f1d8ab9 Update the diff table of contents to use hierarchical views and edit distance renames
Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:

  - Show a hierarchy, with compression for single-sibling children.
  - Use the same icons, instead of "M D" and "(img)" stuff.
  - Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
  - Show path changes within the path list.

I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.

Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21183
2020-04-28 12:27:37 -07:00
epriestley
a7b2327c34 Fix an issue with "Auditors:" where an edge edit was used as a PHID list
Summary:
See <https://discourse.phabricator-community.org/t/runtimeexception-during-import-of-commit/3801>. When importing commits with "Auditors:", a raw transaction new value (with an edge edit map using a "+" key) may be passed as an unmentionable PHID list.

Instead, pass an actual PHID list.

Test Plan:
  - Pushed a commit with "Auditors: duck".
  - Ran daemons.
    - Before patch: umentionable PHID exception.
    - After patch: clean commit import.
  - Verified "duck" was added as an auditor.

Differential Revision: https://secure.phabricator.com/D21181
2020-04-28 05:49:03 -07:00
epriestley
befeb17f6f Improve the construction of synthetic "comparison/intradiff" changesets
Summary:
Ref T13523. Currently, when building a "comparison" changeset, metadata is taken from the left changeset. This is somewhat arbitrary.

This means that intradiffs of images don't work properly because the rendered changeset has only the left (usually "old") information.

Later, some of the code attempts to ignore the file data stored on the changeset and reconstruct the correct file data, which is how the result ends up not-completely-wrong.

Be more careful about building sensible-ish metadata, and then just use it directly later on. This fixes the "spooky" code referencing D955 + D6851.

There are some related issues, where "change type" and "file type" are selected arbitrarily and then used to determine whether the change has an "old/new" state or not (i.e., is the left side of the diff empty, since the change creates the file)?

In many cases, neither of the original changesets have a "change type" which will answer this question correctly. Separate this concept from "has state" from "change type", and make more of the code ask narrower questions about the specific conditions or states it cares about, rather than "change type".

Test Plan:
  - Created a revision with Diff 1, Diff 2, and Diff 3. Diff 1 takes an image from "null -> A". Diff 2 takes the same image from "null -> B". Diff 3 takes the same image from "A -> B'.
  - Intradiffed 1v2 and 1v3.
  - Before patch:
    - Left side usually missing, which is incorrect (should always be "A").
    - Change properties are a mess ("null -> image/png" for MIME type, e.g.)
    - Uninteresting/incorrect "unix:filemode" stuff.
  - After patch;
    - Left side shows state "A".
    - Change properties only show size changes (which is correct).

{F7402012}

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21180
2020-04-28 05:09:22 -07:00
epriestley
5eaa0f24e7 Use "@" to silence "GC list" warnings from "apc_store()" and "apcu_store()"
Summary:
Fixes T13525. Since D21044, the intermittent GC list warnings are treated more severely and can become user-visible errors.

Silence them, since this seems to be the only realistic response in most versions of APC/APCu.

Test Plan: Will deploy.

Maniphest Tasks: T13525

Differential Revision: https://secure.phabricator.com/D21179
2020-04-28 04:13:37 -07:00
epriestley
aa20faeaa5 Fix an invalid index access for synthetic lint inline comments from Harbormaster
Summary:
Ref T13524. If a Harbormaster lint message has no line number (which is permitted), we try to access an invalid index here. This is an exception after D21044.

Treat comments with no line number as unchanged. These comments do not have "ghost" behavior and do not port across diffs.

Test Plan:
  - Used "harbormaster.sendmessage" to submit lint with no line number on a changeset.
  - Viewed changeset.
    - Before patch: "Undefined index: <null>" error.
    - After patch: Clean changeset with lint message.

{F7400072}

Maniphest Tasks: T13524

Differential Revision: https://secure.phabricator.com/D21178
2020-04-27 14:20:55 -07:00
epriestley
6617005365 Make Conduit "www-form-urlencoded" parsing of "true" and "false" case-insensitive
Summary:
See PHI1710. Python encodes `True` as `True` (with an uppercase "T") when building URLs.

We currently do not accept this as a "truthy" value, but it's reasonable and unambiguous. Accept "True", "TRUE", "tRuE", etc.

Test Plan: Made a cURL conduit call with "True" and "tRuE". Before patch: failure to decoded booleans; after patch: successful interpretation of "true" variations.

Differential Revision: https://secure.phabricator.com/D21177
2020-04-27 13:28:47 -07:00
epriestley
b5bed7b0fa Make omitting "value" from a transaction description an explicit error
Summary: See PHI1710. Until D21044, some transactions could omit "value" and apply correctly. This now throws an exception when accessing `$xaction['value']`. All transactions are expected to have a "value" key, so require it explicitly rather than implicitly.

Test Plan: Submitted a transaction with a "type" but no "value". After D21044, got a language-level exception. After this change, got an explicit exception.

Differential Revision: https://secure.phabricator.com/D21176
2020-04-27 13:16:00 -07:00
epriestley
604811bfc5 Fix a Diffusion issue where commits that do not show changesets would incorrectly try to render changesets
Summary:
See <https://discourse.phabricator-community.org/t/loading-certain-svn-commits-cause-unhandled-exception/3795/>.

Commits with no changesets (for example, deleted commits) don't generate a "$changesets".

Test Plan: Viewed a commit with no changesets. Before change: exception. After change: saw unusual commit state.

Differential Revision: https://secure.phabricator.com/D21175
2020-04-27 10:36:09 -07:00
epriestley
c12db28251 For changesets that affect binaries, use the new binary file content hash as an effect hash
Summary: Ref T13522. When changesets update an image, we currently compute no effect hash. A content hash of the image (or other binary file) is a reasonable effect hash, and enalbes effect-hash-based behavior, including hiding files in intradiffs.

Test Plan:
  - Created a revision affecting `cat.png` and `quack.txt` (currently, there must be 2+ changesets to trigger the hide logic).
  - Updated it with the exact same changes.
  - Viewed revision:
    - Saw the image renderered in the interdiff.
  - Applied patch.
  - Ran `bin/differential rebuild-changesets ...`.
  - Viewed revision:
    - Saw both changesets collapse as unchanged.

Maniphest Tasks: T13522

Differential Revision: https://secure.phabricator.com/D21174
2020-04-27 08:34:55 -07:00
epriestley
b0c295e545 Fix some PHP 7.4 array index access issues
Summary: Ref T13518. See <https://discourse.phabricator-community.org/t/more-exceptions-when-viewing-diffs/3789/>. Under PHP 7.4, accessing an array index of values like `false` and `null` is no longer valid. This is great, but we occasionally do it.

Test Plan:
  - Upgraded to PHP 7.4.
  - Loaded revisions with added/changed lines, inlines, and Asana support configured.
  - Before patch: saw various fatals around accessing indexes of booleans and nulls.
  - After patch: clean revision.

Maniphest Tasks: T13518

Differential Revision: https://secure.phabricator.com/D21172
2020-04-26 08:35:06 -07:00
epriestley
7355bb7f29 Skip "null" lines when constructing raw documents for DocumentEngine rendering
Summary: See <https://discourse.phabricator-community.org/t/more-exceptions-when-viewing-diffs/3789>. This is a similar issue with the same datastructure.

Test Plan: Works correctly under PHP 7.3, but this may not be the end of things.

Differential Revision: https://secure.phabricator.com/D21171
2020-04-25 17:34:45 -07:00
epriestley
a226d74133 Use "rest/api/3/myself" to retrieve JIRA profile details, not "rest/auth/1/session"
Summary:
Ref T13493. At time of writing, the old API method no longer functions: `1/session` does not return an `accountId` but all calls now require one.

Use the modern `3/myself` API instead. The datastructure returned by `2/user` (older appraoch) and `3/myself` (newer approach) is more or less the same, as far as I can tell.

Test Plan: Linked an account against modern-at-time-of-writing Atlassian-hosted JIRA.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21170
2020-04-25 14:05:22 -07:00
epriestley
40d2346f29 Add a missing "null" check when rebuilding old/new diff content
Summary:
See <https://discourse.phabricator-community.org/t/exceptions-when-viewing-diffs/3787>. This list may include `null` values.

Until PHP 7.4, `$x = null; echo $x['y'];` does not emit a warning. Sneaky!

Test Plan:
  - Traced `null` values from `reparseHunksForSpecialAttributes()`, saw them no longer incorporated into corpus bodies.
  - This has some amount of test coverage.

Differential Revision: https://secure.phabricator.com/D21169
2020-04-25 09:23:05 -07:00
epriestley
454ecb56e3 When proxying HTTP repository responses from repository nodes, discard content description headers
Summary:
Ref T13517. See that task for details about the underlying issue here.

Currently, we may decode a compressed response, then retransmit it with leftover "Content-Encoding" and "Content-Length" headers. Instead, strip these headers.

Test Plan:
  - In a clustered repository setup, cloned a Git repository over HTTP.
  - Before: Error while processing content unencoding: invalid stored block lengths
  - After: Clean clone.

Maniphest Tasks: T13517

Differential Revision: https://secure.phabricator.com/D21167
2020-04-25 07:51:46 -07:00
epriestley
6f7147376f Parse "multipart/form-data" bodies even if "enable_post_data_reading" is on
Summary:
Ref T4369. During T13507, I set my "max_post_size" to a very small value, like 7 (i.e., 7 bytes). This essentially disables "enable_post_data_reading" even if the setting is technically on.

This breaks forms which use "multipart/form-data", which are rare but not nonexistent. Notably, forms in Config use this setting (because of `ui.header` stuff?) although perhaps they should not or no longer need to.

This can be fixed by parsing the raw input.

Since the only reason we don't parse the raw input is concern that we may not be able to read it (per documentation, but never actually observed), and we do a `strlen()` test anyway, just read it unconditionally.

This should fix cases where POST data wasn't read because of "max_post_size" without impacting anything else.

Test Plan: With very small "max_post_size", updated "ui.footer-items" in Config. Before: form acted as a no-op. After: form submitted.

Maniphest Tasks: T4369

Differential Revision: https://secure.phabricator.com/D21165
2020-04-24 11:25:57 -07:00
epriestley
5a460e4ea5 Stick the page footer in the right place on Formation View pages
Summary: Ref T13516. This isn't terribly clean, but get the page footer into the bottom of the content page on FormationView pages so it doesn't overlap into the side panel.

Test Plan: With and without a footer, viewed normal and FormationView pages. Saw footers in appropriate places at appropriate times.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21166
2020-04-24 11:25:31 -07:00
epriestley
d05d8f6558 Add a very forgiving GC for Differential viewstate information
Summary:
Ref T13455. Viewstates are fairly small and will probably grow less quickly than the changeset table, but the data is also not important to retain in the long term: if you revisit a change several months after hiding some files, it's fine if we've forgotten that you adjusted the view parameters.

Add a GC with a long default collection policy (180 days) so installs can manage the size of this table if it becomes necessary.

Test Plan: Ran via `bin/garbage` to adjust the GC policy and collect viewstates.

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21164
2020-04-23 14:17:48 -07:00
epriestley
4793bfcb7c Don't show the "file tree" view on tablets/phones
Summary: Ref T13516. Hide this UI on devices without the screen width to reasonably support it.

Test Plan: Viewed a revision at various window widths, saw the elements vanish at device widths and reappear at desktop widths.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21162
2020-04-23 13:40:42 -07:00
epriestley
d2572f8b33 Refine more Differential review state behaviors
Summary:
Ref T13516.

- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21161
2020-04-23 10:14:52 -07:00
epriestley
0ede616f31 Update the "View Options" menu for recent filetree changes
Summary:
Ref T13516. Minor improvements here:

  - Show key commands in the "View Options" dropdown.
  - Organize it slightly better.
  - Improve disabled item behaviors a little bit.
  - Add a "Browse Directory" action.
  - Rename "...in Diffusion" to "...in Repository".
  - Make "d", "D", and "h" use the same targeting rules as "\".
  - When you hide a file with the "h" menu item, select it.

Test Plan: Poked at the menu a lot, ran into less questionable behavior.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21160
2020-04-23 08:23:12 -07:00
epriestley
befbea2f00 Don't pass "No newline at end of file." annotations to DocumentEngines as literal diff text
Summary: See PHI1707, which has a Jupyter notebook which fails to diff nicely when modified. The root cause seems to be that the document does not end in a newline.

Test Plan: Applied patch, diffed the file, got a Jupyter diff out of it.

Differential Revision: https://secure.phabricator.com/D21159
2020-04-22 20:22:33 -07:00
epriestley
60de1506fe Make "hidden" changesets sticky, and show hidden state in the filetree
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.

We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.

Test Plan: {F7375468}

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21158
2020-04-22 16:12:42 -07:00
epriestley
a72a66caa8 Mark "low importance" and "owned" changes in the filetree
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.

Test Plan: {F7375327}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21157
2020-04-22 11:22:34 -07:00
epriestley
ff88eb588e Show change information in file icons in the filetree
Summary: Ref T13516. Restores "deleted"/"added" information to the tree icons.

Test Plan: {F7375145}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21156
2020-04-22 08:38:29 -07:00
epriestley
12eddb18fb Entirely replace the old filetree UI with the "flank" UI
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.

Restores the inline tips in the path tree.

Test Plan: {F7374175}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21154
2020-04-22 08:32:02 -07:00
epriestley
ba8071bbef Roughly style the new "flank" paths UI
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.

Test Plan: {F7374096}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21153
2020-04-22 08:31:40 -07:00
epriestley
8cd1f9a309 Generate file trees from changesets in the new flank UI
Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.

Test Plan: {F7373967}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21152
2020-04-22 08:31:17 -07:00
epriestley
646280972b Glue the new FormationView on top of the older Filetree view in Differential
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.

Test Plan: {F7373838}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21151
2020-04-22 08:29:04 -07:00
epriestley
fef2cdabfe Add a "FormationView" to support dynamic flank panels
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.

This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.

Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.

Test Plan: No callers and no user-visible impact.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21150
2020-04-22 08:23:21 -07:00
epriestley
ef69c7969f Restore editor behavior to Diffusion and support "\" shortcut
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.

The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".

Test Plan:
  - Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
  - Clicked a line, hit "\", got the file opened to that line.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21149
2020-04-19 09:41:37 -07:00
epriestley
f02024615a Add "short name", "id", and "phid" variables for external editor URIs
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.

Test Plan: Changed settings to use new variables, saw links generate appropriately.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21147
2020-04-19 09:37:53 -07:00
epriestley
c79094d7a8 Add static errors, supported protocols, and a dynamic function listing to external editor settings page
Summary:
Ref T13515.

  - Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
  - Generate a list of functions from an authority in the parser.
  - Generate a list of protocols from configuration.

Test Plan: {F7370872}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21146
2020-04-19 09:37:31 -07:00
epriestley
7a79131bf2 Replace old hard-coded URI-based "changes saved" jank with new overgeneralized cookie-based "changes saved" jank
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.

This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.

Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:

  - Set a cookie on the redirect which identifies the form we just saved.
  - On page startup: if this cookie exists, save the value and clear it.
  - If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.

This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?

Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21144
2020-04-19 09:04:31 -07:00
epriestley
3984c14260 Tokenize external editor links so they can be safely materialized on the client
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.

The parser also can't identify invalid variables.

Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.

(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)

Test Plan:
  - Added a bunch of tests for the actual parser.
  - Used "Open in Editor" in Differential.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21143
2020-04-19 09:02:49 -07:00
epriestley
4168335619 Remove the "Edit Multiple Files" external editor setting
Summary: Ref T13515. No callsites actually use this, most editors don't support it, it doesn't seem terribly useful for the ones that do, it makes template-based APIs for line-number substitution complicated, and we can probably just loop on `window.open()` anyway.

Test Plan: Grepped for affected symbols, found no more references. Loaded settings page, saw no more setting.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21142
2020-04-19 09:02:03 -07:00
epriestley
8bdc713352 Make the "Keyboard Shortcuts" dialog in Differential less hideous
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.

Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21141
2020-04-19 09:01:07 -07:00
epriestley
84cd4a3854 Move "External Editor" settings to a separate settings group
Summary:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.

Give them a separate group.

Test Plan: {F7370500}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21140
2020-04-19 08:59:43 -07:00
epriestley
c3c55d82ae Make "renderer", "engine", and "encoding" sticky across reloads in Differential and Diffusion
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.

Some complexity here arises from these concerns:

  - In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
  - In all other cases, we render the changeset with AJAX.

So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.

Then, some bookkeeping issues:

  - At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
  - Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".

Test Plan:
  - Viewed changes in Differential, Diffusion, and in standalone mode.
  - Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
  - Started typing a comment, cancelled it, hit the undo UI.

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21138
2020-04-19 08:59:09 -07:00
epriestley
8aac55cc57 Make "Highlight As..." sticky across reloads in Diffusion and Differential
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.

The storage generates a "PhabricatorChangesetViewState" configuration object as an output.

When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.

The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.

Test Plan:
  - Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
  - Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
  - Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.

Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21137
2020-04-19 08:58:39 -07:00
epriestley
3adf082002 When inlines would disable a file shield in a diff, still apply the shield if all the comments are collapsed
Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.

These files don't get shielded if they have inline comments.

But, if the viewer has collapsed all the comments, we can shield the file again.

Test Plan:
  - Created a change affecting files A and B, with three diffs:
    - Touch A and B.
    - Touch B only.
    - Touch nothing.
  - Added an inline to A and collapsed it.
  - Viewed Diff 1 vs Diff 2:
    - Saw A collapse with a note about inlines.
    - Saw B changes, normally.
  - Viewed Diff 2 vs Diff 3:
    - Saw A collapse with a note about inlines.
    - Saw B collapse normally.
  - Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21136
2020-04-17 10:52:40 -07:00
epriestley
3d966d8a41 Add an "Open in External Editor" keystroke to Differential
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.

Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21135
2020-04-17 10:06:46 -07:00
epriestley
83eb7447a1 When Owners packages are archived, annotate them in tokenizer results
Summary: Fixes T13512. Archived packages in Owners are missing hinting, but should have it.

Test Plan:
Before:

{F7369122}

After:

{F7369128}

Maniphest Tasks: T13512

Differential Revision: https://secure.phabricator.com/D21134
2020-04-17 06:18:04 -07:00
Austin McKinley
ef1340bd32 Add Ferret support to Paste
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?

Also updates table names in `PhabricatorPasteQuery`.

Test Plan: Created some pastes, indexed them, searched for them.

Reviewers: amckinley

Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20650
2020-04-16 14:10:23 -07:00
epriestley
2748f83e12 Modularize Ferret fulltext functions
Summary: Ref T13511. Currently, Ferret fulltext field functions (like "title:") are hard-coded. Modularize them so extensions may define new ones.

Test Plan: Added a new custom field which emits data for the indexer, searched for "animal-noises:moo", "animal-noises:-", etc., in global search and application search.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21131
2020-04-16 13:41:13 -07:00
epriestley
894d9b6587 Remove Ferret function aliases and overrides
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.

This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.

Regardless, these don't actually work. Searching for "description:x" is a syntax error.

Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.

Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21130
2020-04-16 13:40:17 -07:00
epriestley
9bdf477f2f Combine the two different ngram-splitting algorithms into a single engine
Summary:
Ref T13501. Depends on D21127. With the "prefix" behavior removed in D21127, we now have two virtually identical copies of the same code.

The newer one in Ferret is better: it slices utf8 correctly and is slightly more efficient on large inputs. Pull it out and make all callers call into it.

Test Plan:
  - Grepped for all affected symbols.
  - Ran `bin/search index --force ...` to reindex various objects (tasks, files).
  - Searched for things in the UI.

Maniphest Tasks: T13501

Differential Revision: https://secure.phabricator.com/D21128
2020-04-16 09:45:00 -07:00
epriestley
fb3f423279 Remove broken and unfixable "prefix" ngram behavior
Summary:
Ref T13501. The older ngram code has some "prefix" behavior that tries to handle cases where a user issues a very short (one or two character) query.

This code doesn't work, presumably never worked, and can not be made to work (or, at least, I don't see a way, and am fairly sure one does not exist).

If the user searches for "xy", we can find trigrams in the form "xy*" using the index, but not in the form "*xy". The code makes a misguided effort to look for " xy", but this will only find "xy" in words that begin with "xy", like "xylophone".

For example, searching Files for "om" does not currently find "random.txt".

Remove this behavior. Without engaging the trigram index, these queries fall back to an unidexed "LIKE" table scan, but that's about the best we can do.

Test Plan: Searched for "om", hit "random.txt".

Maniphest Tasks: T13501

Differential Revision: https://secure.phabricator.com/D21127
2020-04-16 09:44:37 -07:00
epriestley
b1b9c844ac Remove unused "getAllFunctionFields()" from Ferret
Summary: Ref T13511. This function does nothing interesting and has no callers.

Test Plan: Grepped for callers.

Maniphest Tasks: T13511

Differential Revision: https://secure.phabricator.com/D21126
2020-04-16 09:43:25 -07:00
epriestley
3573170dfa Compress file downloads if the client sends "Accept-Encoding: gzip" and we guess the file might compress alright
Summary:
Ref T13507. We currently compress normal responses, but do not compress file data responses because most files we serve are images and already compressed.

However, there are some cases where large files may be highly compressible (e.g., huge XML files stored in LFS) and we can benefit from compressing responses.

Make a reasonable guess about whether compression is beneficial and enable compression if we guess it is.

Test Plan:
  - Used `curl ...` to download an image with `Accept-Encoding: gzip`. Got raw image data in the response (as expected, because we don't expect images to be worthwhile to recompress).
  - Used `curl ...` to download a text file with `Accept-Encoding: gzip`. Got a compressed response. Decompressed the response into the original file.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21125
2020-04-15 11:53:35 -07:00
epriestley
45665dd3b4 Hide "notification.servers" configuration and don't follow redirects from Aphlict
Summary:
See <https://hackerone.com/reports/850114>.

An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.

Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.

There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.

  - Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
  - Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.

Test Plan:
  - Viewed configuration in web UI.
  - Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.

{F7365973}

Differential Revision: https://secure.phabricator.com/D21123
2020-04-15 07:00:51 -07:00
epriestley
b52fa96238 Disable automatic decoding of "Content-Encoding" responses during "Accept-Encoding" setup test
Summary:
Ref T13507. Now that we handle processing of "Content-Encoding: gzip" headers by default, this setup check can get a decompressed body back. Since it specifically wants a raw body back, disable this behavior.

Also, "@" a couple things which can get in the way if they fail now that error handling is more aggressive about throwing on warnings.

Test Plan: Ran setup check after other changes in T13507, got clean result.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21122
2020-04-15 06:28:29 -07:00
epriestley
0ea6d131e0 In Conduit responses, assert that Phabricator supports a "gzip" capability
Summary: Ref T13507. If we believe the server can accept "Content-Encoding: gzip" requests, make the claim in an "X-Conduit-Capabilities" header in responses. Clients can use request compression on subsequent requests.

Test Plan: See D21119 for the client piece.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21120
2020-04-14 16:51:03 -07:00
epriestley
6b05d2be28 Add a setup warning to detect "SetInputFilter DEFLATE" and other "Content-Encoding" request mangling
Summary: Ref T13507. See that task for discussion.

Test Plan: Faked different response behaviors and hit both variations of this error.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21116
2020-04-14 14:48:43 -07:00
epriestley
99cbc20778 Reduce the verbosity of the "Aphlict" log
Summary:
See PHI1692. Currently, the Aphlict log is ridiculously verbose. As an initial pass at improving this:

  - When starting in "debug" mode, pass "--debug=1" to Node.
  - In Node, separate logging into "log" (lower-volume, more-important messages) and "trace" (higher-volume, less-important messages).
  - Only print "trace" messages in "debug" mode.

Test Plan: Ran Aphlict in debug and non-debug modes. Behavior unchanged in debug mode, but log has more sensible verbosity in non-debug mode.

Differential Revision: https://secure.phabricator.com/D21115
2020-04-14 13:24:44 -07:00
epriestley
59c855276b Provide a "--local" flag to "bin/conduit call" to force in-process execution
Summary:
See PHI1692. Currently, it's hard to get a local profile or "--trace" of some Diffusion API methods, since they always proxy via HTTP -- even if the local node can serve the request.

This always-proxy behavior is intentional (so we always go down the same code path, to limit surprises) but inconvenient when debugging. Allow an operator to connect to a node which can serve a request and issue a `--local` call to force in-process execution.

This makes it straightforward to "--trace" or "--xprofile" the call.

Test Plan: Ran `bin/conduit call ...` with and without `--local` using a Diffusion method on a clustered repository. Without `--local`, saw proxy via HTTP. With `--local`, saw in-process execution.

Differential Revision: https://secure.phabricator.com/D21114
2020-04-14 13:24:26 -07:00
epriestley
4655a5f059 Document the "field present" and "field absent" operators in Ferret
Summary: Ref T13509. Adds documentation for the new operators.

Test Plan: Read documentation, tried examples, got sensible-seeming results.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21112
2020-04-14 11:08:34 -07:00
epriestley
b3a8754013 Make the Ferret query compiler keep functions sticky across non-initial quoted tokens
Summary: Ref T13509. In `title:big "red" dog`, keep "title" sticky across all three terms, since this seems like it's probably the best match for intent.

Test Plan: Added unit tests; ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21111
2020-04-14 11:00:20 -07:00
epriestley
0511b2a012 Implement the "present" and "absent" operators in the Ferret execution engine
Summary:
Ref T13509. Now that the compiler can parse these queries, actually implement them.

These are fairly easy to implement:

  - For present, just "JOIN". If it works, the field is present.
  - For absent, we "LEFT JOIN" and then "WHERE any_column IS NULL".

Test Plan: Searched for various documents with and without fields present, got sensible results in Maniphest. For example, "body:-" finds tasks with no body, "body:- duck" finds tasks with no body and "duck" elsewhere in the content, and so on.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21110
2020-04-14 10:55:30 -07:00
epriestley
143f86d60b Tighten query compiler rules around spaces inside and after operators
Summary:
Ref T13509. Since `title:- cat` is now ambiguous, forbid spaces after operators.

Also, forbid spaces inside operators, although this has no effect today.

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21109
2020-04-14 10:51:55 -07:00
epriestley
8fa8d0e648 Make Ferret query functions sticky only if their values are not quoted
Summary:
Ref T13509. Currently, functions are "sticky", but this stickness is in the query execution layer.

Instead:

  - move stickiness to the query compiler; and
  - make it so that functions are not sticky if their arguments are quoted.

For example:

  - `title:x y` previously meant `title:x title:y` (and still does). The "title:" is sticky.
  - `title:"x" y` previously meant `title:x title:y`. It now means `title:x all:y`. The "title:" is not sticky because the argument is quoted.

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21108
2020-04-14 10:47:51 -07:00
epriestley
f31b9987ba Add "absent" and "present" field operators to the Ferret query compiler
Summary: Ref T13509. Parse "xyz:-" as "xyz is absent" and "xyz:~" as "xyz is present". These are new operators which the compiler emits separately from "not" and "substring".

Test Plan: Added unit tests, ran unit tests.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21107
2020-04-14 10:47:20 -07:00
epriestley
5c30a60e30 Tighten Ferret query parsing of empty tokens and empty functions
Summary:
Ref T13509. Certain query tokens like `title:=""` are currently accepted by the parser but discarded, and have no impact on the query. This isn't desirable.

Instead, require that tokens making an assertion about field content must be nonempty.

Test Plan: Added unit tests, made them pass.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21106
2020-04-14 10:32:46 -07:00
epriestley
471e89a8b7 Add "uri" to "paste.search" API output
Summary: Ref T13490. This simplifies some client behavior in the general case.

Test Plan: Called API method, saw URIs.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21105
2020-04-13 16:17:33 -07:00
epriestley
19e0abcb27 Fix an issue where raw diffs that are not attached to revisions could skip repository policy checks
Summary:
See PHI1697. If a diff is not attached to a revision (for example, if it was created with "arc diff --only"), but is attached to a repository, it is supposed to be visible only to users who can see that repository.

It currently skips this extended policy check and may incorrectly be visible to too many users.

(Once a diff is attached to a revision, this rule is enforced properly via the revision policy.)

Test Plan:
  - Set repository R to be visible only to Alice.
  - As Alice, created a diff from a working copy of repository R with "arc diff --only".
  - As Bailey, viewed the diff.
    - Before: visible diff.
    - After: policy exception (as expected).

Differential Revision: https://secure.phabricator.com/D21103
2020-04-13 12:08:35 -07:00
epriestley
5597f4e6f2 Add "uri" to the fields returned by "differential.revision.search"
Summary: Ref T13490. This simplifies mostly-theoretical cases where you're accessing Phabricator via arc-over-ssh and the Conduit protocol + domain may differ from the production protocol + domain.

Test Plan: Called API via web UI, saw sensible URI values in results.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21102
2020-04-13 12:06:39 -07:00
epriestley
c3be82fe6e Fix an out-of-date API call on the destruction pathway for Pholio mocks
Summary:
See <https://discourse.phabricator-community.org/t/destroying-a-mock-using-bin-remove-destroy-mx-gives-an-error/3728>.

Currently, Pholio calls an older API method on the mock destruction pathway. This call was introduced in D19911 but the callsite was only partially updated in D19914.

Test Plan: Ran "bin/remove destroy Mx" to destroy a mock. Before: fatal with a bad call; after: clean destruction.

Differential Revision: https://secure.phabricator.com/D21081
2020-04-10 08:01:34 -07:00
epriestley
58fbf64a27 Refine handling of "@task" attributes in Diviner
Summary: Ref T13505. See that task for details. When a class has exactly one "@task" block, this API returns a string. Some day, this should be made more consistent.

Test Plan: Viewed a class with exactly one "@task", no more fatal. Viewed classes with zero and more than one "@task" attributes, got clean renderings.

Maniphest Tasks: T13505

Differential Revision: https://secure.phabricator.com/D21062
2020-04-06 11:51:33 -07:00
epriestley
271e104c7e Update DivinerAtomController for a long-ago change to the docblock parser API
Summary: Ref T13505. See that task for discussion.

Test Plan: Ran `diviner generate` locally, found a page fataling on this `strlen()`, applied patch, got a sketchy but not-broken page.

Maniphest Tasks: T13505

Differential Revision: https://secure.phabricator.com/D21061
2020-04-06 11:31:31 -07:00
epriestley
f1d1ec3d77 Add an "isDone" flag to "transaction.search" for Differential inline comments
Summary: See PHI1684. Expose the published state of the "Done" checkbox to the API.

Test Plan: Made API calls on a comment in all four states, got correct published states via the API in all cases.

Differential Revision: https://secure.phabricator.com/D21059
2020-04-05 09:36:15 -07:00
epriestley
33b73d887a If daemon running-as-user setup check fails its query, don't bother with it
Summary:
See <https://discourse.phabricator-community.org/t/upgrade-from-sep-30-2016/3702/>. A user performing an upgrade from 2016 to 2020 ran into an issue where this setup query is overheating.

This is likely caused by too many rows changing state during query execution, but the particulars aren't important since this setup check isn't too critical and will catch the issue eventually. It's fine to just move on if this query fails for any reason.

Test Plan: Forced the query to overheat, loaded setup issues, got overheating fatal. Applied patch, no more fatal.

Differential Revision: https://secure.phabricator.com/D21057
2020-04-03 16:18:55 -07:00
epriestley
1e7cc72cd8 Improve performance when marking commits as unreachable after multiple ref deletions
Summary:
See PHI1688. If many refs with a large amount of shared ancestry are deleted from a repository, we can spend much longer than necessary marking their mutual ancestors as unreachable over and over again.

For example, if refs A, B and C all point near the head of an obsolete "develop" branch and have about 1K shared commits reachable from no other refs, deleting all three refs will lead to us performing 3,000 mark-as-unreachable operations (once for each "<ref, commit>" pair).

Instead, we can stop exploring history once we reach an already-unreachable commit.

Test Plan:
  - Destroyed 7 similar refs simultaneously.
  - Ran `bin/repository refs`, saw 7 entries appear in the `oldref` table.
  - Ran `bin/repository discover` with some debugging statements added, saw sensible-seeming behavior which didn't double-mark any newly-unreachable refs.

Differential Revision: https://secure.phabricator.com/D21056
2020-04-03 13:28:42 -07:00
epriestley
1a59cae743 Update some Phabricator behaviors for changes to Futures
Summary:
Depends on D21053. Ref T11968. Three things have changed:

  - Overseers can no longer use FutureIterator to continue execution of an arbitrary list of futures from any state. Use FuturePool instead.
  - Same with repository daemons.
  - Probably (?) fix an API change in the Harbormaster exec future.

Test Plan:
  - Ran "bin/phd debug task" and "bin/phd debug pull", no longer saw Future-management related errors.
  - The Harbormaster future is easiest to test by just seeing if production works once this change is deployed there.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11968

Differential Revision: https://secure.phabricator.com/D21054
2020-04-03 12:28:16 -07:00
epriestley
067b04aaf1 If HTTP response headers are already sent, don't fiddle with "zlib.output_compression"
Summary:
We write some synthetic HTTP responses inside unit tests. Some responses have an indirect side effect of adjusting "zlib.output_compression", but this adjustment fails if headers have already been output. From a CLI context, headers appear to count as already-output after we write anything to stdout:

```
<?php

echo headers_sent() ? "Y" : "N";
echo "\n";
echo headers_sent() ? "Y" : "N";
echo "\n";
```

This script prints "N", then "Y".

Recently, the default severity of warnings was increased in libphutil; this has been a long-standing warning but now causes test failures.

This behavior is sort of silly but the whole thing is kind of moot anyway. Just skip it if "headers_sent()" is true.

Test Plan: Ran "arc unit --everything", got clean results.

Differential Revision: https://secure.phabricator.com/D21055
2020-04-03 12:24:58 -07:00
Arturas Moskvinas arturas@uber.com
62f5bdbbd2 According to Jira Project keys must start with an uppercase letter, followed by one or more uppercase alphanumeric characters
Summary: Jira allows creating projects which contain number in names, phabricator will not allow such projects but it should

Test Plan: Pasted URL with Jira project which contain number in project name and it was parsed and resolved properly in phabricator

Reviewers: epriestley, Pawka, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21040
2020-03-09 22:04:23 +02:00
epriestley
d0f4554dbe Read both email addresses and Google Account IDs from Google OAuth
Summary:
Ref T13493. Google returns a lower-quality account identifier ("email") and a higher-quality account identifier ("id"). We currently read only "email".

Change the logic to read both "email" and "id", so that if Google ever moves away from "email" the transition will be a bit easier.

Test Plan: Linked/unlinked a Google account, looked at the external account identifier table.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21028
2020-02-24 13:26:42 -08:00
epriestley
785f3c98da Extract raw commit messages from Git more faithfully across Git versions
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".

In newer versions of Git, the raw body can be extracted exactly.

Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.

Test Plan:
  - Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
  - Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
  - Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.

Maniphest Tasks: T5028

Differential Revision: https://secure.phabricator.com/D21027
2020-02-24 12:37:45 -08:00
epriestley
e58ef418c7 Read both older "key" and newer "accountId" identifiers from JIRA during authentication
Summary:
Depends on D21022. Ref T13493. The JIRA API has changed from using "key" to identify users to using "accountId".

By reading both identifiers, this linkage "just works" if you run against an old version of JIRA, a new version of JIRA, or an intermediate version of JIRA.

It also "just works" if you run old JIRA, upgrade to intermediate JIRA, everyone refreshes their link at least once, then you upgrade to new JIRA.

This is a subset of cases and does not include "sudden upgrade to new JIRA", but it's strictly better than the old behavior for all cases it covers.

Test Plan: Linked, unlinked, and logged in with JIRA. Looked at the "ExternalAccountIdentifier" table and saw a sensible value.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21023
2020-02-22 17:49:47 -08:00
epriestley
802b5aca05 Remove all readers and writers of "accountID" on "ExternalAccount"
Summary: Depends on D21019. Ref T13493. There are no more barriers to removing readers and writers of "accountID"; the new "ExternalAccountIdentity" table can replace it completely.

Test Plan: Linked and unlinked OAuth accounts, logged in with OAuth accounts, tried to double-link OAuth accounts, grepped for affected symbols.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21022
2020-02-22 17:49:22 -08:00
epriestley
84b5ad09e6 Remove all readers and all nontrivial writers for "accountType" and "accountDomain" on "ExternalAccount"
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.

Remove this key.

Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.

This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".

Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13493, T6703

Differential Revision: https://secure.phabricator.com/D21019
2020-02-22 17:48:46 -08:00
epriestley
b8f0613b30 Update Asana feed publishing integration for "ExternalAccountIdentifier"
Summary: Depends on D21017. Ref T13493. Update the Asana integration so it reads the "ExternalAccountIdentifier" table instead of the old "accountID" field.

Test Plan: Linked an Asana account, used `bin/feed republish` to publish activity to Asana.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21018
2020-02-22 17:48:16 -08:00
epriestley
bcaf60015a Write ExternalAccountIdentifiers when interacting with external authentication providers
Summary:
Depends on D21015. When we sync an external account and get a list of account identifiers, write them to the database.

Nothing reads them yet and we still write "accountId", this just prepares us for reads.

Test Plan: Linked, refreshed, unlinked, and re-linked an external account. Peeked at the database and saw a sensible-looking row.

Differential Revision: https://secure.phabricator.com/D21016
2020-02-22 17:46:51 -08:00
epriestley
0872051bfa Make AuthProvider, ExternalAccount, and ExternalAccountIdentifier all Destructible
Summary: Depends on D21014. Ref T13493. Make these objects all use destructible interfaces and destroy sub-objects appropriately.

Test Plan:
  - Used `bin/remove destroy --trace ...` to destroy a provider, a user, and an external account.
  - Observed destruction of sub-objects, including external account identifiers.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21015
2020-02-22 17:46:29 -08:00
epriestley
05eb16d6de Update unusual handling of external accounts in "Password" auth provider
Summary:
Depends on D21013. Ref T13493. When users log in with most providers, the provider returns an "ExternalAccount" identifier (like an Asana account GUID) and the workflow figures out where to go from there, usually a decision to try to send the user to registration (if the external account isn't linked to anything yet) or login (if it is).

In the case of password providers, the password is really a property of an existing account, so sending the user to registration never makes sense. We can bypass the "external identifier" indirection layer and just say "username -> internal account" instead of "external GUID -> internal mapping -> internal account".

Formalize this so that "AuthProvider" can generate either a "map this external account" value or a "use this internal account" value.

This stops populating "accountID" on "password" "ExternalAccount" objects, but this was only an artifact of convenience. (These records don't really need to exist at all, but there's little harm in going down the same workflow as everything else for consistency.)

Test Plan: Logged in with a username/password. Wiped the external account table and repeated the process.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21014
2020-02-22 17:46:04 -08:00
epriestley
e43ecad8af Make external account identifier APIs return multiple identifiers
Summary:
Depends on D21012. Ref T13493. Currently, auth adapters return a single identifier for each external account.

Allow them to return more than one identifier, to better handle cases where an API changes from providing a lower-quality identifier to a higher-quality identifier.

On its own, this change doesn't change any user-facing behavior.

Test Plan: Linked and unlinked external accounts.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21013
2020-02-22 17:45:45 -08:00
epriestley
4094624828 Remove an ancient no-op check for duplicated external accounts
Summary:
Ref T13493. This check was introduced in D4647, but the condition can never be reached in modern Phabricator because the table has a unique key on `<accountType, accountDomain, accountID>` -- so no row can ever exist with the same value for that tuple but a different ID.

(I'm not entirely sure if it was reachable in D4647 either.)

Test Plan: Used `SHOW CREATE TABLE` to look at keys on the table and reasoned that this block can never have any effect.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21012
2020-02-22 17:45:19 -08:00
epriestley
70845a2d13 Add an "ExternalAccountIdentifier" table
Summary:
Depends on D21010. Ref T13493. External accounts may have multiple different unique identifiers, most often when v1 of the API makes a questionable choice (and provies a mutable, non-unique, or PII identifier) and v2 of the API uses an immutable, unique, random identifier.

Allow Phabricator to store multiple identifiers per external account.

Test Plan: Storage only, see followup changes.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21011
2020-02-22 17:44:13 -08:00
epriestley
fbf050167e Stop exposing raw "accountID" values directly in the web UI
Summary:
Ref T13493. The "AuthAccountView" UI element currently exposes raw account ID values, but I'm trying to make these many-to-one.

This isn't terribly useful as-is, so get rid of it. This element could use a design refresh in general.

Test Plan: Viewed the UI element in "External Accounts".

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21010
2020-02-22 17:41:55 -08:00
epriestley
149155ee20 Whitelist "vscode://" as an allowed Editor protocol
Summary:
See PHI1647, which asks for "vscode://" to be a configurable protocol on hosted Phacility instances.

I made the configuration editable in D21008, but this can reasonably just come upstream too.

Test Plan: Viewed config in Config, set my editor URI to `vscode://blahblah`.

Differential Revision: https://secure.phabricator.com/D21009
2020-02-20 12:45:35 -08:00
epriestley
29923cc71a Remove old code for sending email to external users who create objects via inbound mail
Summary:
Ref T13493. I'm updating callers to `getAccountID()` to prepare to move it to a separate table.

This callsite once supported this flow:

  - External users with no accounts send mail to `bugs@`.
  - This creates tasks in Maniphest.
  - They're CC'd when the tasks are updated.

However, after T12237 we never actually send this mail (since their addresses are necessarily unverified).

I left this code in in case this needed to be revisited, but it hasn't been an issue. Just remove it and treat these users as undeliverable.

Test Plan: As a cursory test for nothing being horribly broken, sent some object mail.

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21007
2020-02-20 12:41:51 -08:00
epriestley
64cc4fe915 Add a test to verify that all routing maps are plausibly valid, and remove some dead routes
Summary:
Previously, see D20999. See also <https://discourse.phabricator-community.org/t/the-phutil-library-phutil-has-not-been-loaded/3543/>.

There are a couple of dead "Config" routes after recent changes. Add test coverage to make sure routes all point somewhere valid, then remove all the dead routes that turned up.

Test Plan: Ran tests, saw failures. Removed dead routes, got clean tests.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21000
2020-02-14 18:06:24 -08:00
epriestley
4790a3d94b Stop trying to version-check libphutil in "Config"
Summary:
Ref T13395. No library with this name loads any more, so we can't version check it.

(Ideally, the version check stuff would be more graceful when it fails now, since it's required to load "Config" after I moved it off a separate page.)

Test Plan: Loaded "Config".

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20995
2020-02-14 08:44:50 -08:00
epriestley
dc35ce79e4 Unprototype "Draft" mode in Differential
Summary: Fixes T2543. This mode has been a stable prototype for a very long time now; promote it so "--draft" can promote out of "experimental" in Arcanist.

Test Plan: See T2543 for discussion.

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D20983
2020-02-12 16:20:39 -08:00
epriestley
35a18146a2 Merge a small amount of remaining "libphutil/" code with Phabricator, break libphutil dependency
Summary: Ref T13395. Moves a small amount of remaining "libphutil/" code into "phabricator/" and stops us from loading "libphutil/".

Test Plan: Browsed around; there are likely remaining issues.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20981
2020-02-12 15:17:36 -08:00
epriestley
f9b3e3360b Continue moving classes with no callers in libphutil or Arcanist to Phabricator
Summary: Ref T13395. Move cache classes, syntax highlighters, other markup classes, and sprite sheets to Phabricator.

Test Plan: Attempted to find any callers for any of this stuff in libphutil or Arcanist and couldn't.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20977
2020-02-12 13:14:04 -08:00
Arturas Moskvinas arturas@uber.com
8cc6fe465c Fix diffusion.branchquery returning dictionary instead of array when branches are filtered out
Summary:
`diffusion.branchquery` can return dictionary instead of array if some branches are filtered out.
Eg.:
```
{
  "result": [
    {
      "shortName": "master",
      "commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
      "refType": "branch",
      "rawFields": {
        "objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
        "objecttype": "commit",
```
might become:
```
{
  "result": {
    "1": {
      "shortName": "master",
      "commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
      "refType": "branch",
      "rawFields": {
        "objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
        "objecttype": "commit",

```
Reproduction - find repository which has couple of branches, setup to track only some of them, execute `diffusion.branchquery` API call - result is dictionary instead of array

Test Plan: Apply patch, execution `diffusion.branchquery` call - result is no longer dictionary if it was one before

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D20973
2020-02-12 11:50:22 -08:00
epriestley
af84f215f9 Move lingering "Aphront" classes to Phabricator
Summary: Ref T13395. Moves some Aphront classes from libphutil to Phabricator.

Test Plan: Grepped for symbols in libphutil and Arcanist.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20975
2020-02-12 11:50:14 -08:00
epriestley
2327578adc Respect linebreaks in full HTML tables in Remarkup
Summary:
Fixes T5427. See PHI1630. See also T13160 and D20568.

In the full HTML table syntax with "<table>", respect linebreaks as literals inside "<td>" cells.

Test Plan: Previewed some full-HTML tables with and without linebreaks, saw what seemed like sensible rendering behavior.

Maniphest Tasks: T5427

Differential Revision: https://secure.phabricator.com/D20971
2020-02-06 15:01:16 -08:00
epriestley
9d1af762d5 In summary interfaces, don't render very large inline remarkup details for unit test messages
Summary: Ref T10635. An install with large blocks of remarkup (4MB) in test details is reporting slow page rendering. This is expected, but I've mostly given up on fighting this unless I absolutely have to. Degrade the interface more aggressively.

Test Plan:
  - Submitted a large block of test details in remarkup format.
  - Before patch: they rendered inline.
  - After patch: degraded display.
  - Verified small blocks are not changed.

{F7180727}

{F7180728}

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T10635

Differential Revision: https://secure.phabricator.com/D20970
2020-02-05 14:26:38 -08:00
epriestley
fd46c597ae When sorting subscriber references for display in the curtain UI, sort without case sensitivity
Summary: Ref T13486. Currently, "Zarbo" sorts above "alice", but this isn't expected for a list of (mostly) human usernames.

Test Plan: Loaded a task with subscribers with mixed-case usernames.

Maniphest Tasks: T13486

Differential Revision: https://secure.phabricator.com/D20969
2020-02-04 15:26:05 -08:00
epriestley
fdbe9ba149 Improve Remarkup parsing performance for certain large input blocks
Summary: Fixes T13487. In PHI1628, an install has a 4MB remarkup corpus which takes a long time to render. This is broadly expected, but a few reasonable improvements fell out of running it through the profiler.

Test Plan:
  - Saw local cold-cache end-to-end rendering time drop from 12s to 4s for the highly secret input corpus.
  - Verified output has the same hashes before/after.
  - Ran all remarkup unit tests.

Maniphest Tasks: T13487

Differential Revision: https://secure.phabricator.com/D20968
2020-02-04 15:07:00 -08:00
epriestley
0e82bd024a Use the new "CurtainObjectRefList" UI element for subscribers
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.

Also, improve the sorting and ordering behavior.

This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.

Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.

Maniphest Tasks: T13486

Differential Revision: https://secure.phabricator.com/D20967
2020-02-04 12:38:41 -08:00
epriestley
2a92fef879 Improve wrapping and overflow behavior for curtain panels containing long usernames
Summary:
Ref T13486. When a curtain element like "Author" in Maniphest has a very long username, the wrapping and overflow behavior is poor: the date is obscured.

Adjust curtain elements which contain lists of references to other objects to improve wrapping behavior (put the date on a separate line) and overflow behavior (so we get a "..." when a name overflows).

Test Plan: {F7179376}

Maniphest Tasks: T13486

Differential Revision: https://secure.phabricator.com/D20966
2020-02-04 12:31:18 -08:00
epriestley
84fd5cd5bb Fix an issue where intracontent empty lines were incorrectly trimmed in quoted blocks
Summary: Fixes T13335. When processing quoted blocks, we remove leading empty lines. This logic incorrectly continued after encountering a nonempty line.

Test Plan: Added a test, made it pass. Previewed blocks in web UI.

Maniphest Tasks: T13335

Differential Revision: https://secure.phabricator.com/D20965
2020-02-04 08:09:50 -08:00
epriestley
0f1acb6cef Update GitHub API calls to use "Authorization" header instead of "access_token" URI parameter
Summary: Fixes T13485. GitHub has deprecated the "access_token" URI parameter for API authentication. Update to "Authorization: token ...".

Test Plan: Linked and unlinked a GitHub account locally.

Maniphest Tasks: T13485

Differential Revision: https://secure.phabricator.com/D20964
2020-02-04 07:58:03 -08:00
epriestley
6d4c6924d6 Update Herald rule creation workflow to use more modern UI elements
Summary: Ref T13480. Creating a rule in Herald currently uses the older radio-button flow. Update it to the "clickable menu" flow to simplify it a little bit.

Test Plan: Created new personal, object, and global rules. Hit the object rule error conditions.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20956
2020-02-04 07:37:54 -08:00
epriestley
4904d7711e When publishing a commit, copy "Related Tasks" from the associated revision (if one exists)
Summary:
Fixes T13463. Currently, if you use the web UI to set "Related Tasks" for a revision, the resulting commit does not link to the tasks.

If you use "Ref ..." in the message instead, the resulting commit does link to the tasks.

Broadly, this should all be cleaner (see T3577) but we can step toward better behavior by just copying these edges when commits are published.

Test Plan:
  - Created a revision.
  - Used the web UI to edit "Related Tasks".
  - Landed the revision.
  - Saw the commit link to the tasks as though I'd used "Ref ..." in the message.

Maniphest Tasks: T13463

Differential Revision: https://secure.phabricator.com/D20961
2020-02-04 07:05:09 -08:00
epriestley
530145ba3b Give "Config" a full-width, hierarchical layout
Summary:
Depends on D20933. Ref T13362. This reorganizes Config a bit and attempts to simplify it.

Subsections are now in a landing page console and groupings have been removed. We "only" have 75 values you can edit from the web UI nowadays, which is still a lot, but less overwhelming than it was in the past. And the trend is generally downward, as config is removed/simplified or moved into application settings.

This also gets rid of the "gigantic blobs of JSON in the UI".

Test Plan: Browsed all Config sections.

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20934
2020-02-04 06:59:51 -08:00
epriestley
26c2a1ba68 Move existing "Console" interfaces away from "setFixed(...)" on "TwoColumnView"
Summary: Depends on D20931. Ref T13362. Move all "Console"-style interfaces to use a consistent layout based on a new "LauncherView" which just centers the content.

Test Plan: Viewed all affected interfaces.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20933
2020-02-04 06:52:23 -08:00
epriestley
cb481f36c5 Carve out a separate "Services" section of Config
Summary: Depends on D20930. Ref T13362. Put all the "Services" parts of Config in their own section.

Test Plan: Clicked through each section. This is just an organization / UI change with no significant behavioral impact.

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20931
2020-02-04 06:47:31 -08:00
epriestley
a72ade9475 Carve out a separate "Modules/Extensions" section of Config
Summary:
Ref T13362. Config is currently doing a ton of stuff and fairly overwhelming. Separate out "Modules/Extensions" so it can live in its own section.

(This stuff is mostly useful for development and normal users rarely need to end up here.)

Test Plan: Visited seciton, clicked around. This is just a visual change.

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20930
2020-02-04 06:41:55 -08:00
epriestley
c42c5983aa Fix an issue where loading a mangled project graph could fail too abruptly
Summary:
Ref T13484. If you load a subproject S which has a mangled/invalid `parentPath`, the query currently tries to execute an empty edge query and fatals.

Instead, we want to deny-by-default in the policy layer but not fail the query. The subproject should become restricted but not fatal anything related to it.

See T13484 for a future refinement where we could identify "broken / data integrity issue" objects explicilty.

Test Plan:
  - Modified the `projectPath` of some subproject in the database to `QQQQ...`.
  - Loaded that project page.
  - Before patch: fatal after issuing bad edge query.
  - After patch: "functionally correct" policy layer failure, although an explicit "data integrity issue" failure would be better.

Maniphest Tasks: T13484

Differential Revision: https://secure.phabricator.com/D20963
2020-02-03 08:54:04 -08:00
epriestley
42e46bbe5a Fix an issue where Herald rules could fail to evaluate at post-commit time
Summary: Ref T13480. Some Herald fields need audit information, which recent changes to Herald adapters discarded. For now, just load it unconditionally.

Test Plan: Triggered an Audit-related rule locally.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20962
2020-02-03 05:09:43 -08:00
epriestley
ccf28a8112 Fix an issue where the last line of block-based diffs could be incorrectly hidden
Summary:
Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.

For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.

Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.

Maniphest Tasks: T13468

Differential Revision: https://secure.phabricator.com/D20959
2020-01-30 08:19:09 -08:00
epriestley
12c3370988 When issuing a "no-op" MFA token because no MFA is configured, don't give the timeline story a badge
Summary:
Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.

An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.

(Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)

When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.

This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.

Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.

Test Plan:
  - As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
  - As a user with MFA, renamed a user. Got badged stories in both cases.

Maniphest Tasks: T13475

Differential Revision: https://secure.phabricator.com/D20958
2020-01-30 07:35:40 -08:00
epriestley
c99485e8a0 Add "Author's Packages" and "Committer's Packages" Herald rules for Commits and Hooks
Summary: Fixes T13480. Adds the remaining missing Owners package rules for Herald commit adapters.

Test Plan: Created hooks which care about these fields, pushed commits, saw sensible transcript values.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20957
2020-01-29 15:52:07 -08:00
epriestley
6628cd2b4f In Herald "Commit" rules, use repository identities to identify authors and committers
Summary: Ref T13480. The Herald "Commit" rules still use raw commit data properties to identify authors and committers. Instead, use repository identities.

Test Plan: Wrote a Herald rule using all four fields, ran it against various commits with and without known authors. Checked transcript for sensible field values.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20955
2020-01-29 15:48:59 -08:00
epriestley
41f143f7fe Respect repository identities when figuring out authors/committers in Herald pre-commit hook rules
Summary:
Ref T13480. Currently, Herald commit hook rules use a raw address resolution query to identify the author and committer for a commit. This will get the wrong answer when the raw identity string has been explicitly bound to some non-default user (most often, it will fail to identify an author when one exists).

Instead, use the "IdentityEngine" to properly resolve identities.

Test Plan: Authored a commit as `X <y@example.com>`, a raw identity with no "natural" matches to users (e.g., no user with that email or username). Bound the identity to a particular user in Diffusion. Wrote a Herald pre-commit content rule, pushed the commit. Saw Herald recognize the correct user when evaluating rules.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20953
2020-01-29 15:15:11 -08:00
epriestley
a0a346be34 In Herald transcripts, render some field values in a more readable way
Summary:
Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.

Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.

Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20951
2020-01-29 15:14:06 -08:00
epriestley
19662e33bc In Herald transcript rendering, don't store display labels in keys
Summary:
Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).

Instead, keep values as list items.

Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20949
2020-01-29 15:11:41 -08:00
epriestley
a5a9a5e002 Remove legacy pre-loading of handles from Herald rendering
Summary: Ref T13480. When Herald renders rules, it partly uses a very old handle pre-loading mechanism where PHIDs are extracted and loaded upfront. This was obsoleted a long time ago and was pretty shaky even when it worked. Get rid of it to simplify the code a little.

Test Plan: Viewed Herald rules rendered into static text with PHID list actions, saw handles. Grepped for all affected methods.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20948
2020-01-29 15:07:23 -08:00
epriestley
b38449ce8f Implement an "Author's packages" Herald field for Differential
Summary: Ref T13480. Add an "Author's packages" field to Herald to support writing rules like "if affected packages include X, and author's packages do not include X, raise the alarm".

Test Plan: Wrote and executed rules with the field, saw a sensible field value in the transcript.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20947
2020-01-22 18:27:51 -08:00
epriestley
6c4500046f Add "Project tags added" and "Project tags removed" fields in Herald
Summary: Ref T13480. These fields don't serve a specific strong use case, but are broadly reasonable capabilities after "state" vs "change" actions were relaxed by T13283.

Test Plan: Wrote rules using the new fields, added and removed projects (and did neither) to fire them / not fire them. Inspected the transcripts to see the project PHIDs making it to the field values.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20946
2020-01-22 18:20:57 -08:00
epriestley
6ccb6a6463 Update "git rev-parse" invocation to work in Git 2.25.0
Summary:
Fixes T13479. The behavior of "git rev-parse --show-toplevel" has changed in Git 2.25.0, and it now fails in bare repositories.

Instead, use "git rev-parse --git-dir" to sanity-check the working copy. This appears to have more stable behavior across Git versions, although it's a little more complicated for our purposes.

Test Plan:
  - Ran `bin/repository update ...` on an observed, bare repository.
  - ...on an observed, non-bare ("legacy") repository.
  - ...on a hosted, bare repository.

Maniphest Tasks: T13479

Differential Revision: https://secure.phabricator.com/D20945
2020-01-16 11:39:23 -08:00
epriestley
f806528983 Allow the Herald Rule Editor to apply generic "Edge" transactions
Summary: Fixes T13469. Currently, "Mute" applies a generic edge transaction but the editor doesn't whitelist them.

Test Plan: Muted a Herald rule.

Maniphest Tasks: T13469

Differential Revision: https://secure.phabricator.com/D20943
2020-01-15 08:29:46 -08:00