Summary:
Depends on D20292. Ref T13259. This converts the rest of the `getPagingValueMap()` callsites to operate on internal cursors instead.
These are pretty one-off for the most part, so I'll annotate them inline.
Test Plan:
- Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
- Queried edges with "edge.search", paged through them using the "after" cursor.
- Poked around the other stuff without catching any brokenness.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20293
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary:
Ref T13259.
(NOTE) This is "infrastructure/guts" only and breaks some stuff in Query subclasses. I'll fix that stuff in a followup, it's just going to be a larger diff that's mostly mechanical.
When a user clicks "Next Page" on a tasks view and gets `?after=100`, we want to show them the next 100 //visible// tasks. It's possible that tasks 1-100 are visible, but tasks 101-788 are not, and the next visible task is 789.
We load task ID `100` first, to make sure they can actually see it: you aren't allowed to page based on objects you can't see. If we let you, you could use "order=title&after=100", plus creative retitling of tasks, to discover the title of task 100: create tasks named "A", "B", etc., and see which one is returned first "after" task 100. If it's "D", you know task 100 must start with "C".
Assume the user can see task 100. We run a query like `id > 100` to get the next 100 tasks.
However, it's possible that few (or none) of these tasks can be seen. If the next visible task is 789, none of the tasks in the next page of results will survive policy filtering.
So, for queries after the initial query, we need to be able to page based on tasks that the user can not see: we want to be able to issue `id > 100`, then `id > 200`, and so on, until we overheat or find a page of results (if 789-889 are visible, we'll make it there before overheating).
Currently, we do this in a not-so-great way:
- We pass the external cursor (`100`) directly to the subquery.
- We query for that object using `getPagingViewer()`, which is a piece of magic that returns the real viewer on the first page and the omnipotent viewer on the 2nd..nth page. This is very sketchy.
- The subquery builds paging values based on that object (`array('id' => 100)`).
- We turn the last result from the subquery back into an external cursor (`200`) and save it for the next time.
Note that the last step happens BEFORE policy (and other) filtering.
The problems with this are:
- The phantom-schrodinger's-omnipotent-viewer thing isn't explicity bad, but it's sketchy and generally not good. It feels like it could easily lead to a mistake or bug eventually.
- We issue an extra query each time we page results, to convert the external cursor back into a map (`100`, `200`, `300`, etc).
- In T13259, there's a new problem: this only works if the object is filtered out for policy reasons and the omnipotent viewer can still see it. It doesn't work if the object is filtered for some other reason.
To expand on the third point: in T13259, we hit a case where 100+ consecutive objects are broken (they point to a nonexistent `repositoryID`). These objects get filtered unconditionally. It doesn't matter if the viewer is omnipotent or not.
In that case: we set the next external cursor from the raw results (e.g., `200`). Then we try to load it (using the omnipotent viewer) to turn it into a map of values for paging. This fails because the object isn't loadable, even as the omnipotent viewer.
---
To fix this stuff, the new approach steps back a little bit. Primarily, I'm separating "external cursors" from "internal cursors".
An "External Cursor" is a string that we can pass in `?after=X` URIs. It generally identifies an object which the user can see.
An "Internal Cursor" is a raw result from `loadPage()`, i.e. before policy filtering. Usually, (but not always) this is a `LiskDAO` object that doesn't have anything attached yet and hasn't been policy filtered.
We now do this, broadly:
- Convert the external cursor to an internal cursor.
- Execute the query using internal cursors.
- If necessary, convert the last visible result back into an external cursor at the very end.
This fixes all the problems:
- Sketchy Omnipotent Viewer: We no longer ever use an omnipotent viewer. (We pick cursors out of the result set earlier, instead.)
- Too Many Queries: We only issue one query at the beginning, when going from "external" to "internal". This query is generally unavoidable since we need to make sure the viewer can see the object and that it's a real / legitimate object. We no longer have to query an extra time for each page.
- Total Failure on Invalid Objects: we now page directly with objects out of `loadPage()`, before any filtering, so we can page over invisible or invalid objects without issues.
This change switches us over to internal/external cursors, and makes simple cases (ID-based ordering) work correctly. It doesn't work for complex cases yet since subclasses don't know how to get paging values out of an internal cursor yet. I'll update those in a followup.
Test Plan: For now, poked around a bit. Some stuff is broken, but normal ID-based lists load correctly and page properly. See next diff for a more detailed test plan.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20291
Summary:
See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff.
Currently, edits work roughly like this:
- Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y.
- Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y.
- Run Herald rules on X.
- Publish the edit to X.
The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this:
- Begin editing X.
- Update properties on X.
- Begin inverse-edge editing Y.
- Update properties on Y.
- Run (actually, skip) Herald rules on Y.
- Publish edits to Y.
- Run Herald rules on X.
- Publish edits to X.
Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems:
- Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today.
- Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.
Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete:
- Begin editing X.
- Update properties on X.
- Begin inverse-edge editing Y.
- Update properties on Y.
- Skip Herald on Y.
- Run Herald rules on X.
- Publish X.
- Publish all child-editors of X.
- Publish Y.
Test Plan:
- Created "Must Encrypt" Herald rules for Tasks and Revisions.
- Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it.
- In `bin/mail list-outbound`, saw:
- Mail about object "A" all flagged as "Must Encrypt".
- Normal mail from object B not flagged "Must Encrypt".
- Mail from object B about changing relationships to object A flagged as "Must Encrypt".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20283
Summary:
Fixes T13265. See that task for discussion. Briefly:
- For mailers that use other mailers (SMTP, Sendmail), optionally let administrators set `"message-id": false` to improve threading behavior if their local Postfix is ultimately sending through SES or some other mailer which will replace the "Message-ID" header.
Also:
- Postmark is currently marked as supporting "Message-ID", but it does not actually support "Message-ID" on `secure.phabricator.com` (mail arrives with a non-Phabricator message ID). I suspect this was just an oversight in building or refactoring the adapter; correct it.
- Remove the "encoding" parameter from "sendmail". It think this was just missed in the cleanup a couple months ago; it is no longer used or documented.
Test Plan: Added and ran unit tests. (These feel like overkill, but this is super hard to test on real code.) See T13265 for evidence that this overall approach improves behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13265
Differential Revision: https://secure.phabricator.com/D20285
Summary:
Ref T13074. Currently, if you "Move Tasks to Column..." on a board and some of the tasks require MFA to edit, the workflow fatals out.
After this change, it works properly. You still have to answer a separate MFA prompt for //each// task, which is a little ridiculous, but at least doable. A reasonable future refinement would be to batch these MFA prompts, but this is currently the only use case for that.
Test Plan: Set a task to a "Require MFA" status, bulk-moved it with other tasks on a workboard. Was prompted, answered MFA prompt, got a move.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20282
Summary:
See PHI1098. When users comment on objects, they are automatically subscribed. And when `@alice` mentions `@bailey` on a task, that usually subscribes `@bailey`.
These rules make less sense if the user is a bot. There's generally no reason for a bot to automatically subscribe to objects it acts on (it's not going to read email and follow up later), and it can always subscribe itself pretty easily if it wants (since everything is `*.edit` now and supports subscribe transactions).
Also, don't subscribe bots when they're mentioned for similar reasons. If users really want to subscribe bots, they can do so explicitly.
These rules seem slightly like "bad implicit magic" since it's not immediately obvious why `@abc` subscribes that user but `@xyz` may not, but some of these rules are fairly complicated already (e.g., `@xyz` doesn't subscribe them if they unsubscribed or are implicitly subscribed) and this feels like it gets the right/desired result almost-always.
Test Plan:
On a fresh task:
- Mentioned a bot in a comment with `@bot`.
- Before patch: bot got CC'd.
- After patch: no CC.
- Called `maniphest.edit` via the API to add a comment as a bot.
- Before patch: bot got CC'd.
- After patch: no CC.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20284
Summary:
Depends on D20277. Ref T10333.
- Put profile icons on "Group by Owner".
- Add a similar "Group by Author". Probably not terribly useful, but cheap to implement now.
- Add "Sort by Title". Very likely not terribly useful, but cheap to implement and sort of flexible?
Test Plan: {F6265396}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20278
Summary: Depends on D20279. See D20269. Agreed that explicit `-1` is probably more clear.
Test Plan: Viewed boards in each sort/group order.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20281
Summary:
Depends on D20276. Ref T10333. This one is a little bit rough/experimental, and I'm sort of curious what feedback we get about it. Weird stuff:
- All statuses are always shown, even if the filter prevents tasks in that status from appearing (which is the default, since views are "Open Tasks" by default).
- Pro: you can close tasks by dragging them to a closed status.
- Con: lots of empty groups.
- The "Duplicate" status is shown.
- Pro: Shows closed duplicate tasks.
- Con: Dragging tasks to "Duplicate" works, but is silly.
- Since boards show "open tasks" by default, dragging stuff to a closed status and then reloading the board causes it to vanish. This is kind of how everything works, but more obvious/defaulted on "Status".
These issues might overwhelm its usefulness, but there isn't much cost to nuking it in the future if feedback is mostly negative/confused.
Test Plan: Grouped a workboard by status, dragged stuff around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20277
Summary: Depends on D20275. Fixes T10578. This is a static sorting (like "By Date Created") where you can't change point values by dragging. You can still drag cards between columns, or use the "Edit" icon to change point values.
Test Plan: {F6265191}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10578
Differential Revision: https://secure.phabricator.com/D20276
Summary:
Depends on D20274. Ref T10578. This is en route to an ordering by points, it's just a simpler half-step on the way there.
Allow columns to be sorted by creation date, so the newest tasks rise to the top.
In this ordering you can never reposition cards, since editing a creation date by dragging makes no sense. This will be true of the "points" ordering too (although we could imagine doing something like prompting the user, some day).
Test Plan: Viewed boards by "natural" (allows reordering both when dragging within and between columns), "priority" (reorder only within columns), and "creation date" (reorder never). Dragged cards around between and within columns, got apparently sensible behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10578
Differential Revision: https://secure.phabricator.com/D20275
Summary:
Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things.
Instead:
- Replace the edit pencil with a red lock.
- Provide cursor hints for grabbable / not grabbable.
- Don't let users pick up cards they can't edit.
Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10722
Differential Revision: https://secure.phabricator.com/D20274
Summary: Depends on D20272. Ref T13074. When a task requires MFA to edit, you currently get a fatal. Provide a cancel URI so the prompt works and the edit can go through.
Test Plan:
- Locked a task, dragged it on a workboard.
- Before: fatal trying to build an MFA gate.
- After: got MFA gated, answered prompt, action went through.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20273
Summary:
Depends on D20270. Ref T10333. If you create a task with a new owner, or edit a task and change the priority/owner, we want to move it (and possibly create a new header) when the response comes back.
Make sure the response includes the appropriate details about the object's header and position.
Test Plan:
- Grouped by Owner.
- Created a new task with a new owner, saw the header appear.
- Edited a task and changed it to give it a new owner, saw the header appear.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20271
Summary: Depends on D20269. Ref T10333. Now that orderings are modularized, this is fairly easy to implement. This isn't super fancy for now (e.g., no profile images) but I'll touch it up in a general polish followup.
Test Plan: {F6264596}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20270
Summary:
Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`.
Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering.
Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).
Then move the existing "Natural" and "Priority" orders into this new hierarchy.
This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.
Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20269
Summary:
Depends on D20265. Ref T10333. Now that neither task lists nor workboards use subpriority, we can remove all the readers and writers.
I'm not actually getting rid of the column data yet, but anticipate doing that in a future change.
Note that the subpriority algorithm (removed here) is possibly better than the "natural order" algorithm still in use. It's a bit more clever, and likely performs far fewer writes. I might make the "natural order" code use an algorithm more similar to the "subpriority" algorithm in the future.
Test Plan: Grepped for `subpriority`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20266
Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".
When you drag the bottom-most task under "Alice" to the top, what does that mean?
Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.
Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.
We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.
In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.
Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.
This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.
Test Plan:
- Viewed and dragged a natural board.
- Viewed and dragged a priority board.
- Dragged within and between groups of 0, 1, and multiple items.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20265
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.
I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.
This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.
As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".
Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.
The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.
It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.
(This just removes UI, the actual subpriority system is still intact and still used on workboards.)
Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20263
Summary:
Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards.
As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority.
(Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.)
Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333, T8135
Differential Revision: https://secure.phabricator.com/D20248
Summary:
Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are:
- Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal".
- Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column.
- Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135).
- Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations).
- Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it.
NOTE: This only makes these headers work for display.
They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup.
Test Plan: {F6257686}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20247
Summary:
Ref T13249. Ref T13258. In some cases, builds are not idempotent and should not be restarted casually.
If the scary part is at the very end (deploy / provision / whatever), it could be okay to restart them if they previously failed.
Also, make the "reasons why you can't restart" and "explanations of why you can't restart" logic a little more cohesive.
Test Plan:
- Tried to restart builds in various states (failed/not failed, restartable always/if failed/never, already restarted), got appropriate errors or restarts.
- (I'm not sure the "Autoplan" error is normally reachable, since you can't edit autoplans to configure things to let you try to restart them.)
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T13249
Differential Revision: https://secure.phabricator.com/D20252
Summary:
Depends on D20259. Now that we can index Herald rules to affected objects, show callers on the "Webhooks" UI.
A few other rule types could get indexes too ("Sign Legalpad Documents", "Add Reviewers", "Add Subscribers"), but I think they're less likely to be useful since those triggers are usually more obvious (the transaction timeline makes it clearer what happened/why). We could revisit this in the future now that it's a possibility.
Test Plan: {F6260106}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20260
Summary:
See PHI1123. The key on this table is `<resource, type, code>` but we currently query for only `<type, code>`. This can't use the key.
Constrain the query to the resource we expect (the repository) so it can use the key.
Test Plan: Pushed files using LFS. See PHI1123 for more, likely.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20261
Summary:
Ref T13258. Provide an easy way to find rules which trigger a particular build plan from the build plan page.
The implementation here ends up a little messy: we can't just search for `actionType = 'build' AND targetPHID = '<build plan PHID>'` since the field is a blob of JSON.
Instead, make rules indexable and write a "build plan is affected by rule actions" edge when indexing rules, then search on that edge.
For now, only "Run Build Plan: ..." rules actually write this edge, since I think (?) that it doesn't really have meaningful values for other edge types today. Maybe "Call Webhooks", and you could get a link from a hook to rules that trigger it? Reasonable to do in the future.
Things end up a little bit rough overall, but I think this panel is pretty useful to add to the Build Plan page.
This index needs to be rebuilt with `bin/search index --type HeraldRule`. I'll call this out in the changelog but I'm not planning to explicitly migrate or add an activity, since this is only really important for larger installs and they probably (?) read the changelog. As rules are edited over time, this will converge to the right behavior even if you don't rebuild the index.
Test Plan: {F6260095}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20259
Summary: Depends on D20256. Ref T13249. See PHI1115. This primarily makes `bin/policy unlock --owner epriestley T123` work. This is important for "Edit Locked" tasks, since changing the edit policy doesn't really do anything.
Test Plan: Hard-locked a task as "alice", reassigned it to myself with `bin/policy unlock --owner epriestley`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20257
Summary:
See PHI1115. Ref T13249. Currently, you can `bin/policy unlock` objects which have become inaccessible through some sort of policy mistake.
This script uses a very blunt mechanism to perform unlocks: just manually calling `setXPolicy()` and then trying to `save()` the object. Improve things a bit:
- More surgical: allow selection of which policies you want to adjust with "--view", "--edit", and "--owner" (potentially important for some objects like Herald rules which don't have policies, and "edit-locked" tasks which basically ignore the edit policy).
- More flexible: Instead of unlocking into "All Users" (which could be bad for stuff like Passphrase credentials, since you create a short window where anyone can access them), take a username as a parameter and set the policy to "just that user". Normally, you'd run this as `bin/policy unlock --view myself --edit myself` or similar, now.
- More modular: We can't do "owner" transactions in a generic way, but lay the groundwork for letting applications support providing an owner reassignment mechanism.
- More modern: Use transactions, not raw `set()` + `save()`.
This previously had some hard-coded logic around unlocking applications. I've removed it, and the new generic stuff doesn't actually work. It probably should be made to work at some point, but I believe it's exceptionally difficult to lock yourself out of applications, and you can unlock them with `bin/config set phabricator.application-settings ...` anyway so I'm not too worried about this. It's also hard to figure out the PHID of an application and no one has ever asked about this so I'd guess the reasonable use rate of `bin/policy unlock` to unlock applications in the wild may be zero.
Test Plan:
- Used `bin/policy unlock` to unlock some objects, saw sensible transactions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20256
Summary:
Ref T13249. See PHI1115. I initially wanted to make `bin/policy unlock --owner <user> H123` work to transfer ownership of a Herald rule, although I'm no longer really sure this makes much sense.
In any case, this makes things a little better and more modern.
I removed the storage table for rule comments. Adding comments to Herald rules doesn't work and probably doesn't make much sense.
Test Plan: Created and edited Herald rules, grepped for all the transaction type constants.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20258
Summary:
Ref T13249. Using "--as" to call some Conduit methods as a user can currently fatal when trying to access settings/preferences.
Allow inline regeneration of user caches.
Test Plan: Called `project.edit` to add a member. Before: constructing a policy field tried to access the user's preferences and failed. After: Smooth sailing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20255
Summary:
Ref T13249.
- When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes.
- Move the markers slightly to the left, to align them with the gutter.
- Make them slightly opaque so they're a little less prominent.
Test Plan: See screenshots.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20251
Summary:
Ref T13258. The general idea here is "if arc land prompted you and you hit 'y', you get a warning about it on the timeline".
This is similar to the existing warning about landing revisions in the wrong state and hitting "y" to get through that. See D18808, previously.
These warnings make it easier to catch process issues at a glance, especially because the overall build status is now more complicated (and may legally include some failures on tests which are marked as unimportant).
The transaction stores which builds had problems, but I'm not doing anything to render that for now. I think you can usually figure it out from the UI already; if not, we could refine this.
Test Plan:
- Used `bin/differential attach-commit` to trigger extraction/attachment.
- Attached a commit to a revision with various build states, and various build plan "Warn When Landing" flags.
- Got sensible warnings and non-warnings based on "Warn When Landing" setting.
{F6251631}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20239
Summary: Ref T13258. Make the "Affects Buildable" option actually work.
Test Plan:
- As in previous change, created a "wait for HTTP request" build plan and had it always run against every revision.
- Created revisions, waited a bit, then sent the build a "Fail" message, with different values of "Affects Buildable":
- "Always": Same behavior as today. Buildable waited for the build, then failed when it failed.
- "While Building": Buildable waited for the build, but passed even though it failed (buildable has green checkmark even though build is red):
{F6250359}
- "Never": Buildable passed immediately (buildable has green checkmark even though build is still running):
{F6250360}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20233
Summary: Ref T13258. Makes the new "Hold Drafts" behavior actually work.
Test Plan:
- Created a build plan which does "Make HTTP Request" somewhere random and then waits for a message.
- Created a Herald rule which "Always" runs this plan.
- Created revisions, loaded them, then sent their build targets a "fail" message a short time later.
- With "Always": Current behavior. Revision was held as a draft while building, and returned to me for changes when the build failed.
- With "If Building": Revision was held as a draft while building, but promoted once the build failed.
- With "Never": Revision promoted immediately, ignoring the build completely.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20232
Summary:
Ref T13258. Implements the "Restartable" behavior, to control whether a build may be restarted or not.
This is fairly straightforward because there are already other existing reasons that a build may not be able to be restarted.
Test Plan: Restarted a build. Marked it as not restartable, saw "Restart" action become disabled. Tried to restart it anyway, got a useful error message.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20230
Summary:
Ref T13258. Fixes T11415. This makes "Runnable" actually do something:
- With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan.
- If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions.
This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago.
Test Plan:
- Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user.
- With "If Editable", unable to run, pause, resume, abort, or restart as another user.
- With "If Viewable", those actions work.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T11415
Differential Revision: https://secure.phabricator.com/D20229
Summary: Ref T13258. This will support changing behaviors in "arc land".
Test Plan: Called "harbormaster.buildplan.search", saw behavior information in results.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20228
Summary:
Depends on D20219. Ref T13258. Ref T11415. Installs sometimes have long-running builds or unimportant builds which they may not want to hold up drafts, affect buildable status, or warn during `arc land`.
Some builds have side effects (like deployment or merging) and are not idempotent. They can cause problems if restarted.
In other cases, builds are isolated and idempotent and generally safe, and it's okay for marketing interns to restart them.
To address these cases, add "Behaviors" to Build Plans:
- Hold Drafts: Controls how the build affects revision promotion from "Draft".
- Warn on Land: Controls the "arc land" warning.
- Affects Buildable: Controls whether we care about this build when figuring out if a buildable passed or failed overall.
- Restartable: Controls whether this build may restart or not.
- Runnable: Allows you to weaken the requirements to run the build if you're confident it's safe to run it on arbitrary old versions of things.
NOTE: This only implements UI, none of these options actually do anything yet.
Test Plan:
Mostly poked around the UI. I'll actually implement these behaviors next, and vet them more thoroughly.
{F6244828}
{F6244830}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T11415
Differential Revision: https://secure.phabricator.com/D20220
Summary:
Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer.
If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response".
Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do.
Test Plan:
- Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before).
- Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before).
- Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20235
Summary: See PHI1112. See T784. Although some more general/flexible solution is arriving eventually, adding this rule seems reasonable for now, since it's not a big deal if we remove it later to replace this with some fancier system.
Test Plan: Created a diff with the official Go generated marker, saw the changeset marked as generated.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20237
Summary:
Ref T13259. If we miss the separate CSRF step in Duo and proceed directly to prompting, we may fail to build a response which turns into a real control and fatal on `null->setLabel()`.
Instead, let MFA providers customize their "bare prompt dialog" response, then make Duo use the same "you have an outstanding request" response for the CSRF and no-CSRF workflows.
Test Plan: Hit Duo auth on a non-CSRF workflow (e.g., edit an MFA provider with Duo enabled). Previously: `setLabel()` fatal. After patch: smooth sailing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20234
Summary:
See PHI1063. See PHI1114. Ref T13253. Currently, you can't `bin/worker execute` an archived task and can't `bin/worker retry` a successful task.
Although it's good not to do these things by default (particularly, retrying a successful task will double its effects), there are plenty of cases where you want to re-run something for testing/development/debugging and don't care that the effect will repeat (you're in a dev environment, the effect doesn't matter, etc).
Test Plan: Ran `bin/worker execute/retry` against archived/successful tasks. Got prompted to add more flags, then got re-execution.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253
Differential Revision: https://secure.phabricator.com/D20246
Summary:
Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful.
In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking.
For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see).
Test Plan:
- Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels.
- With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning).
- With `quiet`, got nothing (bad: we want the credential error).
- With `ERROR`, got no warning but did get an error (good!).
Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13121
Differential Revision: https://secure.phabricator.com/D20240
Summary:
Fixes T13260. "QUERY_STRING" and "REQUEST_URI" are similar for our purposes here, but our nginx documentation tells you to pass "QUERY_STRING" and doesn't tell you to pass "REQUEST_URI". We also use "QUERY_STRING" in a couple of other places already, and already have a setup check for it.
Use "QUERY_STRING" instead of "REQUEST_URI".
Test Plan: Visited `/oauth/google/?a=b`, got redirected with parameters preserved.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13260
Differential Revision: https://secure.phabricator.com/D20227
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-muting-completed-bulk-jobs/2449>. Bulk Jobs have an "edge" table but currently do not support edge transactions. Add support.
This stops "Mute Notifications" from fataling.
The action probably doesn't do what the reporting user expects (it stops edits to the job object from sending notifications; it does not stop the edits the job performs from sending notifications) but I think this change puts us in a better place no matter what, even if we eventually clarify or remove this behavior.
Test Plan: Clicked "Mute Notifications" on a bulk job, got an effect instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20226
Summary:
See <https://discourse.phabricator-community.org/t/traceback-rendering-task-query-in-dashboard/2450/>.
It looks like this blames to D19126, which added some more complex constraint logic but overlooked "range" constraints, which are handled separately.
Test Plan:
- Added a custom "date" field to Maniphest with `"search": true`.
- Executed a range query against the field.
Then:
- Before: Warnings about undefined indexes in the log.
- After: No such warnings.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20225
Summary:
See PHI1059. If you close a task, we apply an "alice closed a subtask: X" transaction to its parents.
This transaction is purely informative, but currently requires `CAN_EDIT` permission after T13186. However, we'd prefer to post this transaction anyway, even if: the parent is locked; or the parent is not editable by the acting user.
Replace the implicit `CAN_EDIT` requirement with no requirement.
(This transaction is only applied internally (by closing a subtask) and can't be applied via the API or any other channel, so this doesn't let attackers spam a bunch of bogus subtask closures all over the place or anything.)
Test Plan:
- Created a parent task A with subtask B.
- Put task A into an "Edits Locked" status.
- As a user other than the owner of A, closed B.
Then:
- Before: Policy exception when trying to apply the "alice closed a subtask: B" transaction to A.
- After: B closed, A got a transaction despite being locked.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20223
Summary:
Depends on D20218. Ref T13258. It's somewhat cumbersome to get from build plans to related builds but this is a reasonable thing to want to do, so make it a little easier.
Also clean up / standardize / hint a few things a little better.
Test Plan: {F6244116}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20219
Summary: Depends on D20217. Ref T13258. Mostly for completeness. You can't edit build steps so this may not be terribly useful, but you can do bulk policy edits or whatever?
Test Plan: Edited a build plan via API.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20218
Summary: Depends on D20216. Ref T13258. Bland infrastructure update to prepare for bigger things.
Test Plan: Created and edited a build plan.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20217
Summary: Ref T13088. Build Plans currently have a "Created" date in the right-hand "Curtain" UI, but this is unusual and the creation date is evident from the timeline. It's also not obvious why anyone would care. Remove it for simplicity/consistency. I think this may have just been a placeholder during initial implementation.
Test Plan: Viewed a build plan, no more "Created" element.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D20216
Summary:
See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.
Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.
Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20214
Summary: Depends on D20209. Ref T13255. It would probably be nice to make this into a "real" `*.search` API method some day, but at least document the features for now.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20211
Summary:
Depends on D20208. Ref T13255. See that task for some long-winded discussion and rationale. Short version:
- This is a list of operations instead of a list of old/new PHIDs because of scalability issues for large lists (T13056).
- This is a fairly verbose list (instead of, for example, the more concise internal map we sometimes use with "+" and "-" as keys) to try to make the structure obvious and extensible in the future.
- The "add" and "remove" echo the `*.edit` operations.
Test Plan: Called `transaction.search` on an object with project tag changes, saw them in the results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20209
Summary: Ref T13255. The "transaction.search" API method currently does not support author constraints, but this is a reasonable thing to support.
Test Plan: Queried transactions by author, hit the error cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20208
Summary: Fixes T13254. See that task for details.
Test Plan: Used iOS Simulator to do a login locally, didn't get blocked. Verified CSP includes "m.facebook.com".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13254
Differential Revision: https://secure.phabricator.com/D20206
Summary: Ref T13251. See <https://phabricator.wikimedia.org/T216849>.
Test Plan:
- With more than 100 projects (or, set `$limit = 3`)...
- Edit a task, then click the "Browse" magnifying glass icon next to the "Tags" typeahead.
- Before change: fatal on 'q' being null.
- After change: no fatal.
Reviewers: amckinley, 20after4
Reviewed By: amckinley
Maniphest Tasks: T13251
Differential Revision: https://secure.phabricator.com/D20204
Summary:
Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running.
Also displays this information in the daemon status console; see screenshot: {F6225726}
Test Plan:
Stopped daemons, ran `bin/search index --all --background` to create lots of tasks, restarted daemons, observed expected values for `dateCreated` and `epochArchived` in the archive worker table.
Also tested the changes to `unarchiveTask` by forcing a search task to permanently fail and then `bin/worker retry`ing it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5401
Differential Revision: https://secure.phabricator.com/D20200
Summary:
Depends on D20197. Ref T13161. We currently try to build a "ScopeEngine" even for diffs with no context (e.g., `git diff` instead of `git diff -U9999`).
Since we don't have any context, we won't really be able to figure out anything useful about scopes. Also, since ScopeEngine is pretty strict about what it accepts, we crash.
In these cases, just don't build a ScopeEngine.
Test Plan: Viewed a diff I copy/pasted with `git diff` instead of an `arc diff` / `git diff -U99999`, got a sensible diff with no context instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20198
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:
{F6225179}
If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:
{F6225181}
Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:
{F6225183}
Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20197
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.
I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.
Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20196
Summary:
Ref T13253. Long ago, daemon logs were visible in the web UI. They were removed because access to logs generally does not conform to policy rules, and may leak the existence (and sometimes contents) of hidden objects, occasionally leak credentials in certain error messages, etc.
These bits and pieces were missed.
Test Plan: Grepped for removed symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253
Differential Revision: https://secure.phabricator.com/D20199
Summary:
Ref T13161.
- Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
- The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.
Test Plan: Got slightly better rendering for some diffs locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20195
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Ref T12822. Fixes a few things:
- Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
- "Show More Context" rows now render, highlight, and select properly.
- Prepares for nodes to have copy-text which is different from display-text.
- Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.
Test Plan:
- Selected and copied weird ranges in Firefox.
- Kept an eye on "Show More Context" rows across select and copy operations.
- Generally poked around in Safari/Firefox/Chrome.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20192
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.
As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.
We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.
In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.
In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".
Finally, we have to figure out which side of the diff we're copying from. The rule here is:
- If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
- Otherwise, use normal document copy rules.
So the overall flow here is:
- Listen for clicks.
- When the user clicks the left or right side of the diff, store what they clicked.
- When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
- (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
- When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
- When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.
Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20191
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.
After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.
This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.
Test Plan:
- Grepped for `zws`, `grep -i zero | grep -i width`.
- Copied text out of Differential: got both sides of the diff (not ideal).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20189
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".
But times have changed!
- In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
- In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.
The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.
Test Plan:
- Created, edited, deleted inlines in 1-up and 2-up views.
- Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20188
Summary:
Depends on D20185. Ref T13161. Fixes T6791.
See some discusison in T13161. I want to move to a world where:
- whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
- the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.
D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.
However, a second aspect to this is how the diff is "aligned". If this file:
```
A
```
..is changed to this file:
```
X
A
Y
Z
```
...diff tools will generally produce this diff:
```
+ X
A
+ Y
+ Z
```
This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:
```
1 X
1 A 2 A
3 Y
4 Z
```
However, if the new file looks like this instead:
```
X
A'
Y
Z
```
...we get a diff like this:
```
- A
+ X
+ A'
+ Y
+ Z
```
This one aligns like this:
```
1 A
1 X
2 A'
3 Y
4 Z
```
This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:
```
1 X
1 A 2 A'
3 Y
4 Z
```
Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.
Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.
This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:
- Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
- Normalize the files (for example, by removing whitespace from each line).
- Diff the normalized files to produce a second diff.
- Parse that diff.
- Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.
Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).
Test Plan:
{F6217133}
(Also, fix a unit test.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T6791
Differential Revision: https://secure.phabricator.com/D20187
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.
However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.
We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.
I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.
Move toward detecting and rendering indent depth changes. Followup work:
- These should get colorblind colors and the design can probably use a little more tweaking.
- The OneUp mode is okay, but could be improved.
- Whitespace mode can now be removed completely.
- I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.
Test Plan: {F6214084}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20181
Summary:
Depends on D20179. Ref T13088. See PHI351. See PHI1018. In various cases, unit tests names are 19 paths mashed together.
This is probably not an ideal name, and the test harness should probably pick a better name, but if users are fine with it and don't want to do the work to summarize on their own, accept them. We may summarize with "..." in some cases depending on how this fares in the UI.
The actual implementation is a separate "strings" table which is just `<hash-of-string, full-string>`. The unit message table can end up being mostly strings, so this should reduce storage requirements a bit.
For now, I'm not forcing a migration: new writes use the new table, existing rows retain the data. I plan to provide a migration tool, recommend migration, then force migration eventually.
Prior to that, I'm likely to move at least some other columns to use this table (e.g., lint names), since we have a lot of similar data (arbitrarily long user string constants that we are unlikely to need to search or filter).
Test Plan: {F6213819}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D20180
Summary:
Depends on D20177. Fixes T12425. See <https://discourse.phabricator-community.org/t/importing-libphutil-repository-on-fresh-phabricator-triggers-an-error/2391/>.
Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state).
I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs.
Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error.
Test Plan:
- Faked errors in both cases (initial load, re-load inside the locked section).
- Ran indexes in strict/non-strict mode.
- Got exception reports from both branches in strict mode.
- Got task success without errors in both cases in non-strict mode.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12425
Differential Revision: https://secure.phabricator.com/D20178
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).
They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.
(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)
Test Plan:
{F6212044}
{F6212045}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20173
Summary:
Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context.
We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time.
Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics.
Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249, T11738
Differential Revision: https://secure.phabricator.com/D20171
Summary: Stragglers from the great "%Q" migration.
Test Plan: Ran a query for tasks with parent tasks.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20183
Summary:
Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.
Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.
Increase the requirements for the actual transaction, which mostly applies to API changes:
- To remove subscribers other than yourself, require CAN_EDIT.
- To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.
This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.
Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20174
Summary:
Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).
Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)
When "edits" are locked:
- The edit policy looks like "no one" to normal callers.
- In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
- We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.
For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene?
Test Plan:
- Defined "Soft Locked" and "Hard Locked" statues.
- "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
- Saw nice new policy guidance icon in header.
{F6210362}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20165
Summary:
Ref T13248. This will probably need quite a bit of refinement, but we can reasonably allow subtype definitions to adjust custom field behavior.
Some places where we use fields are global, and always need to show all the fields. For example, on `/maniphest/`, where you can search across all tasks, you need to be able to search across all fields that are present on any task.
Likewise, if you "export" a bunch of tasks into a spreadsheet, we need to have columns for every field.
However, when you're clearly in the scope of a particular task (like viewing or editing `T123`), there's no reason we can't hide fields based on the task subtype.
To start with, allow subtypes to override "disabled" and "name" for custom fields.
Test Plan:
- Defined several custom fields and several subtypes.
- Disabled/renamed some fields for some subtypes.
- Viewed/edited tasks of different subtypes, got desired field behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13248
Differential Revision: https://secure.phabricator.com/D20161
Summary:
Ref T13253. Fixes T6615. See that task for discussion.
- Remove three keys which serve no real purpose: `dataID` doesn't do anything for us, and the two `leaseOwner` keys are unused.
- Rename `leaseOwner_2` to `key_owner`.
- Fix an issue where `dataID` was nullable in the active table and non-nullable in the archive table.
In practice, //all// workers have data, so all workers have a `dataID`: if they didn't, we'd already fatal when trying to move tasks to the archive table. Just clean this up for consistency, and remove the ancient codepath which imagined tasks with no data.
Test Plan:
- Ran `bin/storage upgrade`, inspected tables.
- Ran `bin/phd debug taskmaster`, worked through a bunch of tasks with no problems.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13253, T6615
Differential Revision: https://secure.phabricator.com/D20175
Summary: Ref T13088. Prepares for putting test names in a separate table to release the 255-character limit.
Test Plan: Viewed revisions, buildables, builds, test lists, specific tests.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D20179
Summary:
See PHI1073. Improve the UX here:
- When there are a small number of connected tasks, no changes.
- When there are too many total connected tasks, but not too many directly connected tasks, show hint text with a "View Standalone Graph" button to view more of the graph.
- When there are too many directly connected tasks, show better hint text with a "View Standalone Graph" button.
- Always show a "View Standalone Graph" option in the dropdown menu.
- Add a standalone view which works the same way but has a limit of 2,000.
- This view doesn't have "View Standalone Graph" links, since they'd just link back to the same page, but is basically the same otherwise.
- Increase the main page task limit from 100 to 200.
Test Plan:
Mobile View:
{F6210326}
Way too much stuff:
{F6210327}
New persistent link to the standalone page:
{F6210328}
Kind of too much stuff:
{F6210329}
Standalone view:
{F6210330}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D20164
Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.
If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.
This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".
Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:
- When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
- This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.
So: step forward, but more work to be done.
Test Plan: {F6211115}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20170
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.
Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20169
Summary:
Depends on D20175. Ref T12425. Ref T13253. Currently, importing commits can stall search index rebuilds, since index rebuilds use an older priority from before T11677 and weren't really updated for D16585.
In general, we'd like to complete all indexing tasks before continuing repository imports. A possible exception is if you rebuild an entire index with `bin/search index --rebuild-the-world`, but we could queue those at a separate lower priority if issues arise.
Test Plan: Ran some search indexing through the queue.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253, T12425
Differential Revision: https://secure.phabricator.com/D20177
Summary: Ref T13250. Some of these parameters may be NULL, and `alter()` is no longer happy about that.
Test Plan: Ran daemon tasks that happened to render some memes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20176
