From 4959944afb704e392f712f303db12908792ddc9c Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Tue, 14 Jul 2015 12:20:06 -0700 Subject: [PATCH] feat(signatures): Initial signature support Summary: - Draft Store extensions can now implement `prepareNewDraft` to have an opportunity to change a draft before it's displayed for the first time. - When composers are torn down, they delete their draft if it is still pristine. This makes the behavior of closing unedited popout drafts the same as leaving unedited inline drafts. - The DraftStoreProxy keeps the initial body of the draft *if* it started in a pristine state. This means "is the body empty" is just a simple == check, and it takes into account anything added to the body by extensions. - Calling Actions.destroyDraft doesn't blow up anymore if the draft session can't be found. This was a bug and meant that you couldn't destroy drafts which hadn't been previously edited, and also meant that bad things(tm) happened when you called destroyDraft twice, which seemed like overkill. - DestroyDraft task now exits gracefully when the draft cannot be found. You can test this feature by adding the following to your config.cson: ``` signatures: NAMESPACEID: "

\"Nylas\"

Gleb Polyakov 
gleb@nylas.com / 404-786-4100

Nylas 
http://www.nylas.com

" ``` specs for draft store extension hooks, some draft store refactoring Test Plan: Run a few new specs that make sure extensions are run Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1741 --- .../lib/draft-extension.coffee | 14 ++++++ .../composer-signature/lib/main.coffee | 11 ++++ .../composer-signature/package.json | 16 ++++++ .../spec/draft-extension-spec.coffee | 36 +++++++++++++ .../composer/lib/composer-view.cjsx | 12 ++--- .../composer/spec/composer-view-spec.cjsx | 27 +++------- spec-nylas/stores/draft-store-spec.coffee | 40 +++++++++++++-- src/flux/stores/draft-store-extension.coffee | 16 +++++- src/flux/stores/draft-store-proxy.coffee | 23 +++++++-- src/flux/stores/draft-store.coffee | 50 +++++++++++-------- src/flux/tasks/destroy-draft.coffee | 2 +- 11 files changed, 188 insertions(+), 59 deletions(-) create mode 100644 internal_packages/composer-signature/lib/draft-extension.coffee create mode 100644 internal_packages/composer-signature/lib/main.coffee create mode 100755 internal_packages/composer-signature/package.json create mode 100644 internal_packages/composer-signature/spec/draft-extension-spec.coffee diff --git a/internal_packages/composer-signature/lib/draft-extension.coffee b/internal_packages/composer-signature/lib/draft-extension.coffee new file mode 100644 index 000000000..2e03e00d8 --- /dev/null +++ b/internal_packages/composer-signature/lib/draft-extension.coffee @@ -0,0 +1,14 @@ +{DraftStoreExtension, NamespaceStore} = require 'nylas-exports' + +class SignatureDraftStoreExtension extends DraftStoreExtension + @prepareNewDraft: (draft) -> + namespaceId = NamespaceStore.current().id + signature = atom.config.get("signatures.#{namespaceId}") + return unless signature + + insertionPoint = draft.body.indexOf(' + DraftStore.registerExtension(Extension) + + deactivate: -> + DraftStore.unregisterExtension(Extension) + + serialize: -> @state diff --git a/internal_packages/composer-signature/package.json b/internal_packages/composer-signature/package.json new file mode 100755 index 000000000..e2773ade9 --- /dev/null +++ b/internal_packages/composer-signature/package.json @@ -0,0 +1,16 @@ +{ + "name": "composer-signature", + "version": "0.1.0", + "main": "./lib/main", + "description": "A small extension to the draft store that implements signatures", + "license": "Proprietary", + "private": true, + "engines": { + "atom": "*" + }, + "windowTypes": { + "composer": true + }, + "dependencies": { + } +} diff --git a/internal_packages/composer-signature/spec/draft-extension-spec.coffee b/internal_packages/composer-signature/spec/draft-extension-spec.coffee new file mode 100644 index 000000000..dd28655aa --- /dev/null +++ b/internal_packages/composer-signature/spec/draft-extension-spec.coffee @@ -0,0 +1,36 @@ +{Message} = require 'nylas-exports' + +SignatureDraftStoreExtension = require '../lib/draft-extension' + +describe "SignatureDraftStoreExtension", -> + describe "prepareNewDraft", -> + describe "when a signature is defined", -> + beforeEach -> + @signature = "
This is my signature.
" + spyOn(atom.config, 'get').andCallFake => + @signature + + it "should insert the signature at the end of the message or before the first blockquote", -> + a = new Message + draft: true + body: 'This is a test!
Hello world
' + b = new Message + draft: true + body: 'This is a another test.' + + SignatureDraftStoreExtension.prepareNewDraft(a) + expect(a.body).toEqual("This is a test!
This is my signature.
Hello world
") + SignatureDraftStoreExtension.prepareNewDraft(b) + expect(b.body).toEqual("This is a another test
This is my signature.
") + + describe "when a signature is not defined", -> + beforeEach -> + spyOn(atom.config, 'get').andCallFake -> + null + + it "should not do anything", -> + a = new Message + draft: true + body: 'This is a test!
Hello world
' + SignatureDraftStoreExtension.prepareNewDraft(a) + expect(a.body).toEqual('This is a test!
Hello world
') diff --git a/internal_packages/composer/lib/composer-view.cjsx b/internal_packages/composer/lib/composer-view.cjsx index 21a11571d..b9f99266f 100644 --- a/internal_packages/composer/lib/composer-view.cjsx +++ b/internal_packages/composer/lib/composer-view.cjsx @@ -71,7 +71,7 @@ class ComposerView extends React.Component 'composer:show-and-focus-cc': @_showAndFocusCc 'composer:focus-to': => @focus "textFieldTo" 'composer:send-message': => @_sendDraft() - 'composer:delete-empty-draft': => @_deleteEmptyDraft() + 'composer:delete-empty-draft': => @_deleteDraftIfEmpty() "core:undo": @undo "core:redo": @redo } @@ -83,6 +83,7 @@ class ComposerView extends React.Component componentWillUnmount: => @_unmounted = true # rarf @_teardownForDraft() + @_deleteDraftIfEmpty() @_uploadUnlisten() if @_uploadUnlisten @_draftStoreUnlisten() if @_draftStoreUnlisten @_keymapUnlisten.dispose() if @_keymapUnlisten @@ -518,16 +519,11 @@ class ComposerView extends React.Component }) return + bodyIsEmpty = draft.body is @_proxy.draftPristineBody() body = QuotedHTMLParser.removeQuotedHTML(draft.body.toLowerCase().trim()) forwarded = Utils.isForwardedMessage(draft) hasAttachment = (draft.files ? []).length > 0 - # We insert empty br tags before quoted text. - # Our quoted text parser adds additional document elements - onlyHasBr = (/^(]*>)+$/gi).test(body) - onlyHasDoc = (/^<\/head>

<\/body>$/i).test(body) - bodyIsEmpty = body.length is 0 or onlyHasBr or onlyHasDoc - warnings = [] if draft.subject.length is 0 @@ -642,7 +638,7 @@ class ComposerView extends React.Component @undoManager.saveToHistory(historyItem) - _deleteEmptyDraft: => + _deleteDraftIfEmpty: => return unless @_proxy if @_proxy.draft().pristine then Actions.destroyDraft(@props.localId) diff --git a/internal_packages/composer/spec/composer-view-spec.cjsx b/internal_packages/composer/spec/composer-view-spec.cjsx index 35c227324..f20b3e9dd 100644 --- a/internal_packages/composer/spec/composer-view-spec.cjsx +++ b/internal_packages/composer/spec/composer-view-spec.cjsx @@ -47,6 +47,7 @@ passThroughStub = (props={}) -> draftStoreProxyStub = (localId, returnedDraft) -> listen: -> -> draft: -> (returnedDraft ? new Message(draft: true)) + draftPristineBody: -> null draftLocalId: localId cleanup: -> changes: @@ -335,21 +336,17 @@ describe "populated composer", -> expect(dialogArgs.buttons).toEqual ['Edit Message'] describe "empty body warning", -> - it "warns if the body of the email is empty", -> - useDraft.call @, to: [u1], body: "" - makeComposer.call(@) - @composer._sendDraft() - expect(Actions.sendDraft).not.toHaveBeenCalled() - expect(@dialog.showMessageBox).toHaveBeenCalled() - dialogArgs = @dialog.showMessageBox.mostRecentCall.args[1] - expect(dialogArgs.buttons).toEqual ['Cancel', 'Send Anyway'] + it "warns if the body of the email is still the pristine body", -> + pristineBody = "

" - it "warns if the body of the email is all quoted text", -> useDraft.call @, to: [u1] subject: "Hello World" - body: "

" + body: pristineBody makeComposer.call(@) + + spyOn(@composer._proxy, 'draftPristineBody').andCallFake -> pristineBody + @composer._sendDraft() expect(Actions.sendDraft).not.toHaveBeenCalled() expect(@dialog.showMessageBox).toHaveBeenCalled() @@ -365,16 +362,6 @@ describe "populated composer", -> @composer._sendDraft() expect(Actions.sendDraft).toHaveBeenCalled() - it "does not warn if the user has typed a single character in their reply", -> - useDraft.call @, - to: [u1] - subject: "Hello World" - body: "1

This is my quoted text!
" - makeComposer.call(@) - @composer._sendDraft() - expect(Actions.sendDraft).toHaveBeenCalled() - expect(@dialog.showMessageBox).not.toHaveBeenCalled() - it "does not warn if the user has attached a file", -> useDraft.call @, to: [u1] diff --git a/spec-nylas/stores/draft-store-spec.coffee b/spec-nylas/stores/draft-store-spec.coffee index 4028f45ba..4f26e64fe 100644 --- a/spec-nylas/stores/draft-store-spec.coffee +++ b/spec-nylas/stores/draft-store-spec.coffee @@ -5,6 +5,7 @@ ModelQuery = require '../../src/flux/models/query' NamespaceStore = require '../../src/flux/stores/namespace-store' DatabaseStore = require '../../src/flux/stores/database-store' DraftStore = require '../../src/flux/stores/draft-store' +DraftStoreExtension = require '../../src/flux/stores/draft-store-extension' TaskQueue = require '../../src/flux/stores/task-queue' SendDraftTask = require '../../src/flux/tasks/send-draft' DestroyDraftTask = require '../../src/flux/tasks/destroy-draft' @@ -19,7 +20,14 @@ msgFromMe = null msgWithReplyTo = null fakeMessages = null +class TestExtension extends DraftStoreExtension + @prepareNewDraft: (draft) -> + draft.body = "Edited by TestExtension!" + draft.body + describe "DraftStore", -> + beforeEach -> + spyOn(atom, 'newWindow').andCallFake -> + describe "creating drafts", -> beforeEach -> fakeThread = new Thread @@ -274,6 +282,19 @@ describe "DraftStore", -> , (model) -> expect(model.subject).toEqual("Fwd: Fake subject") + describe "extensions", -> + beforeEach -> + DraftStore.registerExtension(TestExtension) + afterEach -> + DraftStore.unregisterExtension(TestExtension) + + it "should give extensions a chance to customize the draft via ext.prepareNewDraft", -> + @_callNewMessageWithContext {threadId: fakeThread.id} + , (thread, message) -> + {} + , (model) -> + expect(model.body.indexOf("Edited by TestExtension!")).toBe(0) + describe "context", -> it "should accept `thread` or look up a thread when given `threadId`", -> @_callNewMessageWithContext {thread: fakeThread} @@ -430,11 +451,10 @@ describe "DraftStore", -> DraftStore._onDestroyDraft('abc') expect(@draftReset).toHaveBeenCalled() - it "should throw if the draft session is not in the window", -> + it "should not throw if the draft session is not in the window", -> expect -> DraftStore._onDestroyDraft('other') - .toThrow() - expect(@draftReset).not.toHaveBeenCalled() + .not.toThrow() it "should queue a destroy draft task", -> DraftStore._onDestroyDraft('abc') @@ -603,6 +623,20 @@ describe "DraftStore", -> expect(@draftCleanup).toHaveBeenCalled describe "mailto handling", -> + describe "extensions", -> + beforeEach -> + DraftStore.registerExtension(TestExtension) + afterEach -> + DraftStore.unregisterExtension(TestExtension) + + it "should give extensions a chance to customize the draft via ext.prepareNewDraft", -> + received = null + spyOn(DatabaseStore, 'persistModel').andCallFake (draft) -> + received = draft + Promise.resolve() + DraftStore._onHandleMailtoLink('mailto:bengotow@gmail.com') + expect(received.body.indexOf("Edited by TestExtension!")).toBe(0) + it "should correctly instantiate drafts for a wide range of mailto URLs", -> received = null spyOn(DatabaseStore, 'persistModel').andCallFake (draft) -> diff --git a/src/flux/stores/draft-store-extension.coffee b/src/flux/stores/draft-store-extension.coffee index a3a477cb1..7ccc7f953 100644 --- a/src/flux/stores/draft-store-extension.coffee +++ b/src/flux/stores/draft-store-extension.coffee @@ -44,7 +44,21 @@ class DraftStoreExtension [] ### - Public: Override onMouseUp in your DraftStoreExtension subclass to transform + Public: Override prepareNewDraft to modify a brand new draft before it is displayed + in a composer. This is one of the only places in the application where it's safe + to modify the draft object you're given directly to add participants to the draft, + add a signature, etc. + + By default, new drafts are considered `pristine`. If the user leaves the composer + without making any changes, the draft is discarded. If your extension populates + the draft in a way that makes it "populated" in a valuable way, you should set + `draft.pristine = false` so the draft saves, even if no further changes are made. + ### + @prepareNewDraft: (draft) -> + return + + ### + Public: Override finalizeSessionBeforeSending in your DraftStoreExtension subclass to transform the {DraftStoreProxy} editing session just before the draft is sent. This method gives you an opportunity to make any final substitutions or changes after any {::warningsForSending} have been displayed. diff --git a/src/flux/stores/draft-store-proxy.coffee b/src/flux/stores/draft-store-proxy.coffee index f9e532e05..5c821a468 100644 --- a/src/flux/stores/draft-store-proxy.coffee +++ b/src/flux/stores/draft-store-proxy.coffee @@ -88,27 +88,36 @@ class DraftStoreProxy @listenTo DraftStore, @_onDraftChanged @listenTo Actions.didSwapModel, @_onDraftSwapped + @_draft = false + @_draftPristineBody = null + @changes = new DraftChangeSet @draftLocalId, => if !@_draft throw new Error("DraftChangeSet was modified before the draft was prepared.") @trigger() if draft - @_draft = draft + @_setDraft(draft) @_draftPromise = Promise.resolve(@) - else - @_draft = false - @_draftPromise = null @prepare().catch (error) -> console.error(error) console.error(error.stack) throw new Error("DraftStoreProxy prepare() failed with error #{error.toString()}.") + # Public: Returns the draft object with the latest changes applied. + # draft: -> @changes.applyToModel(@_draft) @_draft + # Public: Returns the initial body of the draft when it was pristine, or null if the + # draft was never pristine in this editing session. Useful for determining if the + # body is still in an unchanged / empty state. + # + draftPristineBody: -> + @_draftPristineBody + prepare: -> @_draftPromise ?= new Promise (resolve, reject) => DatabaseStore = require './database-store' @@ -127,6 +136,12 @@ class DraftStoreProxy _setDraft: (draft) -> if !draft.body? throw new Error("DraftStoreProxy._setDraft - new draft has no body!") + + # We keep track of the draft's initial body if it's pristine when the editing + # session begins. This initial value powers things like "are you sure you want + # to send with an empty body?" + if draft.pristine + @_draftPristineBody = draft.body @_draft = draft @trigger() diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index a51e32528..8eb83a998 100644 --- a/src/flux/stores/draft-store.coffee +++ b/src/flux/stores/draft-store.coffee @@ -200,6 +200,26 @@ class DraftStore @_newMessageWithContext context, (thread, message) -> forwardMessage: message + _finalizeAndPersistNewMessage: (draft) => + # Give extensions an opportunity to perform additional setup to the draft + for extension in @_extensions + continue unless extension.prepareNewDraft + extension.prepareNewDraft(draft) + + # Normally we'd allow the DatabaseStore to create a localId, wait for it to + # commit a LocalLink and resolve, etc. but it's faster to create one now. + draftLocalId = generateTempId() + + # Optimistically create a draft session and hand it the draft so that it + # doesn't need to do a query for it a second from now when the composer wants it. + @_draftSessions[draftLocalId] = new DraftStoreProxy(draftLocalId, draft) + + Promise.all([ + DatabaseStore.bindToLocalId(draft, draftLocalId) + DatabaseStore.persistModel(draft) + ]).then => + return Promise.resolve({draftLocalId}) + _newMessageWithContext: ({thread, threadId, message, messageId, popout}, attributesCallback) => return unless NamespaceStore.current() @@ -290,16 +310,7 @@ class DraftStore threadId: thread.id namespaceId: thread.namespaceId - # Normally we'd allow the DatabaseStore to create a localId, wait for it to - # commit a LocalLink and resolve, etc. but it's faster to create one now. - draftLocalId = generateTempId() - - # Optimistically create a draft session and hand it the draft so that it - # doesn't need to do a query for it a second from now when the composer wants it. - @_draftSessions[draftLocalId] = new DraftStoreProxy(draftLocalId, draft) - - DatabaseStore.bindToLocalId(draft, draftLocalId) - DatabaseStore.persistModel(draft).then => + @_finalizeAndPersistNewMessage(draft).then ({draftLocalId}) => Actions.composePopoutDraft(draftLocalId) if popout @@ -323,10 +334,8 @@ class DraftStore pristine: true namespaceId: namespace.id - DatabaseStore.persistModel(draft).then => - DatabaseStore.localIdForModel(draft).then (draftLocalId, options={}) => - options.newDraft = true - @_onPopoutDraftLocalId(draftLocalId, options) + @_finalizeAndPersistNewMessage(draft).then ({draftLocalId}) => + @_onPopoutDraftLocalId(draftLocalId, {newDraft: true}) _onPopoutDraftLocalId: (draftLocalId, options = {}) => return unless NamespaceStore.current() @@ -368,23 +377,20 @@ class DraftStore if query[attr] draft[attr] = ContactStore.parseContactsInString(query[attr]) - DatabaseStore.persistModel(draft).then => - DatabaseStore.localIdForModel(draft).then(@_onPopoutDraftLocalId) + @_finalizeAndPersistNewMessage(draft).then ({draftLocalId}) => + @_onPopoutDraftLocalId(draftLocalId) _onDestroyDraft: (draftLocalId) => session = @_draftSessions[draftLocalId] - if not session - throw new Error("Couldn't find the draft session in the current window") - # Immediately reset any pending changes so no saves occur - session.changes.reset() + if session + session.changes.reset() + @_doneWithSession(session) # Queue the task to destroy the draft Actions.queueTask(new DestroyDraftTask(draftLocalId)) - @_doneWithSession(session) - atom.close() if @_isPopout() # The user request to send the draft diff --git a/src/flux/tasks/destroy-draft.coffee b/src/flux/tasks/destroy-draft.coffee index 08f99582e..2f3e569be 100644 --- a/src/flux/tasks/destroy-draft.coffee +++ b/src/flux/tasks/destroy-draft.coffee @@ -26,7 +26,7 @@ class DestroyDraftTask extends Task return Promise.reject(new Error("Attempt to call DestroyDraftTask.performLocal without @draftLocalId")) DatabaseStore.findByLocalId(Message, @draftLocalId).then (draft) => - return resolve() unless draft + return Promise.resolve() unless draft @draft = draft DatabaseStore.unpersistModel(draft)