BUGFIX: Show and apply all changes in workspace review #4588

ahaeslich · 2023-10-08T09:14:28Z

Fixes: #4587
Relates: #4563

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

- Fix assignment of `siteNode` - Fix path generation to fix grouping of changes by document - Fix publishing of deleted node - Fix checkbox behaviour to not publish new child nodes independent of parent Relates: #4573

ahaeslich · 2023-10-10T13:48:30Z

While testing this change in our project we found that:

Selecting all nodes by using "Select all current changes" and click on " Publish selected changes to live" (Calls publishOrDiscardNodesAction) works. ✅

Selecting nothing and click on " Publish all changes to live" (Calls publishWorkspaceAction) will lead to this error message:

The base workspace has been modified in the meantime; please rebase.

Edit: ... sometimes 🤯
Edit 2: can reproduce this strange behaviour without my changes ... happy us ... this calls for a new issue (#4591)

bwaidelich

As written on Slack, I found it hard to review this PR because it contains unrelated changes (it seems).
Also without any tests it's hard to tell what changes actually fix the bug. Maybe you could add some inline comments (in code or just as PR comments) to explain the fix?

bwaidelich · 2023-10-12T08:00:38Z

Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php

                    }
                }

                // Neither $documentNode, $siteNode or its cannot really be null, this is just for type checks;
                // We should probably throw an exception though
                if ($documentNode !== null && $siteNode !== null && $siteNode->nodeName) {
                    $siteNodeName = $siteNode->nodeName->value;
-                    $documentPath = implode('/', array_slice(array_map(
+                    $documentPath = implode('/', array_reverse(array_map(


I'm trying to understand: (how) is this related to the issue?

In both places I didn't quite understand why we need the paths in the first place. It's also used by the current JS logic to select and group nodes by document. And in neos 8 that starts at the side node and not with the lowest node.

And in neos 8 that starts at the side node and not with the lowest node

So that's why we change array_slice to array_reverse right?

bwaidelich · 2023-10-12T08:00:47Z

Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php

                    $relativePath = str_replace(
                        sprintf('//%s/%s', $siteNodeName, $documentPath),
                        '',
-                        implode('/', array_map(
+                        implode('/', array_reverse(array_map(


Same as above.

Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php

Relates: #4573

ahaeslich · 2023-10-12T08:37:54Z

As written on Slack, I found it hard to review this PR because it contains unrelated changes (it seems). Also without any tests it's hard to tell what changes actually fix the bug. Maybe you could add some inline comments (in code or just as PR comments) to explain the fix?

Thx for your questions ❤️ I added some comments in the code as I think we might need them there for a future refactoring.

Relates: #4573

ahaeslich · 2023-10-13T10:19:39Z

We can update this to use Neos.Neos:Site when #4563 is merged

mhsdesign

I can confirm that the changes count doesnt reflect any new changes (the green ones) neither for user workspaces or an actual workspace:

before

after (this fix)

modifications seem to be correctly reflected already:

and still work after this fix.

So thanks for taking care of this. I approve by testing ;)

…e-changes

ahaeslich · 2023-10-17T08:15:24Z

Thx @mhsdesign for merging ❤️. So I think we should be good to go here.

bwaidelich

This one is a bit hard to review (by reading). But we agreed to go for it as it definitely make things better!

Thanks for taking care!

This fixes a merge error introduced with fa38119 We check `$ancestor` to be of nodeType `Neos.Neos:Site` so this is the correct value for `$siteNode`. Relates: #4588

BUGFIX: Show and apply all changes in workspace review

480d8de

- Fix assignment of `siteNode` - Fix path generation to fix grouping of changes by document - Fix publishing of deleted node - Fix checkbox behaviour to not publish new child nodes independent of parent Relates: #4573

ahaeslich requested review from dlubitz and Sebobo October 8, 2023 09:14

github-actions bot added Bug 9.0 labels Oct 8, 2023

ahaeslich added the 9.0beta1 label Oct 9, 2023

ahaeslich mentioned this pull request Oct 9, 2023

Odd things about (user) workspaces #4566

Open

9 tasks

ahaeslich requested a review from mhsdesign October 9, 2023 10:03

ahaeslich mentioned this pull request Oct 10, 2023

BUG: Using "Publish all changes to live" in the workspace modul throws error #4591

Closed

1 task

ahaeslich requested review from kitsunet, mficzel, nezaniel, markusguenther and bwaidelich October 11, 2023 09:45

bwaidelich reviewed Oct 12, 2023

View reviewed changes

Add comments and remove unused variable

83cb58e

Relates: #4573

Updated and comment path calculation for computeSiteChanges

2e7c5dc

Relates: #4573

ahaeslich requested a review from bwaidelich October 13, 2023 08:39

Merge branch '9.0' into bugfix/review-workspace-changes

0f2dc77

mhsdesign approved these changes Oct 16, 2023

View reviewed changes

Merge remote-tracking branch 'origin/9.0' into bugfix/review-workspac…

fa38119

…e-changes

bwaidelich approved these changes Oct 17, 2023

View reviewed changes

bwaidelich merged commit fc37b92 into 9.0 Oct 17, 2023
5 checks passed

bwaidelich deleted the bugfix/review-workspace-changes branch October 17, 2023 10:20

ahaeslich mentioned this pull request Oct 19, 2023

BUGFIX: Set correct siteNode in WorkspaceController #4627

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Show and apply all changes in workspace review #4588

BUGFIX: Show and apply all changes in workspace review #4588

ahaeslich commented Oct 8, 2023 •

edited

Loading

ahaeslich commented Oct 10, 2023 •

edited

Loading

bwaidelich left a comment

bwaidelich Oct 12, 2023

ahaeslich Oct 12, 2023

bwaidelich Oct 12, 2023

ahaeslich Oct 12, 2023

bwaidelich Oct 12, 2023

ahaeslich Oct 12, 2023

ahaeslich commented Oct 12, 2023

ahaeslich commented Oct 13, 2023

mhsdesign left a comment

ahaeslich commented Oct 17, 2023

bwaidelich left a comment

BUGFIX: Show and apply all changes in workspace review #4588

BUGFIX: Show and apply all changes in workspace review #4588

Conversation

ahaeslich commented Oct 8, 2023 • edited Loading

ahaeslich commented Oct 10, 2023 • edited Loading

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich Oct 12, 2023

Choose a reason for hiding this comment

ahaeslich Oct 12, 2023

Choose a reason for hiding this comment

bwaidelich Oct 12, 2023

Choose a reason for hiding this comment

ahaeslich Oct 12, 2023

Choose a reason for hiding this comment

bwaidelich Oct 12, 2023

Choose a reason for hiding this comment

ahaeslich Oct 12, 2023

Choose a reason for hiding this comment

ahaeslich commented Oct 12, 2023

ahaeslich commented Oct 13, 2023

mhsdesign left a comment

Choose a reason for hiding this comment

ahaeslich commented Oct 17, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

ahaeslich commented Oct 8, 2023 •

edited

Loading

ahaeslich commented Oct 10, 2023 •

edited

Loading