From b4115ed879a8218feeac6a01c0446cff5f22247e Mon Sep 17 00:00:00 2001 From: Felix Hammerl Date: Thu, 5 Dec 2013 18:28:18 +0100 Subject: [PATCH] fix unread and answered flags are updated --- src/js/controller/mail-list.js | 39 +--- src/js/controller/write.js | 6 - src/js/dao/email-dao.js | 177 ++++++++++++++---- test/new-unit/email-dao-test.js | 263 ++++++++++++++++++++++++--- test/new-unit/mail-list-ctrl-test.js | 9 +- test/new-unit/write-ctrl-test.js | 20 +- 6 files changed, 397 insertions(+), 117 deletions(-) diff --git a/src/js/controller/mail-list.js b/src/js/controller/mail-list.js index 3bc9c96..0379c32 100644 --- a/src/js/controller/mail-list.js +++ b/src/js/controller/mail-list.js @@ -60,9 +60,16 @@ define(function(require) { } $scope.state.mailList.selected = email; + $scope.state.read.toggle(true); - // // mark selected message as 'read' - // markAsRead(email); + // if the email is unread, please sync the new state. + // otherweise forget about it. + if (!email.unread) { + return; + } + + email.unread = false; + $scope.synchronize(); }; $scope.synchronize = function(callback) { @@ -204,34 +211,6 @@ define(function(require) { function getFolder() { return $scope.state.nav.currentFolder; } - - // function markAsRead(email) { - // // marking mails as read is meaningless in the outbox - // if (getFolder().type === 'Outbox') { - // return; - // } - - // $scope.state.read.toggle(true); - // if (!window.chrome || !chrome.socket) { - // return; - // } - - // if (!email.unread) { - // return; - // } - - // email.unread = false; - // emailDao.imapMarkMessageRead({ - // folder: getFolder().path, - // uid: email.uid - // }, function(err) { - // if (err) { - // updateStatus('Error marking read!'); - // $scope.onError(err); - // return; - // } - // }); - // } }; function createDummyMails() { diff --git a/src/js/controller/write.js b/src/js/controller/write.js index b861a8c..54cdb0f 100644 --- a/src/js/controller/write.js +++ b/src/js/controller/write.js @@ -181,12 +181,6 @@ define(function(require) { // mark list object $scope.replyTo.answered = true; - - // mark remote imap object - emailDao.markAnswered({ - uid: $scope.replyTo.uid, - folder: $scope.state.nav.currentFolder.path - }, $scope.onError); } }; diff --git a/src/js/dao/email-dao.js b/src/js/dao/email-dao.js index 3d39c8c..cd2806a 100644 --- a/src/js/dao/email-dao.js +++ b/src/js/dao/email-dao.js @@ -143,15 +143,26 @@ define(function(require) { EmailDAO.prototype.sync = function(options, callback) { /* * Here's how delta sync works: - * delta1: storage > memory => we deleted messages, remove from remote + * + * First, we sync the messages between memory and local storage, based on their uid + * delta1: storage > memory => we deleted messages, remove from remote and memory * delta2: memory > storage => we added messages, push to remote <<< not supported yet - * delta3: memory > imap => we deleted messages directly from the remote, remove from memory and storage - * delta4: imap > memory => we have new messages available, fetch to memory and storage + * + * Second, we check the delta for the flags + * deltaF1: memory > storage => we changed flags, sync them to the remote and memory + * + * Third, we go on to sync between imap and memory, again based on uid + * delta3: memory > imap => we deleted messages directly from the remote, remove from memory and storage + * delta4: imap > memory => we have new messages available, fetch to memory and storage + * + * Fourth, we pull changes in the flags downstream + * deltaF2: imap > memory => we changed flags directly on the remote, sync them to the storage and memory */ var self = this, folder, - delta1 /*, delta2 */ , delta3, delta4, + delta1 /*, delta2 */ , delta3, delta4, //message + deltaF1, deltaF2, isFolderInitialized; @@ -237,24 +248,22 @@ define(function(require) { /* * delta1: storage > memory => we deleted messages, remove from remote * delta2: memory > storage => we added messages, push to remote + * deltaF1: memory > storage => we changed flags, sync them to the remote and memory */ delta1 = checkDelta(messages, folder.messages); // delta2 = checkDelta(folder.messages, messages); // not supported yet - - if (_.isEmpty(delta1) /* && _.isEmpty(delta2)*/ ) { - // if there is no delta, head directly to imap sync - callback(); - doImapDelta(); - return; - } + deltaF1 = checkFlags(folder.messages, messages); doDelta1(); function doDelta1() { + if (_.isEmpty(delta1)) { + doDeltaF1(); + return; + } + var after = _.after(delta1.length, function() { - // doDelta2(); when it is implemented - callback(); - doImapDelta(); + doDeltaF1(); }); delta1.forEach(function(message) { @@ -282,6 +291,47 @@ define(function(require) { }); }); } + + function doDeltaF1() { + if (_.isEmpty(deltaF1)) { + callback(); + doImapDelta(); + return; + } + + var after = _.after(deltaF1.length, function() { + callback(); + doImapDelta(); + }); + + deltaF1.forEach(function(message) { + self._mark({ + folder: folder.path, + uid: message.uid, + unread: message.unread, + answered: message.answered + }, function(err) { + if (err) { + self._account.busy = false; + callback(err); + return; + } + + self._localStoreMessages({ + folder: folder.path, + emails: [message] + }, function(err) { + if (err) { + self._account.busy = false; + callback(err); + return; + } + + after(); + }); + }); + }); + } }); } @@ -304,18 +354,13 @@ define(function(require) { }); /* - * delta3: memory > imap => we deleted messages directly from the remote, remove from memory and storage - * delta4: imap > memory => we have new messages available, fetch to memory and storage + * delta3: memory > imap => we deleted messages directly from the remote, remove from memory and storage + * delta4: imap > memory => we have new messages available, fetch to memory and storage + * deltaF2: imap > memory => we changed flags directly on the remote, sync them to the storage and memory */ delta3 = checkDelta(folder.messages, headers); delta4 = checkDelta(headers, folder.messages); - - if (_.isEmpty(delta3) && _.isEmpty(delta4)) { - // if there is no delta, we're done - self._account.busy = false; - callback(); - return; - } + deltaF2 = checkFlags(headers, folder.messages); doDelta3(); @@ -357,13 +402,11 @@ define(function(require) { function doDelta4() { // no delta, we're done here if (_.isEmpty(delta4)) { - self._account.busy = false; - callback(); + doDeltaF2(); } var after = _.after(delta4.length, function() { - self._account.busy = false; - callback(); + doDeltaF2(); }); delta4.forEach(function(header) { @@ -391,6 +434,9 @@ define(function(require) { return; } + message.answered = header.answered; + message.unread = header.unread; + // add the encrypted message to the local storage self._localStoreMessages({ folder: folder.path, @@ -417,6 +463,44 @@ define(function(require) { }); }); } + + // we have a mismatch concerning flags between imap and memory. + // pull changes from imap. + function doDeltaF2() { + if (_.isEmpty(deltaF2)) { + self._account.busy = false; + callback(); + return; + } + + var after = _.after(deltaF2.length, function() { + self._account.busy = false; + callback(); + }); + + deltaF2.forEach(function(header) { + // we don't work on the header, we work on the live object + var msg = _.findWhere(folder.messages, { + uid: header.uid + }); + + msg.unread = header.unread; + msg.answered = header.answered; + self._localStoreMessages({ + folder: folder.path, + emails: [msg] + }, function(err) { + if (err) { + self._account.busy = false; + callback(err); + return; + } + + after(); + }); + }); + } + }); } @@ -441,6 +525,27 @@ define(function(require) { return delta; } + /* + * checks if there are some flags that have changed in a and b + */ + function checkFlags(a, b) { + var i, aI, bI, + delta = []; + + // find the delta + for (i = a.length - 1; i >= 0; i--) { + aI = a[i]; + bI = _.findWhere(b, { + uid: aI.uid + }); + if (bI && (aI.unread !== bI.unread || aI.answered !== bI.answered)) { + delta.push(aI); + } + } + + return delta; + } + function isVerificationMail(email) { return email.subject === str.subjectPrefix + str.verificationSubject; } @@ -472,9 +577,10 @@ define(function(require) { } // public key has been verified, mark the message as read, delete it, and ignore it in the future - self.markRead({ + self._mark({ folder: options.folder, - uid: email.uid + uid: email.uid, + unread: false }, function(err) { if (err) { localCallback(err); @@ -551,19 +657,12 @@ define(function(require) { } }; - EmailDAO.prototype.markRead = function(options, callback) { + EmailDAO.prototype._mark = function(options, callback) { this._imapClient.updateFlags({ path: options.folder, uid: options.uid, - unread: false - }, callback); - }; - - EmailDAO.prototype.markAnswered = function(options, callback) { - this._imapClient.updateFlags({ - path: options.folder, - uid: options.uid, - answered: true + unread: options.unread, + answered: options.answered }, callback); }; diff --git a/test/new-unit/email-dao-test.js b/test/new-unit/email-dao-test.js index d15adc2..e8c9d74 100644 --- a/test/new-unit/email-dao-test.js +++ b/test/new-unit/email-dao-test.js @@ -31,7 +31,9 @@ define(function(require) { address: 'qwe@qwe.de' }], subject: '[whiteout] qweasd', - body: '-----BEGIN PGP MESSAGE-----\nasd\n-----END PGP MESSAGE-----' + body: '-----BEGIN PGP MESSAGE-----\nasd\n-----END PGP MESSAGE-----', + unread: false, + answered: false }; verificationUuid = 'OMFG_FUCKING_BASTARD_UUID_FROM_HELL!'; verificationMail = { @@ -44,7 +46,8 @@ define(function(require) { }], // list of receivers subject: "[whiteout] New public key uploaded", // Subject line body: 'yadda yadda bla blabla foo bar https://keys.whiteout.io/verify/' + verificationUuid, // plaintext body - unread: true + unread: true, + answered: false }; dummyDecryptedMail = { uid: 1234, @@ -55,7 +58,9 @@ define(function(require) { address: 'qwe@qwe.de' }], subject: 'qweasd', - body: 'asd' + body: 'asd', + unread: false, + answered: false }; nonWhitelistedMail = { uid: 1234, @@ -113,7 +118,7 @@ define(function(require) { expect(obj).to.equal(o); done(); }; - + dao._imapClient.onIncomingMessage(o); }); }); @@ -594,7 +599,9 @@ define(function(require) { folder: folder }).yields(null, [{ uid: dummyEncryptedMail.uid, - subject: '[whiteout] ' + dummyEncryptedMail // the object has already been manipulated as a side-effect... + subject: '[whiteout] ' + dummyEncryptedMail.subject, // the object has already been manipulated as a side-effect... + unread: dummyEncryptedMail.unread, + answered: dummyEncryptedMail.answered }]); dao.sync({ @@ -1234,9 +1241,10 @@ define(function(require) { imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); keychainStub.verifyPublicKey.withArgs(verificationUuid).yields(); - markReadStub = sinon.stub(dao, 'markRead').withArgs({ + markReadStub = sinon.stub(dao, '_mark').withArgs({ folder: folder, - uid: verificationMail.uid + uid: verificationMail.uid, + unread: false }).yields(); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage').withArgs({ folder: folder, @@ -1283,7 +1291,7 @@ define(function(require) { imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); keychainStub.verifyPublicKey.yields(); - markReadStub = sinon.stub(dao, 'markRead').yields(); + markReadStub = sinon.stub(dao, '_mark').yields(); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage').yields({}); dao.sync({ @@ -1326,7 +1334,7 @@ define(function(require) { imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); keychainStub.verifyPublicKey.yields(); - markReadStub = sinon.stub(dao, 'markRead').yields({}); + markReadStub = sinon.stub(dao, '_mark').yields({}); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage'); dao.sync({ @@ -1369,7 +1377,7 @@ define(function(require) { imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); keychainStub.verifyPublicKey.yields({}); - markReadStub = sinon.stub(dao, 'markRead'); + markReadStub = sinon.stub(dao, '_mark'); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage'); dao.sync({ @@ -1413,7 +1421,7 @@ define(function(require) { localListStub = sinon.stub(dao, '_localListMessages').yields(null, []); imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); - markReadStub = sinon.stub(dao, 'markRead'); + markReadStub = sinon.stub(dao, '_mark'); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage'); dao.sync({ @@ -1456,7 +1464,7 @@ define(function(require) { localListStub = sinon.stub(dao, '_localListMessages').yields(null, []); imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [verificationMail]); imapGetStub = sinon.stub(dao, '_imapGetMessage').yields(null, verificationMail); - markReadStub = sinon.stub(dao, 'markRead'); + markReadStub = sinon.stub(dao, '_mark'); imapDeleteStub = sinon.stub(dao, '_imapDeleteMessage'); dao.sync({ @@ -1482,38 +1490,239 @@ define(function(require) { done(); }); }); - }); - describe('markAsRead', function() { - it('should work', function(done) { - imapClientStub.updateFlags.withArgs({ - path: 'asdf', - uid: 1, - unread: false + it('should sync tags from memory to imap and storage', function(done) { + var folder, localListStub, imapListStub, invocations, + markStub, localStoreStub; + + invocations = 0; + folder = 'FOLDAAAA'; + dao._account.folders = [{ + type: 'Folder', + path: folder, + messages: [dummyDecryptedMail] + }]; + + var inStorage = JSON.parse(JSON.stringify(dummyEncryptedMail)); + var inImap = JSON.parse(JSON.stringify(dummyEncryptedMail)); + dummyDecryptedMail.unread = inImap.unread = true; + + localListStub = sinon.stub(dao, '_localListMessages').yields(null, [inStorage]); + imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [inImap]); + markStub = sinon.stub(dao, '_mark').withArgs({ + folder: folder, + uid: dummyDecryptedMail.uid, + unread: dummyDecryptedMail.unread, + answered: dummyDecryptedMail.answered + }).yields(); + localStoreStub = sinon.stub(dao, '_localStoreMessages').withArgs({ + folder: folder, + emails: [dummyDecryptedMail] }).yields(); - dao.markRead({ - folder: 'asdf', - uid: 1 + dao.sync({ + folder: folder }, function(err) { - expect(imapClientStub.updateFlags.calledOnce).to.be.true; expect(err).to.not.exist; + + if (invocations === 0) { + expect(dao._account.busy).to.be.true; + invocations++; + return; + } + + expect(dao._account.busy).to.be.false; + expect(dao._account.folders[0]).to.not.be.empty; + expect(localListStub.calledOnce).to.be.true; + expect(imapListStub.calledOnce).to.be.true; + expect(markStub.calledOnce).to.be.true; + expect(localStoreStub.calledOnce).to.be.true; + + done(); + }); + }); + + it('should error while syncing tags from memory to storage', function(done) { + var folder, localListStub, imapListStub, invocations, + markStub, localStoreStub; + + invocations = 0; + folder = 'FOLDAAAA'; + dao._account.folders = [{ + type: 'Folder', + path: folder, + messages: [dummyDecryptedMail] + }]; + + var inStorage = JSON.parse(JSON.stringify(dummyEncryptedMail)); + var inImap = JSON.parse(JSON.stringify(dummyEncryptedMail)); + dummyDecryptedMail.unread = inImap.unread = true; + + localListStub = sinon.stub(dao, '_localListMessages').yields(null, [inStorage]); + imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [inImap]); + markStub = sinon.stub(dao, '_mark').yields(); + localStoreStub = sinon.stub(dao, '_localStoreMessages').yields({}); + + dao.sync({ + folder: folder + }, function(err) { + expect(err).to.exist; + + expect(dao._account.busy).to.be.false; + expect(dao._account.folders[0]).to.not.be.empty; + expect(localListStub.calledOnce).to.be.true; + expect(markStub.calledOnce).to.be.true; + expect(localStoreStub.calledOnce).to.be.true; + expect(imapListStub.called).to.be.false; + done(); + }); + }); + + it('should error while syncing tags from memory to imap', function(done) { + var folder, localListStub, imapListStub, invocations, + markStub, localStoreStub; + + invocations = 0; + folder = 'FOLDAAAA'; + dao._account.folders = [{ + type: 'Folder', + path: folder, + messages: [dummyDecryptedMail] + }]; + + var inStorage = JSON.parse(JSON.stringify(dummyEncryptedMail)); + var inImap = JSON.parse(JSON.stringify(dummyEncryptedMail)); + dummyDecryptedMail.unread = inImap.unread = true; + + localListStub = sinon.stub(dao, '_localListMessages').yields(null, [inStorage]); + imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [inImap]); + markStub = sinon.stub(dao, '_mark').yields({}); + localStoreStub = sinon.stub(dao, '_localStoreMessages'); + + dao.sync({ + folder: folder + }, function(err) { + expect(err).to.exist; + + expect(dao._account.busy).to.be.false; + expect(dao._account.folders[0]).to.not.be.empty; + expect(localListStub.calledOnce).to.be.true; + expect(markStub.calledOnce).to.be.true; + expect(localStoreStub.called).to.be.false; + expect(imapListStub.called).to.be.false; + done(); + }); + }); + + it('should sync tags from imap to memory and storage', function(done) { + var folder, localListStub, imapListStub, invocations, + markStub, localStoreStub; + + invocations = 0; + folder = 'FOLDAAAA'; + dao._account.folders = [{ + type: 'Folder', + path: folder, + messages: [dummyDecryptedMail] + }]; + + var inStorage = JSON.parse(JSON.stringify(dummyEncryptedMail)); + var inImap = JSON.parse(JSON.stringify(dummyEncryptedMail)); + dummyDecryptedMail.unread = inStorage.unread = true; + + localListStub = sinon.stub(dao, '_localListMessages').yields(null, [inStorage]); + imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [inImap]); + markStub = sinon.stub(dao, '_mark'); + localStoreStub = sinon.stub(dao, '_localStoreMessages').withArgs({ + folder: folder, + emails: [dummyDecryptedMail] + }).yields(); + + dao.sync({ + folder: folder + }, function(err) { + expect(err).to.not.exist; + + if (invocations === 0) { + expect(dao._account.busy).to.be.true; + invocations++; + return; + } + + expect(dao._account.busy).to.be.false; + expect(dao._account.folders[0]).to.not.be.empty; + expect(localListStub.calledOnce).to.be.true; + expect(imapListStub.calledOnce).to.be.true; + expect(markStub.called).to.be.false; + expect(localStoreStub.calledOnce).to.be.true; + + expect(dummyDecryptedMail.unread).to.equal(inImap.unread); + + done(); + }); + }); + + it('should error while syncing tags from imap to storage', function(done) { + var folder, localListStub, imapListStub, invocations, + markStub, localStoreStub; + + invocations = 0; + folder = 'FOLDAAAA'; + dao._account.folders = [{ + type: 'Folder', + path: folder, + messages: [dummyDecryptedMail] + }]; + + var inStorage = JSON.parse(JSON.stringify(dummyEncryptedMail)); + var inImap = JSON.parse(JSON.stringify(dummyEncryptedMail)); + dummyDecryptedMail.unread = inStorage.unread = true; + + localListStub = sinon.stub(dao, '_localListMessages').yields(null, [inStorage]); + imapListStub = sinon.stub(dao, '_imapListMessages').yields(null, [inImap]); + markStub = sinon.stub(dao, '_mark'); + localStoreStub = sinon.stub(dao, '_localStoreMessages').yields({}); + + dao.sync({ + folder: folder + }, function(err) { + + if (invocations === 0) { + expect(err).to.not.exist; + expect(dao._account.busy).to.be.true; + invocations++; + return; + } + + expect(err).to.exist; + expect(dao._account.busy).to.be.false; + expect(dao._account.folders[0]).to.not.be.empty; + expect(localListStub.calledOnce).to.be.true; + expect(imapListStub.calledOnce).to.be.true; + expect(markStub.called).to.be.false; + expect(localStoreStub.calledOnce).to.be.true; + + expect(dummyDecryptedMail.unread).to.equal(inImap.unread); + done(); }); }); }); - describe('markAsAnswered', function() { + describe('mark', function() { it('should work', function(done) { imapClientStub.updateFlags.withArgs({ path: 'asdf', uid: 1, - answered: true + unread: false, + answered: false }).yields(); - dao.markAnswered({ + dao._mark({ folder: 'asdf', - uid: 1 + uid: 1, + unread: false, + answered: false }, function(err) { expect(imapClientStub.updateFlags.calledOnce).to.be.true; expect(err).to.not.exist; diff --git a/test/new-unit/mail-list-ctrl-test.js b/test/new-unit/mail-list-ctrl-test.js index d56b16d..e5b1e49 100644 --- a/test/new-unit/mail-list-ctrl-test.js +++ b/test/new-unit/mail-list-ctrl-test.js @@ -75,7 +75,11 @@ define(function(require) { mocks.module('maillisttest'); mocks.inject(function($rootScope, $controller) { scope = $rootScope.$new(); - scope.state = {}; + scope.state = { + read: { + toggle: function() {} + } + }; ctrl = $controller(MailListCtrl, { $scope: scope }); @@ -210,11 +214,12 @@ define(function(require) { scope.state.nav = { currentFolder: currentFolder }; + scope.state.mailList.selected = undefined; scope.synchronize(); // emails array is also used as the outbox's pending mail - expect(scope.state.mailList.selected).to.deep.equal(emails[0]); + expect(scope.state.mailList.selected).to.deep.equal(emails[emails.length - 1]); }); }); diff --git a/test/new-unit/write-ctrl-test.js b/test/new-unit/write-ctrl-test.js index 96e6ee7..7275e2f 100644 --- a/test/new-unit/write-ctrl-test.js +++ b/test/new-unit/write-ctrl-test.js @@ -141,7 +141,7 @@ define(function(require) { }); describe('send to outbox', function() { - it('should work', function(done) { + it('should work', function() { var verifyToSpy = sinon.spy(scope, 'verifyTo'), re = { from: [{ @@ -159,21 +159,15 @@ define(function(require) { scope.emptyOutbox = function() {}; emailDaoMock.store.yields(); - emailDaoMock.markAnswered.yields(); - - scope.onError = function(err) { - expect(err).to.not.exist; - expect(scope.state.writer.open).to.be.false; - expect(emailDaoMock.store.calledOnce).to.be.true; - expect(emailDaoMock.store.calledOnce).to.be.true; - expect(verifyToSpy.calledOnce).to.be.true; - - scope.verifyTo.restore(); - done(); - }; scope.state.writer.write(re); scope.sendToOutbox(); + + expect(scope.state.writer.open).to.be.false; + expect(emailDaoMock.store.calledOnce).to.be.true; + expect(verifyToSpy.calledOnce).to.be.true; + + scope.verifyTo.restore(); }); it('should not work and not close the write view', function(done) {