Page MenuHomePhabricator

Add a rejected status to `ArcanistDifferentialRevisionStatus`.
AbandonedPublic

Authored by joshuaspence on May 27 2014, 11:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 6:59 PM
Unknown Object (File)
Sat, Dec 7, 5:39 PM
Unknown Object (File)
Tue, Dec 3, 5:12 PM
Unknown Object (File)
Tue, Nov 26, 11:47 PM
Unknown Object (File)
Fri, Nov 22, 9:27 PM
Unknown Object (File)
Fri, Nov 22, 8:45 AM
Unknown Object (File)
Nov 17 2024, 9:09 PM
Unknown Object (File)
Nov 14 2024, 6:47 AM
Subscribers

Details

Summary

Ref T4720. Add a ArcanistDifferentialRevisionStatus::REJECTED constant.

Test Plan

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

joshuaspence retitled this revision from to Add a rejected status to `ArcanistDifferentialRevisionStatus`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

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

Sure, that's fine with me.