Merge Request Notes in a project migrated from Github cannot be resolved.

Update

17.05.2023

I'm adjusting this issue description to focus on problem and solution that can be resolved on the Frontend. @krisberry-ext will create a separate issue to remove LegacyDiffNote, as changes affect many places in GitLab, not only GitHub importer.

In this way we separate two problems and can better focus on appropriate solutions.

Problem

From the @krisberry-ext note:

All projects imported from GitHub have two notes types DiffNote and LegacyDiffNote. Only comments with suggestions (and if the commit is still present on the GitHub side) are imported with DiffNotes that are RESOLVABLE. All other comments will not have a resolve button.
The count of unresolved threads is correct. It counts only DiffNotes that are resolvable. However, navigation buttons cause confusion as they navigate to the nearest note and go through all of the notes instead of unresolved ones only.
The resolve button is seen for users who meet conditions from https://docs.gitlab.com/ee/user/discussions/#resolve-a-thread and only on DiffNotes

Proposed solution

Navigation button should go only over resolvable and unresolved comments.

@iamphill pointed out here how it can be fixed.

As an outcome, an MR should be mergeable when resolvable threads are resolved, also when the restriction that "all threads must be resolved to merge" is on.

------------- Below the original issue description

Summary

Notes for Merge Requests that were migrated from GitHub, are being identified as needing resolution in the merge request. These are blocking the merge when all threads need to be resolved, but they cannot be resolved and have "resolvable": false in the note data.

Steps to reproduce

Visit tgdp/request-for-comment!31 (merged)
Switch to the 'Changes' tab.
See '5 unresolved threads'.
Click the down arrow to scroll to the first note with the body text 'Agree with this!'.
See that there isn't a 'Resolve thread' button in the reply field.
Click into the 'Reply...' field and see that the UI element for text entry disappears and see the browser Console log for script errors.

Another bit of information through the API.

Query this API endpoint: https://gitlab.com/api/v4/projects/39683039/merge_requests/31/notes/1112929474
See that the parameter "resolvable" is set to false.

Example Project

I can't make an example since this seems to be related to importing the project from Github. This is one of the Merge Requests that demonstrate this issue.

tgdp/request-for-comment!31 (merged)

What is the current bug behavior?

These unresolvable notes are showing as unresolved threads and blocking merge when the preference to require threads to be resolved before merge is enabled. We cannot resolve these threads in the UI and cannot update the Note data through the API to allow it to be resolved.

We had to disable the requirement to resolve threads on merge requests while we finish working through the set of merge requests that were imported from Github and have comments/notes with this problem.

New comments on these MRs do not show this problem behavior and can be resolved as expected.

What is the expected correct behavior?

Either do not flag these notes as threads that need to be resolved, or allow them to be replied to and resolved in the UI.

Relevant logs and/or screenshots

Console Log Errors after clicking Reply... in the screenshot above.

instrument.js:109 TypeError: Cannot read properties of null (reading 'base_sha')
    at a.markdownPreviewPath (note_form.vue:178:1)
    at dn.get (vue.runtime.esm.js:4495:25)
    at dn.evaluate (vue.runtime.esm.js:4597:21)
    at a.markdownPreviewPath (vue.runtime.esm.js:4851:17)
    at a.<anonymous> (note_form.vue?6f7c:3:511)
    at t._render (vue.runtime.esm.js:3569:22)
    at a.r (vue.runtime.esm.js:4081:21)
    at dn.get (vue.runtime.esm.js:4495:25)
    at new dn (vue.runtime.esm.js:4484:12)
    at t (vue.runtime.esm.js:4088:3)
    at En.$mount (vue.runtime.esm.js:8459:10)
    at init (vue.runtime.esm.js:3137:13)
    at vue.runtime.esm.js:6022:9
    at f (vue.runtime.esm.js:5969:9)
    at vue.runtime.esm.js:6260:11
    at C (vue.runtime.esm.js:6363:29)
    at vue.runtime.esm.js:6237:9
    at C (vue.runtime.esm.js:6363:29)
    at vue.runtime.esm.js:6237:9
    at C (vue.runtime.esm.js:6363:29)
    at vue.runtime.esm.js:6237:9
    at C (vue.runtime.esm.js:6363:29)
    at vue.runtime.esm.js:6237:9
    at C (vue.runtime.esm.js:6363:29)
    at a.__patch__ (vue.runtime.esm.js:6526:9)
    at t._update (vue.runtime.esm.js:3963:19)
    at a.r (vue.runtime.esm.js:4081:10)
    at dn.get (vue.runtime.esm.js:4495:25)
    at dn.run (vue.runtime.esm.js:4570:22)
    at ln (vue.runtime.esm.js:4326:13)
    at Array.<anonymous> (vue.runtime.esm.js:1989:12)
    at Jt (vue.runtime.esm.js:1915:12)

instrument.js:109 TypeError: Cannot read properties of undefined (reading 'focus')
    at a.mounted (note_form.vue:228:1)
    at Ht (vue.runtime.esm.js:1863:57)
    at Ke (vue.runtime.esm.js:4235:7)
    at Object.insert (vue.runtime.esm.js:3158:7)
    at j (vue.runtime.esm.js:6390:28)
    at a.__patch__ (vue.runtime.esm.js:6609:5)
    at t._update (vue.runtime.esm.js:3963:19)
    at a.r (vue.runtime.esm.js:4081:10)
    at dn.get (vue.runtime.esm.js:4495:25)
    at dn.run (vue.runtime.esm.js:4570:22)
    at ln (vue.runtime.esm.js:4326:13)
    at Array.<anonymous> (vue.runtime.esm.js:1989:12)
    at Jt (vue.runtime.esm.js:1915:12)

Here is the example Note data that we cannot resolve.

{
    "id": 1112929474,
    "type": "LegacyDiffNote",
    "body": "Agree with this!",
    "attachment": null,
    "author": {
        "id": 11406411,
        "username": "kickoke",
        "name": "Tina Lüdtke",
        "state": "active",
        "avatar_url": "https://gitlab.com/uploads/-/system/user/avatar/11406411/avatar.png",
        "web_url": "https://gitlab.com/kickoke"
    },
    "created_at": "2022-09-14T18:43:08.000Z",
    "updated_at": "2022-11-03T21:14:29.255Z",
    "system": false,
    "noteable_id": 179035674,
    "noteable_type": "MergeRequest",
    "resolvable": false,
    "confidential": false,
    "internal": false,
    "noteable_iid": 31,
    "commands_changes": {}
}

Output of checks

This bug happens on GitLab.com using the standard and Next (canary) infrastructure.

Results of GitLab environment info

Expand for output related to GitLab environment info


(For installations with omnibus-gitlab package run and paste the output of:
`sudo gitlab-rake gitlab:env:info`)

(For installations from source run and paste the output of:
`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)

Results of GitLab application Check

Expand for output related to the GitLab application check

(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true)
(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)
(we will only investigate if the tests are passing)

Possible fixes

Edited May 22, 2023 by Magdalena Frankiewicz