Forms with task lists can be overwritten when editing simultaneously
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?
-
Changelog entry added, if necessary [ ] Documentation created/updated-
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides [ ] Conforms to the database guides-
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer