Merge pull request #6097 from nextcloud/backport/6094/stable28

[stable28] No conflict dialogue in read only

cypress/e2e/conflict.spec.js

@@ -32,28 +32,43 @@ const variants = [
 variants.forEach(function({ fixture, mime }) {
 	const fileName = fixture
 	describe(`${mime} (${fileName})`, function() {
-		const getWrapper = () => cy.get('.viewer__content .text-editor__wrapper.has-conflicts')
+		const getWrapper = () => cy.get('.text-editor__wrapper.has-conflicts')
 
 		before(() => {
 			initUserAndFiles(user, fileName)
 		})
 
 		beforeEach(function() {
 			cy.login(user)
-			cy.visit('/apps/files')
 		})
 
-		it('displays conflicts', function() {
-			cy.openFile(fileName)
-
-			cy.log('Inspect editor')
-			cy.getContent()
-				.type('Hello you cruel conflicting world')
+		it('no actual conflict - just reload', function() {
+			// start with different content
+			cy.uploadFile('frontmatter.md', mime, fileName)
+			// just a read only session opened
+			cy.shareFile(`/${fileName}`)
+				.then(token => cy.visit(`/s/${token}`))
+			cy.getContent().should('contain', 'Heading')
+			cy.intercept({ method: 'POST', url: '**/session/*/push' })
+				.as('push')
+			cy.wait('@push')
 			cy.uploadFile(fileName, mime)
+			cy.get('#editor-container .document-status', { timeout: 30000 })
+				.should('contain', 'session has expired')
+			// Reload button works
+			cy.get('#editor-container .document-status a.button')
+				.contains('Reload')
+				.click()
+			getWrapper().should('not.exist')
+			cy.getContent().should('contain', 'Hello world')
+			cy.getContent().should('not.contain', 'Heading')
+		})
+
+		it('displays conflicts', function() {
+			createConflict(fileName, mime)
 
-			cy.get('#viewer .modal-header button.header-close').click()
-			cy.get('#viewer').should('not.exist')
 			cy.openFile(fileName)
+
 			cy.get('.text-editor .document-status')
 				.should('contain', 'Document has been changed outside of the editor.')
 			getWrapper()
@@ -68,57 +83,55 @@ variants.forEach(function({ fixture, mime }) {
 		})
 
 		it('resolves conflict using current editing session', function() {
-			cy.openFile(fileName)
-
-			cy.log('Inspect editor')
-			cy.getContent()
-				.type('Hello you cruel conflicting world')
-			cy.uploadFile(fileName, mime)
+			createConflict(fileName, mime)
 
-			cy.get('#viewer .modal-header button.header-close').click()
-			cy.get('#viewer').should('not.exist')
 			cy.openFile(fileName)
-
 			cy.get('[data-cy="resolveThisVersion"]').click()
 
-			getWrapper()
-				.should('not.exist')
-
+			getWrapper().should('not.exist')
 			cy.get('[data-cy="resolveThisVersion"]')
 				.should('not.exist')
-
-			cy.get('.text-editor__main')
-				.should('contain', 'Hello world')
-			cy.get('.text-editor__main')
-				.should('contain', 'cruel conflicting')
+			cy.getContent().should('contain', 'Hello world')
+			cy.getContent().should('contain', 'cruel conflicting')
 		})
 
 		it('resolves conflict using server version', function() {
-			cy.openFile(fileName)
-
-			cy.log('Inspect editor')
-			cy.getContent()
-				.type('Hello you cruel conflicting world')
-			cy.uploadFile(fileName, mime)
+			createConflict(fileName, mime)
 
-			cy.get('#viewer .modal-header button.header-close').click()
-			cy.get('#viewer').should('not.exist')
 			cy.openFile(fileName)
-
 			cy.get('[data-cy="resolveServerVersion"]')
 				.click()
 
-			getWrapper()
-				.should('not.exist')
+			getWrapper().should('not.exist')
 			cy.get('[data-cy="resolveThisVersion"]')
 				.should('not.exist')
 			cy.get('[data-cy="resolveServerVersion"]')
 				.should('not.exist')
+			cy.getContent().should('contain', 'Hello world')
+			cy.getContent().should('not.contain', 'cruel conflicting')
+		})
 
-			cy.get('.text-editor__main')
-				.should('contain', 'Hello world')
-			cy.get('.text-editor__main')
-				.should('not.contain', 'cruel conflicting')
+		it('hides conflict in read only session', function() {
+			createConflict(fileName, mime)
+			cy.shareFile(`/${fileName}`)
+				.then((token) => {
+					cy.logout()
+					cy.visit(`/s/${token}`)
+				})
+			cy.getContent().should('contain', 'cruel conflicting')
+			getWrapper().should('not.exist')
 		})
+
 	})
 })
+
+function createConflict(fileName, mime) {
+	cy.visit('/apps/files')
+	cy.openFile(fileName)
+	cy.log('Inspect editor')
+	cy.getContent()
+		.type('Hello you cruel conflicting world')
+	cy.uploadFile(fileName, mime)
+	cy.get('#viewer .modal-header button.header-close').click()
+	cy.get('#viewer').should('not.exist')
+}

lib/Listeners/BeforeNodeDeletedListener.php

@@ -50,9 +50,12 @@ public function handle(Event $event): void {
 			return;
 		}
 		$node = $event->getNode();
-		if ($node instanceof File && $node->getMimeType() === 'text/markdown') {
+		if (!$node instanceof File) {
+			return;
+		}
+		if ($node->getMimeType() === 'text/markdown') {
 			$this->attachmentService->deleteAttachments($node);
-			$this->documentService->resetDocument($node->getId(), true);
 		}
+		$this->documentService->resetDocument($node->getId(), true);
 	}
 }

lib/Listeners/BeforeNodeWrittenListener.php

@@ -31,7 +31,6 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\Files\Events\Node\BeforeNodeWrittenEvent;
 use OCP\Files\File;
-use OCP\Files\NotFoundException;
 
 /**
  * @template-implements IEventListener<Event|BeforeNodeWrittenEvent>
@@ -48,19 +47,17 @@ public function handle(Event $event): void {
 			return;
 		}
 		$node = $event->getNode();
+		if (!$node instanceof File) {
+			return;
+		}
+		if ($this->documentService->isSaveFromText()) {
+			return;
+		}
+		// Reset document session to avoid manual conflict resolution if there's no unsaved steps
 		try {
-			if ($node instanceof File && $node->getMimeType() === 'text/markdown') {
-				if (!$this->documentService->isSaveFromText()) {
-					// Reset document session to avoid manual conflict resolution if there's no unsaved steps
-					try {
-						$this->documentService->resetDocument($node->getId());
-					} catch (DocumentHasUnsavedChangesException) {
-						// Do not throw during event handling in this is expected to happen
-					}
-				}
-			}
-		} catch (NotFoundException) {
-			// This might occur on new files when a NonExistingFile is passed and we cannot access the mimetype
+			$this->documentService->resetDocument($node->getId());
+		} catch (DocumentHasUnsavedChangesException) {
+			// Do not throw during event handling in this is expected to happen
 		}
 	}
 }

src/components/Editor.vue

@@ -29,13 +29,14 @@
 		<DocumentStatus v-if="displayedStatus"
 			:idle="idle"
 			:lock="lock"
+			:is-resolving-conflict="isResolvingConflict"
 			:sync-error="syncError"
 			:has-connection-issue="hasConnectionIssue"
 			@reconnect="reconnect" />
 
 		<SkeletonLoading v-if="!contentLoaded && !displayedStatus" />
 		<Wrapper v-if="displayed"
-			:sync-error="syncError"
+			:is-resolving-conflict="isResolvingConflict"
 			:has-connection-issue="hasConnectionIssue"
 			:content-loaded="contentLoaded"
 			:show-author-annotations="showAuthorAnnotations"
@@ -72,7 +73,7 @@
 				<ContentContainer v-show="contentLoaded"
 					ref="contentWrapper" />
 			</MainContainer>
-			<Reader v-if="hasSyncCollission"
+			<Reader v-if="isResolvingConflict"
 				:content="syncError.data.outsideChange"
 				:is-rich-editor="isRichEditor" />
 		</Wrapper>
@@ -258,6 +259,9 @@ export default {
 		isRichWorkspace() {
 			return this.richWorkspace
 		},
+		isResolvingConflict() {
+			return this.hasSyncCollission && !this.readOnly
+		},
 		hasSyncCollission() {
 			return this.syncError && this.syncError.type === ERROR_TYPE.SAVE_COLLISSION
 		},

src/components/Editor/DocumentStatus.vue

@@ -51,7 +51,7 @@
 			</p>
 		</NcNoteCard>
 
-		<CollisionResolveDialog v-if="hasSyncCollission" :sync-error="syncError" />
+		<CollisionResolveDialog v-if="isResolvingConflict" :sync-error="syncError" />
 	</div>
 </template>
 
@@ -89,6 +89,10 @@ export default {
 			type: Boolean,
 			require: true,
 		},
+		isResolvingConflict: {
+			type: Boolean,
+			require: true,
+		},
 	},
 
 	data() {

src/components/Editor/Wrapper.vue

@@ -23,7 +23,7 @@
 <template>
 	<div class="text-editor__wrapper"
 		:class="{
-			'has-conflicts': hasSyncCollission,
+			'has-conflicts': isResolvingConflict,
 			'is-rich-workspace': $isRichWorkspace,
 			'is-rich-editor': $isRichEditor,
 			'show-color-annotations': showAuthorAnnotations
@@ -33,7 +33,6 @@
 </template>
 
 <script>
-import { ERROR_TYPE } from './../../services/SyncService.js'
 import { useIsRichEditorMixin, useIsRichWorkspaceMixin } from './../Editor.provider.js'
 import { OUTLINE_STATE, OUTLINE_ACTIONS } from './Wrapper.provider.js'
 import useStore from '../../mixins/store.js'
@@ -60,9 +59,9 @@ export default {
 	},
 
 	props: {
-		syncError: {
-			type: Object,
-			default: null,
+		isResolvingConflict: {
+			type: Boolean,
+			require: true,
 		},
 		hasConnectionIssue: {
 			type: Boolean,
@@ -93,10 +92,6 @@ export default {
 		...mapState({
 			viewWidth: (state) => state.text.viewWidth,
 		}),
-
-		hasSyncCollission() {
-			return this.syncError && this.syncError.type === ERROR_TYPE.SAVE_COLLISSION
-		},
 		showOutline() {
 			return this.isAbleToShowOutline
 				? this.outline.visible