From e536c7a8d58d9cc12b9712f85ccc69f05ee01093 Mon Sep 17 00:00:00 2001 From: Chidozie Ononiwu <31145988+chidozieononiwu@users.noreply.github.com> Date: Tue, 6 Feb 2024 13:30:01 -0800 Subject: [PATCH] Move Approval Request to APIRevisions (#7557) --- .../TestsBaseFixture.cs | 2 +- .../APIViewWeb/LeanModels/ReviewListModels.cs | 13 +++---- .../Managers/APIRevisionsManager.cs | 36 +++++++++++++++++++ .../Interfaces/IAPIRevisionsManager.cs | 2 ++ .../Interfaces/INotificationManager.cs | 2 +- .../Managers/Interfaces/IReviewManager.cs | 2 -- .../Managers/NotificationManager.cs | 22 ++++++------ .../APIViewWeb/Managers/ReviewManager.cs | 36 ------------------- .../Pages/Assemblies/RequestedReviews.cshtml | 4 +-- .../Assemblies/RequestedReviews.cshtml.cs | 22 ++++++++---- .../APIViewWeb/Pages/Assemblies/Review.cshtml | 5 +-- .../Pages/Assemblies/Review.cshtml.cs | 15 ++++---- .../CosmosAPIRevisionsRepository.cs | 16 +++++++++ .../Repositories/CosmosReviewRepository.cs | 16 --------- .../ICosmosAPIRevisionsRepository.cs | 2 ++ .../Interfaces/ICosmosReviewRepository.cs | 1 - 16 files changed, 103 insertions(+), 93 deletions(-) diff --git a/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs b/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs index 1da77008383..ffee988e8bc 100644 --- a/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs +++ b/src/dotnet/APIView/APIViewIntegrationTests/TestsBaseFixture.cs @@ -97,7 +97,7 @@ public TestsBaseFixture() var telemetryClient = new Mock(); - var notificationManager = new NotificationManager(config, ReviewRepository, cosmosUserProfileRepository, telemetryClient.Object); + var notificationManager = new NotificationManager(config, ReviewRepository, APIRevisionRepository, cosmosUserProfileRepository, telemetryClient.Object); var devopsArtifactRepositoryMoq = new Mock(); devopsArtifactRepositoryMoq.Setup(_ => _.DownloadPackageArtifact(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) diff --git a/src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs b/src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs index 30ca9ea8abe..df235ec88ac 100644 --- a/src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs +++ b/src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs @@ -89,12 +89,16 @@ public class LegacyRevisionModel public bool IsApproved { get; set; } } - public class ReviewListItemModel + public class BaseListitemModel { [JsonProperty("id")] public string Id { get; set; } = IdHelper.GenerateId(); public string PackageName { get; set; } public string Language { get; set; } + } + + public class ReviewListItemModel : BaseListitemModel + { public HashSet Subscribers { get; set; } = new HashSet(); public List ChangeHistory { get; set; } = new List(); public List AssignedReviewers { get; set; } = new List(); @@ -106,19 +110,16 @@ public class ReviewListItemModel public bool IsDeleted { get; set; } } - public class APIRevisionListItemModel + public class APIRevisionListItemModel : BaseListitemModel { - [JsonProperty("id")] - public string Id { get; set; } = IdHelper.GenerateId(); public string ReviewId { get; set; } - public string PackageName { get; set; } - public string Language { get; set; } public List Files { get; set; } = new List(); public string Label { get; set; } public List ChangeHistory { get; set; } = new List(); public APIRevisionType APIRevisionType { get; set; } public int? PullRequestNo { get; set; } public Dictionary> HeadingsOfSectionsWithDiff { get; set; } = new Dictionary>(); + public List AssignedReviewers { get; set; } = new List(); public bool IsApproved { get; set; } public HashSet Approvers { get; set; } = new HashSet(); public string CreatedBy { get; set; } diff --git a/src/dotnet/APIView/APIViewWeb/Managers/APIRevisionsManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/APIRevisionsManager.cs index 13c37b6eb1f..256c7905363 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/APIRevisionsManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/APIRevisionsManager.cs @@ -730,6 +730,42 @@ public async Task CreateAPIRevisionAsync(string userNa return apiRevision; } + /// + /// Assign reviewers to a review + /// + /// + /// + /// + /// + public async Task AssignReviewersToAPIRevisionAsync(ClaimsPrincipal User, string apiRevisionId, HashSet reviewers) + { + APIRevisionListItemModel apiRevision = await _apiRevisionsRepository.GetAPIRevisionAsync(apiRevisionId); + foreach (var reviewer in reviewers) + { + if (!apiRevision.AssignedReviewers.Where(x => x.AssingedTo == reviewer).Any()) + { + var reviewAssignment = new ReviewAssignmentModel() + { + AssingedTo = reviewer, + AssignedBy = User.GetGitHubLogin(), + AssingedOn = DateTime.Now, + }; + apiRevision.AssignedReviewers.Add(reviewAssignment); + } + } + await _apiRevisionsRepository.UpsertAPIRevisionAsync(apiRevision); + } + + /// + /// Get Reviews that have been assigned for review to a user + /// + /// + /// + public async Task> GetAPIRevisionsAssignedToUser(string userName) + { + return await _apiRevisionsRepository.GetAPIRevisionsAssignedToUser(userName); + } + /// /// Generate the Revision on a DevOps Pipeline /// diff --git a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IAPIRevisionsManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IAPIRevisionsManager.cs index 37754c81d9c..7555bf06b2b 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IAPIRevisionsManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IAPIRevisionsManager.cs @@ -38,5 +38,7 @@ public Task CreateAPIRevisionAsync(string userName, st public Task UpdateAPIRevisionAsync(APIRevisionListItemModel revision, LanguageService languageService); public Task UpdateAPIRevisionAsync(APIRevisionListItemModel revision); public Task AutoArchiveAPIRevisions(int archiveAfterMonths); + public Task AssignReviewersToAPIRevisionAsync(ClaimsPrincipal User, string apiRevisionId, HashSet reviewers); + public Task> GetAPIRevisionsAssignedToUser(string userName); } } diff --git a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/INotificationManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/INotificationManager.cs index fba4b4655e2..6fa124dbb36 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/INotificationManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/INotificationManager.cs @@ -10,7 +10,7 @@ public interface INotificationManager { public Task NotifySubscribersOnComment(ClaimsPrincipal user, CommentItemModel comment); public Task NotifyUserOnCommentTag(CommentItemModel comment); - public Task NotifyApproversOfReview(ClaimsPrincipal user, string reviewId, HashSet reviewers); + public Task NotifyApproversOfReview(ClaimsPrincipal user, string apiRevisionId, HashSet reviewers); public Task NotifySubscribersOnNewRevisionAsync(ReviewListItemModel review, APIRevisionListItemModel revision, ClaimsPrincipal user); public Task ToggleSubscribedAsync(ClaimsPrincipal user, string reviewId); public Task SubscribeAsync(ReviewListItemModel review, ClaimsPrincipal user); diff --git a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IReviewManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IReviewManager.cs index f851fc07fd9..1101b1cd24e 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IReviewManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/Interfaces/IReviewManager.cs @@ -9,7 +9,6 @@ public interface IReviewManager { public Task> GetReviewsAsync(string language, bool? isClosed = false); public Task GetReviewAsync(string language, string packageName, bool? isClosed = false); - public Task> GetReviewsAssignedToUser(string userName); public Task<(IEnumerable Reviews, int TotalCount, int TotalPages, int CurrentPage, int? PreviousPage, int? NextPage)> GetPagedReviewListAsync( IEnumerable search, IEnumerable languages, bool? isClosed, bool? isApproved, int offset, int limit, string orderBy); public Task GetReviewAsync(ClaimsPrincipal user, string id); @@ -19,7 +18,6 @@ public interface IReviewManager public Task ToggleReviewIsClosedAsync(ClaimsPrincipal user, string id); public Task ToggleReviewApprovalAsync(ClaimsPrincipal user, string id, string revisionId, string notes=""); public Task ApproveReviewAsync(ClaimsPrincipal user, string reviewId, string notes = ""); - public Task AssignReviewersToReviewAsync(ClaimsPrincipal User, string reviewId, HashSet reviewers); public Task GenerateAIReview(string reviewId, string revisionId); public Task UpdateReviewsInBackground(HashSet updateDisabledLanguages, int backgroundBatchProcessCount); } diff --git a/src/dotnet/APIView/APIViewWeb/Managers/NotificationManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/NotificationManager.cs index 9a2b3b7db14..ed201668e0a 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/NotificationManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/NotificationManager.cs @@ -24,6 +24,7 @@ public class NotificationManager : INotificationManager { private readonly string _apiviewEndpoint; private readonly ICosmosReviewRepository _reviewRepository; + private readonly ICosmosAPIRevisionsRepository _apiRevisionRepository; private readonly ICosmosUserProfileRepository _userProfileRepository; private readonly string _testEmailToAddress; private readonly string _emailSenderServiceUrl; @@ -32,12 +33,13 @@ public class NotificationManager : INotificationManager private const string ENDPOINT_SETTING = "Endpoint"; public NotificationManager(IConfiguration configuration, - ICosmosReviewRepository reviewRepository, + ICosmosReviewRepository reviewRepository, ICosmosAPIRevisionsRepository apiRevisionRepository, ICosmosUserProfileRepository userProfileRepository, TelemetryClient telemetryClient) { _apiviewEndpoint = configuration.GetValue(ENDPOINT_SETTING); _reviewRepository = reviewRepository; + _apiRevisionRepository = apiRevisionRepository; _userProfileRepository = userProfileRepository; _testEmailToAddress = configuration["apiview-email-test-address"] ?? ""; _emailSenderServiceUrl = configuration["azure-sdk-emailer-url"] ?? ""; @@ -61,15 +63,15 @@ public async Task NotifyUserOnCommentTag(CommentItemModel comment) } } - public async Task NotifyApproversOfReview(ClaimsPrincipal user, string reviewId, HashSet reviewers) + public async Task NotifyApproversOfReview(ClaimsPrincipal user, string apiRevisionId, HashSet reviewers) { var userProfile = await _userProfileRepository.TryGetUserProfileAsync(user.GetGitHubLogin()); - var review = await _reviewRepository.GetReviewAsync(reviewId); + var apiRevision = await _apiRevisionRepository.GetAPIRevisionAsync(apiRevisionId); foreach (var reviewer in reviewers) { var reviewerProfile = await _userProfileRepository.TryGetUserProfileAsync(reviewer); - await SendUserEmailsAsync(review, reviewerProfile, - GetApproverReviewHtmlContent(userProfile, review)); + await SendUserEmailsAsync(apiRevision, reviewerProfile, + GetApproverReviewHtmlContent(userProfile, apiRevision)); } } @@ -135,10 +137,10 @@ public async Task UnsubscribeAsync(ReviewListItemModel review, ClaimsPrincipal u public static string GetUserEmail(ClaimsPrincipal user) => user.FindFirstValue(ClaimConstants.Email); - private string GetApproverReviewHtmlContent(UserProfileModel user, ReviewListItemModel review) + private string GetApproverReviewHtmlContent(UserProfileModel user, T model) where T : BaseListitemModel { - var reviewName = review.PackageName; - var reviewLink = new Uri($"{_apiviewEndpoint}/Assemblies/Review/{review.Id}"); + var reviewName = model.PackageName; + var reviewLink = new Uri($"{_apiviewEndpoint}/Assemblies/Review/{model.Id}"); var poster = user.UserName; var userLink = new Uri($"{_apiviewEndpoint}/Assemblies/Profile/{poster}"); var requestsLink = new Uri($"{_apiviewEndpoint}/Assemblies/RequestedReviews/"); @@ -183,7 +185,7 @@ private string GetHtmlContent(CommentItemModel comment, ReviewListItemModel revi private static string GetContentHeading(CommentItemModel comment, bool includeHtml) => $"{(includeHtml ? $"{comment.CreatedBy}" : $"{comment.CreatedBy}")} commented on this review."; - private async Task SendUserEmailsAsync(ReviewListItemModel review, UserProfileModel user, string htmlContent) + private async Task SendUserEmailsAsync(T model, UserProfileModel user, string htmlContent) where T : BaseListitemModel { // Always send email to a test address when test address is configured. if (string.IsNullOrEmpty(user.Email)) @@ -192,7 +194,7 @@ private async Task SendUserEmailsAsync(ReviewListItemModel review, UserProfileMo return; } - await SendEmail(user.Email, $"Notification from APIView - {review.PackageName}", htmlContent); + await SendEmail(user.Email, $"Notification from APIView - {model.PackageName}", htmlContent); } private async Task SendEmailsAsync(ReviewListItemModel review, ClaimsPrincipal user, string htmlContent, ISet notifiedUsers) { diff --git a/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs b/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs index 864d3488213..f076e089979 100644 --- a/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs +++ b/src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs @@ -79,16 +79,6 @@ public Task GetReviewAsync(string language, string packageN return _reviewsRepository.GetReviewAsync(language, packageName, isClosed); } - /// - /// Get Reviews that have been assigned for review to a user - /// - /// - /// - public async Task> GetReviewsAssignedToUser(string userName) - { - return await _reviewsRepository.GetReviewsAssignedToUser(userName); - } - /// /// Get List of Reviews for the Review Page /// @@ -266,32 +256,6 @@ public async Task ApproveReviewAsync(ClaimsPrincipal user, string reviewId, stri await ToggleReviewApproval(user, review, notes); } - /// - /// Assign reviewers to a review - /// - /// - /// - /// - /// - public async Task AssignReviewersToReviewAsync(ClaimsPrincipal User, string reviewId, HashSet reviewers) - { - ReviewListItemModel review = await _reviewsRepository.GetReviewAsync(reviewId); - foreach (var reviewer in reviewers) - { - if (!review.AssignedReviewers.Where(x => x.AssingedTo == reviewer).Any()) - { - var reviewAssignment = new ReviewAssignmentModel() - { - AssingedTo = reviewer, - AssignedBy = User.GetGitHubLogin(), - AssingedOn = DateTime.Now, - }; - review.AssignedReviewers.Add(reviewAssignment); - } - } - await _reviewsRepository.UpsertReviewAsync(review); - } - /// /// Sends info to AI service for generating initial review on APIReview file /// diff --git a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml index 6a9f236dedf..1c6b5e64e04 100644 --- a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml +++ b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml @@ -28,7 +28,7 @@ { @foreach (var apiRevision in Model.ActiveAPIRevisions) { - var assignment = Model.Reviews.Single(r => r.Id == apiRevision.ReviewId).AssignedReviewers.First(ar => ar.AssingedTo == User.GetGitHubLogin()); + var assignment = apiRevision.AssignedReviewers.First(ar => ar.AssingedTo == User.GetGitHubLogin()); var approvalRequestedOn = assignment.AssingedOn; var requestBy = assignment.AssignedBy; @@ -98,7 +98,7 @@ { @foreach (var apiRevision in Model.ApprovedAPIRevisions) { - var assignment = Model.Reviews.Single(r => r.Id == apiRevision.ReviewId).AssignedReviewers.First(ar => ar.AssingedTo == User.GetGitHubLogin()); + var assignment = apiRevision.AssignedReviewers.First(ar => ar.AssingedTo == User.GetGitHubLogin()); var approvalRequestedOn = assignment.AssingedOn; var requestBy = assignment.AssignedBy; diff --git a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml.cs b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml.cs index 67b884abaf2..0042e034612 100644 --- a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml.cs +++ b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/RequestedReviews.cshtml.cs @@ -17,7 +17,7 @@ public class RequestedReviews: PageModel private readonly IAPIRevisionsManager _apiRevisionsManager; public readonly UserPreferenceCache _preferenceCache; - public IEnumerable Reviews { get; set; } = new List(); + public IEnumerable APIRevisions { get; set; } = new List(); public IEnumerable ActiveAPIRevisions { get; set; } = new List(); public IEnumerable ApprovedAPIRevisions { get; set; } = new List(); @@ -30,16 +30,24 @@ public RequestedReviews(IReviewManager reviewManager, IAPIRevisionsManager apiRe public async Task OnGetAsync() { - Reviews = await _reviewManager.GetReviewsAssignedToUser(User.GetGitHubLogin()); + APIRevisions = await _apiRevisionsManager.GetAPIRevisionsAssignedToUser(User.GetGitHubLogin()); - foreach (var review in Reviews.OrderByDescending(r => r.AssignedReviewers.Select(x => x.AssingedOn))) + List activeAPIRevs = new List(); + List approvedAPIRevs = new List(); + foreach (var apiRevison in APIRevisions.OrderByDescending(r => r.AssignedReviewers.Select(x => x.AssingedOn))) { - var apiRevisoins = await _apiRevisionsManager.GetAPIRevisionsAsync(review.Id); - ActiveAPIRevisions = ActiveAPIRevisions.Concat(apiRevisoins.Where(r => r.IsApproved == false)); + if (!apiRevison.IsApproved) + { + activeAPIRevs.Add(apiRevison); + } - // Remove all approvals over a week old - ApprovedAPIRevisions = ApprovedAPIRevisions.Concat(apiRevisoins.Where(r => r.IsApproved == true).Where(r => r.ChangeHistory.First(c => c.ChangeAction == APIRevisionChangeAction.Approved).ChangedOn >= DateTime.Now.AddDays(-7))); + if (apiRevison.IsApproved && apiRevison.ChangeHistory.First(c => c.ChangeAction == APIRevisionChangeAction.Approved).ChangedOn >= DateTime.Now.AddDays(-7)) + { + approvedAPIRevs.Add(apiRevison); + } } + ActiveAPIRevisions = activeAPIRevs; + ApprovedAPIRevisions = approvedAPIRevs; ApprovedAPIRevisions.OrderByDescending(r => r.ChangeHistory.First(c => c.ChangeAction == APIRevisionChangeAction.Approved).ChangedOn); return Page(); diff --git a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml index 3a9d52a2487..789f7a57c49 100644 --- a/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml +++ b/src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml @@ -192,13 +192,14 @@ var anyChecked = false; }
+
    • @foreach (var approver in Model.ReviewContent.PreferredApprovers) {
    • - @if (Model.ReviewContent.Review.AssignedReviewers != null && Model.ReviewContent.Review.AssignedReviewers.Where(a => a.AssingedTo == approver).Any()) + @if (Model.ReviewContent.ActiveAPIRevision.AssignedReviewers != null && Model.ReviewContent.ActiveAPIRevision.AssignedReviewers.Where(a => a.AssingedTo == approver).Any()) {