From f6b5b7ea30ed5fadb46ee19d4ceb096aa12ed770 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Mon, 15 Jun 2015 18:23:58 -0700 Subject: [PATCH] fix(T1251) When a returnsModel request returns a 404, unpersist the object Summary: Give DatabaseStore trigger payload a 'type' so that you can handle the deletion of objects (clear focus if the object is focused, know to remove it from database view) Fix specs and add one for new DatabaseView => unpersist handler Test Plan: Run new test case Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1639 --- .../thread-list/lib/thread-list-store.coffee | 2 +- spec-nylas/database-view-spec.coffee | 43 +++++++++++++------ spec-nylas/stores/database-store-spec.coffee | 5 ++- src/flux/nylas-api.coffee | 34 ++++++++++++--- src/flux/stores/database-store.coffee | 10 ++--- src/flux/stores/database-view.coffee | 33 ++++++++++---- src/flux/stores/focused-content-store.coffee | 5 ++- src/flux/stores/message-store.coffee | 9 +++- src/flux/tasks/add-remove-tags.coffee | 8 ++-- 9 files changed, 108 insertions(+), 41 deletions(-) diff --git a/internal_packages/thread-list/lib/thread-list-store.coffee b/internal_packages/thread-list/lib/thread-list-store.coffee index 1a2031ec1..ec799f3c0 100644 --- a/internal_packages/thread-list/lib/thread-list-store.coffee +++ b/internal_packages/thread-list/lib/thread-list-store.coffee @@ -93,7 +93,7 @@ ThreadListStore = Reflux.createStore _onDataChanged: (change) -> if change.objectClass is Thread.name - @_view.invalidate({changed: change.objects, shallow: true}) + @_view.invalidate({change: change, shallow: true}) if change.objectClass is Message.name threadIds = _.uniq _.map change.objects, (m) -> m.threadId diff --git a/spec-nylas/database-view-spec.coffee b/spec-nylas/database-view-spec.coffee index 3e284d3c7..ea975666a 100644 --- a/spec-nylas/database-view-spec.coffee +++ b/spec-nylas/database-view-spec.coffee @@ -111,9 +111,9 @@ describe "DatabaseView", -> describe "when the shallow option is provided with specific changed items", -> it "should determine whether changes to these items make page(s) invalid", -> - spyOn(@view, 'invalidateIfItemsInconsistent').andCallFake -> - @view.invalidate({shallow: true, changed: ['a']}) - expect(@view.invalidateIfItemsInconsistent).toHaveBeenCalled() + spyOn(@view, 'invalidateAfterDatabaseChange').andCallFake -> + @view.invalidate({shallow: true, change: {objects: ['a'], type: 'persist'}}) + expect(@view.invalidateAfterDatabaseChange).toHaveBeenCalled() describe "invalidateMetadataFor", -> it "should clear cached metadata for just the items whose ids are provided", -> @@ -129,7 +129,7 @@ describe "DatabaseView", -> expect(@view.retrievePageMetadata).toHaveBeenCalled() expect(@view.retrievePageMetadata.calls[0].args[0]).toEqual('1') - describe "invalidateIfItemsInconsistent", -> + describe "invalidateAfterDatabaseChange", -> beforeEach -> @inbox = new Tag(id: 'inbox', name: 'Inbox') @archive = new Tag(id: 'archive', name: 'archive') @@ -154,40 +154,40 @@ describe "DatabaseView", -> spyOn(@view, 'invalidateRetainedRange') it "should invalidate the entire range if more than 5 items are provided", -> - @view.invalidateIfItemsInconsistent([@a, @b, @c, @d, @e, @f]) + @view.invalidateAfterDatabaseChange({objects:[@a, @b, @c, @d, @e, @f], type:'persist'}) expect(@view.invalidateRetainedRange).toHaveBeenCalled() it "should invalidate the entire range if a provided item is in the set but no longer matches the set", -> a = new Thread(@a) a.tags = [@archive] - @view.invalidateIfItemsInconsistent([a]) + @view.invalidateAfterDatabaseChange({objects:[a], type:'persist'}) expect(@view.invalidateRetainedRange).toHaveBeenCalled() it "should invalidate the entire range if a provided item is not in the set but matches the set", -> incoming = new Thread(id: 'a', subject: 'a', tags:[@inbox], lastMessageTimestamp: new Date()) - @view.invalidateIfItemsInconsistent([incoming]) + @view.invalidateAfterDatabaseChange({objects:[incoming], type:'persist'}) expect(@view.invalidateRetainedRange).toHaveBeenCalled() it "should invalidate the entire range if a provided item matches the set and the value of it's sorting attribute has changed", -> a = new Thread(@a) a.lastMessageTimestamp = new Date(1428526909533) - @view.invalidateIfItemsInconsistent([a]) + @view.invalidateAfterDatabaseChange({objects:[a], type:'persist'}) expect(@view.invalidateRetainedRange).toHaveBeenCalled() it "should not do anything if no provided items are in the set or belong in the set", -> archived = new Thread(id: 'zz', tags: [@archive]) - @view.invalidateIfItemsInconsistent([archived]) + @view.invalidateAfterDatabaseChange({objects:[archived], type: 'persist'}) expect(@view.invalidateRetainedRange).not.toHaveBeenCalled() it "should replace items in place otherwise", -> a = new Thread(@a) a.subject = 'Subject changed, nothing to see here!' - @view.invalidateIfItemsInconsistent([a]) + @view.invalidateAfterDatabaseChange({objects:[a], type: 'persist'}) expect(@view.invalidateRetainedRange).not.toHaveBeenCalled() a = new Thread(@a) a.tags = [@inbox, @archive] # not realistic, but doesn't change membership in set - @view.invalidateIfItemsInconsistent([a]) + @view.invalidateAfterDatabaseChange({objects:[a], type: 'persist'}) expect(@view.invalidateRetainedRange).not.toHaveBeenCalled() it "should attach the metadata field to replaced items", -> @@ -196,7 +196,7 @@ describe "DatabaseView", -> runs -> e = new Thread(@e) e.subject = subject - @view.invalidateIfItemsInconsistent([e]) + @view.invalidateAfterDatabaseChange({objects:[e], type: 'persist'}) waitsFor -> @view._emitter.emit.callCount > 0 runs -> @@ -211,7 +211,24 @@ describe "DatabaseView", -> runs -> b = new Thread(@b) b.tags = [] - @view.invalidateIfItemsInconsistent([b]) + @view.invalidateAfterDatabaseChange({objects:[b], type: 'persist'}) + waitsFor -> + @view._emitter.emit.callCount > 0 + + it "should optimistically remove them and shift result pages", -> + expect(@view._pages[0].items).toEqual([@a, @c, @d]) + expect(@view._pages[1].items).toEqual([@e, @f]) + + it "should change the lastTouchTime date of changed pages so that refreshes started before the replacement do not revert it's changes", -> + expect(@view._pages[0].lastTouchTime isnt @start).toEqual(true) + expect(@view._pages[1].lastTouchTime isnt @start).toEqual(true) + + describe "when items have been unpersisted but still match criteria", -> + beforeEach -> + spyOn(@view._emitter, 'emit') + @start = @view._pages[1].lastTouchTime + runs -> + @view.invalidateAfterDatabaseChange({objects:[@b], type: 'unpersist'}) waitsFor -> @view._emitter.emit.callCount > 0 diff --git a/spec-nylas/stores/database-store-spec.coffee b/spec-nylas/stores/database-store-spec.coffee index 42b6a1f3e..72418e4bd 100644 --- a/spec-nylas/stores/database-store-spec.coffee +++ b/spec-nylas/stores/database-store-spec.coffee @@ -116,7 +116,7 @@ describe "DatabaseStore", -> expect(DatabaseStore.triggerSoon).toHaveBeenCalled() change = DatabaseStore.triggerSoon.mostRecentCall.args[0] - expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance]}) + expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance], type:'persist'}) it "should call through to writeModels", -> spyOn(DatabaseStore, 'writeModels') @@ -132,6 +132,7 @@ describe "DatabaseStore", -> expect(change).toEqual objectClass: TestModel.name, objects: [testModelInstanceA, testModelInstanceB] + type:'persist' it "should call through to writeModels after checking them", -> spyOn(DatabaseStore, 'writeModels') @@ -158,7 +159,7 @@ describe "DatabaseStore", -> expect(DatabaseStore.triggerSoon).toHaveBeenCalled() change = DatabaseStore.triggerSoon.mostRecentCall.args[0] - expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance]}) + expect(change).toEqual({objectClass: TestModel.name, 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/src/flux/nylas-api.coffee b/src/flux/nylas-api.coffee index ca764bb58..aad7bf80d 100644 --- a/src/flux/nylas-api.coffee +++ b/src/flux/nylas-api.coffee @@ -114,7 +114,6 @@ class NylasAPI options.url ?= "#{@APIRoot}#{options.path}" if options.path options.json ?= true options.auth = {'user': @APIToken, 'pass': '', sendImmediately: true} - options.error ?= @_defaultErrorCallback unless options.method is 'GET' or options.formData options.body ?= {} @@ -123,7 +122,10 @@ class NylasAPI PriorityUICoordinator.settle.then => Actions.didMakeAPIRequest({request: options, response: response}) if error? or response.statusCode > 299 - options.error(new APIError({error, response, body})) + if options.returnsModel and response.statusCode is 404 + @_handleModel404(options.url) + if options.error + options.error(new APIError({error, response, body})) else if options.json if _.isString(body) @@ -133,9 +135,34 @@ class NylasAPI options.error(new APIError({error, response, body})) if options.returnsModel @_handleModelResponse(body) + if options.success options.success(body) + # If we make a request that `returnsModel` and we get a 404, we want to handle + # it intelligently and in a centralized way. This method identifies the object + # that could not be found and purges it from local cache. + # + # Handles: + # + # /namespace/// + # /namespace//?thread_id= + # + _handleModel404: (modelUrl) -> + url = require('url') + {pathname, query} = url.parse(modelUrl, true) + components = pathname.split('/') + klassMap = modelClassMap() + + if components.length is 5 + [root, ns, nsId, collection, klassId] = components + klass = klassMap[collection[0..-2]] # Warning: threads => thread + + if klass and klassId and klassId.length > 0 + console.warn("Deleting #{klass.name}:#{klassId} due to API 404") + DatabaseStore.find(klass, klassId).then (model) -> + DatabaseStore.unpersistModel(model) if model + _handleDeltas: (deltas) -> Actions.longPollReceivedRawDeltas(deltas) console.log("Processing Deltas") @@ -184,9 +211,6 @@ class NylasAPI Promise.settle(destroyPromises) - _defaultErrorCallback: (apiError) -> - console.error("Unhandled Nylas API Error:", apiError.message, apiError) - _handleModelResponse: (json) -> new Promise (resolve, reject) => reject(new Error("handleModelResponse with no JSON provided")) unless json diff --git a/src/flux/stores/database-store.coffee b/src/flux/stores/database-store.coffee index 4ee95e1eb..8af3301cc 100644 --- a/src/flux/stores/database-store.coffee +++ b/src/flux/stores/database-store.coffee @@ -234,7 +234,7 @@ class DatabaseStore if not @_changeAccumulated set(change) - else if @_changeAccumulated.objectClass is change.objectClass + else if @_changeAccumulated.objectClass is change.objectClass and @_changeAccumulated.type is change.type concat(change) else flush() @@ -352,7 +352,7 @@ class DatabaseStore tx.execute('BEGIN TRANSACTION') @writeModels(tx, [model]) tx.execute('COMMIT') - @triggerSoon({objectClass: model.constructor.name, objects: [model]}) + @triggerSoon({objectClass: model.constructor.name, objects: [model], type: 'persist'}) # Public: Asynchronously writes `models` to the cache and triggers a single change # event. Note: Models must be of the same class to be persisted in a batch operation. @@ -373,7 +373,7 @@ class DatabaseStore @writeModels(tx, models) tx.execute('COMMIT') - @triggerSoon({objectClass: models[0].constructor.name, objects: models}) + @triggerSoon({objectClass: models[0].constructor.name, objects: models, type: 'persist'}) # Public: Asynchronously removes `model` from the cache and triggers a change event. # @@ -384,7 +384,7 @@ class DatabaseStore tx.execute('BEGIN TRANSACTION') @deleteModel(tx, model) tx.execute('COMMIT') - @triggerSoon({objectClass: model.constructor.name, objects: [model]}) + @triggerSoon({objectClass: model.constructor.name, objects: [model], type: 'unpersist'}) swapModel: ({oldModel, newModel, localId}) => @inTransaction {}, (tx) => @@ -393,7 +393,7 @@ class DatabaseStore @writeModels(tx, [newModel]) @writeModels(tx, [new LocalLink(id: localId, objectId: newModel.id)]) if localId tx.execute('COMMIT') - @triggerSoon({objectClass: newModel.constructor.name, objects: [oldModel, newModel]}) + @triggerSoon({objectClass: newModel.constructor.name, objects: [oldModel, newModel], type: 'swap'}) Actions.didSwapModel({oldModel, newModel, localId}) ### diff --git a/src/flux/stores/database-view.coffee b/src/flux/stores/database-view.coffee index f581610df..f1e579a77 100644 --- a/src/flux/stores/database-view.coffee +++ b/src/flux/stores/database-view.coffee @@ -90,9 +90,22 @@ class DatabaseView extends ModelView padRetainedRange: ({start, end}) -> {start: start - @_pageSize / 2, end: end + @_pageSize / 2} - invalidate: ({shallow, changed} = {}) -> - if shallow and changed - @invalidateIfItemsInconsistent(changed) + # Public: Call this method when the DatabaseStore triggers and will impact the + # data maintained by this DatabaseView. In the future, the DatabaseView will + # probably observe the DatabaseView directly. + # + # - `options` an Object with the following optional keys which can be used to + # optimize the behavior of the DatabaseView: + # - `change`: The change object provided by the DatabaseStore, with `items` and a `type`. + # - `shallow`: True if this change will not invalidate item metadata, only items. + # + # TODO: In order for the DatabaseView to monitor the DatabaseStore directly, + # it needs to have some way of detatching it's listener when it's no longer needed! + # Need a destructor... + # + invalidate: ({shallow, change} = {}) -> + if shallow and change + @invalidateAfterDatabaseChange(change) else if shallow @invalidateCount() @invalidateRetainedRange() @@ -103,12 +116,14 @@ class DatabaseView extends ModelView @invalidateCount() @invalidateRetainedRange() - invalidateIfItemsInconsistent: (items) -> + invalidateAfterDatabaseChange: (change) -> + items = change.objects + if items.length is 0 return if items.length > 5 - @log('invalidateIfItemsInconsistent on '+items.length+'items would be expensive. Invalidating entire range.') + @log("invalidateAfterDatabaseChange on #{items.length} items would be expensive. Invalidating entire range.") @invalidateCount() @invalidateRetainedRange() return @@ -147,15 +162,17 @@ class DatabaseView extends ModelView for item in items idx = @indexOfId(item.id) + itemIsInSet = idx isnt -1 + itemShouldBeInSet = item.matches(@_matchers) and change.type isnt 'unpersist' indexes.push(idx) # The item matches our set but isn't in our items array - if item.matches(@_matchers) and idx is -1 + if not itemIsInSet and itemShouldBeInSet @log("Item matches criteria but not found in cached set. Invalidating entire range.") pagesCouldHaveChanged = true # The item does not match our set, but is in our items array - else if idx isnt -1 and not item.matches(@_matchers) + else if itemIsInSet and not itemShouldBeInSet @log("Item does not match criteria but is in cached set. Invalidating entire range.") pagesCouldHaveChanged = true @@ -166,7 +183,7 @@ class DatabaseView extends ModelView # The value of the item's sort attribute has changed, and we don't # know if it will be in the same position in a new page. - else if idx isnt -1 and sortAttribute + else if itemIsInSet and sortAttribute existing = @get(idx) existingSortValue = existing[sortAttribute.modelKey] itemSortValue = item[sortAttribute.modelKey] diff --git a/src/flux/stores/focused-content-store.coffee b/src/flux/stores/focused-content-store.coffee index f4f286faf..b175001f6 100644 --- a/src/flux/stores/focused-content-store.coffee +++ b/src/flux/stores/focused-content-store.coffee @@ -122,7 +122,10 @@ class FocusedContentStore continue unless val and val.constructor.name is change.objectClass for obj in change.objects if val.id is obj.id - data[key] = obj + if change.type is 'unpersist' + data[key] = null + else + data[key] = obj touched.push(key) if touched.length > 0 diff --git a/src/flux/stores/message-store.coffee b/src/flux/stores/message-store.coffee index 1362d8b34..534b48faa 100644 --- a/src/flux/stores/message-store.coffee +++ b/src/flux/stores/message-store.coffee @@ -133,8 +133,7 @@ MessageStore = Reflux.createStore # If no items were returned, attempt to load messages via the API. If items # are returned, this will trigger a refresh here. if @_items.length is 0 - namespace = NamespaceStore.current() - NylasAPI.getCollection namespace.id, 'messages', {thread_id: @_thread.id} + @_fetchMessages() loaded = false @_expandItemsToDefault() @@ -169,6 +168,10 @@ MessageStore = Reflux.createStore if item.unread or item.draft or idx is @_items.length - 1 @_itemsExpanded[item.id] = true + _fetchMessages: -> + namespace = NamespaceStore.current() + NylasAPI.getCollection namespace.id, 'messages', {thread_id: @_thread.id} + _fetchMessageIdFromAPI: (id) -> return if @_inflight[id] @@ -179,6 +182,8 @@ MessageStore = Reflux.createStore returnsModel: true success: => delete @_inflight[id] + error: => + delete @_inflight[id] _sortItemsForDisplay: (items) -> # Re-sort items in the list so that drafts appear after the message that diff --git a/src/flux/tasks/add-remove-tags.coffee b/src/flux/tasks/add-remove-tags.coffee index ab4e23f9e..fa263f9ca 100644 --- a/src/flux/tasks/add-remove-tags.coffee +++ b/src/flux/tasks/add-remove-tags.coffee @@ -66,10 +66,10 @@ class AddRemoveTagsTask extends Task error: reject onAPIError: (apiError) -> - if "archive" in @tagIdsToAdd - msg = "Failed to archive thread: '#{@thread.subject}'" - Actions.postNotification({message: msg, type: "error"}) - @_rollbackLocal() + if apiError.response.statusCode is 404 + # Do nothing - NylasAPI will destroy the object. + else + @_rollbackLocal() Promise.resolve() _rollbackLocal: ->