GitLab

On project workboard cards, also show the status of linked code reviews;
both the review itself, and any attached Harbormaster CI buildables.

This is already taken care of in the task-detail view by 45c740ac,
and extends it to the workboard view. It should probably share more code
with the task-detail view.

It will not be accepted upstream in its current form; it was felt in
https://secure.phabricator.com/T7076 discussion that performing multiple
queries for each revision to get the current state was too much. This
makes it exceedingly unlikely that doing the same number of queries for
every task in a workboard would be acceptable.

There was discussion of how to fix that, but it was essentially
impossible, and explicitly discouraged for anyone to even try.

Enforce a local policy, that on purchasing tasks, the approver will be
auto-assigned if there is no assignee.

The 'correct' fix would probably be a Herald rule, if it were actually
possible to implement. An EditEngine extension might work as well, but
this was easier.

Enforce a local policy that all tasks must have an associated project.

When we attach a new diff to a Differential revision, clear out its
'Depends On' field. This is so we don't end up with dependency cycles
when we rebase/cherry-pick commits out of order.

As upstream do not use rebasing/chery-picking/multi-patch-branch
workflows, this is unlikely to be accepted.

This is really lame. The Ruby OAuth2 client can only pass its token in
the form data (which Phab is not prepared to accept), or as part of the
Authorization header (which PHP strips out).

Use a function only available in newer PHP to scrape the Authorization
header from the raw stream.

I have no idea what the correct fix is.

Mattermost OAuth requires two separate URIs, whereas the Phabricator
OAuth server only allows returning to a single URI. Special case
Mattermost and get on with life.

The correct fix is to actually allow multiple values in the OAuth
configuration. I don't know off the top of my head how this would work,
e.g. a tokenising field, or a multi-line field (which I don't know how
to achieve without Remarkup), or ... ?

PhabricatorApplicationTransactionEditor contains logic (inside
combineTransactions -> mergeTransactions ->
mergePHIDOrEdgeTransactions), which will combine two transactions of the
same edge type and author, then apply the operations in a deterministic
order.

This breaks our change to remove dependencies when updating a
Differential revision, since we (acting as the user who uploaded the
revision) remove the DifferentialRevisionDependsOn edge, then have the
Remarkup block parser add the dependencies from the commit message
later.

The two (simplified) transactions of:
{
    "1-from-our-change-to-differential": {
        "type": "edge",
        "-": {
	    "PHID-DREV-1234": [...], // remove previous dep
	}
    },
    "2-from-remarkup-parsing": {
        "type": "edge",
        "+": {
	    "PHID-DREV-1234": [...], // add dep from commit message
	}
    }
}

get merged into:
{
    "1-combined": {
        "type": "edge",
	"-": {
	    "PHID-DREV-1234": [...], // remove previous dep
	},
	"+": {
	    "PHID-DREV-1234": [...], // add dep from commit message
	}
    }
}

getPHIDTransactionNewValue() then returns an empty dictionary, because
it always executes the add before the remove, regardless of ordering.
The correct fix would be quite invasive to the transaction editor
(making the combine function considerably less naïve, and always
preserving order of operation WRT identical PHIDs); the quick fix for
now (at least) is to just make add operations execute after remove, thus
'fixing' it for the only case we really care about.

The correct fix is more time than worthwhile to achieve, especially
since it's extremely difficult to achieve without code modifications.

This is completely broken with modern transactions. It should also be
rewritten to match Phabricator's coding style.

If a transaction has an object that a particular user cannot see, then
mark the transaction as hidden for the default transaction view. This
particularly elides 'foo moved this task from Restricted Workboard
Column to Restricted Workboard Column on the Restricted Project board'
messages in the timeline, which are not hugely useful.

This would need a fair bit more work to go upstream, especially eliding
notifications for restricted-only transactions as well.

If the viewer doesn't have permission to see something a task has been
tagged with, then don't show it to them in the Maniphest task list view,
the task detail view, or the workboard view.

This should be extended further to also eliminate it from the
transaction history (in the task detail view) and also from
notifications, but it's a start.

This would need quite a bit more work to go upstream.

Add a 'subtype' parameter to custom fields, which means they will only
be visible on (and validated with) tasks of a particular subtype.

Suitable for upstream after much rework:
https://secure.phabricator.com/D17593

By taking the story time from the transaction creation date we ensure that the times reported in the feed view match the ones reported in the item-specific change lists.

This implements an Applications > Phriction configuration option that allows the administrators to specify default view and edit ACL policies for root-level Phriction documents.

Test Plan:
  1. Create a clean test install of Phabricator, login as the admin user
  2. Go to Applications, configure settings for Phriction, set up the ACL you want
  3. Upon creating the first document in Phriction, the ACL chosen by default should be the one you configured in step 2.

As setValueFromStorage() notes, we can accept either JSON strings or arrays for PHID-type custom fields. Handle this in decodeValue by passing through an array if we've received one.

Test Plan:
  - Add Maniphest custom field with PhabricatorPeopleDatasource
  - Create task with field filled
  - Go to Maniphest task detail view
  - Observe no errors in DarkConsole / PHP error log

Ref T13042. Updating blocked tasks creates a new ManiphestTransactionEditor instance from within the current transaction application, which fails to carry over all of the current properties. Failing to update silent means that mails will be generated for updates to blocked tasks, regardless of the setting for the original transaction editor.

Failing to preserve the time can also give large time deltas in corner cases, such as running an importer which pulls tasks from yesteryear.

This equally applies to inverse-edge transactions, though I don't have a ready-made usecase for those.

Summary: See PHI1784. Currently, users who pass an invalid SSH command to Phabricator's SSH handler get an unhelpful error message. Make it more helpful.

Test Plan: Ran `./bin/ssh-exec` with no arguments (old, helpful error), invalid arguments (before: unhelpful error; after: helpful error), and valid arguments (old, helpful behavior).

Differential Revision: https://secure.phabricator.com/D21362

(stable) Fix an issue where "Export Data" could fail if a user had a nonempty custom policy preference

Summary:
The "Export Data" workflow incorrectly uses the "Policy Favorites" setting to choose a default export format. This is just a copy/paste error; the correct setting exists and is unused.

If the setting value is an array (as the "Policy Favorites" value often is), we try to use it as an array index. This generates a runtime exception after D21044.

```
[2020-06-16 06:32:12] EXCEPTION: (RuntimeException) Illegal offset type in isset or empty at [<arcanist>/src/error/PhutilErrorHandler.php:263]
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/search/controller/PhabricatorApplicationSearchController.php:460]
```

  - Use the correct setting.
  - Make sure the value we read is a string.

Test Plan:
  - Used "Export Data" with a nonempty, array-valued "Policy Favorites" setting.
    - Before: runtime exception.
    - After: clean export.
  - Used "Export Data" again, saw my selection from the first time persisted.

Differential Revision: https://secure.phabricator.com/D21361

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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