Ref T4720. Add a ArcanistDifferentialRevisionStatus::REJECTED constant.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4720: Expose "Abandon Revision" to non-authors with a config flag
N/A
Diff Detail
- Repository
- rARC Arcanist
- Branch
- reject
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 718 Build 718: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Per D9303, I don't think this is important or common enough to add a new state for it -- we can get a "good enough" implementation with a tiny amount of work. Instead of adding new states/actions, just:
- When the flag is set, expose the existing ABANDON action to non-authors in the UI.
- When the flag is set, allow non-authors to actually take the action (i.e., don't block them in the TransactionEditor).
- In the UI, label the action "Reject Completely" or similar, instead of "Abandon Revision". This is purely a cosmetic change in the rendering of the dropdown.
- In the UI, render the transaction as "alincoln rejected this revision." instead of "alincoln abandoned this revision." if the actor is not the same as the revision author.
This has downsides:
- If a revision was abandoned, and then commandeered, the old "Abandon" transaction will render in a slightly misleading way. Although the actor "abandoned" at the time, they're no longer the author, so their transaction will change to read "rejected".
- The "Abandoned" state in the UI is a slightly less clean match than a new state would be.
However, it has upsides:
- No migrations.
- No new constants.
- Tiny amount of new complexity.
Since I think it will be rare for installs to enable this option and am worried about the complexity of introducing new optional workflow states (i.e., no one will ever remember to toggle this flag on and test this case when testing changes, and every flag we add like this theoretically doubles the number of unit tests we need to run), I think we should start with the simple, not-quite-perfect implementation and see how imperfect it is in practice. I would guess that even installs which enable this will only use it rarely.
(We can also easily move forward from the "not-quite-perfect" version to a proper version (with a new state and action) if it turns out I'm wrong and the modified ABANDON is too messy or confusing.)