Skip to content

Commit

Permalink
Merge pull request #3503 from grebaldi/bugfix/3184/document-move-discard
Browse files Browse the repository at this point in the history
BUGFIX: Reflect discard of node move changes correctly in the UI
  • Loading branch information
mhsdesign authored May 14, 2024
2 parents 49db1d2 + 95a370e commit 12fa6f4
Show file tree
Hide file tree
Showing 5 changed files with 203 additions and 5 deletions.
20 changes: 19 additions & 1 deletion Classes/ContentRepository/Service/NodeService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -128,11 +129,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;
}
}

/**
Expand Down
22 changes: 21 additions & 1 deletion Classes/Controller/BackendServiceController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -297,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);
Expand Down
2 changes: 1 addition & 1 deletion Classes/TypeConverter/ChangeCollectionConverter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
150 changes: 150 additions & 0 deletions Tests/IntegrationTests/Fixtures/1Dimension/issue-3184.e2e.js
Original file line number Diff line number Diff line change
@@ -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');
}
);
14 changes: 12 additions & 2 deletions Tests/IntegrationTests/TestDistribution/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,25 @@
"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": {
}
}
},
"require-dev": {
"neos/buildessentials": "@dev",
Expand Down

0 comments on commit 12fa6f4

Please sign in to comment.