Page MenuHomePhabricator

Maniphest - allow for searching for tasks based on dependency relationships
ClosedPublic

Authored by btrahan on Jan 9 2015, 11:53 PM.

Details

Summary

Fixes T5352. This is very useful for finding things that should be easy to do ("not blocked") as well as things that are important to do ("blocking"). I have wanted to check out the latter case in our installation, though no promises on what I would end up actually doing from that search result list. =D

I also think supporting something like T6638 is reasonable but the UI seems trickier to me; its some sort of task tokenizer, which I don't think we've done before?

Test Plan

toggled various search options and got reasonable results. When i clicked conflicting things like "blocking" and "not blocking" verified it was like I had not clicked anything at all.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

btrahan updated this revision to Diff 27166.Jan 9 2015, 11:53 PM
btrahan retitled this revision from to Maniphest - allow for searching for tasks based on dependency relationships.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
chad awarded a token.Jan 12 2015, 4:44 PM
epriestley requested changes to this revision.Jan 12 2015, 6:17 PM
epriestley edited edge metadata.

Four points...


First, on this...

T6638: Be able to find tasks by "is a blocker to task Y" or "is blocked by task Y" in the search interface

I kind of don't think we should put this on the search UI. Instead, having a link or icon or button on the task seems better, e.g.:

Blocked by: T123
            T234
            T456
            (View these tasks as a search result set...)

The idea being that the only real use case is to take them to the bulk editor, I think.

We could possibly even just "(Edit all these tasks...)" and give you an option to get back to the search result view if you want.

That would be kind of heavy for a very rare need, but maybe we can come up with some reasonable treatment eventually.


Second, consider this API instead:

->withBlockingTasks(true | false | null)
->withBlockedTasks(true | false | null)

...where true means "find only records with this attribute", false means "find only records without this attribute", and null means "ignore this attribute".

This prevents impossible queries like DEPENDENCY_BLOCKING + DEPENDENCY_NOT_BLOCKING and makes the eventual Conduit API easier to use. The UI would be two select dropdowns instead of 4 checkboxes. I've been doing this with recent APIs in this vein and it seems to work well -- I think the only drawback is that true vs false vs null feels slightly magical.


Third, we have a handful of open tasks which are all "add some fairly easy to implement but not-especially-valuable filter for some rare-ish use case"; I consider this one of them. Others include:

I'm hesitant on all of these because I think adding more rarely-used fields to the query interface becomes a net loss at some point.

T4100 will let us collapse some of these fields, but the desire to add limitless numbers of new fields seems irresistible in the long run.

I wonder if we should make the fields themselves editable and allow users to save configurations of fields -- so you hide the fields you don't use often, reorder the other ones, and save the result as your new search template. On the one hand this would let us add hundreds of query fields without degrading the product much; on the other hand this is a major amount of both UI complexity and technical complexity.


Finally, I caught some inlines.

src/applications/maniphest/query/ManiphestTaskQuery.php
579–580

Consider making these methods (shouldJoinBlockingDependencies()) instead of properties so they're harder to misuse (e.g., it prevents them from being accessed before they are initialized, if this code gets moved around).

623

Stray print_r().

848–852

This needs to be updated for the row-matching joins, or we'll get the same tasks back multiple times if, e.g., a task has 50 dependencies.

This likely shakes out correctly in the long run for tasks with fewer than 100 dependencies, but at the cost of efficiency (we incidentally de-duplicate in the client instead of in MySQL).

This revision now requires changes to proceed.Jan 12 2015, 6:17 PM

There should definitely be a "show me every single search option and let me specify every single search option" page, and we should allow you to search on as many criteria as possible.

For the concern that "adding more rarely-used fields to the query interface becomes a net loss at some point" - why don't we just keep adding fields until we hit that point? When we hit that point, I think we have an easy fix in simple deciding which things are "basic" versus "advanced", then have two sections on the form with the "advanced" collapsed by default.

I disagree the main value in this is to do bulk edits. I think the main use case might be that but the main value is in quickly surfacing tasks that are blocking other tasks.

At least a user, I think we hit that point quite a while ago.

I'm pretty sure I've never used these fields to perform a query as a user of the software:

  • Show unassigned tasks.
  • Show only tasks with no projects.
  • In All Projects
  • Not In Projects
  • In Users' Projects
  • Subscribers
  • Contains Words
  • Task IDs
  • Created After
  • Created Before
  • Updated After
  • Updated Before

I don't expect to ever use these "blocked/blocking" fields, or any of the other fields requested in the tasks above, either.

I disagree the main value in this is to do bulk edits.

Sorry, I mean of T6638 specifically. I don't think there's any other use case for finding "tasks blocked by task X".

There are plenty of other use cases for finding "tasks blocked by anything".

btrahan updated this revision to Diff 27272.Jan 12 2015, 8:53 PM
btrahan edited edge metadata.

Addresses technical feedback, including changing the API to two withX methods. Will address product concerns via T6943.

epriestley accepted this revision.Jan 12 2015, 8:54 PM
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jan 12 2015, 8:54 PM
This revision was automatically updated to reflect the committed changes.