Page MenuHomePhabricator

Move Differential to EditEngine
Closed, ResolvedPublic

Description

EditEngine is new infrastructure for creating and editing objects. Maniphest and a number of other applications have switched to it and reaped great rewards; Differential has not yet. Some of the benefits include:

  • New differential.revision.edit and differential.revision.search API endpoints.
  • Differential would get the new draft code, and stop having zombie comments stick around in the text box after submission.

T10967 should probably happen before/during this.

This is currently a large amount of work with few direct benefits.


Errata:

  • DifferentialRevisionEditController had a setRepositoryPHIDOverride() call for forcing revision repository edits to win over diff repository edits. Do we need / can we simplify this?
  • DifferentialRevisionEditController had some similar code where we defaulted the repository and view policy to those from the diff.
  • The Reviewers transaction has some special casing around commandeering creating an exception to the rule that a revision's author can not be a reviewer.
  • Edit form breadcrumb text strings, etc., could use some finessing -- seem okayish?
  • Default field order on the new edit form differs slightly from old field order?
  • Create diff, Create New Revision flow a little light on handholding?
  • Tasks field is no longer directly writable.
  • JIRA Issues field needs dedicated testing.

Reference screenshot for old form:

Revisions and Commits

rP Phabricator
D17248
D17207
D17191
D17170
D17162
D17163
D17146
D17133
D17132
D17131
D17125
D17120
D17119
D17118
D17117
D17116
D17115
D17114
D17113
D17111
D17109
D17108
D17107
D17106
D17091
D17089
D17088
D17087
D17086
D17085
D17084
D17083
D17082
D17081
D17080
D17079
D17078
D17077
D17075
D17074
D17073
D17072
D17071
D17070
D17068
D17067
D17066
D17061
D17058
D17055
D17059
D17054
D17053
D17050
D17048
D17047
D17044
D17043

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley updated the task description. (Show Details)Dec 16 2016, 8:41 PM
epriestley updated the task description. (Show Details)Dec 21 2016, 9:24 PM

JIRA actually seems fine: arc, web UI, and both the integrations (link / post) work properly for me at HEAD of master.

From D17067:

Both differential.createrevision and differential.updaterevision are now internally implemented by building a differential.revision.edit API call and then executing it.

So should Arcanist be modified to just call differential.revision.edit directly then?

So should Arcanist be modified to just call differential.revision.edit directly then?

Yes, sort of, but likely alongside T10945 and other Arcanist changes (referenced in T12010, but not yet organized there). There's generally no urgency on this because I think differential.updaterevision has no interesting code unique to that method anymore, and should have even less by the time the move to EditEngine completes.

differential.revision.edit does not yet support the "update" operation (or some other operations) and I may want to change its semantics somewhat while implementing it. Two related issues are:

  • T1081: Ability to undo a diff: The active diff on a revision is currently always the diff with the highest ID, so you can not undo updates (without doing a "revert update") and we need to do some extra work when loading active diffs. Neither of these are major issues but they're worth another look if this code is being updated anyway.
  • Harbormaster integration on the actual timeline had some sort of race that involved the update code? I don't recall the particulars but would like to plan through it here.

It also only has reclaim, abandon, close, reopen, plan, request, and commandeer support in my local working copy, slightly ahead of HEAD. It has approve, reject and resign nowhere.

There's significant remaining work here still -- roughly:

  • Finish implementing all the actions on EditEngine.
  • Implement errata on EditEngine. Existing list:
    • Close via commit is never an error.
    • Other close-via-commit metadata.
    • Reviewers do not downgrade on "Requeset Review" from "Accepted".
    • Commandeer needs to apply Herald rules.
    • Commandeer needs to move the old author to become a reviewer and remove the actor as a reviewer.
    • Mail tags -- we might need to do T10448 to avoid leaving a huge mess in place?
  • Figure out how what the timeline/plan is on migrating the old transactions.
  • Implement Update on EditEngine.
    • What's going on with the undo-diff / harbormaster stuff?
  • Extract reviewer rules here -- T10574?
  • Inline comments and inline comment previews.
  • What's happening with inline comments and the API? Probably punt for later generalization?

I'm going to dump everything into master now because I believe any disastrous brokenness should be reasonably easy to fix at this point.

fabe added a subscriber: fabe.Jan 11 2017, 5:27 PM

I think this might have broken one of our Herald rules. The rule in question is an Object rule for Commits on a Project and will open an Audit whenever a commit is made to a repository that's tagged with this project.
At least it did ~2 month ago and after the upgrade it just won't fire. Just using the test console on a new commit in a repo tagged with the project simply says "Rule failed".
To make sure i already removed all the other conditions from it so it only states "always".

If you'd like us to look at it, please file a separate bug report with reproduction instructions, following the guide in Contributing Bug Reports.

epriestley closed this task as Resolved.Feb 15 2017, 1:23 AM
epriestley claimed this task.

I think we've ended up in a fairly stable place here, and remaining work generally has existing tasks motivating it in terms of features (like T10967 for reviewers, T10574 for acceptance rules, T2543 for revision states, various tasks for inlines, T9713 for commit message parsing). Obsoleted transaction types also need migrations (but they should happen alongside the migrations that other features require so we're minimizing the number of weeks with migrations that need special planning) and this dumb mess got left in PhabricatorModularTransaction.php and will probably be there for an uncomfortably long time:

// TODO: Some "final" modifiers have been VERY TEMPORARILY moved aside to
// allow DifferentialTransaction to extend this class without converting
// fully to ModularTransactions.

...but overall I think we're in dramatically better shape than we used to be.