Page MenuHomePhabricator

Define bulk edits in terms of EditEngine, not hard-coded ad-hoc definitions
ClosedPublic

Authored by epriestley on Jan 10 2018, 9:12 PM.
Tags
None
Referenced Files
F13050310: D18863.diff
Fri, Apr 19, 2:44 AM
Unknown Object (File)
Thu, Apr 11, 10:46 AM
Unknown Object (File)
Wed, Apr 10, 9:27 PM
Unknown Object (File)
Sat, Mar 30, 5:40 PM
Unknown Object (File)
Mar 17 2024, 12:06 AM
Unknown Object (File)
Feb 15 2024, 3:44 PM
Unknown Object (File)
Feb 9 2024, 8:26 AM
Unknown Object (File)
Feb 3 2024, 5:29 PM
Subscribers
None

Details

Summary

Depends on D18862. See PHI173. Ref T13025. Fixes T10005. This redefines bulk edits in terms of EditEngine fields, rather than hard-coding the whole thing.

Only text fields -- and, specifically, only the "Title" field -- are supported after this change. Followup changes will add more bulk edit parameter types and broader field support.

However, the title field now works without any Maniphest-specific code, outside of the small amount of binding code in the ManiphestBulkEditor subclass.

Test Plan

Used the bulk edit workflow to change the titles of tasks.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are some pieces here I don't really understand; will come back with lots of questions soon.

src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php
76–78 ↗(On Diff #45246)

Wait, why is this implementation different from PhabricatorEditEngineBulkJobType->buildTransactions() in D18862?

D18862 was basically just a guess at what the implementation should look like, I didn't wire it up until this change. It could really be part of this change, I split them mostly because I wrote that first (before doing any of the other changes here), then backed off and took a different approach, but it was still sitting in a separate commit locally. Moving those changes to D18862 or merging D18862 into this would probably be a little more "pure" in some sense, the changes were just split into somewhat-reasonable chunks on disk already so I left them like that.

When it came time to actually wire it up, making the format look more like the Conduit format (type, value) seemed cleaner than making the format look like the internal format (type, old, new) since most of the other logic ended up very similar to Conduit, which I think is good. That is, "bulk edit" is basically a UI skin on top of "use the API to apply a lot of edits", which seems like the right implementation.

To guess at which parts might not be very obvious:

Edit Types: Each field generates one or more "edit types". From this diff, this probably doesn't make much sense or seem necessary, since "text" fields generate only one edit type. The problem this layer solves is that some fields, like "Subscribers" and "Tags", need to generate several different possible actions, like "Add subscribers", "Remove subscribers", and "Set subscribers to".

So a field like "Subscribers" generates one field in the form UI, but three possible edits via Conduit (subscribers.add, subscribers.remove, and subscribers.set). It also needs to generate three bulk edit actions: "Add subscribers", "Remove subscribers", and "Set subscribers to". Most fields don't need to do this and just generate one "edit type" which is the same as whatever the field does normally, so there's a lot of boilerplate here which doesn't actually do anything, but it will let us generate multiple actions for PHID list fields and other types of complex fields down the road.

Bulk Parameter Types: This introduces "bulk parameter types". Currently, we read incoming data from Conduit with "conduit parameter types", and incoming data from web requests with "HTTP parameter types". These classes are responsible for parsing transactions submitted via Conduit or ?title=x&reviewers=alincoln,gwashington HTTP parameters submitted via GET query string or by posting edit forms.

Although bulk edits happen through web form, they can't exactly use the existing HTTP parameter type code because they look a bit different on the wire. We also need something to specify what the control for each action looks like (for example, "Add subscribers..." needs to have a tokenizer) and neither HTTP parameter types nor Conduit parameter types do this. For normal forms, this is controlled by the edit field itself, but since each edit field may generate several possible bulk edits I wanted to make sure it was possible for them to have different types. A practical issue is that the bulk editor also needs to build all the controls in Javascript, while all the current forms are rendered server side.

This part of the implementation is generally sort of a cross between how the Conduit code works and how the "Comment Action" code works (where you "Add action..." to a comment, like "Accept Revision" or "Change Subscribers"). We can't reuse either one exactly (Conduit doesn't have any associated UI, and the actions available to bulk edit are different than the actions available when commenting) but it tries to reuse as much stuff as possible (like actually rendering the controls on the client with the PHUIX stuff, like "Comment Actions" do) and to generally look like the Conduit stuff.

Basically, this will just end up being a thin indirection layer which points at other pieces to actually do the heavy lifting (like rendering JS controls), but gives us the flexibility to make bulk actions differ from Conduit transactions, comment actions, and form actions so we can handle cases where we want the available actions to be more/less/different than other workflows.

  • Push the worker changes back to D18862.
  • Actually delete the old bulk edit Javascript behavior (the primary JS change here is a move + delete lots of code, not new code).
This revision is now accepted and ready to land.Jan 17 2018, 6:24 PM
This revision was automatically updated to reflect the committed changes.