Page MenuHomePhabricator
Create Task
Maniphest T11114

Move Differential to EditEngine
Closed, ResolvedPublic
Actions

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:

Screen Shot 2016-12-14 at 6.05.21 AM.png (1×1 px, 196 KB)

Revisions and Commits

rP Phabricator
D17248rPdf939f1337fd Fix two issues with embedding other fields inside "Summary" or "Test Plan" in…
D17207rP62cf4e6b95ab Remove some remnants of the old ways commit mesage fields worked from…
D17191rP7d3d0224071a Restore "[Action]" mail subject lines to Differential/Diffusion
D17170rP00e2755eab62 Provide tailored strings for revision creation
D17162rPb08c9b3ffa8a Remove extra container tag on HandleListViews rendering from…
D17163rP425deeb52391 Fix an issue which could prevent blocking reviewers from being removed from…
D17146rP1f2306999bb5 Fix a case where "Accept + Comment" would ignore the "Accept"
D17133rP50de3071acaf Define Differential email action in terms of EditEngine
D17132rP35750b9c615a Make some Differential comment actions (like "Accept" and "Reject") conflict…
D17131rP00313094d3ab Make "View" links on Differential inline comment previews work again
D17125rP7bf49d254e63 Use a more conventional spelling of "CLOSED"
D17120rP69194fdaf510 Make marking comments as "Done" work cleanly on EditEngine
D17119rPa4ba7daf9009 Add transitional support for mail tags to Differential on EditEngine
D17118rPb373dcef7448 Restore some minor state behaviors to Differential on EditEngine
D17117rP9b4090af5565 Restore quote and warning behaviors to Differential EditEngine comment area
D17116rP18249b097fb6 Make inline comment preview and submission mostly work on EditEngine
D17115rPf7b5955d3386 Order actions sensibly within Differential revision comment action groups
D17114rP48fcfeadafe2 Allow comment actions to be grouped; group Differential "Review" and "Revision"…
D17113rP5a6643f36ff8 Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine
D17111rP8b74cd481a47 Restore "Commandeer" action to Differential on EditEngine
D17109rPdeb19b2d572d Restore "Plan Changes" and "Request Review" actions to Differential on…
D17108rPa90ab7f4033d Restore "Close" and "Reopen" actions to Differential on EditEngine
D17107rP3c5a17ba8a55 Restore "Reclaim" and "Abandon" actions to Differential on EditEngine
D17106rPc05306d746d7 Move Differential to EditEngine comments
D17091rP28d74ae572f3 Rename Differenital "EditPro" controller back to "Edit"
D17089rP895cdaca5d1f Simplify "Blame Revision" field in Differential
D17088rP60f41b87e9d0 Simplify "Tasks" field in Differential
D17087rPf1f24e036034 Simplify "Repository" field in Differential
D17086rP18debbfdb4f4 Simplify Differential "Reviewers" field
D17085rP2ebbac86ded9 Simplify Differential "Summary" field
D17084rPc458f09dccb3 Simplify "Test Plan" custom field
D17083rP9e4c16c4c3ac Remove Differential "Title" custom field
D17082rPf552a20c6193 Remove Differential "View Policy" field
D17081rP84572a3b9351 Remove Differential subscribers field
D17080rP3893b5f1a5fa Remove "Revision ID" custom field
D17079rP77601bf58c9a Remove "Reviewed By" Differential field
D17078rP5e606504b7f5 Remove "DifferentialProjectsField" custom field
D17077rP8bba1eba85db Remove "DifferentialParentRevisionsField" custom field
D17075rP5ea071f65834 Remove "DifferentialGitSVNIDField" custom field in Differential
D17074rP4df072cca688 Remove "DifferentialEditPolicyField" custom field
D17073rPbc6522dbca07 Remove "DifferentialConflictsField" custom field
D17072rP93c0ffd02c82 Remove "Child Revisions" custom field in Differential
D17071rP74a0caf9ce16 Remove "Author" CustomField in Differential
D17070rP914d9fa8b94f Simplify Auditors custom field in Differential
D17068rPa74d602b3c33 Make stored custom fields work with v3 EditEngine API
D17067rP64509dcca7be Drive CLI-based revision edits through "differential.revision.edit" API +…
D17066rP24926f9453e5 Move Differential commit message rendering to dedicated classes
D17061rP89d88dafcc41 Fix a Differential exception in invalid/missing fields
D17058rP8476ad1a281c Separate all commit message field parsing out of Differential custom fields
D17055rP552c54668988 Separate commit message parsing and validation from Conduit
D17059rP378387a0785f Fix an issue with mentioning revisions on the new EditEngine code
D17054rP102ea3cfa41d Replace Differential Edit controller with EditEngine-driven EditPro controller
D17053rP32ce21a18197 Allow the new Differential EditEngine form to create/update diffs for revisions
D17050rP7f99f2cde835 Add EditEngine + Modular Transactions for reviewers
D17048rP6c9af81f7adb Support "Test Plan" with modular transactions and EditEngine
D17047rP5349d6bd5c09 Add Summary and Repository EditEngine fields + Modular Transactions to…
D17044rP0906bf547b9a Begin adding "pro" modular transaction fields to Differential
D17043rPeda64b85497b Add a very basic EditPro controller for Differential

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley mentioned this in 2016 Week 51 (Late December).Dec 16 2016, 6:29 PM
epriestley added a commit: rP895cdaca5d1f: Simplify "Blame Revision" field in Differential.Dec 16 2016, 8:03 PM
epriestley added a revision: D17091: Rename Differenital "EditPro" controller back to "Edit".Dec 16 2016, 8:33 PM
epriestley updated the task description. (Show Details)Dec 16 2016, 8:41 PM
epriestley added a commit: rP28d74ae572f3: Rename Differenital "EditPro" controller back to "Edit".Dec 16 2016, 9:17 PM
fooishbar added a subscriber: fooishbar.Dec 19 2016, 9:43 AM
eadler awarded a token.Dec 19 2016, 7:10 PM
epriestley mentioned this in T12033: Large diffs can still repeatedly fail to insert.Dec 20 2016, 12:42 PM
avivey mentioned this in Z1336: General Chat.Dec 20 2016, 9:45 PM
avivey awarded a token.Dec 20 2016, 9:47 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.

epriestley mentioned this in 2016 Week 52 (Very Late December).Dec 27 2016, 6:42 PM
epriestley created subtask T12049: Give EditEngine universal support for "Haunted Mode".Dec 28 2016, 5:01 PM
epriestley added a revision: D17106: Move Differential to EditEngine comments.Dec 28 2016, 6:44 PM
epriestley added a revision: D17107: Restore "Reclaim" and "Abandon" actions to Differential on EditEngine.Dec 28 2016, 8:53 PM
epriestley added a revision: D17108: Restore "Close" and "Reopen" actions to Differential on EditEngine.Dec 28 2016, 9:23 PM
epriestley added a revision: D17109: Restore "Plan Changes" and "Request Review" actions to Differential on EditEngine.Dec 28 2016, 9:42 PM
epriestley added a revision: D17111: Restore "Commandeer" action to Differential on EditEngine.Dec 28 2016, 10:37 PM
joshuaspence added a comment.Dec 29 2016, 10:42 AM

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?

epriestley added a comment.Dec 29 2016, 3:50 PM

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?
epriestley mentioned this in T11659: Differential comment panel haunter does not allow scrolling to bottom of differential revision when no ui.footer-items configured.Dec 29 2016, 4:16 PM
epriestley mentioned this in T4043: Provide a keyboard shortcut to focus the comment textarea in Differential.Dec 29 2016, 4:20 PM
epriestley added a revision: D17113: Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine.Dec 29 2016, 6:53 PM
epriestley closed subtask T12049: Give EditEngine universal support for "Haunted Mode" as Resolved.Dec 29 2016, 8:49 PM
epriestley added a revision: D17114: Allow comment actions to be grouped; group Differential "Review" and "Revision" actions.Dec 29 2016, 9:20 PM
epriestley added a revision: D17115: Order actions sensibly within Differential revision comment action groups.Dec 29 2016, 9:44 PM
epriestley added a revision: D17116: Make inline comment preview and submission mostly work on EditEngine.Dec 30 2016, 10:09 PM
epriestley added a revision: D17117: Restore quote and warning behaviors to Differential EditEngine comment area.Dec 31 2016, 5:01 PM
epriestley added a revision: D17118: Restore some minor state behaviors to Differential on EditEngine.Dec 31 2016, 5:24 PM
epriestley added a revision: D17119: Add transitional support for mail tags to Differential on EditEngine.Dec 31 2016, 5:34 PM
epriestley added a revision: D17120: Make marking comments as "Done" work cleanly on EditEngine.Dec 31 2016, 5:59 PM
epriestley added a comment.Dec 31 2016, 6:06 PM

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

epriestley added a commit: rPc05306d746d7: Move Differential to EditEngine comments.Dec 31 2016, 6:06 PM
epriestley added a commit: rP3c5a17ba8a55: Restore "Reclaim" and "Abandon" actions to Differential on EditEngine.
epriestley added a commit: rPa90ab7f4033d: Restore "Close" and "Reopen" actions to Differential on EditEngine.
epriestley added a commit: rPdeb19b2d572d: Restore "Plan Changes" and "Request Review" actions to Differential on….
epriestley added a commit: rP8b74cd481a47: Restore "Commandeer" action to Differential on EditEngine.Dec 31 2016, 6:09 PM
epriestley added a commit: rP5a6643f36ff8: Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine.
epriestley added a commit: rP48fcfeadafe2: Allow comment actions to be grouped; group Differential "Review" and "Revision"….
epriestley added a commit: rPf7b5955d3386: Order actions sensibly within Differential revision comment action groups.
epriestley added a commit: rP18249b097fb6: Make inline comment preview and submission mostly work on EditEngine.
epriestley added a commit: rP9b4090af5565: Restore quote and warning behaviors to Differential EditEngine comment area.Dec 31 2016, 6:11 PM
epriestley added a commit: rPb373dcef7448: Restore some minor state behaviors to Differential on EditEngine.
epriestley added a commit: rPa4ba7daf9009: Add transitional support for mail tags to Differential on EditEngine.
epriestley added a commit: rP69194fdaf510: Make marking comments as "Done" work cleanly on EditEngine.
epriestley added a revision: D17125: Use a more conventional spelling of "CLOSED".Jan 1 2017, 5:20 PM
epriestley added a commit: rP7bf49d254e63: Use a more conventional spelling of "CLOSED".Jan 1 2017, 5:27 PM
epriestley mentioned this in T7483: Invalid argument supplied for foreach.Jan 1 2017, 5:34 PM
epriestley added a revision: D17131: Make "View" links on Differential inline comment previews work again.Jan 2 2017, 6:56 PM
epriestley added a revision: D17132: Make some Differential comment actions (like "Accept" and "Reject") conflict with one another.Jan 2 2017, 7:22 PM
epriestley added a revision: D17133: Define Differential email action in terms of EditEngine.Jan 2 2017, 7:47 PM
epriestley added a commit: rP00313094d3ab: Make "View" links on Differential inline comment previews work again.Jan 2 2017, 9:24 PM
epriestley added a commit: rP35750b9c615a: Make some Differential comment actions (like "Accept" and "Reject") conflict….
epriestley added a commit: rP50de3071acaf: Define Differential email action in terms of EditEngine.
epriestley added a revision: D17146: Fix a case where "Accept + Comment" would ignore the "Accept".Jan 5 2017, 7:17 PM
epriestley added a commit: rP1f2306999bb5: Fix a case where "Accept + Comment" would ignore the "Accept".Jan 5 2017, 7:30 PM
epriestley mentioned this in 2017 Week 1 (Early January).Jan 7 2017, 1:01 AM
epriestley mentioned this in T12074: conduit API extensions for subprojects and milestones..Jan 9 2017, 3:35 PM
epriestley added a revision: D17163: Fix an issue which could prevent blocking reviewers from being removed from revisions.Jan 9 2017, 4:20 PM
epriestley added a revision: D17162: Remove extra container tag on HandleListViews rendering from ModularTransactions in text mode.Jan 9 2017, 4:21 PM
epriestley added a commit: rP425deeb52391: Fix an issue which could prevent blocking reviewers from being removed from….Jan 9 2017, 4:41 PM
epriestley added a commit: rPb08c9b3ffa8a: Remove extra container tag on HandleListViews rendering from….
epriestley mentioned this in T12085: Upgrading: Changes to Differential Custom Fields in Commit Messages.Jan 9 2017, 5:32 PM
epriestley added a revision: D17170: Provide tailored strings for revision creation.Jan 10 2017, 8:33 PM
epriestley added a commit: rP00e2755eab62: Provide tailored strings for revision creation.Jan 10 2017, 8:45 PM
epriestley mentioned this in T12095: Differential reads and writes draft status to different tables, leading to various UI inconsistencies.Jan 11 2017, 4:06 PM
epriestley mentioned this in D17175: Bring very basic EditEngine support to commits.Jan 11 2017, 4:48 PM
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".

epriestley added a comment.Jan 11 2017, 5:33 PM

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 mentioned this in rP7ff0be1bde2f: Bring very basic EditEngine support to commits.Jan 11 2017, 5:34 PM
epriestley mentioned this in T12097: Object Herald Rules for Commits on Projects no longer working as expected.Jan 11 2017, 6:04 PM
chad mentioned this in T12098: Create Revision from FavoritesMenuItem doesn't work.Jan 12 2017, 3:08 AM
chad added a subtask: T12098: Create Revision from FavoritesMenuItem doesn't work.
epriestley added a revision: D17191: Restore "[Action]" mail subject lines to Differential/Diffusion.Jan 12 2017, 3:47 PM
epriestley added a commit: rP7d3d0224071a: Restore "[Action]" mail subject lines to Differential/Diffusion.Jan 12 2017, 7:44 PM
epriestley mentioned this in D17205: Make yellow "draft" bubbles more generic.Jan 13 2017, 3:37 PM
epriestley mentioned this in rP7276af6a81f4: Make yellow "draft" bubbles more generic.Jan 13 2017, 5:02 PM
epriestley mentioned this in 2017 Week 2 (Mid January).Jan 13 2017, 10:38 PM
epriestley added a revision: D17207: Remove some remnants of the old ways commit mesage fields worked from Differential.Jan 13 2017, 11:18 PM
epriestley added a commit: rP62cf4e6b95ab: Remove some remnants of the old ways commit mesage fields worked from….Jan 13 2017, 11:29 PM
epriestley mentioned this in T12118: Differential email does not contain reviewers/subscribers information anymore.Jan 18 2017, 2:25 PM
epriestley added a revision: D17248: Fix two issues with embedding other fields inside "Summary" or "Test Plan" in Differential with the web UI.Jan 25 2017, 7:25 PM
epriestley added a commit: rPdf939f1337fd: Fix two issues with embedding other fields inside "Summary" or "Test Plan" in….Jan 25 2017, 9:07 PM
epriestley mentioned this in T12197: Audits are no longer triggered using singular "Auditor:".Feb 3 2017, 2:31 PM
epriestley mentioned this in D17294: Add better description to built-in Home.Feb 6 2017, 1:17 PM
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.

epriestley mentioned this in T12350: Add support for "Plan Changes" as a condition for Differential Revision in Herald.Mar 4 2017, 3:28 PM
epriestley mentioned this in D17004: Support Custom Fields in List View.May 4 2017, 9:29 PM
epriestley mentioned this in D18784: Support "Set X to" as an action in Herald for tokenizer/datasource custom fields.Nov 28 2017, 9:31 PM
• GoogleLegacy added a subscriber: • GoogleLegacy.Sep 10 2018, 8:30 AM
epriestley closed subtask T12098: Create Revision from FavoritesMenuItem doesn't work as Resolved.Apr 2 2019, 10:22 PM
Phabricator · Built by Phacility