Page MenuHomePhabricator

Treat "Accepted" and "Accepted but Blocked" differentials differently within the UI
Closed, ResolvedPublic

Description

Ive noticed that some differentials get accepted, but are generally not ready to land and are marked as such with "[Later]" (or similar) in the title.

I wonder if it would be worth making this concept a little bit more concrete (i.e. treating it different in the UI). A couple of cases which come to mind as preconditions for a differential being landed are:

  • After another differential has been landed (dependent).
  • After a maniphest is closed.
  • After a given time period has elapsed.

Event Timeline

joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added a project: Differential.
joshuaspence added a subscriber: joshuaspence.

I'm not sure this is particularly valuable to formalize. At least on this install, these are quite rare (maybe 20 across 9,000 reviews?) and are held for different reasons (data-destroying migrations are #1, but I think everything after that has unique reasons), the reasoning is usually obvious from context (maybe not for D9204, but all the other ones I can think of) -- that is, if you read through the revision it's usually clear why it is being held back. They also usually stay in the correct queues and I don't think they get in the way too much (at least, in my experience).

The (1) case you can already do with dependencies, although that's not as explicit in the UI as it perhaps could be (for example, dependent revisions aren't prevented from moving to "Accepted").

Are you seeing specific problems with these kinds of changes, or just noticing a pattern?

Offhand, I can't really come up with an interaction which would significantly improve handling of these changes: they already usually have the right queuing behavior, they're usually fairly obvious, and putting "[Later]" in the title is really simple.

(It's also normally somewhat bad to hold things, especially in an "Accepted" state, since a review today may not really be valid in 3 months (e.g., an API or practice might have changed, or the best way to build something might be different). There are plenty of exceptions, of course.)

I agree with you on most parts. I do, however, think that accepted-but-blocked differentials should be treated differently from accepted differentials in the UI. In most cases "blocked" can be inferred based on dependencies. It is probably not worth considering other forms of blocking (time based, for example).

joshuaspence renamed this task from Formalise the notion of accepted differentials which are not ready to be landed. to Treat "Accepted" and "Accepted but Blocked" differentials differently within the UI..Jun 3 2014, 5:59 PM
joshuaspence renamed this task from Treat "Accepted" and "Accepted but Blocked" differentials differently within the UI. to Treat "Accepted" and "Accepted but Blocked" differentials differently within the UI.Jul 10 2014, 7:42 PM
epriestley claimed this task.

At HEAD (after D15929), bucketing is now more considerate of individual reviewer status and cases in T5217 should be handled in line with user expectation.

Also at HEAD (after D15930), revisions which are accepted but have at least one unlanded dependency show an icon/note in the UI. This isn't quite a full blown different-status sort of thing, but I'm imagining it's a reasonable balance of concerns. You can also write a custom bucketing algorithm now if you want to fully separate these revisions.

I think the other use cases (time, tasks) are too rare to motivate upstream support, at least at this time.