Ref T9838.
Add a Properties field to Revision, and update a wasAcceptedBeforeClose when closing a revision.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T9838: Add sub-state for Revision: "Closed from Accepted"
- Commits
- rPde6349dd67af: Revision substate CLOSED_FROM_ACCEPTED
A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling differential.query shows the wasAcceptedBeforeClose property was setup correctly.
Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked.
Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
@epriestley, can you take a look at this?
Alternatively, I'll be happy to copy the existing logic from DiffusionCommitRevisionAcceptedHeraldField to DiffusionPreCommitContentRevisionAcceptedHeraldField, and leave the sub-state implementation to some later day.
Comment Actions
I think making this an entirely different state (i.e., "Closed" and "Closed from Accepted" being two different states) is probably a messier change than we need to make. What do you think about this instead?
- Add a wasAcceptedBeforeClose flag to DifferentialRevision (but see below).
- In DifferentialTransactionEditor->applyCustomInternalTransaction(), in the DifferentialTransaction::TYPE_ACTION block:
- If the action is "CLOSE" and the current status is "ACCEPTED", set the wasAcceptedBeforeClose flag.
- Otherwise, clear the wasAcceptedBeforeClose flag.
On adding the flag, it might be better to just add a properties JSON field for flexibility instead of a wasAcceptedBeforeClose column, since the cost of the SQL migration should be the same either way, and I suspect we'll have more of these in the future once stuff like --try ("don't submit for review until tests pass") gets implemented, and we don't have any cases in mind where we need to query by this flag.
I think that would make this a lot simpler and allow us to get away without D15084 or the pain of an arc soft version break (older versions of arc not understanding how to handle this status). I don't really see any downsides -- anything I'm missing?
Comment Actions
I'll give it a go; I wasn't easy with the extent of this change.
Maybe instead of wasAcceptedBeforeClose property, use a substate property? Would that save another change later?
Comment Actions
If you just add properties as a JSON field, the actual call would be setProperty('wasAcceptedBeforeClose', true) or whatever, and we have limitless power beyond that.
Comment Actions
(I feel like most newer objects are getting properties fields or similar these days, since we almost always find something useful to do with them.)
Comment Actions
Start re-writing with property instead of new state.
This is very broken, I'll dig in later this week to see why.
Comment Actions
Some inlines, including one likely (very minor) functional issue, but this looks reasonable to me overall.
| resources/sql/autopatches/20160201.revision.properties.1.sql | ||
|---|---|---|
| 3 | This should be COLLATE {$COLLATE_TEXT}. | |
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
| 240 | Maybe create a DifferentialRevision::PROPERTY_... constant or similar. | |
| src/applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php | ||
| 33 | I'd expect this doesn't actually work, because this is like writing a series of if ($x == $y) clauses in PHP. Specifically: $ cat test.php <?php switch ('undefined') { case true: echo "Undefined is true!\n"; break; default: echo "Undefined is not true.\n"; break; } $ php -f test.php Undefined is true! Consider $revision->hasRevisionProperty() or similar, maybe. (Note that hasProperty() will conflict with the method of the same name on LiskDAO. It might be worthwhile to rename that to hasLiskProperty at some point, but it's vaguely nice to call these getSomethingProperty() so you can grep for them a little better.) | |