Page MenuHomePhabricator

Maniphest: Allow restricting custom fields to subtypes
AbandonedPublic

Authored by epriestley on Apr 1 2017, 4:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 12:35 PM
Unknown Object (File)
Sat, Mar 16, 9:27 AM
Unknown Object (File)
Thu, Mar 14, 10:48 AM
Unknown Object (File)
Thu, Mar 14, 10:48 AM
Unknown Object (File)
Thu, Mar 14, 10:14 AM
Unknown Object (File)
Thu, Mar 14, 9:14 AM
Unknown Object (File)
Fri, Mar 1, 2:39 AM
Unknown Object (File)
Wed, Feb 28, 2:43 AM
Tokens
"Like" token, awarded by benwick.

Details

Reviewers
fooishbar
Group Reviewers
Blessed Reviewers
Summary

Add a new 'subtypes' option to custom fields, allowing them to be restricted to subtypes, when used on an object supporting subtypes.

Currently only implemented in Maniphest, which is a mistake really; should be generic.

An even bigger mistake is the commented-out bit in transaction validation. There are two possible ways of doing this, and I'm not sure which would be better (than the other; either would be better than this).

First, we could put a first pass in which calculated the final/target object subtype by sweeping the transaction list before they were validated, and storing that somewhere.

Alternately, we could put that sweep in the custom-field transaction validation, looking for all subtype transactions (applied on top of the initial value) when validating custom-field transactions. But this could be a bit slow for many custom-field transactions, which is presumably exactly when you'd want this.

Test Plan
  • Add new task subtype with create/edit forms
  • Add new Maniphest custom field, restricted to new subtype, marked as required
  • New field not shown in old subtype forms, and no 'required field' validation errors seen
  • New field shown in new subtype forms, and 'required field' validation errors thrown if empty

Diff Detail

Repository
rP Phabricator
Branch
maniphest-subtypes (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16247
Build 21586: Run Core Tests
Build 21585: arc lint + arc unit

Event Timeline

Per commit message, this is not at all suitable for merging in its current form.

Happy to discuss usecases (presumably in T12314?) as well.

I'm generally leaning toward adding subtypes being a reasonable idea, but was thinking that maybe it should look like this:

"subtypes": {
  "default": {
    "required": true
  },
  "plant": {
    "disabled": true
  },
  "animal": {
    "disabled": true
  },
  "security":  {
    "caption": "..."
  }
}

Basically, let each subtype override keys (other than type, probably search/fulltext since we always need to index, and maybe some special keys like description, options, and copy which might be too much of a pain to make work).

Some realistic-seeming use cases this would resolve (some of which this change resolves as-written, too) are:

  • When adding a field, you might not want it on every subtype by default. You could disable it by default and selectively enable it for subtypes.
  • Some fields might be required only for certain subtypes, but useful to make optional on other subtypes.
  • You could change default field values per subtype a little more easily than by editing forms.
  • You could relabel, recaption, change placeholder text, or provide different instructions per-subtype.

I do think we get most of the useful benefits by just enabling or disabling the field per-subtype (the other stuff is mostly just nice-to-have fluff), but going a step further doesn't necessarily seem more difficult, while it gives us much more flexibility.

If we pursued this approach, I think we end up starting by making PhabricatorStandardCustomField::buildStandardFields() aware of PhabricatorEditEngineSubtypeInterface? That is, rather than trying to apply subtyping at all the places where subtyping matters, try to apply it at the source instead, so we generate a subtype-aware field list in the first place. Then we should get correct behavior everywhere more or less for free, and theoretically only need one if ($obj instanceof PhabricatorEditEngineSubtypeInterface) { ... } clause in the codebase to do it.

If this works, we should never reach any validate...() methods for disabled subtypes because they'll be filtered out long before we get to the heart of transaction application.

This is probably a little bit tricky because, e.g., search and fulltext always need to be active if a field is enabled for any subtype, and some other roles probably do too ("APPLICATIONSEARCH", maybe "CONDUIT" -- but that might be obsolete, probably "HERALD"). So disabling a field might really mean disabling it for most roles, but leaving it active for the "global" roles.

So I'd guess the basic shape of this change starts with:

  • Make PhabricatorStandardCustomField::buildStandardFields() subtype-aware in how it handles configuration, having it override global configuration with subtype-specific configuration.
  • When a field is disabled for the current subtype, but enabled for some subtype, set a narrow disable flag on it like setDisabledForActiveSubtype(true).
  • In PhabricatorCustomField->shouldEnableForRole(), respect this new flag for the "local" roles (like edit and view) but not for the "global" roles (like "APPLICATIONSEARCH"). Some of these cases probably require nuance.

Some of the "roles" are probably also obsolete, or may not make nearly as much sense in a post-EditEngine world as they did in a pre-EditEngine world, but these probably aren't too hard to sort out.

@epriestley Thanks so much for the detailed guidance! Sorry that my turnaround time hasn't quite justified the work you put into this: my time slots are quite fleeting, as I only pretend to be a web guy from time to time.

Basically, let each subtype override keys (other than type, probably search/fulltext since we always need to index, and maybe some special keys like description, options, and copy which might be too much of a pain to make work).

Some realistic-seeming use cases this would resolve (some of which this change resolves as-written, too) are:

  • When adding a field, you might not want it on every subtype by default. You could disable it by default and selectively enable it for subtypes.
  • Some fields might be required only for certain subtypes, but useful to make optional on other subtypes.

This is my justification, where we're starting to explore subtypes for our internal deployments. Examples of this include using Maniphest as a task queue for purchasing requests (perhaps a stopgap to Nuance), to apply more mandatory structure to bug reports than either feature requests or tasks filed by developers for team-internal planning purposes (perhaps again filling Nuance's vacuum), and finally to separate 'epic'-type umbrella tasks used for grouping purposes only from the rest.

  • You could change default field values per subtype a little more easily than by editing forms
  • You could relabel, recaption, change placeholder text, or provide different instructions per-subtype.

The default values are interesting; I'm not sure off the top of my head how difficult the integration with forms would be, but will have a go. The labels/captions/etc almost seem like they deserve different fields, but on the other hand there doesn't seem much reason to generalise all the values but exclude some.

I do think we get most of the useful benefits by just enabling or disabling the field per-subtype (the other stuff is mostly just nice-to-have fluff), but going a step further doesn't necessarily seem more difficult, while it gives us much more flexibility.

If we pursued this approach, [...]

Makes sense, and certainly a lot cleaner than the first cut; I'll have a go over the next few days and update this revision. Thanks again for your help!

@epriestley Noticed you'd been doing proximate work; I'm not planning to push this further forward, so please feel free to commandeer this revision.

@epriestley: I'm thinking about changing this to make it look more like your example (quoted below) as I think that would make custom fields and subtypes quite a lot more useful. Is this something that you would consider merging in the upstream?

I'm generally leaning toward adding subtypes being a reasonable idea, but was thinking that maybe it should look like this:

"subtypes": {
  "default": {
    "required": true
  },
  "plant": {
    "disabled": true
  },
  "animal": {
    "disabled": true
  },
  "security":  {
    "caption": "..."
  }
}

In theory, probably. In practice, anything not coming through a support pact is probably going to sit in queue for an eternity nowadays.

In theory, probably. In practice, anything not coming through a support pact is probably going to sit in queue for an eternity nowadays.

That's unfortunate. I may work on it anyway as maintaining something like this downstream isn't terribly problematic for me. BTW: Thanks for the quick response :)

epriestley abandoned this revision.
epriestley added a reviewer: fooishbar.

Yeah, more-or-less obsoleted by D20161, or we're at least on a path toward obsoletion.