BUGFIX: Fix inconsistencies in document tree with node tree presets #3822

grebaldi · 2024-07-02T17:58:18Z

Add settings during E2E test run

This PR indroduces a mechanism to add additional settings during E2E test run.

This is necessary, because the newly introduced nodeTreePresets.e2e.js test fixture requires node tree presets to be configured. Node tree presets are not active by default, so we need to set up additional settings for this test fixture specifically.

Read details on how this is achieved

The mechanism is achieved by providing two public endpoints in the test distribution:

`POST /test/write-additional-settings`

[!WARNING]
This endpoint is only available in the test distribution and should never be replicated for production use. DO NOT DO THIS IN PRODUCTION!

This endpoint accepts a JSON-encoded object as a body. It's top-most key must be settings. Any value passed to settings will be YAML-serialised and written to FLOW_PATH_CONFIGURATION/Settings.Additional.yaml.

This endpoint then also registers a signal handler for bootstrapShuttingDown, in which it will flush all caches, so that the additional configuration will be used upon the next request.

EXAMPLE

$ curl -X POST -d '{"settings":{"foo": "bar"}}' http://127.0.0.1:8081/test/write-additional-settings --header "Content-Type:application/json"
{"success":true}

This will write the following Settings.Additional.yaml:

foo: bar

`POST /test/remove-additional-settings`

[!WARNING]
This endpoint is only available in the test distribution and should never be replicated for production use. DO NOT DO THIS IN PRODUCTION!

This endpoint does not require any data and will remove the FLOW_PATH_CONFIGURATION/Settings.Additional.yaml file, if it exists.

If the file was removed, this endpoint then also registers a signal handler for bootstrapShuttingDown, in which it will flush all caches, so that the removed configuration will no longer be used upon the next request.

EXAMPLE

$ curl -X POST -d '' http://127.0.0.1:8081/test/remove-additional-settings --header "Content-Type:application/json"
{"success":true}

This will remove Settings.Additional.yaml, if it exists.

Fix for #3816 & #2583

The Problem

The PageTree redux state is tracking hidden nodes, presumably for the purpose to hide all nodes that are not part of the current search result set.

This is indicated by how hiddenContextPaths for page tree search results are calculated:

neos-ui/packages/neos-ui-sagas/src/UI/PageTree/index.js

Lines 191 to 193 in fd5c917

    
           const resultContextPaths = Object.keys(nodes); 
        
           const oldHidden = yield select(selectors.UI.PageTree.getHidden); 
        
           const hiddenContextPaths = oldHidden.filter(i => !resultContextPaths.includes(i));

When node type filter and search term are cleared, the nodes that have been flagged as hidden during the last search, remain hidden aftwards, because the page tree state is not properly cleaned up.

The Solution

For clarity, I decided to flip the hidden state into a visible state. By default visible is set to null, indicating that every node should be shown in the tree.

When a search term or node type filter is set, visible will at first contain all context paths in the search result set (including intermediaries that are only ancestors of matching nodes).

While visible is not null, every node that is added to the in-memory store will be added to the visible list as well.

visible here only means "This node is invited to the tree party". A node that is invited, may still not be shown, because its parent is collapsed. Every node that is not invited, will never be shown, no matter what other toggle state may be set.

Fix for #2800 Part I

The Problem

Since #2794 all structural change operations handle an optional baseNodeType to account for currently set tree filters.

#2794 had been introduced in October 2020. About a year before that, in December 2019, batch operations for tree nodes had been introduced via #2568. #2794 did not consider the changes made in #2568, so the baseNodeType isn't being sent when tree nodes are moved. This leads to the disappearance of all nodes beneath the reference parent.

The logic for this would have needed to be handled in the moveDroppedNodes.js saga. Oddly enough, the 4 years old merge commit of #2794 shows that it wasn't present then:
https://github.com/neos/neos-ui/tree/ed9e04cdcaf5774914a60d866a8589709b4704f8/packages/neos-ui-sagas/src/CR/NodeOperations

In the 5 years old merge commit for #2568, moveDroppedNodes.js does indeed exist:
https://github.com/neos/neos-ui/tree/48233fee50c106334a949647ed3deef6d66dfa3c/packages/neos-ui-sagas/src/CR/NodeOperations

I have no explanation for this.

The Solution

I added the missing baseNodeType to moveDroppedNodes.js.

Fix for #2800 Part II

The problem

Similar to #3816 & #2583, the PageTree state did not recognize when a context path changed (which is what happens, if a node gets moved "into" another one).

The solution

I modified the PageTree reducer, so it tracks the CR.Nodes.UPDATE_PATH action as well.

Additional Changes

One more bugfix: Reloading the tree when in filtered state was broken -> this is fixed now
Tree nodes no longer show superfluous toggle handles
- For this I had to add a nodeTypeFilter option to the getForTreeWithParents finisher of the FlowQuery API
- The result is still not technically correct (as the entire API is quite broken), but it'll at least look correct 😅
I have added deprecation markers to all singular node operations (stuff like copy as opposed to copyMultiple, move as opposed to moveMultiple and so on)
- While I was attempting to fix Moving nodes with Preset Nodetype filter #2800, I stumbled upon this confusion, and since all singular operations can just as well be expressed as multiple operations, we should drop the singular ones entirely
- Follow up to this PR: The operations should be removed in 9.0 (this will require additional work)

Remaining TODOs

The drop targets used to have `visibility: hidden` set via CSS. Testcafé considers them absent, which is semantically correct. This commit uses a different CSS tactic do make the drop targets invisible, but "physically" present.

…2E tests

For this, the E2E test distribution now exposes two endpoints: - /test/write-additional-settings - /test/remove-additional-settings An E2E test can thus via `fetch` add settings to the current setup and remove the altered settings on cleanup. This became necessary, because the tests for the node tree preset feature require explicit settings to configure node tree presets. By default, node tree presets are inactive.

fixes: #3861, #2583

…tent tree

…pped refs: #2800

Every singular node operation can be expressed as a "multiple nodes" operation. Redundancy in this area has lead to bugs.

…ated fixes: #2800

…node tree preset

…rch term is set

…t do not match filter

Sebobo · 2024-07-05T06:36:25Z

You are awesome! Also love the part with the deprecation of the singular operations!
Will look for some time asap to review.

mhsdesign · 2024-07-05T07:42:10Z

Wow ❤️ What is going on :D Thanks i guess :D The tests will probably allow us to reimplement this feature for Neos 9 then as it seems useful after all: #3702 (currently its broken in 9.0 - as its an Neos Ui attached concept where were intercept link clicks ant attach query params)

Also nice idea with the post routes for adjusting settings. I run into the same problem and decided to use different Flow sub context that i dynamically set via header and special magic in index.php ... but a route seems much more flexible.

pKallert

I tried it out locally and works like a charm 👍

packages/react-ui-components/src/SelectBox/selectBox.js

…ectBox This allows us to target every part of a select box specifically in E2E tests.

pKallert

Looks good to me! Thank you so much for these fixes 🌷

bwaidelich

Great work as usual, thanks!
I can't really judge the JS parts, but I can confirm that this fixes some nasty bugs with a Neos 8.3 installation of mine and I could not (yet *g) observe any unwanted side effects

Sebobo

Works as advertised and change looks sensible, awesome ❤️

mhsdesign

forgot to publish my review ;) But looks all good by reading though ^^

mhsdesign · 2024-07-22T08:14:59Z

Classes/Fusion/Helper/NodeInfoHelper.php

     * @return array
     */
-    public function renderNodesWithParents(array $nodes, ControllerContext $controllerContext): array
+    public function renderNodesWithParents(array $nodes, ControllerContext $controllerContext, ?string $nodeTypeFilter = null): array
    {
        // For search operation we want to include all nodes, not respecting the "baseNodeType" setting


that statement is not correct anymore i guess ;)

mhsdesign · 2024-07-22T08:17:17Z

packages/neos-ui-redux-store/src/UI/PageTree/index.ts

    switch (action.type) {
        case system.INIT: {
            const contextPath = action.payload.cr.nodes.documentNode || action.payload.cr.nodes.siteNode;
            draft.focused = contextPath ? [contextPath] : [];
            break;
        }
+        case nodes.ADD:


thanks for providing tests for this new logic ❤️

grebaldi added 4 commits July 2, 2024 19:29

TASK: Make document tree filter toggle button discoverable in E2E tests

57aa6b6

TASK: Make node tree filter select box reset button discoverable in E…

f68e70d

…2E tests

TASK: Make document tree search reset button discoverable in E2E tests

71300ec

github-actions bot added the 9.0 label Jul 2, 2024

grebaldi changed the base branch from 9.0 to 8.3 July 2, 2024 17:58

github-actions bot added 8.3 and removed 9.0 labels Jul 2, 2024

grebaldi added 2 commits July 3, 2024 21:30

TASK: Add E2E tests for node tree presets

9f3e833

grebaldi force-pushed the bugfix/node-tree-presets-inconsistencies branch from a88b9fb to 9f3e833 Compare July 3, 2024 19:31

grebaldi added 4 commits July 3, 2024 22:00

TASK: Add failing E2E test for #3816

324a4bf

TASK: Add failing E2E test for #2583

65b66c8

TASK: Add failing E2E test for #2800

56a884c

BUGFIX: Allow-List nodes in document tree when filters are active

df03f85

fixes: #3861, #2583

This was linked to issues Jul 4, 2024

BUG: NodeTree presets, break expanding collapsed subtree #3816

Closed

Searching for children of already expanded nodes breaks expansion after removing search term #2583

Closed

grebaldi added 5 commits July 4, 2024 17:00

PATCH: Anticipate visibleContextPaths being undefined when in con…

b710da7

…tent tree

BUGFIX: Add baseNodeType to move change when multiple nodes are dro…

e3336a3

…pped refs: #2800

TASK: Deprecate all singular node operations

244a7b2

Every singular node operation can be expressed as a "multiple nodes" operation. Redundancy in this area has lead to bugs.

TASK: Add failing test for #2800 Part II

457fe91

BUGFIX: Track visible state in tree also when context paths are upd…

0195ad5

…ated fixes: #2800

grebaldi linked an issue Jul 4, 2024 that may be closed by this pull request

Moving nodes with Preset Nodetype filter #2800

Closed

grebaldi added 4 commits July 4, 2024 20:44

TASK: Add failing E2E test that shows broken page reload with active …

71bd100

…node tree preset

BUGFIX: Perform search on RELOAD_STATE when node type filter or sea…

c0e725f

…rch term is set

TASK: Add failing E2E test that show superfluous toggle handle

53edaac

BUGFIX: Ensure that filtered nodes provide no info about children tha…

479f9df

…t do not match filter

grebaldi changed the title ~~WIP: Inconsistencies w/ Node Tree Presets~~ BUGFIX: Fix inconsistencies in document tree with node tree presets Jul 4, 2024

grebaldi marked this pull request as ready for review July 4, 2024 20:34

github-actions bot added the Bug Label to mark the change as bugfix label Jul 4, 2024

Sebobo requested review from Sebobo and mhsdesign July 5, 2024 06:36

pKallert reviewed Jul 11, 2024

View reviewed changes

packages/react-ui-components/src/SelectBox/selectBox.js Show resolved Hide resolved

TASK: Distribute prefixed ids across DropDown* components used by Sel…

9e83031

…ectBox This allows us to target every part of a select box specifically in E2E tests.

grebaldi force-pushed the bugfix/node-tree-presets-inconsistencies branch from 81e8d23 to 9e83031 Compare July 12, 2024 08:37

grebaldi mentioned this pull request Jul 12, 2024

BUG: Navigating the Document Tree makes filtered nodes disappear #1691

Closed

pKallert approved these changes Jul 15, 2024

View reviewed changes

bwaidelich approved these changes Jul 19, 2024

View reviewed changes

Sebobo approved these changes Jul 23, 2024

View reviewed changes

Sebobo merged commit bc789ca into 8.3 Jul 23, 2024
10 checks passed

Sebobo deleted the bugfix/node-tree-presets-inconsistencies branch July 23, 2024 14:05

Sebobo mentioned this pull request Jul 24, 2024

Document tree behaves wrong when moving nodes (and other oddities) #2761

Open

grebaldi mentioned this pull request Jul 24, 2024

TASK: Remove deprecated singular node operations #3834

Open

mhsdesign reviewed Jul 24, 2024

View reviewed changes

mhsdesign mentioned this pull request Aug 23, 2024

Neos 9.0 Either reimplement or fully remove presetBaseNodeType #3702

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Fix inconsistencies in document tree with node tree presets #3822

BUGFIX: Fix inconsistencies in document tree with node tree presets #3822

grebaldi commented Jul 2, 2024 •

edited

Loading

Sebobo commented Jul 5, 2024

mhsdesign commented Jul 5, 2024

pKallert left a comment

pKallert left a comment

bwaidelich left a comment

Sebobo left a comment

mhsdesign left a comment

mhsdesign Jul 22, 2024

mhsdesign Jul 22, 2024

	const resultContextPaths = Object.keys(nodes);
	const oldHidden = yield select(selectors.UI.PageTree.getHidden);
	const hiddenContextPaths = oldHidden.filter(i => !resultContextPaths.includes(i));

BUGFIX: Fix inconsistencies in document tree with node tree presets #3822

BUGFIX: Fix inconsistencies in document tree with node tree presets #3822

Conversation

grebaldi commented Jul 2, 2024 • edited Loading

Add settings during E2E test run

POST /test/write-additional-settings

EXAMPLE

POST /test/remove-additional-settings

EXAMPLE

Fix for #3816 & #2583

The Problem

The Solution

Fix for #2800 Part I

The Problem

The Solution

Fix for #2800 Part II

The problem

The solution

Additional Changes

Remaining TODOs

Sebobo commented Jul 5, 2024

mhsdesign commented Jul 5, 2024

pKallert left a comment

Choose a reason for hiding this comment

pKallert left a comment

Choose a reason for hiding this comment

bwaidelich left a comment

Choose a reason for hiding this comment

Sebobo left a comment

Choose a reason for hiding this comment

mhsdesign left a comment

Choose a reason for hiding this comment

mhsdesign Jul 22, 2024

Choose a reason for hiding this comment

mhsdesign Jul 22, 2024

Choose a reason for hiding this comment

grebaldi commented Jul 2, 2024 •

edited

Loading

`POST /test/write-additional-settings`

`POST /test/remove-additional-settings`