Make Sense.PartOfSpeech an object instead of a string (#1350)

* Backend changes to make PartOfSpeech an object
* Mark FwLiteProjectSync tests as integration tests

These tests run a Send/Receive as part of the test and are too slow to
be considered unit tests.

* Fix a couple of tests broken by PoS type change
* Fix failing integration tests
* Fix up part of speech creation in AutoFaker
* don't store PartOfSpeech objects in CreateSenseChange
* load part of speech when loading senses
* ensure EntryReadyForCreation always creates a part of speech when either PartOfSpeechId or PartOfSpeech are defined
* fix UpdateSensePartOfSpeech as now `PartOfSpeech` gets used when creating a sense
* When part of speech GUID not found, should throw
* Change to frontend generated type for PartOfSpeech

Auto-generated types for frontend have picked up the change of
Sense.PartOfSpeech from a string to an object, so now it's time
to change the frontend code accordingly.

* Update entry-data.ts and utils.ts to use IPartOfSpeech

More UI changes will ripple out from here

* Update test sena-3 data with new partOfSpeech objects
* Add other parts of speech found in Sena-3 test data

Having the complete list ensures that the Sena-3 test project's grammar
picker, and the display of the parts of speech, will work.

* VerifyDbModel test now expects PartOfSpeech object
* Can now validate PartOfSpeech GUIDs safely
* handle deleted semantic domains and part of speech in CreateSenseChange
* Add FwLite DB migration for part of speech object

---------

Co-authored-by: Kevin Hahn <[email protected]>

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs

@@ -568,14 +568,15 @@ private ComplexFormComponent ToSenseReference(ILexSense componentSense, ILexEntr
     private Sense FromLexSense(ILexSense sense)
     {
         var enWs = GetWritingSystemHandle("en");
+        var pos = sense.MorphoSyntaxAnalysisRA?.GetPartOfSpeech();
         var s =  new Sense
         {
             Id = sense.Guid,
             EntryId = sense.Entry.Guid,
             Gloss = FromLcmMultiString(sense.Gloss),
             Definition = FromLcmMultiString(sense.Definition),
-            PartOfSpeech = sense.MorphoSyntaxAnalysisRA?.GetPartOfSpeech()?.Name.get_String(enWs).Text ?? "",
-            PartOfSpeechId = sense.MorphoSyntaxAnalysisRA?.GetPartOfSpeech()?.Guid,
+            PartOfSpeech = pos is null ? null : FromLcmPartOfSpeech(pos),
+            PartOfSpeechId = pos?.Guid,
             SemanticDomains = sense.SemanticDomainsRC.Select(FromLcmSemanticDomain).ToList(),
             ExampleSentences = sense.ExamplesOS.Select(sentence => FromLexExampleSentence(sense.Guid, sentence)).ToList()
         };
@@ -887,8 +888,10 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense, BetweenPosition? betw
         var lexSense = LexSenseFactory.Create(sense.Id);
         InsertSense(lexEntry, lexSense, between);
         var msa = new SandboxGenericMSA() { MsaType = lexSense.GetDesiredMsaType() };
-        if (sense.PartOfSpeechId.HasValue && PartOfSpeechRepository.TryGetObject(sense.PartOfSpeechId.Value, out var pos))
+        if (sense.PartOfSpeechId.HasValue)
         {
+            var found = PartOfSpeechRepository.TryGetObject(sense.PartOfSpeechId.Value, out var pos);
+            if (!found) throw new InvalidOperationException($"Part of speech must exist when creating a sense (could not find GUID {sense.PartOfSpeechId.Value})");
             msa.MainPOS = pos;
         }
         lexSense.SandboxMSA = msa;

backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.cs

@@ -25,7 +25,7 @@ public override MultiString Gloss
         set => throw new NotImplementedException();
     }
 
-    public override string PartOfSpeech
+    public override PartOfSpeech? PartOfSpeech
     {
         get => throw new NotImplementedException();
         set { }

backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs

@@ -77,6 +77,7 @@ private async Task WorkaroundMissingWritingSystems()
     }
 
     [Fact]
+    [Trait("Category", "Integration")]
     public async Task DryRunImport_MakesNoChanges()
     {
         await WorkaroundMissingWritingSystems();
@@ -87,6 +88,7 @@ public async Task DryRunImport_MakesNoChanges()
     }
 
     [Fact]
+    [Trait("Category", "Integration")]
     public async Task DryRunImport_MakesTheSameChangesAsImport()
     {
         var dryRunSyncResult = await _syncService.SyncDryRun(_crdtApi, _fwDataApi);
@@ -95,6 +97,7 @@ public async Task DryRunImport_MakesTheSameChangesAsImport()
     }
 
     [Fact]
+    [Trait("Category", "Integration")]
     public async Task DryRunSync_MakesNoChanges()
     {
         await BypassImport();
@@ -107,6 +110,7 @@ public async Task DryRunSync_MakesNoChanges()
 
     [Fact]
     [Trait("Category", "Slow")]
+    [Trait("Category", "Integration")]
     public async Task DryRunSync_MakesTheSameChangesAsSync()
     {
         //syncing requires querying entries, which fails if there are no writing systems, so we import those first
@@ -121,6 +125,7 @@ public async Task DryRunSync_MakesTheSameChangesAsSync()
     }
 
     [Fact]
+    [Trait("Category", "Integration")]
     public async Task FirstSena3SyncJustDoesAnSync()
     {
         _fwDataApi.EntryCount.Should().BeGreaterThan(1000,
@@ -138,6 +143,7 @@ public async Task FirstSena3SyncJustDoesAnSync()
 
     [Fact]
     [Trait("Category", "Slow")]
+    [Trait("Category", "Integration")]
     public async Task SyncWithoutImport_CrdtShouldMatchFwdata()
     {
         await BypassImport();
@@ -153,6 +159,7 @@ public async Task SyncWithoutImport_CrdtShouldMatchFwdata()
     }
 
     [Fact]
+    [Trait("Category", "Integration")]
     public async Task SecondSena3SyncDoesNothing()
     {
         await _syncService.Sync(_crdtApi, _fwDataApi);

backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs

@@ -301,6 +301,14 @@ public async Task PartsOfSpeechSyncInEntries()
         await fwdataApi.CreatePartOfSpeech(noun);
         // Note we do *not* call crdtApi.CreatePartOfSpeech(noun);
 
+        var verb = new PartOfSpeech()
+        {
+            Id = new Guid("86ff66f6-0774-407a-a0dc-3eeaf873daf7"),
+            Name = { { "en", "verb" } },
+            Predefined = true,
+        };
+        await crdtApi.CreatePartOfSpeech(verb);
+
         await fwdataApi.CreateEntry(new Entry()
         {
             LexemeForm = { { "en", "Pear" } },
@@ -311,10 +319,10 @@ await fwdataApi.CreateEntry(new Entry()
         });
         await crdtApi.CreateEntry(new Entry()
         {
-            LexemeForm = { { "en", "Banana" } },
+            LexemeForm = { { "en", "Eat" } },
             Senses =
             [
-                new Sense() { Gloss = { { "en", "Banana" } }, PartOfSpeechId = noun.Id }
+                new Sense() { Gloss = { { "en", "Eat" } }, PartOfSpeechId = verb.Id }
             ]
         });
         await _syncService.Sync(crdtApi, fwdataApi);

backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs

@@ -42,7 +42,7 @@ public void SenseDiffShouldUpdateAllFields()
         var senseDiffToUpdate = SenseSync.SenseDiffToUpdate(before, after);
         ArgumentNullException.ThrowIfNull(senseDiffToUpdate);
         senseDiffToUpdate.Apply(before);
-        before.Should().BeEquivalentTo(after, options => options.Excluding(x => x.Id).Excluding(x => x.EntryId).Excluding(x => x.DeletedAt).Excluding(x => x.ExampleSentences).Excluding(x => x.SemanticDomains));
+        before.Should().BeEquivalentTo(after, options => options.Excluding(x => x.Id).Excluding(x => x.EntryId).Excluding(x => x.DeletedAt).Excluding(x => x.ExampleSentences).Excluding(x => x.SemanticDomains).Excluding(x => x.PartOfSpeech));
     }
 
     [Fact]

backend/FwLite/LcmCrdt.Tests/Changes/JsonPatchChangeTests.cs

@@ -37,13 +37,4 @@ public void NewChangeIPatchDoc_ThrowsForRemoveAtIndex()
         var act = () => new JsonPatchChange<Entry>(Guid.NewGuid(), patch);
         act.Should().Throw<NotSupportedException>();
     }
-
-    [Fact]
-    public void NewPatchDoc_ThrowsForIndexBasedPath()
-    {
-        var patch = new JsonPatchDocument<Entry>();
-        patch.Replace(entry => entry.Senses[0].PartOfSpeech, "noun");
-        var act = () => new JsonPatchChange<Entry>(Guid.NewGuid(), patch);
-        act.Should().Throw<NotSupportedException>();
-    }
 }

backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt

@@ -202,21 +202,23 @@
         Annotations: 
           Relational:ColumnType: jsonb
       Order (double) Required
-      PartOfSpeech (string) Required
-      PartOfSpeechId (Guid?)
+      PartOfSpeechId (Guid?) FK Index
       SemanticDomains (IList<SemanticDomain>) Required
         Annotations: 
           Relational:ColumnType: jsonb
       SnapshotId (no field, Guid?) Shadow FK Index
     Navigations: 
       ExampleSentences (List<ExampleSentence>) Collection ToDependent ExampleSentence
+      PartOfSpeech (PartOfSpeech) ToPrincipal PartOfSpeech
     Keys: 
       Id PK
     Foreign keys: 
       Sense {'EntryId'} -> Entry {'Id'} Required Cascade ToDependent: Senses
+      Sense {'PartOfSpeechId'} -> PartOfSpeech {'Id'} ClientSetNull ToPrincipal: PartOfSpeech
       Sense {'SnapshotId'} -> ObjectSnapshot {'Id'} Unique SetNull
     Indexes: 
       EntryId
+      PartOfSpeechId
       SnapshotId Unique
     Annotations: 
       DiscriminatorProperty: 

backend/FwLite/LcmCrdt.Tests/JsonPatchSenseRewriteTests.cs

@@ -11,14 +11,20 @@ public class JsonPatchSenseRewriteTests
 {
     private JsonPatchDocument<MiniLcm.Models.Sense> _patchDocument = new() { Options = new JsonSerializerOptions(JsonSerializerDefaults.Web) };
 
-    private Sense _sense = new Sense()
+    private Sense _sense = MakeSense("test");
+
+    private static Sense MakeSense(string name)
     {
-        Id = Guid.NewGuid(),
-        EntryId = Guid.NewGuid(),
-        PartOfSpeechId = Guid.NewGuid(),
-        PartOfSpeech = "test",
-        SemanticDomains = [new SemanticDomain() { Id = Guid.NewGuid(), Code = "test", Name = new MultiString() }],
-    };
+        var pos = new PartOfSpeech() { Id = Guid.NewGuid(), Name = {{ "en", name }} };
+        return new Sense()
+        {
+            Id = Guid.NewGuid(),
+            EntryId = Guid.NewGuid(),
+            PartOfSpeech = pos,
+            PartOfSpeechId = pos.Id,
+            SemanticDomains = [new SemanticDomain() { Id = Guid.NewGuid(), Code = "test", Name = new MultiString() }],
+        };
+    }
 
     [Fact]
     public void RewritePartOfSpeechChangesIntoSetPartOfSpeechChange()

backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs

@@ -1,4 +1,5 @@
 using System.Text.Json.Serialization;
+using LcmCrdt.Utils;
 using SIL.Harmony;
 using SIL.Harmony.Changes;
 using SIL.Harmony.Entities;
@@ -15,8 +16,7 @@ public CreateSenseChange(Sense sense, Guid entryId) : base(sense.Id == Guid.Empt
         Definition = sense.Definition;
         SemanticDomains = sense.SemanticDomains;
         Gloss = sense.Gloss;
-        PartOfSpeech = sense.PartOfSpeech;
-        PartOfSpeechId = sense.PartOfSpeechId;
+        PartOfSpeechId = sense.PartOfSpeech?.Id ?? sense.PartOfSpeechId;
     }
 
     [JsonConstructor]
@@ -29,7 +29,6 @@ private CreateSenseChange(Guid entityId, Guid entryId) : base(entityId)
     public double Order { get; set; }
     public MultiString? Definition { get; set; }
     public MultiString? Gloss { get; set; }
-    public string? PartOfSpeech { get; set; }
     public Guid? PartOfSpeechId { get; set; }
     public IList<SemanticDomain>? SemanticDomains { get; set; }
 
@@ -42,9 +41,8 @@ public override async ValueTask<Sense> NewEntity(Commit commit, ChangeContext co
             Order = Order,
             Definition = Definition ?? new MultiString(),
             Gloss = Gloss ?? new MultiString(),
-            PartOfSpeech = PartOfSpeech ?? string.Empty,
-            PartOfSpeechId = PartOfSpeechId,
-            SemanticDomains = SemanticDomains ?? [],
+            PartOfSpeechId = await context.DeletedAsNull(PartOfSpeechId),
+            SemanticDomains = await context.FilterDeleted(SemanticDomains ?? []).ToArrayAsync(),
             DeletedAt = await context.IsObjectDeleted(EntryId) ? commit.DateTime : (DateTime?)null
         };
     }

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs

@@ -12,18 +12,17 @@ public override async ValueTask ApplyChange(Sense entity, ChangeContext context)
         if (PartOfSpeechId is null)
         {
             entity.PartOfSpeechId = null;
-            entity.PartOfSpeech = string.Empty;
             return;
         }
 
         var partOfSpeech = await context.GetCurrent<PartOfSpeech>(PartOfSpeechId.Value);
         if (partOfSpeech is null or { DeletedAt: not null })
         {
             entity.PartOfSpeechId = null;
-            entity.PartOfSpeech = string.Empty;
+            entity.PartOfSpeech = null;
             return;
         }
         entity.PartOfSpeechId = partOfSpeech.Id;
-        entity.PartOfSpeech = partOfSpeech.Name["en"];
+        entity.PartOfSpeech = partOfSpeech;
     }
 }

backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs

@@ -276,6 +276,7 @@ private async IAsyncEnumerable<Entry> GetEntries(
         queryable = queryable
             .LoadWith(e => e.Senses)
             .ThenLoad(s => s.ExampleSentences)
+            .LoadWith(e => e.Senses).ThenLoad(s => s.PartOfSpeech)
             .LoadWith(e => e.ComplexForms)
             .LoadWith(e => e.Components)
             .AsQueryable()
@@ -295,6 +296,7 @@ private async IAsyncEnumerable<Entry> GetEntries(
         var entry = await Entries.AsTracking(false)
             .LoadWith(e => e.Senses)
             .ThenLoad(s => s.ExampleSentences)
+            .LoadWith(e => e.Senses).ThenLoad(s => s.PartOfSpeech)
             .LoadWith(e => e.ComplexForms)
             .LoadWith(e => e.Components)
             .AsQueryable()
@@ -321,16 +323,15 @@ await dataModel.AddChanges(ClientId,
     public async Task BulkCreateEntries(IAsyncEnumerable<Entry> entries)
     {
         var semanticDomains = await SemanticDomains.ToDictionaryAsync(sd => sd.Id, sd => sd);
-        var partsOfSpeech = await PartsOfSpeech.ToDictionaryAsync(p => p.Id, p => p);
         await dataModel.AddChanges(ClientId,
             entries.ToBlockingEnumerable()
-                .SelectMany(entry => CreateEntryChanges(entry, semanticDomains, partsOfSpeech))
+                .SelectMany(entry => CreateEntryChanges(entry, semanticDomains))
                 //force entries to be created first, this avoids issues where references are created before the entry is created
                 .OrderBy(c => c is CreateEntryChange ? 0 : 1)
         );
     }
 
-    private IEnumerable<IChange> CreateEntryChanges(Entry entry, Dictionary<Guid, SemanticDomain> semanticDomains, Dictionary<Guid, PartOfSpeech> partsOfSpeech)
+    private IEnumerable<IChange> CreateEntryChanges(Entry entry, Dictionary<Guid, SemanticDomain> semanticDomains)
     {
         yield return new CreateEntryChange(entry);
 
@@ -350,11 +351,6 @@ private IEnumerable<IChange> CreateEntryChanges(Entry entry, Dictionary<Guid, Se
                 .Select(sd => semanticDomains.TryGetValue(sd.Id, out var selectedSd) ? selectedSd : null)
                 .OfType<MiniLcm.Models.SemanticDomain>()
                 .ToList();
-            if (sense.PartOfSpeechId is not null && partsOfSpeech.TryGetValue(sense.PartOfSpeechId.Value, out var partOfSpeech))
-            {
-                sense.PartOfSpeechId = partOfSpeech.Id;
-                sense.PartOfSpeech = partOfSpeech.Name["en"] ?? string.Empty;
-            }
             sense.Order = senseOrder++;
             yield return new CreateSenseChange(sense, entry.Id);
             var exampleOrder = 1;
@@ -469,12 +465,6 @@ private async IAsyncEnumerable<IChange> CreateSenseChanges(Guid entryId, Sense s
         sense.SemanticDomains = await SemanticDomains
             .Where(sd => sense.SemanticDomains.Select(s => s.Id).Contains(sd.Id))
             .ToListAsync();
-        if (sense.PartOfSpeechId is not null)
-        {
-            var partOfSpeech = await PartsOfSpeech.FirstOrDefaultAsync(p => p.Id == sense.PartOfSpeechId);
-            sense.PartOfSpeechId = partOfSpeech?.Id;
-            sense.PartOfSpeech = partOfSpeech?.Name["en"] ?? string.Empty;
-        }
 
         yield return new CreateSenseChange(sense, entryId);
         var exampleOrder = 1;
@@ -490,6 +480,7 @@ private async IAsyncEnumerable<IChange> CreateSenseChanges(Guid entryId, Sense s
         var entry = await Entries.AsTracking(false)
             .LoadWith(e => e.Senses)
             .ThenLoad(s => s.ExampleSentences)
+            .LoadWith(e => e.Senses).ThenLoad(s => s.PartOfSpeech)
             .AsQueryable()
             .SingleOrDefaultAsync(e => e.Id == entryId);
         var sense = entry?.Senses.FirstOrDefault(s => s.Id == id);
@@ -500,6 +491,9 @@ private async IAsyncEnumerable<IChange> CreateSenseChanges(Guid entryId, Sense s
     public async Task<Sense> CreateSense(Guid entryId, Sense sense, BetweenPosition? between = null)
     {
         await validators.ValidateAndThrow(sense);
+        if (sense.PartOfSpeechId.HasValue && await GetPartOfSpeech(sense.PartOfSpeechId.Value) is null)
+            throw new InvalidOperationException($"Part of speech must exist when creating a sense (could not find GUID {sense.PartOfSpeechId.Value})");
+
         sense.Order = await OrderPicker.PickOrder(Senses.Where(s => s.EntryId == entryId), between);
         await dataModel.AddChanges(ClientId, await CreateSenseChanges(entryId, sense).ToArrayAsync());
         return await dataModel.GetLatest<Sense>(sense.Id) ?? throw new NullReferenceException();