From d19533ff7fc21283fcf9f2ed7a12d322b65eb4f1 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Fri, 4 Dec 2015 16:29:26 -0800 Subject: [PATCH] fix(counts): compute deltas for unpersist events, more specs This fix should resolve #489 --- spec/stores/database-store-spec.coffee | 24 ++++-- spec/stores/thread-counts-store-spec.coffee | 88 +++++++++++++-------- src/flux/stores/database-store.coffee | 38 +++++---- src/flux/stores/thread-counts-store.coffee | 28 ++++--- 4 files changed, 110 insertions(+), 68 deletions(-) diff --git a/spec/stores/database-store-spec.coffee b/spec/stores/database-store-spec.coffee index 0a091c152..88f1700fb 100644 --- a/spec/stores/database-store-spec.coffee +++ b/spec/stores/database-store-spec.coffee @@ -141,6 +141,7 @@ describe "DatabaseStore", -> change = DatabaseStore._accumulateAndTrigger.mostRecentCall.args[0] expect(change).toEqual objectClass: TestModel.name, + objectIds: [testModelInstanceA.id, testModelInstanceB.id] objects: [testModelInstanceA, testModelInstanceB] type:'persist' @@ -189,8 +190,12 @@ describe "DatabaseStore", -> advanceClock() expect(@beforeDatabaseChange).toHaveBeenCalledWith( DatabaseStore._query, - [testModelInstanceA, testModelInstanceB], - [testModelInstanceA.id, testModelInstanceB.id], + { + objects: [testModelInstanceA, testModelInstanceB] + objectIds: [testModelInstanceA.id, testModelInstanceB.id] + objectClass: testModelInstanceA.constructor.name + type: 'persist' + }, undefined ) expect(DatabaseStore._writeModels).not.toHaveBeenCalled() @@ -203,8 +208,12 @@ describe "DatabaseStore", -> advanceClock() expect(@afterDatabaseChange).toHaveBeenCalledWith( DatabaseStore._query, - [testModelInstanceA, testModelInstanceB], - [testModelInstanceA.id, testModelInstanceB.id], + { + objects: [testModelInstanceA, testModelInstanceB] + objectIds: [testModelInstanceA.id, testModelInstanceB.id] + objectClass: testModelInstanceA.constructor.name + type: 'persist' + }, "value" ) @@ -263,7 +272,12 @@ describe "DatabaseStore", -> expect(DatabaseStore._accumulateAndTrigger).toHaveBeenCalled() change = DatabaseStore._accumulateAndTrigger.mostRecentCall.args[0] - expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance], type:'unpersist'}) + expect(change).toEqual({ + objectClass: TestModel.name, + objectIds: [testModelInstance.id] + objects: [testModelInstance], + type:'unpersist' + }) describe "when the model has collection attributes", -> it "should delete all of the elements in the join tables", -> diff --git a/spec/stores/thread-counts-store-spec.coffee b/spec/stores/thread-counts-store-spec.coffee index 3446122e8..96a031e20 100644 --- a/spec/stores/thread-counts-store-spec.coffee +++ b/spec/stores/thread-counts-store-spec.coffee @@ -243,39 +243,61 @@ describe "CategoryDatabaseMutationObserver", -> labels: [@label1, @label3] describe "given a set of modifying models", -> - it "should call countsDidChange with the folder / label membership deltas", -> - queryResolves = [] - query = jasmine.createSpy('query').andCallFake => - new Promise (resolve, reject) -> - queryResolves.push(resolve) + scenarios = [{ + type: 'persist', + expected: { + l3: -1, + l2: -1, + l4: 1 + } + },{ + type: 'unpersist', + expected: { + l1: -1, + l3: -2, + l2: -1 + } + }] + scenarios.forEach ({type, expected}) -> + it "should call countsDidChange with the folder / label membership deltas (#{type})", -> + queryResolves = [] + query = jasmine.createSpy('query').andCallFake => + new Promise (resolve, reject) -> + queryResolves.push(resolve) - countsDidChange = jasmine.createSpy('countsDidChange') - m = new ThreadCountsStore.CategoryDatabaseMutationObserver(countsDidChange) + countsDidChange = jasmine.createSpy('countsDidChange') + m = new ThreadCountsStore.CategoryDatabaseMutationObserver(countsDidChange) - beforePromise = m.beforeDatabaseChange(query, [@threadA, @threadB, @threadC], [@threadA.id, @threadB.id, @threadC.id]) - expect(query.callCount).toBe(2) - expect(query.calls[0].args[0]).toEqual("SELECT `Thread`.id as id, `Thread-Label`.`value` as catId FROM `Thread` INNER JOIN `Thread-Label` ON `Thread`.`id` = `Thread-Label`.`id` WHERE `Thread`.id IN ('A','B','C') AND `Thread`.unread = 1") - expect(query.calls[1].args[0]).toEqual("SELECT `Thread`.id as id, `Thread-Folder`.`value` as catId FROM `Thread` INNER JOIN `Thread-Folder` ON `Thread`.`id` = `Thread-Folder`.`id` WHERE `Thread`.id IN ('A','B','C') AND `Thread`.unread = 1") - queryResolves[0]([ - {id: @threadA.id, catId: @label1.id}, - {id: @threadA.id, catId: @label3.id}, - {id: @threadB.id, catId: @label2.id}, - {id: @threadB.id, catId: @label3.id}, - ]) - queryResolves[1]([]) + beforePromise = m.beforeDatabaseChange(query, { + type: type + objects: [@threadA, @threadB, @threadC], + objectIds: [@threadA.id, @threadB.id, @threadC.id] + objectClass: Thread.name + }) + expect(query.callCount).toBe(2) + expect(query.calls[0].args[0]).toEqual("SELECT `Thread`.id as id, `Thread-Label`.`value` as catId FROM `Thread` INNER JOIN `Thread-Label` ON `Thread`.`id` = `Thread-Label`.`id` WHERE `Thread`.id IN ('A','B','C') AND `Thread`.unread = 1") + expect(query.calls[1].args[0]).toEqual("SELECT `Thread`.id as id, `Thread-Folder`.`value` as catId FROM `Thread` INNER JOIN `Thread-Folder` ON `Thread`.`id` = `Thread-Folder`.`id` WHERE `Thread`.id IN ('A','B','C') AND `Thread`.unread = 1") + queryResolves[0]([ + {id: @threadA.id, catId: @label1.id}, + {id: @threadA.id, catId: @label3.id}, + {id: @threadB.id, catId: @label2.id}, + {id: @threadB.id, catId: @label3.id}, + ]) + queryResolves[1]([]) - waitsForPromise => - beforePromise.then (result) => - expect(result).toEqual({ - categories: { - l1: -1, - l3: -2, - l2: -1 - } - }) - m.afterDatabaseChange(query, [@threadA, @threadB, @threadC], [@threadA.id, @threadB.id, @threadC.id], result) - expect(countsDidChange).toHaveBeenCalledWith({ - l3: -1, - l2: -1, - l4: 1 - }) + waitsForPromise => + beforePromise.then (result) => + expect(result).toEqual({ + categories: { + l1: -1, + l3: -2, + l2: -1 + } + }) + m.afterDatabaseChange(query, { + type: type + objects: [@threadA, @threadB, @threadC], + objectIds: [@threadA.id, @threadB.id, @threadC.id] + objectClass: Thread.name + }, result) + expect(countsDidChange).toHaveBeenCalledWith(expected) diff --git a/src/flux/stores/database-store.coffee b/src/flux/stores/database-store.coffee index fdaa5cac8..0995b19da 100644 --- a/src/flux/stores/database-store.coffee +++ b/src/flux/stores/database-store.coffee @@ -447,17 +447,20 @@ class DatabaseStore extends NylasStore clones.push(model.clone()) ids[model.id] = true - ids = Object.keys(ids) + # Note: It's important that we clone the objects since other code could mutate + # them during the save process. We want to guaruntee that the models you send to + # persistModels are saved exactly as they were sent. @atomicMutation => - @_runMutationHooks('beforeDatabaseChange', clones, ids).then (data) => + metadata = + objectClass: clones[0].constructor.name + objectIds: Object.keys(ids) + objects: clones + type: 'persist' + @_runMutationHooks('beforeDatabaseChange', metadata).then (data) => @_writeModels(clones).then => - @_runMutationHooks('afterDatabaseChange', clones, ids, data) - @_accumulateAndTrigger({ - objectClass: clones[0].constructor.name - objects: clones - type: 'persist' - }) + @_runMutationHooks('afterDatabaseChange', metadata, data) + @_accumulateAndTrigger(metadata) # Public: Asynchronously removes `model` from the cache and triggers a change event. # @@ -472,14 +475,15 @@ class DatabaseStore extends NylasStore model = model.clone() @atomicMutation => - @_runMutationHooks('beforeDatabaseChange', [model], [model.id]).then (data) => + metadata = + objectClass: model.constructor.name, + objectIds: [model.id] + objects: [model], + type: 'unpersist' + @_runMutationHooks('beforeDatabaseChange', metadata).then (data) => @_deleteModel(model).then => - @_runMutationHooks('afterDatabaseChange', [model], [model.id], data) - @_accumulateAndTrigger({ - objectClass: model.constructor.name, - objects: [model], - type: 'unpersist' - }) + @_runMutationHooks('afterDatabaseChange', metadata, data) + @_accumulateAndTrigger(metadata) persistJSONObject: (key, json) -> jsonString = serializeRegisteredObjects(json) @@ -512,10 +516,10 @@ class DatabaseStore extends NylasStore removeMutationHook: (hook) -> @_databaseMutationHooks = _.without(@_databaseMutationHooks, hook) - _runMutationHooks: (selectorName, models, ids, data = []) -> + _runMutationHooks: (selectorName, metadata, data = []) -> beforePromises = @_databaseMutationHooks.map (hook, idx) => Promise.try => - hook[selectorName](@_query, models, ids, data[idx]) + hook[selectorName](@_query, metadata, data[idx]) Promise.all(beforePromises).catch (e) => unless NylasEnv.inSpecMode() diff --git a/src/flux/stores/thread-counts-store.coffee b/src/flux/stores/thread-counts-store.coffee index 25a28ea72..f6879728a 100644 --- a/src/flux/stores/thread-counts-store.coffee +++ b/src/flux/stores/thread-counts-store.coffee @@ -10,14 +10,14 @@ Folder = require '../models/folder' Label = require '../models/label' WindowBridge = require '../../window-bridge' -JSONObjectKey = 'UnreadCounts-V1' +JSONObjectKey = 'UnreadCounts-V2' class CategoryDatabaseMutationObserver constructor: (@_countsDidChange) -> - beforeDatabaseChange: (query, models, ids) => - if models[0].constructor.name is 'Thread' - idString = "'" + ids.join("','") + "'" + beforeDatabaseChange: (query, {type, objects, objectIds, objectClass}) => + if objectClass is Thread.name + idString = "'" + objectIds.join("','") + "'" Promise.props labelData: query("SELECT `Thread`.id as id, `Thread-Label`.`value` as catId FROM `Thread` INNER JOIN `Thread-Label` ON `Thread`.`id` = `Thread-Label`.`id` WHERE `Thread`.id IN (#{idString}) AND `Thread`.unread = 1", []) folderData: query("SELECT `Thread`.id as id, `Thread-Folder`.`value` as catId FROM `Thread` INNER JOIN `Thread-Folder` ON `Thread`.`id` = `Thread-Folder`.`id` WHERE `Thread`.id IN (#{idString}) AND `Thread`.unread = 1", []) @@ -31,16 +31,18 @@ class CategoryDatabaseMutationObserver else Promise.resolve() - afterDatabaseChange: (query, models, ids, beforeResolveValue) => - if models[0].constructor.name is 'Thread' + afterDatabaseChange: (query, {type, objects, objectIds, objectClass}, beforeResolveValue) => + if objectClass is Thread.name {categories} = beforeResolveValue - for thread in models - continue unless thread.unread - for collection in ['labels', 'folders'] - if thread[collection] - for cat in thread[collection] - categories[cat.id] ?= 0 - categories[cat.id] += 1 + + if type is 'persist' + for thread in objects + continue unless thread.unread + for collection in ['labels', 'folders'] + if thread[collection] + for cat in thread[collection] + categories[cat.id] ?= 0 + categories[cat.id] += 1 for key, val of categories delete categories[key] if val is 0