From d0d96f90f5d0bd15e7136be0d193fc63575a2542 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Fri, 27 Mar 2015 16:37:13 -0700 Subject: [PATCH] feat(unread-notifications): Check that unread messages are in inbox Summary: When we get unread messages, grab their threads and then make sure they're in the inbox before notifying. Test Plan: See updated specs! Reviewers: evan Reviewed By: evan Differential Revision: https://review.inboxapp.com/D1361 --- .../stylesheets/activity-bar.less | 1 + .../unread-notifications/lib/main.coffee | 68 ++++++++++----- .../spec/main-spec.coffee | 84 +++++++++++++++---- src/flux/inbox-api.coffee | 9 +- 4 files changed, 123 insertions(+), 39 deletions(-) diff --git a/internal_packages/inbox-activity-bar/stylesheets/activity-bar.less b/internal_packages/inbox-activity-bar/stylesheets/activity-bar.less index 9de2bd8e7..800ec7196 100755 --- a/internal_packages/inbox-activity-bar/stylesheets/activity-bar.less +++ b/internal_packages/inbox-activity-bar/stylesheets/activity-bar.less @@ -31,6 +31,7 @@ input.filter { margin-left: 4px; padding: 2px; + color:black; vertical-align: middle; width: 400px; } diff --git a/internal_packages/unread-notifications/lib/main.coffee b/internal_packages/unread-notifications/lib/main.coffee index b48bdb574..fb747fba9 100644 --- a/internal_packages/unread-notifications/lib/main.coffee +++ b/internal_packages/unread-notifications/lib/main.coffee @@ -1,5 +1,5 @@ _ = require 'underscore-plus' -{Actions} = require 'inbox-exports' +{Actions, DatabaseStore, Thread} = require 'inbox-exports' module.exports = activate: -> @@ -12,26 +12,52 @@ module.exports = serialize: -> - _onNewMailReceived: (models) -> - # Display a notification if we've received new messages - newUnreadMessages = _.filter (models['message'] ? []), (msg) => - msg.unread is true and msg.date?.valueOf() >= @activationTime + _onNewMailReceived: (incoming) -> + new Promise (resolve, reject) => + incomingMessages = incoming['message'] ? [] + incomingThreads = incoming['thread'] ? [] - if newUnreadMessages.length is 1 - msg = newUnreadMessages.pop() - notif = new Notification(msg.from[0].displayName(), { - body: msg.subject - tag: 'unread-update' - }) - notif.onclick = -> - atom.displayWindow() - Actions.selectTagId("inbox") - Actions.selectThreadId(msg.threadId) + # Filter for new messages + newUnread = _.filter incomingMessages, (msg) => + msg.unread is true and msg.date?.valueOf() >= @activationTime - if newUnreadMessages.length > 1 - new Notification("#{newUnreadMessages.length} Unread Messages", { - tag: 'unread-update' - }) + return resolve() if newUnread.length is 0 - if newUnreadMessages.length > 0 - atom.playSound('new_mail.ogg') + # For each message, find it's corresponding thread. First, look to see + # if it's already in the `incoming` payload (sent via delta sync + # at the same time as the message.) If it's not, try loading it from + # the local cache. + # + # Note we may receive multiple unread msgs for the same thread. + # Using a map and ?= to avoid repeating work. + threads = {} + for msg in newUnread + threads[msg.threadId] ?= _.findWhere(incomingThreads, {id: msg.threadId}) + threads[msg.threadId] ?= DatabaseStore.find(Thread, msg.threadId) + + Promise.props(threads).then (threads) -> + + # Filter new unread messages to just the ones in the inbox + newUnreadInInbox = _.filter newUnread, (msg) -> + threads[msg.threadId]?.hasTagId('inbox') + + if newUnreadInInbox.length is 1 + msg = newUnreadInInbox.pop() + notif = new Notification(msg.from[0].displayName(), { + body: msg.subject + tag: 'unread-update' + }) + notif.onclick = -> + atom.displayWindow() + Actions.selectTagId("inbox") + Actions.selectThreadId(msg.threadId) + + if newUnreadInInbox.length > 1 + new Notification("#{newUnreadInInbox.length} Unread Messages", { + tag: 'unread-update' + }) + + if newUnreadInInbox.length > 0 + atom.playSound('new_mail.ogg') + + resolve() diff --git a/internal_packages/unread-notifications/spec/main-spec.coffee b/internal_packages/unread-notifications/spec/main-spec.coffee index d2d7272c6..259e4fde6 100644 --- a/internal_packages/unread-notifications/spec/main-spec.coffee +++ b/internal_packages/unread-notifications/spec/main-spec.coffee @@ -1,57 +1,111 @@ _ = require 'underscore-plus' +Promise = require 'bluebird' Contact = require '../../../src/flux/models/contact' Message = require '../../../src/flux/models/message' +Thread = require '../../../src/flux/models/thread' +Tag = require '../../../src/flux/models/tag' +DatabaseStore = require '../../../src/flux/stores/database-store' Main = require '../lib/main' describe "UnreadNotifications", -> beforeEach -> Main.activate() - spyOn(window, 'Notification').andCallFake -> + + @threadA = new Thread + tags: [new Tag(id: 'inbox')] + @threadB = new Thread + tags: [new Tag(id: 'archive')] + @msg1 = new Message unread: true date: new Date() from: [new Contact(name: 'Ben', email: 'ben@example.com')] subject: "Hello World" + threadId: "A" @msg2 = new Message unread: true date: new Date() from: [new Contact(name: 'Mark', email: 'mark@example.com')] subject: "Hello World 2" + threadId: "A" + @msgUnreadButArchived = new Message + unread: true + date: new Date() + from: [new Contact(name: 'Mark', email: 'mark@example.com')] + subject: "Hello World 2" + threadId: "B" @msgRead = new Message unread: false date: new Date() from: [new Contact(name: 'Mark', email: 'mark@example.com')] subject: "Hello World Read Already" + threadId: "A" @msgOld = new Message unread: true date: new Date(2000,1,1) from: [new Contact(name: 'Mark', email: 'mark@example.com')] subject: "Hello World Old" + threadId: "A" + + spyOn(DatabaseStore, 'find').andCallFake (klass, id) => + return Promise.resolve(@threadA) if id is 'A' + return Promise.resolve(@threadB) if id is 'B' + return Promise.resolve(null) + + spyOn(window, 'Notification').andCallFake -> + spyOn(Promise, 'props').andCallFake (dict) -> + dictOut = {} + for key, val of dict + if val.value? + dictOut[key] = val.value() + else + dictOut[key] = val + Promise.resolve(dictOut) afterEach -> Main.deactivate() it "should create a Notification if there is one unread message", -> - Main._onNewMailReceived({message: [@msgRead, @msg1]}) - expect(window.Notification).toHaveBeenCalled() - expect(window.Notification.mostRecentCall.args).toEqual([ 'Ben', { body : 'Hello World', tag : 'unread-update' } ]) + waitsForPromise => + Main._onNewMailReceived({message: [@msgRead, @msg1]}) + .then -> + expect(window.Notification).toHaveBeenCalled() + expect(window.Notification.mostRecentCall.args).toEqual([ 'Ben', { body : 'Hello World', tag : 'unread-update' } ]) it "should create a Notification if there is more than one unread message", -> - Main._onNewMailReceived({message: [@msg1, @msg2, @msgRead]}) - expect(window.Notification).toHaveBeenCalled() - expect(window.Notification.mostRecentCall.args).toEqual([ '2 Unread Messages', { tag : 'unread-update' } ]) + waitsForPromise => + Main._onNewMailReceived({message: [@msg1, @msg2, @msgRead]}) + .then -> + expect(window.Notification).toHaveBeenCalled() + expect(window.Notification.mostRecentCall.args).toEqual([ '2 Unread Messages', { tag : 'unread-update' } ]) it "should not create a Notification if there are no new messages", -> - Main._onNewMailReceived({message: []}) - expect(window.Notification).not.toHaveBeenCalled() - Main._onNewMailReceived({}) - expect(window.Notification).not.toHaveBeenCalled() + waitsForPromise -> + Main._onNewMailReceived({message: []}) + .then -> + expect(window.Notification).not.toHaveBeenCalled() + + waitsForPromise -> + Main._onNewMailReceived({}) + .then -> + expect(window.Notification).not.toHaveBeenCalled() + + it "should not notify about unread messages that are outside the inbox", -> + waitsForPromise => + Main._onNewMailReceived({message: [@msgUnreadButArchived, @msg1]}) + .then -> + expect(window.Notification).toHaveBeenCalled() + expect(window.Notification.mostRecentCall.args).toEqual([ 'Ben', { body : 'Hello World', tag : 'unread-update' } ]) it "should not create a Notification if the new messages are not unread", -> - Main._onNewMailReceived({message: [@msgRead]}) - expect(window.Notification).not.toHaveBeenCalled() + waitsForPromise => + Main._onNewMailReceived({message: [@msgRead]}) + .then -> + expect(window.Notification).not.toHaveBeenCalled() it "should not create a Notification if the new messages are actually old ones", -> - Main._onNewMailReceived({message: [@msgOld]}) - expect(window.Notification).not.toHaveBeenCalled() + waitsForPromise => + Main._onNewMailReceived({message: [@msgOld]}) + .then -> + expect(window.Notification).not.toHaveBeenCalled() diff --git a/src/flux/inbox-api.coffee b/src/flux/inbox-api.coffee index c81142156..19e544fa7 100644 --- a/src/flux/inbox-api.coffee +++ b/src/flux/inbox-api.coffee @@ -146,14 +146,17 @@ class InboxAPI # each type of model in the `create` hash, waits for them all to resolve. create[type] = @_handleModelResponse(items) for type, items of create Promise.props(create).then (created) => - if _.flatten(_.values(created)).length > 0 - Actions.didPassivelyReceiveNewModels(created) - # Apply all the deltas to modify objects. Gets promises for handling # each type of model in the `modify` hash, waits for them all to resolve. modify[type] = @_handleModelResponse(items) for type, items of modify Promise.props(modify).then (modified) -> + # Now that we've persisted creates/updates, fire an action + # that allows other parts of the app to update based on new models + # (notifications) + if _.flatten(_.values(created)).length > 0 + Actions.didPassivelyReceiveNewModels(created) + # Apply all of the deletions destroyPromises = destroy.map (delta) -> console.log(" - 1 #{delta.object} (#{delta.id})")