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:

Details

Commits
D17248 / rPdf939f1337fd: Fix two issues with embedding other fields inside "Summary" or "Test Plan" in…
D17207 / rP62cf4e6b95ab: Remove some remnants of the old ways commit mesage fields worked from…
D17191 / rP7d3d0224071a: Restore "[Action]" mail subject lines to Differential/Diffusion
D17170 / rP00e2755eab62: Provide tailored strings for revision creation
D17162 / rPb08c9b3ffa8a: Remove extra container tag on HandleListViews rendering from…
D17163 / rP425deeb52391: Fix an issue which could prevent blocking reviewers from being removed from…
D17146 / rP1f2306999bb5: Fix a case where "Accept + Comment" would ignore the "Accept"
D17133 / rP50de3071acaf: Define Differential email action in terms of EditEngine
D17132 / rP35750b9c615a: Make some Differential comment actions (like "Accept" and "Reject") conflict…
D17131 / rP00313094d3ab: Make "View" links on Differential inline comment previews work again
D17125 / rP7bf49d254e63: Use a more conventional spelling of "CLOSED"
D17120 / rP69194fdaf510: Make marking comments as "Done" work cleanly on EditEngine
D17119 / rPa4ba7daf9009: Add transitional support for mail tags to Differential on EditEngine
D17118 / rPb373dcef7448: Restore some minor state behaviors to Differential on EditEngine
D17117 / rP9b4090af5565: Restore quote and warning behaviors to Differential EditEngine comment area
D17116 / rP18249b097fb6: Make inline comment preview and submission mostly work on EditEngine
D17115 / rPf7b5955d3386: Order actions sensibly within Differential revision comment action groups
D17114 / rP48fcfeadafe2: Allow comment actions to be grouped; group Differential "Review" and "Revision"…
D17113 / rP5a6643f36ff8: Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine
D17111 / rP8b74cd481a47: Restore "Commandeer" action to Differential on EditEngine
D17109 / rPdeb19b2d572d: Restore "Plan Changes" and "Request Review" actions to Differential on…
D17108 / rPa90ab7f4033d: Restore "Close" and "Reopen" actions to Differential on EditEngine
D17107 / rP3c5a17ba8a55: Restore "Reclaim" and "Abandon" actions to Differential on EditEngine
D17106 / rPc05306d746d7: Move Differential to EditEngine comments
D17091 / rP28d74ae572f3: Rename Differenital "EditPro" controller back to "Edit"
D17089 / rP895cdaca5d1f: Simplify "Blame Revision" field in Differential
D17088 / rP60f41b87e9d0: Simplify "Tasks" field in Differential
D17087 / rPf1f24e036034: Simplify "Repository" field in Differential
D17086 / rP18debbfdb4f4: Simplify Differential "Reviewers" field
D17085 / rP2ebbac86ded9: Simplify Differential "Summary" field
D17084 / rPc458f09dccb3: Simplify "Test Plan" custom field
D17083 / rP9e4c16c4c3ac: Remove Differential "Title" custom field
D17082 / rPf552a20c6193: Remove Differential "View Policy" field
D17081 / rP84572a3b9351: Remove Differential subscribers field
D17080 / rP3893b5f1a5fa: Remove "Revision ID" custom field
D17079 / rP77601bf58c9a: Remove "Reviewed By" Differential field
D17078 / rP5e606504b7f5: Remove "DifferentialProjectsField" custom field
D17077 / rP8bba1eba85db: Remove "DifferentialParentRevisionsField" custom field
D17075 / rP5ea071f65834: Remove "DifferentialGitSVNIDField" custom field in Differential
D17074 / rP4df072cca688: Remove "DifferentialEditPolicyField" custom field
D17073 / rPbc6522dbca07: Remove "DifferentialConflictsField" custom field
D17072 / rP93c0ffd02c82: Remove "Child Revisions" custom field in Differential
D17071 / rP74a0caf9ce16: Remove "Author" CustomField in Differential
D17070 / rP914d9fa8b94f: Simplify Auditors custom field in Differential
D17068 / rPa74d602b3c33: Make stored custom fields work with v3 EditEngine API
D17067 / rP64509dcca7be: Drive CLI-based revision edits through "differential.revision.edit" API +…
D17066 / rP24926f9453e5: Move Differential commit message rendering to dedicated classes
D17061 / rP89d88dafcc41: Fix a Differential exception in invalid/missing fields
D17058 / rP8476ad1a281c: Separate all commit message field parsing out of Differential custom fields
D17055 / rP552c54668988: Separate commit message parsing and validation from Conduit
D17059 / rP378387a0785f: Fix an issue with mentioning revisions on the new EditEngine code
D17054 / rP102ea3cfa41d: Replace Differential Edit controller with EditEngine-driven EditPro controller
D17053 / rP32ce21a18197: Allow the new Differential EditEngine form to create/update diffs for revisions
D17050 / rP7f99f2cde835: Add EditEngine + Modular Transactions for reviewers
D17048 / rP6c9af81f7adb: Support "Test Plan" with modular transactions and EditEngine
D17047 / rP5349d6bd5c09: Add Summary and Repository EditEngine fields + Modular Transactions to…
D17044 / rP0906bf547b9a: Begin adding "pro" modular transaction fields to Differential
D17043 / rPeda64b85497b: Add a very basic EditPro controller for Differential

Related Objects

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.