Desktop: Batch delete for Notebooks added #2730
Conversation
|
@tessus, is this okay? |
|
Yes, looks good. |
tessus
changed the title
|
@laurent22, can this be merged? |
|
@RedDocMD please do not send mentions for merge attention, your PR will not be lost, it will be processed in due course. |
|
Are there unit tests for Have you reviewed the unit tests for |
|
@mic704b , there doesn't seem to be a unit test for |
|
@mic704b, I have added the tests I had mentioned above |
|
@PackElend, can you please add the GSoC tag for this PR? |
|
Code looks good. @mic704b feel free to merge if the tests are good to go. |
|
@mic704b, I have made the changes as you had instructed |
CliClient/tests/models_Note.js
Outdated
| let noOfNotes = 20; | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note1${i}`, parent_id: f1.id }); | ||
| } | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note2${i}`, parent_id: f2.id }); | ||
| } |
There was a problem hiding this comment.
Instead use createNTestNotes from test-utils.js (also applies to next test case)
CliClient/tests/models_Note.js
Outdated
| let noOfNotes = 20; | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note1${i}`, parent_id: f1.id }); | ||
| } | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note2${i}`, parent_id: f2.id }); | ||
| } | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note3${i}`, parent_id: f3.id }); | ||
| } | ||
| for (let i = 0; i < noOfNotes; i++) { | ||
| await Note.save({ title: `note4${i}`, parent_id: f4.id }); | ||
| } |
There was a problem hiding this comment.
Please use createNTestNotes for these
CliClient/tests/models_Note.js
Outdated
| expect(beforeDelete.length).toBe(afterDelete.length); | ||
| let count = 0; | ||
| for (let i = 0; i < beforeDelete.length; i++) { | ||
| if (beforeDelete[i].id == afterDelete[i].id) ++count; | ||
| } | ||
| expect(beforeDelete.length).toBe(count); |
There was a problem hiding this comment.
Good
but you can reduce all of this to:
expect(sortedIds(afterDelete)).toEqual(sortedIds(beforeDelete));
CliClient/tests/models_Note.js
Outdated
| let notesInFolder1IDs = await Folder.noteIds(f1.id); | ||
| let notesInFolder2IDs = await Folder.noteIds(f2.id); | ||
|
|
||
| const selectRandomly = (source, count) => { |
There was a problem hiding this comment.
Nice. However lets not have a random element in the tests. Please remove and simplify this case a bit. If you want to cover more combinations, make extra test cases explicitly. I'd be happy with just one though, to start with.
Nice work though, you seem to know your javascript!
|
@mic704b, I have made the changes you said and resolved the conflict |
|
Thanks @RedDocMD changes look good. You've just got a few linter problems to fix. |
|
@mic704b, I think I have fixed the linter issues |
|
Thanks @RedDocMD, all good. |
Fixes #2703