From 927677ee72208be2672886ef829e65a1cd608835 Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Thu, 21 Nov 2024 12:51:24 -0600 Subject: [PATCH] Add subscriptions to the check aggregator to enable multiple webviews --- c-sharp-tests/Checks/CheckRunResultTests.cs | 36 +- c-sharp/Checks/CheckResultsRecorder.cs | 97 ++++-- c-sharp/Checks/CheckRunResult.cs | 59 ++-- c-sharp/Checks/CheckRunner.cs | 313 +++++++++++------- c-sharp/Checks/ParatextCheckDetails.cs | 3 +- c-sharp/ConcurrentHashSet.cs | 19 ++ .../JsonUtils/ConcurrentHashSetConverter.cs | 33 ++ c-sharp/JsonUtils/SerializationOptions.cs | 3 +- c-sharp/ThreadingUtils.cs | 2 +- extensions/src/hello-world/src/checks.ts | 13 +- extensions/src/platform-scripture/cspell.json | 2 + .../src/platform-scripture/jest.config.ts | 12 + .../src/platform-scripture/package.json | 4 + .../src/checking-results-list.web-view.tsx | 25 +- .../src/checks/check-aggregator.service.ts | 293 +++++++++++++--- .../src/checks/check-aggregator.utils.test.ts | 298 +++++++++++++++++ .../src/checks/check-aggregator.utils.ts | 96 ++++++ .../configure-checks.component.tsx | 10 +- .../extension-host-check-runner.service.ts | 117 ++++++- .../persisted-check-run-result.model.ts | 124 +++++++ .../src/configure-checks.web-view.tsx | 87 +++-- .../src/types/platform-scripture.d.ts | 120 +++++-- .../src/platform-scripture/tsconfig.json | 2 +- package-lock.json | 18 +- 24 files changed, 1461 insertions(+), 325 deletions(-) create mode 100644 c-sharp/ConcurrentHashSet.cs create mode 100644 c-sharp/JsonUtils/ConcurrentHashSetConverter.cs create mode 100644 extensions/src/platform-scripture/jest.config.ts create mode 100644 extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts create mode 100644 extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts create mode 100644 extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts diff --git a/c-sharp-tests/Checks/CheckRunResultTests.cs b/c-sharp-tests/Checks/CheckRunResultTests.cs index 318e1d9ec0..6ca129558e 100644 --- a/c-sharp-tests/Checks/CheckRunResultTests.cs +++ b/c-sharp-tests/Checks/CheckRunResultTests.cs @@ -5,18 +5,22 @@ namespace TestParanextDataProvider.Checks; public class CheckRunResultTests { - [TestCase("checkId", "projectId", "message", "GEN 1:1", 1, "GEN 1:2", 5, true)] - [TestCase("ABC", "projectId", "message", "GEN 1:1", 1, "GEN 1:2", 5, false)] - [TestCase("checkId", "ABC", "message", "GEN 1:1", 1, "GEN 1:2", 5, false)] - [TestCase("checkId", "projectId", "ABC", "GEN 1:1", 1, "GEN 1:2", 5, false)] - [TestCase("checkId", "projectId", "message", "GEN 1:2", 1, "GEN 1:2", 5, false)] - [TestCase("checkId", "projectId", "message", "GEN 1:1", 2, "GEN 1:2", 5, false)] - [TestCase("checkId", "projectId", "message", "GEN 1:1", 1, "GEN 1:3", 5, false)] - [TestCase("checkId", "projectId", "message", "GEN 1:1", 1, "GEN 1:2", 7, false)] + [TestCase("checkId", "resType", "projectId", "msg", false, "GEN 1:1", 1, "GEN 1:2", 5, true)] + [TestCase("ABC", "resType", "projectId", "msg", false, "GEN 1:1", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "ABC", "projectId", "msg", false, "GEN 1:1", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "ABC", "msg", false, "GEN 1:1", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "projectId", "ABC", false, "GEN 1:1", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "projectId", "msg", true, "GEN 1:1", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "projectId", "msg", false, "GEN 1:2", 1, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "projectId", "msg", false, "GEN 1:1", 2, "GEN 1:2", 5, false)] + [TestCase("checkId", "resType", "projectId", "msg", false, "GEN 1:1", 1, "GEN 1:3", 5, false)] + [TestCase("checkId", "resType", "projectId", "msg", false, "GEN 1:1", 1, "GEN 1:2", 7, false)] public void Equality_Objects_ComparedByValue( string checkId2, + string checkResultType2, string projectId2, string message2, + bool isDenied2, string verseRefStart2, int offsetStart2, string verseRefEnd2, @@ -28,13 +32,25 @@ bool expectedResult CheckLocation start1 = new(vrefStart1, 1); VerseRef vrefEnd1 = new("GEN 1:2"); CheckLocation end1 = new(vrefEnd1, 5); - CheckRunResult checkRunResult1 = new("checkId", "projectId", "message", "", start1, end1); + CheckRunResult checkRunResult1 = + new("checkId", "resType", "projectId", "msg", "", false, vrefStart1, start1, end1); VerseRef vrefStart2 = new(verseRefStart2); CheckLocation start2 = new(vrefStart2, offsetStart2); VerseRef vrefEnd2 = new(verseRefEnd2); CheckLocation end2 = new(vrefEnd2, offsetEnd2); - CheckRunResult checkRunResult2 = new(checkId2, projectId2, message2, "", start2, end2); + CheckRunResult checkRunResult2 = + new( + checkId2, + checkResultType2, + projectId2, + message2, + "", + isDenied2, + vrefStart2, + start2, + end2 + ); Assert.That(checkRunResult1 == checkRunResult2, Is.EqualTo(expectedResult)); Assert.That(checkRunResult1.Equals(checkRunResult2), Is.EqualTo(expectedResult)); diff --git a/c-sharp/Checks/CheckResultsRecorder.cs b/c-sharp/Checks/CheckResultsRecorder.cs index 8ce8a8de08..c672cc54e8 100644 --- a/c-sharp/Checks/CheckResultsRecorder.cs +++ b/c-sharp/Checks/CheckResultsRecorder.cs @@ -31,10 +31,13 @@ public void RecordError( CheckRunResults.Add( new CheckRunResult( checkId, + messageId.InternalValue, projectId, message, // ParatextData adds a space at the end sometimes that isn't in the text token.Text.TrimEnd(), + false, + token.VerseRef, // Actual offsets will be calculated below after results have been filtered new CheckLocation(token.VerseRef, offset), new CheckLocation(token.VerseRef, 0) @@ -55,10 +58,13 @@ public void RecordError( CheckRunResults.Add( new CheckRunResult( checkId, + messageId.InternalValue, projectId, message, // ParatextData adds a space at the end sometimes that isn't in the text text.TrimEnd(), + false, + vref, // Actual offsets will be calculated below after results have been filtered new CheckLocation(vref, selectionStart), new CheckLocation(vref, 0) @@ -87,42 +93,85 @@ public List TrimResultsFromBook(int bookNum) } /// - /// Remove all results that are not within the given range + /// After a check has finished running, filter and complete filling in data on the results found. + /// This will:
+ /// 1. Remove all results that are not within the given ranges
+ /// 2. Lookup whether each check result was previously denied
+ /// 3. Calculate actual offsets for each result /// - public void FilterResults(CheckInputRange range) + public void PostProcessResults( + CheckInputRange[]? ranges, + ErrorMessageDenials? denials, + UsfmBookIndexer? indexer + ) { for (int i = CheckRunResults.Count - 1; i >= 0; i--) { var result = CheckRunResults[i]; - var verseRef = result.Start.VerseRef; - if (!range.IsWithinRange(result.ProjectId, verseRef.BookNum, verseRef.ChapterNum)) - CheckRunResults.RemoveAt(i); - } - } - /// - /// Given an indexed view of USFM text, determine the actual offsets to include for each result - /// - public void CalculateActualOffsets(UsfmBookIndexer indexer) - { - foreach (var result in CheckRunResults) - { - var verseIndex = indexer.GetIndex(result.Start.VerseRef); - if (!verseIndex.HasValue) + // Filter by ranges first to throw out whatever we can + if (ranges != null) { - result.Start.Offset = 0; - continue; + var vref = result.Start.VerseRef; + bool isWithinAnyRange = false; + foreach (var range in ranges) + { + if (range.IsWithinRange(result.ProjectId, vref.BookNum, vref.ChapterNum)) + { + isWithinAnyRange = true; + break; + } + } + if (!isWithinAnyRange) + { + CheckRunResults.RemoveAt(i); + continue; + } } - var textIndex = indexer.Usfm.IndexOf(result.Text, verseIndex.Value); - if (textIndex < 0) + // Lookup whether a check was previously denied + if (denials != null) { - result.Start.Offset = 0; - continue; + var isDenied = denials.IsDenied( + new Enum(result.CheckResultType), + result.VerseRef, + result.MessageFormatString, + result.Text + ); + if (isDenied != result.IsDenied) + CheckRunResults[i] = new CheckRunResult( + result.CheckId, + result.CheckResultType, + result.ProjectId, + result.MessageFormatString, + result.Text, + isDenied, + result.VerseRef, + result.Start, + result.End + ); } - result.Start.Offset += textIndex - verseIndex.Value; - result.End.Offset = result.Start.Offset + result.Text.Length; + // Calculate actual offsets + if (indexer != null) + { + var verseIndex = indexer.GetIndex(result.Start.VerseRef); + if (!verseIndex.HasValue) + { + result.Start.Offset = 0; + continue; + } + + var textIndex = indexer.Usfm.IndexOf(result.Text, verseIndex.Value); + if (textIndex < 0) + { + result.Start.Offset = 0; + continue; + } + + result.Start.Offset += textIndex - verseIndex.Value; + result.End.Offset = result.Start.Offset + result.Text.Length; + } } } } diff --git a/c-sharp/Checks/CheckRunResult.cs b/c-sharp/Checks/CheckRunResult.cs index f50617413f..6b423e3210 100644 --- a/c-sharp/Checks/CheckRunResult.cs +++ b/c-sharp/Checks/CheckRunResult.cs @@ -1,4 +1,5 @@ using System.Text.Json.Serialization; +using SIL.Scripture; namespace Paranext.DataProvider.Checks; @@ -6,54 +7,46 @@ namespace Paranext.DataProvider.Checks; /// Represents a single error/issue flagged by a check in a given project. This class must /// serialize/deserialize to the CheckRunResult type defined in TypeScript. /// -public sealed class CheckRunResult( - string checkId, - string projectId, - string messageFormatString, - string text, - CheckLocation start, - CheckLocation end +public sealed record CheckRunResult( + string CheckId, + string CheckResultType, + string ProjectId, + string MessageFormatString, + [property: JsonIgnore] string Text, + bool IsDenied, + VerseRef VerseRef, + CheckLocation Start, + CheckLocation End ) : IEquatable { - public string CheckId { get; } = checkId; - public string ProjectId { get; } = projectId; - public string MessageFormatString { get; } = messageFormatString; - - [JsonIgnore] - public string Text { get; } = text; - public CheckLocation Start { get; } = start; - public CheckLocation End { get; } = end; - - public override bool Equals(object? obj) - { - return obj is CheckRunResult checkRunResult && Equals(checkRunResult); - } - public bool Equals(CheckRunResult? other) { - if (ReferenceEquals(other, null)) + if (other is null) return false; return CheckId == other.CheckId + && CheckResultType == other.CheckResultType && ProjectId == other.ProjectId && MessageFormatString == other.MessageFormatString && Text == other.Text + && IsDenied == other.IsDenied + && VerseRef.ToStringWithVersification() == other.VerseRef.ToStringWithVersification() && Start == other.Start && End == other.End; } - public static bool operator ==(CheckRunResult a, CheckRunResult b) - { - return a.Equals(b); - } - - public static bool operator !=(CheckRunResult a, CheckRunResult b) - { - return !(a == b); - } - public override int GetHashCode() { - return HashCode.Combine(CheckId, ProjectId, MessageFormatString, Text, Start, End); + int hash = HashCode.Combine( + CheckId, + CheckResultType, + ProjectId, + MessageFormatString, + Text, + IsDenied, + VerseRef, + Start + ); + return HashCode.Combine(hash, End); } } diff --git a/c-sharp/Checks/CheckRunner.cs b/c-sharp/Checks/CheckRunner.cs index 4e5ff121a4..ecfdeea532 100644 --- a/c-sharp/Checks/CheckRunner.cs +++ b/c-sharp/Checks/CheckRunner.cs @@ -1,9 +1,11 @@ +using System.Collections.Concurrent; using System.Text.Json; using Paranext.DataProvider.ParatextUtils; using Paranext.DataProvider.Projects; using Paratext.Checks; using Paratext.Data; using Paratext.Data.Checking; +using SIL.Scripture; namespace Paranext.DataProvider.Checks; @@ -21,6 +23,7 @@ private sealed class CheckForProject(ScriptureCheckBase check, string checkId, s { public ScriptureCheckBase Check { get; } = check; public CheckResultsRecorder ResultsRecorder { get; } = new(checkId, projectId); + public object Lock = new(); } #endregion @@ -54,10 +57,14 @@ private sealed class CheckForProject(ScriptureCheckBase check, string checkId, s { CheckType.RepeatedWord.InternalValue, new(CheckType.RepeatedWord) }, }; private CheckInputRange[] _activeRanges = []; - private readonly Dictionary _dataSourcesByProjectId = []; - private readonly Dictionary<(string checkId, string projectId), CheckForProject> _checksByIds = - []; - private readonly object _dataProviderLock = new(); + private readonly ConcurrentDictionary _dataSourcesByProjectId = []; + private readonly ConcurrentDictionary _denialsByProjectId = []; + private readonly ConcurrentDictionary< + (string checkId, string projectId), + CheckForProject + > _checksByIds = []; + private readonly object _setActiveRangesLock = new(); + private readonly object _runChecksLock = new(); #endregion @@ -68,6 +75,8 @@ private sealed class CheckForProject(ScriptureCheckBase check, string checkId, s { return [ + ("allowCheckResult", AllowCheckResult), + ("denyCheckResult", DenyCheckResult), ("disableCheck", DisableCheck), ("enableCheck", EnableCheck), ("getActiveRanges", GetActiveRanges), @@ -106,32 +115,40 @@ private bool SetActiveRanges(JsonElement _ignore, CheckInputRange[]? ranges) throw new ArgumentException("Ranges cannot span between books"); } - foreach (var projectId in _activeRanges.Select(range => range.ProjectId).Distinct()) - GetOrCreateDataSource(projectId).ScrText.TextChanged -= RerunChecks; - foreach (var projectId in ranges.Select(range => range.ProjectId).Distinct()) - GetOrCreateDataSource(projectId).ScrText.TextChanged += RerunChecks; - _activeRanges = ranges; - - bool notifyOfUpdatedCheckResults = false; - foreach (var projectId in ranges.Select(range => range.ProjectId).Distinct()) + lock (_setActiveRangesLock) { - bool producedNewOrDifferentResults = RunChecksForProject(projectId); - notifyOfUpdatedCheckResults |= producedNewOrDifferentResults; - } + var oldProjectIds = _activeRanges.Select(range => range.ProjectId).Distinct(); + var newProjectIds = ranges.Select(range => range.ProjectId).Distinct(); - List updateEvents = [DATA_TYPE_ACTIVE_RANGES]; - if (notifyOfUpdatedCheckResults) - updateEvents.Add(DATA_TYPE_CHECK_RESULTS); + foreach (var projectId in oldProjectIds) + GetOrCreateDataSource(projectId).ScrText.TextChanged -= RerunChecks; + foreach (var projectId in newProjectIds) + GetOrCreateDataSource(projectId).ScrText.TextChanged += RerunChecks; + _activeRanges = ranges; - SendDataUpdateEvent(updateEvents, "active ranges update"); - return true; + foreach (var projectId in newProjectIds) + RunChecksForProject(projectId); + + SendDataUpdateEvent(DATA_TYPE_ACTIVE_RANGES, "SetActiveRanges"); + return true; + } } private List GetCheckResults(JsonElement _ignore) { var retVal = new List(); + foreach (var check in _checksByIds.Values) - retVal.AddRange(check.ResultsRecorder.CheckRunResults); + { + lock (check.Lock) + { + retVal.AddRange(check.ResultsRecorder.CheckRunResults); + } + + if (retVal.Count >= 1000) + break; + } + if (retVal.Count > 1000) { var fullCount = retVal.Count; @@ -139,73 +156,108 @@ private List GetCheckResults(JsonElement _ignore) retVal.TrimExcess(); Console.WriteLine($"Trimming {fullCount} check results to 1000"); } + Console.WriteLine($"Returning {retVal.Count} check results"); return retVal; } - private string[] EnableCheck(string checkId, string projectId) + private void EnableCheck(string checkId, string projectId) { ArgumentException.ThrowIfNullOrEmpty(checkId); ArgumentException.ThrowIfNullOrEmpty(projectId); - List updateEvents = []; - var enabledProjectIds = _checkDetailsByCheckId[checkId].EnabledProjectIds; if (enabledProjectIds.Contains(projectId)) - Console.WriteLine($"Check {checkId} for project {projectId} was already enabled"); - else { - updateEvents.Add(DATA_TYPE_AVAILABLE_CHECKS); - enabledProjectIds.Add(projectId); - var check = CheckFactory.CreateCheck(checkId, GetOrCreateDataSource(projectId)); - _checksByIds.Add((checkId, projectId), new CheckForProject(check, checkId, projectId)); - Console.WriteLine($"Enabled check {checkId} for project {projectId}"); - if (RunChecksForProject(projectId)) - updateEvents.Add(DATA_TYPE_CHECK_RESULTS); + Console.WriteLine($"Check {checkId} for project {projectId} was already enabled"); + return; } - if (updateEvents.Count > 0) - SendDataUpdateEvent(updateEvents, "enable check update"); - return []; + enabledProjectIds.Add(projectId); + var check = CheckFactory.CreateCheck(checkId, GetOrCreateDataSource(projectId)); + _checksByIds.TryAdd((checkId, projectId), new CheckForProject(check, checkId, projectId)); + Console.WriteLine($"Enabled check {checkId} for project {projectId}"); + RunChecksForProject(projectId); + SendDataUpdateEvent(DATA_TYPE_AVAILABLE_CHECKS, "EnableCheck"); } - private bool DisableCheck(string checkId, string? projectId) + private void DisableCheck(string checkId, string? projectId) { ArgumentException.ThrowIfNullOrEmpty(checkId); - List updateEvents = [DATA_TYPE_AVAILABLE_CHECKS]; + HashSet updateEvents = [DATA_TYPE_AVAILABLE_CHECKS]; var checkDetails = _checkDetailsByCheckId[checkId]; + string[] projectIds; if (string.IsNullOrEmpty(projectId)) + projectIds = _checksByIds + .Keys.Where(ids => ids.checkId == checkId) + .Select(ids => ids.projectId) + .Distinct() + .ToArray(); + else + projectIds = [projectId]; + + var cid = checkId; + foreach (var pid in projectIds) { - Console.WriteLine($"Disabled check {checkId} for all projects"); - checkDetails.EnabledProjectIds.Clear(); - bool resultsRemoved = false; - foreach (var ids in _checksByIds.Keys.Where(ids => ids.checkId == checkId)) + var checkData = _checksByIds[(cid, pid)]; + lock (checkData.Lock) { - resultsRemoved |= _checksByIds[ids].ResultsRecorder.CheckRunResults.Count > 0; - _checksByIds.Remove(ids); + if (checkData.ResultsRecorder.CheckRunResults.Count > 0) + updateEvents.Add(DATA_TYPE_CHECK_RESULTS); + _checksByIds.TryRemove((cid, pid), out _); + Console.WriteLine( + checkDetails.EnabledProjectIds.Remove(pid) + ? $"Disabled check {cid} for project {pid}" + : $"Project {pid} was not enabled for check {cid}" + ); } - if (resultsRemoved) - updateEvents.Add(DATA_TYPE_CHECK_RESULTS); + } + + SendDataUpdateEvent(updateEvents.ToList(), "disable check update"); + } - SendDataUpdateEvent(updateEvents, "disable check update"); + private bool DenyCheckResult( + string checkId, + string checkResultType, + string projectId, + VerseRef vRef, + string selectedText, + string? _checkResultUniqueId + ) + { + if (!_checksByIds.TryGetValue((checkId, projectId), out var check)) + throw new Exception($"Check {checkId} is not enabled for project {projectId}"); + lock (check.Lock) + { + var denials = GetOrCreateDenials(projectId); + denials.AddDenial(MessageId.UnknownUseMsgText, vRef, checkResultType, selectedText); + check.ResultsRecorder.PostProcessResults(null, denials, null); + SendDataUpdateEvent(DATA_TYPE_CHECK_RESULTS, "Denied check result"); return true; } + } - var projectIds = checkDetails.EnabledProjectIds; - int resultsCount = _checksByIds[(checkId, projectId)].ResultsRecorder.CheckRunResults.Count; - _checksByIds.Remove((checkId, projectId)); - Console.WriteLine( - projectIds.Remove(projectId) - ? $"Disabled check {checkId} for project {projectId}" - : $"Project {projectId} was not enabled for check {checkId}" - ); - if (resultsCount > 0) - updateEvents.Add(DATA_TYPE_CHECK_RESULTS); - - SendDataUpdateEvent(updateEvents, "disable check update"); - return true; + private bool AllowCheckResult( + string checkId, + string checkResultType, + string projectId, + VerseRef vRef, + string selectedText, + string? _checkResultUniqueId + ) + { + if (!_checksByIds.TryGetValue((checkId, projectId), out var check)) + throw new Exception($"Check {checkId} has not been run for project {projectId}"); + lock (check.Lock) + { + var denials = GetOrCreateDenials(projectId); + denials.RemoveDenial(MessageId.UnknownUseMsgText, vRef, checkResultType, selectedText); + check.ResultsRecorder.PostProcessResults(null, denials, null); + SendDataUpdateEvent(DATA_TYPE_CHECK_RESULTS, "Allowed check result"); + return true; + } } private void RerunChecks(object? sender, TextChangedEventArgs e) @@ -219,55 +271,68 @@ private void RerunChecks(object? sender, TextChangedEventArgs e) return; } - bool notifyOfNewOrDifferentResults = false; - // Only run the checks if we have some range active that includes the text that changed if (_activeRanges.Any((range) => range.IsWithinRange(projectId, e.BookNum, e.ChapterNum))) - notifyOfNewOrDifferentResults = RunChecksForProject(projectId); + RunChecksForProject(projectId); + } - if (notifyOfNewOrDifferentResults) - SendDataUpdateEvent(DATA_TYPE_CHECK_RESULTS, "RerunChecks"); + /// + /// Asynchronously run all enabled checks on a project for books within active ranges + /// + private void RunChecksForProject(string projectId) + { + ThreadingUtils.RunTask( + Task.Run(() => RunChecksForProjectInternal(projectId)), + $"RunChecksForProject {projectId}" + ); } /// /// Run all enabled checks on a project for books within active ranges /// - /// true if some check produced any new or different results, false otherwise - private bool RunChecksForProject(string projectId) + private void RunChecksForProjectInternal(string projectId) { - bool retVal = false; + lock (_runChecksLock) + { + bool signalUpdatedData = false; - // Text has to be tokenized for the checks before the checks can run - var enabledChecksForProject = _checksByIds - .Where((kvp) => kvp.Key.projectId == projectId) - .Select((kvp) => kvp.Value.Check); - CheckDataFormat neededDataFormat = 0; - foreach (var check in enabledChecksForProject) - neededDataFormat |= check.NeededFormat; + // Text has to be tokenized for the checks before the checks can run + var enabledChecksForProject = _checksByIds + .Where((kvp) => kvp.Key.projectId == projectId) + .Select((kvp) => kvp.Value.Check); + CheckDataFormat neededDataFormat = 0; + foreach (var check in enabledChecksForProject) + neededDataFormat |= check.NeededFormat; - foreach (var range in _activeRanges.Where((range) => range.ProjectId == projectId)) - { - // "GetText" will tokenize the text for checks to use - // "0" chapter number means all chapters - _dataSourcesByProjectId[projectId].GetText(range.Start.BookNum, 0, neededDataFormat); - - var scrText = LocalParatextProjects.GetParatextProject(projectId); - var indexer = new UsfmBookIndexer(scrText.GetText(range.Start.BookNum)); - - foreach ( - var checkId in _checkDetailsByCheckId - .Values.Where( - (checkDetails) => checkDetails.EnabledProjectIds.Contains(projectId) - ) - .Select((checkDetails) => checkDetails.CheckId) - ) + foreach (var range in _activeRanges.Where((range) => range.ProjectId == projectId)) { - var check = _checksByIds[(checkId, projectId)].Check; - bool newResultsReturned = RunCheck(checkId, check, range, indexer); - retVal |= newResultsReturned; + // "GetText" will tokenize the text for checks to use + // "0" chapter number means all chapters + _dataSourcesByProjectId[projectId] + .GetText(range.Start.BookNum, 0, neededDataFormat); + + var scrText = LocalParatextProjects.GetParatextProject(projectId); + var indexer = new UsfmBookIndexer(scrText.GetText(range.Start.BookNum)); + + foreach ( + var checkId in _checkDetailsByCheckId + .Values.Where( + (checkDetails) => checkDetails.EnabledProjectIds.Contains(projectId) + ) + .Select((checkDetails) => checkDetails.CheckId) + ) + { + if (_checksByIds.TryGetValue((checkId, projectId), out var x)) + { + bool newResultsReturned = RunCheck(checkId, x.Check, range, indexer); + signalUpdatedData |= newResultsReturned; + } + } } + + if (signalUpdatedData) + SendDataUpdateEvent(DATA_TYPE_CHECK_RESULTS, "RunChecksForProject"); } - return retVal; } /// @@ -280,41 +345,45 @@ private bool RunCheck( UsfmBookIndexer indexer ) { - CheckResultsRecorder recorder; - if (!_checksByIds.TryGetValue((checkId, range.ProjectId), out var data)) - { - var checkData = new CheckForProject(check, checkId, range.ProjectId); - _checksByIds.Add((checkId, range.ProjectId), checkData); - recorder = checkData.ResultsRecorder; - } - else - recorder = data.ResultsRecorder; + var checkData = _checksByIds.GetOrAdd( + (checkId, range.ProjectId), + (ids) => new CheckForProject(check, ids.checkId, ids.projectId) + ); + CheckResultsRecorder recorder = checkData.ResultsRecorder; + ErrorMessageDenials denials = GetOrCreateDenials(range.ProjectId); - var removedItems = recorder.TrimResultsFromBook(range.Start.BookNum); - int totalBeforeRunning = recorder.CheckRunResults.Count; - check.Run(range.Start.BookNum, GetOrCreateDataSource(range.ProjectId), recorder); - recorder.FilterResults(range); - recorder.CalculateActualOffsets(indexer); - int totalAfterRunning = recorder.CheckRunResults.Count; + lock (checkData.Lock) + { + var removedItems = recorder.TrimResultsFromBook(range.Start.BookNum); + int totalBeforeRunning = recorder.CheckRunResults.Count; + check.Run(range.Start.BookNum, GetOrCreateDataSource(range.ProjectId), recorder); + recorder.PostProcessResults(_activeRanges, denials, indexer); + int totalAfterRunning = recorder.CheckRunResults.Count; - if (totalAfterRunning == totalBeforeRunning) - return removedItems.Count != 0; + if (totalAfterRunning == totalBeforeRunning) + return removedItems.Count != 0; - if (totalAfterRunning != totalBeforeRunning + removedItems.Count) - return true; + if (totalAfterRunning != totalBeforeRunning + removedItems.Count) + return true; - return removedItems.Exists(item => !recorder.CheckRunResults.Contains(item)); + return removedItems.Exists(item => !recorder.CheckRunResults.Contains(item)); + } } private ChecksDataSource GetOrCreateDataSource(string projectId) { - if (!_dataSourcesByProjectId.TryGetValue(projectId, out var dataSource)) - { - var scrText = LocalParatextProjects.GetParatextProject(projectId); - dataSource = new ChecksDataSource(scrText); - _dataSourcesByProjectId.Add(projectId, dataSource); - } - return dataSource; + return _dataSourcesByProjectId.GetOrAdd( + projectId, + id => new ChecksDataSource(LocalParatextProjects.GetParatextProject(id)) + ); + } + + private ErrorMessageDenials GetOrCreateDenials(string projectId) + { + return _denialsByProjectId.GetOrAdd( + projectId, + id => ErrorMessageDenials.Get(LocalParatextProjects.GetParatextProject(id)) + ); } #endregion diff --git a/c-sharp/Checks/ParatextCheckDetails.cs b/c-sharp/Checks/ParatextCheckDetails.cs index 32c0cb2941..58ae7240bf 100644 --- a/c-sharp/Checks/ParatextCheckDetails.cs +++ b/c-sharp/Checks/ParatextCheckDetails.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using Paratext.Data.Checking; using PtxUtils; @@ -12,5 +13,5 @@ public sealed class ParatextCheckDetails(Enum checkType) public string CheckName { get; } = checkType.ToString(); public string CheckDescription { get; } = checkType.ToLocalizedString(); public string CheckId { get; } = checkType.InternalValue; - public List EnabledProjectIds { get; } = []; + public ConcurrentHashSet EnabledProjectIds { get; } = new ConcurrentHashSet(); } diff --git a/c-sharp/ConcurrentHashSet.cs b/c-sharp/ConcurrentHashSet.cs new file mode 100644 index 0000000000..e4d2166e3f --- /dev/null +++ b/c-sharp/ConcurrentHashSet.cs @@ -0,0 +1,19 @@ +using System.Collections.Concurrent; + +namespace Paranext.DataProvider; + +public class ConcurrentHashSet + where T : notnull +{ + private readonly ConcurrentDictionary _dictionary = new(); + + public int Count => _dictionary.Count; + + public bool Add(T item) => _dictionary.TryAdd(item, true); + + public bool Remove(T item) => _dictionary.TryRemove(item, out _); + + public bool Contains(T item) => _dictionary.ContainsKey(item); + + public IEnumerable GetItems() => _dictionary.Keys; +} diff --git a/c-sharp/JsonUtils/ConcurrentHashSetConverter.cs b/c-sharp/JsonUtils/ConcurrentHashSetConverter.cs new file mode 100644 index 0000000000..374768e193 --- /dev/null +++ b/c-sharp/JsonUtils/ConcurrentHashSetConverter.cs @@ -0,0 +1,33 @@ +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace Paranext.DataProvider.JsonUtils; + +public class ConcurrentHashSetConverter : JsonConverter> + where T : notnull +{ + public override ConcurrentHashSet Read( + ref Utf8JsonReader reader, + Type typeToConvert, + JsonSerializerOptions options + ) + { + var list = JsonSerializer.Deserialize>(ref reader, options); + var retVal = new ConcurrentHashSet(); + if (list != null) + { + foreach (var item in list) + retVal.Add(item); + } + return retVal; + } + + public override void Write( + Utf8JsonWriter writer, + ConcurrentHashSet value, + JsonSerializerOptions options + ) + { + JsonSerializer.Serialize(writer, value.GetItems(), options); + } +} diff --git a/c-sharp/JsonUtils/SerializationOptions.cs b/c-sharp/JsonUtils/SerializationOptions.cs index 061baa2ee3..ac370191ce 100644 --- a/c-sharp/JsonUtils/SerializationOptions.cs +++ b/c-sharp/JsonUtils/SerializationOptions.cs @@ -23,8 +23,9 @@ public static JsonSerializerOptions CreateSerializationOptions() IgnoreReadOnlyProperties = false, // Need types to be serialized }; options.Converters.Add(new CommentConverter()); - options.Converters.Add(new VerseRefConverter()); + options.Converters.Add(new ConcurrentHashSetConverter()); options.Converters.Add(new RegistrationDataConverter()); + options.Converters.Add(new VerseRefConverter()); return options; } diff --git a/c-sharp/ThreadingUtils.cs b/c-sharp/ThreadingUtils.cs index be7b777d14..b71855ad4a 100644 --- a/c-sharp/ThreadingUtils.cs +++ b/c-sharp/ThreadingUtils.cs @@ -23,7 +23,7 @@ public static class ThreadingUtils /// timeout is provided, then the task is not waited on at all. /// true if the task completed within the given timeout (or immediately if no timeout /// is provided), false otherwise - public static bool RunTask(Task task, string description = "task", TimeSpan? timeout = null) + public static bool RunTask(Task task, string description, TimeSpan? timeout = null) { bool taskCompleted = task.IsCompleted; if (taskCompleted) diff --git a/extensions/src/hello-world/src/checks.ts b/extensions/src/hello-world/src/checks.ts index 0a427e3cc6..da72c31e15 100644 --- a/extensions/src/hello-world/src/checks.ts +++ b/extensions/src/hello-world/src/checks.ts @@ -76,15 +76,18 @@ class HelloCheck implements Check { const verseIndex = usfm.lastIndexOf('\\v ', cursor) + 3; const inVerse = parseInt(usfm.slice(verseIndex, verseIndex + 4), 10); const offset = cursor - verseIndex - inVerse.toString().length - 1; + const verseRef = new VerseRef(range.start.book, inChapter.toString(), inVerse.toString()); const newResult: CheckRunResult = { + checkId: 'sheep', + checkResultType: 'sheep', + isDenied: false, projectId: this.targetProjectId ?? '', messageFormatString: 'Found the word "sheep"', - start: { - verseRef: new VerseRef(range.start.book, inChapter.toString(), inVerse.toString()), - offset, - }, + selectedText: 'sheep', + verseRef, + start: { verseRef, offset }, end: { - verseRef: new VerseRef(range.start.book, inChapter.toString(), inVerse.toString()), + verseRef, offset: offset + 5, }, }; diff --git a/extensions/src/platform-scripture/cspell.json b/extensions/src/platform-scripture/cspell.json index f331240e09..dee5684374 100644 --- a/extensions/src/platform-scripture/cspell.json +++ b/extensions/src/platform-scripture/cspell.json @@ -17,6 +17,8 @@ "appdata", "asyncs", "autodocs", + "bbbccc", + "bbbcccvvv", "biblionexus", "camelcase", "consts", diff --git a/extensions/src/platform-scripture/jest.config.ts b/extensions/src/platform-scripture/jest.config.ts new file mode 100644 index 0000000000..786ba1fdde --- /dev/null +++ b/extensions/src/platform-scripture/jest.config.ts @@ -0,0 +1,12 @@ +module.exports = { + preset: 'ts-jest', + testEnvironment: 'node', + testMatch: ['/src/**/*.test.ts'], + moduleFileExtensions: ['js', 'jsx', 'ts', 'tsx', 'json'], + transform: { + '\\.(ts|tsx|js|jsx)$': 'ts-jest', + }, + moduleNameMapper: { + '^@platform-scripture/(.*)$': '$1', + }, +}; diff --git a/extensions/src/platform-scripture/package.json b/extensions/src/platform-scripture/package.json index 88bce9b451..3ed964a38c 100644 --- a/extensions/src/platform-scripture/package.json +++ b/extensions/src/platform-scripture/package.json @@ -26,6 +26,7 @@ "lint:styles": "stylelint **/*.{css,scss}", "lint-fix": "npm run lint-fix:scripts && npm run lint:styles -- --fix", "lint-fix:scripts": "npm run format && npm run lint:scripts", + "test": "jest --silent", "typecheck": "tsc -p ./tsconfig.json" }, "browserslist": [], @@ -41,6 +42,7 @@ "@biblionexus-foundation/scripture-utilities": "^0.0.4", "@swc/core": "^1.7.35", "@tailwindcss/typography": "^0.5.15", + "@types/jest": "^29.5.14", "@types/node": "^20.16.11", "@types/react": "^18.3.11", "@types/react-dom": "^18.3.1", @@ -67,6 +69,7 @@ "eslint-plugin-react": "^7.37.1", "eslint-plugin-react-hooks": "^4.6.2", "glob": "^10.4.5", + "jest": "^29.7.0", "lucide-react": "^0.452.0", "papi-dts": "file:../../../lib/papi-dts", "platform-bible-react": "file:../../../lib/platform-bible-react", @@ -83,6 +86,7 @@ "swc-loader": "^0.2.6", "tailwindcss": "^3.4.13", "tailwindcss-animate": "^1.0.7", + "ts-jest": "^29.2.5", "ts-node": "^10.9.2", "tsconfig-paths": "^4.2.0", "tsconfig-paths-webpack-plugin": "^4.1.0", diff --git a/extensions/src/platform-scripture/src/checking-results-list.web-view.tsx b/extensions/src/platform-scripture/src/checking-results-list.web-view.tsx index 7eae9ac85d..b191a722db 100644 --- a/extensions/src/platform-scripture/src/checking-results-list.web-view.tsx +++ b/extensions/src/platform-scripture/src/checking-results-list.web-view.tsx @@ -1,6 +1,6 @@ import { WebViewProps } from '@papi/core'; import { Label, ResultsSet, ScriptureResultsViewer, usePromise } from 'platform-bible-react'; -import { useCallback, useEffect, useMemo } from 'react'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; import { useData, useLocalizedStrings } from '@papi/frontend/react'; import { CheckRunnerCheckDetails, CheckRunResult } from 'platform-scripture'; import { Canon } from '@sillsdev/scripture'; @@ -92,9 +92,18 @@ global.webViewComponent = function CheckingResultsListWebView({ projectId, updateWebViewDefinition, }: WebViewProps) { - const [checkResults] = useData('platformScripture.checkAggregator').CheckResults(undefined, []); + const [subscriptionId, setSubscriptionId] = useState(''); + + const handleSubscriptionIdChange = (event: React.ChangeEvent) => { + setSubscriptionId(event.target.value); + }; + + const [checkResults] = useData('platformScripture.checkAggregator').CheckResults( + subscriptionId, + [], + ); const [availableChecks] = useData('platformScripture.checkAggregator').AvailableChecks( - undefined, + subscriptionId, [], ); @@ -162,6 +171,16 @@ global.webViewComponent = function CheckingResultsListWebView({ return (
{label && } + {/* Really we don't want a separate place for subscription input. The check results UI should + be tied to the subscription configuration so they share the same subscription ID. */} +
+ +
); diff --git a/extensions/src/platform-scripture/src/checks/check-aggregator.service.ts b/extensions/src/platform-scripture/src/checks/check-aggregator.service.ts index 89b105f60e..41e56a88ff 100644 --- a/extensions/src/platform-scripture/src/checks/check-aggregator.service.ts +++ b/extensions/src/platform-scripture/src/checks/check-aggregator.service.ts @@ -1,4 +1,4 @@ -import { createSyncProxyForAsyncObject } from 'platform-bible-utils'; +import { createSyncProxyForAsyncObject, newGuid } from 'platform-bible-utils'; import { DataProviderUpdateInstructions, IDataProviderEngine, @@ -6,87 +6,288 @@ import { } from '@papi/core'; import papi, { DataProviderEngine, logger } from '@papi/backend'; import { + CheckAggregatorDataTypes, CheckInputRange, + CheckResultClassifier, CheckRunResult, CheckRunnerCheckDetails, - CheckRunnerDataTypes, + CheckSubscriptionId, + CheckSubscriptionManager, ICheckAggregatorService, ICheckRunner, + SettableCheckDetails, } from 'platform-scripture'; +import { VerseRef } from '@sillsdev/scripture'; import { CHECK_RUNNER_NETWORK_OBJECT_TYPE } from './check.model'; +import { + aggregateProjectIdsByCheckId, + aggregateRanges, + isWithinRange, + SubscriptionData, +} from './check-aggregator.utils'; -class CheckDataProviderEngine - extends DataProviderEngine - implements IDataProviderEngine +class CheckAggregatorDataProviderEngine + extends DataProviderEngine + implements + IDataProviderEngine, + CheckResultClassifier, + CheckSubscriptionManager { // Maps check runner IDs to their ICheckRunner objects checkRunners = new Map(); - // The most recent values that were set via `setActiveRanges` - lastRangesSet: CheckInputRange[] = []; + // Maps subscription IDs that we created to data about the subscription + subscriptionsBySubscriptionId = new Map(); - // Required method since this is a data provider engine - // eslint-disable-next-line @typescript-eslint/class-methods-use-this - async setAvailableChecks(): Promise> { - throw new Error('setAvailableChecks disabled'); - } + // #region Checks - async setActiveRanges( - _ignore: undefined, - ranges: CheckInputRange[], - ): Promise> { - this.lastRangesSet = ranges; + async getAvailableChecks( + subscriptionId: CheckSubscriptionId, + ): Promise { + if (!subscriptionId) return []; + const subscriptionData = this.findSubscription(subscriptionId); + + // Gather up all checks that are currently registered await this.refreshCheckRunners(); + const retVal: CheckRunnerCheckDetails[] = []; await Promise.all( [...this.checkRunners.values()].map(async (checkRunner) => { - await checkRunner.setActiveRanges(undefined, ranges); + const checksAvailable = await checkRunner.getAvailableChecks(undefined); + if (checksAvailable.length === 0) return; + retVal.push(...checksAvailable); }), ); - return 'ActiveRanges'; + + // Mark which projects this subscription is using for each check + const { enabledProjectsByCheckId } = subscriptionData; + retVal.forEach((checkDetails) => { + checkDetails.enabledProjectIds = [ + ...(enabledProjectsByCheckId.get(checkDetails.checkId) ?? []), + ]; + }); + + return retVal; } - // Required method since this is a data provider engine - // eslint-disable-next-line @typescript-eslint/class-methods-use-this - async setCheckResults(): Promise> { - throw new Error('setCheckResults disabled'); + async setAvailableChecks( + subscriptionId: CheckSubscriptionId, + newAvailableChecks: SettableCheckDetails[], + ): Promise> { + if (!subscriptionId) return false; + const subscriptionData = this.findSubscription(subscriptionId); + + // Take a snapshot + const beforeUpdate = aggregateProjectIdsByCheckId(this.subscriptionsBySubscriptionId); + + // Update the subscription + subscriptionData.enabledProjectsByCheckId.clear(); + newAvailableChecks.forEach((singleCheckDetails) => { + subscriptionData.enabledProjectsByCheckId.set( + singleCheckDetails.checkId, + new Set(singleCheckDetails.enabledProjectIds), + ); + }); + + // Take another snapshot + const afterUpdate = aggregateProjectIdsByCheckId(this.subscriptionsBySubscriptionId); + + // Enable checks that weren't previously enabled + const toEnable: { checkId: string; projectId: string }[] = []; + afterUpdate.forEach((projectIds, checkId) => { + projectIds.forEach((projectId) => { + const beforeProjectIds = beforeUpdate.get(checkId); + if (!beforeProjectIds || !beforeProjectIds.has(projectId)) + toEnable.push({ checkId, projectId }); + }); + }); + await Promise.all(toEnable.map((x) => this.enableCheck(x.checkId, x.projectId))); + + // Disable checks that are no longer enabled + const toDisable: { checkId: string; projectId: string }[] = []; + beforeUpdate.forEach((projectIds, checkId) => { + projectIds.forEach((projectId) => { + const afterProjectIds = afterUpdate.get(checkId); + if (!afterProjectIds || !afterProjectIds.has(projectId)) + toDisable.push({ checkId, projectId }); + }); + }); + await Promise.all(toDisable.map((x) => this.disableCheck(x.checkId, x.projectId))); + + return ['AvailableChecks', 'CheckResults']; } - async getAvailableChecks(): Promise { + // #endregion + + // #region Active Ranges + + async getActiveRanges(subscriptionId: CheckSubscriptionId): Promise { + if (!subscriptionId) return []; + return this.findSubscription(subscriptionId).ranges; + } + + async setActiveRanges( + subscriptionId: CheckSubscriptionId, + ranges: CheckInputRange[], + ): Promise> { + if (!subscriptionId) return false; + + // Update the subscription + this.findSubscription(subscriptionId).ranges = ranges; + + // Aggregate all ranges from all subscriptions + const aggregatedRanges = aggregateRanges(this.subscriptionsBySubscriptionId); + + // Push the single, aggregated view of ranges into all of the active check runners await this.refreshCheckRunners(); - const retVal: CheckRunnerCheckDetails[] = []; await Promise.all( [...this.checkRunners.values()].map(async (checkRunner) => { - const checksAvailable = await checkRunner.getAvailableChecks(undefined); - if (checksAvailable.length === 0) return; - logger.debug(`Checks available ${JSON.stringify(checksAvailable)}`); - retVal.push(...checksAvailable); + await checkRunner.setActiveRanges(undefined, aggregatedRanges); }), ); - return retVal; + return ['ActiveRanges', 'CheckResults']; } - async getActiveRanges(): Promise { - return this.lastRangesSet; + // #endregion + + // #region Include Denied Results + + async getIncludeDeniedResults(subscriptionId: CheckSubscriptionId): Promise { + if (!subscriptionId) return false; + return this.findSubscription(subscriptionId).includeDeniedResults; } - async getCheckResults(): Promise { + async setIncludeDeniedResults( + subscriptionId: CheckSubscriptionId, + newIncludeDeniedResults: boolean, + ): Promise> { + if (!subscriptionId) return false; + this.findSubscription(subscriptionId).includeDeniedResults = newIncludeDeniedResults; + return ['IncludeDeniedResults', 'CheckResults']; + } + + // #endregion + + // #region Check Results + + async getCheckResults(subscriptionId: CheckSubscriptionId): Promise { + if (!subscriptionId) return []; + + // Find all applicable check runners for our subscription + const subscription = this.findSubscription(subscriptionId); + const checkIdsForSubscription = Array.from(subscription.enabledProjectsByCheckId.keys()); + const checkRunners = new Set(); + await Promise.all( + checkIdsForSubscription.map(async (checkId: string) => { + const checkRunner = await this.findCheckRunnerForCheckId(checkId); + checkRunners.add(checkRunner); + }), + ); + + // Get results from all applicable check runners and filter based on the subscription const retVal: CheckRunResult[] = []; await Promise.all( - [...this.checkRunners.values()].map(async (checkRunner) => { + [...checkRunners].map(async (checkRunner) => { const results = await checkRunner.getCheckResults(undefined); - if (results.length === 0) return; - retVal.push(...results); + results.forEach((result) => { + // Filter by "denied" + if (result.isDenied && !subscription.includeDeniedResults) return; + + // Filter by range + const { verseRef } = result; + if (!subscription.ranges.some((range) => isWithinRange(range, verseRef))) return; + + // Filter by check + project + if (!result.checkId) return; + const projects = subscription.enabledProjectsByCheckId.get(result.checkId); + if (projects?.has(result.projectId)) retVal.push(result); + }); }), ); return retVal; } - async enableCheck(checkId: string, projectId: string): Promise { + // Required method since this is a data provider engine + // eslint-disable-next-line class-methods-use-this, @typescript-eslint/class-methods-use-this + async setCheckResults(): Promise> { + throw new Error('setCheckResults disabled'); + } + + // #endregion + + // #region Deny/Allow Individual Results + + async allowCheckResult( + checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ): Promise { + const checkRunner = await this.findCheckRunnerForCheckId(checkId); + const retVal = checkRunner.allowCheckResult( + checkId, + checkResultType, + projectId, + verseRef, + selectedText, + checkResultUniqueId, + ); + this.notifyUpdate('CheckResults'); + return retVal; + } + + async denyCheckResult( + checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ): Promise { + const checkRunner = await this.findCheckRunnerForCheckId(checkId); + const retVal = checkRunner.denyCheckResult( + checkId, + checkResultType, + projectId, + verseRef, + selectedText, + checkResultUniqueId, + ); + this.notifyUpdate('CheckResults'); + return retVal; + } + + // #endregion + + // #region Subscriptions + + async createSubscription(): Promise { + const retVal = newGuid(); + this.subscriptionsBySubscriptionId.set(retVal, { + enabledProjectsByCheckId: new Map>(), + ranges: [], + includeDeniedResults: true, + }); + logger.debug(`Created check subscription: ${retVal}`); + return retVal; + } + + async deleteSubscription(subscriptionId: CheckSubscriptionId): Promise { + logger.debug(`Deleted check subscription: ${subscriptionId}`); + return this.subscriptionsBySubscriptionId.delete(subscriptionId); + } + + // #endregion + + // #region Helper methods + + private async enableCheck(checkId: string, projectId: string): Promise { const checkRunner = await this.findCheckRunnerForCheckId(checkId); return checkRunner.enableCheck(checkId, projectId); } - async disableCheck(checkId: string, projectId?: string): Promise { + private async disableCheck(checkId: string, projectId?: string): Promise { const checkRunner = await this.findCheckRunnerForCheckId(checkId); return checkRunner.disableCheck(checkId, projectId); } @@ -121,7 +322,10 @@ class CheckDataProviderEngine this.checkRunners.set(checkRunnerDetails.id, checkRunner); // Make sure the new check runner has the correct ranges set - await checkRunner.setActiveRanges(undefined, this.lastRangesSet); + await checkRunner.setActiveRanges( + undefined, + aggregateRanges(this.subscriptionsBySubscriptionId), + ); // Pass along updated results from check runners const resultsUnsubscriber = await checkRunner.subscribeCheckResults( @@ -149,6 +353,13 @@ class CheckDataProviderEngine ); } + private findSubscription(subscriptionId: CheckSubscriptionId) { + // Find this subscription + const retVal = this.subscriptionsBySubscriptionId.get(subscriptionId); + if (!retVal) throw new Error(`Subscription ID not found: ${subscriptionId}`); + return retVal; + } + // Try to find the one check runner that is hosting the desired check ID private async findCheckRunnerForCheckId(checkId: string): Promise { let ranRefresh = false; @@ -190,6 +401,8 @@ class CheckDataProviderEngine return retVal; } + + // #endregion } /** @@ -207,7 +420,7 @@ async function initialize(): Promise { try { dataProvider = await papi.dataProviders.registerEngine( checkAggregatorServiceProviderName, - new CheckDataProviderEngine(), + new CheckAggregatorDataProviderEngine(), ); resolve(); } catch (error) { diff --git a/extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts b/extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts new file mode 100644 index 0000000000..23aa691567 --- /dev/null +++ b/extensions/src/platform-scripture/src/checks/check-aggregator.utils.test.ts @@ -0,0 +1,298 @@ +import { VerseRef } from '@sillsdev/scripture'; +import { + aggregateProjectIdsByCheckId, + aggregateRanges, + isWithinRange, +} from './check-aggregator.utils'; + +describe('aggregateRanges', () => { + it('should merge overlapping ranges', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 5, 1), end: new VerseRef(1, 9, 1), projectId: 'proj1' }, + ], + includeDeniedResults: false, + }, + ], + [ + 'sub2', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 2, 1), end: new VerseRef(1, 8, 1), projectId: 'proj1' }, + ], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateRanges(subscriptions); + // Using JSON.stringify here because the constructors for VerseRef set different internal fields + expect(JSON.stringify(result)).toEqual( + JSON.stringify([ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 9, 1), projectId: 'proj1' }, + ]), + ); + }); + + it('should merge overlapping ranges that lack ends', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 2, 1), projectId: 'proj1' }, + ], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateRanges(subscriptions); + // Using JSON.stringify here because the constructors for VerseRef set different internal fields + expect(JSON.stringify(result)).toEqual( + JSON.stringify([{ start: new VerseRef(1, 1, 1), projectId: 'proj1' }]), + ); + }); + + it('should not merge non-overlapping ranges', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 1, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 2, 1), end: new VerseRef(1, 2, 1), projectId: 'proj1' }, + ], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateRanges(subscriptions); + expect(result).toEqual([ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 1, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 2, 1), end: new VerseRef(1, 2, 1), projectId: 'proj1' }, + ]); + }); + + it('should not merge different projects', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 2, 1), end: new VerseRef(1, 4, 1), projectId: 'proj2' }, + ], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateRanges(subscriptions); + expect(result).toEqual([ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1), projectId: 'proj1' }, + { start: new VerseRef(1, 2, 1), end: new VerseRef(1, 4, 1), projectId: 'proj2' }, + ]); + }); + + it('should handle empty subscriptions', () => { + const subscriptions = new Map(); + + const result = aggregateRanges(subscriptions); + expect(result).toEqual([]); + }); + + it('should handle single range', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 1, 1), projectId: 'proj1' }, + ], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateRanges(subscriptions); + expect(result).toEqual([ + { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 1, 1), projectId: 'proj1' }, + ]); + }); +}); + +describe('isWithinRange', () => { + it('should return true for verse within range', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1) }; + const verseRef = new VerseRef(1, 2, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return false for verse outside range', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1) }; + const verseRef = new VerseRef(1, 4, 1); + expect(isWithinRange(range, verseRef)).toBe(false); + }); + + it('should return true for verse at the start of range', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1) }; + const verseRef = new VerseRef(1, 1, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return true for verse at the end of range', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 3, 1) }; + const verseRef = new VerseRef(1, 3, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return true for verse within range without end', () => { + const range = { start: new VerseRef(1, 1, 1) }; + const verseRef = new VerseRef(1, 2, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return false for verse outside range without end', () => { + const range = { start: new VerseRef(1, 1, 1) }; + const verseRef = new VerseRef(2, 1, 1); + expect(isWithinRange(range, verseRef)).toBe(false); + }); + + it('should return true for verse within range with different chapters', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 2, 1) }; + const verseRef = new VerseRef(1, 2, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return false for verse outside range with different chapters', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(1, 2, 1) }; + const verseRef = new VerseRef(1, 3, 1); + expect(isWithinRange(range, verseRef)).toBe(false); + }); + + it('should return true for verse within range with different books', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(2, 1, 1) }; + const verseRef = new VerseRef(2, 1, 1); + expect(isWithinRange(range, verseRef)).toBe(true); + }); + + it('should return false for verse outside range with different books', () => { + const range = { start: new VerseRef(1, 1, 1), end: new VerseRef(2, 1, 1) }; + const verseRef = new VerseRef(3, 1, 1); + expect(isWithinRange(range, verseRef)).toBe(false); + }); +}); + +describe('aggregateProjectIdsByCheckId', () => { + it('should aggregate project IDs by check ID', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map([ + ['check1', new Set(['proj1', 'proj2'])], + ['check2', new Set(['proj3'])], + ]), + ranges: [], + includeDeniedResults: false, + }, + ], + [ + 'sub2', + { + enabledProjectsByCheckId: new Map([ + ['check1', new Set(['proj2', 'proj4'])], + ['check3', new Set(['proj5'])], + ]), + ranges: [], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateProjectIdsByCheckId(subscriptions); + expect(result).toEqual( + new Map([ + ['check1', new Set(['proj1', 'proj2', 'proj4'])], + ['check2', new Set(['proj3'])], + ['check3', new Set(['proj5'])], + ]), + ); + }); + + it('should handle empty subscriptions', () => { + const subscriptions = new Map(); + + const result = aggregateProjectIdsByCheckId(subscriptions); + expect(result).toEqual(new Map()); + }); + + it('should handle subscriptions with no enabled projects', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map(), + ranges: [], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateProjectIdsByCheckId(subscriptions); + expect(result).toEqual(new Map()); + }); + + it('should handle single subscription with single check ID', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map([['check1', new Set(['proj1'])]]), + ranges: [], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateProjectIdsByCheckId(subscriptions); + expect(result).toEqual(new Map([['check1', new Set(['proj1'])]])); + }); + + it('should handle multiple subscriptions with overlapping check IDs', () => { + const subscriptions = new Map([ + [ + 'sub1', + { + enabledProjectsByCheckId: new Map([['check1', new Set(['proj1'])]]), + ranges: [], + includeDeniedResults: false, + }, + ], + [ + 'sub2', + { + enabledProjectsByCheckId: new Map([['check1', new Set(['proj2'])]]), + ranges: [], + includeDeniedResults: false, + }, + ], + ]); + + const result = aggregateProjectIdsByCheckId(subscriptions); + expect(result).toEqual(new Map([['check1', new Set(['proj1', 'proj2'])]])); + }); +}); diff --git a/extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts b/extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts new file mode 100644 index 0000000000..5f184b291a --- /dev/null +++ b/extensions/src/platform-scripture/src/checks/check-aggregator.utils.ts @@ -0,0 +1,96 @@ +import { VerseRef } from '@sillsdev/scripture'; +import { CheckInputRange, CheckSubscriptionId, ScriptureRange } from 'platform-scripture'; + +/* All these types and functions could live in `check-aggregator.service.ts`, but to get jest tests + * to run properly, it was much easier to pull these standalone functions into a different file and + * test them directly. + */ + +export type SubscriptionData = { + enabledProjectsByCheckId: Map>; + ranges: CheckInputRange[]; + includeDeniedResults: boolean; +}; + +export function aggregateRanges( + subscriptions: Map, +): CheckInputRange[] { + const retVal: CheckInputRange[] = []; + + // Collect all ranges from all subscriptions + const allRanges: CheckInputRange[] = []; + subscriptions.forEach((subscription) => allRanges.push(...subscription.ranges)); + + // Sort ranges so any overlapping ranges will be adjacent to each other + allRanges.sort((a, b) => { + if (a.projectId < b.projectId) return -1; + if (a.projectId > b.projectId) return 1; + return a.start.BBBCCC - b.start.BBBCCC; + }); + + // Merge overlapping ranges + if (allRanges.length > 0) retVal.push(allRanges[0]); + for (let i = 1; i < allRanges.length; i++) { + const range = retVal[retVal.length - 1]; + const nextRange = allRanges[i]; + if (rangesAreOverlapping(range, nextRange)) + retVal[retVal.length - 1] = mergeRanges(range, nextRange); + else retVal.push(nextRange); + } + + return retVal; +} + +export function isWithinRange(range: ScriptureRange, verseRef: VerseRef): boolean { + const startBookNum = range.start.bookNum; + const endBookNum = range.end?.bookNum ?? startBookNum; + if (verseRef.bookNum < startBookNum || verseRef.bookNum > endBookNum) return false; + + const startChapterNum = range.start.chapterNum; + const endChapterNum = range.end?.chapterNum ?? 999; + if (verseRef.chapterNum < startChapterNum || verseRef.chapterNum > endChapterNum) return false; + + // Don't worry about verse numbers for now since the UI only works at the chapter level + return true; +} + +export function aggregateProjectIdsByCheckId( + subscriptions: Map, +): Map> { + const retVal = new Map>(); + subscriptions.forEach((subscription) => { + subscription.enabledProjectsByCheckId.forEach((projects, checkId) => { + const aggregatedProjects = retVal.get(checkId); + if (!aggregatedProjects) retVal.set(checkId, new Set(projects)); + else projects.forEach((project) => aggregatedProjects.add(project)); + }); + }); + return retVal; +} + +function rangesAreOverlapping(existingRange: CheckInputRange, newRange: CheckInputRange) { + // Don't worry about verse numbers for now since the UI only works at the chapter level + const existingStart = existingRange.start.BBBCCC; + const existingEnd = existingRange.end?.BBBCCC ?? existingRange.start.bookNum * 1000 + 999; + const newStart = newRange.start.BBBCCC; + const newEnd = newRange.end?.BBBCCC ?? newRange.start.bookNum * 1000 + 999; + return ( + newRange.projectId === existingRange.projectId && + ((newStart >= existingStart && newStart <= existingEnd) || + (newEnd >= existingStart && newEnd <= existingEnd) || + (newStart <= existingStart && newEnd >= existingEnd)) + ); +} + +function mergeRanges(existingRange: CheckInputRange, newRange: CheckInputRange): CheckInputRange { + const start = + newRange.start.BBBCCC < existingRange.start.BBBCCC ? newRange.start : existingRange.start; + let end: VerseRef | undefined; + if (existingRange.end && newRange.end) + end = newRange.end.BBBCCC > existingRange.end.BBBCCC ? newRange.end : existingRange.end; + return { + start: new VerseRef(start), + end: end ? new VerseRef(end) : undefined, + projectId: existingRange.projectId, + }; +} diff --git a/extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx b/extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx index 6b08a5823f..d5666e91da 100644 --- a/extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx +++ b/extensions/src/platform-scripture/src/checks/configure-checks/configure-checks.component.tsx @@ -24,7 +24,6 @@ type ConfigureChecksProps = { projectId: string | undefined; availableChecks: CheckRunnerCheckDetails[]; handleSelectCheck: (checkLabel: string, selected: boolean) => void; - selectedChecks: string[]; activeRanges: CheckInputRange[]; handleActiveRangesChange: (newActiveRanges: CheckInputRange[]) => void; }; @@ -33,7 +32,6 @@ export default function ConfigureChecks({ projectId, availableChecks, handleSelectCheck, - selectedChecks, activeRanges, handleActiveRangesChange, }: ConfigureChecksProps) { @@ -164,7 +162,13 @@ export default function ConfigureChecks({ check.checkDescription)} - selectedListItems={selectedChecks} + selectedListItems={ + projectId + ? availableChecks + .filter((check) => check.enabledProjectIds.includes(projectId)) + .map((check) => check.checkDescription) + : [] + } handleSelectListItem={handleSelectCheck} /> diff --git a/extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts b/extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts index 3d5569ea02..99811ded64 100644 --- a/extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts +++ b/extensions/src/platform-scripture/src/checks/extension-host-check-runner.service.ts @@ -2,6 +2,7 @@ import papi, { DataProviderEngine, dataProviders, logger } from '@papi/backend'; import { Mutex, MutexMap, UnsubscriberAsync, UnsubscriberAsyncList } from 'platform-bible-utils'; import { DataProviderUpdateInstructions, + ExtensionDataScope, IDataProviderEngine, IDisposableDataProvider, } from '@papi/core'; @@ -10,16 +11,19 @@ import { CheckCreatorFunction, CheckDetails, CheckDetailsWithCheckId, + CheckEnablerDisabler, CheckInputRange, + CheckResultClassifier, CheckRunResult, CheckRunnerCheckDetails, CheckRunnerDataTypes, ICheckHostingService, ICheckRunner, - IUSFMBookProjectDataProvider, } from 'platform-scripture'; import { VerseRef } from '@sillsdev/scripture'; +import type { ProjectDataProviderInterfaces } from 'papi-shared-types'; import { CHECK_RUNNER_NETWORK_OBJECT_TYPE } from './check.model'; +import { PersistedCheckRunResults } from './persisted-check-run-result.model'; /** Details about a check that include a way to create the check */ type CheckDetailsWithCreator = CheckDetailsWithCheckId & { @@ -36,14 +40,25 @@ type DetailsAboutCheckInProject = { latestResults: CheckRunResult[]; }; +type CheckRunnerPdps = { + usfmBookPdp: ProjectDataProviderInterfaces['platformScripture.USFM_Book']; + basePdp: ProjectDataProviderInterfaces['platform.base']; +}; + +const deniedDataScope: ExtensionDataScope = { + extensionName: 'platform-scripture', + dataQualifier: 'deniedResultsList', +}; + class CheckRunnerEngine extends DataProviderEngine - implements IDataProviderEngine + implements IDataProviderEngine, CheckEnablerDisabler, CheckResultClassifier { - activeRanges: CheckInputRange[] = []; - mutexesPerCheck = new MutexMap(); - enabledChecksByProjectId = new Map(); - subscribedProjects = new Map(); + private activeRanges: CheckInputRange[] = []; + private mutexesPerCheck = new MutexMap(); + private enabledChecksByProjectId = new Map(); + private subscribedProjects = new Map(); + private deniedCheckResultsByProjectId = new Map(); // #region Checks @@ -77,9 +92,8 @@ class CheckRunnerEngine throw new Error('setAvailableChecks disabled - use enableCheck and disableCheck'); } - async enableCheck(checkId: string, projectId: string): Promise { - const lock = this.mutexesPerCheck.get(checkId); - const retVal = await lock.runExclusive(async () => { + async enableCheck(checkId: string, projectId: string): Promise { + await this.mutexesPerCheck.get(checkId).runExclusive(async () => { let checksForProject = this.enabledChecksByProjectId.get(projectId); // If the check is already enabled for this project, there is nothing to do if ( @@ -91,7 +105,7 @@ class CheckRunnerEngine logger.debug( `Re-enabling check ${checkId} for project ${projectId} that was already enabled`, ); - return []; + return; } // Make sure we know about the check @@ -124,12 +138,17 @@ class CheckRunnerEngine // Automatically rebuild results when project data changes if (!this.subscribedProjects.has(projectId)) { // This is assuming that whenever project data changes, this PDP will see an update. - const pdp = await papi.projectDataProviders.get('platformScripture.USFM_Book', projectId); - // We should check a second time since we `await`ed on the previous line + const usfmBookPdp = await papi.projectDataProviders.get( + 'platformScripture.USFM_Book', + projectId, + ); + const basePdp = await papi.projectDataProviders.get('platform.base', projectId); + const pdps: CheckRunnerPdps = { usfmBookPdp, basePdp }; + // We should check a second time since we `await`ed a few lines back if (!this.subscribedProjects.has(projectId)) { - this.subscribedProjects.set(projectId, pdp); + this.subscribedProjects.set(projectId, pdps); // We just need something to tell us when project data changes - await pdp.subscribeBookUSFM( + await usfmBookPdp.subscribeBookUSFM( new VerseRef(1, 1, 1), async () => { // Ideally we'd want to check if the text that changed is inside of an active range @@ -145,7 +164,7 @@ class CheckRunnerEngine { retrieveDataImmediately: false, whichUpdates: '*' }, ); - pdp.onDidDispose(() => { + usfmBookPdp.onDidDispose(() => { this.enabledChecksByProjectId.get(projectId)?.forEach((enabledCheckDetails) => { this.disableCheck(enabledCheckDetails.checkId, projectId); }); @@ -155,10 +174,10 @@ class CheckRunnerEngine } } - return warnings; + if (warnings.length > 0) + logger.warn(`Warnings when enabling check ${checkId}: ${JSON.stringify(warnings)}`); }); await this.rebuildResults(projectId); - return retVal; } async disableCheck(checkId: string, projectId?: string): Promise { @@ -210,6 +229,44 @@ class CheckRunnerEngine // #region Results + async denyCheckResult( + _checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ): Promise { + const deniedResults = await this.loadDeniedResults(projectId); + const retVal = deniedResults.addResult({ + checkResultType, + verseRef, + selectedText, + checkResultUniqueId, + }); + if (retVal) await this.saveDeniedResults(projectId); + return retVal; + } + + async allowCheckResult( + _checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ): Promise { + const deniedResults = await this.loadDeniedResults(projectId); + const retVal = deniedResults.removeResult({ + checkResultType, + verseRef, + selectedText, + checkResultUniqueId, + }); + if (retVal) await this.saveDeniedResults(projectId); + return retVal; + } + async getCheckResults(): Promise { const retVal: CheckRunResult[] = []; this.enabledChecksByProjectId.forEach((checkDetails) => { @@ -217,7 +274,13 @@ class CheckRunnerEngine retVal.push(...oneCheckDetails.latestResults); }); }); - return retVal; + return Promise.all( + retVal.map(async (checkRunResult) => { + const deniedResults = await this.loadDeniedResults(checkRunResult.projectId); + checkRunResult.isDenied = deniedResults.containsResult(checkRunResult); + return checkRunResult; + }), + ); } // Because this is a data provider, we have to provide this method even though it always throws @@ -226,6 +289,24 @@ class CheckRunnerEngine throw new Error('setCheckResults disabled'); } + async loadDeniedResults(projectId: string): Promise { + let deniedResults = this.deniedCheckResultsByProjectId.get(projectId); + if (!deniedResults) { + deniedResults = new PersistedCheckRunResults( + await this.subscribedProjects.get(projectId)?.basePdp.getExtensionData(deniedDataScope), + ); + this.deniedCheckResultsByProjectId.set(projectId, deniedResults); + } + return deniedResults; + } + + async saveDeniedResults(projectId: string): Promise { + const deniedResults = this.deniedCheckResultsByProjectId.get(projectId); + if (!deniedResults) return; + const data = deniedResults.serialize(); + await this.subscribedProjects.get(projectId)?.basePdp.setExtensionData(deniedDataScope, data); + } + async rebuildResults(projectId?: string): Promise { // First dimension of array is by project ID. Second dimension is by check for the project. const twoDimArray = this.activeRanges.map((checkRange) => { diff --git a/extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts b/extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts new file mode 100644 index 0000000000..e1db908dfb --- /dev/null +++ b/extensions/src/platform-scripture/src/checks/persisted-check-run-result.model.ts @@ -0,0 +1,124 @@ +import { VerseRef } from '@sillsdev/scripture'; +import { deserialize, serialize } from 'platform-bible-utils'; +import { CheckRunResult } from 'platform-scripture'; + +export type PersistedCheckRunResult = { + /** Type of check result */ + checkResultType: string; + /** Single verse reference identifying where the check result occurred */ + verseRef: VerseRef; + /** Project text that was selected in the check result */ + selectedText: string; + /** Distinct ID for this check result if it might occur more than once in a single verse */ + checkResultUniqueId?: string; +}; + +export class PersistedCheckRunResults { + /** Map of book number to chapter number to array of PersistedCheckRunResult */ + private results: Record> = {}; + + /** + * @param serializedResults Return value of a previous call to `serialize` on a + * `PersistedCheckRunResults` object + */ + constructor(serializedResults?: string) { + if (serializedResults) this.results = deserialize(serializedResults); + } + + /** + * Serialize the contents of this object to a string + * + * @returns JSON string representing this object + */ + serialize(): string { + return serialize(this.results); + } + + /** + * Adds a PersistedCheckRunResult to the set + * + * @param result Item to add to the set + * @returns `true` if the item did not already exist and was added to the set, `false` if the item + * already existed in the set + */ + addResult(result: PersistedCheckRunResult): boolean { + if (this.containsResult(result)) return false; + const { bookNum, chapterNum } = result.verseRef; + if (!this.results[bookNum]) this.results[bookNum] = {}; + if (!this.results[bookNum][chapterNum]) this.results[bookNum][chapterNum] = []; + this.results[bookNum][chapterNum].push(result); + return true; + } + + /** + * Removes a PersistedCheckRunResult from the set + * + * @param result Item to remove from the set + * @returns `true` if the item was removed from the set, `false` otherwise + */ + removeResult(result: PersistedCheckRunResult): boolean { + const { bookNum, chapterNum } = result.verseRef; + const chapterResults = this.results[bookNum]?.[chapterNum]; + if (!chapterResults) return false; + + const index = chapterResults.findIndex((existingResult) => { + return ( + existingResult.checkResultType === result.checkResultType && + existingResult.verseRef.verse === result.verseRef.verse && + existingResult.selectedText === result.selectedText && + existingResult.checkResultUniqueId === result.checkResultUniqueId + ); + }); + if (index === -1) return false; + + chapterResults.splice(index, 1); + if (chapterResults.length === 0) delete this.results[bookNum][chapterNum]; + if (Object.keys(this.results[bookNum]).length === 0) delete this.results[bookNum]; + + return true; + } + + /** + * Check if the provided item is already represented in the existing set of results + * + * @param result Item to query for existence in the set + * @returns `true` if the result already exists in the set, `false` if it does not already exist + */ + containsResult(result: CheckRunResult | PersistedCheckRunResult): boolean { + const possibleMatches = this.getPersistedCheckRunResults( + result.verseRef.bookNum, + result.verseRef.chapterNum, + ); + + return !!possibleMatches.find((possibleMatch) => { + return ( + possibleMatch.checkResultType === result.checkResultType && + possibleMatch.verseRef.verse === result.verseRef.verse && + possibleMatch.selectedText === result.selectedText && + possibleMatch.checkResultUniqueId === result.checkResultUniqueId + ); + }); + } + + /** + * Get all persisted check run results as a single array + * + * @returns Array of all PersistedCheckRunResult objects + */ + getAllPersistedCheckRunResults(): PersistedCheckRunResult[] { + const allResults: PersistedCheckRunResult[] = []; + Object.values(this.results).forEach((chapters) => { + Object.values(chapters).forEach((results) => { + allResults.push(...results); + }); + }); + return allResults; + } + + private getPersistedCheckRunResults( + bookNum: number, + chapterNum: number, + ): PersistedCheckRunResult[] { + return this.results[bookNum]?.[chapterNum] || []; + } +} diff --git a/extensions/src/platform-scripture/src/configure-checks.web-view.tsx b/extensions/src/platform-scripture/src/configure-checks.web-view.tsx index ef02982241..528519c68b 100644 --- a/extensions/src/platform-scripture/src/configure-checks.web-view.tsx +++ b/extensions/src/platform-scripture/src/configure-checks.web-view.tsx @@ -1,9 +1,15 @@ import { WebViewProps } from '@papi/core'; import { useData, useDataProvider } from '@papi/frontend/react'; -import { CheckInputRange, CheckRunnerCheckDetails } from 'platform-scripture'; +import { + CheckInputRange, + CheckRunnerCheckDetails, + CheckSubscriptionId, + SettableCheckDetails, +} from 'platform-scripture'; import { Canon, VerseRef } from '@sillsdev/scripture'; -import { useCallback, useEffect, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { Sonner, sonner } from 'platform-bible-react'; +import { logger } from '@papi/frontend'; import ConfigureChecks from './checks/configure-checks/configure-checks.component'; const defaultCheckRunnerCheckDetails: CheckRunnerCheckDetails = { @@ -32,19 +38,34 @@ const prettyPrintVerseRef = (verseRef: VerseRef): string => { }; global.webViewComponent = function ConfigureChecksWebView({ projectId }: WebViewProps) { - const checkRunner = useDataProvider('platformScripture.checkAggregator'); + const checkAggregator = useDataProvider('platformScripture.checkAggregator'); - const [availableChecks, , availableChecksIsLoading] = useData( - 'platformScripture.checkAggregator', - ).AvailableChecks(undefined, [defaultCheckRunnerCheckDetails]); + // Obtain a subscription ID and cleanup old subscriptions + const [subscriptionId, setSubscriptionId] = useState(''); + const subscriptionIdRef = useRef(''); + useEffect(() => { + const fetchSubscriptionId = async () => { + const subId = await checkAggregator?.createSubscription(); + if (subId) { + logger.debug(`Created check subscription: ${subId}`); + setSubscriptionId(subId); + subscriptionIdRef.current = subId; + } + }; - const selectedChecks = useMemo((): string[] => { - if (!projectId) return []; - const enabledChecks = availableChecks.filter((check) => - check.enabledProjectIds.includes(projectId), - ); - return enabledChecks.map((check) => check.checkDescription); - }, [availableChecks, projectId]); + fetchSubscriptionId(); + + return () => { + if (subscriptionIdRef.current) { + checkAggregator?.deleteSubscription(subscriptionIdRef.current); + logger.debug(`Deleted check subscription: ${subscriptionIdRef.current}`); + } + }; + }, [checkAggregator]); + + const [availableChecks, setAvailableChecks, availableChecksIsLoading] = useData( + 'platformScripture.checkAggregator', + ).AvailableChecks(subscriptionId, [defaultCheckRunnerCheckDetails]); const defaultScriptureRange: CheckInputRange = useMemo(() => { return { @@ -54,13 +75,13 @@ global.webViewComponent = function ConfigureChecksWebView({ projectId }: WebView }, [projectId]); const [activeRanges, setActiveRanges] = useData('platformScripture.checkAggregator').ActiveRanges( - undefined, + subscriptionId, useMemo(() => [defaultScriptureRange], [defaultScriptureRange]), ); const updateSelectedChecks = useCallback( async (checkDescription: string, selected: boolean) => { - if (!checkRunner || !projectId) return; + if (!setAvailableChecks || !projectId) return; if (availableChecksIsLoading) { sonner.warning(`${selected ? 'Enabling' : 'Disabling'} check failed.`, { description: 'Please try again later.', @@ -69,26 +90,23 @@ global.webViewComponent = function ConfigureChecksWebView({ projectId }: WebView const checkId = findCheckIdFromDescription(availableChecks, checkDescription); if (!checkId) throw new Error(`No available check found with checkLabel ${checkDescription}`); - if (selected) { - const newCheckFeedback = await checkRunner.enableCheck(checkId, projectId); - if (newCheckFeedback && newCheckFeedback.length > 0) { - sonner.warning( - `Warnings/errors occurred when trying to enable the ${checkDescription} check. - Enabling may or may not have been successful. - The following warning/errors have been encountered:`, - { - description: `${newCheckFeedback.map((feedback) => ` ${feedback}`)}`, - }, - ); - } else { - sonner.success(`Successfully enabled ${checkDescription} check`); - } - } else { - checkRunner.disableCheck(checkId, projectId); - sonner.success(`Successfully disabled ${checkDescription} check`); - } + const checksToSet = availableChecks.map( + (checkDetails: CheckRunnerCheckDetails): SettableCheckDetails => { + const retVal = { + checkId: checkDetails.checkId, + enabledProjectIds: [...checkDetails.enabledProjectIds], + }; + if (checkId !== retVal.checkId) return retVal; + if (selected) retVal.enabledProjectIds.push(projectId); + else retVal.enabledProjectIds = retVal.enabledProjectIds.filter((id) => id !== projectId); + return retVal; + }, + ); + await setAvailableChecks(checksToSet); + + sonner.success(`Successfully ${selected ? 'enabled' : 'disabled'} ${checkDescription} check`); }, - [availableChecks, availableChecksIsLoading, checkRunner, projectId], + [availableChecks, availableChecksIsLoading, projectId, setAvailableChecks], ); const updateActiveRanges = useCallback( @@ -115,7 +133,6 @@ global.webViewComponent = function ConfigureChecksWebView({ projectId }: WebView projectId={projectId} availableChecks={availableChecks} handleSelectCheck={updateSelectedChecks} - selectedChecks={selectedChecks} activeRanges={activeRanges} handleActiveRangesChange={updateActiveRanges} /> diff --git a/extensions/src/platform-scripture/src/types/platform-scripture.d.ts b/extensions/src/platform-scripture/src/types/platform-scripture.d.ts index 81942e0210..3b72b135dc 100644 --- a/extensions/src/platform-scripture/src/types/platform-scripture.d.ts +++ b/extensions/src/platform-scripture/src/types/platform-scripture.d.ts @@ -495,7 +495,7 @@ declare module 'platform-scripture' { // #endregion - // #region Check Data Types + // #region Check Types /** Details about a check provided by the check itself */ export type CheckDetails = { @@ -589,8 +589,14 @@ declare module 'platform-scripture' { export type CheckRunResult = { /** ID of the check that produced this result */ checkId?: string; + /** Identifies a distinct class of check results that a check produced */ + checkResultType: string; + /** Distinct ID for this check result if it might occur more than once in a single verse */ + checkResultUniqueId?: string; /** ID of the project evaluated by the check */ projectId: string; + /** Project text that was selected in the check result */ + selectedText: string; /** * Format string or {@link LocalizeKey} of the format string to display regarding the range of * text referenced in this result. A format string should be of the form "... {arg1} ... {arg2} @@ -609,6 +615,10 @@ declare module 'platform-scripture' { * @example {name: Layla} */ formatStringArguments?: { [key: string]: string }; + /** Indicates if a user decided this check result should be considered incorrect going forward */ + isDenied: boolean; + /** VerseRef that most closely identifies a single place where the check applies */ + verseRef: VerseRef; /** Starting point where the check result applies in the document */ start: CheckLocation; /** Ending point where the check result applies in the document */ @@ -617,7 +627,7 @@ declare module 'platform-scripture' { // #endregion - // #region Check Runner Data Types + // #region Check Runner Types /** Details about a check provided by a {@link ICheckRunner} */ export type CheckRunnerCheckDetails = CheckDetailsWithCheckId & { @@ -625,6 +635,15 @@ declare module 'platform-scripture' { enabledProjectIds: string[]; }; + /** + * Details about a check (as identified by its checkId) that can be set on a subscription by the + * subscription owner + */ + export type SettableCheckDetails = Omit< + CheckRunnerCheckDetails, + 'checkName' | 'checkDescription' + >; + /** Data types provided by a service that runs checks */ export type CheckRunnerDataTypes = { AvailableChecks: DataProviderDataType; @@ -632,28 +651,49 @@ declare module 'platform-scripture' { CheckResults: DataProviderDataType; }; + export type CheckEnablerDisabler = { + /** Enable the check with the given checkId to run on the given project */ + enableCheck: (checkId: string, projectId: string) => Promise; + + /** Disable the check with the given checkId from producing results for the given project */ + disableCheck: (checkId: string, projectId?: string) => Promise; + }; + + export type CheckResultClassifier = { + /** + * Mark one particular check result as "denied", meaning the user has marked it as incorrect for + * the given text + */ + denyCheckResult: ( + checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ) => Promise; + /** Reverse the denial of one particular check result */ + allowCheckResult: ( + checkId: string, + checkResultType: string, + projectId: string, + verseRef: VerseRef, + selectedText: string, + checkResultUniqueId?: string, + ) => Promise; + }; + /** * All processes that can run checks are expected to implement this type in a data provider * registered with object type 'checkRunner' */ - export type ICheckRunner = IDataProvider & { - /** - * Enable the check with the given checkId to run on the given project. Note that no results - * will be returned unless `getCheckResults` is run before or after the check has been enabled. - * `getCheckResults` is used to indicate what project content should be checked within each - * project. `enableCheck` is used to indicate which checks should be enabled on which projects. - * - * @returns Warnings from the check that indicate results might not be what the user desires - */ - enableCheck: (checkId: string, projectId: string) => Promise; - - /** Disable the check with the given checkId from producing results for the given project. */ - disableCheck: (checkId: string, projectId?: string) => Promise; - }; + export type ICheckRunner = IDataProvider & + CheckEnablerDisabler & + CheckResultClassifier; // #endregion - // #region Check Hosting (in the Extension Host) Data Types + // #region Check Hosting (in the Extension Host) Types /** * Service for hosting TS/JS checks inside the extension host that are registered using the @@ -677,7 +717,37 @@ declare module 'platform-scripture' { // #endregion - // #region Check Aggregator Data Types + // #region Check Aggregator Types + + /** Uniquely identifies one subscriber to the check service */ + export type CheckSubscriptionId = string; + + export type CheckSubscriptionManager = { + /** Create a new subscription keyed by the returned subscription ID */ + createSubscription: () => Promise; + + /** + * Deactivate and throw away the subscription with the given ID + * + * @returns `true` if the subscription could be deleted, `false` otherwise + */ + deleteSubscription: (subscriptionId: CheckSubscriptionId) => Promise; + }; + + /** + * Data types provided by a service that aggregates check results for multiple callers across + * multiple ICheckRunner instances + */ + export type CheckAggregatorDataTypes = { + AvailableChecks: DataProviderDataType< + CheckSubscriptionId, + CheckRunnerCheckDetails[], + SettableCheckDetails[] + >; + ActiveRanges: DataProviderDataType; + IncludeDeniedResults: DataProviderDataType; + CheckResults: DataProviderDataType; + }; /** * Service that multiplexes/demultiplexes calls across all {@link ICheckRunner} data providers so @@ -686,9 +756,11 @@ declare module 'platform-scripture' { * * Use the "platformScripture.checkAggregator" data provider name to access the service. */ - export type ICheckAggregatorService = ICheckRunner & { - dataProviderName: string; - }; + export type ICheckAggregatorService = IDataProvider & + CheckResultClassifier & + CheckSubscriptionManager & { + dataProviderName: string; + }; // #endregion } @@ -731,7 +803,11 @@ declare module 'papi-shared-types' { export interface DataProviders { /** Use this to work with checks that are running in any process */ 'platformScripture.checkAggregator': ICheckAggregatorService; - /** Use this to work with checks that are explicitly hosted in the extension host */ + /** + * You should probably use 'platformScripture.checkAggregator' instead. This data provider only + * includes checks registered with this one particular {@link ICheckRunner}. The aggregator + * includes all checks registered with all {@link ICheckRunner} instances. + */ 'platformScripture.extensionHostCheckRunner': ICheckRunner; } diff --git a/extensions/src/platform-scripture/tsconfig.json b/extensions/src/platform-scripture/tsconfig.json index 29f4025b9c..504a21458b 100644 --- a/extensions/src/platform-scripture/tsconfig.json +++ b/extensions/src/platform-scripture/tsconfig.json @@ -23,7 +23,7 @@ "jsx": "react-jsx", "typeRoots": [ // Include default type declarations - "node_modules/@types", + "../../../node_modules/@types", // Include papi-dts type declarations (for papi.d.ts) "../../../lib", // Include core extensions' type declarations diff --git a/package-lock.json b/package-lock.json index 5e3544df43..6d157045b9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -688,6 +688,7 @@ "@biblionexus-foundation/scripture-utilities": "^0.0.4", "@swc/core": "^1.7.35", "@tailwindcss/typography": "^0.5.15", + "@types/jest": "^29.5.14", "@types/node": "^20.16.11", "@types/react": "^18.3.11", "@types/react-dom": "^18.3.1", @@ -714,6 +715,7 @@ "eslint-plugin-react": "^7.37.1", "eslint-plugin-react-hooks": "^4.6.2", "glob": "^10.4.5", + "jest": "^29.7.0", "lucide-react": "^0.452.0", "papi-dts": "file:../../../lib/papi-dts", "platform-bible-react": "file:../../../lib/platform-bible-react", @@ -730,6 +732,7 @@ "swc-loader": "^0.2.6", "tailwindcss": "^3.4.13", "tailwindcss-animate": "^1.0.7", + "ts-jest": "^29.2.5", "ts-node": "^10.9.2", "tsconfig-paths": "^4.2.0", "tsconfig-paths-webpack-plugin": "^4.1.0", @@ -12235,9 +12238,9 @@ } }, "node_modules/@types/jest": { - "version": "29.5.13", - "resolved": "https://registry.npmjs.org/@types/jest/-/jest-29.5.13.tgz", - "integrity": "sha512-wd+MVEZCHt23V0/L642O5APvspWply/rGY5BcW4SUETo2UzPU3Z26qr8jC2qxpimI2jjx9h7+2cj2FwIr01bXg==", + "version": "29.5.14", + "resolved": "https://registry.npmjs.org/@types/jest/-/jest-29.5.14.tgz", + "integrity": "sha512-ZN+4sdnLUbo8EVvVc2ao0GFW6oVrQRPn4K2lglySj7APvSrgzxHiNNK99us4WDMi57xxA2yggblIAMNhXOotLQ==", "dev": true, "dependencies": { "expect": "^29.0.0", @@ -39698,9 +39701,9 @@ } }, "@types/jest": { - "version": "29.5.13", - "resolved": "https://registry.npmjs.org/@types/jest/-/jest-29.5.13.tgz", - "integrity": "sha512-wd+MVEZCHt23V0/L642O5APvspWply/rGY5BcW4SUETo2UzPU3Z26qr8jC2qxpimI2jjx9h7+2cj2FwIr01bXg==", + "version": "29.5.14", + "resolved": "https://registry.npmjs.org/@types/jest/-/jest-29.5.14.tgz", + "integrity": "sha512-ZN+4sdnLUbo8EVvVc2ao0GFW6oVrQRPn4K2lglySj7APvSrgzxHiNNK99us4WDMi57xxA2yggblIAMNhXOotLQ==", "dev": true, "requires": { "expect": "^29.0.0", @@ -49993,6 +49996,7 @@ "@sillsdev/scripture": "^2.0.2", "@swc/core": "^1.7.35", "@tailwindcss/typography": "^0.5.15", + "@types/jest": "^29.5.14", "@types/node": "^20.16.11", "@types/react": "^18.3.11", "@types/react-dom": "^18.3.1", @@ -50019,6 +50023,7 @@ "eslint-plugin-react": "^7.37.1", "eslint-plugin-react-hooks": "^4.6.2", "glob": "^10.4.5", + "jest": "^29.7.0", "lucide-react": "^0.452.0", "papi-dts": "file:../../../lib/papi-dts", "platform-bible-react": "file:../../../lib/platform-bible-react", @@ -50036,6 +50041,7 @@ "swc-loader": "^0.2.6", "tailwindcss": "^3.4.13", "tailwindcss-animate": "^1.0.7", + "ts-jest": "^29.2.5", "ts-node": "^10.9.2", "tsconfig-paths": "^4.2.0", "tsconfig-paths-webpack-plugin": "^4.1.0",