Move note with attachment to different storage Fix for #1788

browser/main/SideNav/StorageItem.js

@@ -14,6 +14,8 @@ import i18n from 'browser/lib/i18n'
 
 const { remote } = require('electron')
 const { Menu, dialog } = remote
+const escapeStringRegexp = require('escape-string-regexp')
+const path = require('path')
 
 class StorageItem extends React.Component {
   constructor (props) {
@@ -201,7 +203,7 @@ class StorageItem extends React.Component {
       createdNoteData.forEach((newNote) => {
         dispatch({
           type: 'MOVE_NOTE',
-          originNote: noteData.find((note) => note.content === newNote.content),
+          originNote: noteData.find((note) => note.content === newNote.oldContent),
           note: newNote
         })
       })
@@ -223,7 +225,8 @@ class StorageItem extends React.Component {
     const { folderNoteMap, trashedSet } = data
     const SortableStorageItemChild = SortableElement(StorageItemChild)
     const folderList = storage.folders.map((folder, index) => {
-      const isActive = !!(location.pathname.match(new RegExp('\/storages\/' + storage.key + '\/folders\/' + folder.key)))
+      let folderRegex = new RegExp(escapeStringRegexp(path.sep) + 'storages' + escapeStringRegexp(path.sep) + storage.key + escapeStringRegexp(path.sep) + 'folders' + escapeStringRegexp(path.sep) + folder.key)
+      const isActive = !!(location.pathname.match(folderRegex))
       const noteSet = folderNoteMap.get(storage.key + '-' + folder.key)
 
       let noteCount = 0
@@ -253,7 +256,7 @@ class StorageItem extends React.Component {
       )
     })
 
-    const isActive = location.pathname.match(new RegExp('\/storages\/' + storage.key + '$'))
+    const isActive = location.pathname.match(new RegExp(escapeStringRegexp(path.sep) + 'storages' + escapeStringRegexp(path.sep) + storage.key + '$'))
 
     return (
       <div styleName={isFolded ? 'root--folded' : 'root'}

browser/main/lib/dataApi/attachmentManagement.js

@@ -3,6 +3,7 @@ const fs = require('fs')
 const path = require('path')
 const findStorage = require('browser/lib/findStorage')
 const mdurl = require('mdurl')
+const fse = require('fs-extra')
 const escapeStringRegexp = require('escape-string-regexp')
 
 const STORAGE_FOLDER_PLACEHOLDER = ':storage'
@@ -180,6 +181,28 @@ function getAbsolutePathsOfAttachmentsInContent (markdownContent, storagePath) {
   return result
 }
 
+/**
+ * @description Moves the attachments of the current note to the new location.
+ * Returns a modified version of the given content so that the links to the attachments point to the new note key.
+ * @param {String} oldPath Source of the note to be moved
+ * @param {String} newPath Destination of the note to be moved
+ * @param {String} noteKey Old note key
+ * @param {String} newNoteKey New note key
+ * @param {String} noteContent Content of the note to be moved
+ * @returns {String} Modified version of noteContent in which the paths of the attachments are fixed
+ */
+function moveAttachments (oldPath, newPath, noteKey, newNoteKey, noteContent) {
+  const src = path.join(oldPath, DESTINATION_FOLDER, noteKey)
+  const dest = path.join(newPath, DESTINATION_FOLDER, newNoteKey)
+  if (fse.existsSync(src)) {
+    fse.moveSync(src, dest)
+  }
+  if (noteContent) {
+    return noteContent.replace(new RegExp(STORAGE_FOLDER_PLACEHOLDER + escapeStringRegexp(path.sep) + noteKey, 'g'), path.join(STORAGE_FOLDER_PLACEHOLDER, newNoteKey))
+  }
+  return noteContent
+}
+
 /**
  * @description Deletes all :storage and noteKey references from the given input.
  * @param input Input in which the references should be deleted
@@ -243,6 +266,7 @@ module.exports = {
   getAbsolutePathsOfAttachmentsInContent,
   removeStorageAndNoteReferences,
   deleteAttachmentsNotPresentInNote,
+  moveAttachments,
   STORAGE_FOLDER_PLACEHOLDER,
   DESTINATION_FOLDER
 }

browser/main/lib/dataApi/copyImage.js

@@ -1,38 +0,0 @@
-const fs = require('fs')
-const path = require('path')
-const { findStorage } = require('browser/lib/findStorage')
-
-// TODO: ehhc: delete this
-
-/**
- * @description Copy an image and return the path.
- * @param {String} filePath
- * @param {String} storageKey
- * @param {Boolean} rename create new filename or leave the old one
- * @return {Promise<any>} an image path
- */
-function copyImage (filePath, storageKey, rename = true) {
-  return new Promise((resolve, reject) => {
-    try {
-      const targetStorage = findStorage(storageKey)
-
-      const inputImage = fs.createReadStream(filePath)
-      const imageExt = path.extname(filePath)
-      const imageName = rename ? Math.random().toString(36).slice(-16) : path.basename(filePath, imageExt)
-      const basename = `${imageName}${imageExt}`
-      const imageDir = path.join(targetStorage.path, 'images')
-      if (!fs.existsSync(imageDir)) fs.mkdirSync(imageDir)
-      const outputImage = fs.createWriteStream(path.join(imageDir, basename))
-      outputImage.on('error', reject)
-      inputImage.on('error', reject)
-      inputImage.on('end', () => {
-        resolve(basename)
-      })
-      inputImage.pipe(outputImage)
-    } catch (e) {
-      return reject(e)
-    }
-  })
-}
-
-module.exports = copyImage

browser/main/lib/dataApi/moveNote.js

@@ -6,6 +6,7 @@ const CSON = require('@rokt33r/season')
 const keygen = require('browser/lib/keygen')
 const sander = require('sander')
 const { findStorage } = require('browser/lib/findStorage')
+const attachmentManagement = require('./attachmentManagement')
 
 function moveNote (storageKey, noteKey, newStorageKey, newFolderKey) {
   let oldStorage, newStorage
@@ -63,36 +64,20 @@ function moveNote (storageKey, noteKey, newStorageKey, newFolderKey) {
           noteData.key = newNoteKey
           noteData.storage = newStorageKey
           noteData.updatedAt = new Date()
+          noteData.oldContent = noteData.content
 
           return noteData
         })
-        .then(function moveImages (noteData) {
-          if (oldStorage.path === newStorage.path) return noteData
-
-          const searchImagesRegex = /!\[.*\]\(:storage\/(.+)\)/gi
-          let match = searchImagesRegex.exec(noteData.content)
-
-          const moveTasks = []
-          while (match != null) {
-            const [, filename] = match
-            const oldPath = path.join(oldStorage.path, 'attachments', filename)
-            const newPath = path.join(newStorage.path, 'attachments', filename)
-            // TODO: ehhc: attachmentManagement
-            moveTasks.push(
-              sander.copyFile(oldPath).to(newPath)
-                .then(() => {
-                  fs.unlinkSync(oldPath)
-                })
-            )
-
-            // find next occurence
-            match = searchImagesRegex.exec(noteData.content)
+        .then(function moveAttachments (noteData) {
+          if (oldStorage.path === newStorage.path) {
+            return noteData
           }
 
-          return Promise.all(moveTasks).then(() => noteData)
+          noteData.content = attachmentManagement.moveAttachments(oldStorage.path, newStorage.path, noteKey, newNoteKey, noteData.content)
+          return noteData
         })
         .then(function writeAndReturn (noteData) {
-          CSON.writeFileSync(path.join(newStorage.path, 'notes', noteData.key + '.cson'), _.omit(noteData, ['key', 'storage']))
+          CSON.writeFileSync(path.join(newStorage.path, 'notes', noteData.key + '.cson'), _.omit(noteData, ['key', 'storage', 'oldContent']))
           return noteData
         })
         .then(function deleteOldNote (data) {

package.json

@@ -57,9 +57,11 @@
     "codemirror-mode-elixir": "^1.1.1",
     "electron-config": "^0.2.1",
     "electron-gh-releases": "^2.0.2",
+    "escape-string-regexp": "^1.0.5",
     "filenamify": "^2.0.0",
     "flowchart.js": "^1.6.5",
     "font-awesome": "^4.3.0",
+    "fs-extra": "^5.0.0",
     "i18n-2": "^0.7.2",
     "iconv-lite": "^0.4.19",
     "immutable": "^3.8.1",

tests/dataApi/attachmentManagement.test.js

@@ -7,6 +7,7 @@ const findStorage = require('browser/lib/findStorage')
 jest.mock('unique-slug')
 const uniqueSlug = require('unique-slug')
 const mdurl = require('mdurl')
+const fse = require('fs-extra')
 
 const systemUnderTest = require('browser/main/lib/dataApi/attachmentManagement')
 
@@ -312,3 +313,59 @@ it('should test that deleteAttachmentsNotPresentInNote does not delete reference
   }
   expect(fsUnlinkCallArguments.includes(path.join(attachmentFolderPath, dummyFilesInFolder[0]))).toBe(false)
 })
+
+it('should test that moveAttachments moves attachments only if the source folder existed', function () {
+  fse.existsSync = jest.fn(() => false)
+  fse.moveSync = jest.fn()
+
+  const oldPath = 'oldPath'
+  const newPath = 'newPath'
+  const oldNoteKey = 'oldNoteKey'
+  const newNoteKey = 'newNoteKey'
+  const content = ''
+
+  const expectedSource = path.join(oldPath, systemUnderTest.DESTINATION_FOLDER, oldNoteKey)
+
+  systemUnderTest.moveAttachments(oldPath, newPath, oldNoteKey, newNoteKey, content)
+  expect(fse.existsSync).toHaveBeenCalledWith(expectedSource)
+  expect(fse.moveSync).not.toHaveBeenCalled()
+})
+
+it('should test that moveAttachments moves attachments to the right destination', function () {
+  fse.existsSync = jest.fn(() => true)
+  fse.moveSync = jest.fn()
+
+  const oldPath = 'oldPath'
+  const newPath = 'newPath'
+  const oldNoteKey = 'oldNoteKey'
+  const newNoteKey = 'newNoteKey'
+  const content = ''
+
+  const expectedSource = path.join(oldPath, systemUnderTest.DESTINATION_FOLDER, oldNoteKey)
+  const expectedDestination = path.join(newPath, systemUnderTest.DESTINATION_FOLDER, newNoteKey)
+
+  systemUnderTest.moveAttachments(oldPath, newPath, oldNoteKey, newNoteKey, content)
+  expect(fse.existsSync).toHaveBeenCalledWith(expectedSource)
+  expect(fse.moveSync).toHaveBeenCalledWith(expectedSource, expectedDestination)
+})
+
+it('should test that moveAttachments returns a correct modified content version', function () {
+  fse.existsSync = jest.fn()
+  fse.moveSync = jest.fn()
+
+  const oldPath = 'oldPath'
+  const newPath = 'newPath'
+  const oldNoteKey = 'oldNoteKey'
+  const newNoteKey = 'newNoteKey'
+  const testInput =
+    'Test input' +
+    '![' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + oldNoteKey + path.sep + 'image.jpg](imageName}) \n' +
+    '[' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + oldNoteKey + path.sep + 'pdf.pdf](pdf})'
+  const expectedOutput =
+    'Test input' +
+    '![' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + newNoteKey + path.sep + 'image.jpg](imageName}) \n' +
+    '[' + systemUnderTest.STORAGE_FOLDER_PLACEHOLDER + path.sep + newNoteKey + path.sep + 'pdf.pdf](pdf})'
+
+  const actualContent = systemUnderTest.moveAttachments(oldPath, newPath, oldNoteKey, newNoteKey, testInput)
+  expect(actualContent).toBe(expectedOutput)
+})

yarn.lock

@@ -3637,6 +3637,14 @@ fs-extra@^1.0.0:
     jsonfile "^2.1.0"
     klaw "^1.0.0"
 
+fs-extra@^5.0.0:
+  version "5.0.0"
+  resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-5.0.0.tgz#414d0110cdd06705734d055652c5411260c31abd"
+  dependencies:
+    graceful-fs "^4.1.2"
+    jsonfile "^4.0.0"
+    universalify "^0.1.0"
+
 fs-plus@2.x:
   version "2.10.1"
   resolved "https://registry.yarnpkg.com/fs-plus/-/fs-plus-2.10.1.tgz#3204781d7840611e6364e7b6fb058c96327c5aa5"
@@ -5309,6 +5317,12 @@ jsonfile@^2.0.0, jsonfile@^2.1.0:
   optionalDependencies:
     graceful-fs "^4.1.6"
 
+jsonfile@^4.0.0:
+  version "4.0.0"
+  resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-4.0.0.tgz#8771aae0799b64076b76640fca058f9c10e33ecb"
+  optionalDependencies:
+    graceful-fs "^4.1.6"
+
 jsonify@~0.0.0:
   version "0.0.0"
   resolved "https://registry.yarnpkg.com/jsonify/-/jsonify-0.0.0.tgz#2c74b6ee41d93ca51b7b5aaee8f503631d252a73"
@@ -8734,6 +8748,10 @@ unique-temp-dir@^1.0.0:
     os-tmpdir "^1.0.1"
     uid2 "0.0.3"
 
+universalify@^0.1.0:
+  version "0.1.1"
+  resolved "https://registry.yarnpkg.com/universalify/-/universalify-0.1.1.tgz#fa71badd4437af4c148841e3b3b165f9e9e590b7"
+
 unpipe@~1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/unpipe/-/unpipe-1.0.0.tgz#b2bf4ee8514aae6165b4817829d21b2ef49904ec"