Make undo work with multiple slots.
Open, LowPublic

Description

When using the "undo" function to undo an edit (or a series of edits), changes to all slots should be undone, not just changes to the main slot.
This means that the logic for undoing edits needs to be extracted from EditPage.

At present, undo simply fails if multiple slots are involved, see T194412: Make undo fail gracefully for non-main slots..

Note that undo presently allows manual editing of the content to be undone. This is not possible for all kinds of content (see ContentHandler::supportsDirectEditing), and EditPage also does not currently support editing multiple slots at once.

It would probably be acceptable to not offer the ability to manually edit during an undo (check with product/UX), at least if the undo spans multiple slots. Or manual editing is only offered for the main slot - this seems ok for the SDC use case, but may be surprising when multiple slots are text based. We should at least show a warning explaining this.

Proposed implementation:
Create an UndoAction that works for multiple slots, and functions be showing a diff, and a button to confirm the edit, with no editing ability. In Action::getActionName(), if $actionName === "edit", check if the page has multiple slots (a WikiPage is constructed towards the end of that method anyway). If it has multiple slots, set $actionName = 'undo', triggering the new code.

UndoAction may later gain editing ability, or may otherwise be consolidated with code that is being factored out of EditPage. For now, this approach allows us to implement undo logic that will work for SDC without touching the old behavior. Note that undo logic will have to be factored out of EditPage anyway at some point. This could act as a first step.

However: depending on how much of the logic in EditPage::internalAttemptSave needs to be replicated (especially, conflict resolution and various hook points, including edit filters), it may make more sense to at least have a partial refactoring of EditPage, to avoid duplicating this logic.

Some considerations:

daniel created this task.Mar 15 2018, 6:13 PM
daniel triaged this task as Normal priority.
Anomie added a subscriber: Anomie.EditedMar 15 2018, 8:48 PM

The logic for determining the Content object for the undo based on the supplied 'undo' and 'undoafter' URL parameters looks like it's already in ContentHandler::getUndoContent(). EditPage calls through the stub in WikiPage, while ApiEditPage calls ContentHandler directly.

But note that the logic for undoing edits via the web UI ends with displaying the edit form with the undo content loaded (or not, if there's a "merge" conflict), so this probably depends on T174033: Refactor EditPage to allow multiple slots to be edited atomically [MCR].

Perhaps free form editing can be dropped from the undo function completely.

That sounds like a "torches and pitchforks" type of change.

EditPage will already have to deal with not-editing slots that aren't directly editable, all this would mean is the not-editing carrying forward the Content object from ContentHandler::getUndoContent() instead of inheriting the slot. We'll probably need to do something similar anyway for when someone edits an old revision rather than the current one.

When the undo operation spans multiple slots, free form editing should be disabled.

Same.

As a baseline, undo operations could just be denied of they would affect any slots other than the main slot.

That's probably the most straightforward approach until T174033 is done. Although the Commons editor community might consider that a blocker to SDC deployment.

Lydia_Pintscher moved this task from incoming to monitoring on the Wikidata board.Apr 23 2018, 7:28 AM
daniel renamed this task from Make action=undo work properly with multiple slots. to Make action=undo work with multiple slots..May 15 2018, 10:03 AM
daniel removed a project: Epic.
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)Jul 12 2018, 8:22 AM
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)
Tgr added a subscriber: Tgr.Jul 12 2018, 8:18 PM

When you are undoing past edits (ie. the "new" end of the undo range is not the current edit), the undo operation creates a content (array of contents, post-MCR) that does not exist on disk. That content needs to be presented to the user and then saved. Currently, EditPage just displays that content in the edit form; adding support for undo-without-edit would be nontrivial work, while multi-slot editing gives you multi-slot undo for free. (OTOH maybe we need undo-without-edit anyway if some slots don't support editing but support undo?)

daniel added a comment.EditedJul 12 2018, 8:30 PM

maybe we need undo-without-edit anyway if some slots don't support editing but support undo?

Yes. This is already the case for SDC. MediaInfo does not support direct editing. Wikibase has its own implementation of edit=undo, which just shows a diff (no content view). But that also only works with the main slot.

I note there's no reason the edit form with the undo content would have to actually be editable for any particular field or having the raw content of that field being displayed to the user, as long as the preview and show changes functions work sensibly. In the extreme case, an "undo without edit" could have all fields' content as <input type="hidden"> or as data stored in the session server-side.

an "undo without edit" could have all fields' content as <input type="hidden"> or as data stored in the session server-side.

No need for even that, all data is already stored on the server side.

daniel lowered the priority of this task from Normal to Low.Jul 13 2018, 4:56 PM
daniel updated the task description. (Show Details)Jul 13 2018, 6:23 PM
daniel renamed this task from Make action=undo work with multiple slots. to Make undo work with multiple slots..Jul 13 2018, 6:48 PM
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)Jul 15 2018, 6:34 PM

Change 447099 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] MCR: Add temporary web UI mcrundo action

https://gerrit.wikimedia.org/r/447099