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

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()
-    if @props.message.canReplyAll() and defaultReplyType is 'reply-all'
+      defaultReplyType = atom.config.get('core.sending.defaultReplyType')
-      return [replyAll, reply, forward]
+      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) ->

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'
+    if lastMsg.canReplyAll()
-      return 'reply-all'
+      if defaultReplyType is 'reply-all'
+        return 'reply-all'
+      else
+        return 'reply'
     else
       return 'reply'
 

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"
+  name: TEST_ACCOUNT_NAME
-  email: "user1@nylas.com"
+  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

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 =
+evan = new Contact
-  name: "Contact One"
+  name: "Evan Morikawa"
-  email: "contact1@nylas.com"
+  email: "evan@nylas.com"
-contact_2 =
+ben = new Contact
-  name: "Contact Two"
+  name: "Ben Gotow"
-  email: "contact2@nylas.com"
+  email: "ben@nylas.com"
-contact_3 =
+team = new Contact
-  name: ""
+  name: "Nylas Team"
-  email: "contact3@nylas.com"
+  email: "team@nylas.com"
-contact_4 =
+edgehill = new Contact
-  name: "Contact Four"
+  name: "Edgehill"
-  email: ""
+  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]
+      to: [evan]
-      cc: [contact_1]
+      cc: [evan]
-      bcc: [contact_1]
+      bcc: [evan]
-      from: [contact_1]
+      from: [evan]
     expect(m3.participants().length).toBe 1
 
     m4 = new Message
-      to: [contact_1]
+      to: [evan]
-      cc: [contact_2, contact_3, contact_4]
+      cc: [ben, team, noEmail]
-      bcc: [contact_3]
+      bcc: [team]
-      from: [contact_3]
+      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]
+      bcc: [team]
-      from: [contact_2]
+      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", ->

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')

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

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 = [].concat(@to, @cc)
-      cc = _.filter cc, (p) -> !_.contains(excluded, p.email)
+      cc = filterCc(cc)
 
-    to = _.uniq to, (p) -> p.email.toLowerCase().trim()
+    to = _.uniq to, (p) -> Utils.toEquivalentEmailForm(p.email)
-    cc = _.uniq cc, (p) -> p.email.toLowerCase().trim()
+    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

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)
 

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: -> /<img\s+[^>]*src="([^"]*)"[^>]*>/g