From 17c00c0e5412c6b16da9d8340344fa6fe541cc22 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Mon, 3 Aug 2015 11:31:09 -0700 Subject: [PATCH] fix(change-category): Redefine objectIds when objects cannot be found Fixes https://sentry.nylas.com/sentry/edgehill/group/1717/ --- .../tasks/change-folder-task-spec.coffee | 61 +++++++++++++------ src/flux/tasks/change-category-task.coffee | 8 +++ 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/spec-nylas/tasks/change-folder-task-spec.coffee b/spec-nylas/tasks/change-folder-task-spec.coffee index 2e7bc59e3..950e46c1e 100644 --- a/spec-nylas/tasks/change-folder-task-spec.coffee +++ b/spec-nylas/tasks/change-folder-task-spec.coffee @@ -16,9 +16,7 @@ testMessages = {} describe "ChangeFolderTask", -> beforeEach -> - spyOn(DatabaseStore, 'persistModel').andCallFake -> Promise.resolve() - spyOn(DatabaseStore, 'persistModels').andCallFake -> Promise.resolve() - spyOn(DatabaseStore, 'find').andCallFake (klass, id) => + @_findFunction = (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 -> - expect(NylasAPI.incrementOptimisticChangeCount) - .toHaveBeenCalledWith(Thread, 't1') + waitsForPromise => + @basicThreadTask.performLocal().then -> + 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 -> - expect(NylasAPI.decrementOptimisticChangeCount) - .toHaveBeenCalledWith(Thread, 't1') + waitsForPromise => + @basicThreadTask.performLocal(reverting: true).then -> + 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 => - thread = DatabaseStore.persistModel.calls[0].args[0] - expect(thread.folders).toEqual [@testFolders['f1']] + waitsForPromise => + @basicThreadTask.performLocal().then => + 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 -> - messages = DatabaseStore.persistModels.calls[0].args[0] - # We expect 2 because 1 of our 3 messages already has the folder - # we want. - expect(messages.length).toBe 2 - for message in messages - expect(message.folder).toEqual expectedFolder + waitsForPromise => + @basicThreadTask.performLocal().then -> + messages = DatabaseStore.persistModels.calls[0].args[0] + # We expect 2 because 1 of our 3 messages already has the folder + # we want. + expect(messages.length).toBe 2 + for message in messages + expect(message.folder).toEqual expectedFolder ## MORE TESTS COMING SOON diff --git a/src/flux/tasks/change-category-task.coffee b/src/flux/tasks/change-category-task.coffee index 785019bcf..24ccd4c39 100644 --- a/src/flux/tasks/change-category-task.coffee +++ b/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