fix ERR_CONTENT_LENGTH_MISMATCH on task checkboxes

related #24815 (moved)

@dbalexandre what do you think of this approach?
It is just a draft =)

assigned to @felipe_artur

added 1 commit

• fc393aeb - WIP

Compare with previous version

resolved all discussions

@dbalexandre Do you think we should send lock version when updating task list?
I think it makes sense to do so.

added 1 commit

• 61319aa3 - Fix update merge request task list throwing stale object

Compare with previous version

@iamphill

All the functions that are related to task lists disableTaskList initTaskList updateTasklist are
duplicated on notes.js, issue.js and merge_request.js can we refactor them to be on just one class?
I was wondering to do this on this MR but not sure if you are going to change these files entirely.

cc @jschatz1

@felipe_artur i'm not aware of any refactor planned. I guess we could create a technical debt issue & tackle in when we can.

Alright i think that would be the ideal.

Did this issue get created? What's its ID?

Just did: https://gitlab.com/gitlab-org/gitlab-ce/issues/27246

assigned to @smcgivern

unmarked as a Work In Progress

I think it is better to send lock version here since editing the tasklist is editting the description

Yes but when we send second query from Task List it should already contain new lock_version that is not available on the page (it's still on the server), see https://gitlab.com/gitlab-org/gitlab-ce/issues/19745#note_13028305

added 1 commit

• fb332e5b - Fix update merge request task list throwing stale object

Compare with previous version

added 1 commit

• 02ba7bcb - Catch json stale object errors on merge requests controller

Compare with previous version

@vsizov what do you think of this? Could it also be used for https://gitlab.com/gitlab-org/gitlab-ce/issues/26746?

assigned to @vsizov

@smcgivern To answer this question I need to investigate #26746 (moved) first. At first glance it's not clear what happens there because assignee field does not bump lock_version column (at least it should not, by design). I will investigate it when we fix all Next Patch Release issues. Anyway, I don't think it make sense to block this MR as it looks good to me.

assigned to @smcgivern

@felipe_artur thanks, nicely done! Does this only apply to MRs, or do we need to do it on issues too?

assigned to @felipe_artur

changed milestone to %8.17

@smcgivern Yes. We have the same problem with issues, no json edit template. I will make the proper changes.

added 1 commit

• 87d39c48 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

added 1 commit

• 721a6053 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

added 681 commits

• 721a6053...e74c6ae6 - 680 commits from branch master
• 5d38dcf9 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

added 1 commit

• b6dd412c - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

@felipe_artur What was the root of the problem? When we change TaskList it sends PATCH request to the server via AJAX and on that particular MR that is described in the issue the ActiveRecord::StaleObjectError exception had been raising during updating an issuable. The question is why it's raising?

Have you also seen this issue https://gitlab.com/gitlab-org/gitlab-ce/issues/19745 ?

I feel like this MR is intended to fix #19745 (moved) but not #24815 (moved)

Also this MR breaks the Task List feature, see

I described this problem in issue #19745 (moved) few months ago, this is why we have not fixed it yet because it's not that trivial task. We need to find a solution first.

So the summary is: This MR is absolutely needed and it's intended to fix #19745 (moved) but it's only half of the work.

@vsizov The problem was caused by a migration that set lock_version to 0 in some resources including the merge request
on #24815 (moved) description.

We were throwing 500 because when updating task lists we did not have an :edit json template which is the problem described here: https://gitlab.com/gitlab-org/gitlab-ce/issues/19745#note_17110900, this is the main purpose of the MR.

I also tried to solve the locking problem with tasklists the usual way(optimistic locking but i forgot to update the lock version), but i see that there is still some discussion about which approach to take on #19745 (moved) .

Should we stick only with the template fix for now?

Also the person that fix the locking problem probably should also fix this one: https://gitlab.com/gitlab-org/gitlab-ce/issues/27246
It will make things easier.

We are waiting our new teammate which will join soon(feb 6th) to continue with this one.

cc @jschatz1

The problem was caused by a migration that set lock_version to 0

Do we already have the fix for that? I mean migration that fixes #24815 (moved) . I think it's all we need to fix that issue then.

@vsizov I saw an issue in 8.16 related to that: https://gitlab.com/gitlab-org/gitlab-ce/issues/25228

Checkout https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6968 for a PoC of passing the lock version in the response.

@smcgivern is this %9.0 now?

@felipe_artur what happened with this?

Waiting for this https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9016/diffs so we can display the error message on for all issuables, then we can merge.

After this MR we should also consider implementing the POC https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6968

marked as a Work In Progress

added 2356 commits

• b6dd412c...61e2a3eb - 2355 commits from branch master
• a1991c9f - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

added 1 commit

• cb59aed5 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

added 1 commit

• 48270995 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

unmarked as a Work In Progress

@vsizov This does not closes #24815 (moved) but it is necessary. We are throwing 500 for stable object errors.

Could you take a look?

assigned to @vsizov

LGTM

Spec failure looks related https://gitlab.com/gitlab-org/gitlab-ce/builds/11045494

added 1 commit

• 4c8e333e - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

Thanks @vsizov

assigned to @smcgivern

added 1 commit

• 0d000d35 - Fix issuable stale object error handler for js when updating tasklists

Compare with previous version

Thanks @felipe_artur! The downtime check failure is unrelated.

changed milestone to %9.0

merged