Page MenuHomePhabricator

Modularize PhabricatorEditEngineConfigurationTransaction

Authored by amckinley on Jun 19 2019, 10:19 PM.



Ref T13319. Ref PHI1302. Migrate PhabricatorEditEngineConfigurationTransaction to modular transactions and add some additional transaction rendering to make these edits less opaque.

Test Plan

Hit all the form edit controllers, viewed resulting transaction timeline.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

To avoid the extreme case of ComComComJava-itis we could maybe just drop the word Configuration from these classes? No other type of EditEngine ... Transaction is ever likely to exist or make sense.

Couple of minor things inline but everything looks good to me.

This should also rebase above or below D20594 or something, but we can sort that out when title rendering comes in.

22 ↗(On Diff #49128)

(Per below, swap to renderValue() / renderOldValue() / etc.)

40 ↗(On Diff #49128)

This should be a "Required" error, not an "Invalid" error, I think.

23–24 ↗(On Diff #49128)

Here or in some future change, these should convert from "%s" + $old to %s + renderOldValue(), which has the nicer italic rendering in the web UI that reduce ambiguity when the value is a bunch of quotation marks.

To avoid the extreme case of ComComComJava-itis we could maybe just drop the word Configuration from these classes?

That's how I had it originally 😿 I need to write a Sublime plugin for renaming classes.

  • Renamed classes to remove Configuration
  • Created PhabricatorApplicationTransactionJSONDiffDetailView to reduce JSON-related boilerplate
  • Found one callsite to switch to PhabricatorApplicationTransactionJSONDiffDetailView
  • Implemented getTitle for the various JSON-encoded transactions
  • Requested fixes

Rename some variables for clarity.

One inline thing, but the rest of this looks correct as far as I can tell.


D20594 should rebase in here somehow still, I think -- the part where we look up the field label, like "Visible To" instead of "policy.view".

This revision is now accepted and ready to land.Jun 20 2019, 9:07 PM