From 041817c589d6b656a89fcb2af4f4fc5f90f81f18 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Thu, 10 Mar 2016 11:03:38 -0800 Subject: [PATCH] patch(save): Only save drafts when necessary, avoid sync-engine issues Summary: Previously, we have saved drafts back to the user's provider through the sync engine. There are a handful of very serious edge case issues we're working to solve that are creating a bad user experience. (#933, #1175, #1504, #1237) For now, we're going to change the behavior of N1 to mitagate these issues. - If you create a draft in N1, we will not sync it to other mail clients while you're working on it. - If you enable send later, we'll start syncing the draft to the server as before. - If you created the draft in another client, we'll sync the draft to the server as before. Fix specs Test Plan: Run specs Reviewers: evan, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D2706 --- .../send-later/lib/send-later-store.js | 11 ++++- spec/stores/draft-store-proxy-spec.coffee | 49 +++++++++---------- src/flux/stores/draft-store-proxy.coffee | 27 ++++++---- src/flux/stores/draft-store.coffee | 3 +- src/flux/tasks/syncback-draft-task.coffee | 1 - 5 files changed, 52 insertions(+), 39 deletions(-) diff --git a/internal_packages/send-later/lib/send-later-store.js b/internal_packages/send-later/lib/send-later-store.js index 2d8532b2d..7210f9d5c 100644 --- a/internal_packages/send-later/lib/send-later-store.js +++ b/internal_packages/send-later/lib/send-later-store.js @@ -1,6 +1,12 @@ /** @babel */ import NylasStore from 'nylas-store' -import {NylasAPI, Actions, Message, DatabaseStore} from 'nylas-exports' +import { + NylasAPI, + Actions, + Message, + DatabaseStore, + SyncbackDraftTask, +} from 'nylas-exports' import SendLaterActions from './send-later-actions' import {PLUGIN_ID, PLUGIN_NAME} from './send-later-constants' @@ -28,6 +34,9 @@ class SendLaterStore extends NylasStore { return NylasAPI.authPlugin(this.pluginId, this.pluginName, accountId) .then(()=> { Actions.setMetadata(messages, this.pluginId, metadata); + + // Important: Do not remove this unless N1 is syncing drafts by default. + Actions.queueTask(new SyncbackDraftTask(draftClientId)); }) .catch((error)=> { NylasEnv.reportError(error); diff --git a/spec/stores/draft-store-proxy-spec.coffee b/spec/stores/draft-store-proxy-spec.coffee index 521259865..b626dfb60 100644 --- a/spec/stores/draft-store-proxy-spec.coffee +++ b/spec/stores/draft-store-proxy-spec.coffee @@ -35,12 +35,6 @@ describe "DraftChangeSet", -> @changeSet.add(body: 'Hello World!') expect(@changeSet._pending.body).toEqual('Hello World!') - describe "when the immediate option is passed", -> - it "should commit", -> - spyOn(@changeSet, 'commit') - @changeSet.add({body: 'Hello World!'}, {immediate: true}) - expect(@changeSet.commit).toHaveBeenCalled() - describe "otherwise", -> it "should commit after thirty seconds", -> spyOn(@changeSet, 'commit') @@ -50,7 +44,6 @@ describe "DraftChangeSet", -> expect(@changeSet.commit).toHaveBeenCalled() describe "commit", -> - it "should resolve immediately if the pending set is empty", -> @changeSet._pending = {} waitsForPromise => @@ -193,53 +186,57 @@ describe "DraftStoreProxy", -> expect(@proxy.draft().subject).toEqual(updatedDraft.subject) it "atomically commits changes", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft)) + spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft)) spyOn(DatabaseStore, 'inTransaction').andCallThrough() + @proxy.changes.add({body: "123"}) waitsForPromise => - @proxy.changes.add({body: "123"}, {immediate: true}).then => + @proxy.changes.commit().then => expect(DatabaseStore.inTransaction).toHaveBeenCalled() expect(DatabaseStore.inTransaction.calls.length).toBe 1 it "persist the applied changes", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft)) + spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft)) + @proxy.changes.add({body: "123"}) waitsForPromise => - @proxy.changes.add({body: "123"}, {immediate: true}).then => + @proxy.changes.commit().then => expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled() updated = DatabaseTransaction.prototype.persistModel.calls[0].args[0] expect(updated.body).toBe "123" - it "queues a SyncbackDraftTask", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft)) - waitsForPromise => - @proxy.changes.add({body: "123"}, {immediate: true}).then => - expect(Actions.queueTask).toHaveBeenCalled() - task = Actions.queueTask.calls[0].args[0] - expect(task.draftClientId).toBe "client-id" + # Note: Syncback temporarily disabled + # + # it "queues a SyncbackDraftTask", -> + # spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft)) + # @proxy.changes.add({body: "123"}) + # waitsForPromise => + # @proxy.changes.commit().then => + # expect(Actions.queueTask).toHaveBeenCalled() + # task = Actions.queueTask.calls[0].args[0] + # expect(task.draftClientId).toBe "client-id" it "doesn't queues a SyncbackDraftTask if no Syncback is passed", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft)) + spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft)) waitsForPromise => @proxy.changes.commit({noSyncback: true}).then => expect(Actions.queueTask).not.toHaveBeenCalled() describe "when findBy does not return a draft", -> it "continues and persists it's local draft reference, so it is resaved and draft editing can continue", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(null)) + spyOn(DatabaseStore, "run").andReturn(Promise.resolve(null)) + @proxy.changes.add({body: "123"}) waitsForPromise => - @proxy.changes.add({body: "123"}, {immediate: true}).then => + @proxy.changes.commit().then => expect(DatabaseTransaction.prototype.persistModel).toHaveBeenCalled() updated = DatabaseTransaction.prototype.persistModel.calls[0].args[0] expect(updated.body).toBe "123" - expect(Actions.queueTask).toHaveBeenCalled() - task = Actions.queueTask.calls[0].args[0] - expect(task.draftClientId).toBe "client-id" it "does nothing if the draft is marked as destroyed", -> - spyOn(DatabaseStore, "findBy").andReturn(Promise.resolve(@draft)) + spyOn(DatabaseStore, "run").andReturn(Promise.resolve(@draft)) spyOn(DatabaseStore, 'inTransaction').andCallThrough() waitsForPromise => @proxy._destroyed = true - @proxy.changes.add({body: "123"}, {immediate: true}).then => + @proxy.changes.add({body: "123"}) + @proxy.changes.commit().then => expect(DatabaseStore.inTransaction).not.toHaveBeenCalled() describe "draft pristine body", -> diff --git a/src/flux/stores/draft-store-proxy.coffee b/src/flux/stores/draft-store-proxy.coffee index eeda6287c..c9dc93d20 100644 --- a/src/flux/stores/draft-store-proxy.coffee +++ b/src/flux/stores/draft-store-proxy.coffee @@ -36,19 +36,16 @@ class DraftChangeSet clearTimeout(@_timer) @_timer = null - add: (changes, {immediate, silent}={}) => + add: (changes, {silent} = {}) => @_pending = _.extend(@_pending, changes) @_pending['pristine'] = false @_onTrigger() unless silent - if immediate - @commit() - else - clearTimeout(@_timer) if @_timer - @_timer = setTimeout(@commit, 10000) + + clearTimeout(@_timer) if @_timer + @_timer = setTimeout(@commit, 10000) commit: ({noSyncback}={}) => @_commitChain = @_commitChain.finally => - if Object.keys(@_pending).length is 0 return Promise.resolve(true) @@ -165,7 +162,7 @@ class DraftStoreProxy inMemoryDraft = @_draft DatabaseStore.inTransaction (t) => - t.findBy(Message, clientId: inMemoryDraft.clientId).then (draft) => + t.findBy(Message, clientId: inMemoryDraft.clientId).include(Message.attributes.body).then (draft) => # This can happen if we get a "delete" delta, or something else # strange happens. In this case, we'll use the @_draft we have in # memory to apply the changes to. On the `persistModel` in the @@ -173,14 +170,24 @@ class DraftStoreProxy # `SyncbackDraftTask` may then fail due to differing Ids not # existing, but if this happens it'll 404 and recover gracefully # by creating a new draft - if not draft then draft = inMemoryDraft - + draft ?= inMemoryDraft updatedDraft = @changes.applyToModel(draft) return t.persistModel(updatedDraft) + .then => return if noSyncback + # We have temporarily disabled the syncback of most drafts to user's mail + # providers, due to a number of issues in the sync-engine that we're still + # firefighting. + # + # For now, drafts are only synced when you choose "Send Later", and then + # once they have a serverId we sync them periodically here. + # + return unless @_draft.serverId + Actions.queueTask(new SyncbackDraftTask(@draftClientId)) + DraftStoreProxy.DraftChangeSet = DraftChangeSet module.exports = DraftStoreProxy diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index f8bda4eb1..640c68f40 100644 --- a/src/flux/stores/draft-store.coffee +++ b/src/flux/stores/draft-store.coffee @@ -546,7 +546,8 @@ class DraftStore @sessionForClientId(messageClientId).then (session) -> files = _.clone(session.draft().files) ? [] files = _.reject files, (f) -> f.id is file.id - session.changes.add({files}, immediate: true) + session.changes.add({files}) + session.changes.commit() _onDraftSendingFailed: ({draftClientId, threadId, errorMessage}) -> @_draftsSending[draftClientId] = false diff --git a/src/flux/tasks/syncback-draft-task.coffee b/src/flux/tasks/syncback-draft-task.coffee index 4ee59fa09..db6640139 100644 --- a/src/flux/tasks/syncback-draft-task.coffee +++ b/src/flux/tasks/syncback-draft-task.coffee @@ -12,7 +12,6 @@ SyncbackMetadataTask = require './syncback-metadata-task' Message = require '../models/message' Account = require '../models/account' -# MutateDraftTask module.exports = class SyncbackDraftTask extends Task