1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 21:26:14 +01:00
Commit graph

16076 commits

Author SHA1 Message Date
epriestley
1277db9452 When users hover over a column trigger menu, show a "preview" with the rules instead of a tooltip
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
2019-03-25 14:02:10 -07:00
epriestley
614f39b806 Show a trigger rule summary on the rule view page
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
2019-03-25 13:29:12 -07:00
epriestley
ff128e1b32 Write workboard trigger rules to the database
Summary: Ref T5474. Read and write trigger rules so users can actually edit them.

Test Plan: Added, modified, and removed trigger rules. Saved changes, used "Show Details" to review edits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20302
2019-03-25 13:26:21 -07:00
epriestley
567dea5449 Mostly make the editor UI for triggers work
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
2019-03-25 13:25:14 -07:00
epriestley
a5b3e33e3c Don't show workboard action previews if the action won't have any effect
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
2019-03-25 13:24:01 -07:00
epriestley
5dca1569b5 Preview the effects of a drag-and-drop operation on workboards
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
2019-03-25 13:22:56 -07:00
epriestley
149f8cc959 Hard code a "close task" action on every column Trigger
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
2019-03-25 13:21:55 -07:00
epriestley
916bf1a8f9 Allow triggers to be attached to and removed from workboard columns
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
2019-03-25 13:21:26 -07:00
epriestley
0204489a52 Modularize workboard column transactions
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
2019-03-25 13:14:25 -07:00
epriestley
252b6f2260 Provide basic scaffolding for workboard column triggers
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
2019-03-25 13:13:58 -07:00
epriestley
917fedafe6 Support exporting custom "Options/Select" fields to Excel/JSON/CSV/etc
Summary:
See <https://discourse.phabricator-community.org/t/exporting-custom-select-fields-as-part-of-query-exports/2466/>.

In JSON, export both the internal key and the visible value. For other formats, export the visible label.

Test Plan:
  - Added a custom options/select field.
  - Exported CSV, JSON, Text, got sensible output.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20316
2019-03-25 12:58:14 -07:00
epriestley
a5226366d3 Make notifications visually clearer, like Feed
Summary: See downstream <https://phabricator.wikimedia.org/T166358>. The notifications menu is missing some CSS to color and style values in stories like "renamed task from X to Y".

Test Plan:
Before:

{F6302123}

After:

{F6302122}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20310
2019-03-25 11:37:00 -07:00
epriestley
b081053e26 Don't show a "Manage" button in Legalpad if the user is signing a TOS document
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
2019-03-25 11:30:46 -07:00
epriestley
930cc7a6dd Generalize Legalpad validation logic for "Require Signature"
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
2019-03-25 11:26:13 -07:00
epriestley
e15b3dd3c6 When a repository is inactive, mark its handle as "closed"
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
2019-03-25 11:25:37 -07:00
epriestley
79e36dc7fa Explain the relationship between "Runnable" and "Restartable" more clearly in Build Plans
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
2019-03-25 11:24:04 -07:00
epriestley
c3563ca156 Correct use of the paging API in Phame
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
2019-03-25 10:55:21 -07:00
epriestley
1d73ae3b50 Fix two minor timezone display issues
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
2019-03-25 10:53:36 -07:00
epriestley
2ebe675ae5 Fix an unusual internal cursor in Conpherence
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
2019-03-25 10:52:58 -07:00
epriestley
7f90570636 When paging by Ferret "rank", page using "HAVING rank > ...", not "WHERE rank > ..."
Summary:
Ref T13091. The Ferret "rank" column is a function of the query text and looks something like `SELECT ..., 2 + 2 AS rank, ...`.

You can't apply conditions to this kind of dynamic column with a WHERE clause: you get a slightly unhelpful error like "column rank unknown in where clause". You must use HAVING:

```
mysql> SELECT 2 + 2 AS x WHERE x = 4;
ERROR 1054 (42S22): Unknown column 'x' in 'where clause'
mysql> SELECT 2 + 2 AS x HAVING x = 4;
+---+
| x |
+---+
| 4 |
+---+
1 row in set (0.00 sec)
```

Add a flag to paging column definitions to let them specify that they must be applied with HAVING, then apply the whole paging clause with HAVING if any column requires HAVING.

Test Plan:
  - In Maniphest, ran a fulltext search matching more than 100 results, ordered by "Relevance", then clicked "Next Page".
  - Before patch: query with `... WHERE rank > 123 OR ...` caused MySQL error because `rank` is not a WHERE-able column.
  - After patch: query builds as `... HAVING rank > 123 OR ...`, pages properly, no MySQL error.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13091

Differential Revision: https://secure.phabricator.com/D20298
2019-03-25 10:52:36 -07:00
epriestley
f047b90d93 Don't draw the task graph line image on devices by default
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
2019-03-25 10:52:11 -07:00
epriestley
b825570734 Fix transaction queries failing on "withIDs()" after clicking "Show Older"
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
2019-03-24 12:25:29 -07:00
epriestley
f0ae75c991 Fix an issue with "nextPage()" on worker trigger queries
Ref T13266. We never page these queries, and previously never reached the
"nextPage()" method. The call order changed recently and this method is now
reachable. For now, just no-op it rather than throwing.
2019-03-23 10:41:06 -07:00
epriestley
aae7bdbbdd Reference raw "title" row in Phriction paging, not "c.title"
Trivial mistake -- the query "SELECT x.y ..." selects a column named "y".
The other calls to "getRawRowProperty()" get this right.
2019-03-23 07:27:42 -07:00
epriestley
c16a8c7ab2 Fix a content join condition in Phriction
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.
2019-03-23 07:18:01 -07:00
epriestley
b90e02bec2 Select Ferret fulltext columns in results so fulltext queries work under UNION
Summary:
Ref T13091. In Differential, if you provide a query and "Sort by: Relevance", we build a query like this:

```
((SELECT revision.* FROM ... ORDER BY rank) UNION ALL (SELECT revision.* FROM ... ORDER BY rank)) ORDER BY rank
```

The internal "ORDER BY rank" is technically redundant (probably?), but doesn't hurt anything, and makes construction easier.

The problem is that the outer "ORDER BY rank" at the end, which attempts to order the results of the two parts of the UNION, can't actually order them, since `rank` wasn't selected.

(The column isn't actually "rank", which //is// selected -- it's the document modified/created subcolumns, which are not.)

To fix this, actually select the fulltext columns into the result set.

Test Plan:
  - Ran a non-empty fulltext query in Differential with "Bucket: Required Action" selected so the UNION construction fired.
  - Ran normal queries in Maniphest and global search.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13091

Differential Revision: https://secure.phabricator.com/D20297
2019-03-19 13:22:42 -07:00
epriestley
a8ba8217d8 Skip Ferret fulltext columns in "ORDER BY" if there's no fulltext query
Summary:
Ref T13091. If you "Order By: Relevance" but don't actually specify a query, we currently raise a bare exception.

This operation is sort of silly/pointless, but it seems like it's probably best to just return the results for the other constraints in the fallback order (usually, by ID). Alternatively, we could raise a non-bare exception here ("You need to provide a fulltext query to order by relevance.")

Test Plan: Queried tasks by relevance with no actual query text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13091

Differential Revision: https://secure.phabricator.com/D20296
2019-03-19 13:20:34 -07:00
epriestley
3940c8e1f4 Make the UI when you use an invalid cursor ("?after=19874189471232892") a little nicer
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
2019-03-19 13:04:07 -07:00
epriestley
18b444e427 When queries overheat, raise an exception
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
2019-03-19 13:02:59 -07:00
epriestley
8449c1793a Convert complex query subclasses to use internal cursors
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
2019-03-19 13:02:16 -07:00
epriestley
d4847c3eeb Convert simple query subclasses to use internal cursors
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
2019-03-19 13:00:27 -07:00
epriestley
1b0ef43910 Separate internal and external Query Cursors more cleanly, to fix pagination against broken objects
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
2019-03-19 12:59:58 -07:00
epriestley
a6fd8f0479 When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state
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
2019-03-18 15:20:45 -07:00
epriestley
b469a5134d Allow "SMTP" and "Sendmail" mailers to have "Message-ID" behavior configured in "cluster.mailers"
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
2019-03-16 12:35:55 -07:00
epriestley
492b03628f Fix a typo in Drydock "Land" operations
Summary: Misspelling.

Test Plan: Careful reading.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20290
2019-03-15 14:53:05 -07:00
epriestley
df53d72e79 Allow "Move Tasks to Column..." to prompt for MFA
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
2019-03-13 16:30:17 -07:00
epriestley
04f9e72cbd Don't subscribe bots implicitly when they act on objects, or when they are mentioned
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
2019-03-13 16:28:40 -07:00
epriestley
a6e17fb702 Improve workboard "Owner" grouping, add "Author" grouping and "Title" sort
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
2019-03-12 14:30:38 -07:00
epriestley
8d74492875 Make workboard sort-order inversions more clear by using "-1 * ..." instead of "(int)-(int)"
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
2019-03-12 14:06:18 -07:00
epriestley
a400d82932 Add "Group by Status" to Workboards
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
2019-03-12 13:55:54 -07:00
epriestley
03b7aca019 Implement "Sort by Points" on workboards
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
2019-03-12 13:54:18 -07:00
epriestley
c020f027bb Add an "Sort by Creation Date" filter to workboards and modularize remaining order behaviors
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
2019-03-12 13:46:49 -07:00
epriestley
804be81f5d Provide better UI feedback about cards that can't be dragged or edited
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
2019-03-12 13:43:46 -07:00
epriestley
74de153e59 Allow MFA task edits to go through on workboards
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
2019-03-12 13:42:33 -07:00
epriestley
1bdf446a80 Improve rendering of empty workboard columns in header views
Summary:
Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly.

When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing.

Test Plan: {F6264611}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20272
2019-03-12 13:40:20 -07:00
epriestley
21dd79b35a When creating or editing a card on a sorted/grouped workboard, adjust headers appropriately
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
2019-03-12 13:28:31 -07:00
epriestley
2fdab434fa Implement "Group by Owner" on Workboards
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
2019-03-12 13:25:55 -07:00
epriestley
9a8019d4a9 Modularize workboard column orders
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
2019-03-12 13:07:50 -07:00
epriestley
7d849afd16 Add a "WorkboardCardTemplate" class to make workboard client code easier to reason about
Summary:
Depends on D20266. Boards currently have several `whateverMap<cardPHID => stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`.

The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns).

Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20267
2019-03-12 13:01:52 -07:00
epriestley
4bad6bc42a Remove all readers/writers for task "subpriority"
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
2019-03-12 12:57:04 -07:00