mhutchie#482 New "Mark as Reviewed" & "Mark as Not Reviewed" actions on the file context menu in the Commit Details View when a Code Review is in progress. Squash Merge of @dan1994's PR mhutchie#483.

Author: dan1994

src/extensionState.ts

@@ -359,24 +359,31 @@ export class ExtensionState extends Disposable {
 	}
 
 	/**
-	 * Record that a file has been reviewed in a Code Review.
+	 * Update information for a specific Code Review.
 	 * @param repo The repository the Code Review is in.
 	 * @param id The ID of the Code Review.
-	 * @param file The file that has been reviewed.
+	 * @param remainingFiles The files remaining for review.
+	 * @param lastViewedFile The last viewed file. If null, don't change the last viewed file.
+	 * @returns An error message if request can't be completed.
 	 */
-	public updateCodeReviewFileReviewed(repo: string, id: string, file: string) {
-		let reviews = this.getCodeReviews();
-		if (typeof reviews[repo] !== 'undefined' && typeof reviews[repo][id] !== 'undefined') {
-			let i = reviews[repo][id].remainingFiles.indexOf(file);
-			if (i > -1) reviews[repo][id].remainingFiles.splice(i, 1);
-			if (reviews[repo][id].remainingFiles.length > 0) {
-				reviews[repo][id].lastViewedFile = file;
-				reviews[repo][id].lastActive = (new Date()).getTime();
-			} else {
-				removeCodeReview(reviews, repo, id);
+	public updateCodeReview(repo: string, id: string, remainingFiles: string[], lastViewedFile: string | null) {
+		const reviews = this.getCodeReviews();
+
+		if (typeof reviews[repo] === 'undefined' || typeof reviews[repo][id] === 'undefined') {
+			return Promise.resolve('The Code Review could not be found.');
+		}
+
+		if (remainingFiles.length > 0) {
+			reviews[repo][id].remainingFiles = remainingFiles;
+			reviews[repo][id].lastActive = (new Date()).getTime();
+			if (lastViewedFile !== null) {
+				reviews[repo][id].lastViewedFile = lastViewedFile;
 			}
-			this.setCodeReviews(reviews);
+		} else {
+			removeCodeReview(reviews, repo, id);
 		}
+
+		return this.setCodeReviews(reviews);
 	}
 
 	/**

src/gitGraphView.ts

@@ -227,9 +227,6 @@ export class GitGraphView extends Disposable {
 					error: await this.dataSource.cleanUntrackedFiles(msg.repo, msg.directories)
 				});
 				break;
-			case 'codeReviewFileReviewed':
-				this.extensionState.updateCodeReviewFileReviewed(msg.repo, msg.id, msg.filePath);
-				break;
 			case 'commitDetails':
 				let data = await Promise.all<GitCommitDetailsData, string | null>([
 					msg.commitHash === UNCOMMITTED
@@ -576,6 +573,12 @@ export class GitGraphView extends Disposable {
 					...await this.dataSource.getTagDetails(msg.repo, msg.tagName)
 				});
 				break;
+			case 'updateCodeReview':
+				this.sendMessage({
+					command: 'updateCodeReview',
+					error: await this.extensionState.updateCodeReview(msg.repo, msg.id, msg.remainingFiles, msg.lastViewedFile)
+				});
+				break;
 			case 'viewDiff':
 				this.sendMessage({
 					command: 'viewDiff',

src/types.ts

@@ -645,12 +645,6 @@ export interface ResponseCleanUntrackedFiles extends ResponseWithErrorInfo {
 	readonly command: 'cleanUntrackedFiles';
 }
 
-export interface RequestCodeReviewFileReviewed extends RepoRequest {
-	readonly command: 'codeReviewFileReviewed';
-	readonly id: string;
-	readonly filePath: string;
-}
-
 export interface RequestCommitDetails extends RepoRequest {
 	readonly command: 'commitDetails';
 	readonly commitHash: string;
@@ -1161,6 +1155,17 @@ export interface ResponseTagDetails extends ResponseWithErrorInfo {
 	readonly message: string;
 }
 
+export interface RequestUpdateCodeReview extends RepoRequest {
+	readonly command: 'updateCodeReview';
+	readonly id: string;
+	readonly remainingFiles: string[];
+	readonly lastViewedFile: string | null;
+}
+
+export interface ResponseUpdateCodeReview extends ResponseWithErrorInfo {
+	readonly command: 'updateCodeReview';
+}
+
 export interface RequestViewDiff extends RepoRequest {
 	readonly command: 'viewDiff';
 	readonly fromHash: string;
@@ -1207,7 +1212,6 @@ export type RequestMessage =
 	| RequestCheckoutCommit
 	| RequestCherrypickCommit
 	| RequestCleanUntrackedFiles
-	| RequestCodeReviewFileReviewed
 	| RequestCommitDetails
 	| RequestCompareCommits
 	| RequestCopyFilePath
@@ -1256,6 +1260,7 @@ export type RequestMessage =
 	| RequestShowErrorDialog
 	| RequestStartCodeReview
 	| RequestTagDetails
+	| RequestUpdateCodeReview
 	| RequestViewDiff
 	| RequestViewDiffWithWorkingFile
 	| RequestViewFileAtRevision
@@ -1315,6 +1320,7 @@ export type ResponseMessage =
 	| ResponseSetWorkspaceViewState
 	| ResponseStartCodeReview
 	| ResponseTagDetails
+	| ResponseUpdateCodeReview
 	| ResponseViewDiff
 	| ResponseViewDiffWithWorkingFile
 	| ResponseViewFileAtRevision

tests/extensionState.test.ts

@@ -1167,124 +1167,132 @@ describe('ExtensionState', () => {
 		});
 	});
 
-	describe('updateCodeReviewFileReviewed', () => {
-		it('Should remove the reviewed file, set it as the last viewed file, and update the last active time', () => {
-			// Setup
+	describe('updateCodeReview', () => {
+		const repo = '/path/to/repo';
+		const id = 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2';
+		const startTime = 1587559257000;
+		const endTime = startTime + 1000;
+
+		beforeEach(() => {
 			const codeReviews = {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559257000,
+				[repo]: {
+					[id]: {
+						lastActive: startTime,
 						lastViewedFile: 'file1.txt',
 						remainingFiles: ['file2.txt', 'file3.txt']
 					}
 				}
 			};
+
 			extensionContext.workspaceState.get.mockReturnValueOnce(codeReviews);
 			extensionContext.workspaceState.update.mockResolvedValueOnce(null);
+		});
 
+		it('Should update the reviewed files and change the last viewed file', async () => {
 			// Run
-			extensionState.updateCodeReviewFileReviewed('/path/to/repo', 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2', 'file2.txt');
+			const result = await extensionState.updateCodeReview(repo, id, ['file3.txt'], 'file2.txt');
 
-			// Assert
+			// Asset
+			expect(result).toBeNull();
 			expect(extensionContext.workspaceState.update).toHaveBeenCalledWith('codeReviews', {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559258000,
+				[repo]: {
+					[id]: {
+						lastActive: endTime,
 						lastViewedFile: 'file2.txt',
 						remainingFiles: ['file3.txt']
 					}
 				}
 			});
 		});
 
-		it('Should ignore removing reviewed files if it has already be stored as reviewed', () => {
-			// Setup
-			const codeReviews = {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559257000,
+		it('Should update the reviewed files without changing the last viewed file', async () => {
+			// Run
+			const result = await extensionState.updateCodeReview(repo, id, ['file3.txt'], null);
+
+			// Assert
+			expect(result).toBeNull();
+			expect(extensionContext.workspaceState.update).toHaveBeenCalledWith('codeReviews', {
+				[repo]: {
+					[id]: {
+						lastActive: endTime,
 						lastViewedFile: 'file1.txt',
 						remainingFiles: ['file3.txt']
 					}
 				}
-			};
-			extensionContext.workspaceState.get.mockReturnValueOnce(codeReviews);
-			extensionContext.workspaceState.update.mockResolvedValueOnce(null);
+			});
+		});
 
+		it('Should update the not reviewed files without changing the last viewed file', async () => {
 			// Run
-			extensionState.updateCodeReviewFileReviewed('/path/to/repo', 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2', 'file2.txt');
+			const result = await extensionState.updateCodeReview(repo, id, ['file2.txt', 'file3.txt', 'file4.txt'], null);
 
 			// Assert
+			expect(result).toBeNull();
 			expect(extensionContext.workspaceState.update).toHaveBeenCalledWith('codeReviews', {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559258000,
-						lastViewedFile: 'file2.txt',
-						remainingFiles: ['file3.txt']
+				[repo]: {
+					[id]: {
+						lastActive: endTime,
+						lastViewedFile: 'file1.txt',
+						remainingFiles: ['file2.txt', 'file3.txt', 'file4.txt']
 					}
 				}
 			});
 		});
 
-		it('Should remove the code review the last file in it has been reviewed', () => {
-			// Setup
-			const codeReviews = {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559257000,
+		it('Should set the last viewed file', async () => {
+			// Run
+			const result = await extensionState.updateCodeReview(repo, id, ['file2.txt', 'file3.txt'], 'file2.txt');
+
+			// Assert
+			expect(result).toBeNull();
+			expect(extensionContext.workspaceState.update).toHaveBeenCalledWith('codeReviews', {
+				[repo]: {
+					[id]: {
+						lastActive: endTime,
 						lastViewedFile: 'file2.txt',
-						remainingFiles: ['file3.txt']
+						remainingFiles: ['file2.txt', 'file3.txt']
 					}
 				}
-			};
-			extensionContext.workspaceState.get.mockReturnValueOnce(codeReviews);
-			extensionContext.workspaceState.update.mockResolvedValueOnce(null);
+			});
+		});
 
+		it('Should remove the code review the last file in it has been reviewed', async () => {
 			// Run
-			extensionState.updateCodeReviewFileReviewed('/path/to/repo', 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2', 'file3.txt');
+			const result = await extensionState.updateCodeReview(repo, id, [], null);
 
 			// Assert
+			expect(result).toBeNull();
 			expect(extensionContext.workspaceState.update).toHaveBeenCalledWith('codeReviews', {});
 		});
 
-		it('Shouldn\'t change the state if no code review could be found in the specified repository', () => {
-			// Setup
-			const codeReviews = {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559258000,
-						lastViewedFile: 'file1.txt',
-						remainingFiles: ['file2.txt', 'file3.txt']
-					}
-				}
-			};
-			extensionContext.workspaceState.get.mockReturnValueOnce(codeReviews);
+		it('Shouldn\'t change the state if no code review could be found in the specified repository', async () => {
+			// Run
+			const result = await extensionState.updateCodeReview(repo + '1', id, ['file3.txt'], null);
+
+			// Assert
+			expect(result).toBe('The Code Review could not be found.');
+			expect(extensionContext.workspaceState.update).toHaveBeenCalledTimes(0);
+		});
 
+		it('Shouldn\'t change the state if no code review could be found with the specified id', async () => {
 			// Run
-			extensionState.updateCodeReviewFileReviewed('/path/to/repo1', 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2', 'file2.txt');
+			const result = await extensionState.updateCodeReview(repo, id + '1', ['file3.txt'], null);
 
 			// Assert
+			expect(result).toBe('The Code Review could not be found.');
 			expect(extensionContext.workspaceState.update).toHaveBeenCalledTimes(0);
 		});
 
-		it('Shouldn\'t change the state if no code review could be found with the specified id', () => {
+		it('Should return an error message when workspaceState.update rejects', async () => {
 			// Setup
-			const codeReviews = {
-				'/path/to/repo': {
-					'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2': {
-						lastActive: 1587559258000,
-						lastViewedFile: 'file1.txt',
-						remainingFiles: ['file2.txt', 'file3.txt']
-					}
-				}
-			};
-			extensionContext.workspaceState.get.mockReturnValueOnce(codeReviews);
+			extensionContext.workspaceState.update.mockReset();
+			extensionContext.workspaceState.update.mockRejectedValueOnce(null);
 
 			// Run
-			extensionState.updateCodeReviewFileReviewed('/path/to/repo', '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', 'file2.txt');
+			const result = await extensionState.updateCodeReview(repo, id, ['file3.txt'], 'file2.txt');
 
-			// Assert
-			expect(extensionContext.workspaceState.update).toHaveBeenCalledTimes(0);
+			// Asset
+			expect(result).toBe('Visual Studio Code was unable to save the Git Graph Workspace State Memento.');
 		});
 	});