neos-ui icon indicating copy to clipboard operation
neos-ui copied to clipboard

FEATURE: Integrate conflict resolution with publish/discard dialog workflow

Open grebaldi opened this issue 1 year ago • 5 comments

https://github.com/neos/neos-ui/assets/2522299/5177f375-a586-4c97-8511-a9b60fd4b7a9

solves: https://github.com/neos/neos-ui/issues/3761 fixes: #3785 builds on: https://github.com/neos/neos-ui/pull/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

  • [x] Move Conflicts concept to Application\Shared namespace
    • [x] Move ConflictsOccurred to Application\Shared namespace
    • [x] Move Conflict to Application\Shared namespace
    • [x] Move Conflicts to Application\Shared namespace
    • [x] Move ConflictsBuilder to Application\Shared namespace
    • [x] Move ReasonForConflict to Application\Shared namespace
    • [x] Move TypeOfChange to Application\Shared namespace
    • [x] Move IconLabel to Application\Shared namespace
  • [x] Add command handlers for all Publish-related commands & make sure they throw ConflictsOccurred
    • [x] Move PublishChangesInDocument -> PublishChangesInDocument\PublishChangesInDocumentCommand
    • [x] Create PublishChangesInDocumentCommandHandler
    • [x] Ensure that PublishChangesInDocumentCommandHandler throws ConflictsOccurred
    • [x] Move PublishChangesInSite -> PublishChangesInSite\PublishChangesInSiteCommand
    • [x] Create PublishChangesInSiteCommandHandler
    • [x] Ensure that PublishChangesInSiteCommandHandler throws ConflictsOccurred
  • [x] Turn ConflictsOccurred into a Result DTO rather than an exception
  • [x] Eliminate redundancy in catch (WorkspaceRebaseFailed $e) clauses
  • [x] Trigger conflict resolution from within Publish saga
    • [x] Add conflicts case to PublishingResponse
    • [x] Add PublishingState.CONFLICTS
  • [x] 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)
  • [x] Fix: TypeError node.children is undefined (regression after https://github.com/neos/neos-ui/pull/3756 - Race Condition)
  • [x] 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)
  • [x] Fix: #3785

grebaldi avatar Apr 22 '24 15:04 grebaldi

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)

mhsdesign avatar May 15 '24 16:05 mhsdesign

@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 avatar May 15 '24 18:05 grebaldi

@pKallert FYI: While rebasing, I encountered conflicts with the fix you've provided in https://github.com/neos/neos-ui/pull/3634. To resolve this, I have moved the translation handling from BackendServiceController::publishChangesInDocumentAction to PublishChangesInDocumentCommandHandler::handle: https://github.com/neos/neos-ui/blob/faee8cad95b72ad96e2541a0b40e09d28645393b/Classes/Application/PublishChangesInDocument/PublishChangesInDocumentCommandHandler.php#L62-L74

Does that match your intend?

@mhsdesign One of the conflicting commits was one of yours, but was introduced in #3634 as well: https://github.com/neos/neos-ui/commit/29a73a166afe1f3e2a545a577494b62875262785

So, same question goes to you as well :)

grebaldi avatar Jun 14 '24 14:06 grebaldi

Yes looks perfect thanks for taking care :)

mhsdesign avatar Jun 15 '24 07:06 mhsdesign

Hi i kindof forget about this ^^ Would this be something for the next beta to include?

And while navigating the code i found that this file is no longer in use packages/neos-ui-redux-store/src/UI/SyncWorkspaceModal/index.ts so we might as well delete that? Seems its a leftover from paulas previous pr?

mhsdesign avatar Aug 28 '24 08:08 mhsdesign

That rebasing doesnt work is actually a core bug and will be resolved via: https://github.com/neos/neos-development-collection/pull/5301 ... that fixes most of the tests locally but one is still failing ...

mhsdesign avatar Oct 26 '24 22:10 mhsdesign