Desktop: Fixes #16167: Fix conflict created when syncing a just-published note (#16182)

.gitignore

@@ -1478,6 +1478,7 @@ packages/lib/models/Alarm.js
 packages/lib/models/BaseItem.test.js
 packages/lib/models/BaseItem.js
 packages/lib/models/ConflictNoteState.js
+packages/lib/models/Folder.publishing.test.js
 packages/lib/models/Folder.sharing.test.js
 packages/lib/models/Folder.test.js
 packages/lib/models/Folder.js

.ignore.eslint

@@ -1503,6 +1503,7 @@ packages/lib/models/Alarm.js
 packages/lib/models/BaseItem.test.js
 packages/lib/models/BaseItem.js
 packages/lib/models/ConflictNoteState.js
+packages/lib/models/Folder.publishing.test.js
 packages/lib/models/Folder.sharing.test.js
 packages/lib/models/Folder.test.js
 packages/lib/models/Folder.js

packages/lib/Synchronizer.ts

@@ -1248,6 +1248,17 @@ export default class Synchronizer {
 			}
 		}
 
+		try {
+			// Update published/unpublished status after the main sync to avoid conflicts.
+			// See https://github.com/laurent22/joplin/issues/16167.
+			if (!hasCaughtError && !this.cancelling()) {
+				await Note.updatePublishedNotes(this.shareService_ ? this.shareService_.shares : []);
+			}
+		} catch (error) {
+			logger.error('Failed to save note publication status', error);
+			this.progressReport_.errors.push(error);
+		}
+
 		if (syncLock) {
 			this.lockHandler().stopAutoLockRefresh(syncLock);
 			await this.lockHandler().releaseLock(LockType.Sync, this.lockClientType(), this.clientId_);

packages/lib/models/Folder.publishing.test.ts

@@ -0,0 +1,104 @@
+import { setupDatabaseAndSynchronizer, switchClient, resourceService, createFolderTree } from '../testing/test-utils';
+import Folder from '../models/Folder';
+import { ShareType, StateShare } from '../services/share/reducer';
+import Note from './Note';
+
+const publishedFolderShareState = (folderId: string): StateShare => ({
+	id: `share-${folderId}`,
+	type: ShareType.PublishedFolder,
+	folder_id: folderId,
+	note_id: '',
+	master_key_id: '',
+});
+
+type ItemSlice = { title: string };
+
+const expectPublished = async (items: ItemSlice[], published = true) => {
+	for (let item of items) {
+		item = (await Folder.loadByTitle(item.title)) ?? await Note.loadByTitle(item.title);
+		expect(item).toMatchObject({
+			title: item.title,
+			is_shared: published ? 1 : 0,
+		});
+	}
+};
+
+const expectUnpublished = (items: ItemSlice[]) => expectPublished(items, false);
+
+describe('models/Folder.publishing', () => {
+
+	beforeEach(async () => {
+		await setupDatabaseAndSynchronizer(1);
+		await switchClient(1);
+	});
+
+	it('should set is_shared=1 on descendants of a published folder', async () => {
+		const root = await createFolderTree('', [
+			{
+				title: 'root',
+				children: [
+					{
+						title: 'sub-folder 1',
+						children: [],
+					},
+					{
+						title: 'sub-folder 2',
+						children: [
+							{ title: 'sub-sub-folder', children: [] },
+							{ title: 'published note 1' },
+							{ title: 'published note 2' },
+						],
+					},
+					{ title: 'published note 3' },
+				],
+			},
+			{
+				title: 'unpublished folder',
+				children: [
+					{ title: 'unpublished note' },
+				],
+			},
+		]);
+
+		const shareState: StateShare[] = [
+			publishedFolderShareState(root.id),
+		];
+
+		await Folder.updateAllShareIds(
+			resourceService(),
+			shareState,
+		);
+
+		// After the first round, only folders should have is_shared=1
+		await expectPublished([
+			'root',
+			'sub-folder 1',
+			'sub-folder 2',
+			'sub-sub-folder',
+		].map(title => ({ title })));
+		await expectUnpublished([
+			'published note 1',
+			'published note 2',
+			'published note 3',
+			'unpublished folder',
+			'unpublished note',
+		].map(title => ({ title })));
+
+		// Should update published notes when calling Note.updateNotePublicationStatus
+		await Note.updatePublishedNotes(shareState);
+
+		await expectPublished([
+			'root',
+			'sub-folder 1',
+			'sub-folder 2',
+			'sub-sub-folder',
+			'published note 1',
+			'published note 2',
+			'published note 3',
+		].map(title => ({ title })));
+		await expectUnpublished([
+			'unpublished folder',
+			'unpublished note',
+		].map(title => ({ title })));
+	});
+});

packages/lib/models/Folder.ts

@@ -781,19 +781,24 @@ export default class Folder extends BaseItem {
 	public static async updateAllShareIds(resourceService: ResourceService, activeShares: StateShare[]) {
 		await this.updateFolderShareIds(activeShares);
 		await this.updateNoteShareIds();
+		await this.updateResourceShareIds(resourceService);
 
+		// Don't update note publication status here: Doing so can cause conflicts if updateAllShareIds
+		// is called just before sync
+		await this.updateFolderPublishStatus_(activeShares);
+	}
+
+	private static async updateFolderPublishStatus_(activeShares: StateShare[]) {
 		const publishedFolderRootIds = activeShares
 			.filter(share => share.type === ShareType.PublishedFolder && !!share.folder_id)
 			.map(share => share.folder_id);
-		const directlyPublishedNoteIds = activeShares
-			.filter(share => share.type === ShareType.Note && !!share.note_id)
-			.map(share => share.note_id);
 		const publishedFolderIds = unique(publishedFolderRootIds.concat(
-			...(await Promise.all(publishedFolderRootIds.map(id => this.allChildrenFolders(id)))).map(folders => folders.map(f => f.id)),
+			(await Promise.all(
+				publishedFolderRootIds.map(id => this.allChildrenFolders(id)),
+			)).flatMap(folders => folders.map(f => f.id)),
 		));
 
 		const publishedFolderIdSet = new Set(publishedFolderIds);
-		const directlyPublishedNoteIdSet = new Set(directlyPublishedNoteIds);
 
 		if (publishedFolderIds.length) {
 			for (const folder of await this.all({ fields: ['id', 'is_shared'] })) {
@@ -801,29 +806,6 @@ export default class Folder extends BaseItem {
 				await this.updateShareStatus({ ...folder, type_: BaseModel.TYPE_FOLDER }, true);
 			}
 		}
-
-		const noteIsSharedSql = [
-			directlyPublishedNoteIds.length ? `id IN (${this.escapeIdsForSql(directlyPublishedNoteIds)})` : '',
-			publishedFolderIds.length ? `parent_id IN (${this.escapeIdsForSql(publishedFolderIds)})` : '',
-		].filter(v => !!v).join(' OR ');
-
-		let notesToUpdate: NoteEntity[] = [];
-		if (noteIsSharedSql) {
-			notesToUpdate = await this.db().selectAll(`
-				SELECT id, parent_id, is_shared
-				FROM notes
-				WHERE is_shared = 0 AND (${noteIsSharedSql})
-			`);
-		}
-
-		for (const note of notesToUpdate) {
-			await this.updateShareStatus(
-				{ ...note, type_: BaseModel.TYPE_NOTE },
-				directlyPublishedNoteIdSet.has(note.id) || publishedFolderIdSet.has(note.parent_id),
-			);
-		}
-
-		await this.updateResourceShareIds(resourceService);
 	}
 
 	// Clear the "share_id" property for the items that are associated with a

packages/lib/models/Note.ts

@@ -29,6 +29,7 @@ import { ALL_NOTES_FILTER_ID } from '../reserved-ids';
 import NoteLockNote from '../services/noteLock/NoteLockNote';
 import isNoteLockEnabled from '../services/noteLock/isNoteLockEnabled';
 import isItemId from './utils/isItemId';
+import { ShareType, StateShare } from '../services/share/reducer';
 
 export interface PreviewsOrder {
 	by: string;
@@ -599,6 +600,38 @@ export default class Note extends BaseItem {
 		return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 0');
 	}
 
+	public static async updatePublishedNotes(activeShares: StateShare[]) {
+		const directlyPublishedNoteIds = activeShares
+			.filter(share => share.type === ShareType.Note && !!share.note_id)
+			.map(share => share.note_id);
+
+		const loadUnpublishedWithDirectShare = async (): Promise<NoteEntity[]> => {
+			if (directlyPublishedNoteIds.length === 0) return [];
+
+			return await this.db().selectAll(`
+				SELECT id, parent_id, is_shared
+				FROM notes
+				WHERE is_shared = 0 AND id IN (${this.escapeIdsForSql(directlyPublishedNoteIds)})
+			`);
+		};
+		const unpublishedNotesInPublishedFolders: NoteEntity[] = await this.db().selectAll(`
+			SELECT notes.id, notes.parent_id, notes.is_shared
+			FROM notes
+			JOIN folders ON notes.parent_id = folders.id
+			WHERE notes.is_shared = 0 AND folders.is_shared = 1
+				AND notes.is_conflict = 0
+				AND notes.deleted_time = 0
+		`);
+
+		const notesToPublish = unpublishedNotesInPublishedFolders.concat(await loadUnpublishedWithDirectShare());
+		for (const note of notesToPublish) {
+			await this.updateShareStatus(
+				{ ...note, type_: BaseModel.TYPE_NOTE },
+				true,
+			);
+		}
+	}
+
 	public static async updateGeolocation(noteId: string): Promise<NoteEntity | null> {
 		if (!Setting.value('trackLocation')) return null;
 		if (!Note.updateGeolocationEnabled_) return null;