Page MenuHomePhabricator

Modularize PhabricatorEditEngineConfigurationTransaction
ClosedPublic

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

Details

Summary

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

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Jun 19 2019, 10:19 PM
amckinley requested review of this revision.Jun 19 2019, 10:21 PM

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.

src/applications/transactions/xaction/PhabricatorEditEngineConfigurationNameTransaction.php
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.

src/applications/transactions/xaction/PhabricatorEditEngineConfigurationSubtypeTransaction.php
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.

amckinley marked 3 inline comments as done.Jun 20 2019, 8:12 PM
amckinley updated this revision to Diff 49133.Jun 20 2019, 8:25 PM
  • 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
amckinley edited the summary of this revision. (Show Details)Jun 20 2019, 8:28 PM
amckinley updated this revision to Diff 49135.Jun 20 2019, 9:04 PM

Rename some variables for clarity.

epriestley accepted this revision.Jun 20 2019, 9:07 PM

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

src/applications/transactions/xaction/PhabricatorEditEngineDefaultTransaction.php
20–24

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
amckinley updated this revision to Diff 49143.Jun 20 2019, 10:03 PM

Pick up changes from D20594.