From 1b87c3c0c29f48e6c5c90e83e464e9ff1ca70e11 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 1 Jul 2026 12:27:08 -0700 Subject: [PATCH 1/4] Fix LT-19237: preserve Data Notebook fields merging record-type layouts The Data Notebook writes one layout per record type, all sharing the same class/type/name and differing only by choiceGuid. Leaving choiceGuid out of the merge key let Send/Receive confuse one record type's layout for another's, dropping most fields from a record type. Co-Authored-By: Claude Opus 4.8 --- .../CustomLayoutMergeStrategiesMethod.cs | 7 +- .../FieldWorksCustomLayoutTypeHandlerTests.cs | 111 ++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs index 83fde532d..16fc3456b 100644 --- a/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs +++ b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs @@ -29,7 +29,12 @@ internal static void AddElementStrategies(MergeStrategies mergeStrategies) #region 'layout' and children. elStrat = new ElementStrategy(false) { - MergePartnerFinder = new FindByMultipleKeyAttributes(new List { "class", "type", "name" }), + // LT-19237: The Data Notebook emits one per record type, all sharing the + // same class/type/name and differing only by choiceGuid. Without choiceGuid in the + // key the merger cannot tell record-type layouts apart and cross-matches them, + // losing fields and duplicating layouts. Layouts without a choiceGuid all key on a + // null value, so they continue to match each other exactly as before. + MergePartnerFinder = new FindByMultipleKeyAttributes(new List { "class", "type", "name", "choiceGuid" }), ContextDescriptorGenerator = new FieldWorkCustomLayoutContextGenerator() }; mergeStrategies.SetStrategy("layout", elStrat); diff --git a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs index 6051c1134..4d0aa4ce6 100644 --- a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs +++ b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs @@ -323,6 +323,117 @@ public void SampleMergeWithConflicts() Assert.IsFalse(results.Contains("combinedkey")); } + [Test] + public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() + { + // LT-19237: The Data Notebook writes one per record type, and every one of + // them shares class="RnGenericRec" type="detail" name="Normal"; they are distinguished + // ONLY by the choiceGuid attribute. Before this fix the layout merge key was + // {class, type, name}, so two record-type layouts were indistinguishable to the merger. + // When two users had the layouts in a different order and each edited a DIFFERENT + // record type's layout, the merger cross-matched them, fabricating spurious conflicts, + // losing most fields from one layout and duplicating the other. + // The GUIDs, the reversed ordering, and the edited parts below are taken from the real + // Nukak reproduction attached to the ticket (SUEL vs FamiliaTrujillo). + const string commonAncestor = +@" + + + + + + + + + + + + + + +"; + + // OURS (SUEL): the two record-type layouts are in REVERSED order, and only the + // 08e4d456 layout is edited (Hypothesis visibility ifdata->always, custom field added). + const string ourContent = +@" + + + + + + + + + + + + + + + +"; + + // THEIRS (FamiliaTrujillo): ancestor order; only the B7EA5156 layout is edited + // (SeeAlso & ExternalMaterials always->ifdata, custom field gains a visibility). + const string theirContent = +@" + + + + + + + + + + + + + + +"; + + var matchesExactlyOne = new List + { + // Exactly one layout survives per record type (the bug duplicated one and mangled the other). + "LayoutInventory/layout[@choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f']", + "LayoutInventory/layout[@choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC']", + // Our edit landed in the 08e4d456 layout... + "LayoutInventory/layout[@choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f']/part[@ref='Hypothesis' and @visibility='always']", + // ...and did NOT bleed into the B7EA5156 layout. + "LayoutInventory/layout[@choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC']/part[@ref='Hypothesis' and @visibility='ifdata']", + // Their edits landed in the B7EA5156 layout... + "LayoutInventory/layout[@choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC']/part[@ref='SeeAlso' and @visibility='ifdata']", + // ...and did NOT bleed into the 08e4d456 layout. + "LayoutInventory/layout[@choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f']/part[@ref='SeeAlso' and @visibility='always']" + }; + + // A correct merge of edits to DIFFERENT record-type layouts has NO conflicts. + // (The change-report expectations are pinned from the observed correct merge output.) + // Each side's edits are applied (none lost): our 08e4d456 layout has one attribute + // change (Hypothesis) plus one added part (Custom); their B7EA5156 layout has three + // changes (SeeAlso, ExternalMaterials, Custom visibility). Atomic edits surface + // as XmlChangedRecordReport; the added as XmlAdditionChangeReport. + var expectedChanges = new List + { + typeof(XmlChangedRecordReport), + typeof(XmlChangedRecordReport), + typeof(XmlChangedRecordReport), + typeof(XmlChangedRecordReport), + typeof(XmlAdditionChangeReport) + }; + + FieldWorksTestServices.DoMerge( + FileHandler, + _ourFile, ourContent, + _commonFile, commonAncestor, + _theirFile, theirContent, + matchesExactlyOne, null, + 0, new List(), + expectedChanges.Count, expectedChanges); + } + [Test] public void SampleMergeWithMissingAncestor() { From 83377db1bd5e49ea55cc3939387d5355ff7551a2 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 1 Jul 2026 13:29:42 -0700 Subject: [PATCH 2/4] Correct LT-19237 test comments Drop the unverifiable SUEL/FamiliaTrujillo user-to-side attribution and note the test data is a trimmed subset of the ticket's reproduction. Co-Authored-By: Claude Opus 4.8 --- .../FieldWorksCustomLayoutTypeHandlerTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs index 4d0aa4ce6..a671c093b 100644 --- a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs +++ b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs @@ -333,8 +333,8 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() // When two users had the layouts in a different order and each edited a DIFFERENT // record type's layout, the merger cross-matched them, fabricating spurious conflicts, // losing most fields from one layout and duplicating the other. - // The GUIDs, the reversed ordering, and the edited parts below are taken from the real - // Nukak reproduction attached to the ticket (SUEL vs FamiliaTrujillo). + // The GUIDs, the reversed ordering, and the edited fields below are a trimmed subset of + // the real Nukak reproduction attached to the ticket. const string commonAncestor = @" @@ -353,7 +353,7 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() "; - // OURS (SUEL): the two record-type layouts are in REVERSED order, and only the + // OURS: the two record-type layouts are in REVERSED order, and only the // 08e4d456 layout is edited (Hypothesis visibility ifdata->always, custom field added). const string ourContent = @" @@ -374,7 +374,7 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() "; - // THEIRS (FamiliaTrujillo): ancestor order; only the B7EA5156 layout is edited + // THEIRS: ancestor order; only the B7EA5156 layout is edited // (SeeAlso & ExternalMaterials always->ifdata, custom field gains a visibility). const string theirContent = @" From 701efb57a5fbbc1a5d5e3895eecc5832981d2df2 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 1 Jul 2026 15:51:44 -0700 Subject: [PATCH 3/4] Fix LT-19237: match record-type layouts by choiceGuid in the diff path The two-way diff collector keyed Data Notebook layouts on class/type/name only, so record types that differ only by choiceGuid collided and threw a duplicate-key exception when viewing history or change reports. Key on choiceGuid too, matching the merge fix. Co-Authored-By: Claude Opus 4.8 --- .../CustomLayoutDataCollectorMethod.cs | 8 ++- .../FieldWorksCustomLayoutTypeHandlerTests.cs | 50 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs index 2a04467a6..406e8eefe 100644 --- a/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs +++ b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs @@ -15,9 +15,15 @@ internal static class CustomLayoutDataCollectorMethod internal static Dictionary GetDataFromRevision(FileInRevision revision, HgRepository repository) { var doc = XDocument.Parse(revision.GetFileContents(repository)); + // LT-19237: include choiceGuid in the key. The Data Notebook emits one per + // record type, all sharing class/type/name and differing only by choiceGuid; without it + // the keys collide and ToDictionary throws a duplicate-key ArgumentException. choiceGuid + // is optional, so layouts without it key on class+type+name+"" (byte-identical to the old + // key). Matches the merge partner key in CustomLayoutMergeStrategiesMethod. var data = doc.Root.Elements("layout") .ToDictionary(layoutElement => - layoutElement.Attribute("class").Value + layoutElement.Attribute("type").Value + layoutElement.Attribute("name").Value, + layoutElement.Attribute("class").Value + layoutElement.Attribute("type").Value + layoutElement.Attribute("name").Value + + (layoutElement.Attribute("choiceGuid")?.Value ?? ""), layoutElement => LibTriboroughBridgeSharedConstants.Utf8.GetBytes(layoutElement.ToString())); var layoutTypeElement = doc.Root.Element("layoutType"); diff --git a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs index a671c093b..7760c2916 100644 --- a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs +++ b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs @@ -272,6 +272,56 @@ orderby rev.Number.LocalRevisionNumber } } + [Test] + public void Diff_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() + { + // LT-19237 (two-way diff path): the Data Notebook emits one per record type, + // all sharing class="RnGenericRec" type="detail" name="Normal" and differing only by + // choiceGuid. The diff data collector keyed layouts on class+type+name alone, so two + // record-type layouts collided and ToDictionary threw a duplicate-key ArgumentException + // when a user viewed history/change reports. The key now includes choiceGuid (matching + // the merge fix), so each record-type layout is diffed against its own counterpart. + const string parent = +@" + + + + + + + + + +"; + + // Edit ONLY the 08e4d456 layout: Hypothesis visibility ifdata->always. ('ifdata' occurs + // only in that layout, so the B7EA5156 layout is left untouched.) + var child = parent.Replace("ifdata", "always"); + + using (var repositorySetup = new RepositorySetup("randy")) + { + repositorySetup.AddAndCheckinFile("RnGenericRec." + FlexBridgeConstants.fwlayout, parent); + repositorySetup.ChangeFileAndCommit("RnGenericRec." + FlexBridgeConstants.fwlayout, child, "change it"); + var hgRepository = repositorySetup.Repository; + var allRevisions = (from rev in hgRepository.GetAllRevisions() + orderby rev.Number.LocalRevisionNumber + select rev).ToList(); + var firstFiR = hgRepository.GetFilesInRevision(allRevisions[0]).First(); + var secondFiR = hgRepository.GetFilesInRevision(allRevisions[1]).First(); + + // Before the fix this call threw ArgumentException (duplicate key) instead of returning. + var result = FileHandler.Find2WayDifferences(firstFiR, secondFiR, hgRepository).ToList(); + + Assert.AreEqual(1, result.Count); + var onlyReport = result[0]; + Assert.IsInstanceOf(onlyReport); + Assert.AreEqual(firstFiR.FullPath, onlyReport.PathToFile); + // The change is attributed to the edited record type (08e4d456), not the other layout. + var changedLayout = ((XmlChangedRecordReport)onlyReport).ChildNode; + Assert.AreEqual("08e4d456-ce03-4bc1-9231-38caca76b80f", changedLayout.Attributes["choiceGuid"].Value); + } + } + [Test] public void SampleMergeWithNoConflicts() { From ecf6a85c6f7a97af7ed31ec451a15d22fd1cdd32 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 1 Jul 2026 15:58:51 -0700 Subject: [PATCH 4/4] Normalize LT-19237 test data indentation to tabs Indent the elements in the layout test data with a tab, matching the other tests in this fixture (and real FLEx layout output); they were 4-space indented. Whitespace only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- .../FieldWorksCustomLayoutTypeHandlerTests.cs | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs index 7760c2916..8d6772286 100644 --- a/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs +++ b/src/LibFLExBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs @@ -285,12 +285,12 @@ public void Diff_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() @" - - + + - - + + "; @@ -389,17 +389,17 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() @" - - - - + + + + - - - - - + + + + + "; @@ -409,18 +409,18 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() @" - - - - - + + + + + - - - - - + + + + + "; @@ -430,17 +430,17 @@ public void Merge_RecordTypeLayoutsAreMatchedByChoiceGuid_LT19237() @" - - - - + + + + - - - - - + + + + + ";