Ref T4720. DifferentialAction::ACTION_REJECT is more suited for the "permanently reject this revision" action.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4720: Expose "Abandon Revision" to non-authors with a config flag
I haven't tested this yet. Will update once I get a dev box up and running.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- diff-change
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 717 Build 717: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
It may not be worth making these changes just for the sake of having "nicer" constants. If it's not worth the effort then we can just leave as is.
Comment Actions
Yeah, I think we shouldn't pursue this, at least for now:
- Installs with large datasets are pretty sensitive to the downtime caused by migrations of large tables; this is a large-ish table and the migration is low-value.
- differential_comment no longer has readers or writers and is just a safety table in case we botched the migration. D8242 will drop it. We can probably land that soon, but I've been waiting for all the large installs to finish the migration. The new table is differential_transaction, and the action constant is stored in newValue for transactions with transactionType equal to differential:action, so the correct form of this a bit more complicated and operates on a slightly larger dataset. Doing this change also makes un-botching any migration issues discovered later more difficult. While we're pretty much in the clear on this, we're not 100% in the clear yet. If we did want to pursue this, I'd at least want to wait until after D8242 lands.
- I think ACTION_CHANGE, while probably more correct, is also a bit more ambiguous than ACTION_REJECT. Without context, ACTION_CHANGE could mean almost anything.
- This constant has been around for a very long time and it's relatively likely (compared to other janitorial/cleanup changes we might make) that installs (particularly Facebook) may be using it in third-party code.
- We don't have a compelling feature-related reason to do this. This is more of a public relations / communication issue, but it's vaguely nice to couple compatibility breaks with significant feature changes since it's less of a pain for users to deal with breaks if they feel like they're getting something cool out of it.
- To the degree that this is moving the current REJECT action out of the way to make room for a new REJECT action, I think that's not desirable since it would silently change the behavior of third-party code. It's OK to break handlers and such occasionally, but clean breaks where you hit an undefined runtime constant are a lot better than silent breaks where the behavior changes without notice.
Also:
- I think the T4720 change should just be an "ABANDON" action under the hood (not a new type of action); we'll just permit non-authors to perform it. This will be a little bit confusing since the UI text will probably be "reject", but it won't be as bad as a new REJECT action or state existing.
Comment Actions
Yeah, I agree with you on this and had the feeling that this was a bad idea. I am happy to just modify the behavior of DifferentialAction::ACTION_ABANDON rather than create a new state entirely.