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: CopyNodesRecursively vs Content Dimensions #5054

Open
1 task done
nezaniel opened this issue May 14, 2024 · 4 comments
Open
1 task done

BUG: CopyNodesRecursively vs Content Dimensions #5054

nezaniel opened this issue May 14, 2024 · 4 comments
Assignees

Comments

@nezaniel
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

When we copy a node, it is not possible to copy a variant of said node to the same location without losing the connection

Expected Behavior

We need to find a way to make interdimensional node copy possible, similar to MoveNode

Steps To Reproduce

Create a node
Vary that node to a different DSP
Copy the node in the source DSP
Copy the node in the varied DSP
See how the aggregateIds differ and the copied variants are detached

Environment

- Flow: 9.0.x-dev
- Neos: 9.0.x-dev
- PHP: 8.2

Anything else?

No response

@nezaniel
Copy link
Member Author

nezaniel commented May 14, 2024

To elaborate on this: Given the following variation and content graph:

CopyNodes

When we copy Node "mc-nodeface" in origin DSP {"language": "ltz"} to a new parent "nodington-iii" in the same DSP, the following happens:

CopyNodes-Copy1

The first weird thing is, that "nodimus-copied" is now varied to ltz, where its original did only exist as a virtual variant. That's acceptable though imho.

What troubles me more is what happens if I decide to not only copy the node, but all its variants (which kind of makes sense, even by default). Since I cannot define a DSP set which to copy, I have to do this by hand with a second command:

CopyNodes-Copy2

By default, the nodes again get new - and thus differing - aggregateIds, although they were copied from the same original. We copied one node aggregate into two and there is no way to remediate this (except, of course, with vary, copy&paste content, delete). And if we tried again with the same NodeAggregateId mapping, node creation would fail due to the aggregates already existing.

What could we do instead?
Either we also copy all variants by default or we need the ability to copy via variation instead of creation. In the latter case, we'd need to store the NodeAggregateId mapping $somewhere and on the next copy reevaluate it and perform variation on the target Ids instead of creation.

Questions so far?

@mhsdesign
Copy link
Member

Some of the desired 8.3 behaviour is actually documented in this commit: 02a8a18

($detachedCopy is set if isAggregate() was true in the node type)

Create a recursive copy of this node below $referenceNode with $nodeName.

$detachedCopy only has an influence if we are copying from one dimension to the other, possibly creating a new
node variant:

  • If $detachedCopy is true, the whole (recursive) copy is done without connecting original and copied node,
    so NOT CREATING a new node variant.
  • If $detachedCopy is false, and the node does not yet have a variant in the target dimension, we are CREATING
    a new node variant.

As a caller of this method, $detachedCopy should be true if $this->getNodeType()->isAggregate() is true, and false
otherwise.

Behaviour seemed to be depending on document vs content

@mhsdesign mhsdesign moved this to In Progress 🚧 in Neos 9.0 Release Board Nov 26, 2024
@mhsdesign mhsdesign assigned mhsdesign and unassigned skurfuerst Nov 26, 2024
@mhsdesign mhsdesign moved this from In Progress 🚧 to Blocked in Neos 9.0 Release Board Nov 26, 2024
@mhsdesign mhsdesign moved this from Blocked to Prioritized 🔥 in Neos 9.0 Release Board Dec 2, 2024
@bwaidelich
Copy link
Member

bwaidelich commented Dec 6, 2024

As decided in todays weekly, we plan to:

  • ...disallow copy across dimensions for now
    • the UI needs to be adjusted such that the paste button is disabled in a different dimension (potentially with some hint why that is)
    • as a follow-up the copy/paste process could be made interactive such that the editor can choose to change the dimension, ...
  • ...extend the CreateNodeVariant command (and corresponding events) by an optional "newParentNodeAggregateId" property that allows to create variants with a different parent in one go
    • @nezaniel plans to work on that
    • This will extend the events, i.e. it's not a breaking change
    • Probably only ContentGraph and DocumentUriPath projections need to be adjusted
  • ...create a UI component that allows the editor to create a variant of the currently selected node in arbitrary dimensions
    • This might be a interim solution until we have a proper UI/backend module for variation handling
    • It will only be possible to select a variant that contains the same parent node (wish: display all potential variants but disallow them to be selected with a corresponding explanation, wish2: calculate allowed dimensions lazily when interacting with the component, not on every page load)

mhsdesign added a commit to mhsdesign/neos-ui that referenced this issue Dec 7, 2024
see neos/neos-development-collection#5054 (comment)

Previously the `$targetDimensionSpacePoint` was also wrongly chosen, and it was attempted to paste the node into the subjects home dimension
@mhsdesign
Copy link
Member

with #5403 we might actually be able to reimplement the old 8.3, which i attempted but failed very fast because:

Details

@contentrepository @adapters=DoctrineDBAL
@flowEntities
Feature: Create node aggregate with node with dimensions

  Background:
    Given using the following content dimensions:
      | Identifier | Values    | Generalizations |
      | language   | de,gsw,fr | gsw->de, fr     |
    And using the following node types:
    """yaml
    'Neos.ContentRepository.Testing:Document': {}
    'Neos.ContentRepository.Testing:Content':
      properties:
        text:
          type: string
    """
    And using identifier "default", I define a content repository
    And I am in content repository "default"
    And the command CreateRootWorkspace is executed with payload:
      | Key                  | Value                |
      | workspaceName        | "live"               |
      | workspaceTitle       | "Live"               |
      | workspaceDescription | "The live workspace" |
      | newContentStreamId   | "cs-identifier"      |

    And I am in workspace "live"
    And I am in dimension space point {"language": "de"}
    And the command CreateRootNodeAggregateWithNode is executed with payload:
      | Key             | Value                         |
      | nodeAggregateId | "lady-eleonode-rootford"      |
      | nodeTypeName    | "Neos.ContentRepository:Root" |

    And the following CreateNodeAggregateWithNode commands are executed:
      | nodeAggregateId        | parentNodeAggregateId  | nodeTypeName                            | initialPropertyValues |
      | sir-david-nodenborough | lady-eleonode-rootford | Neos.ContentRepository.Testing:Document | {} |
      | nody-mc-nodeface       | sir-david-nodenborough | Neos.ContentRepository.Testing:Content  | {"text": "german"} |

    Then I am in dimension space point {"language": "fr"}
    And the following CreateNodeAggregateWithNode commands are executed:
      | nodeAggregateId            | parentNodeAggregateId  | nodeTypeName                            |
      | sir-nodeward-nodington-iii | lady-eleonode-rootford | Neos.ContentRepository.Testing:Document |


  Scenario: Paste variant into other spezialisation
    And the command CreateNodeVariant is executed with payload:
      | Key             | Value              |
      | nodeAggregateId | "sir-david-nodenborough" |
      | sourceOrigin    | {"language":"de"} |
      | targetOrigin    | {"language":"fr"}  |

    When insert variant is executed with payload:
      | Key                         | Value               |
      | sourceDimensionSpacePoint   | {"language": "de"}                  |
      | sourceNodeAggregateId       | "nody-mc-nodeface"      |
      | targetDimensionSpacePoint   | {"language": "fr"}                  |
      | targetParentNodeAggregateId | "sir-david-nodenborough"  |

    And I expect the node aggregate "nody-mc-nodeface" to exist
    And I expect this node aggregate to be classified as "regular"
    And I expect this node aggregate to be unnamed
    And I expect this node aggregate to be of type "Neos.ContentRepository.Testing:Content"
    And I expect this node aggregate to occupy dimension space points [{"language": "de"}, {"language": "fr"}]
    And I expect this node aggregate to disable dimension space points []
    And I expect this node aggregate to have no child node aggregates
    And I expect this node aggregate to have the parent node aggregates ["sir-david-nodenborough"]


    And I am in dimension space point {"language": "de"}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{"language": "de"}
    And I expect this node to have the following properties:
      | Key  | Value        |
      | text | "german" |


    And I am in dimension space point {"language": "fr"}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{"language": "fr"}
    And I expect this node to have the following properties:
      | Key  | Value        |
      | text | "german" |

  Scenario: Paste variant into other root dimension
    When insert variant is executed with payload:
      | Key                         | Value               |
      | sourceDimensionSpacePoint   | {"language": "de"}                  |
      | sourceNodeAggregateId       | "nody-mc-nodeface"      |
      | targetDimensionSpacePoint   | {"language": "fr"}                  |
      | targetParentNodeAggregateId | "sir-nodeward-nodington-iii"  |

    And I expect the node aggregate "nody-mc-nodeface" to exist
    And I expect this node aggregate to be classified as "regular"
    And I expect this node aggregate to be unnamed
    And I expect this node aggregate to be of type "Neos.ContentRepository.Testing:Content"
    And I expect this node aggregate to occupy dimension space points [{"language": "de"}, {"language": "fr"}]
    And I expect this node aggregate to disable dimension space points []
    And I expect this node aggregate to have no child node aggregates
    And I expect this node aggregate to have the parent node aggregates ["sir-nodeward-nodington", "sir-nodeward-nodington-iii"]

    And I am in dimension space point {"language": "de"}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{"language": "de"}
    And I expect this node to have the following properties:
      | Key  | Value        |
      | text | "german" |

    And I am in dimension space point {"language": "fr"}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{"language": "fr"}
    And I expect this node to have the following properties:
      | Key  | Value        |
      | text | "german" |

  # todo
  # test recursive behaviour
  # fail if variant exist
  # test generalisation vs spezi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Prioritized 🔥
Development

No branches or pull requests

4 participants