Skip to content

Fix LT-19237: preserve Data Notebook fields merging record-type layouts #429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jasonleenaylor merged 4 commits into from
Jul 2, 2026
Merged

Fix LT-19237: preserve Data Notebook fields merging record-type layouts #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b87c3c
Fix LT-19237: preserve Data Notebook fields merging record-type layouts
jasonleenaylor Jul 1, 2026
83377db
Correct LT-19237 test comments
jasonleenaylor Jul 1, 2026
701efb5
Fix LT-19237: match record-type layouts by choiceGuid in the diff path
jasonleenaylor Jul 1, 2026
ecf6a85
Normalize LT-19237 test data indentation to tabs
jasonleenaylor Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutDataCollectorMethod.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,15 @@ internal static class CustomLayoutDataCollectorMethod
internal static Dictionary<string, byte[]> GetDataFromRevision(FileInRevision revision, HgRepository repository)
{
var doc = XDocument.Parse(revision.GetFileContents(repository));
// LT-19237: include choiceGuid in the key. The Data Notebook emits one <layout> 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");
Expand Down
7 changes: 6 additions & 1 deletion src/LibFLExBridge-ChorusPlugin/Handling/ConfigLayout/CustomLayoutMergeStrategiesMethod.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ internal static void AddElementStrategies(MergeStrategies mergeStrategies)
#region 'layout' and children.
elStrat = new ElementStrategy(false)
{
MergePartnerFinder = new FindByMultipleKeyAttributes(new List<string> { "class", "type", "name" }),
// LT-19237: The Data Notebook emits one <layout> 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<string> { "class", "type", "name", "choiceGuid" }),
ContextDescriptorGenerator = new FieldWorkCustomLayoutContextGenerator()
};
mergeStrategies.SetStrategy("layout", elStrat);
Expand Down
161 changes: 161 additions & 0 deletions ...xBridge-ChorusPluginTests/Handling/ConfigLayout/FieldWorksCustomLayoutTypeHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <layout> 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 =
@"<?xml version='1.0' encoding='utf-8'?>
<LayoutInventory>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
</layout>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC' version='25'>
<part ref='Title' visibility='always' />
<part ref='SeeAlso' visibility='always' />
</layout>
</LayoutInventory>";

// 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<XmlChangedRecordReport>(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()
{
Expand Down Expand Up @@ -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 <layout> 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 =
@"<?xml version='1.0' encoding='utf-8'?>
<LayoutInventory>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
<part ref='SeeAlso' visibility='always' />
<part ref='ExternalMaterials' visibility='always' />
</layout>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
<part ref='SeeAlso' visibility='always' />
<part ref='ExternalMaterials' visibility='always' />
<part ref='Custom' param='Esquema de Materiales Culturales' />
</layout>
</LayoutInventory>";

// 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 =
@"<?xml version='1.0' encoding='utf-8'?>
<LayoutInventory>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
<part ref='SeeAlso' visibility='always' />
<part ref='ExternalMaterials' visibility='always' />
<part ref='Custom' param='Esquema de Materiales Culturales' />
</layout>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='always' />
<part ref='SeeAlso' visibility='always' />
<part ref='ExternalMaterials' visibility='always' />
<part ref='Custom' param='Esquema de Materiales Culturales' />
</layout>
</LayoutInventory>";

// THEIRS: ancestor order; only the B7EA5156 layout is edited
// (SeeAlso & ExternalMaterials always->ifdata, custom field gains a visibility).
const string theirContent =
@"<?xml version='1.0' encoding='utf-8'?>
<LayoutInventory>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='08e4d456-ce03-4bc1-9231-38caca76b80f' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
<part ref='SeeAlso' visibility='always' />
<part ref='ExternalMaterials' visibility='always' />
</layout>
<layout class='RnGenericRec' type='detail' name='Normal' choiceGuid='B7EA5156-EA5E-11DE-9F9C-0013722F8DEC' version='25'>
<part ref='Title' visibility='always' />
<part ref='Hypothesis' visibility='ifdata' />
<part ref='SeeAlso' visibility='ifdata' />
<part ref='ExternalMaterials' visibility='ifdata' />
<part ref='Custom' param='Esquema de Materiales Culturales' visibility='ifdata' />
</layout>
</LayoutInventory>";

var matchesExactlyOne = new List<string>
{
// 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 <part> edits surface
// as XmlChangedRecordReport; the added <part> as XmlAdditionChangeReport.
var expectedChanges = new List<Type>
{
typeof(XmlChangedRecordReport),
typeof(XmlChangedRecordReport),
typeof(XmlChangedRecordReport),
typeof(XmlChangedRecordReport),
typeof(XmlAdditionChangeReport)
};

FieldWorksTestServices.DoMerge(
FileHandler,
_ourFile, ourContent,
_commonFile, commonAncestor,
_theirFile, theirContent,
matchesExactlyOne, null,
0, new List<Type>(),
expectedChanges.Count, expectedChanges);
}

[Test]
public void SampleMergeWithMissingAncestor()
{
Expand Down
Loading