From feb8af476a77f0a0a1c40f6ad6a02264431a7fe2 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 16 Jan 2019 10:30:49 +1100 Subject: [PATCH] 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'));