Page MenuHomePhabricator

Reimplement Countdown transactions using Modular Transaction framework
ClosedPublic

Authored by amckinley on Apr 12 2017, 11:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 10:41 PM
Unknown Object (File)
Sat, Mar 16, 1:20 AM
Unknown Object (File)
Sat, Mar 16, 1:15 AM
Unknown Object (File)
Wed, Mar 13, 5:48 PM
Unknown Object (File)
Thu, Mar 7, 5:52 AM
Unknown Object (File)
Feb 2 2024, 11:22 AM
Unknown Object (File)
Jan 7 2024, 7:48 AM
Unknown Object (File)
Dec 28 2023, 9:08 PM
Subscribers
Tokens
"Hungry Hippo" token, awarded by epriestley.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16444
Build 21885: Run Core Tests
Build 21884: arc lint + arc unit

Event Timeline

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

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

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

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

Screen Shot 2017-04-13 at 10.45.00 AM.png (870×2 px, 169 KB)
.
Confirmed that only description edits render details.

This revision was automatically updated to reflect the committed changes.