Page MenuHomePhabricator

Reimplement Countdown transactions using Modular Transaction framework
ClosedPublic

Authored by amckinley on Apr 12 2017, 11:45 PM.

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.Apr 12 2017, 11:45 PM

Actual test plan to come after @epriestley confirms that this is more-or-less on the right track.

epriestley edited edge metadata.Apr 12 2017, 11:50 PM

Couple of really minor inlines but this all looks great to me.

src/applications/countdown/editor/PhabricatorCountdownEditor.php
16–17

You should be able to throw these away -- modular transactions figure these out automatically.

src/applications/countdown/xaction/PhabricatorCountdownEpochTransaction.php
10

Just put an (int)$object->... cast here...

19–21

...and then you should be able to get rid of this.

52

This one should be newRequiredError() I think.

The difference between "required" and "invalid" is that some edits are allowed to go through even if required fields are missing. For example, you can award a token or subscribe to a countdown even if it somehow has missing information.

So generally if a field is empty, but shouldn't be empty, use newRequiredError(). If it has an improper value, use newInvalidError().

src/applications/countdown/xaction/PhabricatorCountdownTitleTransaction.php
19

You can omit quotes in ModularTransactions -- we used to have to quote everything, but renderOldValue() and renderNewValue() now give these a nice italic style. This also disambiguates cases where someone renames something to "quote quote '" quotes " double quote because they are oh-so-clever.

37–48

(These are right.)

amckinley marked 6 inline comments as done.Apr 13 2017, 1:19 AM
amckinley updated this revision to Diff 42500.Apr 13 2017, 1:20 AM
  • requested changes
Harbormaster completed remote builds in B16431: Diff 42500.
epriestley accepted this revision.Apr 13 2017, 2:15 AM

Test plan could use slightly fewer owls but this looks good to me and I couldn't figure out any way to break it locally. 🐦

src/applications/countdown/xaction/PhabricatorCountdownEpochTransaction.php
40–42

I think you can get rid of this -- this kind of transaction will never generate a separate mail section because it doesn't implement newChangeDetailView(), since it isn't a big block of text that needs a complictated diff to show changes.

64–66

(No callers?)

This revision is now accepted and ready to land.Apr 13 2017, 2:15 AM
amckinley updated this revision to Diff 42513.Apr 13 2017, 5:46 PM

Tested creating a Countdown and modifying all three fields: title, description, end time. See

.
Confirmed that only description edits render details.

amckinley marked 2 inline comments as done.Apr 13 2017, 5:53 PM
This revision was automatically updated to reflect the committed changes.