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.

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

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

Merged
merged 24 commits into from
Oct 29, 2024

Conversation

grebaldi
Copy link
Contributor

@grebaldi grebaldi commented Apr 22, 2024

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

solves: #3761
fixes: #3845
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
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:

} 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 added 9 commits June 28, 2024 17:20
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.
This includes the following tasks;
- Move `PublishChangesInDocument` ->
`PublishChangesInDocument\PublishChangesInDocumentCommand`
- Create `PublishChangesInDocumentCommandHandler`
- Move `PublishChangesInSite` ->
`PublishChangesInSite\PublishChangesInSiteCommand`
- Create `PublishChangesInSiteCommandHandler`
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.
This is to prevent the flakiness described in #3785.
grebaldi added 3 commits June 28, 2024 17:30
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.
…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
@mhsdesign
Copy link
Member

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?

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

There where some changes in the e2e setup and the test dont pass all on 9.0 anymore but that might be fixed here as well. But i still happen to get publish errors when running the tests locally: And the tests fail at a later point because we just click the Discard button with the id #neos-DiscardDialog-Acknowledg even if its an error:

image

mhsdesign added a commit that referenced this pull request Oct 9, 2024
The syncing tests are flaky and create catchup errors rendering following tests also kaput: #3769 (review) thus they will be skipped for now.

Also we click the `button` in chrome and not the `a` as the
> The action target (<a href="/neos/switch/to/neos-test-twodimensions-4">...</a>) is too small to be visible: 284px x 40px.
@mhsdesign
Copy link
Member

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

…rror

Currently, the `#neos-PublishDialog-Acknowledge` id will be used both for the "ok" button in success, and for the "cancel" button in case of any unexpected errors.

To make the e2e tests fail fast we change the id in the error case to `Acknowledge-Error` so we dont accidentally click it.

see also: #3769 (review)
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thank you so much ❤️ tested in combination with the new publishing v3 everything works again locally: neos/neos-development-collection#5301

(I solved conflicts with 9.0 and also pushed a few minor changes)

@mhsdesign mhsdesign merged commit 147a733 into 9.0 Oct 29, 2024
10 checks passed
@mhsdesign mhsdesign deleted the feature/conflict-resolution-03/rebase-during-publish branch October 29, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9.0 Feature Label to mark the change as feature
Projects
None yet
2 participants