Summary:
As backstory: I accidentally added the subscriber `PHID-USER-abcd` to `T1` on this install by calling `maniphest.edit`. I intended to edit `T1` on my local install.
This edit is permitted for messy technical reasons, described in T13429. It's not valid, but it's hard to prevent.
The state we reach is also possible even if the edit is rejected (i.e., someone can go manually update the database).
Regardless of how we get into this state, the state (a non-user subscriber) breaks the UI on the task page when it attempts to test if the subscriber can see the task.
To prevent this, only claim that a Handle can have capabilities if the handle is complete. If the handle is incomplete (an invalid or restricted object), it either can't be meaningfully tested for capabilities or the viewer isn't allowed to know them.
Test Plan:
Viewed `T1` on this install, saw a fatal. Applied the same edit to `T1` locally, got the same fatal. Applied patch, no more fatal. Now saw "Unknown Object (User)" in subscriber curtain.
Specifically, the fatal is:
> Attempting to test capability "view" for handle of type "USER", but this capability has not been attached.
Differential Revision: https://secure.phabricator.com/D21662
Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases.
Test Plan:
Quoting behavior:
- Quoted a comment.
- Cancelled the quoted comment without modifying anything.
- Reloaded page.
- Before changes: quoted comment still exists.
- After changes: quoted comment is deleted.
- Looked at comment count in header, saw consistent behavior (before: weird behavior).
Empty suggestion behavior:
- Created a new comment on a suggestable file.
- Clicked "Suggest Edit" to enable suggestions.
- Without making any text or suggestion changes, clicked "Save".
- Before changes: comment saves, but is empty.
- After changes: comment deletes itself without undo.
General behavior:
- Created and saved an empty comment (deletes itself).
- Created and saved a nonempty comment (saves as draft).
- Created and saved an empty comment with an edit suggestion (saves).
- Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves).
- Edited a comment, saved without changes (save).
- Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident).
- Cancel editing an unchanged comment (cancels without undo).
- Cancel editing a changed comment (cancels with undo).
- Undo'd, got text back.
- Cancel new comment with no text (deletes without undo).
- Cancel new comment with text (deletes with undo).
- Undo'd, got text back.
- Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later).
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21654
Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.
On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.
On the client, load and track all three states explicitly.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21653
Summary:
Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state.
This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed.
Test Plan: Created inlines, clicked "Suggest Edit". See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21652
Summary:
Ref T13559. To allow the client to make correct decisions about what buttons mean, track an explicit "Committed" content state.
This is the last version of the comment that has been saved on the server, and does not exist if the comment has never been saved.
Test Plan: Created comments, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21651
Summary:
Ref T13559. If you create comments A and B, then delete comments A and B, then undo the deletion of A, the UI undoes the deletion of B instead.
This is becasue the undo rows are shipped down with a static scalar metadata reference. When copied multiple times to create multiple undo rows, they reference the same data object.
Preventing this in the general case is a problem with greater scope. For now, just avoid rendering these rows with any metadata so they don't alias a single data object.
Test Plan:
- Created comments A, B.
- Deleted comments A, B.
- Clicked "Undo" on A.
- Before: Deletion of "B" undone.
- After: Deletion of "A" undone.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21650
Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).
Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.
Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".
Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21649
Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.
Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.
Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.
Test Plan: Saved empty and nonempty inlines. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21648
Summary:
Ref T13559. Instead of directly reading form state, make all callers use the "active" state instead. The state reads the form.
No functional changes, just clarifying responsiblites.
Test Plan: Created inlines, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21647
Summary:
Ref T13559. Rather than reading from the document, make client inlines actively track their current "active" state.
The "active" state is what the user currently sees in the client UI.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21646
Summary:
Ref T13559. In an effort to ultimately fix the "quote + cancel" bug, begin formalizing content states on the client.
This creates a "ContentState" client object and moves the authoritative storage for the "hasSuggestion" property to it.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21645
Summary:
Ref T13559. D21261 added caching here, but the logic in rebuilding inlines wasn't quite correct, and could lead to us double-appending.
Instead, when rebuilding, unconditionally discard the old list.
Test Plan:
- Added inline comments to a file in Differential.
- Marked some done.
- Scrolled so the inline comment header was visible, saw "X / Y Comments" button in header.
- Clicked "Show 20 more lines" on the changeset with inlines (or toggle "View Unified" / "View Side-by-Side", or other interactions likely work too).
- Before: saw "X / Y" change improperly (because inlines in that file were double-counted).
- After: saw stable count.
- Grepped for "differential-inline-comment-refresh", got no hits, concluded this event has no listeners.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21642
Summary:
Ref T13647. The ref discovery process prunes commits that no longer exist in the repository before executing "git log <new heads> --not <old heads>" to identify newly published commits.
If we don't do this, the "git log" command will fail if any old head has been pruned from the repository.
Currently, this test for missing commits starts with a call to "git for-each-ref" to attempt to resolve symbols as tag or branch names, but:
- this is painfully slow in repositories with many refs; and
- this is incorrect (not consistent with "git" behavior) for 40-character hex strings, which Git will never resolve as symbolic names.
Instead, when a symbol is a 40-character hex string, skip "git for-each-ref" and jump directly to "git cat-file --batch-check".
Test Plan:
- Ran `bin/repository update` in a repository with 65K refs and extra debugging info.
- Before: took ~30s, three calls to `git for-each-ref`.
- After: took ~20s, two calls to `git for-each-ref`. Same resolution result on queries.
Maniphest Tasks: T13647
Differential Revision: https://secure.phabricator.com/D21658
Summary: Ref T13641. These conditions are swapped, and "activeBindings" loads more data than necessary while "bindings" doesn't load enough.
Test Plan: Called method with each attachment, got good results instead of an exception.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21657
Summary:
Ref T13644. Ref T13638.
- Double-encode the symbol that is used as a path component, similar to Diffusion.
- Fix an outdated reference to ".path", which provided context for symbol lookup.
- Prevent command-clicking headers from looking up the path as a symbol.
Test Plan:
- Command-clicked headers, no longer got a symbol.
- Command-clicked stuff with "/", saw it double-encoded and decoded properly.
- Command-clicked normal symbols, saw "path" populate correctly.
Maniphest Tasks: T13644, T13638
Differential Revision: https://secure.phabricator.com/D21641
Summary:
Ref T9764. These stars are inconsistent, not accessible, and generally weird. They predate icons.
Update them to use icons instead.
Test Plan:
{F8545721}
{F8545722}
{F8545723}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21640
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.
They date from a time long ago when the product didn't have icons.
Modernize them and make them more consistent with the similar statuses in Harbormaster.
Test Plan:
{F8545690}
{F8545691}
{F8545692}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21639
Summary: Ref T9499. When using the manual "Update Diff" workflow on the web, the "Repository" field isn't pre-filled properly. This can lead to revisions losing their repository after a manual update.
Test Plan: Did a manual update of a revision with a repository, saw it stick.
Maniphest Tasks: T9499
Differential Revision: https://secure.phabricator.com/D21638
Summary:
See PHI2032. The Conduit UI shows some "Example Custom Constraints" that are intended to be generic, but use of "statuses" is misleading since many objects have a status and most of them don't support these specific values.
Make it more clear that these are generic values.
Test Plan: Read new text.
Differential Revision: https://secure.phabricator.com/D21637
Summary: Ref T13641. Now that we can provide an "Active Devices" query, provide it and make it the default.
Test Plan: Viewed Almanac devices, got a list of active devices. Clicked "All Devices" to get all devices.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21636
Summary:
Ref T13641. Phabricator sometimes makes intracluster requests that authenticate as a device.
Forbid these requests from authenticating as a disabled device.
Test Plan:
- Ran `bin/ssh-exec --phabricator-ssh-device ...` as an enabled/disabled device (worked; sensible error).
- Made Conduit calls as an enable/disabled device (worked; sensible error).
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21635
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean upgrade, saw migrated values in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21634
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw migrated data in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21633
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw keys migrate in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21632
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw mail keys migrated to mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21631
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21630
Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services.
Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21629
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.
Test Plan:
- Queried for "bindings" and "activeBindings" via Conduit.
- Disabled/enabled devices, saw binding status update in UI.
- Loaded Diffusion cluster layout.
- Grepped for `needBindings()`, `getActiveBindings()`, etc.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21628
Summary:
Ref T13641. Add a "status" property with most of the relevant support code.
This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services.
Test Plan:
- Created a new device.
- Changed the status of a device via web and API.
- Queried for devices via API.
- Searched for active and disabled devices.
- Viewed UI in list view, detail view.
- Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21627
Summary: Ref T13641. Provide minor modernizations before adding a "Disabled" state.
Test Plan: Browsed devices, created a new device.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21626
Summary: Ref T13065. See similar changes attached to that task.
Test Plan: Ran migration, got a clean database state, saw mail keys populate in mail property table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065, T13641
Differential Revision: https://secure.phabricator.com/D21625
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:
- Removes "Javelin at Facebook", long obsolete.
- Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.
Test Plan: `grep` / reading
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21624
Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers.
Test Plan:
Grepped for remaining callers.
{F8539335}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21620
Summary:
Ref T13639. There's currently a hard-to-hit bug where editing the "Repository" of a revision doesn't update this index.
Instead: update the index on repository change, not just diff update.
Test Plan:
- Updated a revision, used debug view to see index update.
- Changed repository on a revision, used debug view to see index update.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21618
Summary:
Ref T13639. Make schema changes:
- Make repositoryID nullable, for revisions with no repository.
- Remove "epoch", which has no readers and no clear use.
- Change the ordering of the key, since "pathID" has more unique values and no queries ever issue without it.
Test Plan:
- Ran `bin/storage upgrade`, got a clean schema.
- Reindexed all revisions with an external script.
- Reviewed index via debug UI, saw appropriate index for non-repositoy revisions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21617
Summary:
Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class.
This change has no functional effect and only moves code.
Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21616
Summary:
Ref T13639. The "Affected Path" table is currently hard to inspect: there's no UI, and using MySQL just gives you a bunch of IDs.
Add a simple UI and a debug-mode link to it.
Test Plan:
{F8539098}
{F8539099}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21615
Summary:
Ref T13639. Updating the affected path table has a peculiar side effect from D19426, which is a simplification of a peculiar side effect from earlier.
Don't condition Owners behavior on path index behavior.
Test Plan: Created a revision.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21614
Summary: See T13639. This change simplifies providing a more modern approach to querying this data via "differential.revision.search".
Test Plan: Called "differential.query" with paths (got an error) and without paths (got a valid query result).
Differential Revision: https://secure.phabricator.com/D21613
Summary:
Ref T13639. In D17754, this:
> OPEN REVISIONS
> Recently updated open revisions affecting this file.
...was simplified into:
> RECENTLY OPEN REVISIONS
This is a bit misleading, since the panel doesn't contain "recently open" results. Use "Recent Open" instead, which is a bit more consistent with other product text. This is still slightly misleading, but probably close enough.
Test Plan: Read text.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21612
Summary:
Ref T13631. This option has a behavior other than the behavior implied by the name and documented.
Document the correct behavior, at least. This can likely be removed after T10574.
Test Plan: Read config option help in Config.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21610
Summary: Ref T13623. In D21603, I made the "partial object" this query returns a raw row, which paging keys can no longer be extracted from properly.
Test Plan: Paged notifications to page 2, no longer saw an error.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21609
Summary:
Fixes T13622. Figuring out what permissions we have seems difficult, so address this a bit more narrowly:
- Make the "access denied" error message a bit more helpful.
- Tailor error handling for the "CREATE TEMPORARY TABLE" statement.
Test Plan:
- Created a new user, granted them "SELECT ON *.*" but not "CREATE TEMPORARY TABLE", ran `bin/storage upgrade --force --apply phabricator:20210215.changeset.02.phid-populate.php`.
- Before: fairly opaque error.
- After: fairly useful error.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13622
Differential Revision: https://secure.phabricator.com/D21608
Summary: Ref T13636. Add routing for "/robots.txt", "favicon.ico", and "/status/" on the ShortSite and BlogSite.
Test Plan: Visted all resources (and 404 pages) on Short and Blog sites, and Platform site.
Maniphest Tasks: T13636
Differential Revision: https://secure.phabricator.com/D21607
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.
- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.
Test Plan:
- Visited all URIs on ResourceSite, got sensible responses.
- Visited all URIs on main site.
- Visited 404 while logged out, got login page.
Maniphest Tasks: T13636, T12919
Differential Revision: https://secure.phabricator.com/D21606
Summary:
Ref T13635. Currently, the JSON DocumentEngine uses "phutil_json_decode()", but this can confuse "{}" and "[]".
Be more careful about how the JSON value is decoded, to preserve the distinction.
Test Plan: {F8520479}
Maniphest Tasks: T13635
Differential Revision: https://secure.phabricator.com/D21605
Summary:
Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see.
Move the story-loading piece to "willFilterPage()" to make our way thorugh this.
Test Plan:
- Made FeedStory return nothing to simulate invisible notifications, loaded page.
- Before: index access fatal.
- After: clean "no notifications".
- Loaded notifications normally, saw normal notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21603
