Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T12524: Modernize Countdown
- Commits
- rP9d56a3d86ec7: Reimplement Countdown transactions using Modular Transaction framework
Diff Detail
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
Comment Actions
Actual test plan to come after @epriestley confirms that this is more-or-less on the right track.
Comment Actions
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.) | |
Comment Actions
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?) | |
Comment Actions
.
Confirmed that only description edits render details.
Tested creating a Countdown and modifying all three fields: title, description, end time. See
Confirmed that only description edits render details.