From a4f3be808675e3b4be2c51f3d4f172022fd3bc8f Mon Sep 17 00:00:00 2001 From: praveenkuttappan Date: Wed, 8 Mar 2023 15:46:40 -0500 Subject: [PATCH 1/2] Disable delete option for PR API reviews --- .../APIView/APIViewWeb/Managers/ReviewManager.cs | 12 ++++++++++++ .../APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml | 2 +- .../Repositories/UnDeletableReviewException.cs | 11 +++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs diff --git a/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs index b68de0a0ef4..b5f94a74f7f 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs @@ -119,6 +119,7 @@ public async Task DeleteReviewAsync(ClaimsPrincipal user, string id) { var reviewModel = await _reviewsRepository.GetReviewAsync(id); await AssertReviewOwnerAsync(user, reviewModel); + AssertReviewDeletion(reviewModel); await _reviewsRepository.DeleteReviewAsync(reviewModel); @@ -261,6 +262,7 @@ public async Task CreateReviewCodeFileModel(string revision public async Task DeleteRevisionAsync(ClaimsPrincipal user, string id, string revisionId) { var review = await GetReviewAsync(user, id); + AssertReviewDeletion(review); var revision = review.Revisions.Single(r => r.RevisionId == revisionId); await AssertRevisionOwner(user, revision); @@ -819,6 +821,16 @@ private LanguageService GetLanguageService(string language) return _languageServices.FirstOrDefault(service => service.Name == language); } + private void AssertReviewDeletion(ReviewModel reviewModel) + { + // We allow deletion of manual API review only. + // Server side assertion to ensure we are not processing any requests to delete automatic and PR API review + if (reviewModel.FilterType != ReviewType.Manual) + { + throw new UnDeletableReviewException(); + } + } + private async Task AssertReviewOwnerAsync(ClaimsPrincipal user, ReviewModel reviewModel) { var result = await _authorizationService.AuthorizeAsync(user, reviewModel, new[] { ReviewOwnerRequirement.Instance }); diff --git a/src/dotnet/APIView/APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml b/src/dotnet/APIView/APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml index 4dcbbc6d236..a9fcc9a2d7e 100644 --- a/src/dotnet/APIView/APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml +++ b/src/dotnet/APIView/APIViewWeb/Pages/Shared/_ReviewsPartial.cshtml @@ -48,7 +48,7 @@ @review.FilterType.ToString() - @if (review.Author == User.GetGitHubLogin()) + @if (review.Author == User.GetGitHubLogin() && review.FilterType == ReviewType.Manual) { diff --git a/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs b/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs new file mode 100644 index 00000000000..11351b3a1a0 --- /dev/null +++ b/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs @@ -0,0 +1,11 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; + +namespace APIViewWeb.Repositories +{ + internal class UnDeletableReviewException : Exception + { + } +} From f26435242d873828d013f42e91719d15a5b804a9 Mon Sep 17 00:00:00 2001 From: Chidozie Ononiwu Date: Fri, 10 Mar 2023 18:46:27 -0800 Subject: [PATCH 2/2] Add Test to Excercise DeleteRevisionAsync --- .../ReviewManagerTests.cs | 19 +++++++++++++++++++ .../TestsBaseFixture.cs | 7 ++++--- .../UnDeletableReviewException.cs | 2 +- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/dotnet/APIView/APIViewIntegrationTests/ReviewManagerTests.cs b/src/dotnet/APIView/APIViewIntegrationTests/ReviewManagerTests.cs index ecb1e9aa1f4..588e8d8876d 100644 --- a/src/dotnet/APIView/APIViewIntegrationTests/ReviewManagerTests.cs +++ b/src/dotnet/APIView/APIViewIntegrationTests/ReviewManagerTests.cs @@ -2,6 +2,8 @@ using Xunit; using System.IO; using System; +using APIViewWeb; +using APIViewWeb.Repositories; namespace APIViewIntegrationTests { @@ -71,5 +73,22 @@ public async Task AddRevisionAsync_Computes_Headings_Of_Sections_With_Diff_B() Assert.All(headingWithDiffInSections, item => Assert.Contains(item, new int[] { 20, 275 })); } + + [Fact] + public async Task Delete_PullRequest_Review_Throws_Exception() + { + var reviewManager = testsBaseFixture.ReviewManager; + var user = testsBaseFixture.User; + var review = await reviewManager.CreateReviewAsync(user, fileNameC, "Azure.Analytics.Purview.Account", fileStreamC, false, "Swagger", false); + Assert.Equal(ReviewType.Manual, review.FilterType); + + review.FilterType = ReviewType.PullRequest; + + // Change review type to PullRequest + await testsBaseFixture.ReviewRepository.UpsertReviewAsync(review); + Assert.Equal(ReviewType.PullRequest, review.FilterType); + + await Assert.ThrowsAsync(async () => await reviewManager.DeleteRevisionAsync(user, review.ReviewId, review.Revisions[0].RevisionId)); + } } } diff --git a/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs b/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs index 49ad8d37c9c..021d4437cec 100644 --- a/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs +++ b/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs @@ -27,6 +27,7 @@ public class TestsBaseFixture : IDisposable public PackageNameManager PackageNameManager { get; private set; } public ReviewManager ReviewManager { get; private set; } + public CosmosReviewRepository ReviewRepository { get; private set; } public ClaimsPrincipal User { get; private set; } public TestsBaseFixture() @@ -63,7 +64,7 @@ public TestsBaseFixture() _ = dataBaseResponse.Database.CreateContainerIfNotExistsAsync("Reviews", "/id"); _ = dataBaseResponse.Database.CreateContainerIfNotExistsAsync("Comments", "/ReviewId"); _ = dataBaseResponse.Database.CreateContainerIfNotExistsAsync("Profiles", "/id"); - var cosmosReviewRepository = new CosmosReviewRepository(config); + ReviewRepository = new CosmosReviewRepository(config); var cosmosCommentsRepository = new CosmosCommentsRepository(config); var cosmosUserProfileRepository = new CosmosUserProfileRepository(config); @@ -79,7 +80,7 @@ public TestsBaseFixture() authorizationServiceMoq.Setup(_ => _.AuthorizeAsync(It.IsAny(), It.IsAny(), It.IsAny>())) .ReturnsAsync(AuthorizationResult.Success); - var notificationManager = new NotificationManager(config, cosmosReviewRepository, cosmosUserProfileRepository); + var notificationManager = new NotificationManager(config, ReviewRepository, cosmosUserProfileRepository); var devopsArtifactRepositoryMoq = new Mock(); devopsArtifactRepositoryMoq.Setup(_ => _.DownloadPackageArtifact(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) @@ -89,7 +90,7 @@ public TestsBaseFixture() .Returns(Task.CompletedTask); ReviewManager = new ReviewManager( - authorizationServiceMoq.Object, cosmosReviewRepository, blobCodeFileRepository, blobOriginalsRepository, cosmosCommentsRepository, + authorizationServiceMoq.Object, ReviewRepository, blobCodeFileRepository, blobOriginalsRepository, cosmosCommentsRepository, languageService, notificationManager, devopsArtifactRepositoryMoq.Object, PackageNameManager); } diff --git a/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs b/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs index 11351b3a1a0..df8683d4e60 100644 --- a/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs +++ b/src/dotnet/APIView/APIViewWeb/Repositories/UnDeletableReviewException.cs @@ -5,7 +5,7 @@ namespace APIViewWeb.Repositories { - internal class UnDeletableReviewException : Exception + public class UnDeletableReviewException : Exception { } }