From 2f4eb595f61284aaa8c7c420359dd880df094e8a Mon Sep 17 00:00:00 2001 From: ehhc Date: Wed, 23 May 2018 19:47:41 +0200 Subject: [PATCH 1/4] OnBlur throws exceptions if the notetype is snippet -> Fixes #1962 --- .../main/lib/dataApi/attachmentManagement.js | 60 ++++++++++--------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/browser/main/lib/dataApi/attachmentManagement.js b/browser/main/lib/dataApi/attachmentManagement.js index 1373cf94d..c83fc79da 100644 --- a/browser/main/lib/dataApi/attachmentManagement.js +++ b/browser/main/lib/dataApi/attachmentManagement.js @@ -232,39 +232,41 @@ function deleteAttachmentFolder (storageKey, noteKey) { * @param noteKey NoteKey of the current note. Is used to determine the belonging attachment folder. */ function deleteAttachmentsNotPresentInNote (markdownContent, storageKey, noteKey) { - const targetStorage = findStorage.findStorage(storageKey) - const attachmentFolder = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) - const attachmentsInNote = getAttachmentsInContent(markdownContent) - const attachmentsInNoteOnlyFileNames = [] - if (attachmentsInNote) { - for (let i = 0; i < attachmentsInNote.length; i++) { - attachmentsInNoteOnlyFileNames.push(attachmentsInNote[i].replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey + escapeStringRegexp(path.sep), 'g'), '')) + if (storageKey && noteKey && markdownContent !== null && typeof markdownContent !== 'undefined') { + const targetStorage = findStorage.findStorage(storageKey) + const attachmentFolder = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) + const attachmentsInNote = getAttachmentsInContent(markdownContent) + const attachmentsInNoteOnlyFileNames = [] + if (attachmentsInNote) { + for (let i = 0; i < attachmentsInNote.length; i++) { + attachmentsInNoteOnlyFileNames.push(attachmentsInNote[i].replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey + escapeStringRegexp(path.sep), 'g'), '')) + } } - } - if (fs.existsSync(attachmentFolder)) { - fs.readdir(attachmentFolder, (err, files) => { - if (err) { - console.error("Error reading directory '" + attachmentFolder + "'. Error:") - console.error(err) - return - } - files.forEach(file => { - if (!attachmentsInNoteOnlyFileNames.includes(file)) { - const absolutePathOfFile = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey, file) - fs.unlink(absolutePathOfFile, (err) => { - if (err) { - console.error("Could not delete '%s'", absolutePathOfFile) - console.error(err) - return - } - console.info("File '" + absolutePathOfFile + "' deleted because it was not included in the content of the note") - }) + if (fs.existsSync(attachmentFolder)) { + fs.readdir(attachmentFolder, (err, files) => { + if (err) { + console.error("Error reading directory '" + attachmentFolder + "'. Error:") + console.error(err) + return } + files.forEach(file => { + if (!attachmentsInNoteOnlyFileNames.includes(file)) { + const absolutePathOfFile = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey, file) + fs.unlink(absolutePathOfFile, (err) => { + if (err) { + console.error("Could not delete '%s'", absolutePathOfFile) + console.error(err) + return + } + console.info("File '" + absolutePathOfFile + "' deleted because it was not included in the content of the note") + }) + } + }) }) - }) - } else { - console.info("Attachment folder ('" + attachmentFolder + "') did not exist..") + } else { + console.info("Attachment folder ('" + attachmentFolder + "') did not exist..") + } } } From 8132dd68471c40b946db0371703064b71dce26ed Mon Sep 17 00:00:00 2001 From: ehhc Date: Mon, 28 May 2018 08:58:09 +0200 Subject: [PATCH 2/4] Fix for the issues raised in the code review --- .../main/lib/dataApi/attachmentManagement.js | 2 +- tests/dataApi/attachmentManagement.test.js | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/browser/main/lib/dataApi/attachmentManagement.js b/browser/main/lib/dataApi/attachmentManagement.js index c83fc79da..096735414 100644 --- a/browser/main/lib/dataApi/attachmentManagement.js +++ b/browser/main/lib/dataApi/attachmentManagement.js @@ -232,7 +232,7 @@ function deleteAttachmentFolder (storageKey, noteKey) { * @param noteKey NoteKey of the current note. Is used to determine the belonging attachment folder. */ function deleteAttachmentsNotPresentInNote (markdownContent, storageKey, noteKey) { - if (storageKey && noteKey && markdownContent !== null && typeof markdownContent !== 'undefined') { + if (storageKey != null && noteKey != null && markdownContent != null) { const targetStorage = findStorage.findStorage(storageKey) const attachmentFolder = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) const attachmentsInNote = getAttachmentsInContent(markdownContent) diff --git a/tests/dataApi/attachmentManagement.test.js b/tests/dataApi/attachmentManagement.test.js index feb9207cb..1418bcfad 100644 --- a/tests/dataApi/attachmentManagement.test.js +++ b/tests/dataApi/attachmentManagement.test.js @@ -383,3 +383,35 @@ it('should test that moveAttachments returns a correct modified content version' const actualContent = systemUnderTest.moveAttachments(oldPath, newPath, oldNoteKey, newNoteKey, testInput) expect(actualContent).toBe(expectedOutput) }) + +it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was null', function () { + const noteKey = null + const storageKey = null + const markdownContent = '' + + findStorage.findStorage = jest.fn() + fs.existsSync = jest.fn() + fs.readdir = jest.fn() + fs.unlink = jest.fn() + + systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) + expect(fs.existsSync).not.toHaveBeenCalled() + expect(fs.readdir).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() +}) + +it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was undefined', function () { + const noteKey = undefined + const storageKey = undefined + const markdownContent = '' + + findStorage.findStorage = jest.fn() + fs.existsSync = jest.fn() + fs.readdir = jest.fn() + fs.unlink = jest.fn() + + systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) + expect(fs.existsSync).not.toHaveBeenCalled() + expect(fs.readdir).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() +}) From 225916fbba461b1ce5ad50baa52c25d3ee977fb9 Mon Sep 17 00:00:00 2001 From: ehhc Date: Mon, 28 May 2018 09:57:07 +0200 Subject: [PATCH 3/4] Fix for the test broken by the merge commit --- tests/dataApi/attachmentManagement.test.js | 66 +++++++++++----------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/tests/dataApi/attachmentManagement.test.js b/tests/dataApi/attachmentManagement.test.js index 0282e1716..439a6749a 100644 --- a/tests/dataApi/attachmentManagement.test.js +++ b/tests/dataApi/attachmentManagement.test.js @@ -339,6 +339,38 @@ it('should test that deleteAttachmentsNotPresentInNote does not delete reference expect(fsUnlinkCallArguments.includes(path.join(attachmentFolderPath, dummyFilesInFolder[0]))).toBe(false) }) +it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was null', function () { + const noteKey = null + const storageKey = null + const markdownContent = '' + + findStorage.findStorage = jest.fn() + fs.existsSync = jest.fn() + fs.readdir = jest.fn() + fs.unlink = jest.fn() + + systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) + expect(fs.existsSync).not.toHaveBeenCalled() + expect(fs.readdir).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() +}) + +it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was undefined', function () { + const noteKey = undefined + const storageKey = undefined + const markdownContent = '' + + findStorage.findStorage = jest.fn() + fs.existsSync = jest.fn() + fs.readdir = jest.fn() + fs.unlink = jest.fn() + + systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) + expect(fs.existsSync).not.toHaveBeenCalled() + expect(fs.readdir).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() +}) + it('should test that moveAttachments moves attachments only if the source folder existed', function () { fse.existsSync = jest.fn(() => false) fse.moveSync = jest.fn() @@ -395,38 +427,6 @@ it('should test that moveAttachments returns a correct modified content version' expect(actualContent).toBe(expectedOutput) }) - -it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was null', function () { - const noteKey = null - const storageKey = null - const markdownContent = '' - - findStorage.findStorage = jest.fn() - fs.existsSync = jest.fn() - fs.readdir = jest.fn() - fs.unlink = jest.fn() - - systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) - expect(fs.existsSync).not.toHaveBeenCalled() - expect(fs.readdir).not.toHaveBeenCalled() - expect(fs.unlink).not.toHaveBeenCalled() -}) - -it('should test that deleteAttachmentsNotPresentInNote does nothing if noteKey, storageKey or noteContent was undefined', function () { - const noteKey = undefined - const storageKey = undefined - const markdownContent = '' - - findStorage.findStorage = jest.fn() - fs.existsSync = jest.fn() - fs.readdir = jest.fn() - fs.unlink = jest.fn() - - systemUnderTest.deleteAttachmentsNotPresentInNote(markdownContent, storageKey, noteKey) - expect(fs.existsSync).not.toHaveBeenCalled() - expect(fs.readdir).not.toHaveBeenCalled() - expect(fs.unlink).not.toHaveBeenCalled() - it('should test that cloneAttachments modifies the content of the new note correctly', function () { const oldNote = {key: 'oldNoteKey', content: 'oldNoteContent', storage: 'storageKey', type: 'MARKDOWN_NOTE'} const newNote = {key: 'newNoteKey', content: 'oldNoteContent', storage: 'storageKey', type: 'MARKDOWN_NOTE'} @@ -435,6 +435,8 @@ it('should test that cloneAttachments modifies the content of the new note corre '![' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + oldNote.key + path.sep + 'image.jpg](imageName}) \n' + '[' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + oldNote.key + path.sep + 'pdf.pdf](pdf})' newNote.content = testInput + findStorage.findStorage = jest.fn() + findStorage.findStorage.mockReturnValue({path: 'dummyStoragePath'}) const expectedOutput = 'Test input' + From b526d489460e22cba9f5db6105068b243b30531f Mon Sep 17 00:00:00 2001 From: ehhc Date: Tue, 5 Jun 2018 11:16:50 +0200 Subject: [PATCH 4/4] CodeReview --- .../main/lib/dataApi/attachmentManagement.js | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/browser/main/lib/dataApi/attachmentManagement.js b/browser/main/lib/dataApi/attachmentManagement.js index a93fc8a50..077f02d8b 100644 --- a/browser/main/lib/dataApi/attachmentManagement.js +++ b/browser/main/lib/dataApi/attachmentManagement.js @@ -245,41 +245,41 @@ function deleteAttachmentFolder (storageKey, noteKey) { * @param noteKey NoteKey of the current note. Is used to determine the belonging attachment folder. */ function deleteAttachmentsNotPresentInNote (markdownContent, storageKey, noteKey) { - if (storageKey != null && noteKey != null && markdownContent != null) { - const targetStorage = findStorage.findStorage(storageKey) - const attachmentFolder = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) - const attachmentsInNote = getAttachmentsInContent(markdownContent) - const attachmentsInNoteOnlyFileNames = [] - if (attachmentsInNote) { - for (let i = 0; i < attachmentsInNote.length; i++) { - attachmentsInNoteOnlyFileNames.push(attachmentsInNote[i].replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey + escapeStringRegexp(path.sep), 'g'), '')) - } + if (storageKey == null || noteKey == null || markdownContent == null) { + return + } + const targetStorage = findStorage.findStorage(storageKey) + const attachmentFolder = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey) + const attachmentsInNote = getAttachmentsInContent(markdownContent) + const attachmentsInNoteOnlyFileNames = [] + if (attachmentsInNote) { + for (let i = 0; i < attachmentsInNote.length; i++) { + attachmentsInNoteOnlyFileNames.push(attachmentsInNote[i].replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey + escapeStringRegexp(path.sep), 'g'), '')) } - - if (fs.existsSync(attachmentFolder)) { - fs.readdir(attachmentFolder, (err, files) => { - if (err) { - console.error("Error reading directory '" + attachmentFolder + "'. Error:") - console.error(err) - return + } + if (fs.existsSync(attachmentFolder)) { + fs.readdir(attachmentFolder, (err, files) => { + if (err) { + console.error('Error reading directory \'' + attachmentFolder + '\'. Error:') + console.error(err) + return + } + files.forEach(file => { + if (!attachmentsInNoteOnlyFileNames.includes(file)) { + const absolutePathOfFile = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey, file) + fs.unlink(absolutePathOfFile, (err) => { + if (err) { + console.error('Could not delete \'%s\'', absolutePathOfFile) + console.error(err) + return + } + console.info('File \'' + absolutePathOfFile + '\' deleted because it was not included in the content of the note') + }) } - files.forEach(file => { - if (!attachmentsInNoteOnlyFileNames.includes(file)) { - const absolutePathOfFile = path.join(targetStorage.path, DESTINATION_FOLDER, noteKey, file) - fs.unlink(absolutePathOfFile, (err) => { - if (err) { - console.error("Could not delete '%s'", absolutePathOfFile) - console.error(err) - return - } - console.info("File '" + absolutePathOfFile + "' deleted because it was not included in the content of the note") - }) - } - }) }) - } else { - console.info("Attachment folder ('" + attachmentFolder + "') did not exist..") - } + }) + } else { + console.debug('Attachment folder (\'' + attachmentFolder + '\') did not exist..') } }