Update overall revision status after reviewers change
ClosedPublic
Actions

Authored by epriestley on Feb 25 2014, 6:21 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T2222: Implement ApplicationTransactions in Differential

Commits

Restricted Diffusion Commit
rPa69cca9fbb15: Update overall revision status after reviewers change

Summary

Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.

Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.

To deal with this in ApplicationTransactions, I did this:

applyFinalEffects() can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
In Differential, check for an overall revision state transition in applyFinalEffects() (e.g., your reject moving the revision to a rejected state).
I'm only writing the transaction if the transition is implied and indirect.
- For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.

The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.

Test Plan

{F118143}

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

This seems reasonable to me too.

On the accept state thing, I'd personally prefer a more anarchist-style where the last person's "accept" or "reject" set the overall state. I know there's lots of context, desire and now legacy to have it the way you have it now though, so yeah.

btrahan accepted this revision.Feb 25 2014, 8:22 PM

On the accept state thing, I'd personally prefer a more anarchist-style where the last person's "accept" or "reject" set the overall state.

Oh, like any accept overrides other rejects? I think we might split "reject" into hard/soft versions, and the "soft" version would work like that. For at least some use cases, I think the blocking/hard rejects are still important, but they're more strict than I'd like right now.

The major thing I'm thinking about by adding this transaction is a case like this:

> a accepted.
> b accepted.
> c accepted.

There's currently no way to determine when the revision became committable. It could have become committable after "a" if there were no reviewers, or after "b" if "b" was a blocking reviewer, or after "c" if "c" was a blocking reviewer. I don't think this causes much confusion, but part of the value of transactions is audibility, and having an unambiguous transaction for every state change seems to serve that.

If it's spammy or annoying we can easily hide it. I'd probably want to retain it in the transaction record just-in-case, but I'm not sure it provides much real value as a visible transaction (although maybe it will make things easier for new users to understand).