Summary:
In some cases, we show a limited number of one type of object somewhere else, like "Recent Such-And-Such" or "Herald Rules Which Use This" or whatever.
We don't do a very good job of communicating that these are partial lists, or how to see all the results. Usually there's a button in the upper right, which is fine, but this could be better.
Add an explicit "more stuff" button that shows up where a pager would appear and makes it clear that (a) the list is partial; and (b) you can click the button to see everything.
Test Plan: {F6302793}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20315
Summary:
Ref T5474. In 99% of cases, a separate "archived/active" status for triggers probably doesn't make much sense: there's not much reason to ever disable/archive a trigger explcitly, and the archival rule is really just "is this trigger used by anything?".
(The one reason I can think of to disable a trigger manually is because you want to put something in a column and skip trigger rules, but you can already do this from the task detail page anyway, and disabling the trigger globally is a bad way to accomplish this if it's in use by other columns.)
Instead of adding a separate "status", just track how many columns a trigger is used by and consider it "inactive" if it is not used by any active columns.
Test Plan: This is slightly hard to test exhaustively since you can't share a trigger across multiple columns right now, but: rebuild indexes, poked around the trigger list and trigger details, added/removed triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20308
Summary:
Ref T5474. Allow columns to play a sound when tasks are dropped.
This is a little tricky because Safari has changed somewhat recently to require some gymnastics to play sounds when the user didn't explicitly click something. Preloading the sound on the first mouse interaction, then playing and immediately pausing it seems to work, though.
Test Plan: Added a trigger with 5 sounds. In Safari, Chrome, and Firefox, dropped a card into the column. In all browsers, heard a nice sequence of 5 sounds played one after the other.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20306
Summary:
Ref T5474. The first rough cut of triggers showed some of the trigger rules in a tooltip when you hover over the "add/remove" trigger menu.
This isn't great since we don't have much room and it's a bit finnicky / hard to read.
Since we have a better way to show effects now in the drop preview, just use that instead. When you hover over the trigger menu, preview the trigger in the "drop effect" element, with a "Trigger: such-and-such" header.
Test Plan:
- This is pretty tough to screenshot.
- Hovered over menu, got a sensible preview of the trigger effects.
- Dragged a card over the menu, no preview.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20304
Summary: Ref T5474. When you view the main page for a rule, show what the rule does before you actually edit it.
Test Plan:
Viewed a real trigger, then faked invalid/unknown rules:
{F6300211}
{F6300212}
{F6300213}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20303
Summary:
Ref T5474. This provides a Herald-like UI for editing workboard trigger rules.
This probably has some missing pieces and doesn't actually save anything to the database yet, but the basics at least roughly work.
Test Plan: {F6299886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20301
Summary:
Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing.
For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions.
Test Plan:
Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases.
Drags which actually cause effects still show the effects correctly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335
Differential Revision: https://secure.phabricator.com/D20300
Summary:
Ref T10335. Ref T5474. When you drag-and-drop a card on a workboard, show a UI hint which lists all the things that the operation will do.
This shows: column moves; changes because of dragging a card to a different header; and changes which will be caused by triggers.
Not implemented here:
- Actions are currently shown even if they have no effect. For example, if you drag a "Normal" task to a different column, it says "Change priority to Normal.". I plan to hide actions which have no effect, but figuring this out is a little bit tricky.
- I'd like to make "trigger effects" vs "non-trigger effects" a little more clear in the future, probably.
Test Plan:
Dragged stuff between columns and headers, and into columns with triggers. Got appropriate preview text hints previewing what the action would do in the UI.
(This is tricky to take a screenshot of since it only shows up while the mouse cursor is down.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335, T5474
Differential Revision: https://secure.phabricator.com/D20299
Summary: Depends on D20287. Ref T5474. This hard-codes a storage value for every trigger, with a "Change status to <default closed status>" rule and two bogus rules. Rules may now apply transactions when cards are dropped.
Test Plan: Dragged cards to a column with a trigger, saw them close.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20288
Summary:
Depends on D20286. Ref T5474. Attaches triggers to columns and makes "Remove Trigger" work.
(There's no "pick an existing named trigger from a list" UI yet, but I plan to add that at some point.)
Test Plan: Attached and removed triggers, saw column UI update appropriately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20287
Summary: Depends on D20279. Ref T5474. Modernize these transactions before I add a new "TriggerTransaction" for setting triggers.
Test Plan: Created a column. Edited a column name and point limit. Hid and un-hid a column. Grepped for removed symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20286
Summary:
Depends on D20278. Ref T5474. This change creates some new empty objects that do nothing, and some new views for looking at those objects. There's no actual useful behavior yet.
The "Edit" controller is custom instead of being driven by "EditEngine" because I expect it to be a Herald-style "add new rules" UI, and EditEngine isn't a clean match for those today (although maybe I'll try to move it over).
The general idea here is:
- Triggers are "real" objects with a real PHID.
- Each trigger has a name and a collection of rules, like "Change status to: X" or "Play sound: Y".
- Each column may be bound to a trigger.
- Multiple columns may share the same trigger.
- Later UI refinements will make the cases around "copy trigger" vs "reference the same trigger" vs "create a new ad-hoc trigger" more clear.
- Triggers have their own edit policy.
- Triggers are always world-visible, like Herald rules.
Test Plan: Poked around, created some empty trigger objects, and nothing exploded. This doesn't actually do anything useful yet since triggers can't have any rule behavior and columns can't actually be bound to triggers.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20279
Summary:
When a TOS-like Legalpad document is marked "Require this document to use Phabricator", the login prompt shows a "Manage" button, but that button doesn't work.
When we're presenting a document as a session gate, don't show "Manage".
Test Plan: Viewed a required document during a session gate (no "Manage" button) and normally (saw "Manage" button).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20312
Summary:
See downstream <https://phabricator.wikimedia.org/T208254>.
I can't actually reproduce any issue here (we only show this field when creating a document, and only if the viewer is an administrator), so maybe this relied on some changes or was originally reported against older code.
Regardless, the validation isn't quite right: it requires administrator privileges to apply this transaction at all, but should only require administrator privileges to change the value.
Test Plan:
Edited Legalpad documents as an administrator and non-administrator before and after the change, with and without signatures being required.
Couldn't reproduce the original issue, but this version is generally more correct/robust.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20311
Summary: See downstream <https://phabricator.wikimedia.org/T182232>. We currently don't mark repository handles as closed.
Test Plan: Mentioned two repositories with `R1` (active) and `R2` (inactive). After patch, saw `R2` visually indicated as closed/inactive.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20309
Summary: See PHI1153. The "Runnable" and "Restartable" behaviors interact (to click "restart", you must be able to run the build AND it must be restartable). Make this more clear.
Test Plan: {F6301739}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20307
Summary: Ref T13266. This callsite is using the older API; swap it to use pagers.
Test Plan: Viewed a Phame blog post with siblings, saw the previous/next posts linked.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: nicolast
Maniphest Tasks: T13263, T13266
Differential Revision: https://secure.phabricator.com/D20319
Summary:
Ref T13263. Two minor issues:
- The "reconcile" dialog shows the wrong sign because JS signs differ from normal signs (for example, PST or PDT or whatever we're in right now is shown as "UTC+7", but should be "UTC-7").
- The big dropdown of possible timezones lumps "UTC+X:30" timezones into "UTC+X".
Test Plan:
- Reconciled "America/Nome", saw negative UTC offsets for "America/Nome" and "America/Los_Angeles" (previously: improperly positive).
- Viewed the big timzone list, saw ":30" and ":45" timezones grouped/labeled more accurately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13263
Differential Revision: https://secure.phabricator.com/D20314
Summary:
See <https://discourse.phabricator-community.org/t/error-when-sending-a-message-chat-room/2548>.
Conpherence calls `setAfterID()` and `setBeforeID()` directly on a subquery, but these methods no longer exist.
Use a pager instead. This code probably shouldn't exist (we should use some other approach to fetch this data in most cases) but that's a larger change.
Test Plan: Sent messages in a Conpherence thread. Before: fatal; after: success. Viewed the Conphrence menu, loaded threads, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20318
Summary:
See downstream <https://phabricator.wikimedia.org/T210482>.
On mobile, the task graph can take up most of the screen. Hide it on devices. Keep it on the standalone view if you're really dedicated and willing to rotate your phone or whatever to see the lines.
Test Plan: Dragged window real narrow, saw graph hide.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20313
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-on-show-older-changes/2545/>.
Before T13266, this query got away without having real paging because it used simple ID paging only and results are never actually hidden (today, you can always see all transactions on an object).
Provide `withIDs()` so the new, slightly stricter paging works.
Test Plan: On an object with "Show Older" in the transaction record, clicked the link. Before: exception in paging code (see Discourse link above). After: transactions loaded cleanly.
Reviewers: amckinley, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D20317
After the cursor changes, we may fatal on pages with a large number of children
because "c.title" is not a selected column. We currently join the "content"
table if "updated" is part of the order vector, but not if "title" is part of
the order vector. This isn't right: "updated" is on the primary table, and only
"content" is on the joined table.
Summary:
Ref T13259. Currently, visiting a page that executes a query with an invalid cursor raises a bare exception that escapes to top level.
Catch this a little sooner and tailor the page a bit.
Test Plan: Visited `/maniphest/?after=335234234223`, saw a nicer exception page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20295
Summary:
Ref T13259. Currently, queries set a flag and return a partial result set when they overheat. This is mostly okay:
- It's very unusual for queries to overheat if they don't have a real viewer.
- Overheating is rare in general.
- In most cases where queries can overheat, the context is a SearchEngine UI, which handles this properly.
In T13259, we hit a case where a query with an omnipotent viewer can overheat: if you have more than 1,000 consecutive commits in the database with invalid `repositoryID` values, we'll overheat and bail out. This is pretty bad, since we don't process everything.
Change this beahvior:
- Throw by default, so this stuff doesn't slip through the cracks.
- Handle the SearchEngine case explicitly ("it's okay to overheat, we'll handle it").
- Make `QueryIterator` disable overheating behavior: if we're iterating over all objects, we want to hit the whole table even if most of it is garbage.
There are some cases where this might cause new exception behavior that we don't necessarily want. For example, in Owners, each package shows "recent commits in this package". If you can't see the first 1,000 recent commits, you'd previously get a slow page with no results. Now you'll probably get an exception.
If these crop up, I think the best approach for now is to deal with them on a case-by-case basis and see how far we get. In the "Owners" case, it might be good to query by repositories you can see first, then query by commits in the package in those repositories. That should give us a better outcome than any generic behavior we could implement.
Test Plan:
- Added 100000 to all repositoryID values for commits on my local install.
- Before making changes, ran `bin/repository rebuild-identities --all --trace`. Saw the script process 1,000 rows and exit silently.
- Applied the first part ("throw by default") and ran `bin/repository rebuild-identities`. Saw the script process 1,000 rows, then raise an exception.
- Applied the second part ("disable for queryiterator") and ran the script again. Saw the script process all 15,000 rows without issues (although none are valid and none actually load).
- Viewed Diffusion, saw appropriate NUX / "overheated" UIs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20294
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:
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
