fix(counts): compute deltas for unpersist events, more specs

This fix should resolve #489

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", ->

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", ->
+    scenarios = [{
-      queryResolves = []
+        type: 'persist',
-      query = jasmine.createSpy('query').andCallFake =>
+        expected: {
-        new Promise (resolve, reject) ->
+          l3: -1,
-          queryResolves.push(resolve)
+          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')
+        countsDidChange = jasmine.createSpy('countsDidChange')
-      m = new ThreadCountsStore.CategoryDatabaseMutationObserver(countsDidChange)
+        m = new ThreadCountsStore.CategoryDatabaseMutationObserver(countsDidChange)
 
-      beforePromise = m.beforeDatabaseChange(query, [@threadA, @threadB, @threadC], [@threadA.id, @threadB.id, @threadC.id])
+        beforePromise = m.beforeDatabaseChange(query, {
-      expect(query.callCount).toBe(2)
+          type: type
-      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")
+          objects: [@threadA, @threadB, @threadC],
-      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")
+          objectIds: [@threadA.id, @threadB.id, @threadC.id]
-      queryResolves[0]([
+          objectClass: Thread.name
-        {id: @threadA.id, catId: @label1.id},
+        })
-        {id: @threadA.id, catId: @label3.id},
+        expect(query.callCount).toBe(2)
-        {id: @threadB.id, catId: @label2.id},
+        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")
-        {id: @threadB.id, catId: @label3.id},
+        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]([
-      queryResolves[1]([])
+          {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 =>
+        waitsForPromise =>
-        beforePromise.then (result) =>
+          beforePromise.then (result) =>
-          expect(result).toEqual({
+            expect(result).toEqual({
-            categories: {
+              categories: {
-              l1: -1,
+                l1: -1,
-              l3: -2,
+                l3: -2,
-              l2: -1
+                l2: -1
-            }
+              }
-          })
+            })
-          m.afterDatabaseChange(query, [@threadA, @threadB, @threadC], [@threadA.id, @threadB.id, @threadC.id], result)
+            m.afterDatabaseChange(query, {
-          expect(countsDidChange).toHaveBeenCalledWith({
+              type: type
-            l3: -1,
+              objects: [@threadA, @threadB, @threadC],
-            l2: -1,
+              objectIds: [@threadA.id, @threadB.id, @threadC.id]
-            l4: 1
+              objectClass: Thread.name
-          })
+            }, result)
+            expect(countsDidChange).toHaveBeenCalledWith(expected)

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)
+          @_runMutationHooks('afterDatabaseChange', metadata, data)
-          @_accumulateAndTrigger({
+          @_accumulateAndTrigger(metadata)
-            objectClass: clones[0].constructor.name
-            objects: clones
-            type: 'persist'
-          })
 
   # 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)
+          @_runMutationHooks('afterDatabaseChange', metadata, data)
-          @_accumulateAndTrigger({
+          @_accumulateAndTrigger(metadata)
-            objectClass: model.constructor.name,
-            objects: [model],
-            type: 'unpersist'
-          })
 
   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()

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) =>
+  beforeDatabaseChange: (query, {type, objects, objectIds, objectClass}) =>
-    if models[0].constructor.name is 'Thread'
+    if objectClass is Thread.name
-      idString = "'" + ids.join("','") +  "'"
+      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) =>
+  afterDatabaseChange: (query, {type, objects, objectIds, objectClass}, beforeResolveValue) =>
-    if models[0].constructor.name is 'Thread'
+    if objectClass is Thread.name
       {categories} = beforeResolveValue
-      for thread in models
+
-        continue unless thread.unread
+      if type is 'persist'
-        for collection in ['labels', 'folders']
+        for thread in objects
-          if thread[collection]
+          continue unless thread.unread
-            for cat in thread[collection]
+          for collection in ['labels', 'folders']
-              categories[cat.id] ?= 0
+            if thread[collection]
-              categories[cat.id] += 1
+              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