diff --git a/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs index 2a04467a..406e8eef 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-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs b/src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs index 83fde532..16fc3456 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 6051c113..8d677228 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() { @@ -323,6 +373,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 fields below are a trimmed subset of + // the real Nukak reproduction attached to the ticket. + const string commonAncestor = +@" + + + + + + + + + + + + + + +"; + + // 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 = +@" + + + + + + + + + + + + + + + +"; + + // THEIRS: 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() {