Mobile: Fixes #10065 filtered deleted and trash folder when counting active notebooks #10087
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hello, this is my first time contributing to Joplin and I have tried my best to follow the given guidelines as much as possible but incase if I missed anything do let me know! Also, any tips to improve my pr would be appreciated! |
|
@PackElend label me please |
| if (!this.props.folders.length) { | ||
| // calculate number of folders except deleted and trash folder | ||
| // trash folder id: de1e7ede1e7ede1e7ede1e7ede1e7ede | ||
| const atleastOneNotebookExists = this.props.folders.filter((folder: FolderEntity) => folder.id !== 'de1e7ede1e7ede1e7ede1e7ede1e7ede' && folder.deleted_time === 0).length > 0; |
There was a problem hiding this comment.
Consider using getTrashFolderId here instead of de1e7ede1e7ede1e7ede1e7ede1e7ede. (getTrashFolderId is defined here).
Thank you for working on this!
There was a problem hiding this comment.
Hey, I have made the suggested changes and updated the pr. Do let me know if any other changes are needed!
| if (!this.props.folders.length) { | ||
| // calculate number of folders except deleted and trash folder | ||
| // trash folder id: de1e7ede1e7ede1e7ede1e7ede1e7ede | ||
| const atleastOneNotebookExists = this.props.folders.filter((folder: FolderEntity) => folder.id !== 'de1e7ede1e7ede1e7ede1e7ede1e7ede' && folder.deleted_time === 0).length > 0; |
There was a problem hiding this comment.
Will this not cause the same error as #10073 ?
There was a problem hiding this comment.
Hey, it is not causing that error as of now, I don't get how the changes I have made can possibly cause that error. Can you please elaborate on it?
There was a problem hiding this comment.
I'm not saying your changes caused that issue, of course. I mean if you have a conflicts folder, won't your code have the same errors as the issue I linked.
There was a problem hiding this comment.
Oh, got it. Then I'll add another condition to check whether the folder id matches with the conflict folder id before checking for the deleted_time attribute. This should resolve it I guess, correct me if I'm wrong.
I can use the conflictFolderId() function to get its id.
OR
maybe we can add deleted_time attribute to the conflicts folder object, that would fix #10073 as well.
before:
public static conflictFolder(): FolderEntity {
const now = Date.now();
return {
type_: this.TYPE_FOLDER,
id: this.conflictFolderId(),
parent_id: '',
title: this.conflictFolderTitle(),
updated_time: now,
user_updated_time: now,
share_id: '',
is_shared: 0,
};
}
after:
public static conflictFolder(): FolderEntity {
const now = Date.now();
return {
type_: this.TYPE_FOLDER,
id: this.conflictFolderId(),
parent_id: '',
title: this.conflictFolderTitle(),
updated_time: now,
user_updated_time: now,
share_id: '',
is_shared: 0,
+ deleted_time: 0,
};
}
| const trashFolderId = getTrashFolderId(); | ||
|
|
||
| // calculate number of folders except deleted and trash folder | ||
| const atleastOneNotebookExists = this.props.folders.filter((folder: FolderEntity) => folder.id !== trashFolderId && folder.deleted_time === 0).length > 0; |
There was a problem hiding this comment.
Please use the term "folder" (not "notebook") in code
| const trashFolderId = getTrashFolderId(); | ||
|
|
||
| // calculate number of folders except deleted and trash folder | ||
| const atleastOneNotebookExists = this.props.folders.filter((folder: FolderEntity) => folder.id !== trashFolderId && folder.deleted_time === 0).length > 0; |
There was a problem hiding this comment.
Since you use the same code in two places please make it a function that takes folders as input and returns a boolean. You can put it in models/Folder.ts
Also name it something like atLeastOneRealFolderExists since the trash folder is a virtual one
There was a problem hiding this comment.
I made the changes you suggested and have added unit test for the same.
packages/lib/models/Folder.test.ts
Outdated
| @@ -353,4 +353,17 @@ describe('models/Folder', () => { | |||
| await expectThrow(async () => Folder.delete(folder2.id, { toTrash: true, toTrashParentId: folder2.id })); | |||
| }); | |||
|
|
|||
| it('should tell if atleast one folder other than trash and deleted exists', async () => { | |||
There was a problem hiding this comment.
Should be "at least" (not sure why "atleast" was in our dictionary but I removed it now)
| await Folder.delete(f1.id, { toTrash: true }); | ||
| folders = await Folder.all({ includeTrash: true }); | ||
| expect(Folder.atLeastOneRealFolderExists(folders)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Thank you for adding the tests. Also please add a case for testing an empty array
There was a problem hiding this comment.
I have added the test case for testing an empty array, please check.
|
Looks good now, thanks @Sidd-R! |
Summary
Filtered the array of folders for trash and deleted folders before taking its length. Earlier both were being considered which led to trash and deleted folders to be considered as active notebooks and the ui of an active notebook was being displayed. When there was an attempt to create a new note/todo list, the Folder.defaultFolder() returned null in screens/Notes.tsx and when its id was accessed, it gave cannot read property error.
Rationale
Filtered the folders using the following logic:
Now when there are no active notebooks, create new notebook option is shown and the floating button for creating new note/todo is hidden.
Before:
After:
Tested on Android 13
Link to my introduction in forum: - https://discourse.joplinapp.org/t/introducing-sidd-r/36498