1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

17460 commits

Author SHA1 Message Date
epriestley
36075f6ce5 Correct a prose diff behavior when prose pieces include newlines
Summary:
See <https://discourse.phabricator-community.org/t/bad-regex-in-prose-diff-logic/3969>.

The prose splitting rules normally guarantee that newlines appear only at the beginning or end of blocks. However, if a prose sentence ends with text like "...x\n.", we can end up with a newline inside a "sentence".

If we do, the regular expression that breaks it into pieces will fail.

Arguably, this is an error in how sentences are split apart (we might prefer to split this into two sentences, "x\n" and ".", rather than a single "x\n." sentence) but in the general case it's not unreasonable for blocks to contain newlines, so a simple fix is to make the pattern more robust.

Test Plan: Added a failing test which includes this behavior, made it pass.

Differential Revision: https://secure.phabricator.com/D21295
2020-05-30 14:11:37 -07:00
epriestley
f686a0b827 In Phortune accounts, prevent self-removal more narrowly
Summary:
Currently, Phortune attempts to prevent users from removing themselves as account managers. It does this by checking that the new list includes them.

Usually this is sufficient, because you can't normally edit an account unless you're already a manager. However, we get the wrong result (incorrect rejection of the edit) if the actor is omnipotent and the acting user was not already a member.

It's okay to edit an account into a state which doesn't include you if you have permission to edit the account and aren't already a manager.

Specifically, this supports more formal tooling around staff modifications to billing accounts, where the actor has staff-omnipotence and the acting user is a staff member and only used for purposes of leaving a useful audit trail.

Test Plan: Elsewhere, ran staff tooling to modify accounts and was able to act as "alice" to add "bailey", even though "alice" was not herself a manager.

Differential Revision: https://secure.phabricator.com/D21288
2020-05-26 07:09:42 -07:00
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
3635a11f84 When cancelling an edit of an inline with content, don't hide the inline
Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted.

Test Plan:
  - Create a comment, type text, save draft, edit comment, cancel.
  - Before: comment hides itself.
  - After: comment properly cancels into pre-edit draft state.

Differential Revision: https://secure.phabricator.com/D21286
2020-05-22 15:40:25 -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
87fb35abb7 Prevent keyboard selection of change blocks inside edit suggestions
Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.

Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21283
2020-05-21 15:37:51 -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
9d5b8bd14a Remove PHPMailer code which generates bogus "Message-ID" email headers
Summary:
See <https://discourse.phabricator-community.org/t/how-to-override-localhost-localdomain-in-email-message-id/3876/>.

Currently, Phabricator generates a "Message-ID" only in a subset of cases (roughly: when the message is first-in-thread and we expect the thread may have more than one message).

In cases where it does not generate a message ID, it expects the SMTP server to generate one for it. Servers will generally do this, and some ONLY do this (that is, they ignore IDs from Phabricator and replace them). Thus, several pieces of configuration control whether Phabricator attempts to generate a "Message-ID" at all.

The PHPMailer code has fallback behavior which generates a "<random>@localhost.localdomain" message ID. This is never desirable and ignores Phabricator-level configuration that Message IDs should not be generated.

For now, remove this code: it is never the desired behavior and sometimes explicitly contradicts the intent of configuration.

Possibly, a better change may be to make Phabricator always generate a message ID in cases where it isn't forbidden from doing so by configuration. However, that's a more complicated change and it's not clear if/when it would produce better behavior, so start here for now.

Test Plan: Confirmed by affected user (see linked thread).

Differential Revision: https://secure.phabricator.com/D21272
2020-05-19 11:38:58 -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
770a5c8412 Fix "r" and "R" both replying with quote on inline comments
Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes.

Test Plan:
  - Clicked an inline, pressed "r", got new empty inline (previously: inline with quote).
  - Clicked an inline, pressed "R", got a new quoted inline.
  - Repeated steps with the menu items, got the expected behaviors.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21268
2020-05-19 09:28:36 -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
93b08f0e83 Fix an out-of-order access issue with inlines
Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor.

Test Plan: Will deploy.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21263
2020-05-15 13:55:58 -07:00
epriestley
e959f93489 Use a more consistent inline highlighting style with fewer redraws
Summary:
Ref T13513. The on-hover-inline reticle has switched over to have cell-based behavior. Switch the on-hover-line-number reticle to use the same behavior.

Also, clean up the dirty/redraw loop slightly: we no longer need to dirty on resize, and we don't need to redraw if the range isn't actually dirty.

Test Plan: Highlighted lines and line ranges. Hovered over inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21262
2020-05-15 12:44:40 -07:00
epriestley
c666cb9f0b Reduce the frequency of DOM scans to rebuild inlines when scrolling revisions
Summary:
Ref T13513. See PHI1734, which raises a concern about the performance of large revisions near the 100-change threshold.

Currently, `getInlines()` is called whenever the scroll position transitions between two changesets, and it performs a relatively complicated DOM scan to lift inlines out of the document.

This shows up as taking a small but nontrivial amount of time in Firefox profiles and should be safely memoizable.

Test Plan:
  - Under Firefox profiling, scrolled through a large revision.
  - Before change: `getInlines()` appeared as the highest-cost thing we're explicitly doing on profiles.
  - After change: `getInlines()` was no longer meaningfully represented on profiles.
  - Created inlines, edited inlines, etc. Didn't identify any broken behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21261
2020-05-15 09:37:41 -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
b021da71a5 When users click headers to select diff UI elements, don't eat the events
Summary: Ref T13513. Currently, clicking inline or changeset headers eats the click events. This doesn't serve any clear purpose, and means these clicks do not clear text selections from the document, which is unusual.

Test Plan:
  - Selected some text in a diff.
  - Clicked a changeset header to select it.
  - Before patch: text selection and context menu were retained.
  - After patch: text selection was cleared and context menu was dismissed.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21255
2020-05-14 14:34:38 -07:00
epriestley
42f1a95a12 Fix a flash of document selection when "oncopy" and "inline on range" behaviors interact
Summary:
Ref T13513. In Safari, do this:

  - view a 2-up diff with content on both sides;
  - select more than one line on the right side; and
  - use your mouse to click "New Inline Comment" in the context menu that pops up.

The mousedown event for the "New Inline Comment" click removes the "copy selection" behavior and creates a flash where both sides of the diff are selected.

This doesn't happen with (most) normal clicks because mouse down on a non-grabbable element clears the document selection.

To avoid this, don't reset the copy selection behavior if the user mouses down on an "<a />". This might not be robust, but seems simple and plausible as a fix.

Test Plan:
  - See above.
  - Before patch: flash of overbroad selection when clicking "New Inline Comment".
  - After patch: no selection flash.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21254
2020-05-14 14:29:46 -07:00
epriestley
f45519c060 When cancelling an inline comment edit, exit the edit state after the response arrives
Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file.

Test Plan:
  - Clicked a line number.
  - Typed some text.
  - Clicked "Cancel".
  - Before patch: paths panel flashes "1".
  - After patch: paths panel stays stable.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21253
2020-05-14 14:28:20 -07:00
epriestley
cfb5de6fa7 Distinguish more carefully between "null" inline offsets and "0" inline offsets
Summary:
Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect.

An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them.

Test Plan:
  - Used the line numbers to add an inline to lines 4-8 of a change.
  - Hovered the inline.
  - Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21252
2020-05-14 14:26:54 -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
ebef22ccc1 Improve select-to-comment behavior in Firefox and on unified diffs
Summary:
Ref T13513.

  - Firefox represents multiple selected rows as a discontinuous range. Accommodate this.
  - Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them.

Test Plan:
  - Selected multiple lines of content in Firefox, got an option to add a comment.
  - Selected content in unified mode, got an option to add a comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21249
2020-05-13 17:19: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
1da54837ea Improve line breaking behavior in Firefox and Chrome under complex conditions
Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.

Test Plan:
  - Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements.
  - This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes.
  - This change appears to have no impact on Safari, which uses the better behavior in all cases.
  - Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line:

Firefox

{F7480567}

Chrome

{F7480568}

  - After Patch: Firefox and Chrome break the line better. Here's Firefox:

{F7480569}

  - Additional context:

Safari Behavior (Unchanged)

{F7480570}

Chrome with no highlighting (desirable behavior). Firefox does the same thing.

{F7480571}

Also tested other cases, which seem never-worse in any browser.

{F7480574}

Differential Revision: https://secure.phabricator.com/D21247
2020-05-13 11:54:42 -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
e7ebd5d9d1 Make "Delete" from inline comment previews function correctly while editing comments
Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.

Test Plan:
  - Started editing an inline, clicked "Delete", got a deletion.
  - Created an inline, typed text,
  - Deleted a normal comment via preview.
  - Deleted a normal comment via the on-inline action.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21238
2020-05-08 08:54:48 -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