From 4cba16cb61b484f37b0762374b1001177f475758 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Wed, 6 Sep 2017 18:18:46 -0700 Subject: [PATCH] Fetch all conversations on startup of app, not on inbox load (#1437) * Fetch all conversations on startup of app, not on inbox load A recent change to fetch conversations less didn't take into account all that can happen in the app without the inbox loaded. That only happens when the window is shown, and messages can come in with the app in the background. In that case, the conversation wouldn't have been loaded from the database, but would be saved to the database anyway, losing data. This change fetches all conversations as soon as the the data store is ready for a fetch. It also introduces failsafe throws to ensure that synchronous ConversationController accesses don't happen until the initial fetch is complete. A new getUnsafe() method was required to account for some of the model setup that happens during that initial conversation fetch. Fixes #1428 FREEBIE * Fix tests: ConversationController.load() required before get() FREEBIE --- js/background.js | 4 ++- js/conversation_controller.js | 58 ++++++++++++++++++++++++++++------- js/expiring_messages.js | 5 ++- js/models/messages.js | 7 +++-- test/fixtures_test.js | 2 +- test/models/messages_test.js | 30 +++++++++++++----- 6 files changed, 82 insertions(+), 24 deletions(-) diff --git a/js/background.js b/js/background.js index 93858c2c3..3e47dd811 100644 --- a/js/background.js +++ b/js/background.js @@ -72,6 +72,8 @@ storage.fetch(); storage.onready(function() { + ConversationController.load(); + window.dispatchEvent(new Event('storage_ready')); setUnreadCount(storage.get("unreadCount", 0)); @@ -518,7 +520,7 @@ getAppView: function(destWindow) { var self = this; - return ConversationController.updateInbox().then(function() { + return ConversationController.loadPromise().then(function() { try { if (self.inboxView) { self.inboxView.remove(); } self.inboxView = new Whisper.InboxView({ diff --git a/js/conversation_controller.js b/js/conversation_controller.js index 6ac5abda5..e9591879f 100644 --- a/js/conversation_controller.js +++ b/js/conversation_controller.js @@ -83,12 +83,24 @@ window.ConversationController = { get: function(id) { + if (!this._initialFetchComplete) { + throw new Error('ConversationController.get() needs complete initial fetch'); + } + + return conversations.get(id); + }, + // Needed for some model setup which happens during the initial fetch() call below + getUnsafe: function(id) { return conversations.get(id); }, createTemporary: function(attributes) { return conversations.add(attributes); }, getOrCreate: function(id, type) { + if (!this._initialFetchComplete) { + throw new Error('ConversationController.get() needs complete initial fetch'); + } + var conversation = conversations.get(id); if (conversation) { return conversation; @@ -114,17 +126,19 @@ return conversation; }, getOrCreateAndWait: function(id, type) { - var conversation = this.getOrCreate(id, type); + return this._initialPromise.then(function() { + var conversation = this.getOrCreate(id, type); - if (conversation) { - return conversation.initialPromise.then(function() { - return conversation; - }); - } + if (conversation) { + return conversation.initialPromise.then(function() { + return conversation; + }); + } - return Promise.reject( - new Error('getOrCreateAndWait: did not get conversation') - ); + return Promise.reject( + new Error('getOrCreateAndWait: did not get conversation') + ); + }.bind(this)); }, getAllGroupsInvolvingId: function(id) { var groups = new Whisper.GroupCollection(); @@ -134,8 +148,30 @@ }); }); }, - updateInbox: function() { - return conversations.fetch(); + loadPromise: function() { + return this._initialPromise; + }, + load: function() { + console.log('ConversationController: starting initial fetch'); + if (this._initialPromise) { + throw new Error('ConversationController.load() has already been called!'); + } + + this._initialPromise = new Promise(function(resolve, reject) { + conversations.fetch().then(function() { + console.log('ConversationController: done with initial fetch'); + this._initialFetchComplete = true; + resolve(); + }.bind(this), function(error) { + console.log( + 'ConversationController: initial fetch failed', + error && error.stack ? error.stack : error + ); + reject(error); + }); + }.bind(this)); + + return this._initialPromise; } }; })(); diff --git a/js/expiring_messages.js b/js/expiring_messages.js index 887f4c13c..327658c16 100644 --- a/js/expiring_messages.js +++ b/js/expiring_messages.js @@ -11,7 +11,10 @@ var expired = new Whisper.MessageCollection(); expired.on('add', function(message) { console.log('message', message.get('sent_at'), 'expired'); - message.getConversation().trigger('expired', message); + var conversation = message.getConversation(); + if (conversation) { + conversation.trigger('expired', message); + } // We delete after the trigger to allow the conversation time to process // the expiration before the message is removed from the database. diff --git a/js/models/messages.js b/js/models/messages.js index e47f7e393..5b75dc1f1 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -147,7 +147,10 @@ return this.imageUrl; }, getConversation: function() { - return ConversationController.get(this.get('conversationId')); + // This needs to be an unsafe call, because this method is called during + // initial module setup. We may be in the middle of the initial fetch to + // the database. + return ConversationController.getUnsafe(this.get('conversationId')); }, getExpirationTimerUpdateSource: function() { if (this.isExpirationTimerUpdate()) { @@ -235,7 +238,7 @@ someRecipientsFailed: function() { var c = this.getConversation(); - if (c.isPrivate()) { + if (!c || c.isPrivate()) { return false; } diff --git a/test/fixtures_test.js b/test/fixtures_test.js index 0765b9501..546d05cca 100644 --- a/test/fixtures_test.js +++ b/test/fixtures_test.js @@ -11,7 +11,7 @@ describe("Fixtures", function() { }); it('renders', function(done) { - ConversationController.updateInbox().then(function() { + ConversationController.loadPromise().then(function() { var view = new Whisper.InboxView({window: window}); view.onEmpty(); view.$el.prependTo($('#render-android')); diff --git a/test/models/messages_test.js b/test/models/messages_test.js index b45e037a1..996ef7229 100644 --- a/test/models/messages_test.js +++ b/test/models/messages_test.js @@ -4,11 +4,13 @@ (function () { 'use strict'; - function clear(done) { - var messages = new Whisper.MessageCollection(); - return messages.fetch().then(function() { - messages.destroyAll(); - done(); + function deleteAllMessages() { + return new Promise(function(resolve, reject) { + var messages = new Whisper.MessageCollection(); + return messages.fetch().then(function() { + messages.destroyAll(); + resolve(); + }, reject); }); } @@ -21,9 +23,18 @@ var attachment = { data: 'datasaurus', contentType: 'plain/text' }; + var source = '+14155555555'; + describe('MessageCollection', function() { - before(clear); - after(clear); + before(function() { + return Promise.all([ + deleteAllMessages(), + ConversationController.load() + ]); + }); + after(function() { + return deleteAllMessages(); + }); it('has no image url', function() { var messages = new Whisper.MessageCollection(); @@ -49,7 +60,10 @@ it('gets incoming contact', function() { var messages = new Whisper.MessageCollection(); - var message = messages.add({ type: 'incoming' }); + var message = messages.add({ + type: 'incoming', + source: source + }); message.getContact(); });