1. 22 Jun, 2020 3 commits
    • Quinn Ebert's avatar
      Default Phriction ACL configuration support · 530c6484
      Quinn Ebert authored
      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.
      530c6484
    • Daniel Stone's avatar
      Accept arrays in PHID custom-field validation · 10648032
      Daniel Stone authored
      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
      10648032
    • Daniel Stone's avatar
      Preserve silent and time when updating blocked tasks · feaa199c
      Daniel Stone authored
      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.
      feaa199c
  2. 16 Jun, 2020 2 commits
    • epriestley's avatar
      (stable) Improve the quality of SSH error messages · d5a7d408
      epriestley authored
      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
      d5a7d408
    • epriestley's avatar
      (stable) Fix an issue where "Export Data" could fail if a user had a nonempty... · c96d52ca
      epriestley authored
      (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
      c96d52ca
  3. 30 May, 2020 1 commit
  4. 26 May, 2020 2 commits
    • epriestley's avatar
      (stable) In Phortune accounts, prevent self-removal more narrowly · 621f9de4
      epriestley authored
      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
      621f9de4
    • epriestley's avatar
      In Phortune accounts, prevent self-removal more narrowly · f686a0b8
      epriestley authored
      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
      f686a0b8
  5. 23 May, 2020 2 commits
  6. 22 May, 2020 6 commits
  7. 21 May, 2020 2 commits
    • epriestley's avatar
      Prevent keyboard selection of change blocks inside edit suggestions · 87fb35ab
      epriestley authored
      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
      87fb35ab
    • epriestley's avatar
      Make "Open in Editor" use the simple line number of the current selected block · 66566f87
      epriestley authored
      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
      66566f87
  8. 20 May, 2020 9 commits
    • epriestley's avatar
      Drop old "differential_commit" table · d3d41324
      epriestley authored
      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
      d3d41324
    • epriestley's avatar
      Use the changeset parse cache to cache suggestion changesets · 6d0dbeb7
      epriestley authored
      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
      6d0dbeb7
    • epriestley's avatar
      Put a readthrough cache in front of inline context construction · 5d0ae283
      epriestley authored
      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
      5d0ae283
    • epriestley's avatar
      Clean up Diffusion behaviors for inline edit suggestions · d2d7e7f5
      epriestley authored
      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
      d2d7e7f5
    • epriestley's avatar
      Render inline comment suggestions as real diffs · 10f24135
      epriestley authored
      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
      10f24135
    • epriestley's avatar
      Roughly support inline comment suggestions · 84656215
      epriestley authored
      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
      84656215
    • epriestley's avatar
      Make server components of inline comment content handling state-oriented · 00430fdb
      epriestley authored
      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
      00430fdb
    • epriestley's avatar
      Allow "has draft inlines?" queries to overheat · 4b2a4470
      epriestley authored
      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
      4b2a4470
    • epriestley's avatar
      Make inline content "state-oriented", not "string-oriented" · 87bc3052
      epriestley authored
      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
      87bc3052
  9. 19 May, 2020 13 commits
    • epriestley's avatar
      Remove PHPMailer code which generates bogus "Message-ID" email headers · 9d5b8bd1
      epriestley authored
      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
      9d5b8bd1
    • epriestley's avatar
      (stable) Treat PHP7 "Throwable" exceptions like other unhandled "Exception"... · 1aa0569b
      epriestley authored
      (stable) 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
      1aa0569b
    • epriestley's avatar
      Treat PHP7 "Throwable" exceptions like other unhandled "Exception" cases in the worker queue · 4257b26a
      epriestley authored
      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
      4257b26a
    • epriestley's avatar
      (stable) Update out-of-date API calls when rendering diffs inline in email · 400c5556
      epriestley authored
      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
      400c5556
    • epriestley's avatar
      Update out-of-date API calls when rendering diffs inline in email · 43a8d876
      epriestley authored
      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
      43a8d876
    • epriestley's avatar
      (stable) Fix an issue where builds with no initiator failed to render in build plans · 4fd7c0f7
      epriestley authored
      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
      4fd7c0f7
    • epriestley's avatar
      Fix an issue where builds with no initiator failed to render in build plans · 86d6abe9
      epriestley authored
      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
      86d6abe9
    • epriestley's avatar
      (stable) Fix "r" and "R" both replying with quote on inline comments · 8dec499b
      epriestley authored
      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
      8dec499b
    • epriestley's avatar
      Fix "r" and "R" both replying with quote on inline comments · 770a5c84
      epriestley authored
      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
      770a5c84
    • epriestley's avatar
      (stable) Fix an unusual issue with intradiff highlighting of files with... · 1157f278
      epriestley authored
      (stable) 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
      1157f278
    • epriestley's avatar
      Fix an unusual issue with intradiff highlighting of files with uncommon end-of-file modifications · 6cf017d6
      epriestley authored
      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
      6cf017d6
    • epriestley's avatar
      (stable) Survive importing Git commits with no commit message and/or no author · 40aab725
      epriestley authored
      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
      40aab725
    • epriestley's avatar
      Survive importing Git commits with no commit message and/or no author · 0c51885c
      epriestley authored
      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
      0c51885c