Page MenuHomePhabricator

Differential Update Feedback / Rough Edges / Status
Closed, ResolvedPublic

Description

In T2222, we've been updating Differential to use the standard ApplicationTransactions and CustomField infrastructure. This involves some UI changes. Some of these are intentional and permanent, some are intentional until things get fully ported, some are changes of opportunity, and some are just broken.

If you run into issues, let us know here.

Related Objects

StatusAssignedTask
DuplicateNone
OpenNone
Resolvedepriestley
OpenNone
Resolvedepriestley
Resolvedepriestley
Resolvedchad
Resolvedchad
OpenNone
OpenNone
DuplicateNone
Resolvedchad
ResolvedNone
Resolvedhsb
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedjoshuaspence
Resolvedepriestley
Resolvedbtrahan
Resolvedbtrahan
Duplicateepriestley
Resolvedepriestley
Wontfixepriestley
Resolvedepriestley
Resolvedepriestley
DuplicateNone
Resolvedepriestley
Resolvedepriestley

Event Timeline

epriestley claimed this task.
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a subscriber: epriestley.
epriestley edited this Maniphest Task.Feb 27 2014, 7:19 PM

Two things we've intentionally obsoleted and removed, which won't be returning:

  • DifferentialFieldSelector mostly no longer does anything, and will vanish completely soon. This will be replaced by standard mechanisms in CustomField. These options aren't available quite yet (they're waiting on the field implementations to stabilize).
  • The differential.revision-custom-detail-renderer setting is obsolete. This allowed you to add actions to Differential. Instead, use standard event mechanisms.
chad added a subscriber: chad.Feb 27 2014, 7:22 PM
chad added a comment.Feb 27 2014, 7:45 PM

Was this the largest 'technical debt' task we have on the books?

Yeah, by far. There will probably be one more ApplicationTransactions migration eventually for Diffusion/Audits, and we have a couple of other things that need general updating (like Owners), but everything else on the books is very small compared to this.

(There are two other tasks -- T1191 and T4045 -- which may involve migration of a large amount of data, but the code changes associated with both of those are trivial.)

We have a bug with "Plan Changes" right now, exemplified in D8366. Generally, there are a bunch of edge cases with state transitions now (T4168, T4167) and some pending work on them (T3202, T2543). All of these weird cases cropped up when we started tracking status individually in T1279.

One possible fix is to separate "needs review" and "plan changes" into different states. Another fix is to invalidate accepts on "plan changes". The latter lets us disable sticky accept more easily.

I think the interesting workflow here is:

  • Revision is accepted;
  • author plans changes;
  • author requests review (without updating, i.e. requests review of the same code).

What state should the revision end up in? If it's "Accepted" (and I think it probably should be), then I think we need a separate "plan changes" state.

Similar to the above, revisions currently aren't kicking back to "Needs Review" after an update if there's at least one "Rejecting" reviewer. This and the above are more-broken-than-intended and will be getting fixed soon, but I'm dealing with a bunch of IRL junk today and may not have a patch until tomorrow.

My specific plan here is:

  • Introduce a new "Changes Planned" revision state. This is like "Needs Revision", but requires an explicit transition ("Request Review", "Update") to get out of. This will fix the "Plan Changes" -> "Accepted" transition. It works just like "Needs Revision" in other settings.
  • Introduce new explicit reviewer states, "Accepted Older Diff" and "Rejected Older Diff". When a revision is updated with a new diff, all "rejects" will be changed to "rejected older diff". If sticky accept is disabled, accepts will be changed to "Accepted older diff". State-based transitions will then take effect, which may move the revision to "accepted" immediately (if we have a sticky accept and no rejects) or may leave it in "needs review".
epriestley edited this Maniphest Task.Mar 5 2014, 1:55 AM
epriestley edited this Maniphest Task.Mar 5 2014, 2:18 AM
epriestley edited this Maniphest Task.Mar 5 2014, 6:47 PM
epriestley edited this Maniphest Task.
avivey added a subscriber: avivey.Mar 5 2014, 7:40 PM

Some of Differential's fields are diff-specific, rather than revision-specific: branch, lint status, unit status, host, path, and then sort-of arcanist project and repository (these are transferred to the revision).

Previously, we monkeyed around with these so they showed the information for the visible diff implicitly, instead of information for the active diff. This wasn't really indicated in the UI, but generally aligned with user expectations. However, it was awkward from an implementation standpoint, and it currently no longer happens.

The primary way I want to address this is by making some of these custom fields of Diffs, and putting a second section in where the diff stuff lives, roughly like this:

+-----------------------------------------------+
| D123 do some stuff                            |
+--------------------------------------+--------+
| prop: value                          | action |
| prop: value                          +--------+
| prop: value                                   |
+-----------------------------------------------+
| SUMMARY                                       |
| did some stuff                                |
+-----------------------------------------------+

+-----------------------------------------------+
| Diff 15                                       |
+--------------------------------------+--------+
| lint: value                          | action |
| unit: value                          +--------+
| brch: value                                   |
+-----------------------------------------------+

If that ends up being hideous, I'll merge the diff stuff into the revision stuff instead (like it used to be), but probably separate it out a little bit.

Among these fields, we may also have been special casing lint and unit results so they didn't show results for the automatic updates on close. I don't have a specific plan for this (I'd like to avoid implementing the same magical rule as before, if it did actually exist, since it's not explicit) but will take a look at smoothing things out.

wotte added a subscriber: wotte.Mar 12 2014, 7:06 PM

If the revision summary is empty, the mail messages now look a little awkward:

Also, the test plan is now gone from the message -- not sure if that was an intentional removal or an oversight.

Test plan removal was "intentional-until-someone-complains". I take it you'd like to see it return?

epriestley edited this Maniphest Task.Mar 14 2014, 1:37 PM
epriestley edited this Maniphest Task.Mar 14 2014, 6:53 PM

I don't feel super strongly, but am happy to see it back -- thanks. :)

epriestley closed this task as Resolved.Mar 21 2014, 3:09 AM

Things seem to have calmed down, see T4649, T4642 for some stragglers.