Desktop,Mobile,Cli: Render misplaced notebooks

[personalizedrefrigerator]

Summary

This pull shows subnotebooks that have a nonexistent parent notebook in the sidebar. Previously, such notebooks were not shown at all.

This change may help users recover from issues similar to #12075 (depending on the cause of #12075).

This pull request is currently a draft, pending refactoring.

Details

It's possible, in some cases, for a notebook to be assigned a parent notebook that does not exist yet or no longer exists (e.g. an incomplete sync, a server bug). Prior to this pull request, such notebooks were not rendered in the notebook tree. With this pull request, these notebooks are rendered in a "Misplaced" notebook.

Screenshot

Tasks

• Add more automated tests.
  • Test Note.previews when the "misplaced" folder is selected.
• Mobile: Verify that it isn't possible to create new notes in the misplaced folder.
  • There's an issue here! This needs to be fixed.
• Mobile: Verify that it isn't possible to rename or move the misplaced folder.
• Mobile: Verify that it isn't possible to move notes or folders to the misplaced folder.
• Desktop or mobile: Verify that this doesn't cause issues if a folder is being deleted by sync.
  • For example, in the case where a parent folder is deleted earlier in the sync process than child folders.
• Desktop: Check how this interacts with the trash folder, particularly for notes.
• Render the "Misplaced" folder near the Conflicts folder (rather than below the trash folder).
• Change the name of the "Misplaced" folder. Folders may be included within "Misplaced" because the parent folder hasn't yet been downloaded from the server.
  • Avoid rendering notes that were added by the current sync. Look at sync_updated_time.
  • Possible alternate name: "...", "partially synced".
    • Consider asking on the forum.
• Desktop: Verify that it isn't possible to create notes or subfolders of the "Misplaced" folder.
• Display notes with an invalid parent_id directly in the "Misplaced" folder.
  • These notes (prior to this pull request) should also be included in "All notes".
• Fix misplaced notebook is hidden when there are misplaced notes, but not notebooks.

Thank you @mrjo118 for helping test this pull request!

[mrjo118]

Joplin server has a task which cleans up orphaned notebooks periodically on the server side (according to code comments it is to prevent some potential key violation). I believe they are the same thing as misplaced notes, so it might be worth considering if that server side task can be removed if we're going to make these items visible in the UI now.

Also, a couple of questions:

1. Will any notebook hierarchy be retained within the Misplaced folder? E.g. if you have multiple levels of nested notebooks, but the top level is notebook is missing, then the rest of the nesting is present?
2. Could the Misplaced folder be made to include notes with a missing parent, not just notebooks?

[personalizedrefrigerator]

Joplin server has a task which cleans up orphaned notebooks periodically on the server side (according to code comments it is to prevent some potential key violation).

If this refers to processOrphanedItems, my understanding is that it updates user_items for items with no user_id and doesn't check parent IDs.

In particular, it:

1. Gets all items with no corresponding user_items entry (or with a user_items entry where user_id is null):
   

   joplin/packages/server/src/models/ItemModel.ts

   Lines 965 to 968 in 58a9c22

   	const orphanedItems: Item[] = await this.db(this.tableName)
   	.select(['items.id', 'items.owner_id'])
   	.leftJoin('user_items', 'user_items.item_id', 'items.id')
   	.whereNull('user_items.user_id');

2. Loads all users that have items missing a user_items entry:
   

   joplin/packages/server/src/models/ItemModel.ts

   Line 971 in 58a9c22

   const users = await this.models().user().loadByIds(userIds, { fields: ['id'] });

3. For each orphaned item,
   • If there are no users with orphaned items, deletes the item,
     

     joplin/packages/server/src/models/ItemModel.ts

     Line 974 in 58a9c22

     if (!users.find(u => u.id)) {

     • Possible bug: users.find(u => u.id) should return the first user with a valid/truthy id. However, based on code comments, it looks like this users.find(u => u.id) was probably intended to do something else?
   • Otherwise, it adds the item to user_items:
     

     joplin/packages/server/src/models/ItemModel.ts

     Line 980 in 58a9c22

     await this.models().userItem().add(orphanedItem.owner_id, orphanedItem.id);

Will any notebook hierarchy be retained within the Misplaced folder? E.g. if you have multiple levels of nested notebooks, but the top level is notebook is missing, then the rest of the nesting is present?

Yes.

Could the Misplaced folder be made to include notes with a missing parent, not just notebooks?

Thank you for the suggestion — I've added an item to the "Tasks" section of the pull request description. I've marked it as "Optional" for now since this change may make the pull request much more complicated and these notes should also be included in "All notes".

[mrjo118]

Yes, processOrphanedItems is what I was referring to, and yes it looks like it is related to user data management, rather than orphaned notes / notebooks. It was quite a while ago I came across this, so I guess I misinterpreted it at the time

[mrjo118]

Just wondering, is there any particular issue that is preventing this coming out of draft, or is it purely because not all testing has been completed?

[personalizedrefrigerator]

Just wondering, is there any particular issue that is preventing this coming out of draft, or is it purely because not all testing has been completed?

It's purely testing related. I'm not aware of any functionality issues.

[mrjo118]

@personalizedrefrigerator I did some local testing of your code change for the remaining scenarios and it seems to be mostly ok, but does have a couple of issues.

For example, in the case where a parent folder is deleted earlier in the sync process than child folders

Steps for testing:

1. Make notebook a, and inside make notebook b and notebook c
2. Put note 1 in a, note 2 in b, and note 3 in c
3. Setup sync with file system sync
4. Delete object on file system for object a, then rename notebook a to a1 in the ui, then click sync
5. Notebook a is replaced with ... in the ui. The misplaced notes are available under ... and notebooks b and c are nested under it
6. Delete object on file system for object b, then rename notebook b to b1 in the ui, then sync
7. Notebook b disappears and its notes are available under ...

So for this test case, it does not cause any issue if notebooks within the misplaced notebook are later deleted without their children being deleted.

However, if I then do:
Delete object on file system for object c, then rename notebook c to c1 in the ui, then sync
Then the misplaced notebook disappears, and the misplaced notes are only available under the all notes notebook. Ideally the misplaced notebook should still be present if only notes are misplaced (but notebooks are not).

In terms of the other test scenarios, you can't rename the misplaced notebook or move to it on mobile, but you can create a note in the misplaced notebook. See video:

studio64_nUxJxdsY8Y.mp4

[personalizedrefrigerator]

Thank you for testing!!! I've updated the to-do checklist.

[mrjo118]

Thank you for testing!!! I've updated the to-do checklist.

FYI you'll need to merge in the latest upstream changes for my 'steps for testing', as they rely on #13167 to work correctly

[mrjo118]

• Fix misplaced notebook is hidden when there are misplaced notes, but not notebooks.

I suspect you could implement this in the Folder.all() function via a sql query (selecting a count of notes and notebooks with the parent id populated and not matching any notebook), in the same way as is done with Note.conflictedCount() in the same function, instead of determining based on misplacedItems.length. And possibly a new option needs to be added similar to the includeConflictFolder field.

joplin/packages/lib/models/Folder.ts

Line 359 in 98c5a9c

const conflictCount = await Note.conflictedCount();

[mrjo118]

Currently, (tested on desktop) if you right click and choose restore note for an individual note within a notebook which is in the trash, the note will have no notebook and only show in the all note notebook. I think it is ok retain that behaviour, but this PR will provide a place where it can be seen more easily

[laurent22]

@personalizedrefrigerator, I'm closing this PR for now. I suggest we look into it at a later time