Summary:
Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.
This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.
Test Plan:
- Created a revision. Herald applied one transaction.
- Used the test console.
- Before: The test console only picked up the single most recent Herald transaction.
- After: The test console picked up the whole transaction group.
{F6464059}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20546
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary: Ref T5378. This class was renamed more than a year ago, in D19087. Remove the leftover compatiblity layer.
Test Plan: `grep`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D20509
Summary:
See PHI1232, which describes a reasonable use case for wanting information about the "draft" ("Hold as Draft / Do Not Auto-Promote") flag.
Also, flesh out "testPlan" and "summary". It's possible these "blob of remarkup" fields might have metadata some day (e.g., a rendered version or a list of PHIDs or something), but we could add more keys, and we already have some other transactions which work like this.
Test Plan: Used "transaction.search" to fetch these transaction types, saw type information and metadata.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20507
Summary:
See PHI1229. An install has a somewhat duct-taped registration flow which can dump users on the "Wait for Approval" screen without clear guidance. The desired guidance is something like "this is totally normal, just wait a bit for a bot to approve you".
Adding guidance here is generally reasonable and consistent with the intent of this feature.
Test Plan: {F6426583}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: kylec
Differential Revision: https://secure.phabricator.com/D20492
Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.
Test Plan:
- Wrote a Herald rule using this field.
- Created a document (rule matched).
- Edited a document (rule matched).
- Edited a document, saving as a draft (no match).
- Edited a draft, updating it (no match).
- Published a draft docuemnt (rule matched).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20520
Summary:
Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.
This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.
The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.
(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)
Test Plan: Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20519
Summary:
Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.
Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.
Test Plan:
- Viewed a modern transcript, saw a list of transactions.
- Viewed an older transcript, saw nothing (since there were no transactions in the transcript).
{F6456431}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20518
Summary:
Depends on D20516. See PHI1247. In D20331, I made the crumbs on workboards point at ancestor workboards.
However, this isn't a great destination if an ancestor doesn't actually have a workboard. In this case, point at the normal profile URI instead.
Test Plan:
- Viewed a milestone workboard with a parent that had no workboard. Saw a profile link instead of a workboard link (new behavior).
- Viewed a milestone workboard with a parent that also had a workboard. Saw a workboard link (existing old behavior still works).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20517
Summary:
Ref T13276. Previously, these edges were added directly with an `EdgeEditor`, so they did not generate transaction stories.
Now, they're added properly, but they aren't terribly useful in feed/mail. Hide them in those contexts, like we already do with other types of similar edges.
Test Plan: Will verify behavior on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20491
Summary:
See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:
- You can subscribe/unsubscribe.
- You can mention it on another object.
- You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).
Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.
For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.
Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.
Test Plan:
- Hard-locked a task.
- Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
- Soft-locked a task.
- Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
- Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
- Added some comments and stuff to a normal non-locked task, no emblems.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20513
