Skip to content

Forms with task lists can be overwritten when editing simultaneously

Brett Walker requested to merge 19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously into master

What does this MR do?

Scenario

User A and user B are viewing the same MR/issue, that has a checkbox item. User A edits the MR/issue description and adds new text, then saves it. User B then checks the checkbox. Since user B has the old description and we send the whole thing to the server, it overwrites what user A did.

Result: loss of edits made by user A

Solution

Supplying the lock_version when updating the checkbox, as outlined by @deckar01 , will solve the first scenario, the most critical one. If the lock_version doesn't match, we can't save the data, and let the user know to either reload the page or automatically re-render with the updated data. They then have to click the checkbox again.

If many people are clicking the boxes in a large MR checklist (which we use frequently) there is the possibility of having to click your checkbox several times, as several people could update the DB underneath you. It does stop the dataloss in the third scenario, but it's not ideal at all.

Scenario

User A and user B are viewing the same MR/issue, that has many many checkbox items. Both need to check off different items at the same time. However, since each one is checking a different item and sending the entire description to the server, they end up overwriting each others checks. Also, performance in checking each box is very slow as the entire markdown description must be re-rendered and displayed.

Solution

The line number of the markdown source associated with the checkbox is calculated in the UI. That and the text of the single source line is passed to the backend. We then update that specific line in the source markdown. Using the SOURCEPOS option in the CommonMark parser, we get the specific html entry in the cached html and update the checkbox. We are then able to update both the markdown and the cached html.

We don't use the lock_version in this case - the description can change in a variety of ways - as long as the exact text still exists on the specific line, we can save the checkbox.

However, since RedCarpet doesn't support source lines, we fallback to using a checkbox indexing method (which checkbox on the page) similar to what is used in the UI

No rendering on the backend is required or checking of references, since we are only toggling a checkbox. The UI could then simply toggle the checkbox.

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/19745

Does this MR meet the acceptance criteria?

Edited by Brett Walker

Merge request reports