Page MenuHomePhabricator

Transactions which modify EditEngine form default values don't include details about the old and new values
Open, NormalPublic

Description

See PHI1302. When you modify the default values of a custom form, you currently get opaque transactions like these:

Specializing these requires a lot of writing specialized language/rendering, but we can reasonably:

  • change the default rendering to show the raw values so that they're accessible, even if they aren't always human-readable;
  • provide a pathway toward specializing rendering; and
  • specialize rendering for the common/easy cases.

Related Objects

Event Timeline

epriestley triaged this task as Normal priority.Jun 19 2019, 7:54 PM
epriestley created this task.
aeiser added a subscriber: aeiser.Jun 19 2019, 8:05 PM

There are a few pieces here, since we have to thread the needle through many layers to get where we want to go.

Ideally, we'd probably like transactions to render the default value changes. For example, when you change the default "Assigned To" for a task, the rendering needs to know that the value is a user PHID and that the meaning of null is "No Owner / Unassigned". At first glance, ManiphestTaskOwnerTransaction seems like the right place to have that logic, since it has similar logic already in getTitle() and getTitleForFeed().

A downside to this is that it implies every single Transaction class needs to have a new rendering method, but I don't see an easy way around this unless we're willing to sacrifice the readability of the story fairly substantially.

We're starting here:

epriestley changed the default value for field "policy.view".

To get from a default value change to a transaction which can render, we need to:

  • Grab the current object, which is a PhabricatorEditEngineConfiguration.
  • Get the associated engine, a PhabricatorEditEngine.
  • Get the list of fields for the engine.
  • Look up the field that had its default value change using the field key.

This part is pretty straightforward, and lets us render this:

epriestley changed the default value for field "Visible To".

That's an improvement, at least.

We can also implement hasChangeDetails() and renderChangeDetails() and just dump the raw old/new values in as JSON, which will give us this:

epriestley changed the default value for field "Visible To". (Show Details)

...with a JSON value diff popup. That's not great for human readability, but pretty good for providing some kind of access to the values.

Beyond that, we need to get the EditType for the field, then try to generate a transaction from the EditType. To do that, we need an actual object (like a Task). There's no way to get a Task out of a TaskEditEngine right now, by design. I think EditEngine can safely get a new wrapper to generate a surrogate transaction for a given default change, though.

An adjacent issue is that the TYPE_DEFAULT transaction currently does not distinguish between "previously, there was no default" and "previously, the default was null". These are usually equivalent, but not always.

This rapidly descending into a big mess of concerns, and PhabricatorEditEngineConfigurationTransaction should probably be modularized before tackling them, so maybe the cheap-and-easy first step is a better starting point.

the (Show Details) would be a great first step for us - and cover our needs. Much like the logs for Herald Rules and other areas.