fix: Do not mark undefined remote as trashed (#2372)

When merging the trashing of a document, we add a `trashed` marker in
its metadata for the side on which it was trashed (e.g. in the
`remote` attribute if the document was trashed on the Cozy) as well as
its global metadata.

However, when a directory is trashed, we also mark all its children
and these might not have been synchronized yet and the trashed side
metadata can be missing.

e.g. given we have the synchronized directory `dir/` and a local only
file `dir/file`, when merging the trashing of `dir/` on the Cozy we:

  1. mark `dir/` as trashed and its `remote` attribute
2. try to do the same for `dir/file` but it has no `remote` attribute,
only a `local` one

We were not checking for the presence of the `remote` attribute and in
this case we would end up with an error while trying to mark
`dir/file` as we'd try to add a `trashed` attribute on `undefined`
(i.e. the missing `remote` attribute).

By checking for the presence of the `remote` attribute we make sure we
don't get this error and don't block the synchronization of the
parent directory trashing.
The child will still be removed with its parent.

core/metadata.js

@@ -517,13 +517,15 @@ function dissociateLocal(doc /*: Metadata */) {
 
 function markAsTrashed(doc /*: Metadata */, sideName /*: SideName */) {
   if (sideName === 'remote') {
-    if (doc.remote && doc.remote.type === REMOTE_DIR_TYPE) {
-      // FIXME: Remote directories have no `trashed` attribute so we know
-      // they're trashed when their path is within the remote trashbin. We
-      // should find a way to reconstruct that path or stop relying on this
-      // function altogether.
-    } else {
-      doc.remote.trashed = true
+    if (doc.remote) {
+      if (doc.remote.type === REMOTE_DIR_TYPE) {
+        // FIXME: Remote directories have no `trashed` attribute so we know
+        // they're trashed when their path is within the remote trashbin. We
+        // should find a way to reconstruct that path or stop relying on this
+        // function altogether.
+      } else {
+        doc.remote.trashed = true
+      }
     }
   } else if (doc.local) {
     doc.local.trashed = true

test/integration/trash.js

@@ -5,6 +5,7 @@ const path = require('path')
 
 const should = require('should')
 
+const pathUtils = require('../../core/utils/path')
 const TestHelpers = require('../support/helpers')
 const configHelpers = require('../support/helpers/config')
 const cozyHelpers = require('../support/helpers/cozy')
@@ -381,6 +382,35 @@ describe('Trash', () => {
           'parent/'
         ])
       })
+
+      context('with unsynced local-only content', () => {
+        beforeEach(async () => {
+          await helpers.local.syncDir.outputFile(
+            `${pathUtils.remoteToLocal(dir.attributes.path)}/local-child-file`,
+            'content'
+          )
+          await helpers.local.scan()
+
+          helpers.resetPouchSpy()
+        })
+
+        it('trashes the directory and its content on the local filesystem', async () => {
+          await cozy.files.trashById(dir._id)
+
+          await helpers.remote.pullChanges()
+          should(helpers.putDocs('path', 'trashed')).deepEqual([
+            // Recursively trash parent/dir; children are trashed first
+            { path: path.normalize('parent/dir/subdir/file'), trashed: true },
+            { path: path.normalize('parent/dir/subdir'), trashed: true },
+            {
+              path: path.normalize('parent/dir/local-child-file'),
+              trashed: true
+            },
+            { path: path.normalize('parent/dir/empty-subdir'), trashed: true },
+            { path: path.normalize('parent/dir'), trashed: true }
+          ])
+        })
+      })
     })
   })
 })

test/unit/metadata.js

@@ -17,6 +17,7 @@ const {
   invalidChecksum,
   invalidPath,
   markSide,
+  markAsTrashed,
   markAsUpToDate,
   assignPlatformIncompatibilities,
   detectIncompatibilities,
@@ -31,6 +32,7 @@ const {
   createConflictingDoc
 } = metadata
 const { DIR_TYPE, NOTE_MIME_TYPE } = require('../../core/remote/constants')
+const { otherSide } = require('../../core/side')
 const pathUtils = require('../../core/utils/path')
 const timestamp = require('../../core/utils/timestamp')
 const Builders = require('../support/builders')
@@ -39,7 +41,7 @@ const { onPlatform, onPlatforms } = require('../support/helpers/platform')
 const pouchHelpers = require('../support/helpers/pouch')
 
 /*::
-import type { Metadata, MetadataRemoteFile, MetadataRemoteDir, MetadataLocalInfo } from '../../core/metadata'
+import type { Metadata, MetadataRemoteFile, MetadataRemoteDir, MetadataLocalInfo, MetadataRemoteInfo } from '../../core/metadata'
 import type { RemoteBase } from '../../core/remote/document'
 */
 
@@ -1239,6 +1241,69 @@ describe('metadata', function() {
     })
   })
 
+  describe('markAsTrashed', () => {
+    let doc
+
+    for (const sideName of ['local', 'remote']) {
+      context(`on the ${sideName} side`, () => {
+        context('with a synchronized file', () => {
+          beforeEach(() => {
+            doc = builders
+              .metafile()
+              .upToDate()
+              .build()
+
+            markAsTrashed(doc, sideName)
+          })
+
+          it('marks the document as trashed', () => {
+            should(doc.trashed).be.true()
+          })
+
+          it(`marks the ${sideName} side as trashed`, () => {
+            // $FlowFixMe we know both sides are present
+            should(doc[sideName].trashed).be.true()
+          })
+        })
+
+        context(`with a ${sideName}-only file`, () => {
+          beforeEach(() => {
+            doc = builders
+              .metafile()
+              .unmerged(sideName) // XXX: we use unmerged to have only one side but markAsTrashed will usually be called on merged documents
+              .build()
+
+            markAsTrashed(doc, sideName)
+          })
+
+          it('marks the document as trashed', () => {
+            should(doc.trashed).be.true()
+          })
+
+          it(`marks the ${sideName} side as trashed`, () => {
+            // $FlowFixMe we know both sides are present
+            should(doc[sideName].trashed).be.true()
+          })
+        })
+
+        context(`with a ${otherSide(sideName)}-only file`, () => {
+          beforeEach(() => {
+            doc = builders
+              .metafile()
+              .unmerged(otherSide(sideName)) // XXX: we use unmerged to have only one side but markAsTrashed will usually be called on merged documents
+              .build()
+
+            markAsTrashed(doc, sideName)
+          })
+
+          it('marks the document as trashed', () => {
+            should(doc.trashed).be.true()
+          })
+        })
+      })
+    }
+  })
+
   describe('outOfDateSide', () => {
     it('returns nothing if sides are not set', () => {
       const doc1 = builders