Foundry376/Mailspring
1
1
Fork 0
mirror of https://github.com/Foundry376/Mailspring.git synced 2024-09-21 15:56:10 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(T1251) When a returnsModel request returns a 404, unpersist the object

Browse source 
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
This commit is contained in:
Ben Gotow 2015-06-15 18:23:58 -07:00
parent e090dd428a
commit f6b5b7ea30
9 changed files with 108 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
internal_packages/thread-list/lib/thread-list-store.coffee
View file

@ -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

43
spec-nylas/database-view-spec.coffee
View file

@ -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

5
spec-nylas/stores/database-store-spec.coffee
View file

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

34
src/flux/nylas-api.coffee
View file

@ -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/<nid>/<collection>/<id>
# /namespace/<nid>/<collection>?thread_id=<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

10
src/flux/stores/database-store.coffee
View file

@ -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})
###

33
src/flux/stores/database-view.coffee
View file

@ -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]

5
src/flux/stores/focused-content-store.coffee
View file

@ -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

9
src/flux/stores/message-store.coffee
View file

@ -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

8
src/flux/tasks/add-remove-tags.coffee
View file

@ -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: ->