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

BUG: 9.0 - Translating document nodes leads to wrong ordering #4742

Closed
11 tasks done
skurfuerst opened this issue Nov 12, 2023 · 4 comments · Fixed by #4975 · May be fixed by #4749
Closed
11 tasks done

BUG: 9.0 - Translating document nodes leads to wrong ordering #4742

skurfuerst opened this issue Nov 12, 2023 · 4 comments · Fixed by #4975 · May be fixed by #4749
Assignees
Milestone

Comments

@skurfuerst
Copy link
Member

skurfuerst commented Nov 12, 2023

Currently the ordering behavior of the CR is undefined for the following scenarios:

  • CreateNodeAggregateWithNode with fallbacks
  • Create Peer Variant
  • Create Generalization

This leads to the problem that different projections implement ordering differently then; leading to subtle bugs.

For instance, the DocumentUriPathProjection needs the ordering as well to find the first child node for FirstChild-shortcuts -- and it currently does it differently than the Content Graph.

That is what I originally wanted to fix; when I stumbled into this rabbit hole ;)

Current Behavior

On the demo site, let's say you have the following english nodes:

  • a
  • b
  • c
  • d

=> now, you translate b to german; leading to the following structure:

  • b

=> now, you translate a to german, leading to the following structure:

  • b
  • a

=> this is wrong, expected would be:

- a
- b

Fixing Plan

  • NodePeerVariantWasCreated
    • highest priority, because it is relevant when simply having a language dimension and starting to translate
    •  extend event to have s.th. like PositionMapping (modeled after MoveNodeMapping)
    • in command handler, use same logic to determine positionMapping as in MoveNode
    • write testcase
    • ensure ordering is respected in graph projection
    • ensure ordering is respected in Routing projection
  • NodeGeneralizationVariantWasCreated
  • NodeAggregateWithNodeWasCreated with fallbacks

NOTE: NodeSpecializationVariantWasCreated does not need to be adjusted, because it only leads to re-wiring of edges, and not to new edges.

@skurfuerst skurfuerst added this to the 9.0 (ES CR) milestone Nov 12, 2023
@skurfuerst skurfuerst self-assigned this Nov 12, 2023
@bwaidelich bwaidelich moved this to Prioritized 🔥 in Neos 9.0 Release Board Nov 13, 2023
@ahaeslich ahaeslich linked a pull request Nov 23, 2023 that will close this issue
6 tasks
@nezaniel
Copy link
Member

nezaniel commented Mar 24, 2024

FYI: I can't detect any issues with only CreateNodeAggregateWithNode alone, which commands are needed for this to fail?
Edit: Edge case found, tested and fixed

@nezaniel
Copy link
Member

nezaniel commented Mar 28, 2024

the content graph part will be fixed in #4961
still missing is the uri path projection part, which will happen in a follow-up

@nezaniel nezaniel self-assigned this Mar 28, 2024
@nezaniel
Copy link
Member

nezaniel commented Apr 3, 2024

follow-up is ready: #4975

@nezaniel
Copy link
Member

nezaniel commented Apr 3, 2024

Also: NodeSpecializationVariantWasCreated needed to be adjusted for edge cases where a succeeding sibling is deleted in a variant that is to be created by variation.

@nezaniel nezaniel moved this from Prioritized 🔥 to Under Review 👀 in Neos 9.0 Release Board Apr 3, 2024
mhsdesign added a commit that referenced this issue Apr 5, 2024
@github-project-automation github-project-automation bot moved this from Under Review 👀 to Done ✅ in Neos 9.0 Release Board Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment