From ba7f9e9a0d36186ca74e4a32e2660c1a48fa165c Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 19 Jul 2024 15:11:49 -0700 Subject: [PATCH 01/10] Add PR status --- .../DataManager/AzureDataManager.cs | 172 ++++++++++++++++-- .../DataManager/AzureDataManagerCache.cs | 2 +- .../DataManager/AzureDataManagerUpdate.cs | 71 ++++++++ .../DataManager/DataManagerUpdateEventArgs.cs | 1 + .../DataManager/IAzureDataManager.cs | 6 + .../DataModel/AzureDataStoreSchema.cs | 12 +- .../DataModel/DataObjects/Identity.cs | 44 +++-- .../DataObjects/PullRequestPolicyStatus.cs | 65 +++++++ .../DataModel/DataObjects/PullRequests.cs | 33 ++-- .../DataModel/DataObjects/Repository.cs | 45 +++++ .../DataObjects/RepositoryReference.cs | 26 ++- .../DataModel/Enums/PolicyStatus.cs | 16 ++ .../Widgets/AzurePullRequestsWidget.cs | 22 ++- src/AzureExtensionServer/Program.cs | 19 +- .../DataStore/DataObjectTests.cs | 12 +- 15 files changed, 489 insertions(+), 57 deletions(-) create mode 100644 src/AzureExtension/DataManager/AzureDataManagerUpdate.cs create mode 100644 src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs create mode 100644 src/AzureExtension/DataModel/Enums/PolicyStatus.cs diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index c0466c4..0e83b14 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -12,6 +12,7 @@ using DevHomeAzureExtension.DeveloperId; using DevHomeAzureExtension.Helpers; using Microsoft.TeamFoundation.Core.WebApi; +using Microsoft.TeamFoundation.Policy.WebApi; using Microsoft.TeamFoundation.SourceControl.WebApi; using Microsoft.TeamFoundation.WorkItemTracking.WebApi; using Microsoft.VisualStudio.Services.WebApi; @@ -262,6 +263,11 @@ public async Task UpdateDataForPullRequestsAsync(AzureUri repositoryUri, string return GetPullRequests(repositoryUri.Organization, repositoryUri.Project, repositoryUri.Repository, developerId, view); } + public PullRequests? GetPullRequestsForLoggedInDeveloperIds() + { + return null; + } + private async Task UpdateDataForQueriesAsync(DataStoreOperationParameters parameters) { _log.Debug($"Inside UpdateDataForQueriesAsync with Parameters: {parameters}"); @@ -548,6 +554,18 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p project = Project.GetOrCreateByTeamProject(DataStore, teamProject, org.Id); } + var repository = Repository.Get(DataStore, project.Id, azureUri.Repository); + if (repository is null) + { + var gitRepository = await gitClient.GetRepositoryAsync(project.InternalId, azureUri.Repository); + if (gitRepository is null) + { + throw new RepositoryNotFoundException(azureUri.Repository); + } + + repository = Repository.GetOrCreate(DataStore, gitRepository, project.Id); + } + var searchCriteria = new GitPullRequestSearchCriteria { Status = PullRequestStatus.Active, @@ -570,24 +588,77 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p break; } - var pullRequests = await gitClient.GetPullRequestsAsync(project.InternalId, azureUri.Repository, searchCriteria, null, null, PullRequestResultLimit); + var pullRequests = await gitClient.GetPullRequestsAsync(project.InternalId, repository.InternalId, searchCriteria, null, null, PullRequestResultLimit); if (pullRequests == null || pullRequests.Count == 0) { // If PRs were null or empty, this is a valid result, so create an empty record. - PullRequests.GetOrCreate(DataStore, azureUri.Repository, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, new JsonObject().ToJsonString()); + PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, new JsonObject().ToJsonString()); + + // If we had any statuses they should be removed here. return; } - // Convert relevant pull request items to Json. dynamic pullRequestsObj = new ExpandoObject(); var pullRequestsObjDict = (IDictionary)pullRequestsObj; + + var policyClient = result.Connection!.GetClient(); foreach (var pullRequest in pullRequests) { + var status = PolicyStatus.Unknown; + var statusReason = string.Empty; + try + { + // ArtifactId is null in the pull request object and it is not the correct object. The ArtifactId for the + // Policy Evaluations API is this: + // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} + var artifactId = $"vstfs:///CodeReview/CodeReviewId/{project.InternalId}/{pullRequest.PullRequestId}"; + Log.Information($"ArtifactId: {artifactId}"); + + var policyEvaluations = await policyClient.GetPolicyEvaluationsAsync(project.InternalId, artifactId); + if (policyEvaluations != null) + { + var countApplicablePolicies = 0; + foreach (var policyEvaluation in policyEvaluations) + { + if (policyEvaluation.Configuration.IsEnabled && policyEvaluation.Configuration.IsBlocking) + { + ++countApplicablePolicies; + var evalStatus = PullRequestPolicyStatus.GetFromPolicyEvaluationStatus(policyEvaluation.Status); + if (evalStatus < status) + { + statusReason = policyEvaluation.Configuration.Type.DisplayName; + status = evalStatus; + } + + Log.Information($" Policy: {policyEvaluation.EvaluationId} Name: {policyEvaluation.Configuration.Type.DisplayName} Enabled: {policyEvaluation.Configuration.IsEnabled} Blocking: {policyEvaluation.Configuration.IsBlocking} {policyEvaluation.Status}"); + } + } + + if (countApplicablePolicies == 0) + { + // If there is no applicable policy, treat the policy status as Approved. + status = PolicyStatus.Approved; + } + } + } + catch (Exception ex) + { + Log.Error(ex, $"Failed getting policy evaluations for pull request: {pullRequest.PullRequestId} {pullRequest.Url}"); + } + + // Set record + Log.Information($"PullRequest Status: {status} Reason: {statusReason}"); + + //// If this was a developer pull request, track status for notifications. + dynamic pullRequestObj = new ExpandoObject(); var pullRequestObjFields = (IDictionary)pullRequestObj; pullRequestObjFields.Add("Id", pullRequest.PullRequestId); pullRequestObjFields.Add("Title", pullRequest.Title); + pullRequestObjFields.Add("Status", pullRequest.Status); + pullRequestObjFields.Add("PolicyStatus", status.ToString()); + pullRequestObjFields.Add("PolicyStatusReason", statusReason); var creator = Identity.GetOrCreateIdentity(DataStore, pullRequest.CreatedBy, result.Connection); pullRequestObjFields.Add("CreatedBy", creator); @@ -611,12 +682,86 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p }; var serializedJson = JsonSerializer.Serialize(pullRequestsObj, serializerOptions); - PullRequests.GetOrCreate(DataStore, azureUri.Repository, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, serializedJson); + PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, serializedJson); } // Foreach AzureUri return; } + public async Task UpdatePullRequestsForLoggedInDeveloperIdsAsync(RequestOptions? options = null, Guid? requestor = null) + { + ValidateDataStore(); + var parameters = new DataStoreOperationParameters + { + RequestOptions = options ?? RequestOptions.RequestOptionsDefault(), + OperationName = "UpdatePullRequestsForLoggedInDeveloperIdsAsync", + PullRequestView = PullRequestView.Mine, + Requestor = requestor ?? Guid.NewGuid(), + }; + + dynamic context = new ExpandoObject(); + var contextDict = (IDictionary)context; + contextDict.Add("Requestor", parameters.Requestor); + + try + { + await UpdateDataStoreAsync(parameters, UpdateDataForDeveloperPullRequestsAsync); + } + catch (Exception ex) + { + contextDict.Add("ErrorMessage", ex.Message); + SendErrorUpdateEvent(_log, this, parameters.Requestor, context, ex); + return; + } + + SendPullRequestUpdateEvent(_log, this, parameters.Requestor, context); + } + + private async Task UpdateDataForDeveloperPullRequestsAsync(DataStoreOperationParameters parameters) + { + _log.Debug($"Inside UpdateDataForDeveloperPullRequestsAsync with Parameters: {parameters}"); + + dynamic context = new ExpandoObject(); + var contextDict = (IDictionary)context; + contextDict.Add("Requestor", parameters.Requestor); + + try + { + // This is a loop over a subset of repositories with a specific developer ID and pull request view specified. + var repositoryReferences = RepositoryReference.GetAll(DataStore); + foreach (var repositoryRef in repositoryReferences) + { + var uri = new AzureUri(repositoryRef.Repository.CloneUrl); + var developerId = repositoryRef.Developer.DeveloperLoginId; + + var uris = new List + { + new(repositoryRef.Repository.CloneUrl), + }; + + var suboperationParameters = new DataStoreOperationParameters + { + Uris = uris, + DeveloperId = repositoryRef.Developer.DeveloperId, + RequestOptions = parameters.RequestOptions, + OperationName = "UpdateDataForDeveloperPullRequestsAsync", + PullRequestView = PullRequestView.Mine, + Requestor = parameters.Requestor, + }; + + await UpdateDataForPullRequestsAsync(suboperationParameters); + } + } + catch (Exception ex) + { + contextDict.Add("ErrorMessage", ex.Message); + SendErrorUpdateEvent(_log, this, parameters.Requestor, context, ex); + return; + } + + SendDeveloperUpdateEvent(_log, this, parameters.Requestor, context); + } + public TeamProject GetTeamProject(string projectName, DeveloperId.DeveloperId developerId, Uri connection) { var result = GetConnection(connection, developerId); @@ -651,12 +796,6 @@ public TeamProject GetTeamProject(string projectName, DeveloperId.DeveloperId de private async Task UpdateDataStoreAsync(DataStoreOperationParameters parameters, Func asyncAction) { parameters.RequestOptions ??= RequestOptions.RequestOptionsDefault(); - if (parameters.DeveloperId == null) - { - _log.Error($"Specified DeveloperId was not found: {parameters.LoginId}"); - throw new ArgumentException($"Specified DeveloperId was not found:"); - } - using var tx = DataStore.Connection!.BeginTransaction(); try @@ -696,6 +835,11 @@ private static void SendErrorUpdateEvent(ILogger logger, object? source, Guid re SendUpdateEvent(logger, source, DataManagerUpdateKind.Error, requestor, context, ex); } + private static void SendDeveloperUpdateEvent(ILogger logger, object? source, Guid requestor, dynamic context, Exception? ex = null) + { + SendUpdateEvent(logger, source, DataManagerUpdateKind.Developer, requestor, context, ex); + } + private static void SendCancelUpdateEvent(ILogger logger, object? source, Guid requestor, dynamic context, Exception? ex = null) { SendUpdateEvent(logger, source, DataManagerUpdateKind.Cancel, requestor, context, ex); @@ -763,6 +907,12 @@ public static ConnectionResult GetConnection(Uri connectionUri, DeveloperId.Deve } public IEnumerable GetRepositories() + { + ValidateDataStore(); + return Repository.GetAll(DataStore); + } + + public IEnumerable GetDeveloperRepositories() { ValidateDataStore(); return Repository.GetAllWithReference(DataStore); @@ -792,12 +942,12 @@ private void PruneObsoleteData() Query.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); PullRequests.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); WorkItemType.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); - Identity.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); // The following are not yet pruned, need to ensure there are no current key references // before deletion. // * Projects are referenced by Pull Requests and Queries. // * Organizations are referenced by Projects. + // * Identity is referenced by ProjectReference and RepositoryReference } // Sets a last-updated in the MetaData. diff --git a/src/AzureExtension/DataManager/AzureDataManagerCache.cs b/src/AzureExtension/DataManager/AzureDataManagerCache.cs index 50a8590..13ddaa8 100644 --- a/src/AzureExtension/DataManager/AzureDataManagerCache.cs +++ b/src/AzureExtension/DataManager/AzureDataManagerCache.cs @@ -150,7 +150,7 @@ private dynamic CreateUpdateEventContext(int errors, int accountsUpdated, int ac private void UpdateOrganization(Account account, DeveloperId.DeveloperId developerId, VssConnection connection, CancellationToken cancellationToken) { // Update account identity information: - var identity = Identity.GetOrCreateIdentity(DataStore, connection.AuthorizedIdentity, connection, true); + var identity = Identity.GetOrCreateIdentity(DataStore, connection.AuthorizedIdentity, connection, developerId.LoginId); _log.Verbose($"Updating organization: {account.AccountName}"); var organization = Organization.GetOrCreate(DataStore, account.AccountUri); diff --git a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs new file mode 100644 index 0000000..f2fde74 --- /dev/null +++ b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using DevHomeAzureExtension.DataManager; +using Microsoft.TeamFoundation.Policy.WebApi; +using Serilog; + +namespace DevHomeAzureExtension; + +public partial class AzureDataManager +{ + // This is how frequently the DataStore update occurs. + private static readonly TimeSpan _updateInterval = TimeSpan.FromMinutes(5); + private static DateTime _lastUpdateTime = DateTime.MinValue; + + public static async Task Update() + { + // Only update per the update interval. + // This is intended to be dynamic in the future. + if (DateTime.Now - _lastUpdateTime < _updateInterval) + { + return; + } + + try + { + await UpdateDeveloperPullRequests(); + } + catch (Exception ex) + { + Log.Error(ex, "Update failed unexpectedly."); + } + + _lastUpdateTime = DateTime.Now; + } + + public static async Task UpdateDeveloperPullRequests() + { + Log.Debug($"Executing UpdateDeveloperPullRequests"); + + var cacheManager = CacheManager.GetInstance(); + if (cacheManager.UpdateInProgress) + { + Log.Information("Cache is being updated, skipping Developer Pull Request Update"); + return; + } + + var identifier = Guid.NewGuid(); + using var dataManager = CreateInstance(identifier.ToString()) ?? throw new DataStoreInaccessibleException(); + await dataManager.UpdatePullRequestsForLoggedInDeveloperIdsAsync(null, identifier); + + // Show any new notifications that were created from the pull request update. + /* + var notifications = dataManager.GetNotifications(); + foreach (var notification in notifications) + { + // Show notifications for failed checkruns for Developer users. + if (notification.Type == NotificationType.CheckRunFailed && notification.User.IsDeveloper) + { + notification.ShowToast(); + } + + // Show notifications for new reviews. + if (notification.Type == NotificationType.NewReview) + { + notification.ShowToast(); + } + } + */ + } +} diff --git a/src/AzureExtension/DataManager/DataManagerUpdateEventArgs.cs b/src/AzureExtension/DataManager/DataManagerUpdateEventArgs.cs index a48f3c3..2a3873e 100644 --- a/src/AzureExtension/DataManager/DataManagerUpdateEventArgs.cs +++ b/src/AzureExtension/DataManager/DataManagerUpdateEventArgs.cs @@ -12,6 +12,7 @@ public enum DataManagerUpdateKind Error, Cache, Cancel, + Developer, } public class DataManagerUpdateEventArgs : EventArgs diff --git a/src/AzureExtension/DataManager/IAzureDataManager.cs b/src/AzureExtension/DataManager/IAzureDataManager.cs index b7bc602..ec637d5 100644 --- a/src/AzureExtension/DataManager/IAzureDataManager.cs +++ b/src/AzureExtension/DataManager/IAzureDataManager.cs @@ -26,15 +26,21 @@ public interface IAzureDataManager : IDisposable Task UpdateDataForPullRequestsAsync(AzureUri repositoryUri, string developerLogin, PullRequestView view, RequestOptions? options = null, Guid? requestor = null); + Task UpdatePullRequestsForLoggedInDeveloperIdsAsync(RequestOptions? options = null, Guid? requestor = null); + Query? GetQuery(string queryId, string developerId); Query? GetQuery(AzureUri queryUri, string developerId); IEnumerable GetRepositories(); + IEnumerable GetDeveloperRepositories(); + // Repository name may not be unique across projects, and projects may not be unique across // organizations, so we need all three to identify the repository. PullRequests? GetPullRequests(string organization, string project, string repositoryName, string developerId, PullRequestView view); PullRequests? GetPullRequests(AzureUri repositoryUri, string developerId, PullRequestView view); + + PullRequests? GetPullRequestsForLoggedInDeveloperIds(); } diff --git a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs index cf96ec9..27ced27 100644 --- a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs +++ b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs @@ -14,7 +14,7 @@ public AzureDataStoreSchema() } // Update this anytime incompatible changes happen with a released version. - private const long SchemaVersionValue = 0x0006; + private const long SchemaVersionValue = 0x0007; private const string Metadata = @"CREATE TABLE Metadata (" + @@ -30,7 +30,7 @@ public AzureDataStoreSchema() "Name TEXT NOT NULL COLLATE NOCASE," + "InternalId TEXT NOT NULL," + "Avatar TEXT NOT NULL COLLATE NOCASE," + - "IsDeveloper INTEGER NOT NULL," + + "DeveloperLoginId TEXT NOT NULL," + "TimeUpdated INTEGER NOT NULL" + ");" + @@ -89,6 +89,10 @@ public AzureDataStoreSchema() "TimeUpdated INTEGER NOT NULL" + ");" + + // While Name and ProjectId should be unique, it is possible renaming occurs and + // we might have a collision if we have cached a repository prior to rename and + // then encounter a different repository with that name. Therefore we will not + // create a unique index on ProjectId and Name. // Repository InternalId is a Guid, so by definition is unique. "CREATE UNIQUE INDEX IDX_Repository_InternalId ON Repository (InternalId);"; @@ -138,7 +142,7 @@ public AzureDataStoreSchema() private const string PullRequests = @"CREATE TABLE PullRequests (" + "Id INTEGER PRIMARY KEY NOT NULL," + - "RepositoryName TEXT NOT NULL COLLATE NOCASE," + + "RepositoryId INTEGER NOT NULL," + "DeveloperLogin TEXT NOT NULL COLLATE NOCASE," + "Results TEXT NOT NULL," + "ProjectId INTEGER NOT NULL," + @@ -148,7 +152,7 @@ public AzureDataStoreSchema() // Developer Pull requests are unique on Org / Project / Repository and // the developer login, and the view. - "CREATE UNIQUE INDEX IDX_PullRequests_ProjectIdRepositoryNameDeveloperLoginViewId ON PullRequests (ProjectId, RepositoryName, DeveloperLogin, ViewId);"; + "CREATE UNIQUE INDEX IDX_PullRequests_ProjectIdRepositoryIdDeveloperLoginViewId ON PullRequests (ProjectId, RepositoryId, DeveloperLogin, ViewId);"; // All Sqls together. private static readonly List _schemaSqlsValue = diff --git a/src/AzureExtension/DataModel/DataObjects/Identity.cs b/src/AzureExtension/DataModel/DataObjects/Identity.cs index 2bb03e5..0fcbf4e 100644 --- a/src/AzureExtension/DataModel/DataObjects/Identity.cs +++ b/src/AzureExtension/DataModel/DataObjects/Identity.cs @@ -5,6 +5,7 @@ using System.Text.Json.Serialization; using Dapper; using Dapper.Contrib.Extensions; +using DevHomeAzureExtension.DeveloperId; using DevHomeAzureExtension.Helpers; using Microsoft.VisualStudio.Services.Profile; using Microsoft.VisualStudio.Services.Profile.Client; @@ -13,6 +14,7 @@ namespace DevHomeAzureExtension.DataModel; +// This represents an Azure DevOps Identity or IdentityRef. [Table("Identity")] public class Identity { @@ -42,9 +44,9 @@ public class Identity public string Avatar { get; set; } = string.Empty; - // Represents whether this identity is associated with a DeveloperId that is logged in. - // This is the backing database column for the IsLoggedInDeveloper property. - public long IsDeveloper { get; set; } = DataStore.NoForeignKey; + // The DeveloperLoginId associated with this identity, if one exists. + // Empty means there is no associated LoggedInDeveloperId. + public string DeveloperLoginId { get; set; } = string.Empty; [JsonIgnore] public long TimeUpdated { get; set; } = DataStore.NoForeignKey; @@ -57,7 +59,23 @@ public class Identity [Write(false)] [Computed] [JsonIgnore] - public bool IsLoggedInDeveloper => IsDeveloper != 0L; + public bool IsLoggedInDeveloper => !string.IsNullOrEmpty(DeveloperLoginId); + + [Write(false)] + [Computed] + public DeveloperId.DeveloperId? DeveloperId + { + get + { + if (!IsLoggedInDeveloper) + { + return null; + } + + var devIdProvider = DeveloperIdProvider.GetInstance(); + return devIdProvider.GetDeveloperIdFromAccountIdentifier(DeveloperLoginId); + } + } public string ToJson() => JsonSerializer.Serialize(this); @@ -145,7 +163,7 @@ private static Identity CreateFromIdentity(Microsoft.VisualStudio.Services.Ident }; } - public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identity, bool isDeveloper = false) + public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identity, string developerLoginId = "") { // Check for existing Identity data. var existingIdentity = GetByInternalId(dataStore, identity.InternalId); @@ -153,11 +171,11 @@ public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identit { identity.Id = existingIdentity.Id; - // If this is a developer, set to developer, but do not set to false. + // If this is a developer, set to developer, but do not set it. // We presume not a developer unless it is explicitly set. - if (isDeveloper) + if (!string.IsNullOrEmpty(developerLoginId)) { - identity.IsDeveloper = 1; + identity.DeveloperLoginId = developerLoginId; } dataStore.Connection!.Update(identity); @@ -192,7 +210,7 @@ public static Identity Get(DataStore? dataStore, long id) } // Creation from an Azure IdentityRef object. - public static Identity GetOrCreateIdentity(DataStore dataStore, IdentityRef? identityRef, VssConnection connection, bool isDeveloper = false) + public static Identity GetOrCreateIdentity(DataStore dataStore, IdentityRef? identityRef, VssConnection connection, string developerLoginId = "") { ArgumentNullException.ThrowIfNull(identityRef); @@ -210,18 +228,19 @@ public static Identity GetOrCreateIdentity(DataStore dataStore, IdentityRef? ide // We don't want to create an identity object and download a new avatar unless it needs to // be updated. In the event of an empty avatar we will retry more frequently to update it, // but not every time. + var isDeveloper = !string.IsNullOrEmpty(developerLoginId); if (existing is null || (isDeveloper && !existing.IsLoggedInDeveloper) || ((DateTime.UtcNow - existing.UpdatedAt) > _updateThreshold) || (string.IsNullOrEmpty(existing.Avatar) && ((DateTime.UtcNow - existing.UpdatedAt) > _avatarRetryDelay))) { var newIdentity = CreateFromIdentityRef(identityRef, connection); - return AddOrUpdateIdentity(dataStore, newIdentity, isDeveloper); + return AddOrUpdateIdentity(dataStore, newIdentity, developerLoginId); } return existing; } // Creation from an Azure Identity object. - public static Identity GetOrCreateIdentity(DataStore dataStore, Microsoft.VisualStudio.Services.Identity.Identity? identity, VssConnection connection, bool isDeveloper = false) + public static Identity GetOrCreateIdentity(DataStore dataStore, Microsoft.VisualStudio.Services.Identity.Identity? identity, VssConnection connection, string developerLoginId = "") { ArgumentNullException.ThrowIfNull(identity); @@ -232,11 +251,12 @@ public static Identity GetOrCreateIdentity(DataStore dataStore, Microsoft.Visual // We don't want to create an identity object and download a new avatar unlesss it needs to // be updated. In the event of an empty avatar we will retry more frequently to update it, // but not every time. + var isDeveloper = !string.IsNullOrEmpty(developerLoginId); if (existing is null || (isDeveloper && !existing.IsLoggedInDeveloper) || ((DateTime.UtcNow - existing.UpdatedAt) > _updateThreshold) || (string.IsNullOrEmpty(existing.Avatar) && ((DateTime.UtcNow - existing.UpdatedAt) > _avatarRetryDelay))) { var newIdentity = CreateFromIdentity(identity, connection); - return AddOrUpdateIdentity(dataStore, newIdentity, isDeveloper); + return AddOrUpdateIdentity(dataStore, newIdentity, developerLoginId); } return existing; diff --git a/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs new file mode 100644 index 0000000..64ad0a7 --- /dev/null +++ b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs @@ -0,0 +1,65 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Dapper; +using Dapper.Contrib.Extensions; +using DevHomeAzureExtension.Helpers; +using Microsoft.TeamFoundation.Policy.WebApi; +using Serilog; + +namespace DevHomeAzureExtension.DataModel; + +[Table("PullRequestPolicyStatus")] +public class PullRequestPolicyStatus +{ + private static readonly Lazy _logger = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(PullRequests)}")); + + private static readonly ILogger _log = _logger.Value; + + [Key] + public long Id { get; set; } = DataStore.NoForeignKey; + + // This should suffice as a unique identifier. + public string ArtifactId { get; set; } = string.Empty; + + // Key in Project table + public long ProjectId { get; set; } = DataStore.NoForeignKey; + + public long PullRequestId { get; set; } = DataStore.NoForeignKey; + + public long TimeUpdated { get; set; } = DataStore.NoForeignKey; + + public long PolicyStatusId { get; set; } = DataStore.NoForeignKey; + + public override string ToString() => ArtifactId; + + [Write(false)] + private DataStore? DataStore { get; set; } + + [Write(false)] + [Computed] + public Project Project => Project.Get(DataStore, ProjectId); + + [Write(false)] + [Computed] + public PolicyStatus PolicyStatus => (PolicyStatus)PolicyStatusId; + + [Write(false)] + [Computed] + public DateTime UpdatedAt => TimeUpdated.ToDateTime(); + + // Map PolicyEvaluationStatus to our enum. Our enum is sorted, but the PolicyEvaluationStatus is not. + public static PolicyStatus GetFromPolicyEvaluationStatus(PolicyEvaluationStatus? policyEvaluationStatus) + { + return policyEvaluationStatus switch + { + PolicyEvaluationStatus.NotApplicable => PolicyStatus.NotApplicable, + PolicyEvaluationStatus.Approved => PolicyStatus.Approved, + PolicyEvaluationStatus.Rejected => PolicyStatus.Rejected, + PolicyEvaluationStatus.Queued => PolicyStatus.Queued, + PolicyEvaluationStatus.Running => PolicyStatus.Running, + PolicyEvaluationStatus.Broken => PolicyStatus.Broken, + _ => PolicyStatus.Unknown, + }; + } +} diff --git a/src/AzureExtension/DataModel/DataObjects/PullRequests.cs b/src/AzureExtension/DataModel/DataObjects/PullRequests.cs index 9067be0..e324669 100644 --- a/src/AzureExtension/DataModel/DataObjects/PullRequests.cs +++ b/src/AzureExtension/DataModel/DataObjects/PullRequests.cs @@ -22,7 +22,8 @@ public class PullRequests [Key] public long Id { get; set; } = DataStore.NoForeignKey; - public string RepositoryName { get; set; } = string.Empty; + // Key in Repository table + public long RepositoryId { get; set; } = DataStore.NoForeignKey; // Key in Project table public long ProjectId { get; set; } = DataStore.NoForeignKey; @@ -36,7 +37,7 @@ public class PullRequests public long TimeUpdated { get; set; } = DataStore.NoForeignKey; - public override string ToString() => DeveloperLogin + "/" + RepositoryName; + public override string ToString() => DeveloperLogin + "/" + Repository.Name; [Write(false)] private DataStore? DataStore { get; set; } @@ -45,6 +46,10 @@ public class PullRequests [Computed] public Project Project => Project.Get(DataStore, ProjectId); + [Write(false)] + [Computed] + public Repository Repository => Repository.Get(DataStore, RepositoryId); + [Write(false)] [Computed] public PullRequestView View => (PullRequestView)ViewId; @@ -57,11 +62,11 @@ public class PullRequests [Computed] public DateTime UpdatedAt => TimeUpdated.ToDateTime(); - private static PullRequests Create(string repositoryName, long projectId, string developerLogin, PullRequestView view, string pullRequests) + private static PullRequests Create(long repositoryId, long projectId, string developerLogin, PullRequestView view, string pullRequests) { return new PullRequests { - RepositoryName = repositoryName, + RepositoryId = repositoryId, ProjectId = projectId, DeveloperLogin = developerLogin, Results = pullRequests, @@ -72,7 +77,7 @@ private static PullRequests Create(string repositoryName, long projectId, string private static PullRequests AddOrUpdate(DataStore dataStore, PullRequests pullRequests) { - var existing = Get(dataStore, pullRequests.ProjectId, pullRequests.RepositoryName, pullRequests.DeveloperLogin, pullRequests.View); + var existing = Get(dataStore, pullRequests.ProjectId, pullRequests.RepositoryId, pullRequests.DeveloperLogin, pullRequests.View); if (existing is not null) { // Update threshold is in case there are many requests in a short period of time. @@ -110,13 +115,13 @@ public static PullRequests Get(DataStore dataStore, long id) return pullRequests ?? new PullRequests(); } - public static PullRequests? Get(DataStore dataStore, long projectId, string repositoryName, string developerLogin, PullRequestView view) + public static PullRequests? Get(DataStore dataStore, long projectId, long repositoryId, string developerLogin, PullRequestView view) { - var sql = @"SELECT * FROM PullRequests WHERE ProjectId = @ProjectId AND RepositoryName = @RepositoryName AND DeveloperLogin = @DeveloperLogin AND ViewId = @ViewId;"; + var sql = @"SELECT * FROM PullRequests WHERE ProjectId = @ProjectId AND RepositoryId = @RepositoryId AND DeveloperLogin = @DeveloperLogin AND ViewId = @ViewId;"; var param = new { ProjectId = projectId, - RepositoryName = repositoryName, + RepositoryId = repositoryId, DeveloperLogin = developerLogin, ViewId = (long)view, }; @@ -140,12 +145,18 @@ public static PullRequests Get(DataStore dataStore, long id) return null; } - return Get(dataStore, project.Id, repositoryName, developerLogin, view); + var repository = Repository.Get(dataStore, project.Id, repositoryName); + if (repository == null) + { + return null; + } + + return Get(dataStore, project.Id, repository.Id, developerLogin, view); } - public static PullRequests GetOrCreate(DataStore dataStore, string repositoryName, long projectId, string developerId, PullRequestView view, string pullRequests) + public static PullRequests GetOrCreate(DataStore dataStore, long repositoryId, long projectId, string developerId, PullRequestView view, string pullRequests) { - var newDeveloperPullRequests = Create(repositoryName, projectId, developerId, view, pullRequests); + var newDeveloperPullRequests = Create(repositoryId, projectId, developerId, view, pullRequests); return AddOrUpdate(dataStore, newDeveloperPullRequests); } diff --git a/src/AzureExtension/DataModel/DataObjects/Repository.cs b/src/AzureExtension/DataModel/DataObjects/Repository.cs index 90b6f95..f6eb137 100644 --- a/src/AzureExtension/DataModel/DataObjects/Repository.cs +++ b/src/AzureExtension/DataModel/DataObjects/Repository.cs @@ -21,6 +21,7 @@ public class Repository [Key] public long Id { get; set; } = DataStore.NoForeignKey; + // Name should be unique within a project scope. public string Name { get; set; } = string.Empty; // Guid representation by ADO. @@ -130,6 +131,29 @@ public static Repository Get(DataStore? dataStore, long id) return repository ?? new Repository(); } + public static Repository? GetByName(DataStore? dataStore, long projectId, string name) + { + if (dataStore == null) + { + return null; + } + + var sql = @"SELECT * FROM Repository WHERE ProjectId = @ProjectId AND Name = @Name;"; + var param = new + { + ProjectId = projectId, + Name = name, + }; + + var repository = dataStore.Connection!.QueryFirstOrDefault(sql, param, null); + if (repository != null) + { + repository.DataStore = dataStore; + } + + return repository; + } + public static IEnumerable GetAll(DataStore dataStore) { var repositories = dataStore.Connection!.GetAll() ?? []; @@ -165,6 +189,27 @@ public static IEnumerable GetAll(DataStore dataStore) return GetByInternalId(dataStore, repository.Id.ToString()); } + public static Repository? Get(DataStore dataStore, long projectId, string repositoryName) + { + var sql = @"SELECT * FROM Repository WHERE ProjectId = @ProjectId AND Name = @RepositoryName;"; + var param = new + { + ProjectId = projectId, + RepositoryName = repositoryName, + }; + + // In rare cases we might have a rename situation that results in more than one record. + // This should be an extremely rare edge case that can be resolved in the next repository + // cache update so we will assume a single record is correct. + var repository = dataStore.Connection!.QueryFirstOrDefault(sql, param, null); + if (repository != null) + { + repository.DataStore = dataStore; + } + + return repository; + } + public static IEnumerable GetAllWithReference(DataStore dataStore) { var sql = @"SELECT * FROM Repository AS R WHERE R.Id IN (SELECT RepositoryId FROM RepositoryReference)"; diff --git a/src/AzureExtension/DataModel/DataObjects/RepositoryReference.cs b/src/AzureExtension/DataModel/DataObjects/RepositoryReference.cs index a40294b..3e189e5 100644 --- a/src/AzureExtension/DataModel/DataObjects/RepositoryReference.cs +++ b/src/AzureExtension/DataModel/DataObjects/RepositoryReference.cs @@ -15,11 +15,11 @@ namespace DevHomeAzureExtension.DataModel; [Table("RepositoryReference")] public class RepositoryReference { - private static readonly Lazy _log = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(RepositoryReference)}")); + private static readonly Lazy _logger = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(RepositoryReference)}")); - private static readonly ILogger Log = _log.Value; + private static readonly ILogger _log = _logger.Value; - private static readonly long WeightPullRequest = 1; + private static readonly long _weightPullRequest = 1; [Key] public long Id { get; set; } = DataStore.NoForeignKey; @@ -33,12 +33,16 @@ public class RepositoryReference [Write(false)] [Computed] - public long Value => PullRequestCount * WeightPullRequest; + public long Value => PullRequestCount * _weightPullRequest; [Write(false)] [Computed] public Identity Developer => Identity.Get(DataStore, DeveloperId); + [Write(false)] + [Computed] + public Repository Repository => Repository.Get(DataStore, RepositoryId); + [Write(false)] private DataStore? DataStore { get; set; } @@ -99,13 +103,25 @@ public static RepositoryReference GetOrCreate(DataStore dataStore, long reposito return repositoryReference; } + public static IEnumerable GetAll(DataStore dataStore) + { + var repositoryReferences = dataStore.Connection!.GetAll() ?? []; + foreach (var repositoryReference in repositoryReferences) + { + repositoryReference.DataStore = dataStore; + } + + _log.Verbose("Getting all repository references."); + return repositoryReferences; + } + public static void DeleteUnreferenced(DataStore dataStore) { // Delete any RepositoryReferences for repositories that do not exist. var sql = @"DELETE FROM RepositoryReference WHERE (RepositoryId NOT IN (SELECT Id FROM Repository)) OR (DeveloperId NOT IN (SELECT Id FROM Identity))"; var command = dataStore.Connection!.CreateCommand(); command.CommandText = sql; - Log.Verbose(DataStore.GetCommandLogMessage(sql, command)); + _log.Verbose(DataStore.GetCommandLogMessage(sql, command)); command.ExecuteNonQuery(); } } diff --git a/src/AzureExtension/DataModel/Enums/PolicyStatus.cs b/src/AzureExtension/DataModel/Enums/PolicyStatus.cs new file mode 100644 index 0000000..f1d355e --- /dev/null +++ b/src/AzureExtension/DataModel/Enums/PolicyStatus.cs @@ -0,0 +1,16 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace DevHomeAzureExtension.DataModel; + +public enum PolicyStatus +{ + // Sorted by severity. Most severe is lowest. + Broken = 1, + Rejected = 2, + Queued = 3, + Running = 4, + Approved = 5, + NotApplicable = 6, + Unknown = 7, +} diff --git a/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs b/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs index 894f17a..27970c6 100644 --- a/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs +++ b/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs @@ -62,13 +62,21 @@ private PullRequestView GetPullRequestView(string viewStr) private string GetIconForPullRequestStatus(string? prStatus) { - return prStatus switch + prStatus ??= string.Empty; + if (Enum.TryParse(prStatus, false, out var policyStatus)) { - "Approved" => IconLoader.GetIconAsBase64("PullRequestApproved.png"), - "Waiting" => IconLoader.GetIconAsBase64("PullRequestWaiting.png"), - "Rejected" => IconLoader.GetIconAsBase64("PullRequestRejected.png"), - _ => IconLoader.GetIconAsBase64("PullRequestReviewNotStarted.png"), - }; + return policyStatus switch + { + PolicyStatus.Approved => IconLoader.GetIconAsBase64("PullRequestApproved.png"), + PolicyStatus.Running => IconLoader.GetIconAsBase64("PullRequestWaiting.png"), + PolicyStatus.Queued => IconLoader.GetIconAsBase64("PullRequestWaiting.png"), + PolicyStatus.Rejected => IconLoader.GetIconAsBase64("PullRequestRejected.png"), + PolicyStatus.Broken => IconLoader.GetIconAsBase64("PullRequestRejected.png"), + _ => IconLoader.GetIconAsBase64("PullRequestReviewNotStarted.png"), + }; + } + + return string.Empty; } protected override bool ValidateConfiguration(WidgetActionInvokedArgs args) @@ -294,7 +302,7 @@ public override void LoadContentData() { { "title", workItem["Title"]?.GetValue() ?? string.Empty }, { "url", workItem["HtmlUrl"]?.GetValue() ?? string.Empty }, - { "status_icon", GetIconForPullRequestStatus(workItem["Status"]?.GetValue()) }, + { "status_icon", GetIconForPullRequestStatus(workItem["PolicyStatus"]?.GetValue()) }, { "number", element.Key }, { "date", TimeSpanHelper.DateTimeOffsetToDisplayString(dateTime, Log) }, { "user", workItem["CreatedBy"]?["Name"]?.GetValue() ?? string.Empty }, diff --git a/src/AzureExtensionServer/Program.cs b/src/AzureExtensionServer/Program.cs index 936db8a..e592a30 100644 --- a/src/AzureExtensionServer/Program.cs +++ b/src/AzureExtensionServer/Program.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using DevHomeAzureExtension.Contracts; +using DevHomeAzureExtension.DataManager; using DevHomeAzureExtension.DataModel; using DevHomeAzureExtension.DevBox; using DevHomeAzureExtension.DevBox.Models; @@ -157,15 +158,31 @@ private static void HandleCOMServerActivation() widgetServer.RegisterWidget(() => widgetProviderInstance); // Cache manager updates account data. - using var cacheManager = DataManager.CacheManager.GetInstance(); + using var cacheManager = CacheManager.GetInstance(); cacheManager?.Start(); + // Set up the data updater. This will schedule updating the Developer Pull Requests. + using var dataUpdater = new DataUpdater(AzureDataManager.Update); + _ = dataUpdater.Start(); + + // Add an update whenever CacheManager is updated. + CacheManager.GetInstance().OnUpdate += HandleCacheUpdate; + // This will make the main thread wait until the event is signaled by the extension class. // Since we have single instance of the extension object, we exit as soon as it is disposed. extensionDisposedEvent.WaitOne(); Log.Information($"Extension is disposed."); } + private static void HandleCacheUpdate(object? source, CacheManagerUpdateEventArgs e) + { + if (e.Kind == CacheManagerUpdateKind.Updated) + { + Log.Debug("Cache was updated, updating developer pull requests."); + _ = AzureDataManager.UpdateDeveloperPullRequests(); + } + } + private static void LogPackageInformation() { var relatedPackageFamilyNames = new string[] diff --git a/test/AzureExtension/DataStore/DataObjectTests.cs b/test/AzureExtension/DataStore/DataObjectTests.cs index a908111..ee7a44e 100644 --- a/test/AzureExtension/DataStore/DataObjectTests.cs +++ b/test/AzureExtension/DataStore/DataObjectTests.cs @@ -345,9 +345,11 @@ public void ReadAndWritePullRequests() var org = Organization.GetOrCreate(dataStore, new Uri("https://dev.azure.com/organization/")); Assert.IsNotNull(org); dataStore.Connection.Insert(new Project { Name = "project", InternalId = "11", OrganizationId = org.Id }); + dataStore.Connection.Insert(new Repository { Name = "repository1", InternalId = "21", CloneUrl = "https://organization/project/_git/repository1/", ProjectId = 1 }); + dataStore.Connection.Insert(new Repository { Name = "repository2", InternalId = "22", CloneUrl = "https://organization/project/_git/repository2/", ProjectId = 1 }); - var p1 = PullRequests.GetOrCreate(dataStore, "repository1", 1, "foo@bar", PullRequestView.Mine, "Results"); - var p2 = PullRequests.GetOrCreate(dataStore, "repository2", 1, "foo@bar", PullRequestView.Mine, "Results"); + var p1 = PullRequests.GetOrCreate(dataStore, 1, 1, "foo@bar", PullRequestView.Mine, "Results"); + var p2 = PullRequests.GetOrCreate(dataStore, 2, 1, "foo@bar", PullRequestView.Mine, "Results"); tx.Commit(); // Verify retrieval and input into data objects. @@ -358,15 +360,15 @@ public void ReadAndWritePullRequests() var pull = PullRequests.Get(dataStore, i); Assert.IsNotNull(pull); Assert.AreEqual($"foo@bar", pull.DeveloperLogin); - Assert.AreEqual($"repository{i}", pull.RepositoryName); + Assert.AreEqual($"repository{i}", pull.Repository.Name); Assert.AreEqual("organization", pull.Project.Organization.Name); Assert.AreEqual(PullRequestView.Mine, pull.View); - TestContext?.WriteLine($" Name: {pull.RepositoryName} Results: {pull.Results}"); + TestContext?.WriteLine($" Name: {pull.Repository.Name} Results: {pull.Results}"); } var findPull = PullRequests.Get(dataStore, "organization", "project", "repository2", "foo@bar", PullRequestView.Mine); Assert.IsNotNull(findPull); - Assert.AreEqual("repository2", findPull.RepositoryName); + Assert.AreEqual("repository2", findPull.Repository.Name); Assert.AreEqual(PullRequestView.Mine, findPull.View); Assert.AreEqual("project", findPull.Project.Name); } From 8adb91bfc760818a2dacd3772a7cb476e9a1d319 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 19 Jul 2024 16:56:56 -0700 Subject: [PATCH 02/10] Use workitem and identity index lookups instead of embedding --- .../DataManager/AzureDataManager.cs | 24 ++++++++++++------- .../DataManager/IAzureDataManager.cs | 4 ++++ .../Widgets/AzurePullRequestsWidget.cs | 6 ++--- .../Widgets/AzureQueryListWidget.cs | 9 +++---- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index 0e83b14..dfa4fd7 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -247,6 +247,18 @@ public async Task UpdateDataForPullRequestsAsync(AzureUri repositoryUri, string return GetQuery(queryUri.Query, developerId); } + public Identity GetIdentity(long id) + { + ValidateDataStore(); + return Identity.Get(DataStore, id); + } + + public WorkItemType GetWorkItemType(long id) + { + ValidateDataStore(); + return WorkItemType.Get(DataStore, id); + } + public PullRequests? GetPullRequests(string organization, string project, string repositoryName, string developerId, PullRequestView view) { ValidateDataStore(); @@ -453,7 +465,7 @@ private async Task UpdateDataForQueriesAsync(DataStoreOperationParameters parame if (fieldValue == IdentityRefFieldValueName) { var identity = Identity.GetOrCreateIdentity(DataStore, workItem.Fields[field] as IdentityRef, result.Connection); - workItemObjFields.Add(field, identity); + workItemObjFields.Add(field, identity.Id); continue; } @@ -467,7 +479,7 @@ private async Task UpdateDataForQueriesAsync(DataStoreOperationParameters parame workItemType = WorkItemType.GetOrCreateByTeamWorkItemType(DataStore, workItemTypeInfo, project.Id); } - workItemObjFields.Add(field, workItemType); + workItemObjFields.Add(field, workItemType.Id); continue; } @@ -612,8 +624,6 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p // Policy Evaluations API is this: // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} var artifactId = $"vstfs:///CodeReview/CodeReviewId/{project.InternalId}/{pullRequest.PullRequestId}"; - Log.Information($"ArtifactId: {artifactId}"); - var policyEvaluations = await policyClient.GetPolicyEvaluationsAsync(project.InternalId, artifactId); if (policyEvaluations != null) { @@ -629,8 +639,6 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p statusReason = policyEvaluation.Configuration.Type.DisplayName; status = evalStatus; } - - Log.Information($" Policy: {policyEvaluation.EvaluationId} Name: {policyEvaluation.Configuration.Type.DisplayName} Enabled: {policyEvaluation.Configuration.IsEnabled} Blocking: {policyEvaluation.Configuration.IsBlocking} {policyEvaluation.Status}"); } } @@ -647,7 +655,7 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p } // Set record - Log.Information($"PullRequest Status: {status} Reason: {statusReason}"); + Log.Debug($"PullRequest: {pullRequest.PullRequestId} Status: {status} Reason: {statusReason}"); //// If this was a developer pull request, track status for notifications. @@ -661,7 +669,7 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p pullRequestObjFields.Add("PolicyStatusReason", statusReason); var creator = Identity.GetOrCreateIdentity(DataStore, pullRequest.CreatedBy, result.Connection); - pullRequestObjFields.Add("CreatedBy", creator); + pullRequestObjFields.Add("CreatedBy", creator.Id); pullRequestObjFields.Add("CreationDate", pullRequest.CreationDate.Ticks); pullRequestObjFields.Add("TargetBranch", pullRequest.TargetRefName); diff --git a/src/AzureExtension/DataManager/IAzureDataManager.cs b/src/AzureExtension/DataManager/IAzureDataManager.cs index ec637d5..f89f46a 100644 --- a/src/AzureExtension/DataManager/IAzureDataManager.cs +++ b/src/AzureExtension/DataManager/IAzureDataManager.cs @@ -32,6 +32,10 @@ public interface IAzureDataManager : IDisposable Query? GetQuery(AzureUri queryUri, string developerId); + Identity GetIdentity(long id); + + WorkItemType GetWorkItemType(long id); + IEnumerable GetRepositories(); IEnumerable GetDeveloperRepositories(); diff --git a/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs b/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs index 27970c6..6c39002 100644 --- a/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs +++ b/src/AzureExtension/Widgets/AzurePullRequestsWidget.cs @@ -297,7 +297,7 @@ public override void LoadContentData() // closer-to-correct time than the zero value decades ago, so use DateTime.UtcNow. var dateTicks = workItem["CreationDate"]?.GetValue() ?? DateTime.UtcNow.Ticks; var dateTime = dateTicks.ToDateTime(); - + var creator = DataManager.GetIdentity(workItem["CreatedBy"]?.GetValue() ?? 0L); var item = new JsonObject { { "title", workItem["Title"]?.GetValue() ?? string.Empty }, @@ -305,9 +305,9 @@ public override void LoadContentData() { "status_icon", GetIconForPullRequestStatus(workItem["PolicyStatus"]?.GetValue()) }, { "number", element.Key }, { "date", TimeSpanHelper.DateTimeOffsetToDisplayString(dateTime, Log) }, - { "user", workItem["CreatedBy"]?["Name"]?.GetValue() ?? string.Empty }, + { "user", creator.Name }, { "branch", workItem["TargetBranch"]?.GetValue().Replace("refs/heads/", string.Empty) }, - { "avatar", workItem["CreatedBy"]?["Avatar"]?.GetValue() }, + { "avatar", creator.Avatar }, }; itemsArray.Add(item); diff --git a/src/AzureExtension/Widgets/AzureQueryListWidget.cs b/src/AzureExtension/Widgets/AzureQueryListWidget.cs index 28b2f71..35d4e3d 100644 --- a/src/AzureExtension/Widgets/AzureQueryListWidget.cs +++ b/src/AzureExtension/Widgets/AzureQueryListWidget.cs @@ -300,18 +300,19 @@ public override void LoadContentData() // closer-to-correct time than the zero value decades ago, so use DateTime.UtcNow. var dateTicks = workItem["System.ChangedDate"]?.GetValue() ?? DateTime.UtcNow.Ticks; var dateTime = dateTicks.ToDateTime(); - + var creator = DataManager.GetIdentity(workItem["System.CreatedBy"]?.GetValue() ?? 0L); + var workItemType = DataManager.GetWorkItemType(workItem["System.WorkItemType"]?.GetValue() ?? 0L); var item = new JsonObject { { "title", workItem["System.Title"]?.GetValue() ?? string.Empty }, { "url", workItem[AzureDataManager.WorkItemHtmlUrlFieldName]?.GetValue() ?? string.Empty }, - { "icon", GetIconForType(workItem["System.WorkItemType"]?["Name"]?.GetValue()) }, + { "icon", GetIconForType(workItemType.Name) }, { "status_icon", GetIconForStatusState(workItem["System.State"]?.GetValue()) }, { "number", element.Key }, { "date", TimeSpanHelper.DateTimeOffsetToDisplayString(dateTime, Log) }, - { "user", workItem["System.CreatedBy"]?["Name"]?.GetValue() ?? string.Empty }, + { "user", creator.Avatar }, { "status", workItem["System.State"]?.GetValue() ?? string.Empty }, - { "avatar", workItem["System.CreatedBy"]?["Avatar"]?.GetValue() ?? string.Empty }, + { "avatar", creator.Avatar }, }; itemsArray.Add(item); From 3dda2065a8624f9963e0a7d6a2ad52560cd5afa1 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Mon, 22 Jul 2024 03:30:06 -0700 Subject: [PATCH 03/10] Add notifications and pull request status tracking --- .../DataManager/AzureDataManager.cs | 340 ++++++++++++------ .../DataManager/AzureDataManagerUpdate.cs | 11 +- .../DataManager/IAzureDataManager.cs | 2 + .../DataModel/AzureDataStoreSchema.cs | 37 +- .../DataModel/DataObjects/Notification.cs | 262 ++++++++++++++ .../DataObjects/PullRequestPolicyStatus.cs | 134 ++++++- .../DataModel/Enums/NotificationType.cs | 12 + .../Strings/en-US/Resources.resw | 12 + 8 files changed, 695 insertions(+), 115 deletions(-) create mode 100644 src/AzureExtension/DataModel/DataObjects/Notification.cs create mode 100644 src/AzureExtension/DataModel/Enums/NotificationType.cs diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index dfa4fd7..cfa6d8a 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -46,6 +46,10 @@ public partial class AzureDataManager : IAzureDataManager, IDisposable // Most data that has not been updated within this time will be removed. private static readonly TimeSpan _dataRetentionTime = TimeSpan.FromDays(1); + private static readonly TimeSpan _notificationRetentionTime = TimeSpan.FromDays(7); + + private static readonly TimeSpan _pullRequestStaleTime = TimeSpan.FromDays(1); + private static readonly string _lastUpdatedKeyName = "LastUpdated"; private static readonly string _name = nameof(AzureDataManager); @@ -259,6 +263,12 @@ public WorkItemType GetWorkItemType(long id) return WorkItemType.Get(DataStore, id); } + public IEnumerable GetNotifications(DateTime? since = null, bool includeToasted = false) + { + ValidateDataStore(); + return Notification.Get(DataStore, since, includeToasted); + } + public PullRequests? GetPullRequests(string organization, string project, string repositoryName, string developerId, PullRequestView view) { ValidateDataStore(); @@ -494,7 +504,7 @@ private async Task UpdateDataForQueriesAsync(DataStoreOperationParameters parame #if DEBUG WriteIndented = true, #else - WriteIndented = false, + WriteIndented = false, #endif }; @@ -534,20 +544,20 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p // Iterate over and process each Uri in the set. foreach (var azureUri in parameters.Uris) { - var result = GetConnection(azureUri.Connection, parameters.DeveloperId); - if (result.Result != ResultType.Success) + var connectionResult = GetConnection(azureUri.Connection, parameters.DeveloperId); + if (connectionResult.Result != ResultType.Success) { - if (result.Exception != null) + if (connectionResult.Exception != null) { - throw result.Exception; + throw connectionResult.Exception; } else { - throw new AzureAuthorizationException($"Failed getting connection: {azureUri.Connection} for {parameters.DeveloperId.LoginId} with {result.Error}"); + throw new AzureAuthorizationException($"Failed getting connection: {azureUri.Connection} for {parameters.DeveloperId.LoginId} with {connectionResult.Error}"); } } - var gitClient = result.Connection!.GetClient(); + var gitClient = connectionResult.Connection!.GetClient(); if (gitClient == null) { throw new AzureClientException($"Failed getting GitHttpClient for {parameters.DeveloperId.LoginId} and {azureUri.Connection}"); @@ -590,10 +600,10 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p case PullRequestView.Unknown: throw new ArgumentException("PullRequestView is unknown"); case PullRequestView.Mine: - searchCriteria.CreatorId = result.Connection!.AuthorizedIdentity.Id; + searchCriteria.CreatorId = connectionResult.Connection!.AuthorizedIdentity.Id; break; case PullRequestView.Assigned: - searchCriteria.ReviewerId = result.Connection!.AuthorizedIdentity.Id; + searchCriteria.ReviewerId = connectionResult.Connection!.AuthorizedIdentity.Id; break; case PullRequestView.All: /* Nothing different for this */ @@ -601,96 +611,14 @@ private async Task UpdateDataForPullRequestsAsync(DataStoreOperationParameters p } var pullRequests = await gitClient.GetPullRequestsAsync(project.InternalId, repository.InternalId, searchCriteria, null, null, PullRequestResultLimit); - if (pullRequests == null || pullRequests.Count == 0) - { - // If PRs were null or empty, this is a valid result, so create an empty record. - PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, new JsonObject().ToJsonString()); - - // If we had any statuses they should be removed here. - return; - } - - dynamic pullRequestsObj = new ExpandoObject(); - var pullRequestsObjDict = (IDictionary)pullRequestsObj; - - var policyClient = result.Connection!.GetClient(); - foreach (var pullRequest in pullRequests) - { - var status = PolicyStatus.Unknown; - var statusReason = string.Empty; - try - { - // ArtifactId is null in the pull request object and it is not the correct object. The ArtifactId for the - // Policy Evaluations API is this: - // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} - var artifactId = $"vstfs:///CodeReview/CodeReviewId/{project.InternalId}/{pullRequest.PullRequestId}"; - var policyEvaluations = await policyClient.GetPolicyEvaluationsAsync(project.InternalId, artifactId); - if (policyEvaluations != null) - { - var countApplicablePolicies = 0; - foreach (var policyEvaluation in policyEvaluations) - { - if (policyEvaluation.Configuration.IsEnabled && policyEvaluation.Configuration.IsBlocking) - { - ++countApplicablePolicies; - var evalStatus = PullRequestPolicyStatus.GetFromPolicyEvaluationStatus(policyEvaluation.Status); - if (evalStatus < status) - { - statusReason = policyEvaluation.Configuration.Type.DisplayName; - status = evalStatus; - } - } - } - - if (countApplicablePolicies == 0) - { - // If there is no applicable policy, treat the policy status as Approved. - status = PolicyStatus.Approved; - } - } - } - catch (Exception ex) - { - Log.Error(ex, $"Failed getting policy evaluations for pull request: {pullRequest.PullRequestId} {pullRequest.Url}"); - } - - // Set record - Log.Debug($"PullRequest: {pullRequest.PullRequestId} Status: {status} Reason: {statusReason}"); - - //// If this was a developer pull request, track status for notifications. - - dynamic pullRequestObj = new ExpandoObject(); - var pullRequestObjFields = (IDictionary)pullRequestObj; - - pullRequestObjFields.Add("Id", pullRequest.PullRequestId); - pullRequestObjFields.Add("Title", pullRequest.Title); - pullRequestObjFields.Add("Status", pullRequest.Status); - pullRequestObjFields.Add("PolicyStatus", status.ToString()); - pullRequestObjFields.Add("PolicyStatusReason", statusReason); - - var creator = Identity.GetOrCreateIdentity(DataStore, pullRequest.CreatedBy, result.Connection); - pullRequestObjFields.Add("CreatedBy", creator.Id); - pullRequestObjFields.Add("CreationDate", pullRequest.CreationDate.Ticks); - pullRequestObjFields.Add("TargetBranch", pullRequest.TargetRefName); - - // The Links lack an html url for these results, construct the Url. - var htmlUrl = $"{project.ConnectionUri}_git/{azureUri.Repository}/pullrequest/{pullRequest.PullRequestId}"; - pullRequestObjFields.Add("HtmlUrl", htmlUrl); - - pullRequestsObjDict.Add(pullRequest.PullRequestId.ToString(CultureInfo.InvariantCulture), pullRequestObj); - } - - JsonSerializerOptions serializerOptions = new() - { -#if DEBUG - WriteIndented = true, -#else - WriteIndented = false, -#endif - }; - - var serializedJson = JsonSerializer.Serialize(pullRequestsObj, serializerOptions); - PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, parameters.DeveloperId.LoginId, parameters.PullRequestView, serializedJson); + await ProcessPullRequests( + connectionResult.Connection!, + pullRequests, + project, + repository, + parameters.DeveloperId.LoginId, + parameters.PullRequestView, + parameters.OperationName == "UpdateDataForDeveloperPullRequestsAsync"); } // Foreach AzureUri return; @@ -770,7 +698,218 @@ private async Task UpdateDataForDeveloperPullRequestsAsync(DataStoreOperationPar SendDeveloperUpdateEvent(_log, this, parameters.Requestor, context); } - public TeamProject GetTeamProject(string projectName, DeveloperId.DeveloperId developerId, Uri connection) + private async Task ProcessPullRequests( + VssConnection connection, + List? pullRequests, + Project project, + Repository repository, + string loginId, + PullRequestView view, + bool isDeveloper) + { + if (pullRequests is null || pullRequests.Count == 0) + { + // If PRs were null or empty, this is a valid result, so create an empty record. + PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, loginId, view, new JsonObject().ToJsonString()); + return; + } + + dynamic pullRequestsObj = new ExpandoObject(); + var pullRequestsObjDict = (IDictionary)pullRequestsObj; + var policyClient = connection.GetClient(); + + foreach (var pullRequest in pullRequests) + { + var status = PolicyStatus.Unknown; + var statusReason = string.Empty; + + // ArtifactId is null in the pull request object and it is not the correct object. The ArtifactId for the + // Policy Evaluations API is this: + // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} + var artifactId = $"vstfs:///CodeReview/CodeReviewId/{project.InternalId}/{pullRequest.PullRequestId}"; + + // Url in the GitPullRequest object is a REST Api Url, and the links lack an html Url, so we must build it. + var htmlUrl = $"{repository.CloneUrl}/pullrequest/{pullRequest.PullRequestId}"; + + try + { + var policyEvaluations = await policyClient.GetPolicyEvaluationsAsync(project.InternalId, artifactId); + GetPolicyStatus(policyEvaluations, out status, out statusReason); + } + catch (Exception ex) + { + _log.Error(ex, $"Failed getting policy evaluations for pull request: {pullRequest.PullRequestId} {pullRequest.Url}"); + } + + if (isDeveloper) + { + CreatePullRequestStatus(pullRequest, artifactId, project.Id, repository.Id, status, statusReason, htmlUrl); + } + + dynamic pullRequestObj = new ExpandoObject(); + var pullRequestObjFields = (IDictionary)pullRequestObj; + + pullRequestObjFields.Add("Id", pullRequest.PullRequestId); + pullRequestObjFields.Add("Title", pullRequest.Title); + pullRequestObjFields.Add("Status", pullRequest.Status); + pullRequestObjFields.Add("PolicyStatus", status.ToString()); + pullRequestObjFields.Add("PolicyStatusReason", statusReason); + + var creator = Identity.GetOrCreateIdentity(DataStore, pullRequest.CreatedBy, connection); + pullRequestObjFields.Add("CreatedBy", creator.Id); + pullRequestObjFields.Add("CreationDate", pullRequest.CreationDate.Ticks); + pullRequestObjFields.Add("TargetBranch", pullRequest.TargetRefName); + pullRequestObjFields.Add("HtmlUrl", htmlUrl); + + pullRequestsObjDict.Add(pullRequest.PullRequestId.ToString(CultureInfo.InvariantCulture), pullRequestObj); + } + + JsonSerializerOptions serializerOptions = new() + { +#if DEBUG + WriteIndented = true, +#else + WriteIndented = false, +#endif + }; + + var serializedJson = JsonSerializer.Serialize(pullRequestsObj, serializerOptions); + PullRequests.GetOrCreate(DataStore, repository.Id, project.Id, loginId, view, serializedJson); + } + + // Gets PolicyStatus and reason for a given list of PolicyEvaluationRecords + private void GetPolicyStatus(List policyEvaluations, out PolicyStatus status, out string statusReason) + { + status = PolicyStatus.Unknown; + statusReason = string.Empty; + + if (policyEvaluations != null) + { + var countApplicablePolicies = 0; + foreach (var policyEvaluation in policyEvaluations) + { + if (policyEvaluation.Configuration.IsEnabled && policyEvaluation.Configuration.IsBlocking) + { + ++countApplicablePolicies; + var evalStatus = PullRequestPolicyStatus.GetFromPolicyEvaluationStatus(policyEvaluation.Status); + if (evalStatus < status) + { + statusReason = policyEvaluation.Configuration.Type.DisplayName; + status = evalStatus; + } + } + } + + if (countApplicablePolicies == 0) + { + // If there is no applicable policy, treat the policy status as Approved. + status = PolicyStatus.Approved; + } + } + } + + private void CreatePullRequestStatus(GitPullRequest pullRequest, string artifactId, long projectId, long repositoryId, PolicyStatus status, string reason, string htmlUrl) + { + _log.Debug($"PullRequest: {pullRequest.PullRequestId} Status: {status} Reason: {reason}"); + var prevStatus = PullRequestPolicyStatus.Get(DataStore, artifactId); + var curStatus = PullRequestPolicyStatus.Add(DataStore, pullRequest, artifactId, projectId, repositoryId, status, reason, htmlUrl); + + if (ShouldCreateRejectedNotification(curStatus, prevStatus)) + { + _log.Information($"Creating Rejected Notification for {curStatus}"); + Notification.Create(DataStore, curStatus, NotificationType.PullRequestRejected); + } + + if (ShouldCreateApprovalNotification(curStatus, prevStatus)) + { + _log.Information($"Creating Approved Notification for {curStatus}"); + Notification.Create(DataStore, curStatus, NotificationType.PullRequestApproved); + } + } + + private bool ShouldCreateRejectedNotification(PullRequestPolicyStatus curStatus, PullRequestPolicyStatus? prevStatus) + { + // If the pull request is not recent, do not create a notification for it. + /* No UpdatedAt time on the pull request, hard to know + if ((DateTime.Now - curStatus.PullRequest.UpdatedAt) > PullRequestStaleTime) + { + return false; + } + */ + + // If the Pull Request is completed or abandoned then there is nothing to do. + if (curStatus.CompletedOrAbandoned) + { + return false; + } + + // Compare pull request status. + if (prevStatus is null) + { + // No previous status for this commit, assume new PR or freshly pushed commit with + // checks likely running. Any check failures here are assumed to be notification worthy. + if (curStatus.Rejected) + { + return true; + } + } + else + { + // A failure isn't necessarily notification worthy if we've already seen it. + // We do not wish to spam the user with failure notifications. + if (curStatus.Rejected) + { + // If the previous status was not failed, or the failure was for a different + // reason, then create a new notification. + if (!prevStatus.Rejected || curStatus.PolicyStatusReason != prevStatus.PolicyStatusReason) + { + return true; + } + } + } + + return false; + } + + private bool ShouldCreateApprovalNotification(PullRequestPolicyStatus curStatus, PullRequestPolicyStatus? prevStatus) + { + // If the pull request is not recent, do not create a notification for it. + /* No UpdatedAt time on the pull request, hard to know + if ((DateTime.Now - curStatus.PullRequest.UpdatedAt) > PullRequestStaleTime) + { + return false; + } + */ + + // If the Pull Request is completed or abandoned then there is nothing to do. + if (curStatus.CompletedOrAbandoned) + { + return false; + } + + // Compare pull request status. + if (prevStatus is null) + { + // No previous status for this PR, it may have been approved between updates. + if (curStatus.Approved) + { + return true; + } + } + else + { + // Only post success notifications if it wasn't previously successful. + // We do not wish to spam the user with success notifications. + if (curStatus.Approved && !prevStatus.Approved) + { + return true; + } + } + + return false; + } + + private TeamProject GetTeamProject(string projectName, DeveloperId.DeveloperId developerId, Uri connection) { var result = GetConnection(connection, developerId); if (result.Result != ResultType.Success) @@ -950,6 +1089,7 @@ private void PruneObsoleteData() Query.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); PullRequests.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); WorkItemType.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); + Notification.DeleteBefore(DataStore, DateTime.UtcNow - _notificationRetentionTime); // The following are not yet pruned, need to ensure there are no current key references // before deletion. diff --git a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs index f2fde74..a701211 100644 --- a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs +++ b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using DevHomeAzureExtension.DataManager; +using DevHomeAzureExtension.DataModel; using Microsoft.TeamFoundation.Policy.WebApi; using Serilog; @@ -50,22 +51,14 @@ public static async Task UpdateDeveloperPullRequests() await dataManager.UpdatePullRequestsForLoggedInDeveloperIdsAsync(null, identifier); // Show any new notifications that were created from the pull request update. - /* var notifications = dataManager.GetNotifications(); foreach (var notification in notifications) { // Show notifications for failed checkruns for Developer users. - if (notification.Type == NotificationType.CheckRunFailed && notification.User.IsDeveloper) - { - notification.ShowToast(); - } - - // Show notifications for new reviews. - if (notification.Type == NotificationType.NewReview) + if (notification.Type == NotificationType.PullRequestRejected || notification.Type == NotificationType.PullRequestApproved) { notification.ShowToast(); } } - */ } } diff --git a/src/AzureExtension/DataManager/IAzureDataManager.cs b/src/AzureExtension/DataManager/IAzureDataManager.cs index f89f46a..89fe3cf 100644 --- a/src/AzureExtension/DataManager/IAzureDataManager.cs +++ b/src/AzureExtension/DataManager/IAzureDataManager.cs @@ -36,6 +36,8 @@ public interface IAzureDataManager : IDisposable WorkItemType GetWorkItemType(long id); + IEnumerable GetNotifications(DateTime? since = null, bool includeToasted = false); + IEnumerable GetRepositories(); IEnumerable GetDeveloperRepositories(); diff --git a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs index 27ced27..385f7bd 100644 --- a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs +++ b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs @@ -14,7 +14,7 @@ public AzureDataStoreSchema() } // Update this anytime incompatible changes happen with a released version. - private const long SchemaVersionValue = 0x0007; + private const long SchemaVersionValue = 0x0008; private const string Metadata = @"CREATE TABLE Metadata (" + @@ -154,6 +154,39 @@ public AzureDataStoreSchema() // the developer login, and the view. "CREATE UNIQUE INDEX IDX_PullRequests_ProjectIdRepositoryIdDeveloperLoginViewId ON PullRequests (ProjectId, RepositoryId, DeveloperLogin, ViewId);"; + // PullRequsetPolicyStatus is a snapshot of a developer's Pull Requests. + private const string PullRequestPolicyStatus = + @"CREATE TABLE PullRequestPolicyStatus (" + + "Id INTEGER PRIMARY KEY NOT NULL," + + "ArtifactId TEXT NULL COLLATE NOCASE," + + "ProjectId INTEGER NOT NULL," + + "RepositoryId INTEGER NOT NULL," + + "PullRequestId INTEGER NOT NULL," + + "Title TEXT NULL COLLATE NOCASE," + + "PolicyStatusId INTEGER NOT NULL," + + "PolicyStatusReason TEXT NULL COLLATE NOCASE," + + "PullRequestStatusId INTEGER NOT NULL," + + "TargetBranchName TEXT NULL COLLATE NOCASE," + + "HtmlUrl TEXT NULL COLLATE NOCASE," + + "TimeUpdated INTEGER NOT NULL," + + "TimeCreated INTEGER NOT NULL" + + ");"; + + private const string Notification = + @"CREATE TABLE Notification (" + + "Id INTEGER PRIMARY KEY NOT NULL," + + "TypeId INTEGER NOT NULL," + + "ProjectId INTEGER NOT NULL," + + "RepositoryId INTEGER NOT NULL," + + "Title TEXT NOT NULL COLLATE NOCASE," + + "Description TEXT NOT NULL COLLATE NOCASE," + + "Identifier TEXT NULL COLLATE NOCASE," + + "Result TEXT NULL COLLATE NOCASE," + + "HtmlUrl TEXT NULL COLLATE NOCASE," + + "ToastState INTEGER NOT NULL," + + "TimeCreated INTEGER NOT NULL" + + ");"; + // All Sqls together. private static readonly List _schemaSqlsValue = [ @@ -167,5 +200,7 @@ public AzureDataStoreSchema() Query, WorkItemType, PullRequests, + PullRequestPolicyStatus, + Notification, ]; } diff --git a/src/AzureExtension/DataModel/DataObjects/Notification.cs b/src/AzureExtension/DataModel/DataObjects/Notification.cs new file mode 100644 index 0000000..e068da9 --- /dev/null +++ b/src/AzureExtension/DataModel/DataObjects/Notification.cs @@ -0,0 +1,262 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Dapper; +using Dapper.Contrib.Extensions; +using DevHomeAzureExtension.Helpers; +using Microsoft.TeamFoundation.SourceControl.WebApi; +using Microsoft.Windows.ApplicationModel.Resources; +using Microsoft.Windows.AppNotifications; +using Microsoft.Windows.AppNotifications.Builder; +using Serilog; + +namespace DevHomeAzureExtension.DataModel; + +/// +/// Represents data for rendering a notification to the user. +/// +/// +/// Notifications are sent to the user as Windows notifications, but may have specific filtering, +/// such as prioritizing certain repositories, or only showing a notification for a current +/// developer. The contents of the notifications table is a representation of potential +/// notifications, it does not necessarily mean the notification will be shown to the user. It is +/// the set of things we believe may be notification-worthy and ultimately user settings and context +/// will determine when and if the notification gets shown. +/// +[Table("Notification")] +public class Notification +{ + private static readonly Lazy _logger = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(Notification)}")); + + private static ILogger Log => _logger.Value; + + [Key] + public long Id { get; set; } = DataStore.NoForeignKey; + + public long TypeId { get; set; } = DataStore.NoForeignKey; + + // Key in Project table + public long ProjectId { get; set; } = DataStore.NoForeignKey; + + // Key in Repository table + public long RepositoryId { get; set; } = DataStore.NoForeignKey; + + public string Title { get; set; } = string.Empty; + + public string Identifier { get; set; } = string.Empty; + + public string Result { get; set; } = string.Empty; + + public string Description { get; set; } = string.Empty; + + public string HtmlUrl { get; set; } = string.Empty; + + public long ToastState { get; set; } = DataStore.NoForeignKey; + + public long TimeCreated { get; set; } = DataStore.NoForeignKey; + + [Write(false)] + private DataStore? DataStore { get; set; } + + [Write(false)] + [Computed] + public DateTime CreatedAt => TimeCreated.ToDateTime(); + + [Write(false)] + [Computed] + public Project Project => Project.Get(DataStore, ProjectId); + + [Write(false)] + [Computed] + public Repository Repository => Repository.Get(DataStore, RepositoryId); + + [Write(false)] + [Computed] + public NotificationType Type => (NotificationType)TypeId; + + [Write(false)] + [Computed] + public bool Toasted + { + get => ToastState != 0; + set + { + ToastState = value ? 1 : 0; + if (DataStore is not null) + { + try + { + DataStore.Connection!.Update(this); + } + catch (Exception ex) + { + // Catch errors so we do not throw for something like this. The local ToastState + // will still be set even if the datastore update fails. This could result in a + // toast later being shown twice, however, so report it as an error. + Log.Error(ex, "Failed setting Notification ToastState for Notification Id = {Id}"); + } + } + } + } + + public override string ToString() => $"[{Type}][{Project}] {Title}"; + + /// + /// Shows a toast formatted based on this notification's NotificationType. + /// + /// True if a toast was shown. + public bool ShowToast() + { + if (Toasted) + { + return false; + } + else if (LocalSettings.ReadSettingAsync("NotificationsEnabled").Result == "false") + { + Toasted = true; + return false; + } + + return Type switch + { + NotificationType.PullRequestRejected => ShowPullRequestRejectedToast(), + NotificationType.PullRequestApproved => ShowPullRequestApprovedToast(), + _ => false, + }; + } + + private bool ShowPullRequestRejectedToast() + { + try + { + Log.Information($"Showing Notification for {this}"); + var nb = new AppNotificationBuilder(); + nb.SetDuration(AppNotificationDuration.Long); + nb.AddArgument("htmlurl", HtmlUrl); + nb.AddText($"❌ {Resources.GetResource("Notifications_Toast_PullRequestRejected/Title", Log)} - {Description}"); + nb.AddText($"#{Identifier} - {Repository.Name}", new AppNotificationTextProperties().SetMaxLines(1)); + nb.AddText(Title); + nb.AddButton(new AppNotificationButton(Resources.GetResource("Notifications_Toast_Button/Dismiss", Log)).AddArgument("action", "dismiss")); + AppNotificationManager.Default.Show(nb.BuildNotification()); + + Toasted = true; + } + catch (Exception ex) + { + Log.Error(ex, $"Failed creating the Notification for {this}"); + return false; + } + + return true; + } + + private bool ShowPullRequestApprovedToast() + { + try + { + Log.Information($"Showing Notification for {this}"); + var nb = new AppNotificationBuilder(); + nb.SetDuration(AppNotificationDuration.Long); + nb.AddArgument("htmlurl", HtmlUrl); + nb.AddText($"✅ {Resources.GetResource("Notifications_Toast_PullRequestApproved/Title", Log)}"); + nb.AddText($"#{Identifier} - {Repository.Name}", new AppNotificationTextProperties().SetMaxLines(1)); + nb.AddText(Title); + nb.AddButton(new AppNotificationButton(Resources.GetResource("Notifications_Toast_Button/Dismiss", Log)).AddArgument("action", "dismiss")); + AppNotificationManager.Default.Show(nb.BuildNotification()); + + Toasted = true; + } + catch (Exception ex) + { + Log.Error(ex, $"Failed creating the Notification for {this}"); + return false; + } + + return true; + } + + public static Notification Create(DataStore dataStore, PullRequestPolicyStatus status, NotificationType type) + { + var pullRequestNotification = new Notification + { + TypeId = (long)type, + ProjectId = status.ProjectId, + RepositoryId = status.RepositoryId, + Title = status.Title, + Description = status.PolicyStatusReason, + Identifier = status.PullRequestId.ToStringInvariant(), + Result = status.PolicyStatus.ToString(), + HtmlUrl = status.HtmlUrl, + ToastState = 0, + TimeCreated = DateTime.Now.ToDataStoreInteger(), + }; + + Add(dataStore, pullRequestNotification); + SetOlderNotificationsToasted(dataStore, pullRequestNotification); + return pullRequestNotification; + } + + public static Notification Add(DataStore dataStore, Notification notification) + { + notification.Id = dataStore.Connection!.Insert(notification); + notification.DataStore = dataStore; + return notification; + } + + public static IEnumerable Get(DataStore dataStore, DateTime? since = null, bool includeToasted = false) + { + since ??= DateTime.MinValue; + var sql = @"SELECT * FROM Notification WHERE TimeCreated > @Time AND ToastState <= @ToastedCount ORDER BY TimeCreated DESC"; + var param = new + { + // Cast to non-nullable type since we ensure it is not null above. + Time = ((DateTime)since).ToDataStoreInteger(), + ToastedCount = includeToasted ? 1 : 0, + }; + + Log.Verbose(DataStore.GetSqlLogMessage(sql, param)); + var notifications = dataStore.Connection!.Query(sql, param, null) ?? []; + foreach (var notification in notifications) + { + notification.DataStore = dataStore; + } + + return notifications; + } + + public static void SetOlderNotificationsToasted(DataStore dataStore, Notification notification) + { + // Get all untoasted notifications for the same type, project, repository, and identifier that are older + // than the specified notification. + var sql = @"SELECT * FROM Notification WHERE TypeId = @TypeId AND RepositoryId = @RepositoryId AND Identifier = @Identifier AND ProjectId = @ProjectId AND TimeCreated < @TimeCreated AND ToastState = 0"; + var param = new + { + notification.TypeId, + notification.RepositoryId, + notification.Identifier, + notification.ProjectId, + notification.TimeCreated, + }; + + Log.Verbose(DataStore.GetSqlLogMessage(sql, param)); + var outDatedNotifications = dataStore.Connection!.Query(sql, param, null) ?? []; + foreach (var olderNotification in outDatedNotifications) + { + olderNotification.DataStore = dataStore; + olderNotification.Toasted = true; + Log.Information($"Found older notification for {olderNotification.Identifier} with result {olderNotification.Result}, marking toasted."); + } + } + + public static void DeleteBefore(DataStore dataStore, DateTime date) + { + // Delete notifications older than the date listed. + var sql = @"DELETE FROM Notification WHERE TimeCreated < $Time;"; + var command = dataStore.Connection!.CreateCommand(); + command.CommandText = sql; + command.Parameters.AddWithValue("$Time", date.ToDataStoreInteger()); + Log.Verbose(DataStore.GetCommandLogMessage(sql, command)); + var rowsDeleted = command.ExecuteNonQuery(); + Log.Verbose(DataStore.GetDeletedLogMessage(rowsDeleted)); + } +} diff --git a/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs index 64ad0a7..d8dfe23 100644 --- a/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs +++ b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs @@ -5,6 +5,7 @@ using Dapper.Contrib.Extensions; using DevHomeAzureExtension.Helpers; using Microsoft.TeamFoundation.Policy.WebApi; +using Microsoft.TeamFoundation.SourceControl.WebApi; using Serilog; namespace DevHomeAzureExtension.DataModel; @@ -12,25 +13,40 @@ namespace DevHomeAzureExtension.DataModel; [Table("PullRequestPolicyStatus")] public class PullRequestPolicyStatus { - private static readonly Lazy _logger = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(PullRequests)}")); + private static readonly Lazy _logger = new(() => Serilog.Log.ForContext("SourceContext", $"DataModel/{nameof(PullRequestPolicyStatus)}")); - private static readonly ILogger _log = _logger.Value; + private static ILogger Log => _logger.Value; [Key] public long Id { get; set; } = DataStore.NoForeignKey; - // This should suffice as a unique identifier. + // This should suffice as a unique identifier, as it is the Project Guid + PullRequestId public string ArtifactId { get; set; } = string.Empty; // Key in Project table public long ProjectId { get; set; } = DataStore.NoForeignKey; + // Key in Repository table + public long RepositoryId { get; set; } = DataStore.NoForeignKey; + public long PullRequestId { get; set; } = DataStore.NoForeignKey; - public long TimeUpdated { get; set; } = DataStore.NoForeignKey; + public string Title { get; set; } = string.Empty; public long PolicyStatusId { get; set; } = DataStore.NoForeignKey; + public string PolicyStatusReason { get; set; } = string.Empty; + + public long PullRequestStatusId { get; set; } = DataStore.NoForeignKey; + + public string TargetBranchName { get; set; } = string.Empty; + + public string HtmlUrl { get; set; } = string.Empty; + + public long TimeUpdated { get; set; } = DataStore.NoForeignKey; + + public long TimeCreated { get; set; } = DataStore.NoForeignKey; + public override string ToString() => ArtifactId; [Write(false)] @@ -42,12 +58,44 @@ public class PullRequestPolicyStatus [Write(false)] [Computed] - public PolicyStatus PolicyStatus => (PolicyStatus)PolicyStatusId; + public Repository Repository => Repository.Get(DataStore, RepositoryId); + + [Write(false)] + [Computed] + public DateTime CreatedAt => TimeCreated.ToDateTime(); [Write(false)] [Computed] public DateTime UpdatedAt => TimeUpdated.ToDateTime(); + [Write(false)] + [Computed] + public PolicyStatus PolicyStatus => (PolicyStatus)PolicyStatusId; + + [Write(false)] + [Computed] + public PullRequestStatus Status => (PullRequestStatus)PullRequestStatusId; + + [Write(false)] + [Computed] + public bool CompletedOrAbandoned => (Status == PullRequestStatus.Completed) || (Status == PullRequestStatus.Abandoned); + + [Write(false)] + [Computed] + public bool ActiveOrNotSet => (Status == PullRequestStatus.Active) || (Status == PullRequestStatus.NotSet); + + [Write(false)] + [Computed] + public bool Waiting => (PolicyStatus == PolicyStatus.Queued) || (PolicyStatus == PolicyStatus.Running); + + [Write(false)] + [Computed] + public bool Rejected => PolicyStatus <= PolicyStatus.Rejected; + + [Write(false)] + [Computed] + public bool Approved => PolicyStatus >= PolicyStatus.Approved; + // Map PolicyEvaluationStatus to our enum. Our enum is sorted, but the PolicyEvaluationStatus is not. public static PolicyStatus GetFromPolicyEvaluationStatus(PolicyEvaluationStatus? policyEvaluationStatus) { @@ -62,4 +110,80 @@ public static PolicyStatus GetFromPolicyEvaluationStatus(PolicyEvaluationStatus? _ => PolicyStatus.Unknown, }; } + + public static PullRequestPolicyStatus Create(GitPullRequest pullRequest, string artifactId, long projectId, long repositoryId, PolicyStatus policyStatus, string reason, string htmlUrl) + { + // Get Current status of this pull request, and create a summary capture. + return new PullRequestPolicyStatus + { + ArtifactId = artifactId, + ProjectId = projectId, + RepositoryId = repositoryId, + PullRequestId = pullRequest.PullRequestId, + Title = pullRequest.Title, + PolicyStatusId = (long)policyStatus, + PolicyStatusReason = reason, + TargetBranchName = pullRequest.TargetRefName, + PullRequestStatusId = (long)pullRequest.Status, + HtmlUrl = htmlUrl, + TimeCreated = DateTime.UtcNow.ToDataStoreInteger(), + }; + } + + public static PullRequestPolicyStatus? Get(DataStore dataStore, string artifactId) + { + var sql = @"SELECT * FROM PullRequestPolicyStatus WHERE ArtifactId = @ArtifactId ORDER BY TimeCreated DESC LIMIT 1;"; + var param = new + { + ArtifactId = artifactId, + }; + + Log.Verbose(DataStore.GetSqlLogMessage(sql, param)); + var pullRequestStatus = dataStore.Connection!.QueryFirstOrDefault(sql, param, null); + if (pullRequestStatus is not null) + { + // Add Datastore so this object can make internal queries. + pullRequestStatus.DataStore = dataStore; + } + + return pullRequestStatus; + } + + public static PullRequestPolicyStatus Add(DataStore dataStore, GitPullRequest pullRequest, string artifactId, long projectId, long repositoryId, PolicyStatus policyStatus, string reason, string htmlUrl) + { + var pullRequestStatus = Create(pullRequest, artifactId, projectId, repositoryId, policyStatus, reason, htmlUrl); + pullRequestStatus.Id = dataStore.Connection!.Insert(pullRequestStatus); + Log.Debug($"Inserted PullRequestPolicyStatus, Id = {pullRequestStatus.Id}"); + + // Remove older records we no longer need. + DeleteOutdatedForPullRequest(dataStore, artifactId); + + pullRequestStatus.DataStore = dataStore; + return pullRequestStatus; + } + + public static void DeleteOutdatedForPullRequest(DataStore dataStore, string artifactId) + { + // Delete any records beyond the most recent 2. + var sql = @"DELETE FROM PullRequestPolicyStatus WHERE ArtifactId = $Id AND Id NOT IN (SELECT Id FROM PullRequestPolicyStatus WHERE ArtifactId = $Id ORDER BY TimeCreated DESC LIMIT 2)"; + var command = dataStore.Connection!.CreateCommand(); + command.CommandText = sql; + command.Parameters.AddWithValue("$Id", artifactId); + Log.Verbose(DataStore.GetCommandLogMessage(sql, command)); + var rowsDeleted = command.ExecuteNonQuery(); + Log.Verbose(DataStore.GetDeletedLogMessage(rowsDeleted)); + } + + public static void DeleteUnreferenced(DataStore dataStore) + { + // Delete any where the HeadSha has no match in pull requests, as that means either the pull request + // no longer exists, or it has pushed new changes and the HeadSha is different, triggering new checks. + // In either case it is safe to remove the associated PullRequestPolicyStatus. + var sql = @"DELETE FROM PullRequestPolicyStatus WHERE HeadSha NOT IN (SELECT HeadSha FROM PullRequest)"; + var command = dataStore.Connection!.CreateCommand(); + command.CommandText = sql; + Log.Verbose(DataStore.GetCommandLogMessage(sql, command)); + var rowsDeleted = command.ExecuteNonQuery(); + Log.Verbose(DataStore.GetDeletedLogMessage(rowsDeleted)); + } } diff --git a/src/AzureExtension/DataModel/Enums/NotificationType.cs b/src/AzureExtension/DataModel/Enums/NotificationType.cs new file mode 100644 index 0000000..235cd0c --- /dev/null +++ b/src/AzureExtension/DataModel/Enums/NotificationType.cs @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace DevHomeAzureExtension.DataModel; + +public enum NotificationType +{ + Unknown = 0, + PullRequestApproved = 1, + PullRequestRejected = 2, + NewReview = 3, +} diff --git a/src/AzureExtension/Strings/en-US/Resources.resw b/src/AzureExtension/Strings/en-US/Resources.resw index 6b5c693..c221891 100644 --- a/src/AzureExtension/Strings/en-US/Resources.resw +++ b/src/AzureExtension/Strings/en-US/Resources.resw @@ -714,4 +714,16 @@ Launch Text shown in the button to launch Dev Box. + + Dismiss + Shown in Toast Notification Dismiss Button + + + Approved + Shown in Toast Notification, title line + + + Rejected + Shown in Toast Notification, title line + \ No newline at end of file From ae3b30813bc23c299f96747f0974665dfcf1a235 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Mon, 22 Jul 2024 15:40:04 -0700 Subject: [PATCH 04/10] Add policy status and notification test --- .../DataModel/AzureDataStoreSchema.cs | 2 +- .../DeveloperId/AuthenticationHelper.cs | 2 +- .../DataStore/DataObjectTests.cs | 53 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs index 385f7bd..0c6acd3 100644 --- a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs +++ b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs @@ -14,7 +14,7 @@ public AzureDataStoreSchema() } // Update this anytime incompatible changes happen with a released version. - private const long SchemaVersionValue = 0x0008; + private const long SchemaVersionValue = 0x0007; private const string Metadata = @"CREATE TABLE Metadata (" + diff --git a/src/AzureExtension/DeveloperId/AuthenticationHelper.cs b/src/AzureExtension/DeveloperId/AuthenticationHelper.cs index e2895a4..f378795 100644 --- a/src/AzureExtension/DeveloperId/AuthenticationHelper.cs +++ b/src/AzureExtension/DeveloperId/AuthenticationHelper.cs @@ -286,7 +286,7 @@ public async Task> AcquireAllDeveloperAccountTokens(string[] public async Task ObtainTokenForLoggedInDeveloperAccount(string[] scopes, string loginId) { - _log.Information($"ObtainTokenForLoggedInDeveloperAccount"); + _log.Debug($"ObtainTokenForLoggedInDeveloperAccount"); AuthenticationResult = null; var existingAccount = await GetDeveloperAccountFromCache(loginId); diff --git a/test/AzureExtension/DataStore/DataObjectTests.cs b/test/AzureExtension/DataStore/DataObjectTests.cs index ee7a44e..05b62c0 100644 --- a/test/AzureExtension/DataStore/DataObjectTests.cs +++ b/test/AzureExtension/DataStore/DataObjectTests.cs @@ -372,4 +372,57 @@ public void ReadAndWritePullRequests() Assert.AreEqual(PullRequestView.Mine, findPull.View); Assert.AreEqual("project", findPull.Project.Name); } + + [TestMethod] + [TestCategory("Unit")] + public void ReadAndWriteStatusAndNotification() + { + using var dataStore = new DataStore("TestStore", TestHelpers.GetDataStoreFilePath(TestOptions), TestOptions.DataStoreOptions.DataStoreSchema!); + Assert.IsNotNull(dataStore); + dataStore.Create(); + Assert.IsNotNull(dataStore.Connection); + + using var tx = dataStore.Connection.BeginTransaction(); + var org = Organization.GetOrCreate(dataStore, new Uri("https://dev.azure.com/organization/")); + Assert.IsNotNull(org); + dataStore.Connection.Insert(new Project { Name = "project", InternalId = "11", OrganizationId = org.Id }); + dataStore.Connection.Insert(new Repository { Name = "repository", InternalId = "21", CloneUrl = "https://organization/project/_git/repository/", ProjectId = 1 }); + + var artifactId = "vstfs:///CodeReview/CodeReviewId/foo/47"; + var title = "Pull Request Test"; + var url = "https://organization/project/_git/repository/pullrequest/47"; + dataStore.Connection.Insert(new PullRequestPolicyStatus + { + ArtifactId = artifactId, + ProjectId = 1, + RepositoryId = 1, + PullRequestId = 47, + Title = title, + PolicyStatusId = 2, + PolicyStatusReason = "Build Fail", + TargetBranchName = "refs/heads/main", + PullRequestStatusId = 1, + HtmlUrl = url, + TimeCreated = DateTime.UtcNow.ToDataStoreInteger(), + }); + + var prStatus = PullRequestPolicyStatus.Get(dataStore, artifactId); + Assert.IsNotNull(prStatus); + + TestContext?.WriteLine($" PR: {prStatus.PullRequestId} Status: {prStatus.PolicyStatusId}: {prStatus.PolicyStatus} - {prStatus.PolicyStatusReason}"); + Assert.AreEqual(url, prStatus.HtmlUrl); + Assert.AreEqual(PolicyStatus.Rejected, prStatus.PolicyStatus); + Assert.AreEqual(artifactId, prStatus.ArtifactId); + + // Create notification from PR Status + var notification = Notification.Create(dataStore, prStatus, NotificationType.PullRequestRejected); + Assert.IsNotNull(notification); + Assert.AreEqual(title, notification.Title); + Assert.AreEqual(1, notification.RepositoryId); + Assert.AreEqual(1, notification.ProjectId); + Assert.AreEqual(PolicyStatus.Rejected.ToString(), notification.Result); + Assert.AreEqual(url, notification.HtmlUrl); + TestContext?.WriteLine($" {notification.Title}"); + TestContext?.WriteLine($" {notification.Description}"); + } } From fcc61e2c977006fec3e405c8e3f44989842b4080 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 23 Jul 2024 14:27:07 -0700 Subject: [PATCH 05/10] Add add stale checks to pull request data, update based on push --- .../DataManager/AzureDataManager.cs | 41 +++++++++++++++---- .../DataManager/AzureDataManagerUpdate.cs | 5 ++- .../DataObjects/PullRequestPolicyStatus.cs | 37 +++++++++++++---- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index cfa6d8a..0f170f1 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -46,9 +46,17 @@ public partial class AzureDataManager : IAzureDataManager, IDisposable // Most data that has not been updated within this time will be removed. private static readonly TimeSpan _dataRetentionTime = TimeSpan.FromDays(1); + // This is how long we will keep a notification in the datastore. private static readonly TimeSpan _notificationRetentionTime = TimeSpan.FromDays(7); - private static readonly TimeSpan _pullRequestStaleTime = TimeSpan.FromDays(1); + // Pull requests without a push in this amount of time are considered too old to + // generate new notifications. + private static readonly TimeSpan _pullRequestIsAncientTime = TimeSpan.FromDays(14); + + // The amount of time we will retain a pull request status record. If a very old pull + // request is resurrected and updated beyond this time it will be treated as a new + // pull request as the previous status record will be gone. + private static readonly TimeSpan _pullRequestStatusRetentionTime = TimeSpan.FromDays(30); private static readonly string _lastUpdatedKeyName = "LastUpdated"; @@ -743,6 +751,22 @@ private async Task ProcessPullRequests( if (isDeveloper) { + // Pull requests do not fully populate commit information. If this is a developer pull request, fetch the + // additional commit information about the last merged source to determine when the last time the pull request + // was pushed a commit. + if (pullRequest.LastMergeSourceCommit is not null) + { + var gitClient = connection.GetClient(); + if (gitClient is not null) + { + var commitRef = await gitClient.GetCommitAsync(pullRequest.LastMergeSourceCommit.CommitId, repository.InternalId); + if (commitRef is not null) + { + pullRequest.LastMergeSourceCommit = commitRef; + } + } + } + CreatePullRequestStatus(pullRequest, artifactId, project.Id, repository.Id, status, statusReason, htmlUrl); } @@ -829,13 +853,12 @@ private void CreatePullRequestStatus(GitPullRequest pullRequest, string artifact private bool ShouldCreateRejectedNotification(PullRequestPolicyStatus curStatus, PullRequestPolicyStatus? prevStatus) { - // If the pull request is not recent, do not create a notification for it. - /* No UpdatedAt time on the pull request, hard to know - if ((DateTime.Now - curStatus.PullRequest.UpdatedAt) > PullRequestStaleTime) + // If the pull request is not recently updated, ignore it. This is to prevent ancient pull requests + // from showing notifications. + if ((DateTime.UtcNow - curStatus.UpdatedAt) > _pullRequestIsAncientTime) { return false; } - */ // If the Pull Request is completed or abandoned then there is nothing to do. if (curStatus.CompletedOrAbandoned) @@ -873,13 +896,12 @@ private bool ShouldCreateRejectedNotification(PullRequestPolicyStatus curStatus, private bool ShouldCreateApprovalNotification(PullRequestPolicyStatus curStatus, PullRequestPolicyStatus? prevStatus) { - // If the pull request is not recent, do not create a notification for it. - /* No UpdatedAt time on the pull request, hard to know - if ((DateTime.Now - curStatus.PullRequest.UpdatedAt) > PullRequestStaleTime) + // If the pull request is not recently updated, ignore it. This is to prevent ancient pull requests + // from showing notifications. + if ((DateTime.UtcNow - curStatus.UpdatedAt) > _pullRequestIsAncientTime) { return false; } - */ // If the Pull Request is completed or abandoned then there is nothing to do. if (curStatus.CompletedOrAbandoned) @@ -1090,6 +1112,7 @@ private void PruneObsoleteData() PullRequests.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); WorkItemType.DeleteBefore(DataStore, DateTime.UtcNow - _dataRetentionTime); Notification.DeleteBefore(DataStore, DateTime.UtcNow - _notificationRetentionTime); + PullRequestPolicyStatus.DeleteBefore(DataStore, DateTime.UtcNow - _pullRequestStatusRetentionTime); // The following are not yet pruned, need to ensure there are no current key references // before deletion. diff --git a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs index a701211..b501da2 100644 --- a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs +++ b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs @@ -37,12 +37,13 @@ public static async Task Update() public static async Task UpdateDeveloperPullRequests() { - Log.Debug($"Executing UpdateDeveloperPullRequests"); + var log = Log.ForContext("SourceContext", $"UpdateDeveloperPullRequests"); + log.Debug($"Executing UpdateDeveloperPullRequests"); var cacheManager = CacheManager.GetInstance(); if (cacheManager.UpdateInProgress) { - Log.Information("Cache is being updated, skipping Developer Pull Request Update"); + log.Information("Cache is being updated, skipping Developer Pull Request Update"); return; } diff --git a/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs index d8dfe23..efcf93e 100644 --- a/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs +++ b/src/AzureExtension/DataModel/DataObjects/PullRequestPolicyStatus.cs @@ -43,8 +43,10 @@ public class PullRequestPolicyStatus public string HtmlUrl { get; set; } = string.Empty; + // Time of the last pull request update (push), if it exists. public long TimeUpdated { get; set; } = DataStore.NoForeignKey; + // Time the pull request was created. public long TimeCreated { get; set; } = DataStore.NoForeignKey; public override string ToString() => ArtifactId; @@ -114,7 +116,7 @@ public static PolicyStatus GetFromPolicyEvaluationStatus(PolicyEvaluationStatus? public static PullRequestPolicyStatus Create(GitPullRequest pullRequest, string artifactId, long projectId, long repositoryId, PolicyStatus policyStatus, string reason, string htmlUrl) { // Get Current status of this pull request, and create a summary capture. - return new PullRequestPolicyStatus + var status = new PullRequestPolicyStatus { ArtifactId = artifactId, ProjectId = projectId, @@ -126,8 +128,26 @@ public static PullRequestPolicyStatus Create(GitPullRequest pullRequest, string TargetBranchName = pullRequest.TargetRefName, PullRequestStatusId = (long)pullRequest.Status, HtmlUrl = htmlUrl, - TimeCreated = DateTime.UtcNow.ToDataStoreInteger(), + TimeCreated = pullRequest.CreationDate.ToUniversalTime().ToDataStoreInteger(), }; + + // Set TimeUpdated to be the most recent commit push time if it exists. + if (pullRequest.LastMergeSourceCommit?.Push is not null) + { + status.TimeUpdated = pullRequest.LastMergeSourceCommit.Push.Date.ToUniversalTime().ToDataStoreInteger(); + if (status.TimeUpdated < status.TimeCreated) + { + // First commits will often be pushed before the pull request is created. In this case, + // we want the TimeUpdated to be at a minimum the TimeCreated. + status.TimeUpdated = status.TimeCreated; + } + } + else + { + status.TimeUpdated = status.TimeCreated; + } + + return status; } public static PullRequestPolicyStatus? Get(DataStore dataStore, string artifactId) @@ -174,16 +194,15 @@ public static void DeleteOutdatedForPullRequest(DataStore dataStore, string arti Log.Verbose(DataStore.GetDeletedLogMessage(rowsDeleted)); } - public static void DeleteUnreferenced(DataStore dataStore) + public static void DeleteBefore(DataStore dataStore, DateTime date) { - // Delete any where the HeadSha has no match in pull requests, as that means either the pull request - // no longer exists, or it has pushed new changes and the HeadSha is different, triggering new checks. - // In either case it is safe to remove the associated PullRequestPolicyStatus. - var sql = @"DELETE FROM PullRequestPolicyStatus WHERE HeadSha NOT IN (SELECT HeadSha FROM PullRequest)"; + // Delete notifications older than the date listed. + var sql = @"DELETE FROM PullRequestPolicyStatus WHERE TimeUpdated < $Time;"; var command = dataStore.Connection!.CreateCommand(); command.CommandText = sql; - Log.Verbose(DataStore.GetCommandLogMessage(sql, command)); + command.Parameters.AddWithValue("$Time", date.ToDataStoreInteger()); + Log.Debug(DataStore.GetCommandLogMessage(sql, command)); var rowsDeleted = command.ExecuteNonQuery(); - Log.Verbose(DataStore.GetDeletedLogMessage(rowsDeleted)); + Log.Debug(DataStore.GetDeletedLogMessage(rowsDeleted)); } } From e39e4ed4aec25a7b10d66b53dfe54dbe67c43724 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 23 Jul 2024 14:53:59 -0700 Subject: [PATCH 06/10] Use UtcNow --- src/AzureExtension/DataManager/AzureDataManagerUpdate.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs index b501da2..9d5fb00 100644 --- a/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs +++ b/src/AzureExtension/DataManager/AzureDataManagerUpdate.cs @@ -18,7 +18,7 @@ public static async Task Update() { // Only update per the update interval. // This is intended to be dynamic in the future. - if (DateTime.Now - _lastUpdateTime < _updateInterval) + if (DateTime.UtcNow - _lastUpdateTime < _updateInterval) { return; } @@ -32,7 +32,7 @@ public static async Task Update() Log.Error(ex, "Update failed unexpectedly."); } - _lastUpdateTime = DateTime.Now; + _lastUpdateTime = DateTime.UtcNow; } public static async Task UpdateDeveloperPullRequests() From 7bc506fbaada1c4756dedbdaa15956da9ff9fbf6 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Wed, 24 Jul 2024 12:19:01 -0700 Subject: [PATCH 07/10] Make developerLoginId nullable --- src/AzureExtension/DataManager/AzureDataManager.cs | 2 -- src/AzureExtension/DataModel/AzureDataStoreSchema.cs | 2 +- src/AzureExtension/DataModel/DataObjects/Identity.cs | 7 +++---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index 0f170f1..f2944d8 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -676,8 +676,6 @@ private async Task UpdateDataForDeveloperPullRequestsAsync(DataStoreOperationPar foreach (var repositoryRef in repositoryReferences) { var uri = new AzureUri(repositoryRef.Repository.CloneUrl); - var developerId = repositoryRef.Developer.DeveloperLoginId; - var uris = new List { new(repositoryRef.Repository.CloneUrl), diff --git a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs index 0c6acd3..c943d23 100644 --- a/src/AzureExtension/DataModel/AzureDataStoreSchema.cs +++ b/src/AzureExtension/DataModel/AzureDataStoreSchema.cs @@ -30,7 +30,7 @@ public AzureDataStoreSchema() "Name TEXT NOT NULL COLLATE NOCASE," + "InternalId TEXT NOT NULL," + "Avatar TEXT NOT NULL COLLATE NOCASE," + - "DeveloperLoginId TEXT NOT NULL," + + "DeveloperLoginId TEXT," + "TimeUpdated INTEGER NOT NULL" + ");" + diff --git a/src/AzureExtension/DataModel/DataObjects/Identity.cs b/src/AzureExtension/DataModel/DataObjects/Identity.cs index 0fcbf4e..52497fa 100644 --- a/src/AzureExtension/DataModel/DataObjects/Identity.cs +++ b/src/AzureExtension/DataModel/DataObjects/Identity.cs @@ -46,7 +46,7 @@ public class Identity // The DeveloperLoginId associated with this identity, if one exists. // Empty means there is no associated LoggedInDeveloperId. - public string DeveloperLoginId { get; set; } = string.Empty; + public string? DeveloperLoginId { get; set; } [JsonIgnore] public long TimeUpdated { get; set; } = DataStore.NoForeignKey; @@ -73,7 +73,7 @@ public DeveloperId.DeveloperId? DeveloperId } var devIdProvider = DeveloperIdProvider.GetInstance(); - return devIdProvider.GetDeveloperIdFromAccountIdentifier(DeveloperLoginId); + return devIdProvider.GetDeveloperIdFromAccountIdentifier(DeveloperLoginId!); } } @@ -163,7 +163,7 @@ private static Identity CreateFromIdentity(Microsoft.VisualStudio.Services.Ident }; } - public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identity, string developerLoginId = "") + public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identity, string? developerLoginId = null) { // Check for existing Identity data. var existingIdentity = GetByInternalId(dataStore, identity.InternalId); @@ -171,7 +171,6 @@ public static Identity AddOrUpdateIdentity(DataStore dataStore, Identity identit { identity.Id = existingIdentity.Id; - // If this is a developer, set to developer, but do not set it. // We presume not a developer unless it is explicitly set. if (!string.IsNullOrEmpty(developerLoginId)) { From 67146a60ea2a5773bdfe465c6d2896a2ab79a079 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Wed, 24 Jul 2024 12:30:20 -0700 Subject: [PATCH 08/10] Remove dead code, fix comment --- src/AzureExtension/DataManager/AzureDataManager.cs | 5 ----- src/AzureExtension/DataManager/IAzureDataManager.cs | 2 -- src/AzureExtension/DataModel/DataObjects/Identity.cs | 1 - 3 files changed, 8 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index f2944d8..c70dd59 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -293,11 +293,6 @@ public IEnumerable GetNotifications(DateTime? since = null, bool i return GetPullRequests(repositoryUri.Organization, repositoryUri.Project, repositoryUri.Repository, developerId, view); } - public PullRequests? GetPullRequestsForLoggedInDeveloperIds() - { - return null; - } - private async Task UpdateDataForQueriesAsync(DataStoreOperationParameters parameters) { _log.Debug($"Inside UpdateDataForQueriesAsync with Parameters: {parameters}"); diff --git a/src/AzureExtension/DataManager/IAzureDataManager.cs b/src/AzureExtension/DataManager/IAzureDataManager.cs index 89fe3cf..f6f0ead 100644 --- a/src/AzureExtension/DataManager/IAzureDataManager.cs +++ b/src/AzureExtension/DataManager/IAzureDataManager.cs @@ -47,6 +47,4 @@ public interface IAzureDataManager : IDisposable PullRequests? GetPullRequests(string organization, string project, string repositoryName, string developerId, PullRequestView view); PullRequests? GetPullRequests(AzureUri repositoryUri, string developerId, PullRequestView view); - - PullRequests? GetPullRequestsForLoggedInDeveloperIds(); } diff --git a/src/AzureExtension/DataModel/DataObjects/Identity.cs b/src/AzureExtension/DataModel/DataObjects/Identity.cs index 52497fa..1f12290 100644 --- a/src/AzureExtension/DataModel/DataObjects/Identity.cs +++ b/src/AzureExtension/DataModel/DataObjects/Identity.cs @@ -45,7 +45,6 @@ public class Identity public string Avatar { get; set; } = string.Empty; // The DeveloperLoginId associated with this identity, if one exists. - // Empty means there is no associated LoggedInDeveloperId. public string? DeveloperLoginId { get; set; } [JsonIgnore] From d2b018b18338292643f926b13d898bde6ca7dd2a Mon Sep 17 00:00:00 2001 From: David Bennett Date: Wed, 24 Jul 2024 12:44:39 -0700 Subject: [PATCH 09/10] Swap operation names to a constant --- src/AzureExtension/DataManager/AzureDataManager.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index c70dd59..2d04a70 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -178,7 +178,7 @@ public async Task UpdateDataForQueriesAsync(IEnumerable queryUris, str LoginId = developerLogin, DeveloperId = DeveloperIdProvider.GetInstance().GetDeveloperIdFromAccountIdentifier(developerLogin), RequestOptions = options ?? RequestOptions.RequestOptionsDefault(), - OperationName = "UpdateDataForQueryAsync", + OperationName = nameof(UpdateDataForQueriesAsync), Requestor = requestor ?? Guid.NewGuid(), }; @@ -218,7 +218,7 @@ public async Task UpdateDataForPullRequestsAsync(AzureUri repositoryUri, string DeveloperId = DeveloperIdProvider.GetInstance().GetDeveloperIdFromAccountIdentifier(developerLogin), PullRequestView = view, RequestOptions = options ?? RequestOptions.RequestOptionsDefault(), - OperationName = "UpdateDataForPullRequestsAsync", + OperationName = nameof(UpdateDataForPullRequestsAsync), Requestor = requestor ?? Guid.NewGuid(), }; @@ -621,7 +621,7 @@ await ProcessPullRequests( repository, parameters.DeveloperId.LoginId, parameters.PullRequestView, - parameters.OperationName == "UpdateDataForDeveloperPullRequestsAsync"); + parameters.OperationName == nameof(UpdateDataForDeveloperPullRequestsAsync)); } // Foreach AzureUri return; @@ -633,7 +633,7 @@ public async Task UpdatePullRequestsForLoggedInDeveloperIdsAsync(RequestOptions? var parameters = new DataStoreOperationParameters { RequestOptions = options ?? RequestOptions.RequestOptionsDefault(), - OperationName = "UpdatePullRequestsForLoggedInDeveloperIdsAsync", + OperationName = nameof(UpdatePullRequestsForLoggedInDeveloperIdsAsync), PullRequestView = PullRequestView.Mine, Requestor = requestor ?? Guid.NewGuid(), }; @@ -681,7 +681,7 @@ private async Task UpdateDataForDeveloperPullRequestsAsync(DataStoreOperationPar Uris = uris, DeveloperId = repositoryRef.Developer.DeveloperId, RequestOptions = parameters.RequestOptions, - OperationName = "UpdateDataForDeveloperPullRequestsAsync", + OperationName = nameof(UpdateDataForDeveloperPullRequestsAsync), PullRequestView = PullRequestView.Mine, Requestor = parameters.Requestor, }; From 1e82f940e67c73b3e630fbad7240b5e8aac03155 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 26 Jul 2024 13:49:39 -0700 Subject: [PATCH 10/10] Review feedback --- src/AzureExtension/DataManager/AzureDataManager.cs | 5 +++-- src/AzureExtension/DataModel/DataObjects/PullRequests.cs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/AzureExtension/DataManager/AzureDataManager.cs b/src/AzureExtension/DataManager/AzureDataManager.cs index 2d04a70..24c3a06 100644 --- a/src/AzureExtension/DataManager/AzureDataManager.cs +++ b/src/AzureExtension/DataManager/AzureDataManager.cs @@ -726,7 +726,8 @@ private async Task ProcessPullRequests( // ArtifactId is null in the pull request object and it is not the correct object. The ArtifactId for the // Policy Evaluations API is this: - // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} + // vstfs:///CodeReview/CodeReviewId/{projectId}/{pullRequestId} + // Documentation: https://learn.microsoft.com/en-us/dotnet/api/microsoft.teamfoundation.policy.webapi.policyevaluationrecord.artifactid var artifactId = $"vstfs:///CodeReview/CodeReviewId/{project.InternalId}/{pullRequest.PullRequestId}"; // Url in the GitPullRequest object is a REST Api Url, and the links lack an html Url, so we must build it. @@ -877,7 +878,7 @@ private bool ShouldCreateRejectedNotification(PullRequestPolicyStatus curStatus, { // If the previous status was not failed, or the failure was for a different // reason, then create a new notification. - if (!prevStatus.Rejected || curStatus.PolicyStatusReason != prevStatus.PolicyStatusReason) + if (!prevStatus.Rejected || (curStatus.PolicyStatusReason != prevStatus.PolicyStatusReason)) { return true; } diff --git a/src/AzureExtension/DataModel/DataObjects/PullRequests.cs b/src/AzureExtension/DataModel/DataObjects/PullRequests.cs index e324669..44cd44f 100644 --- a/src/AzureExtension/DataModel/DataObjects/PullRequests.cs +++ b/src/AzureExtension/DataModel/DataObjects/PullRequests.cs @@ -37,7 +37,7 @@ public class PullRequests public long TimeUpdated { get; set; } = DataStore.NoForeignKey; - public override string ToString() => DeveloperLogin + "/" + Repository.Name; + public override string ToString() => $"{DeveloperLogin}/{Repository.Name}"; [Write(false)] private DataStore? DataStore { get; set; }