Page MenuHomePhabricator

Rebuild the bulk editor on SearchEngine
ClosedPublic

Authored by epriestley on Nov 30 2017, 3:28 PM.
Tags
None
Referenced Files
F13050288: D18806.id45278.diff
Fri, Apr 19, 2:43 AM
F13050287: D18806.id45121.diff
Fri, Apr 19, 2:43 AM
F13050286: D18806.id.diff
Fri, Apr 19, 2:43 AM
Unknown Object (File)
Thu, Apr 11, 8:30 AM
Unknown Object (File)
Thu, Apr 11, 4:04 AM
Unknown Object (File)
Sun, Mar 31, 3:12 AM
Unknown Object (File)
Sat, Mar 30, 5:31 AM
Unknown Object (File)
Mar 5 2024, 12:29 AM
Subscribers
None

Details

Summary

Depends on D18805. Ref T13025. Fixes T10268.

Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.

Also, split it into a base BulkEngine and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.

Also:

  • Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with .bat files, and newer stuff mostly uses "bulk".
  • When objects in the result set can't be edited because you don't have permission, show the status more clearly.

This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.

Test Plan

Screen Shot 2017-11-30 at 7.21.52 AM.png (866×1 px, 135 KB)

  • Bulk edited from Maniphest.
  • Bulk edited from a workboard (no more giant ?ids=.... in the URL).
  • Hit most of the error conditions, I think?
  • Clicked the "Cancel" button.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/maniphest/application/PhabricatorManiphestApplication.php
55

Do we care about breaking any existing bookmarks using the old scheme? I can't think of why you'd bookmark a bulk edit link (but after this diff lands, it would be a little more likely).

src/applications/maniphest/view/ManiphestTaskResultListView.php
258

And definitely no reason to worry about saved POST urls.

src/applications/transactions/bulk/PhabricatorBulkEngine.php
135–137

Hacky in the sense that we'll probably redo this when some other application needs to use this infra for making a bulk editor?

191–194

Ok, so in general this diff adds support for doing bulk edits from saved queries, even though in practice we only generate a saved query that holds the IDs of the currently selected tasks when someone hits the bulk edit button? Or is there UI support for going straight from an arbitrary saved query to the bulk edit flow?

292

And this all looks very Maniphest-specific. Needs a TODO?

This revision is now accepted and ready to land.Nov 30 2017, 11:26 PM
src/applications/maniphest/application/PhabricatorManiphestApplication.php
55

Yeah, I couldn't come up with a reason why you'd bookmark this.

I could maybe imagine that you might have some internal tool that redirects to /batch/?ids=123 or whatever, or a bookmarklet that mangles Workboards, but you just need to replace /batch/ with /bulk/ if you do so that didn't seem like a huge issue. I don't have any direct knowledge of anyone doing this.

src/applications/transactions/bulk/PhabricatorBulkEngine.php
135–137

The many layers of hacks here are:

  • buildApplicationCrumbsForEditEngine() itself is sort of a hack to just make crumb construction public. Maybe crumbs should just be public instead, or maybe there should be some other approach, but I'm not sure. I think there's a TODO on the method definition already.
  • ...ForEditEngine(), but this is BulkEngine, but I didn't want to double-hack and add a ForBulkEngine() since ForEditEngine() is kind of bad enough already.
  • The "Query" crumb should really say something like "Open Tasks" if you came in via a named query, but I punted because rendering the actual query name properly via SearchEngine is kind of a lot of code. Sometimes we don't have a real named query, and sometimes the cancel URI isn't a query at all. I'll probably let the subclass have some control over crumb construction so we can get ManiphestWorkboard XYZBulk Editor or something like that. The behavior is also maybe a little weird if you just come in with a list of IDs.

None of this is too egregious but I expect it'll get at least a bit better soon.

191–194

Yeah, this is accurate.

There's currently no UI support for query-based bulk edits other than typing in the URI, but I think I'm going to at least move a little bit in that direction. One thing we could possibly do is get rid of the task selection in /maniphest/ and just do it here instead, since now we have tools for it. That's nicer if we add bulk edits to other apps, since we don't have to put the whole bulk edit UI and click-to-select interaction into everything.

Also, right now, it's hard to bulk edit tons of stuff. You can sort of do it in Maniphest by setting the query limit to 9999999 and then using "Select All", but at some point the page will fail, and most other apps don't have "limit". So if we made this more SearchEngine/Query oriented that might make support in other applications a lot less invasive.

I'm also imagining changing it so it works like this, to work better with larger result sets:

  • If your query matches more than, say, 1K results, we just say "You're bulk editing more than 1,000 things. They will all be edited, but you can't individually add and remove items from this set since it would take too long to render the entire list into your browser."
  • Then we just pass the query itself to the background task, and it spools up individual tasks for every matching object.

That would let the bulk editor operate on huge result sets without hitting scale issues. I'm not sure that's super valuable, but it's nice to at least have a reasonable plan for when someone does want to edit 1M tasks to add a disclaimer that Legal says is 100% necessary or whatever.

292

This is the part I haven't touched yet that's up next. It's, uh, TODO'd in my head. 🐑

This revision was automatically updated to reflect the committed changes.