From 47f244bc2252b9faacc31d1007d4c1e1d65c0e9d Mon Sep 17 00:00:00 2001 From: Tomas Vik Date: Wed, 30 Jun 2021 07:07:15 +0000 Subject: [PATCH] feat: indicate which changed files have MR discussions --- src/__mocks__/vscode.js | 6 +- src/constants.ts | 3 + .../items/changed_file_item.test.ts | 28 +++++- src/data_providers/items/changed_file_item.ts | 94 +++++++++++-------- .../items/mr_item_model.test.ts | 21 ++--- src/data_providers/items/mr_item_model.ts | 47 +++++++--- src/extension.js | 6 +- ...> change_type_decoration_provider.test.ts} | 8 +- ....ts => change_type_decoration_provider.ts} | 6 +- .../has_comments_decoration_provider.test.ts | 18 ++++ .../has_comments_decoration_provider.ts | 16 ++++ src/test_utils/uri.ts | 2 +- 12 files changed, 177 insertions(+), 78 deletions(-) rename src/review/{file_decoration_provider.test.ts => change_type_decoration_provider.test.ts} (56%) rename src/review/{file_decoration_provider.ts => change_type_decoration_provider.ts} (76%) create mode 100644 src/review/has_comments_decoration_provider.test.ts create mode 100644 src/review/has_comments_decoration_provider.ts diff --git a/src/__mocks__/vscode.js b/src/__mocks__/vscode.js index 5e7d203..bac8396 100644 --- a/src/__mocks__/vscode.js +++ b/src/__mocks__/vscode.js @@ -2,8 +2,10 @@ const { Uri } = require('../test_utils/uri'); const { EventEmitter } = require('../test_utils/event_emitter'); module.exports = { - TreeItem: function TreeItem(label, collapsibleState) { - return { label, collapsibleState }; + TreeItem: function TreeItem(labelOrUri, collapsibleState) { + return typeof labelOrUri === 'string' + ? { label: labelOrUri, collapsibleState } + : { resourceUri: labelOrUri, collapsibleState }; }, ThemeIcon: function ThemeIcon(id) { return { id }; diff --git a/src/constants.ts b/src/constants.ts index 0d79903..e672629 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -9,3 +9,6 @@ export const MODIFIED = 'modified'; export const DO_NOT_SHOW_VERSION_WARNING = 'DO_NOT_SHOW_VERSION_WARNING'; // NOTE: This needs to _always_ be a 3 digits export const MINIMUM_VERSION = '13.5.0'; + +export const CHANGE_TYPE_QUERY_KEY = 'changeType'; +export const HAS_COMMENTS_QUERY_KEY = 'hasComments'; diff --git a/src/data_providers/items/changed_file_item.test.ts b/src/data_providers/items/changed_file_item.test.ts index 8f02080..07afbf2 100644 --- a/src/data_providers/items/changed_file_item.test.ts +++ b/src/data_providers/items/changed_file_item.test.ts @@ -1,4 +1,5 @@ import { PROGRAMMATIC_COMMANDS } from '../../command_names'; +import { CHANGE_TYPE_QUERY_KEY, HAS_COMMENTS_QUERY_KEY } from '../../constants'; import { diffFile, mr, mrVersion } from '../../test_utils/entities'; import { ChangedFileItem } from './changed_file_item'; @@ -8,9 +9,34 @@ describe('ChangedFileItem', () => { 'should not show diff for %s', extension => { const changedImageFile = { ...diffFile, new_path: `file${extension}` }; - const item = new ChangedFileItem(mr, mrVersion, changedImageFile, '/repository/fsPath'); + const item = new ChangedFileItem(mr, mrVersion, changedImageFile, '/repo', () => false); + expect(item.command?.command).toBe(PROGRAMMATIC_COMMANDS.NO_IMAGE_REVIEW); }, ); + + it('should indicate change type', () => { + const changedImageFile = { ...diffFile, new_path: `file.jpg` }; + const item = new ChangedFileItem(mr, mrVersion, changedImageFile, '/repo', () => false); + + expect(item.resourceUri?.query).toContain(`${CHANGE_TYPE_QUERY_KEY}=`); + }); + }); + + describe('captures whether there are comments on the changes', () => { + let areThereChanges: boolean; + + const createItem = () => + new ChangedFileItem(mr, mrVersion, diffFile, '/repository/fsPath', () => areThereChanges); + + it('indicates there are comments', () => { + areThereChanges = true; + expect(createItem().resourceUri?.query).toMatch(`${HAS_COMMENTS_QUERY_KEY}=true`); + }); + + it('indicates there are no comments', () => { + areThereChanges = false; + expect(createItem().resourceUri?.query).toMatch(`${HAS_COMMENTS_QUERY_KEY}=false`); + }); }); }); diff --git a/src/data_providers/items/changed_file_item.ts b/src/data_providers/items/changed_file_item.ts index 287f2e2..e58b91c 100644 --- a/src/data_providers/items/changed_file_item.ts +++ b/src/data_providers/items/changed_file_item.ts @@ -1,10 +1,18 @@ -import { TreeItem, Uri } from 'vscode'; +import * as vscode from 'vscode'; import { posix as path } from 'path'; import { toReviewUri, ReviewParams } from '../../review/review_uri'; import { PROGRAMMATIC_COMMANDS, VS_COMMANDS } from '../../command_names'; -import { ADDED, DELETED, RENAMED, MODIFIED } from '../../constants'; +import { + ADDED, + DELETED, + RENAMED, + MODIFIED, + CHANGE_TYPE_QUERY_KEY, + HAS_COMMENTS_QUERY_KEY, +} from '../../constants'; export type ChangeType = typeof ADDED | typeof DELETED | typeof RENAMED | typeof MODIFIED; +export type HasCommentsFn = (reviewUri: vscode.Uri) => boolean; const getChangeType = (file: RestDiffFile): ChangeType => { if (file.new_file) return ADDED; @@ -28,29 +36,56 @@ const imageExtensions = [ const looksLikeImage = (filePath: string) => imageExtensions.includes(path.extname(filePath).toLowerCase()); -export class ChangedFileItem extends TreeItem { - mr: RestMr; - - mrVersion: RestMrVersion; - - repositoryPath: string; - - file: RestDiffFile; +const getBaseAndHeadUri = ( + mr: RestMr, + mrVersion: RestMrVersion, + file: RestDiffFile, + repositoryPath: string, +) => { + const commonParams: ReviewParams = { + repositoryRoot: repositoryPath, + projectId: mr.project_id, + mrId: mr.id, + }; + const emptyFileUri = toReviewUri(commonParams); + const baseFileUri = file.new_file + ? emptyFileUri + : toReviewUri({ + ...commonParams, + path: file.old_path, + commit: mrVersion.base_commit_sha, + }); + const headFileUri = file.deleted_file + ? emptyFileUri + : toReviewUri({ + ...commonParams, + path: file.new_path, + commit: mrVersion.head_commit_sha, + }); + return { baseFileUri, headFileUri }; +}; - constructor(mr: RestMr, mrVersion: RestMrVersion, file: RestDiffFile, repositoryPath: string) { - const changeType = getChangeType(file); - const query = new URLSearchParams([['changeType', changeType]]).toString(); - super(Uri.file(file.new_path).with({ query })); +export class ChangedFileItem extends vscode.TreeItem { + constructor( + mr: RestMr, + mrVersion: RestMrVersion, + file: RestDiffFile, + repositoryPath: string, + hasComment: HasCommentsFn, + ) { + super(vscode.Uri.file(file.new_path)); this.description = path .dirname(`/${file.new_path}`) .split('/') .slice(1) .join('/'); - this.mr = mr; - this.mrVersion = mrVersion; - this.repositoryPath = repositoryPath; - this.file = file; - + const { baseFileUri, headFileUri } = getBaseAndHeadUri(mr, mrVersion, file, repositoryPath); + const hasComments = hasComment(baseFileUri) || hasComment(headFileUri); + const query = new URLSearchParams([ + [CHANGE_TYPE_QUERY_KEY, getChangeType(file)], + [HAS_COMMENTS_QUERY_KEY, String(hasComments)], + ]).toString(); + this.resourceUri = this.resourceUri?.with({ query }); if (looksLikeImage(file.old_path) || looksLikeImage(file.new_path)) { this.command = { title: 'Images are not supported', @@ -58,27 +93,6 @@ export class ChangedFileItem extends TreeItem { }; return; } - const commonParams: ReviewParams = { - repositoryRoot: repositoryPath, - projectId: mr.project_id, - mrId: mr.id, - }; - const emptyFileUri = toReviewUri(commonParams); - const baseFileUri = file.new_file - ? emptyFileUri - : toReviewUri({ - ...commonParams, - path: file.old_path, - commit: mrVersion.base_commit_sha, - }); - const headFileUri = file.deleted_file - ? emptyFileUri - : toReviewUri({ - ...commonParams, - path: file.new_path, - commit: mrVersion.head_commit_sha, - }); - this.command = { title: 'Show changes', command: VS_COMMANDS.DIFF, diff --git a/src/data_providers/items/mr_item_model.test.ts b/src/data_providers/items/mr_item_model.test.ts index 6732289..7f2e534 100644 --- a/src/data_providers/items/mr_item_model.test.ts +++ b/src/data_providers/items/mr_item_model.test.ts @@ -6,10 +6,12 @@ import { noteOnDiffTextSnippet, multipleNotes, } from '../../../test/integration/fixtures/graphql/discussions.js'; +import * as mrVersion from '../../../test/integration/fixtures/rest/mr_version.json'; import { CommentingRangeProvider } from '../../review/commenting_range_provider'; import { createWrappedRepository } from '../../test_utils/create_wrapped_repository'; import { fromReviewUri } from '../../review/review_uri'; import { WrappedRepository } from '../../git/wrapped_repository'; +import { CHANGE_TYPE_QUERY_KEY, HAS_COMMENTS_QUERY_KEY } from '../../constants'; const createCommentControllerMock = vscode.comments.createCommentController as jest.Mock; @@ -94,6 +96,14 @@ describe('MrItemModel', () => { expect(path).toBe(discussionPosition.oldPath); }); + it('should return changed file items as children', async () => { + gitLabService.getMrDiff = jest.fn().mockResolvedValue(mrVersion); + const [overview, changedItem] = await item.getChildren(); + expect(changedItem.resourceUri?.path).toBe('.deleted.yml'); + expect(changedItem.resourceUri?.query).toMatch(`${CHANGE_TYPE_QUERY_KEY}=deleted`); + expect(changedItem.resourceUri?.query).toMatch(`${HAS_COMMENTS_QUERY_KEY}=false`); + }); + describe('commenting range', () => { it('should not add a commenting range provider if user does not have permission to comment', async () => { canUserCommentOnMr = false; @@ -111,17 +121,6 @@ describe('MrItemModel', () => { expect(commentController.commentingRangeProvider).toBeInstanceOf(CommentingRangeProvider); }); - // this test ensures that we add comment controller to disposables before calling API. - it('comment controller can be disposed regardless of API failures', async () => { - gitLabService.getDiscussions = () => Promise.reject(new Error()); - - await item.getChildren(); - - expect(commentController.dispose).not.toHaveBeenCalled(); - item.dispose(); - expect(commentController.dispose).toHaveBeenCalled(); - }); - it('when we create comment controller for the same MR, we dispose the previously created controller', async () => { await item.getChildren(); diff --git a/src/data_providers/items/mr_item_model.ts b/src/data_providers/items/mr_item_model.ts index 6000bfc..24ca38b 100644 --- a/src/data_providers/items/mr_item_model.ts +++ b/src/data_providers/items/mr_item_model.ts @@ -62,17 +62,23 @@ export class MrItemModel extends ItemModel { return item; } - async getChildren(): Promise { - const overview = new vscode.TreeItem('Overview'); - overview.iconPath = new vscode.ThemeIcon('note'); - overview.command = { + private get overviewItem() { + const result = new vscode.TreeItem('Overview'); + result.iconPath = new vscode.ThemeIcon('note'); + result.command = { command: PROGRAMMATIC_COMMANDS.SHOW_RICH_CONTENT, arguments: [this.mr, this.repository.rootFsPath], title: 'Show MR Overview', }; - const { mrVersion } = await this.repository.reloadMr(this.mr); + return result; + } + + private async getMrDiscussions(): Promise { try { - await this.initializeMrDiscussions(mrVersion); + const discussions = await this.repository.getGitLabService().getDiscussions({ + issuable: this.mr, + }); + return discussions.filter(isTextDiffDiscussion); } catch (e) { handleError( new UserFriendlyError( @@ -83,14 +89,31 @@ export class MrItemModel extends ItemModel { ), ); } + return []; + } + + async getChildren(): Promise { + const { mrVersion } = await this.repository.reloadMr(this.mr); + const discussions = await this.getMrDiscussions(); + + await this.addAllCommentsToVsCode(mrVersion, discussions); + const allUrisWithComments = discussions.map(d => + uriForDiscussion(this.repository, this.mr, d).toString(), + ); const changedFiles = mrVersion.diffs.map( - d => new ChangedFileItem(this.mr, mrVersion, d, this.repository.rootFsPath), + diff => + new ChangedFileItem(this.mr, mrVersion, diff, this.repository.rootFsPath, uri => + allUrisWithComments.includes(uri.toString()), + ), ); - return [overview, ...changedFiles]; + return [this.overviewItem, ...changedFiles]; } - private async initializeMrDiscussions(mrVersion: RestMrVersion): Promise { + private async addAllCommentsToVsCode( + mrVersion: RestMrVersion, + discussions: GqlTextDiffDiscussion[], + ): Promise { const gitlabService = this.repository.getGitLabService(); const userCanComment = await gitlabService.canUserCommentOnMr(this.mr); @@ -101,11 +124,7 @@ export class MrItemModel extends ItemModel { ); this.setDisposableChildren([commentController]); - const discussions = await gitlabService.getDiscussions({ - issuable: this.mr, - }); - const discussionsOnDiff = discussions.filter(isTextDiffDiscussion); - discussionsOnDiff.forEach(discussion => { + discussions.forEach(discussion => { const { position } = firstNoteFrom(discussion); const vsThread = commentController.createCommentThread( uriForDiscussion(this.repository, this.mr, discussion), diff --git a/src/extension.js b/src/extension.js index 09b7d31..52aad6e 100644 --- a/src/extension.js +++ b/src/extension.js @@ -27,7 +27,8 @@ const { submitEdit, createComment, } = require('./commands/mr_discussion_commands'); -const { fileDecorationProvider } = require('./review/file_decoration_provider'); +const { hasCommentsDecorationProvider } = require('./review/has_comments_decoration_provider'); +const { changeTypeDecorationProvider } = require('./review/change_type_decoration_provider'); const { checkVersion } = require('./utils/check_version'); const { checkoutMrBranch } = require('./commands/checkout_mr_branch'); @@ -126,7 +127,8 @@ const activate = context => { registerCiCompletion(context); gitExtensionWrapper.init(); context.subscriptions.push(gitExtensionWrapper); - vscode.window.registerFileDecorationProvider(fileDecorationProvider); + vscode.window.registerFileDecorationProvider(hasCommentsDecorationProvider); + vscode.window.registerFileDecorationProvider(changeTypeDecorationProvider); checkVersion(gitExtensionWrapper, context); }; diff --git a/src/review/file_decoration_provider.test.ts b/src/review/change_type_decoration_provider.test.ts similarity index 56% rename from src/review/file_decoration_provider.test.ts rename to src/review/change_type_decoration_provider.test.ts index 3fc4506..c98bf6d 100644 --- a/src/review/file_decoration_provider.test.ts +++ b/src/review/change_type_decoration_provider.test.ts @@ -1,6 +1,6 @@ import * as vscode from 'vscode'; -import { fileDecorationProvider, decorations } from './file_decoration_provider'; -import { ADDED, DELETED, RENAMED, MODIFIED } from '../constants'; +import { changeTypeDecorationProvider, decorations } from './change_type_decoration_provider'; +import { ADDED, DELETED, RENAMED, MODIFIED, CHANGE_TYPE_QUERY_KEY } from '../constants'; describe('FileDecoratorProvider', () => { it.each` @@ -10,9 +10,9 @@ describe('FileDecoratorProvider', () => { ${RENAMED} | ${decorations[RENAMED]} ${MODIFIED} | ${decorations[MODIFIED]} `('Correctly maps changeType to decorator', ({ changeType, decoration }) => { - const uri: vscode.Uri = vscode.Uri.file(`./test?changeType=${changeType}`); + const uri: vscode.Uri = vscode.Uri.file(`./test?${CHANGE_TYPE_QUERY_KEY}=${changeType}`); const { token } = new vscode.CancellationTokenSource(); - const returnValue = fileDecorationProvider.provideFileDecoration(uri, token); + const returnValue = changeTypeDecorationProvider.provideFileDecoration(uri, token); expect(returnValue).toEqual(decoration); }); diff --git a/src/review/file_decoration_provider.ts b/src/review/change_type_decoration_provider.ts similarity index 76% rename from src/review/file_decoration_provider.ts rename to src/review/change_type_decoration_provider.ts index 260979e..b662dcb 100644 --- a/src/review/file_decoration_provider.ts +++ b/src/review/change_type_decoration_provider.ts @@ -1,5 +1,5 @@ import * as vscode from 'vscode'; -import { ADDED, DELETED, RENAMED, MODIFIED } from '../constants'; +import { ADDED, DELETED, RENAMED, MODIFIED, CHANGE_TYPE_QUERY_KEY } from '../constants'; export const decorations: Record = { [ADDED]: { @@ -19,11 +19,11 @@ export const decorations: Record = { }, }; -export const fileDecorationProvider: vscode.FileDecorationProvider = { +export const changeTypeDecorationProvider: vscode.FileDecorationProvider = { provideFileDecoration: uri => { if (uri.scheme === 'file') { const params = new URLSearchParams(uri.query); - const changeType = params.get('changeType'); + const changeType = params.get(CHANGE_TYPE_QUERY_KEY); if (changeType) { return decorations[changeType]; } diff --git a/src/review/has_comments_decoration_provider.test.ts b/src/review/has_comments_decoration_provider.test.ts new file mode 100644 index 0000000..08b6d8e --- /dev/null +++ b/src/review/has_comments_decoration_provider.test.ts @@ -0,0 +1,18 @@ +import * as vscode from 'vscode'; +import { HAS_COMMENTS_QUERY_KEY } from '../constants'; +import { hasCommentsDecorationProvider } from './has_comments_decoration_provider'; + +describe('FileDecoratorProvider', () => { + it.each` + urlQuery | decoration + ${`?${HAS_COMMENTS_QUERY_KEY}=true`} | ${'💬'} + ${`?${HAS_COMMENTS_QUERY_KEY}=false`} | ${undefined} + ${''} | ${undefined} + `('Correctly maps hasComments query to decorator', async ({ urlQuery, decoration }) => { + const uri: vscode.Uri = vscode.Uri.file(`./test${urlQuery}`); + const { token } = new vscode.CancellationTokenSource(); + const returnValue = await hasCommentsDecorationProvider.provideFileDecoration(uri, token); + + expect(returnValue?.badge).toEqual(decoration); + }); +}); diff --git a/src/review/has_comments_decoration_provider.ts b/src/review/has_comments_decoration_provider.ts new file mode 100644 index 0000000..b06771b --- /dev/null +++ b/src/review/has_comments_decoration_provider.ts @@ -0,0 +1,16 @@ +import * as vscode from 'vscode'; +import { HAS_COMMENTS_QUERY_KEY } from '../constants'; + +export const hasCommentsDecorationProvider: vscode.FileDecorationProvider = { + provideFileDecoration: uri => { + if (uri.scheme !== 'file') { + return undefined; + } + const params = new URLSearchParams(uri.query); + const hasComments = params.get(HAS_COMMENTS_QUERY_KEY) === 'true'; + if (hasComments) { + return { badge: '💬' }; + } + return undefined; + }, +}; diff --git a/src/test_utils/uri.ts b/src/test_utils/uri.ts index 0161bd3..516f3b3 100644 --- a/src/test_utils/uri.ts +++ b/src/test_utils/uri.ts @@ -54,7 +54,7 @@ export class Uri implements vscode.Uri { } toJSON(): string { - return JSON.stringify(this); + return JSON.stringify({ ...this }); } static parse(stringUri: string): Uri { -- GitLab