Skip to content
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.

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

BUGFIX: Recognize removed-state while reducing #5457

Open
wants to merge 1 commit into
base: 8.3
Choose a base branch
from

Conversation

markusguenther
Copy link
Member

@markusguenther markusguenther commented Feb 4, 2025

Please read Issue #3651, to get the problem.

By adding the removed-state as 3rd priority while reducing NodeData results, we should always get the not-removed NodeData, if there are 2 (because of Node-move).

I tested this fix on our projects, and it worked. I can't find something breaking. But since this is deep inside NodeData, I've no idea, if we could get unexpected side-effects on it. Thanks to @Nikdro for the change.

Fixes: #3651

@markusguenther
Copy link
Member Author

markusguenther commented Feb 4, 2025

I was unable to push to @Nikdro branch; therefore I created a new PR with the changes from Niklas.
#3652

Yesterday I stumbled over a similar issue and after checking the code in the Neos-UI @kitsunet led me to the NodeData and NodeDataRepository. As @Nikdro already described, when we move e.g. a node into the own parent page, we create a shadow node that is marked as removed and we have the regular node that is moved and not marked as removed.

For some reason, the NodeDataRepository chooses the shadow node as all are filtered by identifier. The change from Niklas changes the position and so we get the moved node and not the shadow node. It would be better here to check in the UI if the target node is also the parent node and so not create a shadow node, but as this can also have implications these bugfixes seem to be a nice approach.

Behavoir before this change:

Move-node-into-same-parent-with-bug.-.01.mp4

Behavoir after this change:

Move-node-into-same-parent-with-fix.1.-.01.mp4

The only thing I could find was an issue when you move the node into the parent, publish it, and then move the node into the same parent again, without reloading the page. This does not happen when you reload the page after publishing.
So it seems that we have something wrong in the UI.

Move-node-into-same-parent-with-fix-exception-before-reload.1.-.01.mp4

I will rebase this change to Neos 8.3, and then we can hopefully continue with the discussion here. Seems that the only concern was the missing test :) With Neos 9.0, this should be different anyways.

@markusguenther
Copy link
Member Author

Tested the behavior in Neos 9.0 and there we don’t have the issue. Even the last issue with the exception is not happening as the UI reloads the needed components after publishing. 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants