1. 22 May, 2019 20 commits
    • epriestley's avatar
      When creating a Phriction document, mark initial transactions as "create"... · f6af1c43
      epriestley authored
      When creating a Phriction document, mark initial transactions as "create" transactions to fix weird email
      
      Summary:
      Ref T13289. When you create a Phriction document, you currently get an email with the whole new content as a "diff".
      
      You also get extra transactions in the email and on the page.
      
      This is because Phriction isn't on EditEngine and doesn't mark "create" transactions in a modern way. Get them marked properly to fix these obviously-broken behaviors. This can all go away once Phriction switches to EditEngine, although I don't have any particular plans to do that in the immediate future.
      
      Test Plan:
        - Created a new document, viewed email, no longer saw redundant "edited content" transaction or "CHANGES TO CONTENT" diff.
        - Updated a document, viewed email, got interdiff.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20548
      f6af1c43
    • epriestley's avatar
      Drop the "update revision with commit diff" transaction if the revision is already closed · b95bf722
      epriestley authored
      Summary:
      Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.
      
      We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)
      
      To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.
      
      This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.
      
      Test Plan:
        - Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
        - Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
        - After: repeated runs had no effects.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13290
      
      Differential Revision: https://secure.phabricator.com/D20545
      b95bf722
    • epriestley's avatar
      Prevent "Differential Revision: ..." from counting as a mention in commit messages · 1eff4fdc
      epriestley authored
      Summary:
      Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.
      
      Stop it from doing that, since these mentions are silly/redundant/unintended.
      
      The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".
      
      Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291, T13290
      
      Differential Revision: https://secure.phabricator.com/D20544
      1eff4fdc
    • epriestley's avatar
      Stabilize sorting of feed stories with similar strength · fa4dcaa3
      epriestley authored
      Summary:
      See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.
      
      This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:
      
      {F6463721}
      
      Switch to `msortv()`, which is a stable sort. Previously, see also T6861.
      
      If all transactions have the same strength, we'll now consistently pick the first one.
      
      This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.
      
      Test Plan:
      Top story was published after this change and uses the chronologically first transaction as the title story.
      
      Bottom story was published before this change and uses the chronologically second transaction as the title story.
      
      Both stories have two transactions with the same strength ("create" + "add reviewer").
      
      {F6463722}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20540
      fa4dcaa3
    • epriestley's avatar
      Stack chart functions in a more physical way · f91bef64
      epriestley authored
      Summary:
      Ref T13279. See that task for some discussion.
      
      The accumulations of some of the datasets may be negative (e.g., if more tasks are moved out of a project than into it) which can lead to negative area in the stacked chart.
      
      Introduce `min(...)` and `max(...)` to separate a function into points above or below some line, then mangle the areas to pick the negative and positive regions apart so they at least have a plausible physical interpretation and none of the areas are negative.
      
      This is presumably not a final version, I'm just trying to produce a chart that isn't a sequence of overlapping regions with negative areas that is "technically" correct but not really possible to interpret.
      
      Test Plan: {F6439195}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20506
      f91bef64
    • epriestley's avatar
      Store charts earlier and build them out a little later · f190c42b
      epriestley authored
      Summary:
      Ref T13279. Currently, we store a fairly low-level description of functions and datasets in a chart. This will create problems with (for example) translating function labels.
      
      If you view a chart someone links you, it should say "El Charto" if you speak Spanish, not "The Chart" if the original viewer speaks English.
      
      To support this, store a slightly higher level version of the chart: the chart engine key, plus configuration parameters. This is very similar to how SearchEngine works.
      
      For example, the burndown chart now stores a list of project PHIDs, instead of a list of `[accumulate [sum [fact task.open <project-phid>]]]` functions.
      
      (This leaves some serialization code with no callsites, but we may eventually have a "CustomChartEngine" which stores raw functions, so I'm leaving it for now.)
      
      As a result, function labels provided by the chart engine are now translatable.
      
      (Note that the actual chart is meaningless since the underlying facts can't be stacked like they're being stacked, as some are negative in some areas of their accumulation.)
      
      Test Plan: {F6439121}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20504
      f190c42b
    • epriestley's avatar
      Automatically select the range for charts in a general way · 493a6b72
      epriestley authored
      Summary:
      Ref T13279. Replace the hard-coded default range with a range computed by examining the chart data.
      
      Instead of having a "Dataset" return a blob of wire data, "Dataset" now returns a structure with raw wire data plus a range. I expect to add more structured data here in future changes (tooltip/hover event data, maybe function labels).
      
      Test Plan: {F6439101}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20503
      493a6b72
    • epriestley's avatar
      Wrap "<min, max>" chart domain pairs in an "Interval" class · e90360c2
      epriestley authored
      Summary: Ref T13279. Slightly simplify domain handling by putting all the "[x, y]" stuff in an Interval class. I'm planning to do something similar for ranges next, so this should make that easierr.
      
      Test Plan: Viewed chart, saw same chart as before.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20502
      e90360c2
    • epriestley's avatar
      Provide chart function labels over the wire instead of making them up · a80426b3
      epriestley authored
      Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.
      
      Test Plan: {F6438872}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20501
      a80426b3
    • epriestley's avatar
      Label important data on charts · c6052b41
      epriestley authored
      Summary:
      Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.
      
      Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.
      
      Test Plan: {F6438860}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20500
      c6052b41
    • epriestley's avatar
      Roughly support stacked area charts · 81456db5
      epriestley authored
      Summary:
      Ref T13279. This adds support for:
      
        - Datasets can have types, like "stacked area".
        - Datasets can have multiple functions.
        - Charts can store dataset types and datasets with multiple functions.
        - Adds a "stacked area" dataset.
        - Makes D3 actually draw a stacked area chart.
      
      Lots of rough edges here still, but the result looks slightly more like it's supposed to look.
      
      D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.
      
      Test Plan: {F6427780}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20498
      81456db5
    • epriestley's avatar
      Update D3 to the current version · 0776b5ca
      epriestley authored
      Summary:
      Ref T13279. Old D3 seems perfectly fine, but most of the good references seem to have been written by people who update D3 more than once every 10 years (???).
      
      This requires some minor API changes, see next diff.
      
      Test Plan: See next diff.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20497
      0776b5ca
    • epriestley's avatar
      Consolidate burndown logic into a "BurndownChartEngine" · 5c1b91ab
      epriestley authored
      Summary:
      Ref T13279. For now, we need to render burndowns from both Maniphest (legacy) and Projects (new prototype).
      
      Consolidate this logic into a "BurndownChartEngine". I plan to expand this to work a bit like a "SearchEngine", and serve as a UI layer on top of the raw chart features.
      
      The old "ChartEngine" is now "ChartRenderingEngine".
      
      Test Plan:
        - Viewed burndowns ("burnups") in Maniphest.
        - Viewed burndowns in Projects.
        - Saw the same chart.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20496
      5c1b91ab
    • epriestley's avatar
      Add a "Reports" menu item to Projects · 0aee3da1
      epriestley authored
      Summary:
      Ref T13279. Since the use cases that have made it upstream are all for relatively complex charts (e.g., requiring aggregation and composition of multiple data series in nontrivial ways) I'm currently looking at an overall approach like this:
      
        - At least for now, Charts provides a low-level internal-only API for composing charts from raw datasets.
        - This is exposed to users through pre-built `SearchEngine`-like interfaces that provide a small number of more manageable controls (show chart from date X to date Y, show projects A, B, C), but not the full set of composition features (`compose(scale(2), cos())` and such).
        - Eventually, we may put more UI on the raw chart composition stuff and let you build your own fully custom charts by gluing together datasets and functions.
        - Or we may add this stuff in piecemeal to the higher-level UI as tools like "add goal line" or "add trend line" or whatever.
      
      This will let the low-level API mature/evolve a bit before users get hold of it directly, if they ever do. Most requests today are likely satisfiable with a small number of chart engines plus raw API data access, so maybe UI access to flexible charting is far away.
      
      Step toward this by adding a "Reports" section to projects. For now, this just renders a basic burnup for the current project. Followups will add an "Engine" layer above this and make the chart it produces more useful.
      
      Test Plan: {F6426984}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20495
      0aee3da1
    • epriestley's avatar
      Start the fact daemon in "bin/phd start" · f87c1ac3
      epriestley authored
      Summary:
      Depends on D20488. Ref T13279. When installs run `bin/phd start`, start the fact daemon alongside other daemons.
      
      Since "Reports" in Maniphest now relies on Facts data, populate it.
      
      Test Plan: Ran `bin/phd start`, saw the Fact daemon start.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20489
      f87c1ac3
    • epriestley's avatar
      Fix handling of "null" domain values in Charts · 10afe1f2
      epriestley authored
      Summary: Depends on D20487. If you `min(1, 2, null)`, you get `null`. We want `1`.
      
      Test Plan: Viewed a "burnup for project X" chart where one dataseries had no datapoints. Saw a sensible domain selected automatically.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Differential Revision: https://secure.phabricator.com/D20488
      10afe1f2
    • epriestley's avatar
      Remove the legacy chart behavior from Maniphest · 146317f2
      epriestley authored
      Summary: Depends on D20486. Ref T13279. Now that the "Reports" UI uses a panel to draw a real chart from Facts, throw away the copy of the old code.
      
      Test Plan: `grep`
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20487
      146317f2
    • epriestley's avatar
      Replace the chart in Maniphest Reports with a chart driven by Facts · f8ebc71b
      epriestley authored
      Summary:
      Depends on D20485. Ref T13279. This removes the ad-hoc charting in Maniphest and replaces it with a Facts-based chart.
      
      (To do this, we build a dashboard panel inline and render it.)
      
      Test Plan: {F6412720}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20486
      f8ebc71b
    • epriestley's avatar
      Add a rough "Chart" Dashboard Panel · ff6b1387
      epriestley authored
      Summary:
      Depends on D20484. Ref T13279. Allows a chart to render as a panel.
      
      Configuring these is currently quite low-level (you have to manually copy/paste a chart key in), but works well enough.
      
      Test Plan: {F6412708}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20485
      ff6b1387
    • epriestley's avatar
      Render charts from storage instead of just one ad-hoc hard-coded chart · c458b50b
      epriestley authored
      Summary:
      Ref T13279. This changes the chart controller:
      
        - if we have no arguments, build a demo chart and redirect to it;
        - otherwise, load the specified chart from storage and render it.
      
      This mostly prepares for "Chart" panels on dashboards.
      
      Test Plan: Visited `/fact/chart/`, got redirected to a chart from storage.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Maniphest Tasks: T13279
      
      Differential Revision: https://secure.phabricator.com/D20483
      c458b50b
  2. 21 May, 2019 12 commits
    • epriestley's avatar
      Add a "{src ...}" Remarkup rule to provide a more flexible way to reference... · 4180b337
      epriestley authored
      Add a "{src ...}" Remarkup rule to provide a more flexible way to reference source files in Diffusion
      
      Summary: Depends on D20538. Ref T13291. We now recognize full source URIs, but encoding full URIs isn't super human-friendly and we can't do stuff like relative links with them. Add `{src ...}` as a way to get to this behavior that supports options and more flexible syntax.
      
      Test Plan: {F6463607}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291
      
      Differential Revision: https://secure.phabricator.com/D20539
      4180b337
    • epriestley's avatar
      Recognize self-URI links to Diffusion files and give them special rendering behavior · 56e7bde6
      epriestley authored
      Summary:
      Depends on D20530. Ref T13291. When users paste links to files in Diffusion into remarkup contexts, identify them and specialize the rendering.
      
      When the URIs are embedded with `{...}`, parse them in more detail.
      
      This is a lead-up to a `{src ...}` rule which will use the same `View` but give users more options to customize presentation.
      
      Test Plan: {F6463580}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291
      
      Differential Revision: https://secure.phabricator.com/D20538
      56e7bde6
    • epriestley's avatar
      Support "none()" in Differential to find revisions with no (un-resigned) reviewers · 7c1f6519
      epriestley authored
      Summary:
      Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now.
      
      Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche.
      
      Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: <no constraint>" and got sensible results.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20537
      7c1f6519
    • epriestley's avatar
      Support export of feed transactions to CSV/Excel/etc · 2f386957
      epriestley authored
      Summary: Depends on D20534. Ref T13294. Add export support so you can dump these out, print them on paper, notarize them, and store them in a box under a tree or whatever.
      
      Test Plan: Exported transactions to a flat file, read the file.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13294
      
      Differential Revision: https://secure.phabricator.com/D20535
      2f386957
    • epriestley's avatar
      Support filtering feed transactions by object type · 16537f7b
      epriestley authored
      Summary: Depends on D20533. Allow querying for transactions of a specific object type, so you can run queries like "Show all edits to Herald rules between date X and Y".
      
      Test Plan: {F6463478}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20534
      16537f7b
    • epriestley's avatar
      Add support for querying feed transactions by author and date range · 2e5b1885
      epriestley authored
      Summary: Depends on D20531. Ref T13294. Enable finding raw transactions in particular date ranges or with particular authors.
      
      Test Plan: {F6463471}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13294
      
      Differential Revision: https://secure.phabricator.com/D20533
      2e5b1885
    • epriestley's avatar
      Build a rough transaction-level view of Feed · 64211370
      epriestley authored
      Summary:
      Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".
      
      We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.
      
      This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.
      
      To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.
      
      To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.
      
      Test Plan: {F6463076}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13294
      
      Differential Revision: https://secure.phabricator.com/D20531
      64211370
    • epriestley's avatar
      To improve wrapping behavior of rendered README files, don't use "PHUIDocumentView" in Diffusion · 5305ebdd
      epriestley authored
      Summary:
      See PHI1268. We currently do some weird width handling when rendering Diffusion readmes in a document directory view.
      
      I think this came from D12330, which used `PHUIDocumentViewPro` to change the font, but we later reverted the font and were left with the `DocumentView`. Other changes after that modified `DocumentView` to have fixed-width behavior, but it doesn't make much sense here since the content panel is clearly rendered full-width.
      
      Today, the `DocumentView` is a more structural element with methods like `setCurtain()`. Just get rid of it to simplify things, at least as a first step.
      
      Test Plan:
      Before:
      
      {F6463493}
      
      After:
      
      {F6463492}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20536
      5305ebdd
    • epriestley's avatar
      Add a 15-second timeout to external service calls to fill Doorkeeper link tags · 2f0e655a
      epriestley authored
      Summary:
      Depends on D20528. Ref T13291. Ref T13285. Currently, we don't put a timeout on external service calls when enriching URIs for external Asana/JIRA tasks.
      
      Add a 15-second timeout so we'll do something reasonable-ish in the face of a downed service provider. Later, I plan to healthcheck Asana/JIRA providers in a generic way (see T13287) so we can stop making calls if they time out / fail too frequently.
      
      Test Plan:
        - Linked to JIRA and Asana tasks in comments.
        - Set timeout to 0.0001 seconds, saw requests time out.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291, T13285
      
      Differential Revision: https://secure.phabricator.com/D20530
      2f0e655a
    • epriestley's avatar
      Implement Asana and JIRA external links via HyperlinkEngineExtension, not separate Remarkup rules · f4201593
      epriestley authored
      Summary:
      Depends on D20527. Ref T13291. Now that we have more flexible support for URI rewriting, use it for Doorkeeper URIs.
      
      These are used when you set up Asana or JIRA and include the URI to an Asana task or a JIRA issue in a comment.
      
      Test Plan:
        - Linked up to Asana and JIRA.
        - Put Asana and JIRA URIs in comments.
        - Saw the UI update to pull task titles from Asana / JIRA using my OAuth credentials.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291
      
      Differential Revision: https://secure.phabricator.com/D20528
      f4201593
    • epriestley's avatar
      When an object is referenced by URI, treat it like a mention · 33688c8a
      epriestley authored
      Summary:
      Ref T13291. Currently, `T123` is a mention and adds an "alice mentioned this on Txxx." to `T123`, but `https://install.com/T123` is not a mention.
      
      Make the full URI a mention.
      
      Test Plan: Commented a full URI, saw the target object get a mention story in its timeline.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13291
      
      Differential Revision: https://secure.phabricator.com/D20527
      33688c8a
    • epriestley's avatar
      Fix an issue where handles could load with the incorrect viewer when building... · b8f6248e
      epriestley authored
      Fix an issue where handles could load with the incorrect viewer when building mail about changes to related objects
      
      Summary:
      See <https://phabricator.wikimedia.org/T179591>. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.
      
      Later, we introduced `HandlePool` and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.
      
      When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.
      
      Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of `setViewer()` and `clone` games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.
      
      Test Plan:
        - Created task A (public) with subtask B (private).
        - Closed subtask B as a user with access to it.
        - Viewed mail sent to subscribers of task A who can not see subtask B.
          - Before change: mail discloses title of subtask B.
          - After change: mail properly labels subtask B as "Restricted Task".
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20525
      b8f6248e
  3. 19 May, 2019 1 commit
    • epriestley's avatar
      Separate the "configuration" and "evaluation" phases of chart functions · 06778ea5
      epriestley authored
      Summary:
      Depends on D20446. Currently, chart functions are both configured through arguments and evaluated through arguments. This sort of conflates things and makes some logic more difficult than it should be.
      
      Instead:
      
        - Function arguments are used to configure function behavior. For example, `scale(2)` configures a function which does `f(x) => 2 * x`.
        - Evaluation is now separate, after configuration.
      
      We can get rid of "sourceFunction" (which was basically marking one argument as "this is the thing that gets piped in" in a weird magical way) and "canEvaluate()" and "impulse".
      
      Sequences of functions are achieved with `compose(u, v, w)`, which configures a function `f(x) => w(v(u(x)))` (note order is left-to right, like piping `x | u | v | w` to produce `y`).
      
      The new flow is:
      
        - Every chartable function is `compose(...)` at top level, and composes one or more functions. `compose(x)` is longhand for `id(x)`. This just gives us a root/anchor node.
        - Figure out a domain, through various means.
        - Ask the function for a list of good input X values in that domain. This lets function chains which include a "fact" with distinct datapoints tell us that we should evaluate those datapoints.
        - Pipe those X values through the function.
        - We get Y values out.
        - Draw those points.
      
      Also:
      
        - Adds `accumluate()`.
        - Adds `sum()`, which is now easy to implement.
        - Adds `compose()`.
        - All functions can now always evaluate everywhere, they just return `null` if they are not defined at a given X.
        - Adds repeatable arguments for `compose(f, g, ...)` and `sum(f, g, ...)`.
      
      Test Plan: {F6409890}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: yelirekim
      
      Differential Revision: https://secure.phabricator.com/D20454
      06778ea5
  4. 17 May, 2019 2 commits
    • epriestley's avatar
      Remove obsolete Dashboard panel methods with no callsites · a76e91ea
      epriestley authored
      Summary: Ref T13272. Since the move to EditEngine, these methods have no callsites.
      
      Test Plan: `grep`
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13272
      
      Differential Revision: https://secure.phabricator.com/D20484
      a76e91ea
    • epriestley's avatar
      Label transaction groups with a "group ID" so Herald can reconstruct them faithfully · 167f06d3
      epriestley authored
      Summary:
      Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".
      
      This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.
      
      Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.
      
      Test Plan:
        - Ran Herald Test Console, saw faithful selection of recent transactions.
        - Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
        - Called `transaction.search`, saw group ID information available.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13283
      
      Differential Revision: https://secure.phabricator.com/D20524
      167f06d3
  5. 16 May, 2019 5 commits
    • epriestley's avatar
      Improve the performance of tab replacement in common cases · 6bff2cee
      epriestley authored
      Summary:
      See PHI1210. For certain large inputs, we spend more time than we need to replacing tabs with spaces. Add some fast paths:
      
        - When a line only has tabs at the beginning of the line, we don't need to do as much work parsing the rest of the line.
        - When a line has no unicode characters, we don't need to vectorize it to get the right result.
      
      Test Plan:
        - Added test coverage.
        - Profiled this, got a ~60x performance increase on a 36,000 line 3MB text file.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20477
      6bff2cee
    • epriestley's avatar
      Make branch status more clear on Diffusion branches view · c5ecc388
      epriestley authored
      Summary:
      See PHI1225. Ref T13277. In Diffusion, show "default", "permanent", or "not permanent" when looking at branches.
      
      For repositories with 100 or fewer branches, put default and permanent branches on top.
      
      Test Plan: {F6426814}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: leoluk
      
      Maniphest Tasks: T13277
      
      Differential Revision: https://secure.phabricator.com/D20493
      c5ecc388
    • epriestley's avatar
      Expose subscribers transaction data via Conduit "transaction.search" · e059997e
      epriestley authored
      Summary:
      Depends on D20507. See PHI1232. Previously, see T13255 and D20209.
      
      Since nothing seems to have exploded after "projects" was exposed, give "subscribers" the same treatment.
      
      Test Plan: Added, removed, and modified subscribers. Queried transactions with "transaction.search", saw sensible "type" and data.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D20508
      e059997e
    • epriestley's avatar
      Specialize rendering of self-URIs in the form "/X123" · 23bfe0c0
      epriestley authored
      Summary:
      Depends on D20510. Ref T5378. When remarkup includes a hyperlink to the current install in the form "/X123" (which is common), load the corresponding object and specialize the rendering.
      
      This doesn't cover everything (notably, no handling for Diffusion paths yet), but does cover a lot of the most common cases.
      
      The "uri" form preserves the URI as written, but adds an icon, tag, and hovercard.
      
      The "{uri}" form is more similar to `{T123}` and shows the object name.
      
      Test Plan: {F6440367}
      
      Reviewers: amckinley, joshuaspence
      
      Reviewed By: joshuaspence
      
      Subscribers: joshuaspence
      
      Maniphest Tasks: T5378
      
      Differential Revision: https://secure.phabricator.com/D20512
      23bfe0c0
    • epriestley's avatar
      When a user pastes a Phabricator URI into the search box, redirect to the URI · 706826bf
      epriestley authored
      Summary:
      Depends on D20509. See PHI1224. Ref T5378. With some frequency, I paste URIs into the global search input (I am dumb).
      
      When I do this dumb thing, redirect to the URI as though the global search was a URI bar.
      
      Maybe only I am dumb like this, but I don't think it'll hurt anything.
      
      Test Plan: pasted a URI and hit return; tried to eat a rock
      
      Reviewers: amckinley, joshuaspence
      
      Reviewed By: joshuaspence
      
      Maniphest Tasks: T5378
      
      Differential Revision: https://secure.phabricator.com/D20510
      706826bf