Page MenuHomePhabricator

Modularize Dashboard transactions

Authored by epriestley on Apr 11 2019, 9:01 PM.



Depends on D20399. Ref T13272. I'm moving toward fixing all the "moving panels around on Dashboards breaks the entire world" problems.

On the way there, modularize Dashboard transactions.

Test Plan
  • Created a new Dashboard.
  • Edited all fiedls of a dashboard.
  • Archived/restored a dashboard.

Diff Detail

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

Event Timeline

epriestley created this revision.Apr 11 2019, 9:01 PM
epriestley requested review of this revision.Apr 11 2019, 9:03 PM
epriestley added inline comments.Apr 11 2019, 9:06 PM

This has no callsites.


This also has no callsites.

amckinley accepted this revision.Apr 11 2019, 10:05 PM
amckinley added inline comments.

I guess dashboards gained default icons at some point? The original implementation has a "set the icon to foo" case for the title.


These won't be very human readable. Do we care about strings like "@amckinley changed the layout mode from layout-mode-full to layout-mode-half-and-half"? Or is this somehow going through PhabricatorDashboardLayoutConfig::getLayoutModeSelectOptions() to get the human names?

Oh, and I see the old code didn't render anything for this transaction type anyway.


Same as above: dashboards have default titles now?

This revision is now accepted and ready to land.Apr 11 2019, 10:05 PM
epriestley added inline comments.Apr 11 2019, 10:15 PM

Briefly: they'll get a default old value in the next diff, which swaps us to EditEngine.

More context:

In old code, when you created an object for the first time, we wrote a bunch of transactions like this:

name: null -> "some name"
icon: null -> "some icon"
status: null -> "active"

...and so on. Then we had all the transactions do if ($old === null) { return null; }, except one of them which was chosen arbitrarily did if ($old === null) { return pht('%s created this thing.'); }.

That worked okay, mostly, but it had some problems:

  • Transactions needed a lot of special casing, and most transactions needed to no-op a pointless null case.
  • Some objects don't have an obvious "name" sort of transaction to render "created this object". Some objects can even be created with no transactions.
  • After custom forms, it muddied the water between "set value to X" and "changed value from form default X to new value Y". This distinction is sometimes important.

New code on EditEngine does this instead:

  • Initial transactions use the default/form value as the old value.
  • Initial transactions are all marked as "create" transactions.
  • Initial transactions get an additional "create" transaction in the transaction group.

The behavior is then mostly to hide "create" transactions from the UI, except in a handful of cases.

This fixes all the problems:

  • No more null special casing is required, since these create transactions are hidden by default.
  • The "create" transaction always does the "alincoln created this thing" rendering, so we don't have to worry about finding another transaction to stick that on.
  • Since the old value is more accurate, we can special case a few cases we care about (like changing default policies when creating an object) and render/emphasize them.

So new/updated code usually throws away all the null handling. The only downside to doing this is that old transactions (in the database) don't have the "create" flag, so throwing away the null handling can make them render a little weird. For common/long-lived objects like Tasks this would probably raise enough eyebrows that we still have null handling code, but for any lesser-used application no one has ever cared that some AlmanacProperty has a transaction from 2015 like alincoln changed the name of this property from "" to "x.y.z".. If we do hit this stuff, we can also migrate fairly safely.

As written, this code slightly worsens some rendering since this doesn't move us to EditEngine yet so we don't get all the new handling, but that's coming up in the next diff.


I'm going to do another pass on the rendering here after the swap to EditEngine and I fix all the add/remove/move panel transactions.

Oh, another fixed problem was that null couldn't be used as a real value. It can, now.

This revision was automatically updated to reflect the committed changes.