Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop: Fixes #10668: Tags and Delete note not being available on Search and on All Notes list #10729

Merged
merged 14 commits into from
Aug 2, 2024

Conversation

pedr
Copy link
Collaborator

@pedr pedr commented Jul 9, 2024

Fixes #10668

Description

The issue existed because the state selectedFolderId doesn't change when we select All Notes or do a Search. Since we use the selectedFolderId to check if it is identical to the static trash id to identify if we are in the "Trash context", switching from Trash to all notes was like we were still in the same context.

Testing

Test case 1

  1. Go to Trash
  2. Go to All notes
  3. Right-click on a note in Note list : Delete note and Tags action should be available and not greyed out

Test case 2

  1. Go to Trash
  2. Go to All notes
  3. Make a Search for some note that exists
  4. Right-click on a note in Note list : Delete note and Tags actions should be available and not greyed out

@pedr pedr requested a review from laurent22 July 9, 2024 22:54
@pedr
Copy link
Collaborator Author

pedr commented Jul 18, 2024

  • Write it down why the inTrash breaks when we set isSmartFilter is used

I changed how the inTrash enabled condition is defined, but now when we are in the All notes the option to permanently delete note is available in the menu

image

My solution was to separate the logic of itemIsReadOnlySync by moving the "note is read only because is shared" into another function.

That means that I had to update all enabled conditions that used noteIsReadOnly and folderIsReadOnly to also check if the selected note/folder is not on trash. This is more verbose, but it is the only way I found not to enable Permanently delete note for all notes.

Of course, for the Permanenly delete note the logic is different, we are checking if the note is not read-only AND is on Trash.

@pedr pedr marked this pull request as ready for review July 24, 2024 19:35
@pedr
Copy link
Collaborator Author

pedr commented Jul 25, 2024

  • Test again if you can move folder out of trash
  • Revert the change to make permanently delete note not be able to delete any notes, and add logic inside the command.

@@ -68,7 +70,7 @@ export default function stateToWhenClauseContext(state: State, options: WhenClau

// Current location
inConflictFolder: state.selectedFolderId === Folder.conflictFolderId(),
inTrash: state.selectedFolderId === getTrashFolderId() || commandFolder && !!commandFolder.deleted_time,
inTrash: (state.selectedFolderId === getTrashFolderId() || commandFolder && !!commandFolder.deleted_time) && hasDeletedTime,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a check on deleted_time here. Couldn't you just add a check on the selected note?

Copy link
Collaborator Author

@pedr pedr Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, I just kept the check for the selectedNote object in this case and included it in the original inTrash boolean logic.

@pedr
Copy link
Collaborator Author

pedr commented Aug 1, 2024

  • Add test for inTrash definition

@pedr
Copy link
Collaborator Author

pedr commented Aug 2, 2024

@laurent22 I added basic tests like suggested before

expect(result.inTrash).toBe(false);
});

});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use plain English for the titles. Don't use variable names or at least try to avoid this - this is meant to be a high level description of the test. It should show your intention which hopefully can be expressed in English. For example "deleted_time" is "0" means note is deleted, "selectedFolderId is not trash" means the selected folder is not the trash, etc.

@laurent22
Copy link
Owner

Looks good now, thanks Pedro!

@laurent22 laurent22 merged commit 5c8be44 into laurent22:dev Aug 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete note action is greyed out in All notes after going through the Trash
2 participants