From 7f6006d111c64086d7ad91670fad618afc1ab0ff Mon Sep 17 00:00:00 2001 From: koeylai-adsk Date: Tue, 25 Aug 2026 16:56:02 -0400 Subject: [PATCH] DYN-10569: Gate Python port removal warning The "Remove Port?" dialog warned that custom port properties would be lost on every input-port removal from a Python Script node, including freshly added ports that had never been modified. This created friction and implied changes existed where none did. Add PythonNodeBase.HasCustomInputPortProperties(int), which compares a port's live Name and ToolTip against the generated defaults for its index, and gate the dialog on it. Removal always targets the last input port, so only that port is inspected; this also suppresses the prompt when no port remains to remove. The helper is internal, so no public API surface changes. Co-Authored-By: Claude Opus 5 (1M context) --- .../Controls/DynamoNodeButton.cs | 10 +- src/Libraries/PythonNodeModels/PythonNode.cs | 29 +++ .../PythonNodeRemovePortWarningTests.cs | 197 ++++++++++++++++++ .../DynamoPythonTests/PythonEditTests.cs | 167 +++++++++++++++ 4 files changed, 400 insertions(+), 3 deletions(-) create mode 100644 test/DynamoCoreWpfTests/PythonNodeRemovePortWarningTests.cs diff --git a/src/DynamoCoreWpf/Controls/DynamoNodeButton.cs b/src/DynamoCoreWpf/Controls/DynamoNodeButton.cs index 0dc564c6a99..fb228be82b3 100644 --- a/src/DynamoCoreWpf/Controls/DynamoNodeButton.cs +++ b/src/DynamoCoreWpf/Controls/DynamoNodeButton.cs @@ -68,11 +68,15 @@ private void OnDynamoNodeButtonClick(object sender, RoutedEventArgs e) { // Only show the prompt if it is a Python node var nodeVM = (sender as DynamoNodeButton)?.DataContext as NodeViewModel; - if (nodeVM?.NodeModel is PythonNodeModels.PythonNode) - { + if (nodeVM?.NodeModel is PythonNodeModels.PythonNode pythonNode) + { MessageBoxResult result = MessageBoxResult.None; - if (eventName.Equals("RemoveInPort") && ShowWarningForRemovingInPort) + // Removing an input port always removes the last one, so only warn when that + // port carries custom properties that the user would actually lose. This also + // suppresses the prompt when there is no port left to remove. + if (eventName.Equals("RemoveInPort") && ShowWarningForRemovingInPort + && pythonNode.HasCustomInputPortProperties(pythonNode.InPorts.Count - 1)) { result = MessageBoxService.Show ( diff --git a/src/Libraries/PythonNodeModels/PythonNode.cs b/src/Libraries/PythonNodeModels/PythonNode.cs index ddbc628d991..4f0b2d6aad5 100644 --- a/src/Libraries/PythonNodeModels/PythonNode.cs +++ b/src/Libraries/PythonNodeModels/PythonNode.cs @@ -128,6 +128,35 @@ protected override string GetInputTooltip(int index) return "Input #" + index; } + /// + /// Returns true if the input port at has a name or tooltip that + /// differs from the auto-generated default for that index, i.e. the user renamed the port + /// or edited its description through the port context menu. Used to decide whether removing + /// the port would actually discard anything the user configured. + /// Returns false for an out-of-range index, so callers can pass the index of a port that + /// does not exist (for example when there are no input ports left to remove). + /// + /// Index of the input port to inspect. + /// True when the port carries user-customized properties. + internal bool HasCustomInputPortProperties(int index) + { + if (index < 0 || index >= InPorts.Count) + { + return false; + } + + // For PythonNode, port i is created with GetInputName(i)/GetInputTooltip(i), so any + // difference from those defaults is a user edit. + // This does NOT hold for PythonStringNode: it prepends a fixed "script" port and + // overrides GetInputIndex to subtract one, so its port i carries the defaults for + // i - 1 and EVERY untouched port would be reported as customized. That is inert only + // because the sole caller narrows to PythonNode; widening it to PythonNodeBase + // requires correcting the index mapping here first. + var port = InPorts[index]; + return !string.Equals(port.Name, GetInputName(index), StringComparison.Ordinal) + || !string.Equals(port.ToolTip, GetInputTooltip(index), StringComparison.Ordinal); + } + protected AssociativeNode CreateOutputAST( AssociativeNode codeInputNode, List inputAstNodes, List> additionalBindings) diff --git a/test/DynamoCoreWpfTests/PythonNodeRemovePortWarningTests.cs b/test/DynamoCoreWpfTests/PythonNodeRemovePortWarningTests.cs new file mode 100644 index 00000000000..4a866c92cde --- /dev/null +++ b/test/DynamoCoreWpfTests/PythonNodeRemovePortWarningTests.cs @@ -0,0 +1,197 @@ +using System.Linq; +using System.Windows; +using System.Windows.Controls.Primitives; +using Dynamo.Controls; +using Dynamo.Models; +using Dynamo.Nodes; +using Dynamo.Utilities; +using Dynamo.Wpf.Utilities; +using DynamoCoreWpfTests.Utility; +using Moq; +using NUnit.Framework; +using PythonNodeModels; + +namespace DynamoCoreWpfTests +{ + /// + /// Covers the "Remove Port?" warning raised by the '-' button on a Python node. + /// The warning must appear only when the port being removed carries custom properties + /// that the user would lose, rather than on every removal. See DYN-10569. + /// + /// + /// Not tagged "UnitTests": each case starts a full DynamoView via DynamoTestUIBase and takes + /// minutes. The comparison logic itself is covered cheaply by the HasCustomInputPortProperties + /// tests in DynamoPythonTests; this fixture exists to verify the button actually consults it. + /// + [Category("RegressionTests")] + public class PythonNodeRemovePortWarningTests : DynamoTestUIBase + { + private Mock dialogMock; + + [TearDown] + public void ResetMessageBoxOverride() + { + // The override is a static field on MessageBoxService, so it would otherwise + // stay installed for every fixture that runs after this one. + MessageBoxService.OverrideMessageBoxDuringTests(null); + dialogMock = null; + } + + /// + /// Installs a recording message box that answers to any prompt, + /// so a warning does not block the test and can be asserted on afterwards. + /// + /// The result the mocked dialog returns, defaulting to OK. + private void InstallDialogMock(MessageBoxResult answer = MessageBoxResult.OK) + { + dialogMock = new Mock(); + dialogMock + .Setup(m => m.Show(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny())) + .Returns(answer); + + MessageBoxService.OverrideMessageBoxDuringTests(dialogMock.Object); + } + + /// + /// Adds a Python node to the current workspace and returns its realized NodeView. + /// + private NodeView CreatePythonNodeView(out PythonNode pythonNode) + { + pythonNode = new PythonNode(); + Model.ExecuteCommand(new DynamoModel.CreateNodeCommand(pythonNode, 0, 0, true, false)); + DispatcherUtil.DoEventsLoop(); + + return NodeViewOf(); + } + + /// + /// Returns the '-' button that VariableInputNodeViewCustomization adds to the node view. + /// + private static DynamoNodeButton RemovePortButton(NodeView nodeView) + { + var button = nodeView.inputGrid.ChildrenOfType() + .SingleOrDefault(b => "-".Equals(b.Content)); + + Assert.IsNotNull(button, "Expected a single '-' button on the Python node view."); + return button; + } + + private static void ClickButton(DynamoNodeButton button) + { + button.RaiseEvent(new RoutedEventArgs(ButtonBase.ClickEvent)); + DispatcherUtil.DoEventsLoop(); + } + + private void AssertWarningShown(Times times) + { + dialogMock.Verify(m => m.Show(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny()), times); + } + + [Test] + public void WhenRemovingUnmodifiedPythonPortThenNoWarningIsShown() + { + // Arrange: a Python node with its single default input port, left untouched. + InstallDialogMock(); + var nodeView = CreatePythonNodeView(out var pythonNode); + Assert.AreEqual(1, pythonNode.InPorts.Count); + + // Act: click the '-' button. + ClickButton(RemovePortButton(nodeView)); + + // Assert: nothing was customized, so the user is not prompted and the port just goes. + AssertWarningShown(Times.Never()); + Assert.AreEqual(0, pythonNode.InPorts.Count); + } + + [Test] + public void WhenRemovingRenamedPythonPortThenWarningIsShown() + { + // Arrange: a Python node whose last input port has been renamed by the user. + InstallDialogMock(); + var nodeView = CreatePythonNodeView(out var pythonNode); + pythonNode.InPorts[0].Name = "myInput"; + + // Act: click the '-' button. + ClickButton(RemovePortButton(nodeView)); + + // Assert: the user is warned before the rename is discarded. + AssertWarningShown(Times.Once()); + } + + [Test] + public void WhenRemovePortWarningIsCancelledThenPortIsKept() + { + // Arrange: a renamed port, so clicking '-' raises the warning. The mocked dialog + // answers Cancel, standing in for the user declining. + InstallDialogMock(MessageBoxResult.Cancel); + var nodeView = CreatePythonNodeView(out var pythonNode); + pythonNode.InPorts[0].Name = "myInput"; + + // Act: click the '-' button and decline the warning. + ClickButton(RemovePortButton(nodeView)); + + // Assert: declining aborts the removal outright - the port and its custom name survive. + // Without this, nothing verifies that Cancel is honoured rather than ignored. + AssertWarningShown(Times.Once()); + Assert.AreEqual(1, pythonNode.InPorts.Count); + Assert.AreEqual("myInput", pythonNode.InPorts[0].Name); + } + + [Test] + public void WhenRemovingUnmodifiedLastPortWhileEarlierPortIsRenamedThenNoWarningIsShown() + { + // Arrange: two input ports where only the FIRST is renamed. The '-' button removes the + // LAST port, which is untouched, so no customization is actually at risk. + InstallDialogMock(); + var nodeView = CreatePythonNodeView(out var pythonNode); + pythonNode.HandleModelEvent("AddInPort", 0, null); + DispatcherUtil.DoEventsLoop(); + Assert.AreEqual(2, pythonNode.InPorts.Count); + pythonNode.InPorts[0].Name = "myInput"; + + // Act: click the '-' button. + ClickButton(RemovePortButton(nodeView)); + + // Assert: the gate must inspect the port being removed rather than a fixed index, + // so an unrelated rename on an earlier port must not raise the warning. + AssertWarningShown(Times.Never()); + Assert.AreEqual(1, pythonNode.InPorts.Count); + } + + [Test] + public void WhenRemovingRenamedLastPortWhileEarlierPortIsDefaultThenWarningIsShown() + { + // Arrange: two input ports where only the LAST one - the one that will be removed - + // is renamed. This is the mirror of the test above. + InstallDialogMock(); + var nodeView = CreatePythonNodeView(out var pythonNode); + pythonNode.HandleModelEvent("AddInPort", 0, null); + DispatcherUtil.DoEventsLoop(); + pythonNode.InPorts[1].Name = "myInput"; + + // Act: click the '-' button. + ClickButton(RemovePortButton(nodeView)); + + // Assert: the user is warned before the rename on the last port is discarded. + AssertWarningShown(Times.Once()); + } + + [Test] + public void WhenRemovingRetooltippedPythonPortThenWarningIsShown() + { + // Arrange: a customized description alone must also trigger the warning, so that + // the gate cannot be narrowed to only check the port name. + InstallDialogMock(); + var nodeView = CreatePythonNodeView(out var pythonNode); + pythonNode.InPorts[0].ToolTip = "my description"; + + // Act: click the '-' button. + ClickButton(RemovePortButton(nodeView)); + + // Assert: the user is warned before the description is discarded. + AssertWarningShown(Times.Once()); + } + } +} diff --git a/test/Libraries/DynamoPythonTests/PythonEditTests.cs b/test/Libraries/DynamoPythonTests/PythonEditTests.cs index f5b2cc6d842..5e299800761 100644 --- a/test/Libraries/DynamoPythonTests/PythonEditTests.cs +++ b/test/Libraries/DynamoPythonTests/PythonEditTests.cs @@ -1000,5 +1000,172 @@ public void WhenPythonStringNodeCopied_CustomPortNamesAndTooltipsArePreserved() Assert.AreEqual("stringOutput", copiedNode.OutPorts[0].Name); Assert.AreEqual("stringOutputTip", copiedNode.OutPorts[0].ToolTip); } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortUnmodifiedThenHasCustomInputPortPropertiesIsFalse() + { + // Arrange: a PythonNode with TWO default input ports. The second port is essential: + // with a single port, index 0 and InPorts.Count - 1 are the same value, so nothing + // would catch the index being mis-threaded into GetInputName/GetInputTooltip. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + pythonNode.HandleModelEvent("AddInPort", 0, null); + + // Act & Assert: nothing would be lost by removing either port, so neither reports custom properties. + Assert.AreEqual(2, pythonNode.InPorts.Count); + Assert.IsFalse(pythonNode.HasCustomInputPortProperties(0)); + Assert.IsFalse(pythonNode.HasCustomInputPortProperties(1)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortRenamedThenHasCustomInputPortPropertiesIsTrue() + { + // Arrange: a PythonNode with two default input ports. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + pythonNode.HandleModelEvent("AddInPort", 0, null); + + // Act: rename only the last port, as the Rename Port dialog does. + pythonNode.InPorts[1].Name = "myInput"; + + // Assert: the renamed port reports custom properties, the untouched one does not. + Assert.IsTrue(pythonNode.HasCustomInputPortProperties(1)); + Assert.IsFalse(pythonNode.HasCustomInputPortProperties(0)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortTooltipEditedThenHasCustomInputPortPropertiesIsTrue() + { + // Arrange: a PythonNode with a single default input port. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + + // Act: edit only the description, leaving the default port name in place. + pythonNode.InPorts[0].ToolTip = "my description"; + + // Assert: a customized description alone is enough to count as custom properties. + Assert.IsTrue(pythonNode.HasCustomInputPortProperties(0)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortRenamedByCaseOnlyThenHasCustomInputPortPropertiesIsTrue() + { + // Arrange: a PythonNode with a single default input port named "IN[0]". + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + + // Act: change only the casing of the default port name. + pythonNode.InPorts[0].Name = "in[0]"; + + // Assert: the name comparison is deliberately case-sensitive. "in[0]" is a name the + // user typed and would lose on removal, so a case-only rename is still a customization. + Assert.IsTrue(pythonNode.HasCustomInputPortProperties(0)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortTooltipEditedByCaseOnlyThenHasCustomInputPortPropertiesIsTrue() + { + // Arrange: a PythonNode with a single default input port tooltipped "Input #0". + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + + // Act: change only the casing of the default description. + pythonNode.InPorts[0].ToolTip = "input #0"; + + // Assert: the tooltip comparison is case-sensitive for the same reason as the name. + // Pinned separately so weakening either comparison alone fails a test. + Assert.IsTrue(pythonNode.HasCustomInputPortProperties(0)); + } + + // A freshly created PythonNode has exactly one input port, so the only valid index is 0. + // -1 and 1 are the boundaries either side of that range and pin the comparison operators: + // weakening >= to > would let index 1 through to InPorts[1]. -5 and 6 sit well outside and + // pin the guard to the whole invalid range, so narrowing it to equality checks is caught. + // Parameterized rather than four asserts in one test so a failure names the offending index. + [TestCase(-5)] + [TestCase(-1)] + [TestCase(1)] + [TestCase(6)] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonInputPortIndexOutOfRangeThenHasCustomInputPortPropertiesIsFalse(int index) + { + // Arrange: a PythonNode with a single default input port. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + Assert.AreEqual(1, pythonNode.InPorts.Count, + "The test indices assume a single default input port."); + + // Act & Assert: out-of-range indices report false rather than throwing. + Assert.IsFalse(pythonNode.HasCustomInputPortProperties(index)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenPythonNodeHasNoInputPortsThenHasCustomInputPortPropertiesIsFalse() + { + // Arrange: a PythonNode whose only input port has been removed. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + pythonNode.HandleModelEvent("RemoveInPort", 0, null); + Assert.AreEqual(0, pythonNode.InPorts.Count); + + // Act & Assert: with no ports left there is nothing to remove and nothing to lose. + Assert.IsFalse(pythonNode.HasCustomInputPortProperties(pythonNode.InPorts.Count - 1)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenUnmodifiedPortRoundTrippedThenHasCustomInputPortPropertiesIsFalse() + { + // Arrange: a PythonNode with a default input port. Guards the reopened-graph case, + // where serialized default names and tooltips must still read as non-custom. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + + // Act: round-trip through save serialization. + var xmlDoc = new System.Xml.XmlDocument(); + var xmlElement = pythonNode.Serialize(xmlDoc, Dynamo.Graph.SaveContext.Save); + var restoredNode = ViewModel.Model.NodeFactory.CreateNodeFromXml( + xmlElement, Dynamo.Graph.SaveContext.Save, ViewModel.CurrentSpace.ElementResolver) as PythonNode; + + // Assert: the restored default port is still not customized. + Assert.IsNotNull(restoredNode); + Assert.IsFalse(restoredNode.HasCustomInputPortProperties(0)); + } + + [Test] + [Category("UnitTests")] + [Category("RegressionTests")] + public void WhenRenamedPortRoundTrippedThenHasCustomInputPortPropertiesIsTrue() + { + // Arrange: a PythonNode with a renamed and re-described input port. + var pythonNode = new PythonNode(); + ViewModel.CurrentSpace.AddAndRegisterNode(pythonNode); + pythonNode.InPorts[0].Name = "savedInput"; + pythonNode.InPorts[0].ToolTip = "savedInputTip"; + + // Act: round-trip through save serialization. + var xmlDoc = new System.Xml.XmlDocument(); + var xmlElement = pythonNode.Serialize(xmlDoc, Dynamo.Graph.SaveContext.Save); + var restoredNode = ViewModel.Model.NodeFactory.CreateNodeFromXml( + xmlElement, Dynamo.Graph.SaveContext.Save, ViewModel.CurrentSpace.ElementResolver) as PythonNode; + + // Assert: the customization survives the round-trip and is still detected. + Assert.IsNotNull(restoredNode); + Assert.IsTrue(restoredNode.HasCustomInputPortProperties(0)); + } } }