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

Jump to bottom

FEATURE: Integrate conflict resolution with publish/discard dialog workflow #3769

Open
wants to merge 12 commits into
base: 9.0
Choose a base branch
from
Open

FEATURE: Integrate conflict resolution with publish/discard dialog workflow #3769

wants to merge 12 commits into from

Conversation

grebaldi
Copy link
Contributor

@grebaldi grebaldi commented Apr 22, 2024 •
edited
Loading

Peek.2024-05-29.13-09.Publish.+.Sync.mp4

solves: #3761
fixes: #3785
builds on: #3762

The Problem

With the introduction of conflict resolution, users can synchronize their workspace with recent changes in the base workspace by clicking the "sync"-button that is placed aside the publish dropdown. When a workspace is in OUTDATED state, the "sync"-button shows up.

It may happen though that a workspace transitions into OUTDATED state without the UI taking notice of it. If the user then tries to publish their changes, publishing fails with an exception. Only after reloading the entire UI, they'll be able to trigger the "sync"-workflow.

Steps to reproduce

Assuming you have two user accounts with Neos.Neos:Editor role (let's call them "editor-a" and "editor-b"):

  1. Log in as "editor-a"
  2. Create a document "Test"
  3. Add a text node to "Test"
  4. Publish those changes
  5. Edit the text node, without publishing that change
  6. Log in as "editor-b" in a private window (keep "editor-a"'s window open!)
  7. Sync "editor-b"'s workspace, so you can see document "Test"
  8. Remove document "Test"
  9. Publish that change
  10. Switch to "editor-a"'s window (do not reload the page!)
  11. Try to publish the change on the text node that is still pending
  12. Observe:
  • Pre-PR: Publishing results in an exception
  • Post-PR: Publishing triggers conflict resolution

The solution

With this PR, the UI automatically triggers the sync and conflict resolution workflows if a publishing operation fails due to an outdated workspace.

Remaining TODOs

  • Move Conflicts concept to Application\Shared namespace
    • Move ConflictsOccurred to Application\Shared namespace
    • Move Conflict to Application\Shared namespace
    • Move Conflicts to Application\Shared namespace
    • Move ConflictsBuilder to Application\Shared namespace
    • Move ReasonForConflict to Application\Shared namespace
    • Move TypeOfChange to Application\Shared namespace
    • Move IconLabel to Application\Shared namespace
  • Add command handlers for all Publish-related commands & make sure they throw ConflictsOccurred
    • Move PublishChangesInDocument -> PublishChangesInDocument\PublishChangesInDocumentCommand
    • Create PublishChangesInDocumentCommandHandler
    • Ensure that PublishChangesInDocumentCommandHandler throws ConflictsOccurred
    • Move PublishChangesInSite -> PublishChangesInSite\PublishChangesInSiteCommand
    • Create PublishChangesInSiteCommandHandler
    • Ensure that PublishChangesInSiteCommandHandler throws ConflictsOccurred
  • Turn ConflictsOccurred into a Result DTO rather than an exception
  • Eliminate redundancy in catch (WorkspaceRebaseFailed $e) clauses
  • Trigger conflict resolution from within Publish saga
    • Add conflicts case to PublishingResponse
    • Add PublishingState.CONFLICTS
  • Fix: "Node could not be published, because of missing parentNode" after conflict resolution (happens when you do "Publish Document" on a document that gets removed by conflict resolution)
  • Fix: TypeError node.children is undefined (regression after FEATURE: Collapse All Button in Content and Page Tree #3756 - Race Condition)
  • Fix: You still can get stuck in an undefined state in which the saga ceases to continue (happens when the in-between sync is cancelled)
  • Fix: BUG: The new conflict resolution e2e tests are sadly a bit flaky in ci :O #3785

@grebaldi grebaldi force-pushed the feature/conflict-resolution-02/rebase-conflicts branch from 2b55b57 to 3069277 Compare April 22, 2024 15:25
@grebaldi grebaldi force-pushed the feature/conflict-resolution-03/rebase-during-publish branch from 5ae58ec to 8e4a03d Compare April 22, 2024 15:26
@grebaldi grebaldi linked an issue Apr 23, 2024 that may be closed by this pull request
FEATURE: Offer rebase option when publish/discard fails due to conflict #3761
Open
6 tasks
@grebaldi grebaldi force-pushed the feature/conflict-resolution-02/rebase-conflicts branch from 3069277 to f3ac501 Compare April 25, 2024 15:13
@grebaldi grebaldi force-pushed the feature/conflict-resolution-03/rebase-during-publish branch 2 times, most recently from 926004b to 1d493ed Compare April 29, 2024 13:57
Base automatically changed from feature/conflict-resolution-02/rebase-conflicts to 9.0 May 13, 2024 15:07
@mhsdesign
Copy link
Member

Id like to help reviewing this so we can soon make the changes Neos requires regarding the rename an removal of Node fields on 9.0-dev (whithout major conflicts against your pr)

@grebaldi
Copy link
Contributor Author

@mhsdesign Don't worry about conflicts - I'll take care of keeping this PR up-to-date. So, unless anything upstream of the UI needs this change to move on, I'd say that everything else has more priority.

@grebaldi grebaldi force-pushed the feature/conflict-resolution-03/rebase-during-publish branch 2 times, most recently from 2a29d84 to fca73dd Compare May 21, 2024 09:48
@grebaldi grebaldi changed the title WIP: Integrate conflict resolution with publish/discard dialog workflow FEATURE: Integrate conflict resolution with publish/discard dialog workflow May 29, 2024
@grebaldi grebaldi marked this pull request as ready for review May 29, 2024 13:16
@github-actions github-actions bot added the Feature Label to mark the change as feature label May 29, 2024
@mhsdesign mhsdesign self-requested a review June 11, 2024 20:22
@grebaldi grebaldi force-pushed the feature/conflict-resolution-03/rebase-during-publish branch from 50561af to faee8ca Compare June 14, 2024 14:42
@grebaldi
Copy link
Contributor Author

@pKallert
FYI: While rebasing, I encountered conflicts with the fix you've provided in #3634. To resolve this, I have moved the translation handling from BackendServiceController::publishChangesInDocumentAction to PublishChangesInDocumentCommandHandler::handle:

neos-ui/Classes/Application/PublishChangesInDocument/PublishChangesInDocumentCommandHandler.php

Lines 62 to 74 in faee8ca

} catch (NodeAggregateCurrentlyDoesNotExist $e) {
throw new \RuntimeException(
$this->getLabel('NodeNotPublishedMissingParentNode'),
1705053430,
$e
);
} catch (NodeAggregateDoesCurrentlyNotCoverDimensionSpacePoint $e) {
throw new \RuntimeException(
$this->getLabel('NodeNotPublishedParentNodeNotInCurrentDimension'),
1705053432,
$e
);
} catch (WorkspaceRebaseFailed $e) {

Does that match your intend?

@mhsdesign
One of the conflicting commits was one of yours, but was introduced in #3634 as well: 29a73a1

So, same question goes to you as well :)

@mhsdesign
Copy link
Member

Yes looks perfect thanks for taking care :)

@grebaldi
TASK: Move Conflicts concept to Application\Shared namespace
aaeb68a 
This moves the classes `ConflictsOccurred`, `Conflict`, `Conflicts`,
`ConflictsBuilder`, `ReasonForConflict`, `TypeOfChange` and `IconLabel`
into the `Application\Shared` namespace and adjusts all references
accordingly.

These classes are going to be reused by multiple command handlers.
@grebaldi
TASK: Add command handlers for all Publish-related commands
5ff99a8 
This includes the following tasks;
- Move `PublishChangesInDocument` ->
`PublishChangesInDocument\PublishChangesInDocumentCommand`
- Create `PublishChangesInDocumentCommandHandler`
- Move `PublishChangesInSite` ->
`PublishChangesInSite\PublishChangesInSiteCommand`
- Create `PublishChangesInSiteCommandHandler`
@grebaldi
TASK: Ensure that all Publish-related command handlers throw `Conflic…
10c0098 
…tsOccurred`
@grebaldi
TASK: Turn ConflictsOccurred into a Result DTO rather than an excep…
852bad1 
…tion
@grebaldi
TASK: Move logic to create Conflicts from WorkspaceRebaseFailed into …
aec75b0 
…dedicated Factory
@grebaldi
TASK: Use conflict DTO key property as key for iteration in Conflic…
67468b3 
…tList component
@grebaldi
TASK: Trigger conflict resolution from within Publish saga
24927dc
@grebaldi
BUGFIX: Remove race condition in watchNodeInformationChanges
f23176b 
It may happen that `fetchAdditionalNodeMetadata` adds metadata for
a node that has since been removed from the store. The occurance
of such "zombie" nodes leads to problems, because they may lack
crucial properties like `children` that are treated as always-
present by other mechanisms throughout the UI.

This became apparent after #3756.

The CR.Nodes.MERGE action now takes care of preventing zombie nodes
from entering the store.
@grebaldi
BUGFIX: Wait and reload before syncing in E2E test
4205f09 
This is to prevent the flakiness described in #3785.
@grebaldi
TASK: Stabilize E2E tests for synving feature
98d002a 
fixes: #3785
@grebaldi
BUGFIX: Fix undefined state when cancelling sync operations
d8c93a0 
This adds an e2e test case for cancelling and reselecting a resolution strategy
and another e2e test case for immediate resolution cancellation.

Both test cases are fixed by changes to the syncing saga.
@grebaldi
TASK: Handle edge case when automatic syncing during "publish documen…
f2526fd 
…t" removes the document

This also adds two more E2E test cases for publishing with automatic syncing.
@grebaldi grebaldi force-pushed the feature/conflict-resolution-03/rebase-during-publish branch from faee8ca to f2526fd Compare June 28, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhsdesign mhsdesign Awaiting requested review from mhsdesign

Assignees
No one assigned
Labels
9.0 Feature Label to mark the change as feature
Projects
None yet
Milestone
No milestone
2 participants
@grebaldi @mhsdesign