Forms with task lists can be overwritten when editing simultaniously

Summary

Checking a checkbox in issue body rewrites the issue content and tasks changed by another user.

Steps to reproduce

1. Create a new issue.

2. Put a * [ ] Hey! I like to DELETE all the content as an issue body and save the issue.

3. Open a new tab in your browser (don't close the first one).

4. In the new tab open the issue, then click Edit button.

5. Add some cool stuff into the issue body.

6. Get back to the first tab and click to the checkbox.

7. Refresh the issue page. You will see that everything lost you added in the step 5.

Expected behavior

Only checked checkbox is changed. Everything else must stay as it is.

Actual behavior

Checkbox rewrites all the content.

(copied from gitlab-foss#27055 (closed))

---

original description:

@DouweM :"But when person A loads the issue page, person B edits it, and then person A checks off a task, they will overwrite the change by person B"

Related discussion - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5146#note_12954629

@vsizov Can we continue to use tasklist:changed, if we just ask the user to reload the page after we hear back from the server that the change actually couldn't be saved? That way, if everything goes right, we don't need to add a latency to updating the task list.

@DouweM I considered this, it will work OK if you check/uncheck one task, but remember that we need to get new version_lock after changing previous one and use it for AJAX request for the next one!!! The best option I have at this moment is to use tasklist:changed but block every next task until we don't have response for previous one. In most cases user even won't notice it I guess. The latency will be noticeable if you check lots of tasks quickly or if your GitLab instance is overloaded or something else that makes it slow.

@vsizov I think that's an agreeable compromise: slight latency between checking two tasks off.

mentioned in issue gitlab-foss#13450 (moved)

Mentioned in issue gitlab-foss#18173 (closed)

Is this ~Frontend and ~Backend ?

Added 12980 gitlab-foss13452 gitlab-foss27047 gitlab-foss20048 labels

Yes, this is something between.

Added 480950 gitlab-foss127616 labels

I performed a quick test by supplying the lock_version to the API request generated by a tasklist change, and the backend rescued the StaleObjectError, but barfed trying to render :edit because it resolved to projects/issues/edit instead of projects/issues/2/edit.

Mentioned in merge request gitlab-foss!6968 (closed)

gitlab-foss!6968 (closed) is a proof of concept for using the lock version for task list changes.

@DouweM @vsizov In this PoC, rapid checking will actually get handled just like any other conflict, but there is probably a better UX. What do you think about debouncing the updateTaskList() call, then disabling the task list while the ajax is in flight? That should allow rapid checking, then prevent modifying the list until a new lock version is acquired.

Removed ~12980 label

That's not a bug. Optimistic lock is a feature and this task is about adding it for task list.

Added ~874211 and removed ~480950 labels

Here is what a 1500ms debounce looks like with the task list disabled in flight:

Another option is to use a promise chain to ensure that acquiring the lock version is treated as a dependency for sending another update. That would prevent the task list from needing to be disabled and wouldn't change the existing UX.

Here is what a 1500ms debounce looks like with the requests queued in a promise chain:

@DouweM @vsizov Can we add this update soon? There has been a lot of confusion specifically on onboarding issues.

@DouweM @vsizov Can this get prioritized? I keep loosing edits on issues.

@smcgivern Can we assign a milestone?

@smcgivern : Will https://gitlab.com/gitlab-org/gitlab-ce/issues/25049 alleviate some of the pain here? Especially if this is for task lists?

@victorwu It looks like you proposed prioritizing live issue descriptions in https://gitlab.com/gitlab-org/gitlab-ce/issues/24429#note_18936621. Live updates would shrink the window for editing a stale lock considerably.

It would still be nice to guarantee consistency with the lock though. I can imagine an active issue with lots of developers still racing without fixing the lock integration.

Thanks @deckar01 for the clarification. We are aiming to get live issue titles first, and then move on to live issue descriptions after. We should work on these first since that would solve most of the problems described here overall (90%+ of the time probably?). And then by that time, depending on how users are experiencing it, we might want to start investigating full-on google-docs-style real-time editing. And so I can imagine this issue being more relevant then as part of that specific feature. Do let me know if I'm missing some aspect of the problem though.

@smcgivern : Does that make sense? I do recognize the pain points of losing data. But seems like the correct solution is to solve real-time first since that gives a much more intuitive experience to end users so they know what's going on.

@vsizov what do you think? Personally I'm not sure how live updating would work with locking, but please let me know

@smcgivern I'm not sure either. We have to chose one of these two approaches.

Actually, live updating will not work with Task list feature (I hardly imagine it), so it's not about locking at all. So I would implement optimistic lock instead.

Live updating with task lists is tricky indeed @victorwu how were you envisaging this working?

Maybe it's needed to be clarified that we send the whole list in AJAX request, not the only checked item cc @victorwu

Live updating can be an extension of optimistic locking. Instead of erroring when the lock is outdated, you fall back to a merge strategy.

1. Live updates reduce the chance of conflicts
2. Locks prevent conflicts from loosing data
3. Merging resolves conflicts automatically on save
4. Live editing resolves conflicts automatically on keypress

Live updating would be nice for lots of reasons, but merging would really make the UX feel like magic.

Thanks all for the discussion. Let's make sure we follow a top-down, design-driven approach, and then talk about the different technical constraints thereafter.

We should focus on first on the issue title first, which we are likely tackling in https://gitlab.com/gitlab-org/gitlab-ce/issues/25051 and https://gitlab.com/gitlab-org/gitlab-ce/issues/25052. So let's move any relevant discussions there, and we should engage our designers there, documenting all edge cases. We can get back to the discussion soon with our designers in about a week or so once we have confirmed we want want to work on these two in 8.16.

After those two are merged / shipped, then let's tackle the harder problem of issue descriptions (https://gitlab.com/gitlab-org/gitlab-ce/issues/25049 and https://gitlab.com/gitlab-org/gitlab-ce/issues/24873). Feel free to move all discussion there. I do anticipate that these discussions here will come up in that context again and we should refer back to what we've already discussed. And we can collaborate with our designers in those issues.

cc @awhildy + @dimitrieh

I just lost a bunch of work, because of this issue. I wrote out a long QA checklist and it got wiped out with a single click on a stale tab.

mentioned in issue gitlab-foss#26105 (closed)

mentioned in issue gitlab-foss#15597 (moved)

mentioned in merge request gitlab-foss!8567 (merged)

As for a quick fix (prevents data loss and supports repeated checking/unchecking), I come up with an idea:

Add a field last_update_mark as a special lock, its value is a string like 'deckar01:check_tasklist', indicating that @deckar01 just now checked/unchecked an item of tasklist.

• When checking/unchecking a task item, it should carry lock_version.

• The backend can ignore lock_version if he checks another task item and it sees last_update_mark is 'deckar01:check_tasklist'.

• The backend cannot ignore lock_version if someone jumps in and edits something (thus changes last_update_mark to a different value).

mentioned in issue gitlab-foss#29435 (closed)

We have several times experienced that our data was lost after we started using task list. (Some of the changes are also tracked in comments so we have been able to recover the changes from there but not all eg. edits).

Hence I think this issue should be added the "bug" as well as the "data loss" label as this issue causes data loss when it overwrites new changes with old ones, as described in gitlab-foss#29435 (closed).

mentioned in issue gitlab-foss#32431 (closed)

@victorwu can we prioritize this? This is causing us a lot of grief on our CE => EE MRs because @rymai now has a script that auto generates each person to fix each file conflict with an accompanying checklist

mentioned in issue gitlab-foss#27055 (closed)

changed the description

changed title from Optimistic locking for Issue and Merge Request markdown tasks to Task checking overwrites issue content / Optimistic locking for Issue and Merge Request markdown tasks

I have changed the description to the more detailed one of gitlab-foss#27055 (closed).

Will this also fix gitlab-foss#31675?

changed the description

changed title from Task checking overwrites issue content / Optimistic locking for Issue and Merge Request markdown tasks to Forms with task lists can be overwritten when editing simultaniously

The auto description updates doesn't fix this problem? Yeah let's fix this. What's the design though?

changed milestone to %Backlog in gitlab-foss

The auto description updates doesn't fix this problem?

@victorwu I think it will fix this issue (almost completely)

UPDATE: The race condition is still possible but the probability is too low.

Thanks @vsizov . Can we close this? Or should we keep it open? What would be the flow required? When a user clicks save when making a change, the UI tells them that they would lose their changes?

@victorwu I think we should keep the issue open but we could remove the labels that prioritize it because with the "MR/Issue description auto updating" feature the issue becomes less important.

@victorwu

When a user clicks save when making a change, the UI tells them that they would lose their changes?

Something like that.

As I said above the race condition is still possible but the probability is too low. I will try to explain my point. Now we are polling the content updates every N seconds (configurable, 3-10 seconds in a majority of cases ). Let's consider a case when User A opens a form and saves the new content, in this case, the User B who checks a checkbox within 0..N seconds can overwrite the changes of User A. So the risky time is actually 0..N seconds! I propose to show something like "Your changes have not been saved as someone changed the content before you".

@vsizov : Thanks for the explanation. I want to capture all the use cases if possible. I think you mean something like this?

Scenario 1:

• @victorwu opens edit mode for the issue and starts changing the description (https://gitlab.com/gitlab-org/gitlab-ce/issues/24873).
• @vsizov makes a change to the issue description (by changing text or checking a checkbox), and it persists to the db.
• @victorwu finishes the changes and clicks Save changes.
• @victorwu remains in edit mode and sees a warning banner message saying Are you sure? Another user made changes recently. Saving would delete their changes.
• @victorwu clicks Save changes, and this wipes out @vsizov's changes.

Scenario 2:

• @victorwu is looking at an issue description in regular view mode. There is one empty checkbox.
• @vsizov is looking at the same issue description in regular view mode on a different computer.
• The refresh period is 10 seconds.
• Last refresh was at time t for both @victorwu and @vsizov .
• Time is now time t + 1 second.
• @vsizov makes some changes (any checkbox change or text change) and persists them to the db.
• Time is now time t + 2 seconds.
• @victorwu checks the empty checkbox. Immediately the system unchecks the checkbox.
• @victorwu sees a banner message saying Checkbox change not saved since another user made changes recently. Wait a few seconds and try again.
• Time is now time t + 10 seconds.
• @victorwu sees the updated changes refreshed on their screen.
• @victorwu checks the checkbox. No negative feedback. Works as normal.

Scenario 3:

• Same as Scenario 2, but starting off with a checked checkbox.

@victorwu The scenario 1 is already covered with an optimistic lock. It's not exactly like you said but this case is safe now. The 2 and 3 scenarios have to be covered yet.

@victorwu The case I described (which will not be fixed if we implement automatic MR update) is shown in the picture:

Cool thanks @vsizov ! Let's consider it for 9.4 then! So is throwing out the action of checking the checkbox the simplest thing to do? What's going on under the hood? There's a roundtrip with the BE, and during that roundtrip, the BE rejects checking the checkbox?

changed milestone to %9.4 in gitlab-foss

Yes, it should send a request for every click on checkbox.

changed milestone to %Backlog in gitlab-foss

changed milestone to %Next 2-3 months in gitlab-foss

removed to schedule in GitLab FOSS label

We ran into another side effect of the task list not bumping the lock locally: If you check a few boxes, then decide to edit the description before the live update bumps the lock version, the "Someone edited the issue at the same time you did" flash message will be displayed.

mentioned in issue gitlab-foss#37250

moved from gitlab-foss#19745 (moved)