From cca575705915704648e54f0fbc37a4357c4c354e Mon Sep 17 00:00:00 2001 From: Wilhelm Behncke Date: Thu, 15 Feb 2024 14:38:59 +0100 Subject: [PATCH 1/6] TASK: Add E2E test fixture for #3184 --- .../Fixtures/1Dimension/issue-3184.e2e.js | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 Tests/IntegrationTests/Fixtures/1Dimension/issue-3184.e2e.js diff --git a/Tests/IntegrationTests/Fixtures/1Dimension/issue-3184.e2e.js b/Tests/IntegrationTests/Fixtures/1Dimension/issue-3184.e2e.js new file mode 100644 index 0000000000..8e8d20e7b2 --- /dev/null +++ b/Tests/IntegrationTests/Fixtures/1Dimension/issue-3184.e2e.js @@ -0,0 +1,150 @@ +import {beforeEach, checkPropTypes} from './../../utils.js'; +import {Page, PublishDropDown} from './../../pageModel'; +import { Selector } from 'testcafe'; + +/* global fixture:true */ + +// +// Original Issue: https://github.com/neos/neos-ui/issues/3184 +// +fixture`FIX #3184: Discarded node move changes are reflected correctly in the document tree` + .beforeEach(beforeEach) + .afterEach(() => checkPropTypes()); + +// +// This is an excerpt of the document tree in our E2E test distribution, +// stripped down to only show the relevant document nodes for this test +// scenario: +// +// 🗋 Home +// ├─ 🗋 Discarding +// └─ 🗋 Tree multiselect +// ├─ 🗋 MultiA +// ├─ 🗋 MultiB +// └─ 🗋 MultiC +// +// In reference to that hierarchy, we're putting some selectors into variables +// for later use in the concrete test cases, so we don't have to repeat the +// long form over and over: +// +const Discarding = Page.getTreeNodeButton('Discarding'); +const MultiA = Page.getTreeNodeButton('MultiA'); +const MultiB = Page.getTreeNodeButton('MultiB'); +const MultiC = Page.getTreeNodeButton('MultiC'); +const withCmdClick = { + modifiers: { + ctrl: true + } +}; + +test( + 'Scenario #1: Moving nodes and then discarding that change does not lead to an error', + async t => { + // + // Select 🗋 MultiA and 🗋 MultiB, then drag both documents INTO 🗋 MultiC. + // + await t + .click(MultiA) + .click(MultiB, withCmdClick) + .dragToElement(MultiA, MultiC); + + // + // Discard that change. + // + await PublishDropDown.discardAll(); + + // + // Assert that no error flash message shows up. + // + await t + .expect(Selector('[role="alert"][class*="error"]').exists) + .notOk(); + } +); + +test( + 'Scenario #2: Moved nodes do not just disappear after discarding the move change', + async t => { + // + // Select 🗋 MultiA and 🗋 MultiB, then drag both documents INTO 🗋 Discarding. + // + await t + .click(MultiA) + .click(MultiB, withCmdClick) + .dragToElement(MultiA, Discarding); + + // + // Go to 🗋 Tree multiselect, so we can check for stale metadata + // coming from the guest frame. + // We also need to reload to avoid Scenario #1. + // + await Page.goToPage('Tree multiselect'); + await t.eval(() => location.reload(true)); + await Page.waitForIframeLoading(); + + // + // Discard the move change and wait for the guest frame to reload plus + // some extra timeout, so there's a chance for stale metadata to + // overwrite the tree state. + // + await PublishDropDown.discardAll(); + await Page.waitForIframeLoading(); + await t.wait(500); + + // + // Assert that 🗋 MultiA and 🗋 MultiB can still be found. + // + await t + .expect(MultiA.exists) + .ok('🗋 MultiA has disappeared'); + await t + .expect(MultiB.exists) + .ok('🗋 MultiB has disappeared'); + } +) + +test( + 'Scenario #3: Discarding a move change while being on a moved node does not' + + ' lead to an error in the guest frame', + async t => { + // + // Select 🗋 MultiA and 🗋 MultiB, then drag both documents INTO 🗋 MultiC. + // + await t + .click(MultiA) + .click(MultiB, withCmdClick) + .dragToElement(MultiA, MultiC); + + // + // Go to 🗋 Home and reload the backend, so we avoid Scenario #1's + // underlying issue. + // + await Page.goToPage('Home'); + await t.eval(() => location.reload(true)); + + // + // Go to 🗋 MultiA, so we are on a moved node in the guest frame. + // + await Page.goToPage('MultiA'); + + // + // Discard the move change. + // + await PublishDropDown.discardAll(); + + // + // Assert that there's no error showing up in the guest frame and + // that we're instead seeing the next-higher document node. + // + await Page.waitForIframeLoading(); + await t.switchToIframe('[name="neos-content-main"]'); + await t + .expect(Selector('.neos-error-screen').exists) + .notOk('There\'s an unexpected error screen in the guest frame.'); + + await t.switchToMainWindow(); + await t + .expect(Selector('[role="treeitem"] [role="button"][class*="isFocused"]').textContent) + .eql('MultiC'); + } +); From df239206401b395e67a1fd5ed452f4e6fdf93d3e Mon Sep 17 00:00:00 2001 From: Wilhelm Behncke Date: Tue, 23 May 2023 17:15:14 +0200 Subject: [PATCH 2/6] BUGFIX: Provide better error message if node could not be found on discard --- Classes/Controller/BackendServiceController.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Classes/Controller/BackendServiceController.php b/Classes/Controller/BackendServiceController.php index f78bc2d661..b9f1eca37d 100644 --- a/Classes/Controller/BackendServiceController.php +++ b/Classes/Controller/BackendServiceController.php @@ -276,6 +276,14 @@ public function discardAction(array $nodeContextPaths): void try { foreach ($nodeContextPaths as $contextPath) { $node = $this->nodeService->getNodeFromContextPath($contextPath, null, null, true); + if (!$node) { + $error = new Error(); + $error->setMessage(sprintf('Could not find node for context path "%s"', $contextPath)); + + $this->feedbackCollection->add($error); + continue; + } + if ($node->isRemoved() === true) { // When discarding node removal we should re-create it $updateNodeInfo = new UpdateNodeInfo(); From 859c1f23d3db1666706c091e5394f70c90c92f9c Mon Sep 17 00:00:00 2001 From: Wilhelm Behncke Date: Tue, 23 May 2023 17:17:35 +0200 Subject: [PATCH 3/6] BUGFIX: Properly update node info after node move changes have been discarded --- .../ContentRepository/Service/NodeService.php | 19 ++++++++++++++++++- .../Controller/BackendServiceController.php | 14 +++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/Classes/ContentRepository/Service/NodeService.php b/Classes/ContentRepository/Service/NodeService.php index 5547b239b8..49ccb8492a 100644 --- a/Classes/ContentRepository/Service/NodeService.php +++ b/Classes/ContentRepository/Service/NodeService.php @@ -128,11 +128,28 @@ public function getNodeFromContextPath($contextPath, Site $site = null, Domain $ * @return boolean */ public function nodeExistsInWorkspace(NodeInterface $node, Workspace $workspace) + { + return $this->getNodeInWorkspace($node, $workspace) !== null; + } + + /** + * Get the variant of the given node in the given workspace + * + * @param NodeInterface $node + * @param Workspace $workspace + * @return NodeInterface|null + */ + public function getNodeInWorkspace(NodeInterface $node, Workspace $workspace): ?NodeInterface { $context = ['workspaceName' => $workspace->getName()]; $flowQuery = new FlowQuery([$node]); - return $flowQuery->context($context)->count() > 0; + $result = $flowQuery->context($context); + if ($result->count() > 0) { + return $result->get(0); + } else { + return null; + } } /** diff --git a/Classes/Controller/BackendServiceController.php b/Classes/Controller/BackendServiceController.php index b9f1eca37d..242a73bf4a 100644 --- a/Classes/Controller/BackendServiceController.php +++ b/Classes/Controller/BackendServiceController.php @@ -305,7 +305,19 @@ public function discardAction(array $nodeContextPaths): void $reloadDocument = new ReloadDocument(); $this->feedbackCollection->add($reloadDocument); } - } elseif (!$this->nodeService->nodeExistsInWorkspace($node, $node->getWorkSpace()->getBaseWorkspace())) { + } elseif ($nodeInBaseWorkspace = $this->nodeService->getNodeInWorkspace($node, $node->getWorkSpace()->getBaseWorkspace())) { + $nodeHasBeenMoved = $node->getPath() !== $nodeInBaseWorkspace->getPath(); + if ($nodeHasBeenMoved) { + $removeNode = new RemoveNode(); + $removeNode->setNode($node); + $this->feedbackCollection->add($removeNode); + + $updateNodeInfo = new UpdateNodeInfo(); + $updateNodeInfo->setNode($nodeInBaseWorkspace); + $updateNodeInfo->recursive(); + $this->feedbackCollection->add($updateNodeInfo); + } + } else { // If the node doesn't exist in the target workspace, tell the UI to remove it $removeNode = new RemoveNode(); $removeNode->setNode($node); From 2445c2a7525c11578c9d52f455ac3b7a56ac0e74 Mon Sep 17 00:00:00 2001 From: Wilhelm Behncke Date: Wed, 24 May 2023 20:41:12 +0200 Subject: [PATCH 4/6] BUGFIX: Use omniscient content context configuration for node changes --- Classes/ContentRepository/Service/NodeService.php | 1 + Classes/TypeConverter/ChangeCollectionConverter.php | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Classes/ContentRepository/Service/NodeService.php b/Classes/ContentRepository/Service/NodeService.php index 49ccb8492a..0e725bb277 100644 --- a/Classes/ContentRepository/Service/NodeService.php +++ b/Classes/ContentRepository/Service/NodeService.php @@ -103,6 +103,7 @@ public function getNodeFromContextPath($contextPath, Site $site = null, Domain $ if ($includeAll === true) { $contextProperties['invisibleContentShown'] = true; $contextProperties['removedContentShown'] = true; + $contextProperties['inaccessibleContentShown'] = true; } $context = $this->contextFactory->create( diff --git a/Classes/TypeConverter/ChangeCollectionConverter.php b/Classes/TypeConverter/ChangeCollectionConverter.php index c76b48eb96..3ef27ca101 100644 --- a/Classes/TypeConverter/ChangeCollectionConverter.php +++ b/Classes/TypeConverter/ChangeCollectionConverter.php @@ -137,7 +137,7 @@ protected function convertChangeData($changeData) $changeClassInstance->injectPersistenceManager($this->persistenceManager); $subjectContextPath = $changeData['subject']; - $subject = $this->nodeService->getNodeFromContextPath($subjectContextPath); + $subject = $this->nodeService->getNodeFromContextPath($subjectContextPath, null, null, true); if ($subject instanceof Error) { return $subject; From 302da1d18074efeae17d67a0cc584f60d49c410c Mon Sep 17 00:00:00 2001 From: Wilhelm Behncke Date: Thu, 15 Feb 2024 14:44:50 +0100 Subject: [PATCH 5/6] TASK: Temporarily patch test distribution with neos/neos-development-collection#4291 This needs to be undone before merge --- .../TestDistribution/composer.json | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Tests/IntegrationTests/TestDistribution/composer.json b/Tests/IntegrationTests/TestDistribution/composer.json index 41fc86012c..5dab4bb15e 100644 --- a/Tests/IntegrationTests/TestDistribution/composer.json +++ b/Tests/IntegrationTests/TestDistribution/composer.json @@ -6,15 +6,27 @@ "vendor-dir": "Packages/Libraries", "bin-dir": "bin", "allow-plugins": { - "neos/composer-plugin": true + "neos/composer-plugin": true, + "cweagans/composer-patches": true } }, "minimum-stability": "dev", "require": { + "neos/neos-development-collection": "8.3.x-dev", "neos/neos-ui": "8.3.x-dev", "neos/fusion-afx": "*", "neos/test-site": "@dev", - "neos/test-nodetypes": "@dev" + "neos/test-nodetypes": "@dev", + + "cweagans/composer-patches": "^1.7.3" + }, + "extra": { + "patches": { + "neos/neos-development-collection": { + "BUGFIX: Invalidate caches correctly after node move changes have been discarded": + "https://github.com/neos/neos-development-collection/pull/4291.patch" + } + } }, "require-dev": { "neos/buildessentials": "@dev", From 95a370e93f8e487006d8ab5c60b06630232881da Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Tue, 14 May 2024 09:25:35 +0200 Subject: [PATCH 6/6] TASK: Remove obsolete patch for testing --- Tests/IntegrationTests/TestDistribution/composer.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tests/IntegrationTests/TestDistribution/composer.json b/Tests/IntegrationTests/TestDistribution/composer.json index 5dab4bb15e..deaf995344 100644 --- a/Tests/IntegrationTests/TestDistribution/composer.json +++ b/Tests/IntegrationTests/TestDistribution/composer.json @@ -23,8 +23,6 @@ "extra": { "patches": { "neos/neos-development-collection": { - "BUGFIX: Invalidate caches correctly after node move changes have been discarded": - "https://github.com/neos/neos-development-collection/pull/4291.patch" } } },