Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better History Control - Rollbacks and Revertions (215, 340) #1144

Draft
wants to merge 85 commits into
base: develop
Choose a base branch
from

Conversation

Taeir
Copy link
Contributor

@Taeir Taeir commented Jul 23, 2023

Rollback History

  1. Adds the close reason into the question_closed history item.
  2. Adds support for undoing history items (undo single event)
  3. Adds support for rolling back to version (revert to state as it was at a previous point in history).
  4. Adds support for unhiding/revealing history

The idea started as "this should be pretty doable", but the final Implementation was quite tricky and went through multiple iterations. I invite you to ask critical questions about the design, point out missing aspects or suggest design improvements. I have actually written quite some tests which led to the fixing of quite a few bugs already, because I felt the complexity of the feature warranted it.

Supported Events

After discussion, it was decided that rolling back should consider edits only.
For undoes, history hiding/revealing is also possible.

1. Close reasons in history

This PR also adds close reasons to question closed history items. This is necessary to correctly revert to a previous state where the post is closed. I have added a migration which will attempt to add as much information into history as we still can (i.e. if the post is still closed, add it to the last close event). Of course, if posts were reopened after they were closed, this information is irrevocably lost.

Additionally, the comment display now supports (very limited) markdown, which is used to display the close reason in the history:

Close reason in history

2. Undo individual events

Something can only be rolled back (undone) if its changes are still present/matching. More specifically:

  • For edits, you can only rollback the edit if the following hold:
    • if the edit updates the title - the title after the edit matches the current title of the post
    • if the edit updates the body - the body after the edit matches the current body of the post
    • if the edit adds tags - the tags added in the edit are still present on the post
    • if the edit removes tags - the tags removed in the edit are still absent from the post
  • For history hiding, you can only undo the hiding if the history is still hidden

Effectively this means that most changes where you think you should be able to undo them are undoable. Users may expect that if they add one word in the first paragraph and that word is still present, that the edit can be undone, but the implementation is not fancy enough for that.

Events that are rolled back are displayed as striked through in the history.

3. Rolling back to version

In the version overview you now have a new button for rolling back to a version:
Post History Overview

If you click it, you are presented with an overview of the changes that you are about to make:

Rollback overview part 1 Rollback overview part 2

The events which are getting rolled back are striked through and red. The event that you are rolling back to is green and underlined. Unaffected events are displayed as normal.

Underneath a summary of changes is made with diffs of title, body, tags.

The way this is implemented is as follows:

  1. Determine all the events that happened since the event we are trying to revert to
  2. Go through these events one by one to aggregate what the effect of rolling each one back would be
  3. Trigger normal post update (post edit + edit event)

Events that were rolled back due to the revertion to a previous state remain striked through in the history.

Caveat: It is not feasible to determine up front whether the user will be allowed to revert to a particular event. The computations per event are quite complex, and doing it for each event would mean the load time of the history page becomes too long (or needs a much smarter solution). Therefore, all users will be presented the option, and once you click it, we will check what that set of changes would be and whether that set of changes is actually allowed for you.

Implementation Details

Permission checks

Permission checks are done using PostHistoryHelper, broken up into a disallow_<action> for each action to perform. The permission checks here are essentially copied from the posts controller.

The reason these are defined as a disallowed rather than as an allowed, is that we need to get the reason it is disallowed. It returns the message of the disallowance, or nil if not disallowed (i.e. allowed).

Post History changes

  • Added can_undo? which determines whether the post is compatible with the event according to the rules defined above.
  • Added find_predecessor which is able to find the closest predecessor event of a specific type. This is necessary to determine from when until when history should be hidden/revealed
  • Added reverted?, which returns whether this history item is reverted.

Rolling back history hiding

For rolling back history hiding, we need to determine the set of history events which happened before the history hiding we are rolling back, but after any history items coming before them. Consider for example the following situation:

#5, history hidden
#4, post edited, normal edit - accidental redaction
#3, history hidden
#2, post edited, removed PII
#1, initial revision

In this case, if we rollback (undo) #5, we would want the history of #4 and #3 to become visible, but want the history of #2 and #1 to remain hidden. This is what the code does.

A sidenote here, when a redaction is made, it first creates a post edit and then a history hidden. This happens within the same second (usually). This is a potential problem for the unhiding of history. In our example, we need to unhide #3 but not #2, even though both are created at the same time. To address this, we add a second for selection of events, and add an or to still get event #3 in our set of things to reveal.

WIP

  • Rollback history reveal implementation
  • Test undo
    • Edits
    • History hidden undo
    • History reveal undo
  • Test edit rollback

Fixes #215
Fixes #340

Taeir added 29 commits July 21, 2023 15:32
Comments are now rendered as markdown, to allow inclusion of e.g. links.

Fixes codidact#215
Also add a long dash to separate the "Copy Link" from the comment.
I've implemented it in a "simple" way. Something can only be rolled back
if its changes are still present. E.g. you cannot rollback something
that adds a tag that already is no longer present.

Left to figure out:
* How to restore close reasons
* Restrict rollback to only the "most recent" of a particular type?
* Whether we want to do proper linking/tracking of the rollback rather
  than the comment approach
In this column we can store variable data for different types of
history.
For existing post closed history, add the extra info.
Instead of recording a special type, record the inverse of a type in the
post history, but keep the extras associated with a rollback.
Located in post_history_helper#disallow_rollback.
Renamed the button to undo
@Taeir Taeir changed the title Revert to state support Better History Control - Rollbacks and Revertions Aug 1, 2023
Taeir added 30 commits August 6, 2023 14:42
- Use helper methods where possible
- Determine permissions based on changes to be made rather than the
  individual events
Previously, it could contain changes (which came from the aggregation of
history) that would not actually need to be made to the post.
This required nullifying the reverted with items
- Fix inner divs
- Deal with case where post remains closed but close reason changes
- Simplify code
Use the PostActions in both the posts_controller and the
post_history_controller to share the logic for permission checks and
updating behavior.

Consistently use the nomenclature of rollback to.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support rollbacks in edit history Record close reason in history
1 participant