1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-27 16:00:59 +01:00
Commit graph

16642 commits

Author SHA1 Message Date
epriestley
3a76248071 When loading a page with inlines, don't select/focus inlines which we immediately upgrade to "editing"
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.

When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.

Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21217
2020-05-04 15:15:27 -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
1205070687 Use underlines instead of background color to show file moves/renames
Summary: Ref T13520. This is a style tweak that I think looks a little cleaner.

Test Plan: {F7410424}

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21205
2020-05-01 12:23:59 -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
e20feeeee9 Update static resource package definitions
Summary: Ref T13516. Differential got some new UI elements and behaviors, so update static resource package definitions.

Test Plan:
  - Saw JS requests drop from 17 to 4.
  - Saw CSS requests drop from 9 to 3.

(These won't quite match production since some JS/CSS is for DarkConsole.)

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21163
2020-04-23 13:50:19 -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