From faeb319c58f0dbd592d7801a9a7f2d2ae3fa3818 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Tue, 15 Jan 2019 12:21:15 +1100 Subject: [PATCH 1/5] Added profile model test. --- js/models/profile.js | 4 + test/index.html | 1 + test/models/conversations_test.js | 2 +- test/models/profile_test.js | 134 ++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 test/models/profile_test.js diff --git a/js/models/profile.js b/js/models/profile.js index cd8b6ec9b..c20d49bbb 100644 --- a/js/models/profile.js +++ b/js/models/profile.js @@ -17,6 +17,10 @@ } storage.setProfileName = async (newName) => { + if (typeof newName !== 'string' && newName !== null) { + throw Error('Name must be a string!'); + } + // Update our profiles accordingly' const trimmed = newName && newName.trim(); diff --git a/test/index.html b/test/index.html index 192c6f556..55a02a55d 100644 --- a/test/index.html +++ b/test/index.html @@ -426,6 +426,7 @@ + diff --git a/test/models/conversations_test.js b/test/models/conversations_test.js index 62190fec7..de8798a97 100644 --- a/test/models/conversations_test.js +++ b/test/models/conversations_test.js @@ -192,4 +192,4 @@ describe('Conversation', () => { assert.isUndefined(collection.get(convo.id), 'got result for "+"'); }); }); -})(); +}); diff --git a/test/models/profile_test.js b/test/models/profile_test.js new file mode 100644 index 000000000..c57579272 --- /dev/null +++ b/test/models/profile_test.js @@ -0,0 +1,134 @@ +/* global storage */ + +/* eslint no-await-in-loop: 0 */ + +'use strict'; + +const PROFILE_ID = 'local-profile'; + +describe('Profile', () => { + beforeEach(async () => { + await clearDatabase(); + await storage.remove(PROFILE_ID); + }); + + describe('getLocalProfile', () => { + it('returns the local profile', async () => { + const values = [null, 'hello', { a: 'b' }]; + for(let i = 0; i < values.length; i += 1) { + await storage.put(PROFILE_ID, values[i]); + assert.strictEqual(values[i], storage.getLocalProfile()); + } + }); + }); + + describe('saveLocalProfile', () => { + it('saves a profile', async () => { + const values = [null, 'hello', { a: 'b' }]; + for(let i = 0; i < values.length; i += 1) { + await storage.saveLocalProfile(values[i]); + assert.strictEqual(values[i], storage.get(PROFILE_ID)); + } + }); + }); + + describe('removeLocalProfile', () => { + it('removes a profile', async () => { + await storage.saveLocalProfile('a'); + assert.strictEqual('a', storage.getLocalProfile()); + + await storage.removeLocalProfile(); + assert.strictEqual(null, storage.getLocalProfile()); + }); + }); + + describe('setProfileName', () => { + it('throws if a name is not a string', async () => { + const values = [0, { a: 'b'}, [1, 2]]; + for(let i = 0; i < values.length; i += 1) { + try { + await storage.setProfileName(values[i]); + assert.fail(`setProfileName did not throw an error for ${typeof values[i]}`); + } catch (e) { + assert.throws(() => { throw e; }, 'Name must be a string!'); + } + } + }); + + it('does not throw if we pass a string or null', async () => { + const values = [null, '1']; + for(let i = 0; i < values.length; i += 1) { + try { + await storage.setProfileName(values[i]); + } catch (e) { + assert.fail('setProfileName threw an error'); + } + } + }); + + it('saves the display name', async () => { + await storage.setProfileName('hi there!'); + + const expected = { + displayName: 'hi there!', + }; + const profile = storage.getLocalProfile(); + assert.exists(profile.name); + assert.deepEqual(expected, profile.name); + }); + + it('saves the display name without overwriting the other profile properties', async () => { + const profile = { title: 'hello' }; + await storage.put(PROFILE_ID, profile); + await storage.setProfileName('hi there!'); + + const expected = { + ...profile, + name: { + displayName: 'hi there!', + }, + }; + assert.deepEqual(expected, storage.getLocalProfile()); + }); + + it('trims the display name', async () => { + const values = [ + { + current: ' prefix', + expected: 'prefix', + }, + { + current: 'suffix ', + expected: 'suffix', + }, + { + current: 'in middle', + expected: 'in middle', + }, + ]; + + for(let i = 0; i < values.length; i += 1) { + const { current, expected } = values[i]; + await storage.setProfileName(current); + const profile = storage.getLocalProfile(); + const name = { + displayName: expected, + }; + assert.deepEqual(name, profile.name); + } + }); + + it('unsets the name property if it is empty', async () => { + const profile = { + name: { + displayName: 'HI THERE!', + }, + }; + await storage.put(PROFILE_ID, profile); + assert.exists(storage.getLocalProfile().name); + + await storage.setProfileName(''); + assert.notExists(storage.getLocalProfile().name); + }); + }); +}); \ No newline at end of file From 7aad9417c794891dc6f595be02f518eb3c60c6fa Mon Sep 17 00:00:00 2001 From: Mikunj Date: Tue, 15 Jan 2019 13:21:05 +1100 Subject: [PATCH 2/5] Added blocked number controller tests --- js/blocked_number_controller.js | 10 +- test/blocked_number_controller_test.js | 153 +++++++++++++++++++++++++ test/index.html | 1 + 3 files changed, 158 insertions(+), 6 deletions(-) create mode 100644 test/blocked_number_controller_test.js diff --git a/js/blocked_number_controller.js b/js/blocked_number_controller.js index e390961d2..4cbb348de 100644 --- a/js/blocked_number_controller.js +++ b/js/blocked_number_controller.js @@ -22,6 +22,7 @@ return blockedNumbers; }, reset() { + this.unblockAll(); blockedNumbers.reset([]); }, load() { @@ -61,18 +62,15 @@ unblock(number) { storage.removeBlockedNumber(number); - // Make sure we don't add duplicates + // Remove the model from our collection const model = blockedNumbers.getNumber(number); if (model) { blockedNumbers.remove(model); } }, unblockAll() { - const all = blockedNumbers.models; - all.forEach(number => { - storage.removeBlockedNumber(number); - blockedNumbers.remove(number); - }); + const numbers = blockedNumbers.map(m => m.get('number')); + numbers.forEach(n => this.unblock(n)); }, isBlocked(number) { return storage.isBlocked(number); diff --git a/test/blocked_number_controller_test.js b/test/blocked_number_controller_test.js new file mode 100644 index 000000000..de9fbd2d4 --- /dev/null +++ b/test/blocked_number_controller_test.js @@ -0,0 +1,153 @@ +/* global textsecure, BlockedNumberController, storage */ + +'use strict'; + +describe('Blocked Number Controller', () => { + + beforeEach(async () => { + const numbers = storage.getBlockedNumbers(); + numbers.forEach(storage.removeBlockedNumber); + window.getBlockedNumbers().reset([]); + }); + + describe('getAll', () => { + it('returns our blocked numbers', () => { + assert.isEmpty(BlockedNumberController.getAll().models); + + BlockedNumberController.block('1'); + BlockedNumberController.block('2'); + assert.lengthOf(BlockedNumberController.getAll().models, 2); + }); + }); + + describe('reset', () => { + it('clears blocked numbers', () => { + BlockedNumberController.block('1'); + assert.isNotEmpty(BlockedNumberController.getAll().models); + + BlockedNumberController.reset(); + assert.isEmpty(BlockedNumberController.getAll().models); + }); + }); + + describe('load', () => { + it('loads blocked numbers from storage', () => { + BlockedNumberController.load(); + assert.isEmpty(window.getBlockedNumbers().models); + + storage.addBlockedNumber('1'); + storage.addBlockedNumber('2'); + BlockedNumberController.load(); + assert.lengthOf(window.getBlockedNumbers().models, 2); + }); + + it('throws if we have already loaded numbers', () => { + storage.addBlockedNumber('2'); + BlockedNumberController.load(); + assert.throws(() => BlockedNumberController.load(), 'BlockedNumberController: Already loaded!'); + }); + + it('throws if storage is invalid', () => { + const _storage = window.storage; + window.storage = null; + assert.throws(() => BlockedNumberController.load(), 'BlockedNumberController: Could not load blocked numbers'); + window.storage = _storage; + }); + }); + + describe('block', () => { + beforeEach(() => { + BlockedNumberController.load(); + assert.isEmpty(BlockedNumberController.getAll().models); + }); + + it('adds number to the blocked list', () => { + assert.isEmpty(storage.getBlockedNumbers()); + + BlockedNumberController.block('1'); + + const numbers = BlockedNumberController.getAll().models; + assert.lengthOf(numbers, 1); + assert.strictEqual('1', numbers[0].get('number')) + assert.deepEqual(['1'], storage.getBlockedNumbers()); + }); + + it('only blocks the same number once', () => { + BlockedNumberController.block('2'); + BlockedNumberController.block('2'); + assert.lengthOf(BlockedNumberController.getAll().models, 1); + assert.deepEqual(['2'], storage.getBlockedNumbers()); + }); + + it('does not block our own number', () => { + BlockedNumberController.block(textsecure.storage.user.getNumber()); + assert.isEmpty(BlockedNumberController.getAll().models); + assert.isEmpty(storage.getBlockedNumbers()); + }); + }); + + describe('unblock', () => { + beforeEach(() => { + BlockedNumberController.load(); + assert.isEmpty(BlockedNumberController.getAll().models); + }); + + it('removes number from the blocked list', () => { + BlockedNumberController.block('1'); + BlockedNumberController.block('2'); + + assert.lengthOf(BlockedNumberController.getAll().models, 2); + assert.lengthOf(storage.getBlockedNumbers(), 2); + + BlockedNumberController.unblock('1'); + + const numbers = BlockedNumberController.getAll().models; + assert.lengthOf(numbers, 1); + assert.isEmpty(numbers.filter(n => n.get('number') === '1')); + assert.deepEqual(['2'], storage.getBlockedNumbers()); + }); + + it('removes number from the blocked list even if it is not present in the collection', () => { + BlockedNumberController.block('1'); + BlockedNumberController.block('2'); + window.getBlockedNumbers().reset([]); + + assert.isEmpty(window.getBlockedNumbers().models); + assert.lengthOf(storage.getBlockedNumbers(), 2); + + BlockedNumberController.unblock('1'); + assert.deepEqual(['2'], storage.getBlockedNumbers()); + }); + }); + + describe('unblockAll', () => { + it('removes all our blocked numbers', () => { + BlockedNumberController.load(); + + BlockedNumberController.block('1'); + BlockedNumberController.block('2'); + BlockedNumberController.block('3'); + + assert.lengthOf(BlockedNumberController.getAll().models, 3); + assert.lengthOf(storage.getBlockedNumbers(), 3); + + BlockedNumberController.unblockAll(); + + assert.lengthOf(BlockedNumberController.getAll().models, 0); + assert.lengthOf(storage.getBlockedNumbers(), 0); + }); + }); + + describe('isBlocked', () => { + it('returns whether a number is blocked', () => { + BlockedNumberController.load(); + + BlockedNumberController.block('1'); + assert.isOk(BlockedNumberController.isBlocked('1')); + assert.isNotOk(BlockedNumberController.isBlocked('2')); + + BlockedNumberController.unblock('1'); + assert.isNotOk(BlockedNumberController.isBlocked('1')); + }); + }); +}); \ No newline at end of file diff --git a/test/index.html b/test/index.html index 55a02a55d..80e223b1c 100644 --- a/test/index.html +++ b/test/index.html @@ -429,6 +429,7 @@ + From feb8af476a77f0a0a1c40f6ad6a02264431a7fe2 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 16 Jan 2019 10:30:49 +1100 Subject: [PATCH 3/5] Refactored BlockedNumberController. --- js/background.js | 2 +- js/blocked_number_controller.js | 24 ++------- js/models/blockedNumbers.js | 2 +- js/signal_protocol_store.js | 2 +- js/views/blocked_number_view.js | 4 +- test/blocked_number_controller_test.js | 69 +++++++++++++------------- 6 files changed, 46 insertions(+), 57 deletions(-) diff --git a/js/background.js b/js/background.js index e53e79d5b..7f1d436e7 100644 --- a/js/background.js +++ b/js/background.js @@ -432,7 +432,7 @@ try { await ConversationController.load(); - BlockedNumberController.load(); + BlockedNumberController.refresh(); } catch (error) { window.log.error( 'background.js: ConversationController failed to load:', diff --git a/js/blocked_number_controller.js b/js/blocked_number_controller.js index 4cbb348de..7f4b3792c 100644 --- a/js/blocked_number_controller.js +++ b/js/blocked_number_controller.js @@ -1,4 +1,4 @@ -/* global , Whisper, storage, ConversationController */ +/* global , Whisper, storage */ /* global textsecure: false */ /* eslint-disable more/no-then */ @@ -13,34 +13,20 @@ window.getBlockedNumbers = () => blockedNumbers; window.BlockedNumberController = { - getAll() { - try { - this.load(); - } catch (e) { - window.log.warn(e); - } - return blockedNumbers; - }, reset() { this.unblockAll(); blockedNumbers.reset([]); }, - load() { + refresh() { window.log.info('BlockedNumberController: starting initial fetch'); - if (blockedNumbers.length) { - throw new Error('BlockedNumberController: Already loaded!'); - } - if (!storage) { throw new Error('BlockedNumberController: Could not load blocked numbers'); } // Add the numbers to the collection const numbers = storage.getBlockedNumbers(); - blockedNumbers.add( - numbers.map(number => ({ number })) - ); + blockedNumbers.reset(numbers.map(number => ({ number }))); }, block(number) { const ourNumber = textsecure.storage.user.getNumber(); @@ -54,7 +40,7 @@ storage.addBlockedNumber(number); // Make sure we don't add duplicates - if (blockedNumbers.getNumber(number)) + if (blockedNumbers.getModel(number)) return; blockedNumbers.add({ number }); @@ -63,7 +49,7 @@ storage.removeBlockedNumber(number); // Remove the model from our collection - const model = blockedNumbers.getNumber(number); + const model = blockedNumbers.getModel(number); if (model) { blockedNumbers.remove(model); } diff --git a/js/models/blockedNumbers.js b/js/models/blockedNumbers.js index 93cf697d8..f61716d68 100644 --- a/js/models/blockedNumbers.js +++ b/js/models/blockedNumbers.js @@ -85,7 +85,7 @@ comparator(m) { return m.get('number'); }, - getNumber(number) { + getModel(number) { return this.models.find(m => m.get('number') === number); }, }); diff --git a/js/signal_protocol_store.js b/js/signal_protocol_store.js index a51656810..76f001aa6 100644 --- a/js/signal_protocol_store.js +++ b/js/signal_protocol_store.js @@ -864,7 +864,7 @@ ConversationController.reset(); BlockedNumberController.reset(); await ConversationController.load(); - BlockedNumberController.load(); + BlockedNumberController.refresh(); }, async removeAllConfiguration() { await window.Signal.Data.removeAllConfiguration(); diff --git a/js/views/blocked_number_view.js b/js/views/blocked_number_view.js index e5202e7d5..51f53dd03 100644 --- a/js/views/blocked_number_view.js +++ b/js/views/blocked_number_view.js @@ -1,4 +1,5 @@ /* global BlockedNumberController: false */ +/* global getBlockedNumbers: false */ /* global Whisper: false */ /* global storage: false */ /* global i18n: false */ @@ -19,7 +20,8 @@ }, initialize() { storage.onready(() => { - this.collection = BlockedNumberController.getAll(); + BlockedNumberController.refresh(); + this.collection = getBlockedNumbers(); this.listView = new Whisper.BlockedNumberListView({ collection: this.collection, }); diff --git a/test/blocked_number_controller_test.js b/test/blocked_number_controller_test.js index de9fbd2d4..0c68c26b4 100644 --- a/test/blocked_number_controller_test.js +++ b/test/blocked_number_controller_test.js @@ -5,68 +5,68 @@ describe('Blocked Number Controller', () => { beforeEach(async () => { + // Purge everything manually const numbers = storage.getBlockedNumbers(); numbers.forEach(storage.removeBlockedNumber); window.getBlockedNumbers().reset([]); }); - describe('getAll', () => { - it('returns our blocked numbers', () => { - assert.isEmpty(BlockedNumberController.getAll().models); - - BlockedNumberController.block('1'); - BlockedNumberController.block('2'); - assert.lengthOf(BlockedNumberController.getAll().models, 2); - }); - }); - describe('reset', () => { it('clears blocked numbers', () => { BlockedNumberController.block('1'); - assert.isNotEmpty(BlockedNumberController.getAll().models); + assert.isNotEmpty(storage.getBlockedNumbers()); + assert.isNotEmpty(window.getBlockedNumbers().models); BlockedNumberController.reset(); - assert.isEmpty(BlockedNumberController.getAll().models); + assert.isEmpty(storage.getBlockedNumbers()); + assert.isEmpty(window.getBlockedNumbers().models); }); }); - describe('load', () => { + describe('refresh', () => { it('loads blocked numbers from storage', () => { - BlockedNumberController.load(); + BlockedNumberController.refresh(); assert.isEmpty(window.getBlockedNumbers().models); storage.addBlockedNumber('1'); storage.addBlockedNumber('2'); - BlockedNumberController.load(); + BlockedNumberController.refresh(); assert.lengthOf(window.getBlockedNumbers().models, 2); }); - it('throws if we have already loaded numbers', () => { + it('overrides old numbers if we refresh again', () => { + BlockedNumberController.refresh(); + assert.isEmpty(window.getBlockedNumbers().models); + + storage.addBlockedNumber('1'); + BlockedNumberController.refresh(); + assert.isNotEmpty(window.getBlockedNumbers().find(m => m.get('number') === '1')); + + storage.removeBlockedNumber('1'); storage.addBlockedNumber('2'); - BlockedNumberController.load(); - assert.throws(() => BlockedNumberController.load(), 'BlockedNumberController: Already loaded!'); + BlockedNumberController.refresh(); + assert.isNotEmpty(window.getBlockedNumbers().find(m => m.get('number') === '2')); }); it('throws if storage is invalid', () => { const _storage = window.storage; window.storage = null; - assert.throws(() => BlockedNumberController.load(), 'BlockedNumberController: Could not load blocked numbers'); + assert.throws(() => BlockedNumberController.refresh(), 'BlockedNumberController: Could not load blocked numbers'); window.storage = _storage; }); }); describe('block', () => { beforeEach(() => { - BlockedNumberController.load(); - assert.isEmpty(BlockedNumberController.getAll().models); + BlockedNumberController.refresh(); + assert.isEmpty(storage.getBlockedNumbers()); + assert.isEmpty(window.getBlockedNumbers().models); }); it('adds number to the blocked list', () => { - assert.isEmpty(storage.getBlockedNumbers()); - BlockedNumberController.block('1'); - const numbers = BlockedNumberController.getAll().models; + const numbers = window.getBlockedNumbers().models; assert.lengthOf(numbers, 1); assert.strictEqual('1', numbers[0].get('number')) assert.deepEqual(['1'], storage.getBlockedNumbers()); @@ -75,33 +75,34 @@ describe('Blocked Number Controller', () => { it('only blocks the same number once', () => { BlockedNumberController.block('2'); BlockedNumberController.block('2'); - assert.lengthOf(BlockedNumberController.getAll().models, 1); + assert.lengthOf(window.getBlockedNumbers().models, 1); assert.deepEqual(['2'], storage.getBlockedNumbers()); }); it('does not block our own number', () => { BlockedNumberController.block(textsecure.storage.user.getNumber()); - assert.isEmpty(BlockedNumberController.getAll().models); + assert.isEmpty(window.getBlockedNumbers().models); assert.isEmpty(storage.getBlockedNumbers()); }); }); describe('unblock', () => { beforeEach(() => { - BlockedNumberController.load(); - assert.isEmpty(BlockedNumberController.getAll().models); + BlockedNumberController.refresh(); + assert.isEmpty(storage.getBlockedNumbers()); + assert.isEmpty(window.getBlockedNumbers().models); }); it('removes number from the blocked list', () => { BlockedNumberController.block('1'); BlockedNumberController.block('2'); - assert.lengthOf(BlockedNumberController.getAll().models, 2); + assert.lengthOf(window.getBlockedNumbers().models, 2); assert.lengthOf(storage.getBlockedNumbers(), 2); BlockedNumberController.unblock('1'); - const numbers = BlockedNumberController.getAll().models; + const numbers = window.getBlockedNumbers().models; assert.lengthOf(numbers, 1); assert.isEmpty(numbers.filter(n => n.get('number') === '1')); assert.deepEqual(['2'], storage.getBlockedNumbers()); @@ -122,25 +123,25 @@ describe('Blocked Number Controller', () => { describe('unblockAll', () => { it('removes all our blocked numbers', () => { - BlockedNumberController.load(); + BlockedNumberController.refresh(); BlockedNumberController.block('1'); BlockedNumberController.block('2'); BlockedNumberController.block('3'); - assert.lengthOf(BlockedNumberController.getAll().models, 3); + assert.lengthOf(window.getBlockedNumbers().models, 3); assert.lengthOf(storage.getBlockedNumbers(), 3); BlockedNumberController.unblockAll(); - assert.lengthOf(BlockedNumberController.getAll().models, 0); + assert.lengthOf(window.getBlockedNumbers().models, 0); assert.lengthOf(storage.getBlockedNumbers(), 0); }); }); describe('isBlocked', () => { it('returns whether a number is blocked', () => { - BlockedNumberController.load(); + BlockedNumberController.refresh(); BlockedNumberController.block('1'); assert.isOk(BlockedNumberController.isBlocked('1')); From cd3d17f977cfa2ac5d78789069d1f6ac23a3565f Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 16 Jan 2019 13:05:54 +1100 Subject: [PATCH 4/5] Fix review issues. --- test/blocked_number_controller_test.js | 7 +++++-- test/models/profile_test.js | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/test/blocked_number_controller_test.js b/test/blocked_number_controller_test.js index 0c68c26b4..8df157d31 100644 --- a/test/blocked_number_controller_test.js +++ b/test/blocked_number_controller_test.js @@ -31,7 +31,10 @@ describe('Blocked Number Controller', () => { storage.addBlockedNumber('1'); storage.addBlockedNumber('2'); BlockedNumberController.refresh(); - assert.lengthOf(window.getBlockedNumbers().models, 2); + + const blocked = window.getBlockedNumbers().map(m => m.get('number')); + assert.lengthOf(blocked, 2); + assert.deepEqual(['1', '2'], blocked.sort()); }); it('overrides old numbers if we refresh again', () => { @@ -151,4 +154,4 @@ describe('Blocked Number Controller', () => { assert.isNotOk(BlockedNumberController.isBlocked('1')); }); }); -}); \ No newline at end of file +}); diff --git a/test/models/profile_test.js b/test/models/profile_test.js index c57579272..dd20317da 100644 --- a/test/models/profile_test.js +++ b/test/models/profile_test.js @@ -131,4 +131,4 @@ describe('Profile', () => { assert.notExists(storage.getLocalProfile().name); }); }); -}); \ No newline at end of file +}); From b4248afd98ff61b251c9dfea7d5578bd62fc660d Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 16 Jan 2019 15:29:08 +1100 Subject: [PATCH 5/5] Updated a profile test. --- test/models/profile_test.js | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/test/models/profile_test.js b/test/models/profile_test.js index dd20317da..b30d8dfc8 100644 --- a/test/models/profile_test.js +++ b/test/models/profile_test.js @@ -92,30 +92,12 @@ describe('Profile', () => { }); it('trims the display name', async () => { - const values = [ - { - current: ' prefix', - expected: 'prefix', - }, - { - current: 'suffix ', - expected: 'suffix', - }, - { - current: 'in middle', - expected: 'in middle', - }, - ]; - - for(let i = 0; i < values.length; i += 1) { - const { current, expected } = values[i]; - await storage.setProfileName(current); - const profile = storage.getLocalProfile(); - const name = { - displayName: expected, - }; - assert.deepEqual(name, profile.name); - } + await storage.setProfileName(' in middle '); + const profile = storage.getLocalProfile(); + const name = { + displayName: 'in middle', + }; + assert.deepEqual(name, profile.name); }); it('unsets the name property if it is empty', async () => {