Desktop,Mobile: Resolves #10073, #10080: Fix conflicts notebook doesn't work with the trash feature (#10104)

.eslintignore

@@ -828,12 +828,14 @@ packages/lib/models/Tag.js
 packages/lib/models/dateTimeFormats.test.js
 packages/lib/models/settings/FileHandler.js
 packages/lib/models/settings/settingValidations.js
+packages/lib/models/utils/getConflictFolderId.js
 packages/lib/models/utils/isItemId.js
 packages/lib/models/utils/itemCanBeEncrypted.js
 packages/lib/models/utils/onFolderDrop.test.js
 packages/lib/models/utils/onFolderDrop.js
 packages/lib/models/utils/paginatedFeed.js
 packages/lib/models/utils/paginationToSql.js
+packages/lib/models/utils/readOnly.test.js
 packages/lib/models/utils/readOnly.js
 packages/lib/models/utils/resourceUtils.js
 packages/lib/models/utils/types.js
@@ -1082,8 +1084,11 @@ packages/lib/services/synchronizer/utils/syncDeleteStep.js
 packages/lib/services/synchronizer/utils/types.js
 packages/lib/services/trash/emptyTrash.test.js
 packages/lib/services/trash/emptyTrash.js
+packages/lib/services/trash/getTrashFolderId.js
 packages/lib/services/trash/index.test.js
 packages/lib/services/trash/index.js
+packages/lib/services/trash/isTrashableItem.js
+packages/lib/services/trash/isTrashableNoteOrFolder.js
 packages/lib/services/trash/permanentlyDeleteOldItems.test.js
 packages/lib/services/trash/permanentlyDeleteOldItems.js
 packages/lib/services/trash/restoreItems.test.js

.gitignore

@@ -808,12 +808,14 @@ packages/lib/models/Tag.js
 packages/lib/models/dateTimeFormats.test.js
 packages/lib/models/settings/FileHandler.js
 packages/lib/models/settings/settingValidations.js
+packages/lib/models/utils/getConflictFolderId.js
 packages/lib/models/utils/isItemId.js
 packages/lib/models/utils/itemCanBeEncrypted.js
 packages/lib/models/utils/onFolderDrop.test.js
 packages/lib/models/utils/onFolderDrop.js
 packages/lib/models/utils/paginatedFeed.js
 packages/lib/models/utils/paginationToSql.js
+packages/lib/models/utils/readOnly.test.js
 packages/lib/models/utils/readOnly.js
 packages/lib/models/utils/resourceUtils.js
 packages/lib/models/utils/types.js
@@ -1062,8 +1064,11 @@ packages/lib/services/synchronizer/utils/syncDeleteStep.js
 packages/lib/services/synchronizer/utils/types.js
 packages/lib/services/trash/emptyTrash.test.js
 packages/lib/services/trash/emptyTrash.js
+packages/lib/services/trash/getTrashFolderId.js
 packages/lib/services/trash/index.test.js
 packages/lib/services/trash/index.js
+packages/lib/services/trash/isTrashableItem.js
+packages/lib/services/trash/isTrashableNoteOrFolder.js
 packages/lib/services/trash/permanentlyDeleteOldItems.test.js
 packages/lib/services/trash/permanentlyDeleteOldItems.js
 packages/lib/services/trash/restoreItems.test.js

packages/lib/models/Folder.ts

@@ -13,7 +13,9 @@ import syncDebugLog from '../services/synchronizer/syncDebugLog';
 import ResourceService from '../services/ResourceService';
 import { LoadOptions } from './utils/types';
 import ActionLogger from '../utils/ActionLogger';
-import { getTrashFolder, getTrashFolderId } from '../services/trash';
+import { getTrashFolder } from '../services/trash';
+import getConflictFolderId from './utils/getConflictFolderId';
+import getTrashFolderId from '../services/trash/getTrashFolderId';
 const { substrWithEllipsis } = require('../string-utils.js');
 
 const logger = Logger.create('models/Folder');
@@ -162,7 +164,7 @@ export default class Folder extends BaseItem {
 	}
 
 	public static conflictFolderId() {
-		return 'c04f1c7c04f1c7c04f1c7c04f1c7c04f';
+		return getConflictFolderId();
 	}
 
 	public static conflictFolder(): FolderEntity {

packages/lib/models/Note.test.ts

@@ -14,6 +14,7 @@ import * as ArrayUtils from '../ArrayUtils';
 import { ErrorCode } from '../errors';
 import SearchEngine from '../services/search/SearchEngine';
 import { getTrashFolderId } from '../services/trash';
+import getConflictFolderId from './utils/getConflictFolderId';
 
 async function allItems() {
 	const folders = await Folder.all();
@@ -581,4 +582,26 @@ describe('models/Note', () => {
 		}
 	});
 
+	it('should allow deleting conflicts to the trash', async () => {
+		const folder = await Folder.save({});
+
+		const notes = [];
+		for (let i = 0; i < 3; i++) {
+			notes.push(await Note.save({ title: `note${i}`, parent_id: folder.id, is_conflict: 1 }));
+		}
+
+		// Should be in the conflicts folder
+		expect(await Note.previews(getTrashFolderId())).toHaveLength(0);
+		expect(await Note.previews(getConflictFolderId())).toHaveLength(3);
+		expect(await Note.conflictedCount()).toBe(3);
+
+		// Should be moved to the trash folder on delete
+		for (const note of notes) {
+			await Note.delete(note.id, { toTrash: true });
+		}
+
+		expect(await Note.previews(getTrashFolderId())).toHaveLength(3);
+		expect(await Note.previews(getConflictFolderId())).toHaveLength(0);
+		expect(await Note.conflictedCount()).toBe(0);
+	});
 });

packages/lib/models/Note.ts

@@ -405,6 +405,7 @@ export default class Note extends BaseItem {
 
 		if (parentId === Folder.conflictFolderId()) {
 			options.conditions.push('is_conflict = 1');
+			options.conditions.push('deleted_time = 0');
 		} else if (withinTrash) {
 			options.conditions.push('deleted_time > 0');
 		} else {
@@ -502,7 +503,8 @@ export default class Note extends BaseItem {
 
 	public static preview(noteId: string, options: any = null) {
 		if (!options) options = { fields: null };
-		return this.modelSelectOne(`SELECT ${this.previewFieldsSql(options.fields)} FROM notes WHERE is_conflict = 0 AND id = ?`, [noteId]);
+		const excludeConflictsSql = options.excludeConflicts ? 'is_conflict = 0 AND' : '';
+		return this.modelSelectOne(`SELECT ${this.previewFieldsSql(options.fields)} FROM notes WHERE ${excludeConflictsSql} id = ?`, [noteId]);
 	}
 
 	public static async search(options: any = null): Promise<NoteEntity[]> {
@@ -520,11 +522,11 @@ export default class Note extends BaseItem {
 	}
 
 	public static conflictedNotes() {
-		return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 1');
+		return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 1 AND deleted_time = 0');
 	}
 
 	public static async conflictedCount() {
-		const r = await this.db().selectOne('SELECT count(*) as total FROM notes WHERE is_conflict = 1');
+		const r = await this.db().selectOne('SELECT count(*) as total FROM notes WHERE is_conflict = 1 AND deleted_time = 0');
 		return r && r.total ? r.total : 0;
 	}
 

packages/lib/models/utils/getConflictFolderId.ts

@@ -0,0 +1,2 @@
+
+export default () => 'c04f1c7c04f1c7c04f1c7c04f1c7c04f';

packages/lib/models/utils/readOnly.test.ts

@@ -0,0 +1,67 @@
+import { ModelType } from '../../BaseModel';
+import Folder from '../Folder';
+import ItemChange from '../ItemChange';
+import Note from '../Note';
+import { defaultState as defaultShareState, State as ShareState } from '../../services/share/reducer';
+import { ItemSlice, itemIsReadOnlySync } from './readOnly';
+import Resource from '../Resource';
+import shim from '../../shim';
+import { setupDatabaseAndSynchronizer, simulateReadOnlyShareEnv, switchClient, tempFilePath } from '../../testing/test-utils';
+import BaseItem from '../BaseItem';
+
+
+const checkReadOnly = (itemType: ModelType, item: ItemSlice, shareData: ShareState = defaultShareState) => {
+	const syncUserId = '';
+	return itemIsReadOnlySync(itemType, ItemChange.SOURCE_UNSPECIFIED, item, syncUserId, shareData);
+};
+
+const createTestResource = async () => {
+	const tempFile = tempFilePath('txt');
+	await shim.fsDriver().writeFile(tempFile, 'Test', 'utf8');
+	const note1 = await Note.save({ title: 'note' });
+	await shim.attachFileToNote(note1, tempFile);
+};
+
+describe('readOnly', () => {
+	beforeEach(async () => {
+		await setupDatabaseAndSynchronizer(1);
+		await switchClient(1);
+	});
+
+	test('trashed items should be marked as read-only', async () => {
+		let folder = await Folder.save({ title: 'Test' });
+		let note = await Note.save({ parent_id: folder.id, title: 'Test note' });
+
+		expect(checkReadOnly(ModelType.Note, note as ItemSlice)).toBe(false);
+		expect(checkReadOnly(ModelType.Folder, folder as ItemSlice)).toBe(false);
+
+		await Folder.delete(folder.id, { toTrash: true });
+
+		// Should be deleted
+		note = await Note.load(note.id);
+		expect(note.deleted_time).not.toBe(0);
+		folder = await Folder.load(folder.id);
+		expect(folder.deleted_time).not.toBe(0);
+
+		expect(checkReadOnly(ModelType.Note, note as ItemSlice)).toBe(true);
+		expect(checkReadOnly(ModelType.Folder, folder as ItemSlice)).toBe(true);
+	});
+
+	test('should support checking if resources are not read-only', async () => {
+		await createTestResource();
+		const resource = (await Resource.all())[0];
+		expect(checkReadOnly(ModelType.Resource, resource)).toBe(false);
+	});
+
+	test('should support checking that resources are read-only due to a share', async () => {
+		await createTestResource();
+
+		const share_id = '123456';
+		let resource = (await Resource.all())[0];
+		resource = await Resource.save({ ...resource, share_id });
+
+		const cleanup = simulateReadOnlyShareEnv(share_id);
+		expect(checkReadOnly(ModelType.Resource, resource, BaseItem.syncShareCache)).toBe(true);
+		cleanup();
+	});
+});

packages/lib/models/utils/readOnly.ts

@@ -6,6 +6,7 @@ import { State as ShareState } from '../../services/share/reducer';
 import ItemChange from '../ItemChange';
 import Setting from '../Setting';
 import { checkObjectHasProperties } from '@joplin/utils/object';
+import isTrashableItem from '../../services/trash/isTrashableItem';
 
 const logger = Logger.create('models/utils/readOnly');
 
@@ -75,13 +76,17 @@ export const checkIfItemCanBeAddedToFolder = async (itemType: ModelType, Folder:
 // extra `sharePermissionCheckOnly` boolean to do the check for one case or the other. A bit of a
 // hack but good enough for now.
 export const itemIsReadOnlySync = (itemType: ModelType, changeSource: number, item: ItemSlice, userId: string, shareState: ShareState, sharePermissionCheckOnly = false): boolean => {
-	checkObjectHasProperties(item, sharePermissionCheckOnly ? ['share_id'] : ['share_id', 'deleted_time']);
+	if (!sharePermissionCheckOnly && isTrashableItem(itemType, item)) {
+		checkObjectHasProperties(item, ['deleted_time']);
+	}
 
 	// Item is in trash
 	if (!sharePermissionCheckOnly && item.deleted_time) return true;
 
 	if (!needsShareReadOnlyChecks(itemType, changeSource, shareState)) return false;
 
+	checkObjectHasProperties(item, ['share_id']);
+
 	// Item is not shared
 	if (!item.share_id) return false;
 

packages/lib/services/trash/getTrashFolderId.ts

@@ -0,0 +1,6 @@
+
+const getTrashFolderId = () => {
+	return 'de1e7ede1e7ede1e7ede1e7ede1e7ede';
+};
+
+export default getTrashFolderId;

packages/lib/services/trash/index.test.ts

@@ -1,4 +1,5 @@
 import { getDisplayParentId, getTrashFolderId } from '.';
+import { ModelType } from '../../BaseModel';
 
 describe('services/trash', () => {
 
@@ -7,9 +8,11 @@ describe('services/trash', () => {
 			{
 				deleted_time: 0,
 				parent_id: '1',
+				id: 'a',
 			},
 			{
 				deleted_time: 0,
+				id: '1',
 			},
 			'1',
 		],
@@ -17,9 +20,11 @@ describe('services/trash', () => {
 			{
 				deleted_time: 1000,
 				parent_id: '1',
+				id: 'b',
 			},
 			{
 				deleted_time: 0,
+				id: '1',
 			},
 			getTrashFolderId(),
 		],
@@ -27,14 +32,17 @@ describe('services/trash', () => {
 			{
 				deleted_time: 1000,
 				parent_id: '1',
+				id: 'a',
 			},
 			{
 				deleted_time: 1000,
+				id: '1',
 			},
 			'1',
 		],
-	])('should return the display parent ID', (item, itemParent, expected) => {
-		const actual = getDisplayParentId(item, itemParent);
+	])('should return the display parent ID (case %#)', (item, itemParent, expected) => {
+		const defaultProps = { type_: ModelType.Folder };
+		const actual = getDisplayParentId({ ...defaultProps, ...item }, { ...defaultProps, ...itemParent });
 		expect(actual).toBe(expected);
 	});
 

packages/lib/services/trash/index.ts

@@ -3,6 +3,8 @@ import { ModelType } from '../../BaseModel';
 import { _ } from '../../locale';
 import { FolderEntity, FolderIcon, FolderIconType, NoteEntity } from '../database/types';
 import Folder from '../../models/Folder';
+import getTrashFolderId from './getTrashFolderId';
+import isTrashableNoteOrFolder from './isTrashableNoteOrFolder';
 
 // When an item is deleted, all its properties are kept, including the parent ID
 // so that it can potentially be restored to the right folder. However, when
@@ -17,8 +19,17 @@ import Folder from '../../models/Folder';
 // `originalItemParent` is the parent before the item was deleted, which is the
 // folder with ID = item.parent_id
 export const getDisplayParentId = (item: FolderEntity | NoteEntity, originalItemParent: FolderEntity) => {
-	if (!('deleted_time' in item) || !('parent_id' in item)) throw new Error(`Missing "deleted_time" or "parent_id" property: ${JSON.stringify(item)}`);
-	if (originalItemParent && !('deleted_time' in originalItemParent)) throw new Error(`Missing "deleted_time" property: ${JSON.stringify(originalItemParent)}`);
+	if (!('parent_id' in item)) throw new Error(`Missing "parent_id" property: ${JSON.stringify(item)}`);
+	if (!isTrashableNoteOrFolder(item)) {
+		return item.parent_id;
+	}
+
+	if (!('deleted_time' in item)) {
+		throw new Error(`Missing "deleted_time" property: ${JSON.stringify(item)}`);
+	}
+	if (originalItemParent && isTrashableNoteOrFolder(originalItemParent) && !('deleted_time' in originalItemParent)) {
+		throw new Error(`Missing "deleted_time" property: ${JSON.stringify(originalItemParent)}`);
+	}
 
 	if (!item.deleted_time) return item.parent_id;
 
@@ -33,10 +44,6 @@ export const getDisplayParentTitle = (item: FolderEntity | NoteEntity, originalI
 	return originalItemParent && originalItemParent.id === displayParentId ? originalItemParent.title : '';
 };
 
-export const getTrashFolderId = () => {
-	return 'de1e7ede1e7ede1e7ede1e7ede1e7ede';
-};
-
 export const getTrashFolderTitle = () => {
 	return _('Trash');
 };
@@ -77,6 +84,7 @@ export const getTrashFolderIcon = (type: FolderIconType): FolderIcon => {
 
 export const itemIsInTrash = (item: FolderEntity | NoteEntity) => {
 	if (!item) return false;
+	if (!isTrashableNoteOrFolder(item)) return false;
 
 	checkObjectHasProperties(item, ['id', 'deleted_time']);
 
@@ -89,3 +97,5 @@ export const getRestoreFolder = async () => {
 	if (output) return output;
 	return Folder.save({ title });
 };
+
+export { getTrashFolderId };

packages/lib/services/trash/isTrashableItem.ts

@@ -0,0 +1,16 @@
+import { checkObjectHasProperties } from '@joplin/utils/object';
+import { ModelType } from '../../BaseModel';
+import isTrashableNoteOrFolder from './isTrashableNoteOrFolder';
+
+type ItemSlice = { id?: string };
+const isTrashableItem = (itemType: ModelType, item: ItemSlice) => {
+	checkObjectHasProperties(item, ['id']);
+
+	if (itemType !== ModelType.Folder && itemType !== ModelType.Note) {
+		return false;
+	}
+
+	return isTrashableNoteOrFolder(item);
+};
+
+export default isTrashableItem;

packages/lib/services/trash/isTrashableNoteOrFolder.ts

@@ -0,0 +1,14 @@
+import { checkObjectHasProperties } from '@joplin/utils/object';
+import conflictFolderId from '../../models/utils/getConflictFolderId';
+import getTrashFolderId from './getTrashFolderId';
+
+type ItemSlice = { id?: string };
+
+// This function is separate from isTrashableItem to handle the case where we know that an item
+// is either a note or a folder, but don't know which.
+const isTrashableNoteOrFolder = (item: ItemSlice) => {
+	checkObjectHasProperties(item, ['id']);
+	return item.id !== conflictFolderId() && item.id !== getTrashFolderId();
+};
+
+export default isTrashableNoteOrFolder;

packages/lib/services/trash/restoreItems.test.ts

@@ -88,4 +88,15 @@ describe('restoreItems', () => {
 		expect(noteReloaded.parent_id).toBe(folderReloaded2.id);
 	});
 
+	it('should restore a conflict', async () => {
+		const note = await Note.save({ is_conflict: 1, title: 'Test' });
+		await Note.delete(note.id, { toTrash: true });
+
+		await restoreItems(ModelType.Note, [await Note.load(note.id)]);
+
+		const noteReloaded = await Note.load(note.id);
+		expect(noteReloaded.title).toBe('Test');
+		expect(noteReloaded.is_conflict).toBe(1);
+		expect(noteReloaded.deleted_time).toBe(0);
+	});
 });

packages/lib/services/trash/restoreItems.ts

@@ -54,7 +54,8 @@ const restoreItems = async (itemType: ModelType, itemsOrIds: NoteEntity[] | Fold
 		let toSave: FolderEntity | NoteEntity = null;
 
 		if (itemType === ModelType.Note) {
-			toSave = await Note.preview(item.id);
+			// We need to preview conflicts -- they can be trashed.
+			toSave = await Note.preview(item.id, { excludeConflicts: false });
 		} else {
 			toSave = await Folder.load(item.id);
 		}

packages/tools/cspell/dictionary4.txt

@@ -95,3 +95,4 @@ Notyf
 unresponded
 activeline
 Prec
+Trashable