fix(reply-behavior): Reply keyboard shortcuts update existing draft rather than creating new ones

Summary:
When bcc or cc is added, the ComposerView should show the fields. We should never hide a populated field!

Move the logic for determining message reply / replyTo participant sets into the message object. This seems OK because the functions don't modify the message and deal entirely with message attributes

Fix miscelaneous scrollbar issue

The MessageList does not always fire a composeReply action in response to keyboard shortcuts and can update the existing draft instead

There are a couple goals here:
- If you have an existing draft, command-R and command-shift-R should never create a new draft, they should just update and focus the existing draft.
- If you actually press the Reply button on the message item it should still create a new draft. It's still possible to have two.
- If you press Command-R and add a participant, and then press Command-Shift-R, it should add participants but not blow away the one you've added manually.
- If you press Command-Shift-R and then Command-R, it should remove the participants that were part of the "reply all", but leave other participants you've added untouched.

Test Plan: Run new tests!

Reviewers: evan

Reviewed By: evan

Maniphest Tasks: T2140

Differential Revision: https://phab.nylas.com/D1734

internal_packages/composer/lib/composer-view.cjsx

@@ -443,6 +443,12 @@ class ComposerView extends React.Component
         showQuotedText: @isForwardedMessage()
         populated: true
 
+    # It's possible for another part of the application to manipulate the draft
+    # we're displaying. If they've added a CC or BCC, make sure we show those fields.
+    # We should never be hiding the field if it's populated.
+    state.showcc = true if not _.isEmpty(draft.cc)
+    state.showbcc = true if not _.isEmpty(draft.bcc)
+
     @setState(state)
 
   _shouldShowSubject: =>
@@ -575,7 +581,9 @@ class ComposerView extends React.Component
     @focus('textFieldCc')
 
   _onSendingStateChanged: =>
-    @setState isSending: DraftStore.isSendingDraft(@props.localId)
+    isSending = DraftStore.isSendingDraft(@props.localId)
+    if isSending isnt @state.isSending
+      @setState({isSending})
 
   _onEmptyCc: =>
     @setState showcc: false

internal_packages/composer/spec/composer-view-spec.cjsx

@@ -246,7 +246,7 @@ describe "populated composer", ->
         advanceClock(1000)
         expect(@composer.refs['contentBody'].focus).toHaveBeenCalled()
 
-  describe "when emptying cc fields", ->
+  describe "when emptying cc/bcc fields", ->
 
     it "focuses on to when bcc is emptied and there's no cc field", ->
       useDraft.call(@, bcc: [u1])
@@ -255,6 +255,7 @@ describe "populated composer", ->
       spyOn(@composer.refs['textFieldBcc'], 'focus')
 
       bcc = ReactTestUtils.scryRenderedComponentsWithTypeAndProps(@composer, ParticipantsTextField, field: "bcc")[0]
+      @draft.bcc = []
       bcc.props.onEmptied()
 
       expect(@composer.state.showbcc).toBe false
@@ -271,7 +272,7 @@ describe "populated composer", ->
       spyOn(@composer.refs['textFieldBcc'], 'focus')
 
       bcc = ReactTestUtils.scryRenderedComponentsWithTypeAndProps(@composer, ParticipantsTextField, field: "bcc")[0]
-
+      @draft.bcc = []
       bcc.props.onEmptied()
       expect(@composer.state.showbcc).toBe false
       advanceClock(1000)
@@ -287,14 +288,34 @@ describe "populated composer", ->
       spyOn(@composer.refs['textFieldBcc'], 'focus')
 
       cc = ReactTestUtils.scryRenderedComponentsWithTypeAndProps(@composer, ParticipantsTextField, field: "cc")[0]
+      @draft.cc = []
       cc.props.onEmptied()
-
       expect(@composer.state.showcc).toBe false
       advanceClock(1000)
       expect(@composer.refs['textFieldTo'].focus).toHaveBeenCalled()
       expect(@composer.refs['textFieldCc']).not.toBeDefined()
       expect(@composer.refs['textFieldBcc'].focus).not.toHaveBeenCalled()
 
+  describe "when participants are added during a draft update", ->
+    it "shows the cc fields and bcc fields to ensure participants are never hidden", ->
+      useDraft.call(@, cc: [], bcc: [])
+      makeComposer.call(@)
+      expect(@composer.state.showbcc).toBe(false)
+      expect(@composer.state.showcc).toBe(false)
+
+      # Simulate a change event fired by the DraftStoreProxy
+      @draft.cc = [u1]
+      @composer._onDraftChanged()
+
+      expect(@composer.state.showbcc).toBe(false)
+      expect(@composer.state.showcc).toBe(true)
+
+      # Simulate a change event fired by the DraftStoreProxy
+      @draft.bcc = [u2]
+      @composer._onDraftChanged()
+      expect(@composer.state.showbcc).toBe(true)
+      expect(@composer.state.showcc).toBe(true)
+
   describe "When sending a message", ->
     beforeEach ->
       spyOn(atom, "isMainWindow").andReturn true

internal_packages/message-list/lib/message-list.cjsx

@@ -4,7 +4,10 @@ classNames = require 'classnames'
 MessageItem = require "./message-item"
 {Utils,
  Actions,
+ Message,
+ DraftStore,
  MessageStore,
+ DatabaseStore,
  ComponentRegistry,
  AddRemoveTagsTask} = require("nylas-exports")
 
@@ -69,8 +72,8 @@ class MessageList extends React.Component
 
     commands = _.extend {},
       'core:star-item': => @_onStar()
-      'application:reply': => @_onReply()
-      'application:reply-all': => @_onReplyAll()
+      'application:reply': => @_createReplyOrUpdateExistingDraft('reply')
+      'application:reply-all': => @_createReplyOrUpdateExistingDraft('reply-all')
       'application:forward': => @_onForward()
 
     @command_unsubscriber = atom.commands.add('body', commands)
@@ -103,7 +106,7 @@ class MessageList extends React.Component
       if newMessageIds.length > 0
         @_prepareContentForDisplay()
       else if newDraftIds.length > 0
-        @_focusDraft(@refs["composerItem-#{newDraftIds[0]}"])
+        @_focusDraft(@_getDraftElement(newDraftIds[0]))
         @_prepareContentForDisplay()
 
   _newDraftIds: (prevState) =>
@@ -116,9 +119,68 @@ class MessageList extends React.Component
     newMessageIds = _.map(_.reject((@state.messages ? []), (m) -> m.draft), (m) -> m.id)
     return _.difference(newMessageIds, oldMessageIds) ? []
 
+  _getDraftElement: (draftId) =>
+    @refs["composerItem-#{draftId}"]
+
   _focusDraft: (draftElement) =>
     draftElement.focus()
 
+  _createReplyOrUpdateExistingDraft: (type) =>
+    unless type in ['reply', 'reply-all']
+      throw new Error("_createReplyOrUpdateExistingDraft called with #{type}, not reply or reply-all")
+    return unless @state.currentThread
+
+    last = _.last(@state.messages ? [])
+
+    # If the last message on the thread is already a draft, fetch the message it's
+    # in reply to and the draft session and change the participants.
+    if last.draft is true
+      data =
+        session: DraftStore.sessionForLocalId(@state.messageLocalIds[last.id])
+        replyToMessage: Promise.resolve(@state.messages[@state.messages.length - 2])
+
+      if last.replyToMessageId
+        msg = _.findWhere(@state.messages, {id: last.replyToMessageId})
+        if msg
+          data.replyToMessage = Promise.resolve(msg)
+        else
+          data.replyToMessage = DatabaseStore.find(Message, last.replyToMessageId)
+
+      Promise.props(data).then ({session, replyToMessage}) =>
+        return unless replyToMessage and session
+        draft = session.draft()
+        updated = {to: [].concat(draft.to), cc: [].concat(draft.cc)}
+
+        replySet = replyToMessage.participantsForReply()
+        replyAllSet = replyToMessage.participantsForReplyAll()
+
+        if type is 'reply'
+          targetSet = replySet
+
+          # Remove participants present in the reply-all set and not the reply set
+          for key in ['to', 'cc']
+            updated[key] = _.reject updated[key], (contact) ->
+              inReplySet = _.findWhere(replySet[key], {email: contact.email})
+              inReplyAllSet = _.findWhere(replyAllSet[key], {email: contact.email})
+              return inReplyAllSet and not inReplySet
+        else
+          # Add participants present in the reply-all set and not on the draft
+          # Switching to reply-all shouldn't really ever remove anyone.
+          targetSet = replyAllSet
+
+        for key in ['to', 'cc']
+          for contact in targetSet[key]
+            updated[key].push(contact) unless _.findWhere(updated[key], {email: contact.email})
+
+        session.changes.add(updated)
+        @_focusDraft(@_getDraftElement(last.id))
+
+    else
+      if type is 'reply'
+        Actions.composeReply(thread: @state.currentThread, message: last)
+      else
+        Actions.composeReplyAll(thread: @state.currentThread, message: last)
+
   _onStar: =>
     return unless @state.currentThread
     if @state.currentThread.isStarred()
@@ -127,14 +189,6 @@ class MessageList extends React.Component
       task = new AddRemoveTagsTask(@state.currentThread, ['starred'], [])
     Actions.queueTask(task)
 
-  _onReply: =>
-    return unless @state.currentThread
-    Actions.composeReply(thread: @state.currentThread)
-
-  _onReplyAll: =>
-    return unless @state.currentThread
-    Actions.composeReplyAll(thread: @state.currentThread)
-
   _onForward: =>
     return unless @state.currentThread
     Actions.composeForward(thread: @state.currentThread)
@@ -191,17 +245,14 @@ class MessageList extends React.Component
   # Either returns "reply" or "reply-all"
   _replyType: =>
     lastMsg = _.last(_.filter((@state.messages ? []), (m) -> not m.draft))
-    return "reply" if lastMsg?.cc.length is 0 and lastMsg?.to.length is 1
-    return "reply-all"
+    if lastMsg?.cc.length is 0 and lastMsg?.to.length is 1
+      return "reply"
+    else
+      return "reply-all"
 
   _onClickReplyArea: =>
-    return unless @state.currentThread?.id
-    lastMsg = _.last(_.filter((@state.messages ? []), (m) -> not m.draft))
-
-    if @_replyType() is "reply-all"
-      Actions.composeReplyAll(thread: @state.currentThread, message: lastMsg)
-    else
-      Actions.composeReply(thread: @state.currentThread,  message: lastMsg)
+    return unless @state.currentThread
+    @_createReplyOrUpdateExistingDraft(@_replyType())
 
   # There may be a lot of iframes to load which may take an indeterminate
   # amount of time. As long as there is more content being painted onto
@@ -264,7 +315,7 @@ class MessageList extends React.Component
       if message.draft
         components.push <InjectedComponent matching={role:"Composer"}
                          exposedProps={ mode:"inline", localId:@state.messageLocalIds[message.id], onRequestScrollTo:@_onRequestScrollToComposer, threadId:@state.currentThread.id }
-                         ref="composerItem-#{message.id}"
+                         ref={"composerItem-#{message.id}"}
                          key={@state.messageLocalIds[message.id]}
                          className={className} />
       else
@@ -345,7 +396,7 @@ class MessageList extends React.Component
   # If messageId and location are defined, that means we want to scroll
   # smoothly to the top of a particular message.
   _onRequestScrollToComposer: ({messageId, location, selectionTop}={}) =>
-    composer = React.findDOMNode(@refs["composerItem-#{messageId}"])
+    composer = React.findDOMNode(@_getDraftElement(messageId))
     if selectionTop
       messageWrap = React.findDOMNode(@refs.messageWrap)
       wrapRect = messageWrap.getBoundingClientRect()
@@ -356,7 +407,6 @@ class MessageList extends React.Component
     else
       done = ->
       location ?= "bottom"
-      composer = React.findDOMNode(@refs["composerItem-#{messageId}"])
       @scrollToMessage(composer, done, location, 1)
 
   _makeRectVisible: (rect) ->

internal_packages/message-list/spec/message-list-spec.cjsx

@@ -11,6 +11,7 @@ TestUtils = React.addons.TestUtils
  Actions,
  Message,
  Namespace,
+ DraftStore,
  MessageStore,
  NamespaceStore,
  NylasTestUtils,
@@ -28,31 +29,25 @@ MessageList = proxyquire("../lib/message-list", {
 
 MessageParticipants = require "../lib/message-participants"
 
-me = new Namespace(
-  "name": "User One",
-  "email": "user1@nylas.com"
-  "provider": "inbox"
-)
+me = new Namespace
+  name: "User One",
+  emailAddress: "user1@nylas.com"
+  provider: "inbox"
 NamespaceStore._current = me
 
-user_headers =
-  id: null
-  object: null
-  namespace_id: null
-
-user_1 = _.extend _.clone(user_headers),
+user_1 = new Contact
   name: "User One"
   email: "user1@nylas.com"
-user_2 = _.extend _.clone(user_headers),
+user_2 = new Contact
   name: "User Two"
   email: "user2@nylas.com"
-user_3 = _.extend _.clone(user_headers),
+user_3 = new Contact
   name: "User Three"
   email: "user3@nylas.com"
-user_4 = _.extend _.clone(user_headers),
+user_4 = new Contact
   name: "User Four"
   email: "user4@nylas.com"
-user_5 = _.extend _.clone(user_headers),
+user_5 = new Contact
   name: "User Five"
   email: "user5@nylas.com"
 
@@ -270,6 +265,102 @@ describe "MessageList", ->
       cs = TestUtils.scryRenderedDOMComponentsWithClass(@messageList, "footer-reply-area")
       expect(cs.length).toBe 0
 
+  describe "reply behavior (_createReplyOrUpdateExistingDraft)", ->
+    beforeEach ->
+      @messageList.setState(currentThread: test_thread)
+
+    it "should throw an exception unless you provide `reply` or `reply-all`", ->
+      expect( => @messageList._createReplyOrUpdateExistingDraft('lala')).toThrow()
+
+    describe "when there is already a draft at the bottom of the thread", ->
+      beforeEach ->
+        @replyToMessage = new Message
+          id: "reply-id",
+          threadId: test_thread.id
+          date: new Date()
+        @draft = new Message
+          id: "666",
+          draft: true,
+          date: new Date()
+          replyToMessage: @replyToMessage.id
+
+        spyOn(@messageList, '_focusDraft')
+        spyOn(@replyToMessage, 'participantsForReplyAll').andCallFake ->
+          {to: [user_3], cc: [user_2, user_4] }
+        spyOn(@replyToMessage, 'participantsForReply').andCallFake ->
+          {to: [user_3], cc: [] }
+
+        MessageStore._items = [@replyToMessage, @draft]
+        MessageStore.trigger()
+        @messageList.setState(currentThread: test_thread)
+
+        @sessionStub =
+          draft: => @draft
+          changes:
+            add: jasmine.createSpy('session.changes.add')
+        spyOn(DraftStore, 'sessionForLocalId').andCallFake =>
+          Promise.resolve(@sessionStub)
+
+      it "should not fire a composer action", ->
+        spyOn(Actions, 'composeReplyAll')
+        @messageList._createReplyOrUpdateExistingDraft('reply-all')
+        advanceClock()
+        expect(Actions.composeReplyAll).not.toHaveBeenCalled()
+
+      it "should focus the existing draft", ->
+        @messageList._createReplyOrUpdateExistingDraft('reply-all')
+        advanceClock()
+        expect(@messageList._focusDraft).toHaveBeenCalled()
+
+      describe "when reply-all is passed", ->
+        it "should add missing participants", ->
+          @draft.to = [ user_3 ]
+          @draft.cc = []
+          @messageList._createReplyOrUpdateExistingDraft('reply-all')
+          advanceClock()
+          expect(@sessionStub.changes.add).toHaveBeenCalledWith({to: [user_3], cc: [user_2, user_4]})
+
+        it "should not blow away other participants who have been added to the draft", ->
+          user_random_a = new Contact(email: 'other-guy-a@gmail.com')
+          user_random_b = new Contact(email: 'other-guy-b@gmail.com')
+          @draft.to = [ user_3, user_random_a ]
+          @draft.cc = [ user_random_b ]
+          @messageList._createReplyOrUpdateExistingDraft('reply-all')
+          advanceClock()
+          expect(@sessionStub.changes.add).toHaveBeenCalledWith({to: [user_3, user_random_a], cc: [user_random_b, user_2, user_4]})
+
+      describe "when reply is passed", ->
+        it "should remove participants present in the reply-all participant set and not in the reply set", ->
+          @draft.to = [ user_3 ]
+          @draft.cc = [ user_2, user_4 ]
+          @messageList._createReplyOrUpdateExistingDraft('reply')
+          advanceClock()
+          expect(@sessionStub.changes.add).toHaveBeenCalledWith({to: [user_3], cc: []})
+
+        it "should not blow away other participants who have been added to the draft", ->
+          user_random_a = new Contact(email: 'other-guy-a@gmail.com')
+          user_random_b = new Contact(email: 'other-guy-b@gmail.com')
+          @draft.to = [ user_3, user_random_a ]
+          @draft.cc = [ user_2, user_4, user_random_b ]
+          @messageList._createReplyOrUpdateExistingDraft('reply')
+          advanceClock()
+          expect(@sessionStub.changes.add).toHaveBeenCalledWith({to: [user_3, user_random_a], cc: [user_random_b]})
+
+    describe "when there is not an existing draft at the bottom of the thread", ->
+      beforeEach ->
+        MessageStore._items = [m5, m3]
+        MessageStore.trigger()
+        @messageList.setState(currentThread: test_thread)
+
+      it "should fire a composer action based on the reply type", ->
+        spyOn(Actions, 'composeReplyAll')
+        @messageList._createReplyOrUpdateExistingDraft('reply-all')
+        expect(Actions.composeReplyAll).toHaveBeenCalledWith(thread: test_thread, message: m3)
+
+        spyOn(Actions, 'composeReply')
+        @messageList._createReplyOrUpdateExistingDraft('reply')
+        expect(Actions.composeReply).toHaveBeenCalledWith(thread: test_thread, message: m3)
+
   describe "Message minification", ->
     beforeEach ->
       @messageList.MINIFY_THRESHOLD = 3

src/components/scroll-region.cjsx

@@ -199,7 +199,8 @@ class ScrollRegion extends React.Component
 
   _setSharedState: (state) ->
     scrollbar = @props.getScrollbar?() ? @refs.scrollbar
-    scrollbar.setStateFromScrollRegion(state)
+    if scrollbar
+      scrollbar.setStateFromScrollRegion(state)
     @setState(state)
 
   _onScroll: (event) =>

src/flux/models/message.coffee

@@ -4,6 +4,7 @@ File = require './file'
 Model = require './model'
 Contact = require './contact'
 Attributes = require '../attributes'
+NamespaceStore = require '../stores/namespace-store'
 
 ###
 Public: The Message model represents a Message object served by the Nylas Platform API.
@@ -177,6 +178,40 @@ class Message extends Model
         participants["#{(contact?.email ? "").toLowerCase().trim()} #{(contact?.name ? "").toLowerCase().trim()}"] = contact if contact?
     return _.values(participants)
 
+  # Returns a hash with `to` and `cc` keys for authoring a new draft in response
+  # to this message. Takes `replyTo` and other important state into account.
+  participantsForReplyAll: ->
+    to = []
+    cc = []
+
+    if @from[0].email is NamespaceStore.current().emailAddress
+      to = @to
+      cc = @cc
+    else
+      excluded = @from.map (c) -> c.email
+      excluded.push(NamespaceStore.current().emailAddress)
+      if @replyTo.length
+        to = @replyTo
+      else
+        to = @from
+      cc = [].concat(@cc, @to).filter (p) ->
+        !_.contains(excluded, p.email)
+
+    {to, cc}
+
+  participantsForReply: ->
+    to = []
+    cc = []
+
+    if @from[0].email is NamespaceStore.current().emailAddress
+      to = @to
+    else if @replyTo.length
+      to = @replyTo
+    else
+      to = @from
+
+    {to, cc}
+
   # Public: Returns an {Array} of {File} IDs
   fileIds: ->
     _.map @files, (file) -> file.id

src/flux/stores/draft-store.coffee

@@ -179,18 +179,9 @@ class DraftStore
     return unless containsDraft
     @trigger(change)
 
-  _isMe: (contact={}) =>
-    contact.email is NamespaceStore.current().me().email
-
   _onComposeReply: (context) =>
     @_newMessageWithContext context, (thread, message) =>
-      if @_isMe(message.from[0])
-        to = message.to
-      else if message.replyTo.length
-        to = message.replyTo
-      else
-        to = message.from
-
+      {to, cc} = message.participantsForReply()
       return {
         replyToMessage: message
         to: to
@@ -198,19 +189,7 @@ class DraftStore
 
   _onComposeReplyAll: (context) =>
     @_newMessageWithContext context, (thread, message) =>
-      if @_isMe(message.from[0])
-        to = message.to
-        cc = message.cc
-      else
-        excluded = message.from.map (c) -> c.email
-        excluded.push(NamespaceStore.current().me().email)
-        if message.replyTo.length
-          to = message.replyTo
-        else
-          to = message.from
-        cc = [].concat(message.cc, message.to).filter (p) ->
-          !_.contains(excluded, p.email)
-
+      {to, cc} = message.participantsForReplyAll()
       return {
         replyToMessage: message
         to: to