From b761afe96093e40c34621acff9d86e3be67c2dd9 Mon Sep 17 00:00:00 2001 From: Evan Morikawa Date: Mon, 14 Sep 2015 16:18:55 -0400 Subject: [PATCH] fix(reply): better coverge for reply-all participants Summary: Fixes: T3550 Fixes: T3504 Test Plan: many of them Reviewers: dillon, bengotow Reviewed By: dillon, bengotow Subscribers: mg Differential Revision: https://phab.nylas.com/D2017 --- .../message-list/lib/message-controls.cjsx | 11 +- .../message-list/lib/message-list.cjsx | 7 +- .../message-list/spec/message-list-spec.cjsx | 18 +- spec-nylas/models/message-spec.coffee | 333 ++++++++++++++++-- spec/spec-helper.coffee | 8 +- src/flux/models/contact.coffee | 3 +- src/flux/models/message.coffee | 25 +- src/flux/models/utils.coffee | 17 + src/regexp-utils.coffee | 6 +- 9 files changed, 380 insertions(+), 48 deletions(-) diff --git a/internal_packages/message-list/lib/message-controls.cjsx b/internal_packages/message-list/lib/message-controls.cjsx index 38188c35f..d701a9d19 100644 --- a/internal_packages/message-list/lib/message-controls.cjsx +++ b/internal_packages/message-list/lib/message-controls.cjsx @@ -38,11 +38,14 @@ class MessageControls extends React.Component image: 'ic-dropdown-forward.png' select: @_onForward - defaultReplyType = atom.config.get('core.sending.defaultReplyType') - if @props.message.canReplyAll() and defaultReplyType is 'reply-all' - return [replyAll, reply, forward] + if @props.message.canReplyAll() + defaultReplyType = atom.config.get('core.sending.defaultReplyType') + if defaultReplyType is 'reply-all' + return [replyAll, reply, forward] + else + return [reply, replyAll, forward] else - return [reply, replyAll, forward] + return [reply, forward] _dropdownMenu: (items) -> itemContent = (item) -> diff --git a/internal_packages/message-list/lib/message-list.cjsx b/internal_packages/message-list/lib/message-list.cjsx index cae8fbd1b..11f49c218 100755 --- a/internal_packages/message-list/lib/message-list.cjsx +++ b/internal_packages/message-list/lib/message-list.cjsx @@ -248,8 +248,11 @@ class MessageList extends React.Component lastMsg = _.last(_.filter((@state.messages ? []), (m) -> not m.draft)) return 'reply' unless lastMsg - if lastMsg.canReplyAll() and defaultReplyType is 'reply-all' - return 'reply-all' + if lastMsg.canReplyAll() + if defaultReplyType is 'reply-all' + return 'reply-all' + else + return 'reply' else return 'reply' diff --git a/internal_packages/message-list/spec/message-list-spec.cjsx b/internal_packages/message-list/spec/message-list-spec.cjsx index 82fb24606..0b1c8d491 100644 --- a/internal_packages/message-list/spec/message-list-spec.cjsx +++ b/internal_packages/message-list/spec/message-list-spec.cjsx @@ -33,9 +33,11 @@ MessageItemContainer = proxyquire("../lib/message-item-container", { MessageList = proxyquire '../lib/message-list', "./message-item-container": MessageItemContainer +# User_1 needs to be "me" so that when we calculate who we should reply +# to, it properly matches the AccountStore user_1 = new Contact - name: "User One" - email: "user1@nylas.com" + name: TEST_ACCOUNT_NAME + email: TEST_ACCOUNT_EMAIL user_2 = new Contact name: "User Two" email: "user2@nylas.com" @@ -251,7 +253,8 @@ describe "MessageList", -> cs = TestUtils.scryRenderedDOMComponentsWithClass(@messageList, "footer-reply-area") expect(cs.length).toBe 1 - it "prompts for a reply-all when there's more then one participant", -> + it "prompts for a reply-all when there's more than one participant and the default is reply-all", -> + spyOn(atom.config, "get").andReturn "reply-all" MessageStore._items = [m5, m3] MessageStore._thread = test_thread MessageStore.trigger() @@ -259,6 +262,15 @@ describe "MessageList", -> cs = TestUtils.scryRenderedDOMComponentsWithClass(@messageList, "footer-reply-area") expect(cs.length).toBe 1 + it "prompts for a reply-all when there's more than one participant and the default is reply", -> + spyOn(atom.config, "get").andReturn "reply" + MessageStore._items = [m5, m3] + MessageStore._thread = test_thread + MessageStore.trigger() + expect(@messageList._replyType()).toBe "reply" + cs = TestUtils.scryRenderedDOMComponentsWithClass(@messageList, "footer-reply-area") + expect(cs.length).toBe 1 + it "hides the reply type if the last message is a draft", -> MessageStore._items = [m5, m3, draftMessages[0]] MessageStore._thread = test_thread diff --git a/spec-nylas/models/message-spec.coffee b/spec-nylas/models/message-spec.coffee index 429e93575..22014c44a 100644 --- a/spec-nylas/models/message-spec.coffee +++ b/spec-nylas/models/message-spec.coffee @@ -1,17 +1,28 @@ +Utils = require "../../src/flux/models/utils" Message = require "../../src/flux/models/message" +Contact = require "../../src/flux/models/contact" -contact_1 = - name: "Contact One" - email: "contact1@nylas.com" -contact_2 = - name: "Contact Two" - email: "contact2@nylas.com" -contact_3 = - name: "" - email: "contact3@nylas.com" -contact_4 = - name: "Contact Four" - email: "" +evan = new Contact + name: "Evan Morikawa" + email: "evan@nylas.com" +ben = new Contact + name: "Ben Gotow" + email: "ben@nylas.com" +team = new Contact + name: "Nylas Team" + email: "team@nylas.com" +edgehill = new Contact + name: "Edgehill" + email: "edgehill@nylas.com" +noEmail = new Contact + name: "Edgehill" + email: null +me = new Contact + name: TEST_ACCOUNT_NAME + email: TEST_ACCOUNT_EMAIL +almost_me = new Contact + name: TEST_ACCOUNT_NAME + email: "tester+12345@nylas.com" describe "Message", -> it "correctly aggregates participants", -> @@ -22,31 +33,303 @@ describe "Message", -> expect(m1.participants().length).toBe 0 m2 = new Message - to: [contact_1] + to: [evan] cc: [] bcc: [] - from: [contact_2] + from: [ben] expect(m2.participants().length).toBe 2 m3 = new Message - to: [contact_1] - cc: [contact_1] - bcc: [contact_1] - from: [contact_1] + to: [evan] + cc: [evan] + bcc: [evan] + from: [evan] expect(m3.participants().length).toBe 1 m4 = new Message - to: [contact_1] - cc: [contact_2, contact_3, contact_4] - bcc: [contact_3] - from: [contact_3] + to: [evan] + cc: [ben, team, noEmail] + bcc: [team] + from: [team] # because contact 4 has no email expect(m4.participants().length).toBe 3 m5 = new Message - to: [contact_1] + to: [evan] cc: [] - bcc: [contact_3] - from: [contact_2] + bcc: [team] + from: [ben] # because we exclude bccs expect(m5.participants().length).toBe 2 + + describe "participant replies", -> + cases = [ + # Basic cases + { + msg: new Message + from: [evan] + to: [me] + cc: [] + bcc: [] + expected: + to: [evan] + cc: [] + } + { + msg: new Message + from: [evan] + to: [me] + cc: [ben] + bcc: [] + expected: + to: [evan] + cc: [ben] + } + { + msg: new Message + from: [evan] + to: [ben] + cc: [me] + bcc: [] + expected: + to: [evan] + cc: [ben] + } + { + msg: new Message + from: [evan] + to: [me] + cc: [ben, team, evan] + bcc: [] + expected: + to: [evan] + cc: [ben, team] + } + { + msg: new Message + from: [evan] + to: [me, ben, evan, ben, ben, evan] + cc: [] + bcc: [] + expected: + to: [evan] + cc: [ben] + } + { + msg: new Message + from: [evan] + to: [me, ben] + cc: [team, edgehill] + bcc: [evan, me, ben] + expected: + to: [evan] + cc: [ben, team, edgehill] + } + + # From me (replying to a message I just sent) + { + msg: new Message + from: [me] + to: [me] + cc: [] + bcc: [] + expected: + to: [me] + cc: [] + } + { + msg: new Message + from: [me] + to: [ben] + cc: [] + bcc: [] + expected: + to: [ben] + cc: [] + } + { + msg: new Message + from: [me] + to: [ben, team, ben] + cc: [edgehill] + bcc: [] + expected: + to: [ben, team] + cc: [edgehill] + } + { + msg: new Message + from: [me] + to: [ben, team, ben] + cc: [edgehill] + bcc: [] + expected: + to: [ben, team] + cc: [edgehill] + } + # From me in cases my similar alias is used + { + msg: new Message + from: [me] + to: [almost_me] + cc: [ben] + bcc: [] + expected: + to: [almost_me] + cc: [ben] + } + { + msg: new Message + from: [me] + to: [me, almost_me, me] + cc: [ben, almost_me, me, me, ben, ben] + bcc: [] + expected: + to: [me] + cc: [ben] + } + { + msg: new Message + from: [almost_me] + to: [me] + cc: [ben] + bcc: [] + expected: + to: [me] + cc: [ben] + } + { + msg: new Message + from: [almost_me] + to: [almost_me] + cc: [ben] + bcc: [] + expected: + to: [almost_me] + cc: [ben] + } + + # Cases when I'm on email lists + { + msg: new Message + from: [evan] + to: [team] + cc: [] + bcc: [] + expected: + to: [evan] + cc: [team] + } + { + msg: new Message + from: [evan] + to: [team] + cc: [ben, edgehill] + bcc: [] + expected: + to: [evan] + cc: [team, ben, edgehill] + } + { + msg: new Message + from: [evan] + to: [team] + cc: [me] + bcc: [] + expected: + to: [evan] + cc: [team] + } + { + msg: new Message + from: [evan] + to: [team, me] + cc: [ben] + bcc: [] + expected: + to: [evan] + cc: [team, ben] + } + + # Cases when I'm bcc'd + { + msg: new Message + from: [evan] + to: [] + cc: [] + bcc: [me] + expected: + to: [evan] + cc: [] + } + { + msg: new Message + from: [evan] + to: [ben] + cc: [] + bcc: [me] + expected: + to: [evan] + cc: [ben] + } + { + msg: new Message + from: [evan] + to: [ben] + cc: [team, edgehill] + bcc: [me] + expected: + to: [evan] + cc: [ben, team, edgehill] + } + + # Cases my similar alias is used + { + msg: new Message + from: [evan] + to: [almost_me] + cc: [] + bcc: [] + expected: + to: [evan] + cc: [] + } + { + msg: new Message + from: [evan] + to: [ben] + cc: [almost_me] + bcc: [] + expected: + to: [evan] + cc: [ben] + } + { + msg: new Message + from: [evan] + to: [ben] + cc: [] + bcc: [almost_me] + expected: + to: [evan] + cc: [ben] + } + ] + + itString = (prefix, msg) -> + return "#{prefix} from: #{msg.from.map( (c) -> c.email).join(', ')} | to: #{msg.to.map( (c) -> c.email).join(', ')} | cc: #{msg.cc.map( (c) -> c.email).join(', ')} | bcc: #{msg.bcc.map( (c) -> c.email).join(', ')}" + + it "thinks me and almost_me are equivalent", -> + expect(Utils.emailIsEquivalent(me.email, almost_me.email)).toBe true + expect(Utils.emailIsEquivalent(ben.email, me.email)).toBe false + + cases.forEach ({msg, expected}) -> + it itString("Reply All:", msg), -> + expect(msg.participantsForReplyAll()).toEqual expected + + it itString("Reply:", msg), -> + {to, cc} = msg.participantsForReply() + expect(to).toEqual expected.to + expect(cc).toEqual [] + + describe "participantsForReplyAll", -> diff --git a/spec/spec-helper.coffee b/spec/spec-helper.coffee index 415bc9977..1eb84b3cf 100644 --- a/spec/spec-helper.coffee +++ b/spec/spec-helper.coffee @@ -108,6 +108,8 @@ Promise.setScheduler (fn) -> # So it passes the Utils.isTempId test window.TEST_ACCOUNT_CLIENT_ID = "local-test-account-client-id" window.TEST_ACCOUNT_ID = "test-account-server-id" +window.TEST_ACCOUNT_EMAIL = "tester@nylas.com" +window.TEST_ACCOUNT_NAME = "Nylas Test" beforeEach -> atom.testOrganizationUnit = null @@ -152,16 +154,16 @@ beforeEach -> # Log in a fake user spyOn(AccountStore, 'current').andCallFake -> new Account - name: "Nylas Test" + name: TEST_ACCOUNT_NAME provider: "gmail" - emailAddress: 'tester@nylas.com' + emailAddress: TEST_ACCOUNT_EMAIL organizationUnit: atom.testOrganizationUnit clientId: TEST_ACCOUNT_CLIENT_ID serverId: TEST_ACCOUNT_ID usesLabels: -> atom.testOrganizationUnit is "label" usesFolders: -> atom.testOrganizationUnit is "folder" me: -> - new Contact(email: 'tester@nylas.com', name: 'Ben Tester') + new Contact(email: TEST_ACCOUNT_EMAIL, name: TEST_ACCOUNT_NAME) # reset config before each spec; don't load or save from/to `config.json` spyOn(Config::, 'load') diff --git a/src/flux/models/contact.coffee b/src/flux/models/contact.coffee index ad1d81ece..901478161 100644 --- a/src/flux/models/contact.coffee +++ b/src/flux/models/contact.coffee @@ -1,4 +1,5 @@ Model = require './model' +Utils = require './utils' Attributes = require '../attributes' _ = require 'underscore' @@ -81,7 +82,7 @@ class Contact extends Model # the account email, since it is case-insensitive and future-proof. isMe: -> AccountStore = require '../stores/account-store' - @email.toLowerCase() is AccountStore.current()?.emailAddress.toLowerCase() + Utils.emailIsEquivalent(@email, AccountStore.current()?.emailAddress) # Returns a {String} display name. # - "You" if the contact is the current user diff --git a/src/flux/models/message.coffee b/src/flux/models/message.coffee index 1ece23203..103ffd05c 100644 --- a/src/flux/models/message.coffee +++ b/src/flux/models/message.coffee @@ -3,6 +3,7 @@ moment = require 'moment' File = require './file' Label = require './label' +Utils = require './utils' Folder = require './folder' Model = require './model' Event = require './event' @@ -196,7 +197,8 @@ class Message extends Model return @ canReplyAll: -> - @cc.length > 0 or @to.length > 1 + {to, cc} = @participantsForReplyAll() + to.length > 1 or cc.length > 0 # Public: Returns a set of uniqued message participants by combining the # `to`, `cc`, and `from` fields. @@ -215,22 +217,27 @@ class Message extends Model to = [] cc = [] + excluded = @from.map (c) -> Utils.toEquivalentEmailForm(c.email) + excluded.push(Utils.toEquivalentEmailForm(AccountStore.current().emailAddress)) + + filterCc = (cc) -> + return _.filter cc, (p) -> + !_.contains(excluded, Utils.toEquivalentEmailForm(p.email)) + if @isFromMe() to = @to - cc = @cc + cc = filterCc(@cc) else - excluded = @from.map (c) -> c.email - excluded.push(AccountStore.current().emailAddress) if @replyTo.length to = @replyTo else to = @from - cc = [].concat(@cc, @to) - cc = _.filter cc, (p) -> !_.contains(excluded, p.email) + cc = [].concat(@to, @cc) + cc = filterCc(cc) - to = _.uniq to, (p) -> p.email.toLowerCase().trim() - cc = _.uniq cc, (p) -> p.email.toLowerCase().trim() + to = _.uniq to, (p) -> Utils.toEquivalentEmailForm(p.email) + cc = _.uniq cc, (p) -> Utils.toEquivalentEmailForm(p.email) {to, cc} # Public: Returns a hash with `to` and `cc` keys for authoring a new draft in @@ -247,7 +254,7 @@ class Message extends Model else to = @from - to = _.uniq to, (p) -> p.email.toLowerCase().trim() + to = _.uniq to, (p) -> Utils.toEquivalentEmailForm(p.email) {to, cc} # Public: Returns an {Array} of {File} IDs diff --git a/src/flux/models/utils.coffee b/src/flux/models/utils.coffee index 8f343ad57..cb96f2d75 100644 --- a/src/flux/models/utils.coffee +++ b/src/flux/models/utils.coffee @@ -206,6 +206,23 @@ Utils = domain = _.last(email.toLowerCase().trim().split("@")) return (Utils.commonDomains[domain] ? false) + # This looks for and removes plus-ing, it taks a VERY liberal approach + # to match an email address. We'd rather let false positives through. + toEquivalentEmailForm: (email) -> + # https://regex101.com/r/iS7kD5/1 + localPart1 = /([^+]+?)[+@].*/gi.exec(email)?[1] ? "" + + # https://regex101.com/r/iS7kD5/2 + domainPart1 = /@(.+)/gi.exec(email)?[1] ? "" + + email = "#{localPart1}#{domainPart1}".trim().toLowerCase() + return email + + emailIsEquivalent: (email1="", email2="") -> + email1 = Utils.toEquivalentEmailForm(email1) + email2 = Utils.toEquivalentEmailForm(email2) + return email1 is email2 + rectVisibleInRect: (r1, r2) -> return !(r2.left > r1.right || r2.right < r1.left || r2.top > r1.bottom || r2.bottom < r1.top) diff --git a/src/regexp-utils.coffee b/src/regexp-utils.coffee index 0f043c4f3..58a370d8a 100644 --- a/src/regexp-utils.coffee +++ b/src/regexp-utils.coffee @@ -7,7 +7,11 @@ RegExpUtils = # It's also imporant we return a fresh copy of the RegExp every time. A # javascript regex is stateful and multiple functions using this method # will cause unexpected behavior! - emailRegex: -> new RegExp(/([a-z.A-Z0-9%+=_-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4})/g) + # + # See http://tools.ietf.org/html/rfc5322#section-3.4 and + # https://tools.ietf.org/html/rfc6531 and + # https://en.wikipedia.org/wiki/Email_address#Local_part + emailRegex: -> new RegExp(/([a-z.A-Z0-9%#_~$&*+;=:-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4})/g) # https://regex101.com/r/zG7aW4/3 imageTagRegex: -> /]*src="([^"]*)"[^>]*>/g