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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Oct 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
480d8de
BUGFIX: Show and apply all changes in workspace review
ahaeslich Oct 8, 2023
83cb58e
Add comments and remove unused variable
ahaeslich Oct 12, 2023
2e7c5dc
Updated and comment path calculation for computeSiteChanges
ahaeslich Oct 12, 2023
0f2dc77
Merge branch '9.0' into bugfix/review-workspace-changes
ahaeslich Oct 13, 2023
fa38119
Merge remote-tracking branch 'origin/9.0' into bugfix/review-workspac…
mhsdesign Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 65 additions & 66 deletions Neos.Neos/Classes/Controller/Module/Management/WorkspacesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<string,int>
* @throws JsonException
*/
protected function computeChangesCount(Workspace $selectedWorkspace, ContentRepository $contentRepository): array
{
Expand All @@ -749,11 +738,10 @@ protected function computeChangesCount(Workspace $selectedWorkspace, ContentRepo
/**
* Builds an array of changes for sites in the given workspace
* @return array<string,mixed>
* @throws JsonException
*/
protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepository $contentRepository): array
{
$nodeAddressFactory = NodeAddressFactory::create($contentRepository);

$siteChanges = [];
$changes = $contentRepository->projectionState(ChangeFinder::class)
->findByContentStreamId(
Expand Down Expand Up @@ -792,7 +780,11 @@ protected function computeSiteChanges(Workspace $selectedWorkspace, ContentRepos
$documentPathSegments = [];
foreach ($ancestors as $ancestor) {
$pathSegment = $ancestor->nodeName ?: NodeName::fromString($ancestor->nodeAggregateId->value);
$nodePathSegments[] = $pathSegment;
// 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;
}
if ($this->getNodeType($ancestor)->isOfType(NodeTypeNameFactory::NAME_DOCUMENT)) {
$documentPathSegments[] = $pathSegment;
if (is_null($documentNode)) {
Expand All @@ -808,29 +800,46 @@ 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(
// 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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

fn (NodeName $nodeName): string => $nodeName->value,
$documentPathSegments
), 2));
$relativePath = str_replace(
sprintf('//%s/%s', $siteNodeName, $documentPath),
'',
implode('/', 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));
}

$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;
}
ahaeslich marked this conversation as resolved.
Show resolved Hide resolved

// 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(),
$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,
Expand All @@ -848,19 +857,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;
Expand Down
10 changes: 5 additions & 5 deletions ...Neos/Resources/Private/Partials/Module/Management/Workspaces/ContentChangeAttributes.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
{namespace neos=Neos\Neos\ViewHelpers}
<f:if condition="{change.isRemoved}">
<f:then>class="neos-content-change legend-deleted" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.deleted', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:then>class="neos-content-change legend-deleted" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.deleted', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:else>
<f:if condition="{change.isNew}">
<f:then>class="neos-content-change legend-created" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.created', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:then>class="neos-content-change legend-created" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.created', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:else>
<f:if condition="{change.isMoved}">
<f:then>class="neos-content-change legend-moved" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.moved', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:then>class="neos-content-change legend-moved" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.moved', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:else>
<f:if condition="{change.node.hidden}">
<f:then>class="neos-content-change legend-hidden" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.hidden', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:else>class="neos-content-change legend-edited" data-nodepath="{change.node.path}" title="{neos:backend.translate(id: 'workspaces.legend.edited', source: 'Modules', package: 'Neos.Neos')}"</f:else>
<f:then>class="neos-content-change legend-hidden" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.hidden', source: 'Modules', package: 'Neos.Neos')}"</f:then>
<f:else>class="neos-content-change legend-edited" data-nodepath="{nodepath}" title="{neos:backend.translate(id: 'workspaces.legend.edited', source: 'Modules', package: 'Neos.Neos')}"</f:else>
</f:if>
</f:else>
</f:if>
Expand Down
10 changes: 5 additions & 5 deletions Neos.Neos/Resources/Private/Templates/Module/Management/Workspaces/Show.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
<tbody>
<f:for each="{siteChanges}" as="site">
<f:for each="{site.documents}" key="documentPath" as="document">
<tr class="neos-document" data-nodepath="{document.documentNode.path}" data-ismoved="{f:if(condition: document.isMoved, then: 'true', else: 'false')}" data-isnew="{f:if(condition: document.isNew, then: 'true', else: 'false')}">
<tr class="neos-document" data-nodepath="{documentPath}" data-ismoved="{f:if(condition: document.isMoved, then: 'true', else: 'false')}" data-isnew="{f:if(condition: document.isNew, then: 'true', else: 'false')}">
<f:if condition="{document.changes -> f:count()} > 1">
<f:then>
<td class="check neos-priority1">
<label for="check-document-{document.documentNode.identifier}" class="neos-checkbox"><f:form.checkbox id="check-document-{document.documentNode.identifier}" class="neos-check-document" value="{document.documentNode.identifier}"/><span></span></label>
<label for="check-document-{document.documentNode.nodeAggregateId.value}" class="neos-checkbox"><f:form.checkbox id="check-document-{document.documentNode.nodeAggregateId.value}" class="neos-check-document" value="{document.documentNode.nodeAggregateId.value}"/><span></span></label>
</td>
<td class="neos-priority1 path-caption">
</f:then>
Expand All @@ -56,15 +56,15 @@
</div>
</td>
<td class="neos-action neos-folder">
<i class="fold-toggle fas fa-chevron-up icon-white" data-toggle="fold-{document.documentNode.identifier}"></i>
<i class="fold-toggle fas fa-chevron-up icon-white" data-toggle="fold-{document.documentNode.nodeAggregateId.value}"></i>
</td>
</tr>
<f:for each="{document.changes}" key="relativePath" as="change">
<tr class="neos-change fold-{document.documentNode.identifier} document-{document.documentNode.identifier}" data-nodepath="{change.node.path}" data-ismoved="{f:if(condition: change.isMoved, then: 'true', else: 'false')}" data-isnew="{f:if(condition: change.isNew, then: 'true', else: 'false')}">
<tr class="neos-change fold-{document.documentNode.nodeAggregateId.value} document-{document.documentNode.nodeAggregateId.value}" data-nodepath="{relativePath}" data-ismoved="{f:if(condition: change.isMoved, then: 'true', else: 'false')}" data-isnew="{f:if(condition: change.isNew, then: 'true', else: 'false')}">
<td class="check neos-priority1">
<label for="{change.serializedNodeAddress}" class="neos-checkbox"><f:form.checkbox name="nodes[]" value="{change.serializedNodeAddress}" id="{change.serializedNodeAddress}" /><span></span></label>
</td>
<td id="change-{change.serializedNodeAddress}" {f:render(partial: 'Module/Management/Workspaces/ContentChangeAttributes', arguments: {change: change})} data-neos-toggle="tooltip" data-placement="left" data-container="body">
<td id="change-{change.serializedNodeAddress}" {f:render(partial: 'Module/Management/Workspaces/ContentChangeAttributes', arguments: {change: change, nodepath: relativePath})} data-neos-toggle="tooltip" data-placement="left" data-container="body">
<f:render partial="Module/Management/Workspaces/ContentChangeDiff" arguments="{change: change, contentDimensions: contentDimensions}"/>
</td>
<td class="neos-action">
Expand Down