From 480d8dedafc3993cf364c064598a69c06d39c753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anke=20H=C3=A4slich?= Date: Sun, 8 Oct 2023 10:58:02 +0200 Subject: [PATCH 1/3] BUGFIX: Show and apply all changes in workspace review - 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 --- .../Management/WorkspacesController.php | 114 +++++++++--------- .../Workspaces/ContentChangeAttributes.html | 10 +- .../Module/Management/Workspaces/Show.html | 10 +- 3 files changed, 65 insertions(+), 69 deletions(-) diff --git a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php index 34ba8e99ea5..1cc4c6076e6 100644 --- a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php +++ b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php @@ -14,6 +14,8 @@ namespace Neos\Neos\Controller\Module\Management; +use Doctrine\DBAL\DBALException; +use JsonException; use Neos\ContentRepository\Core\ContentRepository; use Neos\ContentRepository\Core\Feature\WorkspacePublication\Dto\NodeIdsToPublishOrDiscard; use Neos\ContentRepository\Core\Feature\WorkspacePublication\Dto\NodeIdToPublishOrDiscard; @@ -78,35 +80,20 @@ class WorkspacesController extends AbstractModuleController #[Flow\Inject] protected ContentRepositoryRegistry $contentRepositoryRegistry; - /** - * @Flow\Inject - * @var SiteRepository - */ - protected $siteRepository; + #[Flow\Inject] + protected SiteRepository $siteRepository; - /** - * @Flow\Inject - * @var PropertyMapper - */ - protected $propertyMapper; + #[Flow\Inject] + protected PropertyMapper $propertyMapper; - /** - * @Flow\Inject - * @var Context - */ - protected $securityContext; + #[Flow\Inject] + protected Context $securityContext; - /** - * @Flow\Inject - * @var UserService - */ - protected $domainUserService; + #[Flow\Inject] + protected UserService $domainUserService; - /** - * @var PackageManager - * @Flow\Inject - */ - protected $packageManager; + #[Flow\Inject] + protected PackageManager $packageManager; /** * Display a list of unpublished content @@ -190,10 +177,7 @@ public function showAction(WorkspaceName $workspace): void ]); } - /** - * @return void - */ - public function newAction() + public function newAction(): void { $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest()) ->contentRepositoryId; @@ -218,7 +202,7 @@ public function createAction( WorkspaceName $baseWorkspace, string $visibility, WorkspaceDescription $description, - ) { + ): void { $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest()) ->contentRepositoryId; $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); @@ -264,11 +248,8 @@ public function createAction( /** * Edit a workspace - * - * @param WorkspaceName $workspaceName - * @return void */ - public function editAction(WorkspaceName $workspaceName) + public function editAction(WorkspaceName $workspaceName): void { $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest()) ->contentRepositoryId; @@ -301,8 +282,12 @@ public function editAction(WorkspaceName $workspaceName) * @param string $workspaceOwner Id of the owner of the workspace * @return void */ - public function updateAction(WorkspaceName $workspaceName, WorkspaceTitle $title, WorkspaceDescription $description, ?string $workspaceOwner) - { + public function updateAction( + WorkspaceName $workspaceName, + WorkspaceTitle $title, + WorkspaceDescription $description, + ?string $workspaceOwner + ): void { $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest()) ->contentRepositoryId; $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); @@ -356,9 +341,12 @@ public function updateAction(WorkspaceName $workspaceName, WorkspaceTitle $title * Delete a workspace * * @param WorkspaceName $workspaceName A workspace to delete - * @return void + * @throws IndexOutOfBoundsException + * @throws InvalidFormatPlaceholderException + * @throws StopActionException + * @throws DBALException */ - public function deleteAction(WorkspaceName $workspaceName) + public function deleteAction(WorkspaceName $workspaceName): void { $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest()) ->contentRepositoryId; @@ -724,6 +712,7 @@ public function discardWorkspaceAction(WorkspaceName $workspace): void * Computes the number of added, changed and removed nodes for the given workspace * * @return array + * @throws JsonException */ protected function computeChangesCount(Workspace $selectedWorkspace, ContentRepository $contentRepository): array { @@ -749,6 +738,7 @@ protected function computeChangesCount(Workspace $selectedWorkspace, ContentRepo /** * Builds an array of changes for sites in the given workspace * @return array + * @throws JsonException */ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepository $contentRepository): array { @@ -792,14 +782,17 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos $documentPathSegments = []; foreach ($ancestors as $ancestor) { $pathSegment = $ancestor->nodeName ?: NodeName::fromString($ancestor->nodeAggregateId->value); - $nodePathSegments[] = $pathSegment; + if (!$this->getNodeType($ancestor)->isOfType(NodeTypeNameFactory::NAME_SITES)) { + $nodePathSegments[] = $pathSegment; + } if ($this->getNodeType($ancestor)->isOfType(NodeTypeNameFactory::NAME_DOCUMENT)) { $documentPathSegments[] = $pathSegment; if (is_null($documentNode)) { $documentNode = $ancestor; } // the site node is the last ancestor of type Document - $siteNode = $documentNode; + // TODO: Check for Neos.Neos:Site instead of regular document after it is introduced + $siteNode = $ancestor; } } @@ -807,29 +800,42 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos // 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( fn (NodeName $nodeName): string => $nodeName->value, $documentPathSegments - ), 2)); + ))); $relativePath = str_replace( sprintf('//%s/%s', $siteNodeName, $documentPath), '', - implode('/', array_map( + implode('/', array_reverse(array_map( fn (NodeName $nodeName): string => $nodeName->value, $nodePathSegments - )) + ))) ); if (!isset($siteChanges[$siteNodeName]['siteNode'])) { $siteChanges[$siteNodeName]['siteNode'] = $this->siteRepository->findOneByNodeName(SiteNodeName::fromString($siteNodeName)); } + $siteChanges[$siteNodeName]['documents'][$documentPath]['documentNode'] = $documentNode; + if ($documentNode->equals($node)) { + $siteChanges[$siteNodeName]['documents'][$documentPath]['isNew'] = $change->created; + $siteChanges[$siteNodeName]['documents'][$documentPath]['isMoved'] = $change->moved; + } + + $nodeAddress = new NodeAddress( + $change->contentStreamId, + $change->originDimensionSpacePoint->toDimensionSpacePoint(), + $change->nodeAggregateId, + $selectedWorkspace->workspaceName + ); $change = [ 'node' => $node, - 'serializedNodeAddress' => $nodeAddressFactory->createFromNode($node)->serializeForUri(), + 'serializedNodeAddress' => $nodeAddress->serializeForUri(), 'isRemoved' => $change->deleted, - 'isNew' => false, + 'isNew' => $change->created, + 'isMoved' => $change->moved, 'contentChanges' => $this->renderContentChanges( $node, $change->contentStreamId, @@ -847,19 +853,9 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos ksort($siteChanges); foreach ($siteChanges as $siteKey => $site) { - /*foreach ($site['documents'] as $documentKey => $document) { - $liveDocumentNode = $liveContext->getNodeByIdentifier($document['documentNode']->getIdentifier()); - $siteChanges[$siteKey]['documents'][$documentKey]['isMoved'] - = $liveDocumentNode && $document['documentNode']->getPath() !== $liveDocumentNode->getPath(); - $siteChanges[$siteKey]['documents'][$documentKey]['isNew'] = $liveDocumentNode === null; - foreach ($document['changes'] as $changeKey => $change) { - $liveNode = $liveContext->getNodeByIdentifier($change['node']->getIdentifier()); - $siteChanges[$siteKey]['documents'][$documentKey]['changes'][$changeKey]['isNew'] - = is_null($liveNode); - $siteChanges[$siteKey]['documents'][$documentKey]['changes'][$changeKey]['isMoved'] - = $liveNode && $change['node']->getPath() !== $liveNode->getPath(); - } - }*/ + foreach ($site['documents'] as $documentKey => $document) { + ksort($siteChanges[$siteKey]['documents'][$documentKey]['changes']); + } ksort($siteChanges[$siteKey]['documents']); } return $siteChanges; diff --git a/Neos.Neos/Resources/Private/Partials/Module/Management/Workspaces/ContentChangeAttributes.html b/Neos.Neos/Resources/Private/Partials/Module/Management/Workspaces/ContentChangeAttributes.html index 8322fe6ad2b..0568d7d1c06 100644 --- a/Neos.Neos/Resources/Private/Partials/Module/Management/Workspaces/ContentChangeAttributes.html +++ b/Neos.Neos/Resources/Private/Partials/Module/Management/Workspaces/ContentChangeAttributes.html @@ -1,16 +1,16 @@ {namespace neos=Neos\Neos\ViewHelpers} - class="neos-content-change legend-deleted" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.deleted', source: 'Modules', package: 'Neos.Neos')}" + class="neos-content-change legend-deleted" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.deleted', source: 'Modules', package: 'Neos.Neos')}" - class="neos-content-change legend-created" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.created', source: 'Modules', package: 'Neos.Neos')}" + class="neos-content-change legend-created" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.created', source: 'Modules', package: 'Neos.Neos')}" - class="neos-content-change legend-moved" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.moved', source: 'Modules', package: 'Neos.Neos')}" + class="neos-content-change legend-moved" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.moved', source: 'Modules', package: 'Neos.Neos')}" - class="neos-content-change legend-hidden" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.hidden', source: 'Modules', package: 'Neos.Neos')}" - class="neos-content-change legend-edited" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.edited', source: 'Modules', package: 'Neos.Neos')}" + class="neos-content-change legend-hidden" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.hidden', source: 'Modules', package: 'Neos.Neos')}" + class="neos-content-change legend-edited" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.edited', source: 'Modules', package: 'Neos.Neos')}" diff --git a/Neos.Neos/Resources/Private/Templates/Module/Management/Workspaces/Show.html b/Neos.Neos/Resources/Private/Templates/Module/Management/Workspaces/Show.html index ec5e58e00d4..f3c0bf613b8 100644 --- a/Neos.Neos/Resources/Private/Templates/Module/Management/Workspaces/Show.html +++ b/Neos.Neos/Resources/Private/Templates/Module/Management/Workspaces/Show.html @@ -28,11 +28,11 @@ - + - + @@ -56,15 +56,15 @@ - + - + - + From 83cb58eeb3930e7e312470676eaf4f8e8b49c259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anke=20H=C3=A4slich?= Date: Thu, 12 Oct 2023 10:33:05 +0200 Subject: [PATCH 2/3] Add comments and remove unused variable Relates: #4573 --- .../Module/Management/WorkspacesController.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php index 1cc4c6076e6..1b016a00406 100644 --- a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php +++ b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php @@ -742,8 +742,6 @@ protected function computeChangesCount(Workspace $selectedWorkspace, ContentRepo */ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepository $contentRepository): array { - $nodeAddressFactory = NodeAddressFactory::create($contentRepository); - $siteChanges = []; $changes = $contentRepository->projectionState(ChangeFinder::class) ->findByContentStreamId( @@ -782,6 +780,8 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos $documentPathSegments = []; foreach ($ancestors as $ancestor) { $pathSegment = $ancestor->nodeName ?: NodeName::fromString($ancestor->nodeAggregateId->value); + // Don't include `sites` path as they are not needed + // by the HTML/JS magic and won't be included as `$documentPathSegments` if (!$this->getNodeType($ancestor)->isOfType(NodeTypeNameFactory::NAME_SITES)) { $nodePathSegments[] = $pathSegment; } @@ -800,6 +800,7 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos // We should probably throw an exception though if ($documentNode !== null && $siteNode !== null && $siteNode->nodeName) { $siteNodeName = $siteNode->nodeName->value; + // $documentPath = implode('/', array_reverse(array_map( fn (NodeName $nodeName): string => $nodeName->value, $documentPathSegments @@ -818,11 +819,15 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos } $siteChanges[$siteNodeName]['documents'][$documentPath]['documentNode'] = $documentNode; + // We need to set `isNew` and `isMoved` on document level to make our JS behave as before. if ($documentNode->equals($node)) { $siteChanges[$siteNodeName]['documents'][$documentPath]['isNew'] = $change->created; $siteChanges[$siteNodeName]['documents'][$documentPath]['isMoved'] = $change->moved; } + // As for changes of type `delete` we are using nodes from the live content stream + // we can't create `serializedNodeAddress` from the node. + // Instead, we use the original stored values. $nodeAddress = new NodeAddress( $change->contentStreamId, $change->originDimensionSpacePoint->toDimensionSpacePoint(), From 2e7c5dc7ba8dd051ab6b78b68f13dd8a75e573cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anke=20H=C3=A4slich?= Date: Thu, 12 Oct 2023 11:06:04 +0200 Subject: [PATCH 3/3] Updated and comment path calculation for computeSiteChanges Relates: #4573 --- .../Module/Management/WorkspacesController.php | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php index 1b016a00406..c197c9f99b2 100644 --- a/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php +++ b/Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php @@ -800,19 +800,18 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos // We should probably throw an exception though if ($documentNode !== null && $siteNode !== null && $siteNode->nodeName) { $siteNodeName = $siteNode->nodeName->value; - // + // Reverse `$documentPathSegments` to start with the site node. + // The paths are used for grouping the nodes and for selecting a tree of nodes. $documentPath = implode('/', array_reverse(array_map( fn (NodeName $nodeName): string => $nodeName->value, $documentPathSegments ))); - $relativePath = str_replace( - sprintf('//%s/%s', $siteNodeName, $documentPath), - '', - implode('/', array_reverse(array_map( - fn (NodeName $nodeName): string => $nodeName->value, - $nodePathSegments - ))) - ); + // Reverse `$nodePathSegments` to start with the site node. + // The paths are used for grouping the nodes and for selecting a tree of nodes. + $relativePath = implode('/', array_reverse(array_map( + fn (NodeName $nodeName): string => $nodeName->value, + $nodePathSegments + ))); if (!isset($siteChanges[$siteNodeName]['siteNode'])) { $siteChanges[$siteNodeName]['siteNode'] = $this->siteRepository->findOneByNodeName(SiteNodeName::fromString($siteNodeName));