diff --git a/src/data_providers/items/mr_item_model.test.ts b/src/data_providers/items/mr_item_model.test.ts index 214790781e39c2f9d615ebeb1c1edfca57415a65..047b4148760fe00dfcab50e92c11e772c15011f9 100644 --- a/src/data_providers/items/mr_item_model.test.ts +++ b/src/data_providers/items/mr_item_model.test.ts @@ -1,6 +1,6 @@ import * as vscode from 'vscode'; import { MrItemModel } from './mr_item_model'; -import { mr } from '../../test_utils/entities'; +import { mr, repository } from '../../test_utils/entities'; import { discussionOnDiff, noteOnDiffTextSnippet, @@ -8,6 +8,7 @@ import { } from '../../../test/integration/fixtures/graphql/discussions.js'; import { CommentingRangeProvider } from '../../review/commenting_range_provider'; import { createWrappedRepository } from '../../test_utils/create_wrapped_repository'; +import { fromReviewUri } from '../../review/review_uri'; const createCommentControllerMock = vscode.comments.createCommentController as jest.Mock; @@ -50,8 +51,7 @@ describe('MrItemModel', () => { 'gitlab-mr-gitlab-org/gitlab!2000', 'Issuable Title', ); - const [uri, range] = createCommentThreadMock.mock.calls[0]; - expect(uri.path).toBe('src/webview/src/components/LabelNoteOld.vue'); + const [_, range] = createCommentThreadMock.mock.calls[0]; expect(range.start.line).toBe(47); expect(commentThread.comments.length).toBe(1); const firstComment = commentThread.comments[0]; @@ -60,6 +60,20 @@ describe('MrItemModel', () => { expect(firstComment.body).toMatch(noteOnDiffTextSnippet); }); + it('should associate the thread with the correct URI', async () => { + await item.getChildren(); + + const [uri] = createCommentThreadMock.mock.calls[0]; + const { mrId, projectId, repositoryRoot, commit, path } = fromReviewUri(uri); + expect(mrId).toBe(mr.id); + expect(projectId).toBe(mr.project_id); + expect(repositoryRoot).toBe(repository.rootFsPath); + + const discussionPosition = discussionOnDiff.notes.nodes[0].position; + expect(commit).toBe(discussionPosition.diffRefs.baseSha); + expect(path).toBe(discussionPosition.oldPath); + }); + describe('commenting range', () => { it('should not add a commenting range provider if user does not have permission to comment', async () => { canUserCommentOnMr = false; diff --git a/src/data_providers/items/mr_item_model.ts b/src/data_providers/items/mr_item_model.ts index 7f1834c39c18d4e036ff62284a4d2ec87728a5a8..c50e1df45f33d248e36c2b0720d222a43c647cea 100644 --- a/src/data_providers/items/mr_item_model.ts +++ b/src/data_providers/items/mr_item_model.ts @@ -1,4 +1,5 @@ import * as vscode from 'vscode'; +import * as assert from 'assert'; import { PROGRAMMATIC_COMMANDS } from '../../command_names'; import { ChangedFileItem } from './changed_file_item'; import { ItemModel } from './item_model'; @@ -9,12 +10,40 @@ import { GitLabCommentThread } from '../../review/gitlab_comment_thread'; import { CommentingRangeProvider } from '../../review/commenting_range_provider'; import { WrappedRepository } from '../../git/wrapped_repository'; import { commentControllerProvider } from '../../review/comment_controller_provider'; +import { GqlTextDiffNote } from '../../gitlab/graphql/shared'; +import { toReviewUri } from '../../review/review_uri'; +import { + commentRangeFromPosition, + commitFromPosition, + pathFromPosition, +} from '../../review/gql_position_parser'; const isTextDiffDiscussion = (discussion: GqlDiscussion): discussion is GqlTextDiffDiscussion => { const firstNote = discussion.notes.nodes[0]; return firstNote?.position?.positionType === 'text'; }; +const firstNoteFrom = (discussion: GqlTextDiffDiscussion): GqlTextDiffNote => { + const note = discussion.notes.nodes[0]; + assert(note, 'discussion should contain at least one note'); + return note; +}; + +const uriForDiscussion = ( + repository: WrappedRepository, + mr: RestMr, + discussion: GqlTextDiffDiscussion, +): vscode.Uri => { + const { position } = firstNoteFrom(discussion); + return toReviewUri({ + path: pathFromPosition(position), + commit: commitFromPosition(position), + repositoryRoot: repository.rootFsPath, + projectId: mr.project_id, + mrId: mr.id, + }); +}; + export class MrItemModel extends ItemModel { constructor(readonly mr: RestMr, readonly repository: WrappedRepository) { super(); @@ -76,13 +105,20 @@ export class MrItemModel extends ItemModel { }); const discussionsOnDiff = discussions.filter(isTextDiffDiscussion); discussionsOnDiff.forEach(discussion => { - return GitLabCommentThread.createThread({ - commentController, - repositoryRoot: this.repository.rootFsPath, - mr: this.mr, + const { position } = firstNoteFrom(discussion); + const vsThread = commentController.createCommentThread( + uriForDiscussion(this.repository, this.mr, discussion), + commentRangeFromPosition(position), + // the comments need to know about the thread, so we first + // create empty thread to be able to create comments + [], + ); + return new GitLabCommentThread( + vsThread, discussion, - gitlabService, - }); + this.repository.getGitLabService(), + this.mr, + ); }); } } diff --git a/src/review/gitlab_comment_thread.test.ts b/src/review/gitlab_comment_thread.test.ts index a00f4295e078297603b6964af8c4cbb211ac28dc..f4fbc3c3407ba2acffa014e244827ccf15cd7e9c 100644 --- a/src/review/gitlab_comment_thread.test.ts +++ b/src/review/gitlab_comment_thread.test.ts @@ -6,12 +6,9 @@ import { } from '../../test/integration/fixtures/graphql/discussions.js'; import { GitLabComment } from './gitlab_comment'; import { GitLabNewService } from '../gitlab/gitlab_new_service'; -import { mr, project } from '../test_utils/entities'; -import { GqlTextDiffNote, GqlTextPosition } from '../gitlab/graphql/shared'; +import { mr } from '../test_utils/entities'; +import { GqlTextDiffNote } from '../gitlab/graphql/shared'; import { GqlTextDiffDiscussion } from '../gitlab/graphql/get_discussions'; -import { fromReviewUri } from './review_uri'; - -const TEST_ROOT = '/repositoryRoot'; describe('GitLabCommentThread', () => { let gitlabCommentThread: GitLabCommentThread; @@ -44,21 +41,13 @@ describe('GitLabCommentThread', () => { }; const createGitLabCommentThread = (discussion: GqlTextDiffDiscussion) => { - const fakeCommentController: vscode.CommentController = { - id: 'id', - label: 'label', - dispose: () => undefined, - createCommentThread: (uri, range, comments) => { - vsCommentThread = { - uri, - range, - comments, - collapsibleState: vscode.CommentThreadCollapsibleState.Collapsed, - canReply: true, - dispose: jest.fn(), - }; - return vsCommentThread; - }, + vsCommentThread = { + uri: {} as any, + range: {} as any, + comments: {} as any, + collapsibleState: vscode.CommentThreadCollapsibleState.Collapsed, + canReply: true, + dispose: jest.fn(), }; gitlabService = ({ setResolved: jest.fn(), @@ -66,13 +55,7 @@ describe('GitLabCommentThread', () => { updateNoteBody: jest.fn(), createNote: jest.fn(), } as unknown) as GitLabNewService; - gitlabCommentThread = GitLabCommentThread.createThread({ - commentController: fakeCommentController, - repositoryRoot: TEST_ROOT, - mr, - discussion, - gitlabService, - }); + gitlabCommentThread = new GitLabCommentThread(vsCommentThread, discussion, gitlabService, mr); }; beforeEach(() => { @@ -83,64 +66,6 @@ describe('GitLabCommentThread', () => { expect(vsCommentThread.collapsibleState).toBe(vscode.CommentThreadCollapsibleState.Expanded); }); - it('takes position from the first note', () => { - expect(vsCommentThread.range.start.line).toBe(noteOnDiff.position.oldLine - 1); // vs code numbers lines from 0 - }); - - describe('new thread URI', () => { - it('populates basic uri parameters', () => { - const { projectId, repositoryRoot, mrId } = fromReviewUri(vsCommentThread.uri); - - expect(projectId).toBe(mr.project_id); - expect(mrId).toBe(mr.id); - expect(repositoryRoot).toBe(TEST_ROOT); - }); - - it('generates uri for discussion on old diff version', () => { - createGitLabCommentThread( - createGqlTextDiffDiscussion({ - ...noteOnDiff, - position: { - ...noteOnDiff.position, - oldLine: 10, // comment is on the old version of the diff - newLine: null, - oldPath: 'old/path.js', - } as GqlTextPosition, - }), - ); - - const { commit, path } = fromReviewUri(vsCommentThread.uri); - - expect(commit).toBe(noteOnDiff.position.diffRefs.baseSha); - expect(path).toBe('old/path.js'); - }); - - it('generates uri for discussion on new diff version', () => { - createGitLabCommentThread( - createGqlTextDiffDiscussion({ - ...noteOnDiff, - position: { - ...noteOnDiff.position, - oldLine: null, - newLine: 10, // comment is on the new version of the diff - newPath: 'new/path.js', - } as GqlTextPosition, - }), - ); - - const { commit, path } = fromReviewUri(vsCommentThread.uri); - - expect(commit).toBe(noteOnDiff.position.diffRefs.headSha); - expect(path).toBe('new/path.js'); - }); - }); - - it('generates uri for the discussion', () => { - // TODO: improve the uri tests once we merge https://gitlab.com/gitlab-org/gitlab-vscode-extension/-/merge_requests/192 - const { baseSha } = noteOnDiff.position.diffRefs; // baseSha points to the commit that the MR started from (old lines) - expect(vsCommentThread.uri.query).toMatch(baseSha); - }); - describe('allowing replies to the thread', () => { const createNoteAndSetCreatePermissions = (createNote: boolean): GqlTextDiffNote => ({ ...(noteOnDiff as GqlTextDiffNote), diff --git a/src/review/gitlab_comment_thread.ts b/src/review/gitlab_comment_thread.ts index c975968ba1c94f267c82ee5627938fd26f7055e3..c9f49b2bec09d21a24514786f42e6683b67412e8 100644 --- a/src/review/gitlab_comment_thread.ts +++ b/src/review/gitlab_comment_thread.ts @@ -2,10 +2,8 @@ import * as vscode from 'vscode'; import * as assert from 'assert'; import { GitLabNewService } from '../gitlab/gitlab_new_service'; import { GitLabComment } from './gitlab_comment'; -import { toReviewUri } from './review_uri'; import { GqlTextDiffDiscussion } from '../gitlab/graphql/get_discussions'; -import { GqlNote, GqlTextDiffNote, GqlTextPosition } from '../gitlab/graphql/shared'; -import { commitFromPosition, pathFromPosition } from './gql_position_parser'; +import { GqlNote, GqlTextDiffNote } from '../gitlab/graphql/shared'; const firstNoteFrom = (discussion: GqlTextDiffDiscussion): GqlTextDiffNote => { const note = discussion.notes.nodes[0]; @@ -17,20 +15,6 @@ const isDiffNote = (note: GqlNote): note is GqlTextDiffNote => { return Boolean(note.position && note.position.positionType === 'text'); }; -const commentRangeFromPosition = (position: GqlTextPosition): vscode.Range => { - const glLine = position.oldLine ?? position.newLine; - const vsPosition = new vscode.Position(glLine - 1, 0); // VS Code numbers lines starting with 0, GitLab starts with 1 - return new vscode.Range(vsPosition, vsPosition); -}; - -interface CreateThreadOptions { - commentController: vscode.CommentController; - repositoryRoot: string; - mr: RestMr; - discussion: GqlTextDiffDiscussion; - gitlabService: GitLabNewService; -} - export class GitLabCommentThread { private resolved: boolean; @@ -124,28 +108,4 @@ export class GitLabCommentThread { this.vsThread.contextValue = this.resolved ? 'resolved' : 'unresolved'; } } - - static createThread({ - commentController, - repositoryRoot, - mr, - discussion, - gitlabService, - }: CreateThreadOptions): GitLabCommentThread { - const { position } = firstNoteFrom(discussion); - const vsThread = commentController.createCommentThread( - toReviewUri({ - path: pathFromPosition(position), - commit: commitFromPosition(position), - repositoryRoot, - projectId: mr.project_id, - mrId: mr.id, - }), - commentRangeFromPosition(position), - // the comments need to know about the thread, so we first - // create empty thread to be able to create comments - [], - ); - return new GitLabCommentThread(vsThread, discussion, gitlabService, mr); - } } diff --git a/src/review/gql_position_parser.test.ts b/src/review/gql_position_parser.test.ts index 072743f016e196a6b39eb36f5cd02eee25fdc1d3..8ce7d454a2bce234ac29d9f18052c548e30b44bb 100644 --- a/src/review/gql_position_parser.test.ts +++ b/src/review/gql_position_parser.test.ts @@ -1,4 +1,9 @@ -import { commitFromPosition, pathFromPosition } from './gql_position_parser'; +import * as vscode from 'vscode'; +import { + commentRangeFromPosition, + commitFromPosition, + pathFromPosition, +} from './gql_position_parser'; import { noteOnDiff } from '../../test/integration/fixtures/graphql/discussions.js'; import { GqlTextPosition } from '../gitlab/graphql/shared'; @@ -6,7 +11,7 @@ const { position } = noteOnDiff; const oldPosition = { ...position, - oldLine: 1, + oldLine: 5, newLine: null, oldPath: 'oldPath.js', diffRefs: { @@ -18,7 +23,7 @@ const oldPosition = { const newPosition = { ...position, oldLine: null, - newLine: 1, + newLine: 20, newPath: 'newPath.js', diffRefs: { ...position.diffRefs, @@ -43,3 +48,14 @@ describe('commitFromPosition', () => { expect(commitFromPosition(newPosition)).toBe('1234'); }); }); + +describe('commentRangeFromPosition', () => { + it('returns range with old line', () => { + const line = new vscode.Position(4, 0); + expect(commentRangeFromPosition(oldPosition)).toEqual(new vscode.Range(line, line)); + }); + it('returns headSha for new position', () => { + const line = new vscode.Position(19, 0); + expect(commentRangeFromPosition(newPosition)).toEqual(new vscode.Range(line, line)); + }); +}); diff --git a/src/review/gql_position_parser.ts b/src/review/gql_position_parser.ts index 5a0fdcbaf6c270351ff449648233120402568074..1f85b59b05299e675f2abb5d3b7b3cfe8e932992 100644 --- a/src/review/gql_position_parser.ts +++ b/src/review/gql_position_parser.ts @@ -1,3 +1,4 @@ +import * as vscode from 'vscode'; import { GqlTextPosition } from '../gitlab/graphql/shared'; const isOld = (position: GqlTextPosition) => position.oldLine !== null; @@ -9,3 +10,9 @@ export const pathFromPosition = (position: GqlTextPosition): string => { export const commitFromPosition = (position: GqlTextPosition): string => { return isOld(position) ? position.diffRefs.baseSha : position.diffRefs.headSha; }; + +export const commentRangeFromPosition = (position: GqlTextPosition): vscode.Range => { + const glLine = position.oldLine ?? position.newLine; + const vsPosition = new vscode.Position(glLine - 1, 0); // VS Code numbers lines starting with 0, GitLab starts with 1 + return new vscode.Range(vsPosition, vsPosition); +};