diff --git a/internal_packages/composer-signature/lib/signature-composer-extension.es6 b/internal_packages/composer-signature/lib/signature-composer-extension.es6 index 044985208..3f882090b 100644 --- a/internal_packages/composer-signature/lib/signature-composer-extension.es6 +++ b/internal_packages/composer-signature/lib/signature-composer-extension.es6 @@ -11,4 +11,14 @@ export default class SignatureComposerExtension extends ComposerExtension { } draft.body = SignatureUtils.applySignature(draft.body, signature); } + + static finalizeSessionBeforeSending = ({session}) => { + // remove the element from the DOM, + // essentially unwraps the signature + const body = session.draft().body; + const changed = body.replace(/<\/?signature>/g, ''); + if (body !== changed) { + session.changes.add({body: changed}) + } + } } diff --git a/internal_packages/composer-signature/lib/signature-utils.es6 b/internal_packages/composer-signature/lib/signature-utils.es6 index d4cab2cd0..d1565d005 100644 --- a/internal_packages/composer-signature/lib/signature-utils.es6 +++ b/internal_packages/composer-signature/lib/signature-utils.es6 @@ -1,24 +1,28 @@ export default { applySignature(body, signature) { // https://regex101.com/r/nC0qL2/1 - const signatureRegex = /
[^]*<\/div>/; + const signatureRegex = /[^]*<\/signature>/; - let signatureHTML = '
' + signature + '
'; - let insertionPoint = body.search(signatureRegex); let newBody = body; + let paddingBefore = ''; + let paddingAfter = ''; - // If there is a signature already present - if (insertionPoint !== -1) { - // Remove it - newBody = newBody.replace(signatureRegex, ""); + // Remove any existing signature in the body + newBody = newBody.replace(signatureRegex, ""); + + // http://www.regexpal.com/?fam=94390 + // prefer to put the signature one
before the beginning of the quote, + // if possible. + let insertionPoint = newBody.search(/<\w+[^>]*gmail_quote/i); + if (insertionPoint === -1) { + insertionPoint = newBody.length; + paddingBefore = '

'; } else { - insertionPoint = newBody.indexOf('${paddingBefore}${signature}${paddingAfter}${contentAfter}`; }, }; diff --git a/internal_packages/composer-signature/spec/signature-composer-extension-spec.es6 b/internal_packages/composer-signature/spec/signature-composer-extension-spec.es6 index 464a9a947..3b36fa40d 100644 --- a/internal_packages/composer-signature/spec/signature-composer-extension-spec.es6 +++ b/internal_packages/composer-signature/spec/signature-composer-extension-spec.es6 @@ -2,19 +2,41 @@ import {Message} from 'nylas-exports'; import SignatureComposerExtension from '../lib/signature-composer-extension'; import SignatureStore from '../lib/signature-store'; +const TEST_SIGNATURE = '
This is my signature.
'; + describe("SignatureComposerExtension", ()=> { + describe("finalizeSessionBeforeSending", ()=> { + it("should unwrap the signature and remove the custom DOM element", ()=> { + const a = new Message({ + draft: true, + accountId: TEST_ACCOUNT_ID, + body: `This is a test! ${TEST_SIGNATURE}
Hello world
`, + }); + const add = jasmine.createSpy('changes.add'); + const session = { + draft: ()=> a, + changes: { + add: add, + }, + } + SignatureComposerExtension.finalizeSessionBeforeSending({session}); + expect(add).toHaveBeenCalledWith({ + body: `This is a test! ${TEST_SIGNATURE}
Hello world
`, + }) + }); + }); + describe("prepareNewDraft", ()=> { describe("when a signature is defined", ()=> { beforeEach(()=> { - this.signature = '
This is my signature.
'; - spyOn(NylasEnv.config, 'get').andCallFake(()=> this.signature); + spyOn(NylasEnv.config, 'get').andCallFake(()=> TEST_SIGNATURE); }); - it("should insert the signature at the end of the message or before the first blockquote and have a newline", ()=> { + it("should insert the signature at the end of the message or before the first quoted text block and have a newline", ()=> { const a = new Message({ draft: true, accountId: TEST_ACCOUNT_ID, - body: 'This is a test!
Hello world
', + body: 'This is a test!
Hello world
', }); const b = new Message({ draft: true, @@ -23,36 +45,36 @@ describe("SignatureComposerExtension", ()=> { }); SignatureComposerExtension.prepareNewDraft({draft: a}); - expect(a.body).toEqual('This is a test!
This is my signature.
Hello world
'); + expect(a.body).toEqual(`This is a test! ${TEST_SIGNATURE}
Hello world
`); SignatureComposerExtension.prepareNewDraft({draft: b}); - expect(b.body).toEqual('This is a another test.

This is my signature.
'); + expect(b.body).toEqual(`This is a another test.

${TEST_SIGNATURE}`); }); - it("should replace the signature if a signature is already present", ()=> { - const scenarios = [ - { - // With blockquote - body: 'This is a test!
SIG
Hello world
', - expected: `This is a test!
${this.signature}
Hello world
`, - }, - { - // Populated signature div - body: 'This is a test!
SIG
', - expected: `This is a test!
${this.signature}
`, - }, - { - // Empty signature div - body: 'This is a test!
', - expected: `This is a test!
${this.signature}
`, - }, - { - // With newlines - body: 'This is a test!
\n
\n
SIG
\n
', - expected: `This is a test!
${this.signature}
`, - }, - ] + const scenarios = [ + { + name: 'With blockquote', + body: `This is a test!
SIG
Hello world
`, + expected: `This is a test! ${TEST_SIGNATURE}
Hello world
`, + }, + { + name: 'Populated signature div', + body: `This is a test!

SIG
`, + expected: `This is a test!

${TEST_SIGNATURE}`, + }, + { + name: 'Empty signature div', + body: 'This is a test! ', + expected: `This is a test!

${TEST_SIGNATURE}`, + }, + { + name: 'With newlines', + body: 'This is a test! \n
\n
SIG
\n', + expected: `This is a test!

${TEST_SIGNATURE}`, + }, + ] - scenarios.forEach((scenario)=> { + scenarios.forEach((scenario)=> { + it(`should replace the signature if a signature is already present (${scenario.name})`, ()=> { const message = new Message({ draft: true, body: scenario.body, @@ -60,7 +82,7 @@ describe("SignatureComposerExtension", ()=> { }) SignatureComposerExtension.prepareNewDraft({draft: message}); expect(message.body).toEqual(scenario.expected) - }) + }); }); }); @@ -73,10 +95,10 @@ describe("SignatureComposerExtension", ()=> { const a = new Message({ draft: true, accountId: TEST_ACCOUNT_ID, - body: 'This is a test!
Hello world
', + body: 'This is a test!
Hello world
', }); SignatureComposerExtension.prepareNewDraft({draft: a}); - expect(a.body).toEqual(`This is a test!
${SignatureStore.DefaultSignature}
Hello world
`); + expect(a.body).toEqual(`This is a test! ${SignatureStore.DefaultSignature}
Hello world
`); }); }); @@ -90,10 +112,10 @@ describe("SignatureComposerExtension", ()=> { const a = new Message({ draft: true, accountId: TEST_ACCOUNT_ID, - body: 'This is a test!
Hello world
', + body: 'This is a test!
Hello world
', }); SignatureComposerExtension.prepareNewDraft({draft: a}); - expect(a.body).toEqual(`This is a test!
Hello world
`); + expect(a.body).toEqual(`This is a test!
Hello world
`); }); }); }); diff --git a/internal_packages/composer-templates/lib/template-store.es6 b/internal_packages/composer-templates/lib/template-store.es6 index 45077cc4f..2a4136037 100644 --- a/internal_packages/composer-templates/lib/template-store.es6 +++ b/internal_packages/composer-templates/lib/template-store.es6 @@ -108,7 +108,7 @@ class TemplateStore extends NylasStore { const draftName = name ? name : draft.subject.replace(TemplateStore.INVALID_TEMPLATE_NAME_REGEX, ''); let draftContents = contents ? contents : QuotedHTMLTransformer.removeQuotedHTML(draft.body); - const sigIndex = draftContents.indexOf('
'); + const sigIndex = draftContents.indexOf(''); draftContents = sigIndex > -1 ? draftContents.slice(0, sigIndex) : draftContents; if (!draftName || draftName.length === 0) { this._displayError('Give your draft a subject to name your template.'); @@ -259,7 +259,7 @@ class TemplateStore extends NylasStore { if (proceed) { const draftContents = QuotedHTMLTransformer.removeQuotedHTML(session.draft().body); - const sigIndex = draftContents.indexOf('
'); + const sigIndex = draftContents.indexOf(''); const signature = sigIndex > -1 ? draftContents.slice(sigIndex) : ''; const draftHtml = QuotedHTMLTransformer.appendQuotedHTML(templateBody + signature, session.draft().body); diff --git a/internal_packages/composer/lib/composer-editor.jsx b/internal_packages/composer/lib/composer-editor.jsx index 3ed72957b..84ede59a3 100644 --- a/internal_packages/composer/lib/composer-editor.jsx +++ b/internal_packages/composer/lib/composer-editor.jsx @@ -144,7 +144,7 @@ class ComposerEditor extends Component { _findLastNodeBeforeQuoteOrSignature(editor) { const walker = document.createTreeWalker(editor.rootNode, NodeFilter.SHOW_TEXT); - const nodesBelowUserBody = editor.rootNode.querySelectorAll('.nylas-n1-signature, .gmail_quote, blockquote'); + const nodesBelowUserBody = editor.rootNode.querySelectorAll('signature, .gmail_quote, blockquote'); let lastNode = null; let node = walker.nextNode(); diff --git a/internal_packages/composer/lib/composer-view.cjsx b/internal_packages/composer/lib/composer-view.cjsx index 6237c98a1..13fd309fb 100644 --- a/internal_packages/composer/lib/composer-view.cjsx +++ b/internal_packages/composer/lib/composer-view.cjsx @@ -732,7 +732,7 @@ class ComposerView extends React.Component _mentionsAttachment: (body) => body = QuotedHTMLTransformer.removeQuotedHTML(body.toLowerCase().trim()) - signatureIndex = body.indexOf('
') + signatureIndex = body.indexOf('') body = body[...signatureIndex] if signatureIndex isnt -1 return body.indexOf("attach") >= 0 diff --git a/internal_packages/composer/spec/composer-view-spec.cjsx b/internal_packages/composer/spec/composer-view-spec.cjsx index 1ffb2cef1..e08b25df0 100644 --- a/internal_packages/composer/spec/composer-view-spec.cjsx +++ b/internal_packages/composer/spec/composer-view-spec.cjsx @@ -194,7 +194,7 @@ describe "ComposerView", -> describe "when sending a forwarded message message", -> beforeEach -> @fwdBody = """

- Begin forwarded message: + ---------- Forwarded message ---------

From: Evan Morikawa <evan@evanmorikawa.com>
Subject: Test Forward Message 1
Date: Sep 3 2015, at 12:14 pm
To: Evan Morikawa <evan@nylas.com>

diff --git a/internal_packages/quick-schedule/lib/main.cjsx b/internal_packages/quick-schedule/lib/main.cjsx index 2b6f29d77..42617ea49 100644 --- a/internal_packages/quick-schedule/lib/main.cjsx +++ b/internal_packages/quick-schedule/lib/main.cjsx @@ -135,7 +135,7 @@ module.exports = text = QuotedHTMLTransformer.removeQuotedHTML(draftHtml) # Check for an N1 signature and split that off - sigIndex = text.indexOf('
') + sigIndex = text.indexOf('') beforeSig = if sigIndex>-1 then text.slice(0,sigIndex) else text afterSig = text.slice(beforeSig.length) diff --git a/spec/stores/draft-store-spec.coffee b/spec/stores/draft-store-spec.coffee index 25f818f9d..a0e67ab2e 100644 --- a/spec/stores/draft-store-spec.coffee +++ b/spec/stores/draft-store-spec.coffee @@ -327,8 +327,9 @@ describe "DraftStore", -> runs -> @model = DatabaseTransaction.prototype.persistModel.mostRecentCall.args[0] - it "should include quoted text", -> - expect(@model.body.indexOf('blockquote') > 0).toBe(true) + it "should include quoted text, but in a div rather than a blockquote", -> + expect(@model.body.indexOf('gmail_quote') > 0).toBe(true) + expect(@model.body.indexOf('blockquote') > 0).toBe(false) expect(@model.body.indexOf(fakeMessage1.body) > 0).toBe(true) it "should not address the message to anyone", -> @@ -497,12 +498,12 @@ describe "DraftStore", -> expect(model.body.indexOf('gmail_quote') > 0).toBe(true) expect(model.body.indexOf('Fake Message 1') > 0).toBe(true) - it "should include the `Begin forwarded message:` line", -> + it "should include the `---------- Forwarded message ---------:` line", -> @_callNewMessageWithContext {threadId: fakeThread.id} , (thread, message) -> forwardMessage: fakeMessage1 , (model) -> - expect(model.body.indexOf('Begin forwarded message') > 0).toBe(true) + expect(model.body.indexOf('---------- Forwarded message ---------') > 0).toBe(true) it "should make the subject the subject of the message, not the thread", -> fakeMessage1.subject = "OLD SUBJECT" diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index 8e9fd245a..e3856c461 100644 --- a/src/flux/stores/draft-store.coffee +++ b/src/flux/stores/draft-store.coffee @@ -337,12 +337,15 @@ class DraftStore replyToMessage = attributes.replyToMessage @_prepareBodyForQuoting(replyToMessage.body).then (body) -> return """ -

+

#{DOMUtils.escapeHTMLCharacters(replyToMessage.replyAttributionLine())}
- #{body} -
""" +
+ #{body} +
+
""" + else if attributes.forwardMessage forwardMessage = attributes.forwardMessage contactsAsHtml = (cs) -> @@ -356,14 +359,13 @@ class DraftStore fields.push("BCC: #{contactsAsHtml(forwardMessage.bcc)}") if forwardMessage.bcc.length > 0 @_prepareBodyForQuoting(forwardMessage.body).then (body) -> return """ -

- Begin forwarded message: +

+ ---------- Forwarded message ---------

#{fields.join('
')}

#{body} -
""" +
""" else return Promise.resolve("") # Eventually we'll want a nicer solution for inline attachments diff --git a/src/services/quoted-html-transformer.coffee b/src/services/quoted-html-transformer.coffee index fff6b68c6..499652da3 100644 --- a/src/services/quoted-html-transformer.coffee +++ b/src/services/quoted-html-transformer.coffee @@ -181,8 +181,10 @@ class QuotedHTMLTransformer el.removeAttribute("data-nylas-quoted-text-original-display") _findGmailQuotes: (doc) -> - # There can sometimes be `div.gmail_quote` that are false positives. - return Array::slice.call(doc.querySelectorAll('blockquote.gmail_quote')) + # Gmail creates both div.gmail_quote and blockquote.gmail_quote. The div + # version marks text but does not cause indentation, but both should be + # considered quoted text. + return Array::slice.call(doc.querySelectorAll('.gmail_quote')) _findOffice365Quotes: (doc) -> elements = doc.querySelectorAll('#divRplyFwdMsg, #OLK_SRC_BODY_SECTION')