Page MenuHomePhabricator

Modularize PhabricatorEditEngineConfigurationTransaction
ClosedPublic

Authored by amckinley on Jun 19 2019, 10:19 PM.
Tags
None
Referenced Files
F12840881: D20595.id49143.diff
Thu, Mar 28, 8:49 PM
F12838159: D20595.id49128.diff
Thu, Mar 28, 5:55 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Subscribers

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
Branch
modularize-editor-xaction (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 23007
Build 31583: Run Core Tests
Build 31582: arc lint + arc unit

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.

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.

  • 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.

src/applications/transactions/xaction/PhabricatorEditEngineDefaultTransaction.php
19–23

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