fix(change-category): Redefine objectIds when objects cannot be found

Fixes https://sentry.nylas.com/sentry/edgehill/group/1717/

spec-nylas/tasks/change-folder-task-spec.coffee

@@ -16,9 +16,7 @@ testMessages = {}
 
 describe "ChangeFolderTask", ->
   beforeEach ->
-    spyOn(DatabaseStore, 'persistModel').andCallFake -> Promise.resolve()
+    @_findFunction = (klass, id) =>
-    spyOn(DatabaseStore, 'persistModels').andCallFake -> Promise.resolve()
-    spyOn(DatabaseStore, 'find').andCallFake (klass, id) =>
       if klass is Thread
         Promise.resolve(testThreads[id])
       else if klass is Message
@@ -28,6 +26,11 @@ describe "ChangeFolderTask", ->
       else
         throw new Error("Not stubbed!")
 
+    spyOn(DatabaseStore, 'persistModel').andCallFake -> Promise.resolve()
+    spyOn(DatabaseStore, 'persistModels').andCallFake -> Promise.resolve()
+    spyOn(DatabaseStore, 'find').andCallFake (klass, id) =>
+      @_findFunction(klass, id)
+
     spyOn(DatabaseStore, 'findAll').andCallFake (klass, finder) =>
       if klass is Message
         Promise.resolve(_.values(testMessages))
@@ -115,7 +118,8 @@ describe "ChangeFolderTask", ->
       waitsForPromise ->
         t.performLocal().catch (error) ->
           expect(error.message).toBe "Must pass an `undoData` to rollback folder changes"
-    it "throws an error if an undo task isn't passed undo data", ->
+
+    it "throws an error if an undo task is passed an empty hash of undo data", ->
       t = new ChangeFolderTask
         folderOrId: 'f1'
         undoData: {}
@@ -138,16 +142,31 @@ describe "ChangeFolderTask", ->
     it 'increments optimistic changes', ->
       spyOn(@basicThreadTask, "localUpdateThread").andReturn Promise.resolve()
       spyOn(NylasAPI, "incrementOptimisticChangeCount")
-      @basicThreadTask.performLocal().then ->
+      waitsForPromise =>
-        expect(NylasAPI.incrementOptimisticChangeCount)
+        @basicThreadTask.performLocal().then ->
-          .toHaveBeenCalledWith(Thread, 't1')
+          expect(NylasAPI.incrementOptimisticChangeCount)
+            .toHaveBeenCalledWith(Thread, 't1')
+
+    it 'removes the objectId from the set if the object cannot be found', ->
+      spyOn(@basicThreadTask, "localUpdateThread").andReturn Promise.resolve()
+      spyOn(NylasAPI, "incrementOptimisticChangeCount")
+      @_findFunction = (klass, id) =>
+        if klass is Thread
+          Promise.resolve(null)
+
+      expect(@basicThreadTask.objectIds).toEqual(['t1'])
+      waitsForPromise =>
+        @basicThreadTask.performLocal().then =>
+          expect(NylasAPI.incrementOptimisticChangeCount).not.toHaveBeenCalled()
+          expect(@basicThreadTask.objectIds).toEqual([])
 
     it 'decrements optimistic changes if reverting', ->
       spyOn(@basicThreadTask, "localUpdateThread").andReturn Promise.resolve()
       spyOn(NylasAPI, "decrementOptimisticChangeCount")
-      @basicThreadTask.performLocal(reverting: true).then ->
+      waitsForPromise =>
-        expect(NylasAPI.decrementOptimisticChangeCount)
+        @basicThreadTask.performLocal(reverting: true).then ->
-          .toHaveBeenCalledWith(Thread, 't1')
+          expect(NylasAPI.decrementOptimisticChangeCount)
+            .toHaveBeenCalledWith(Thread, 't1')
 
     describe "When it's a Regular Task", ->
       it 'sets undo data and ignores messages that already have the folder we want', ->
@@ -161,22 +180,24 @@ describe "ChangeFolderTask", ->
           expect(expectedData).toEqual @basicThreadTask.undoData
 
     it 'updates a thread with the new folder', ->
-      @basicThreadTask.performLocal().then =>
+      waitsForPromise =>
-        thread = DatabaseStore.persistModel.calls[0].args[0]
+        @basicThreadTask.performLocal().then =>
-        expect(thread.folders).toEqual [@testFolders['f1']]
+          thread = DatabaseStore.persistModel.calls[0].args[0]
+          expect(thread.folders).toEqual [@testFolders['f1']]
 
     it "updates a thread's messages with the new folder and ignores messages that already have the same folder", ->
       # Our stub of DatabaseStore.findAll ignores the scoping parameter.
       # We simply return all messages.
 
       expectedFolder = @testFolders['f1']
-      @basicThreadTask.performLocal().then ->
+      waitsForPromise =>
-        messages = DatabaseStore.persistModels.calls[0].args[0]
+        @basicThreadTask.performLocal().then ->
-        # We expect 2 because 1 of our 3 messages already has the folder
+          messages = DatabaseStore.persistModels.calls[0].args[0]
-        # we want.
+          # We expect 2 because 1 of our 3 messages already has the folder
-        expect(messages.length).toBe 2
+          # we want.
-        for message in messages
+          expect(messages.length).toBe 2
-          expect(message.folder).toEqual expectedFolder
+          for message in messages
+            expect(message.folder).toEqual expectedFolder
 
     ## MORE TESTS COMING SOON
 

src/flux/tasks/change-category-task.coffee

@@ -47,6 +47,14 @@ class ChangeCategoryTask extends Task
     @collectCategories().then (categories) =>
       promises = @objectIds.map (objectId) =>
         DatabaseStore.find(@_klass(), objectId).then (object) =>
+          # If we weren't able to find this object, remove it from the objectIds
+          # and carry on. This can happen pretty easily if you undo an action
+          # and other things have happened.
+          if not object
+            idx = @objectIds.indexOf(objectId)
+            @objectIds.splice(idx, 1) unless idx is -1
+            return Promise.resolve()
+
           # Mark that we are optimistically changing this model. This will prevent
           # inbound delta syncs from changing it back to it's old state. Only the
           # operation that changes `optimisticChangeCount` back to zero will