Feat/1246 Allow reordering senses (#1308)

* Add collection position diffing

* Add some collection diff tests and redesign to use after/before positioning

* Exclude Order from external serialization, refactor, add more tests

* Update/extend entry tests for sense reordering and cleanup

* Rename method

* Don't set order on moved sense, because it shouldn't be necessary to set an internal property for an external caller.

* Set sense order when creating entries in bulk

* Standardize using AllSenses (not SensesOS) and fix moving between subsenses

* Cleanup

* Fix tests

* Revert making Sense partial

* Sort senses in place

* Refactor OrderPicker to require siblings

* Rename BetweenPosition props and simplify orderable methods

* Refactor collection diffing to use class/object for add, delete, move & replace

* add back a comment which got lost

* simplify generating random moves for entry sync tests

* specify a namespace for the MiniLcmJson class

* add comment about what the MiniLcmInternalAttribute.cs is used for

* Fix failing test

* Only pass senseId to MoveSense

* Use letters for test glosses, because orders use int

* Don't mock IOrderable in tests

* Revert unintended extra sync

* Fix SetOrderChange type discriminator(s)

* Optimize order picker

* Refactor Orderable diffing with custom diff formatter

* Incorporated PR feedback

---------

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

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs

@@ -9,4 +9,6 @@ protected override Task<IMiniLcmApi> NewApi()
     {
         return Task.FromResult<IMiniLcmApi>(fixture.NewProjectApi("update-entry-test", "en", "en"));
     }
+
+    protected override bool ApiUsesImplicitOrdering => true;
 }

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs

@@ -1,6 +1,5 @@
 using System.Collections.Frozen;
 using System.Globalization;
-using System.Reflection;
 using System.Text;
 using FluentValidation;
 using FwDataMiniLcmBridge.Api.UpdateProxy;
@@ -649,7 +648,7 @@ public IAsyncEnumerable<Entry> SearchEntries(string query, QueryOptions? options
         var entries = GetEntries(e =>
             e.CitationForm.SearchValue(query) ||
             e.LexemeFormOA.Form.SearchValue(query) ||
-            e.SensesOS.Any(s => s.Gloss.SearchValue(query)), options);
+            e.AllSenses.Any(s => s.Gloss.SearchValue(query)), options);
         return entries;
     }
 
@@ -874,9 +873,10 @@ public Task DeleteEntry(Guid id)
         return Task.CompletedTask;
     }
 
-    internal void CreateSense(ILexEntry lexEntry, Sense sense)
+    internal void CreateSense(ILexEntry lexEntry, Sense sense, BetweenPosition? between = null)
     {
-        var lexSense = LexSenseFactory.Create(sense.Id, lexEntry);
+        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))
         {
@@ -886,6 +886,46 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense)
         ApplySenseToLexSense(sense, lexSense);
     }
 
+    internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, BetweenPosition? between = null)
+    {
+        var previousSenseId = between?.Previous;
+        var nextSenseId = between?.Next;
+
+        var previousSense = previousSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == previousSenseId) : null;
+        if (previousSense is not null)
+        {
+            if (previousSense.SensesOS.Count > 0)
+            {
+                // if the sense has sub-senses, our sense will only come directly after it if it is the first sub-sense
+                previousSense.SensesOS.Insert(0, lexSense);
+            }
+            else
+            {
+                // todo the user might have wanted it to be a subsense of previousSense
+                var allSiblings = previousSense.Owner == lexEntry ? lexEntry.SensesOS
+                    : previousSense.Owner is ILexSense parentSense ? parentSense.SensesOS
+                    : throw new InvalidOperationException("Sense parent is not a sense or the expected entry");
+                var insertI = allSiblings.IndexOf(previousSense) + 1;
+                lexEntry.SensesOS.Insert(insertI, lexSense);
+            }
+            return;
+        }
+
+        var nextSense = nextSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == nextSenseId) : null;
+        if (nextSense is not null)
+        {
+            // todo the user might have wanted it to be a subsense of whatever is before nextSense
+            var allSiblings = nextSense.Owner == lexEntry ? lexEntry.SensesOS
+                    : nextSense.Owner is ILexSense parentSense ? parentSense.SensesOS
+                    : throw new InvalidOperationException("Sense parent is not a sense or the expected entry");
+            var insertI = allSiblings.IndexOf(nextSense);
+            lexEntry.SensesOS.Insert(insertI, lexSense);
+            return;
+        }
+
+        lexEntry.SensesOS.Add(lexSense);
+    }
+
     private void ApplySenseToLexSense(Sense sense, ILexSense lexSense)
     {
         if (lexSense.MorphoSyntaxAnalysisRA.GetPartOfSpeech()?.Guid != sense.PartOfSpeechId)
@@ -917,15 +957,15 @@ private void ApplySenseToLexSense(Sense sense, ILexSense lexSense)
         return Task.FromResult(lcmSense is null ? null : FromLexSense(lcmSense));
     }
 
-    public Task<Sense> CreateSense(Guid entryId, Sense sense)
+    public Task<Sense> CreateSense(Guid entryId, Sense sense, BetweenPosition? between = null)
     {
         if (sense.Id == default) sense.Id = Guid.NewGuid();
         if (!EntriesRepository.TryGetObject(entryId, out var lexEntry))
             throw new InvalidOperationException("Entry not found");
         UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Create Sense",
             "Remove sense",
             Cache.ServiceLocator.ActionHandler,
-            () => CreateSense(lexEntry, sense));
+            () => CreateSense(lexEntry, sense, between));
         return Task.FromResult(FromLexSense(SenseRepository.GetObject(sense.Id)));
     }
 
@@ -955,6 +995,24 @@ await Cache.DoUsingNewOrCurrentUOW("Update Sense",
         return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id);
     }
 
+    public Task MoveSense(Guid entryId, Guid senseId, BetweenPosition between)
+    {
+        if (!EntriesRepository.TryGetObject(entryId, out var lexEntry))
+            throw new InvalidOperationException("Entry not found");
+        if (!SenseRepository.TryGetObject(senseId, out var lexSense))
+            throw new InvalidOperationException("Sense not found");
+
+        UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Move Sense",
+            "Move Sense back",
+            Cache.ServiceLocator.ActionHandler,
+            () =>
+            {
+                // LibLCM treats an insert as a move if the sense is already in the entry
+                InsertSense(lexEntry, lexSense, between);
+            });
+        return Task.CompletedTask;
+    }
+
     public Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain)
     {
         UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Add Semantic Domain to Sense",

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

@@ -45,15 +45,9 @@ public override MultiString LiteralMeaning
         set => throw new NotImplementedException();
     }
 
-    public override IList<Sense> Senses
+    public override List<Sense> Senses
     {
-        get =>
-            new UpdateListProxy<Sense>(
-                sense => _lexboxLcmApi.CreateSense(_lcmEntry, sense),
-                sense => _lexboxLcmApi.DeleteSense(Id, sense.Id),
-                i => new UpdateSenseProxy(_lcmEntry.SensesOS[i], _lexboxLcmApi),
-                _lcmEntry.SensesOS.Count
-            );
+        get => throw new NotImplementedException();
         set => throw new NotImplementedException();
     }
 

backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs

@@ -1,15 +1,25 @@
 using FwLiteProjectSync.Tests.Fixtures;
 using MiniLcm.Models;
 using MiniLcm.SyncHelpers;
-using MiniLcm.Tests;
 using MiniLcm.Tests.AutoFakerHelpers;
 using Soenneker.Utils.AutoBogus;
+using Soenneker.Utils.AutoBogus.Config;
 
 namespace FwLiteProjectSync.Tests;
 
 public class EntrySyncTests : IClassFixture<SyncFixture>
 {
-    private static readonly AutoFaker AutoFaker = new(builder => builder.WithOverride(new MultiStringOverride()).WithOverride(new ObjectWithIdOverride()));
+    private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig()
+    {
+        RepeatCount = 5,
+        Overrides =
+        [
+            new MultiStringOverride(),
+            new ObjectWithIdOverride(),
+            new OrderableOverride(),
+        ]
+    });
+
     public EntrySyncTests(SyncFixture fixture)
     {
         _fixture = fixture;
@@ -22,10 +32,18 @@ public async Task CanSyncRandomEntries()
     {
         var createdEntry = await _fixture.CrdtApi.CreateEntry(await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi));
         var after = await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi, entryId: createdEntry.Id);
+
+        after.Senses = AutoFaker.Faker.Random.Shuffle([
+                // copy some senses over, so moves happen
+                ..AutoFaker.Faker.Random.ListItems(createdEntry.Senses),
+                ..(after.Senses)
+            ]).ToList();
+
         await EntrySync.Sync(after, createdEntry, _fixture.CrdtApi);
         var actual = await _fixture.CrdtApi.GetEntry(after.Id);
         actual.Should().NotBeNull();
-        actual.Should().BeEquivalentTo(after, options => options);
+        actual.Should().BeEquivalentTo(after, options => options
+            .For(e => e.Senses).Exclude(s => s.Order));
     }
 
     [Fact]

backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs

@@ -51,7 +51,8 @@ private void ShouldAllBeEquivalentTo(Dictionary<Guid, Entry> crdtEntries, Dictio
                 crdtEntry.Should().BeEquivalentTo(fwdataEntry,
                     options => options
                         .For(e => e.Components).Exclude(c => c.Id)
-                        .For(e => e.ComplexForms).Exclude(c => c.Id),
+                        .For(e => e.ComplexForms).Exclude(c => c.Id)
+                        .For(e => e.Senses).Exclude(s => s.Order),
                     $"CRDT entry {crdtEntry.Id} was synced with FwData");
             }
         }

backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs

@@ -83,7 +83,9 @@ public async Task FirstSyncJustDoesAnImport()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
     }
 
@@ -145,7 +147,9 @@ await crdtApi.CreateEntry(new Entry()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
     }
 
@@ -223,7 +227,9 @@ public async Task CreatingAComplexEntryInFwDataSyncsWithoutIssue()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
 
         // Sync again, ensure no problems or changes
@@ -305,7 +311,9 @@ await crdtApi.CreateEntry(new Entry()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
     }
 
@@ -384,7 +392,9 @@ await crdtApi.CreateEntry(new Entry()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
     }
 
@@ -408,6 +418,7 @@ public async Task UpdatingAnEntryInEachProjectSyncsAcrossBoth()
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
             options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
                 .For(e => e.Components).Exclude(c => c.Id)
                 //todo the headword should be changed
                 .For(e => e.Components).Exclude(c => c.ComponentHeadword)
@@ -475,7 +486,9 @@ public async Task AddingASenseToAnEntryInEachProjectSyncsAcrossBoth()
         var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync();
         var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync();
         crdtEntries.Should().BeEquivalentTo(fwdataEntries,
-            options => options.For(e => e.Components).Exclude(c => c.Id)
+            options => options
+                .For(e => e.Senses).Exclude(s => s.Order)
+                .For(e => e.Components).Exclude(c => c.Id)
                 .For(e => e.ComplexForms).Exclude(c => c.Id));
     }
 

backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs

@@ -1,5 +1,4 @@
-using FwLiteProjectSync.Tests.Fixtures;
-using MiniLcm.Models;
+using MiniLcm.Models;
 using MiniLcm.SyncHelpers;
 using MiniLcm.Tests.AutoFakerHelpers;
 using Soenneker.Utils.AutoBogus;
@@ -11,7 +10,13 @@ public class UpdateDiffTests
 {
     private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig()
     {
-        Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()]
+        RepeatCount = 5,
+        Overrides =
+        [
+            new MultiStringOverride(),
+            new WritingSystemIdOverride(),
+            new OrderableOverride(),
+        ]
     });
 
     [Fact]

backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs

@@ -1,5 +1,6 @@
 using MiniLcm;
 using MiniLcm.Models;
+using MiniLcm.SyncHelpers;
 
 namespace FwLiteProjectSync;
 
@@ -195,9 +196,9 @@ public async Task RemoveComplexFormType(Guid entryId, Guid complexFormTypeId)
         return api.GetSense(entryId, id);
     }
 
-    public Task<Sense> CreateSense(Guid entryId, Sense sense)
+    public Task<Sense> CreateSense(Guid entryId, Sense sense, BetweenPosition? position = null)
     {
-        DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss}"));
+        DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss} between {position?.Previous} and {position?.Next}"));
         return Task.FromResult(sense);
     }
 
@@ -218,6 +219,12 @@ public async Task<Sense> UpdateSense(Guid entryId, Sense before, Sense after)
         return await GetSense(entryId, after.Id) ?? throw new NullReferenceException($"unable to find sense with id {after.Id}");
     }
 
+    public Task MoveSense(Guid entryId, Guid senseId, BetweenPosition between)
+    {
+        DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {senseId} between {between.Previous} and {between.Next}"));
+        return Task.CompletedTask;
+    }
+
     public Task DeleteSense(Guid entryId, Guid senseId)
     {
         DryRunRecords.Add(new DryRunRecord(nameof(DeleteSense), $"Delete sense {senseId}"));

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

@@ -119,6 +119,10 @@
     {
       DerivedType: CreateComplexFormType,
       TypeDiscriminator: CreateComplexFormType
+    },
+    {
+      DerivedType: SetOrderChange<Sense>,
+      TypeDiscriminator: SetOrderChange:Sense
     }
   ],
   IgnoreUnrecognizedTypeDiscriminators: false,

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

@@ -98,7 +98,7 @@
     Navigations: 
       ComplexForms (IList<ComplexFormComponent>) Collection ToDependent ComplexFormComponent
       Components (IList<ComplexFormComponent>) Collection ToDependent ComplexFormComponent
-      Senses (IList<Sense>) Collection ToDependent Sense
+      Senses (List<Sense>) Collection ToDependent Sense
     Keys: 
       Id PK
     Foreign keys: 
@@ -200,6 +200,7 @@
       Gloss (MultiString) Required
         Annotations: 
           Relational:ColumnType: jsonb
+      Order (double) Required
       PartOfSpeech (string) Required
       PartOfSpeechId (Guid?)
       SemanticDomains (IList<SemanticDomain>) Required

backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs

@@ -17,9 +17,15 @@ namespace LcmCrdt.Tests;
 
 public class DataModelSnapshotTests : IAsyncLifetime
 {
-    private static readonly AutoFaker Faker = new AutoFaker(new AutoFakerConfig()
+    private static readonly AutoFaker Faker = new(new AutoFakerConfig()
     {
-        Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()]
+        RepeatCount = 5,
+        Overrides =
+        [
+            new MultiStringOverride(),
+            new WritingSystemIdOverride(),
+            new OrderableOverride(),
+        ]
     });
 
     protected readonly AsyncServiceScope _services;

backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs

@@ -1,7 +1,4 @@
-using SIL.Harmony.Entities;
-using LcmCrdt.Objects;
-using MiniLcm.Tests.AutoFakerHelpers;
-using SIL.Harmony;
+using MiniLcm.Tests.AutoFakerHelpers;
 using Soenneker.Utils.AutoBogus;
 using Soenneker.Utils.AutoBogus.Config;
 
@@ -11,7 +8,13 @@ public class EntityCopyMethodTests
 {
     private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig()
     {
-        Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()]
+        RepeatCount = 5,
+        Overrides =
+        [
+            new MultiStringOverride(),
+            new WritingSystemIdOverride(),
+            new OrderableOverride(),
+        ],
     });
 
     public static IEnumerable<object[]> GetEntityTypes()