Skip to content

Commit

Permalink
Merge pull request #3910 from mhsdesign/bugfix/3908-delicate-operatio…
Browse files Browse the repository at this point in the history 
…ns-after-resolving-conflict

BUGFIX: Improve conflict resolution edge-cases in sync and publish workflow
  • Loading branch information
@mhsdesign
mhsdesign authored Jan 27, 2025
2 parents 5e9e318 + 6ee4e83 commit da19324
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 66 deletions.
2 changes: 1 addition & 1 deletion Resources/Private/Translations/en/SyncWorkspaceDialog.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
<source>Discard all changes in workspace "{workspaceName}"</source>
</trans-unit>
<trans-unit id="resolutionStrategy.DISCARD_ALL.confirmation.message" xml:space="preserve">
<source>You are about to discard all changes in workspace "{workspaceName}". This includes all changes on other sites.
<source>You are about to discard all {numberOfChanges} change(s) in workspace "{workspaceName}". This includes all changes on other sites.

Do you wish to proceed? Be careful: This cannot be undone!</source>
</trans-unit>
Expand Down
62 changes: 36 additions & 26 deletions Tests/IntegrationTests/Fixtures/1Dimension/syncing.e2e.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ fixture`Syncing`
test('Syncing: Create a conflict state between two editors and choose "Discard all" as a resolution strategy during rebase', async t => {
await prepareContentElementConflictBetweenAdminAndEditor(t);
await chooseDiscardAllAsResolutionStrategy(t);
await confirmAndPerformDiscardAll(t);
await finishSynchronization(t);
await performResolutionStrategy(t);
await finishDiscard(t);

await assertThatWeAreOnPage(t, 'Home');
await assertThatWeCannotSeePageInTree(t, 'Sync Demo #1');
Expand All @@ -32,7 +32,7 @@ test('Syncing: Create a conflict state between two editors and choose "Discard a
test('Syncing: Create a conflict state between two editors and choose "Drop conflicting changes" as a resolution strategy during rebase', async t => {
await prepareContentElementConflictBetweenAdminAndEditor(t);
await chooseDropConflictingChangesAsResolutionStrategy(t);
await confirmDropConflictingChanges(t);
await performResolutionStrategy(t);
await finishSynchronization(t);

await assertThatWeAreOnPage(t, 'Home');
Expand All @@ -47,7 +47,7 @@ test('Syncing: Create a conflict state between two editors, start and cancel res
await startSynchronization(t);
await assertThatConflictResolutionHasStarted(t);
await chooseDropConflictingChangesAsResolutionStrategy(t);
await confirmDropConflictingChanges(t);
await performResolutionStrategy(t);
await finishSynchronization(t);

await assertThatWeAreOnPage(t, 'Home');
Expand All @@ -56,44 +56,61 @@ test('Syncing: Create a conflict state between two editors, start and cancel res
await assertThatWeCannotSeePageInTree(t, 'Sync Demo #3');
});

test('Syncing: Create a conflict state between two editors and choose "Drop conflicting changes" as a resolution strategy, then cancel and choose "Discard all" as a resolution strategy during rebase', async t => {
test('Syncing: Create a conflict state between two editors and switch between "Drop conflicting changes" and "Discard all" as a resolution strategy during rebase', async t => {
await prepareContentElementConflictBetweenAdminAndEditor(t);

// switch back and forth
await chooseDiscardAllAsResolutionStrategy(t);
await cancelResolutionStrategy(t);
await chooseDropConflictingChangesAsResolutionStrategy(t);
await cancelDropConflictingChanges(t);
await cancelResolutionStrategy(t);
await chooseDiscardAllAsResolutionStrategy(t);
await confirmAndPerformDiscardAll(t);
await finishSynchronization(t);

await performResolutionStrategy(t);
await finishDiscard(t);

await assertThatWeAreOnPage(t, 'Home');
await assertThatWeCannotSeePageInTree(t, 'Sync Demo #1');
await assertThatWeCannotSeePageInTree(t, 'Sync Demo #2');
await assertThatWeCannotSeePageInTree(t, 'Sync Demo #3');
});

test('Publish + Syncing: Create a conflict state between two editors, then try to publish and choose "Drop conflicting changes" as a resolution strategy during automatic rebase', async t => {
test('Publish + Syncing: Create a conflict state between two editors, then try to publish the site and choose "Drop conflicting changes" as a resolution strategy during automatic rebase', async t => {
await prepareDocumentConflictBetweenAdminAndEditor(t);
await startPublishAll(t);
await assertThatConflictResolutionHasStarted(t);
await chooseDropConflictingChangesAsResolutionStrategy(t);
await confirmDropConflictingChanges(t);
await performResolutionStrategy(t);
await finishPublish(t);

await assertThatWeAreOnPage(t, 'Home');
await assertThatWeCannotSeePageInTree(t, 'This page will be deleted during sync');
});

test('Publish + Syncing: Create a conflict state between two editors, then try to publish the document only and choose "Drop conflicting changes" as a resolution strategy during automatic rebase', async t => {
test('Publish + Syncing: Create a conflict state between two editors, then try to publish the document and choose "Drop conflicting changes" as a resolution strategy during automatic rebase', async t => {
await prepareDocumentConflictBetweenAdminAndEditor(t);
await startPublishDocument(t);
await assertThatConflictResolutionHasStarted(t);
await chooseDropConflictingChangesAsResolutionStrategy(t);
await confirmDropConflictingChanges(t);
await performResolutionStrategy(t);
await finishPublish(t);

await assertThatWeAreOnPage(t, 'Home');
await assertThatWeCannotSeePageInTree(t, 'This page will be deleted during sync');
});

test('Publish + Syncing: Create a conflict state between two editors, then try to publish the site and choose "Discard all" as a resolution strategy', async t => {
await prepareDocumentConflictBetweenAdminAndEditor(t);
await startPublishAll(t);
await assertThatConflictResolutionHasStarted(t);
await chooseDiscardAllAsResolutionStrategy(t);
await performResolutionStrategy(t);
await finishDiscard(t);

await assertThatWeAreOnPage(t, 'Home');
await assertThatWeCannotSeePageInTree(t, 'This page will be deleted during sync');
});

async function prepareContentElementConflictBetweenAdminAndEditor(t) {
await loginAsEditorOnceToInitializeAContentStreamForTheirWorkspaceIfNeeded(t);

Expand Down Expand Up @@ -317,6 +334,11 @@ async function finishPublish(t) {
await t.wait(2000);
}

async function finishDiscard(t) {
await t.click(Selector('#neos-DiscardDialog-Acknowledge'));
await t.wait(2000);
}

async function startSynchronization(t) {
await t.click(Selector('#neos-workspace-rebase'));
await t.click(Selector('#neos-SyncWorkspace-Confirm'));
Expand All @@ -332,29 +354,17 @@ async function chooseDiscardAllAsResolutionStrategy(t) {
await t.click(Selector('#neos-SelectResolutionStrategy-Accept'));
}

async function confirmAndPerformDiscardAll(t) {
await t.click(Selector('#neos-DiscardDialog-Confirm'));
await t.expect(Selector('#neos-DiscardDialog-Acknowledge').exists)
.ok('Acknowledge button for "Discard all" is not available.', {
timeout: 30000
});
// For reasons unknown, we have to press the acknowledge button really
// hard for testcafe to realize our intent...
await t.wait(500);
await t.click(Selector('#neos-DiscardDialog-Acknowledge'));
}

async function chooseDropConflictingChangesAsResolutionStrategy(t) {
await t.click(Selector('#neos-SelectResolutionStrategy-SelectBox'));
await t.click(Selector('[role="button"]').withText('Drop conflicting changes'));
await t.click(Selector('#neos-SelectResolutionStrategy-Accept'));
}

async function confirmDropConflictingChanges(t) {
async function performResolutionStrategy(t) {
await t.click(Selector('#neos-ResolutionStrategyConfirmation-Confirm'));
}

async function cancelDropConflictingChanges(t) {
async function cancelResolutionStrategy(t) {
await t.click(Selector('#neos-ResolutionStrategyConfirmation-Cancel'));
}

Expand Down
10 changes: 9 additions & 1 deletion packages/neos-ui-redux-store/src/CR/Publishing/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,16 @@ export const reducer = (state: State = defaultState, action: Action): State => {

return null;
}

switch (action.type) {
// recursive publishing start, replacing the outer process
case actionTypes.STARTED:
return {
mode: action.payload.mode,
scope: action.payload.scope,
process: {
phase: PublishingPhase.START
}
};
case actionTypes.CANCELLED:
return null;
case actionTypes.CONFIRMED:
Expand Down
28 changes: 21 additions & 7 deletions packages/neos-ui-sagas/src/Publish/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,30 @@ export function * watchPublishing({routes}: {routes: Routes}) {

if (conflictsWereResolved) {
yield put(actions.CR.Publishing.resolveConflicts());

//
// It may happen that after conflicts are resolved, the
// document we're trying to publish no longer exists.
// There are special cases after conflicts is resolved:
//
// * the document we're trying to publish no longer exists
// * the site we're trying to publish no longer contains changes
// * the document we're trying to publish no longer contains changes
//
// We need to finish the publishing operation in this
// case, otherwise it'll lead to an error.
// case, otherwise it'll lead to an error as there is nothing to do.
//
const publishingShouldContinue = scope === PublishingScope.DOCUMENT
? Boolean(yield select(selectors.CR.Nodes.byContextPathSelector(ancestorId)))
: true;
// todo possibly add another phase to actively continue publishing and also make it more transparently if publishing cant continue
// see: https://github.com/neos/neos-ui/issues/3908#issuecomment-2608232225
let publishingShouldContinue = true;
if (scope === PublishingScope.DOCUMENT) {
if (!(yield select(selectors.CR.Nodes.byContextPathSelector(ancestorId)))) {
publishingShouldContinue = false;
} else if ((yield select(selectors.CR.Workspaces.publishableNodesInDocumentSelector)).length === 0) {
publishingShouldContinue = false;
}
} else if (scope === PublishingScope.SITE) {
if ((yield select(selectors.CR.Workspaces.publishableNodesSelector)).length === 0) {
publishingShouldContinue = false;
}
}

if (publishingShouldContinue) {
yield * attemptToPublishOrDiscard();
Expand All @@ -141,6 +154,7 @@ export function * watchPublishing({routes}: {routes: Routes}) {
window.addEventListener('beforeunload', handleWindowBeforeUnload);
yield * attemptToPublishOrDiscard();
} catch (error) {
console.error(error); // log client site errors
yield put(actions.CR.Publishing.fail(error as AnyError));
} finally {
window.removeEventListener('beforeunload', handleWindowBeforeUnload);
Expand Down
28 changes: 14 additions & 14 deletions packages/neos-ui-sagas/src/Sync/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export const makeSyncPersonalWorkspace = (deps: {
yield put(actions.CR.Syncing.fail(result.error));
}
} catch (error) {
console.error(error); // log client site errors
yield put(actions.CR.Syncing.fail(error as AnyError));
} finally {
window.removeEventListener('beforeunload', handleWindowBeforeUnload);
Expand All @@ -92,7 +93,7 @@ export const makeSyncPersonalWorkspace = (deps: {
export const makeResolveConflicts = (deps: {
syncPersonalWorkspace: ReturnType<typeof makeSyncPersonalWorkspace>
}) => {
const discardAll = makeDiscardAll(deps);
const discardAll = makeDiscardAll();

function * resolveConflicts(conflicts: Conflict[]): any {
while (true) {
Expand All @@ -119,8 +120,12 @@ export const makeResolveConflicts = (deps: {
}

if (strategy === ResolutionStrategy.DISCARD_ALL) {
yield * discardAll();
return true;
if (yield * waitForResolutionConfirmation()) {
yield * discardAll();
return false; // don't continue publishing as this is a deletes all
}

continue;
}
}

Expand Down Expand Up @@ -155,16 +160,15 @@ function * waitForRetry() {
return Boolean(retried);
}

const makeDiscardAll = (deps: {
syncPersonalWorkspace: ReturnType<typeof makeSyncPersonalWorkspace>;
}) => {
const makeDiscardAll = () => {
function * discardAll() {
yield put(actions.CR.Publishing.start(
PublishingMode.DISCARD,
PublishingScope.ALL
));

const {cancelled, failed}: {
yield put(actions.CR.Publishing.confirm()); // todo auto-confirm this case
yield put(actions.CR.Syncing.finish()); // stop syncing as discarding takes now over
const {cancelled}: {
cancelled: null | ReturnType<typeof actions.CR.Publishing.cancel>;
failed: null | ReturnType<typeof actions.CR.Publishing.fail>;
finished: null | ReturnType<typeof actions.CR.Publishing.finish>;
Expand All @@ -175,13 +179,9 @@ const makeDiscardAll = (deps: {
});

if (cancelled) {
yield put(actions.CR.Syncing.cancelResolution());
} else if (failed) {
yield put(actions.CR.Syncing.finish());
} else {
yield put(actions.CR.Syncing.confirmResolution());
yield * deps.syncPersonalWorkspace(false);
yield put(actions.CR.Publishing.cancel());
}
yield put(actions.CR.Publishing.finish());
}

return discardAll;
Expand Down
4 changes: 2 additions & 2 deletions ...eos-ui/src/Containers/Modals/SyncWorkspaceDialog/ResolutionStrategyConfirmationDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ const DiscardAllConfirmationDialog: React.FC<{
/>
<I18n
id="Neos.Neos.Ui:SyncWorkspaceDialog:resolutionStrategy.DISCARD_ALL.confirmation.message"
fallback={`You are about to discard all changes in workspace "${props.workspaceName}". This includes all changes on other sites. Do you wish to proceed? Be careful: This cannot be undone!`}
params={props}
fallback={`You are about to discard all ${props.totalNumberOfChangesInWorkspace} change(s) in workspace "${props.workspaceName}". This includes all changes on other sites. Do you wish to proceed? Be careful: This cannot be undone!`}
params={{numberOfChanges: props.totalNumberOfChangesInWorkspace, workspaceName: props.workspaceName}}
/>
</div>
</Dialog>
Expand Down
27 changes: 12 additions & 15 deletions packages/neos-ui/src/Containers/Modals/SyncWorkspaceDialog/SyncWorkspaceDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,18 @@ const SyncWorkspaceDialog: React.FC<SyncWorkspaceDialogProps> = (props) => {
/>
);
case SyncingPhase.RESOLVING:
if (props.syncingState.process.strategy === ResolutionStrategy.FORCE) {
return (
<ResolutionStrategyConfirmationDialog
workspaceName={props.personalWorkspaceName}
baseWorkspaceName={props.baseWorkspaceName}
totalNumberOfChangesInWorkspace={props.totalNumberOfChangesInWorkspace}
strategy={props.syncingState.process.strategy}
conflicts={props.syncingState.process.conflicts}
i18n={props.i18nRegistry}
onCancelConflictResolution={handleCancelConflictResolution}
onConfirmResolutionStrategy={handleConfirmResolutionStrategy}
/>
);
}
return null;
return (
<ResolutionStrategyConfirmationDialog
workspaceName={props.personalWorkspaceName}
baseWorkspaceName={props.baseWorkspaceName}
totalNumberOfChangesInWorkspace={props.totalNumberOfChangesInWorkspace}
strategy={props.syncingState.process.strategy}
conflicts={props.syncingState.process.conflicts}
i18n={props.i18nRegistry}
onCancelConflictResolution={handleCancelConflictResolution}
onConfirmResolutionStrategy={handleConfirmResolutionStrategy}
/>
);
case SyncingPhase.ERROR:
case SyncingPhase.SUCCESS:
return (
Expand Down

0 comments on commit da19324

Please sign in to comment.