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.

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

Jump to bottom

BUGFIX: Don’t copy removed nodes #5186

Merged
merged 1 commit into from
Aug 14, 2024
Merged

BUGFIX: Don’t copy removed nodes #5186

merged 1 commit into from
Aug 14, 2024

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Jul 25, 2024

With this change, removed nodes are not copied anymore in the recursive copy actions.

Also the removed state is now kept when a node is similarisied to prevent unpublished removed nodes from popping up again inadvertendly.

Resolves: #5185

@Sebobo Sebobo force-pushed the bugfix/copy-removed-nodes branch from d5687fd to 0a8de51 Compare July 25, 2024 12:14
@dlubitz dlubitz self-requested a review August 13, 2024 09:17
dlubitz
dlubitz approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Works like charm. Thank you

dlubitz
dlubitz reviewed Aug 13, 2024
View reviewed changes
Neos.ContentRepository/Classes/Domain/Model/NodeData.php
];
if (!$isCopy) {
$propertyNames[] = 'creationDateTime';
$propertyNames[] = 'lastModificationDateTime';
}
if ($sourceNode instanceof NodeData) {
$propertyNames[] = 'index';
$propertyNames[] = 'removed';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a scenatio, where this change made a difference. But I guess it's fine to copy the property here too.

bwaidelich
bwaidelich reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that this change to a central logic of our central domain object does not make any test fail leaves me startled.
Feel free to ignore my admonitory words, but maybe we should add a test to cover the new behavior at least?

@kitsunet
Copy link
Member

Test woudl be great, I am also still straining my brain as to why we did copy them in the first place, wondering if there is/was a good reason, but I raelly cna't come up with anything, so this seems valid.

@Sebobo
BUGFIX: Don’t copy removed nodes
6936b7b 
With this change, removed nodes are not copied anymore
in the recursive copy actions. Also the removed state is kept
when a node is similarisied to prevent unpublished removed
nodes from popping up again inadvertendly.

Resolves: #5185
@Sebobo Sebobo force-pushed the bugfix/copy-removed-nodes branch from 0a8de51 to 6936b7b Compare August 14, 2024 10:13
@Sebobo
Copy link
Member Author

Sebobo commented Aug 14, 2024

Added the test.

Took me some time to get the test failing without the change, as removedContentShown was required for the context to behave like the context created when publishing.

@dlubitz
Copy link
Contributor

dlubitz commented Aug 14, 2024

@kitsunet @bwaidelich Can we merge it now, with the new added tests?

@bwaidelich
Copy link
Member

bwaidelich commented Aug 14, 2024 •
edited
Loading

Thanks for adding a test! Unfortunately that test does not fail for me if I revert the fix
edit: more precisely: To pass the test, I only need to include the NodeData-change

@bwaidelich
Copy link
Member

@Sebobo @dlubitz I tried to adjust the test, but I don't understand what's happening. And while trying to reproduce the issue I realized that I can't reproduce it with Neos.Neos.UI at commit 76da97ba7. Unfortunately there are quite some changes in the meantime – I'll try to pin point it via git bisect

@bwaidelich
Copy link
Member

I'll try to pin point it via git bisect

I don't know why and how, but this seems to be the "culprit": neos/neos-ui@2445c2a

With:

cd Packages/Application/Neos.Neos.Ui
git checkout 2445c2a75

I can reproduce the bug. Checkout out the previous commit:

git checkout 859c1f23d

..I can't.

(not saying that this does not have to be fixed in the CR core, but I don't have all the information to +1/-1 this yet)

@dlubitz
Copy link
Contributor

dlubitz commented Aug 14, 2024

You are right. I was not using latest Neos.Neos.UI while I was testing it. If I checkout latest version, the deleted nodes don't appear in the copy even without this change 🤔 .

@bwaidelich
Copy link
Member

@dlubitz It seems like the change was only applied to 9.0.0-beta10 8.3.9 and 8.3.8 yet

@Sebobo
Copy link
Member Author

Sebobo commented Aug 14, 2024

Thanks for adding a test! Unfortunately that test does not fail for me if I revert the fix edit: more precisely: To pass the test, I only need to include the NodeData-change

You only need one of the two changes. At first I did the nodedata change, and afterwards thought, why should removed nodes even be copied.

kitsunet
kitsunet approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for digging into it. Looks good to me.

@bwaidelich bwaidelich merged commit 2fe0915 into 8.3 Aug 14, 2024
9 checks passed
@bwaidelich bwaidelich deleted the bugfix/copy-removed-nodes branch August 14, 2024 19:50
@bwaidelich
Copy link
Member

Jep, thanks for taking care.
I still wonder whether the UI change that made the bug surface should be reverted/adjusted

@mhsdesign
Copy link
Member

Seems like a legit question tbh there still is some confusion 😅

Luckily neos/neos-ui#3503 which is the pr containing said 2445c2a in the Ui introduced a couple of additional e2e tests:

 FIX #3184: Discarded node move changes are reflected correctly in the document tree
 ✓ Scenario #1: Moving nodes and then discarding that change does not lead to an error
 ✓ Scenario #2: Moved nodes do not just disappear after discarding the move change
 ✓ Scenario #3: Discarding a move change while being on a moved node does not lead to an error in the guest frame

So when running them in all combinations we get the following result concluding that the commit is still required.

Tests Neos Cr Ui
✅ without this with 2445c2a
✅ with this with 2445c2a
❌ Scenario #1 fails * without this without 2445c2a
❌ Scenario #1 fails with this without 2445c2a

* (expected obviously)

error encountered in the tests:

image

See you later alligator

@Sebobo
Copy link
Member Author

Sebobo commented Aug 15, 2024

@mhsdesign so you are saying everything is fine now or not? 🤔

@mhsdesign
Copy link
Member

Yes sorry the conclusion from my tests is that this that based on the provided e2e tests that came with said commit i could not observe any undesired or obsolete behaviour. Everything seems fine as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlubitz dlubitz dlubitz approved these changes

@bwaidelich bwaidelich bwaidelich left review comments

@kitsunet kitsunet kitsunet approved these changes

@mhsdesign mhsdesign Awaiting requested review from mhsdesign

Assignees
No one assigned
Labels
8.3 Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Sebobo @kitsunet @dlubitz @bwaidelich @mhsdesign