1. 29 Mar, 2020 14 commits
    • Daniel Stone's avatar
      LOCAL/POLICY: Maniphest: Auto-assign purchasing tasks to approver · b7af6181
      Daniel Stone authored
      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.
      b7af6181
    • Daniel Stone's avatar
      LOCAL/POLICY: Project: Make project field required for Maniphest · 0818d906
      Daniel Stone authored
      Enforce a local policy that all tasks must have an associated project.
      0818d906
    • Daniel Stone's avatar
      LOCAL/POLICY: Differential: Clear 'Depends On' when attaching new diff · 349cdac6
      Daniel Stone authored
      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.
      349cdac6
    • Daniel Stone's avatar
      HACK: Conduit: Accept OAuth2 Authorization header · 5e9cf61a
      Daniel Stone authored
      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.
      5e9cf61a
    • Daniel Stone's avatar
      HACK: OAuth: Accept Mattermost double-URI · f07c152b
      Daniel Stone authored
      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 ... ?
      f07c152b
    • Daniel Stone's avatar
      HACK: Reverse add/remove transaction application order · 661572ef
      Daniel Stone authored
      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.
      661572ef
    • Emanuele Aina's avatar
      BROKEN: Add the `phill` command line tool to import projects and tasks · ef978a90
      Emanuele Aina authored
      This is completely broken with modern transactions. It should also be
      rewritten to match Phabricator's coding style.
      ef978a90
    • Daniel Stone's avatar
      WIP: Transactions: Hide transactions involving restricted objects · 3e949520
      Daniel Stone authored
      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.
      3e949520
    • Daniel Stone's avatar
      WIP: Maniphest: Hide hidden project tags for tasks · 287324d0
      Daniel Stone authored
      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.
      287324d0
    • Daniel Stone's avatar
      WIP: Maniphest: Allow restricting custom fields to subtypes · bbc9a7d7
      Daniel Stone authored
      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
      bbc9a7d7
    • Emanuele Aina's avatar
      Derive story/mention time from transactions · 745775ca
      Emanuele Aina authored
      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.
      745775ca
    • Quinn Ebert's avatar
      Default Phriction ACL configuration support · 3643cd1f
      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.
      3643cd1f
    • Daniel Stone's avatar
      Accept arrays in PHID custom-field validation · 20dd5a7a
      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
      20dd5a7a
    • Daniel Stone's avatar
      Preserve silent and time when updating blocked tasks · 64fae39e
      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.
      64fae39e
  2. 12 Feb, 2020 1 commit
  3. 06 Feb, 2020 1 commit
  4. 05 Feb, 2020 1 commit
    • epriestley's avatar
      In summary interfaces, don't render very large inline remarkup details for unit test messages · 9d1af762
      epriestley authored
      Summary: Ref T10635. An install with large blocks of remarkup (4MB) in test details is reporting slow page rendering. This is expected, but I've mostly given up on fighting this unless I absolutely have to. Degrade the interface more aggressively.
      
      Test Plan:
        - Submitted a large block of test details in remarkup format.
        - Before patch: they rendered inline.
        - After patch: degraded display.
        - Verified small blocks are not changed.
      
      {F7180727}
      
      {F7180728}
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T10635
      
      Differential Revision: https://secure.phabricator.com/D20970
      9d1af762
  5. 04 Feb, 2020 12 commits
  6. 03 Feb, 2020 3 commits
  7. 30 Jan, 2020 3 commits
    • epriestley's avatar
      (stable) Promote 2020 Week 5 · cc11dff7
      epriestley authored
      cc11dff7
    • epriestley's avatar
      Fix an issue where the last line of block-based diffs could be incorrectly hidden · ccf28a81
      epriestley authored
      Summary:
      Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.
      
      For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.
      
      Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.
      
      Maniphest Tasks: T13468
      
      Differential Revision: https://secure.phabricator.com/D20959
      ccf28a81
    • epriestley's avatar
      When issuing a "no-op" MFA token because no MFA is configured, don't give the... · 12c33709
      epriestley authored
      When issuing a "no-op" MFA token because no MFA is configured, don't give the timeline story a badge
      
      Summary:
      Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.
      
      An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.
      
      (Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)
      
      When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.
      
      This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.
      
      Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.
      
      Test Plan:
        - As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
        - As a user with MFA, renamed a user. Got badged stories in both cases.
      
      Maniphest Tasks: T13475
      
      Differential Revision: https://secure.phabricator.com/D20958
      12c33709
  8. 29 Jan, 2020 5 commits
    • epriestley's avatar
      Add "Author's Packages" and "Committer's Packages" Herald rules for Commits and Hooks · c99485e8
      epriestley authored
      Summary: Fixes T13480. Adds the remaining missing Owners package rules for Herald commit adapters.
      
      Test Plan: Created hooks which care about these fields, pushed commits, saw sensible transcript values.
      
      Maniphest Tasks: T13480
      
      Differential Revision: https://secure.phabricator.com/D20957
      c99485e8
    • epriestley's avatar
      In Herald "Commit" rules, use repository identities to identify authors and committers · 6628cd2b
      epriestley authored
      Summary: Ref T13480. The Herald "Commit" rules still use raw commit data properties to identify authors and committers. Instead, use repository identities.
      
      Test Plan: Wrote a Herald rule using all four fields, ran it against various commits with and without known authors. Checked transcript for sensible field values.
      
      Maniphest Tasks: T13480
      
      Differential Revision: https://secure.phabricator.com/D20955
      6628cd2b
    • epriestley's avatar
      Respect repository identities when figuring out authors/committers in Herald pre-commit hook rules · 41f143f7
      epriestley authored
      Summary:
      Ref T13480. Currently, Herald commit hook rules use a raw address resolution query to identify the author and committer for a commit. This will get the wrong answer when the raw identity string has been explicitly bound to some non-default user (most often, it will fail to identify an author when one exists).
      
      Instead, use the "IdentityEngine" to properly resolve identities.
      
      Test Plan: Authored a commit as `X <y@example.com>`, a raw identity with no "natural" matches to users (e.g., no user with that email or username). Bound the identity to a particular user in Diffusion. Wrote a Herald pre-commit content rule, pushed the commit. Saw Herald recognize the correct user when evaluating rules.
      
      Maniphest Tasks: T13480
      
      Differential Revision: https://secure.phabricator.com/D20953
      41f143f7
    • epriestley's avatar
      In Herald transcripts, render some field values in a more readable way · a0a346be
      epriestley authored
      Summary:
      Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.
      
      Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.
      
      Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.
      
      Maniphest Tasks: T13480
      
      Differential Revision: https://secure.phabricator.com/D20951
      a0a346be
    • epriestley's avatar
      In Herald transcript rendering, don't store display labels in keys · 19662e33
      epriestley authored
      Summary:
      Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).
      
      Instead, keep values as list items.
      
      Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.
      
      Maniphest Tasks: T13480
      
      Differential Revision: https://secure.phabricator.com/D20949
      19662e33