From a258f1a66b553b33760ac913bfced1f111c6d617 Mon Sep 17 00:00:00 2001 From: lilia Date: Fri, 4 Dec 2015 17:38:41 -0800 Subject: [PATCH] Refactor number parsing and validation Refactor libphonenumber.validateNumber into libphonenumber.parseNumber, which encapsulates the try-catch pattern used in number parsing and returns an object of info about the input number rather tha throwing since we expect to get some invalid number inputs the user is typing. In the conversation model, * Separate phone number validation from search token updating. * Perform token update before save if the number was valid. * Stop storing unneeded number variants as conversation properties. // FREEBIE --- js/libphonenumber-util.js | 25 ++++++++-------- js/models/conversations.js | 48 ++++++++++++------------------- js/views/phone-input-view.js | 16 +++++------ test/index.html | 1 + test/libphonenumber_util_test.js | 29 +++++++++++++++++++ test/models/conversations_test.js | 37 ++++++++++++++++++++++++ 6 files changed, 105 insertions(+), 51 deletions(-) create mode 100644 test/libphonenumber_util_test.js diff --git a/js/libphonenumber-util.js b/js/libphonenumber-util.js index da399b93..4a57716b 100644 --- a/js/libphonenumber-util.js +++ b/js/libphonenumber-util.js @@ -34,20 +34,19 @@ return (cc !== 0) ? cc : ""; }, - verifyNumber: function(number, regionCode) { - var parsedNumber = libphonenumber.parse(number, regionCode); + parseNumber: function(number, defaultRegionCode) { + try { + var parsedNumber = libphonenumber.parse(number, defaultRegionCode); - if(!regionCode || regionCode == 'ZZ') { - regionCode = libphonenumber.getRegionCodeForNumber(parsedNumber); - } - - var isValidNumber = libphonenumber.isValidNumber(parsedNumber); - var isValidNumberForRegion = libphonenumber.isValidNumberForRegion(parsedNumber, regionCode); - - if (isValidNumber && isValidNumberForRegion) { - return libphonenumber.format(parsedNumber, libphonenumber.PhoneNumberFormat.E164); - } else { - throw new Error("The number seems not to be valid."); + return { + isValidNumber: libphonenumber.isValidNumber(parsedNumber), + regionCode: libphonenumber.getRegionCodeForNumber(parsedNumber), + countryCode: '' + parsedNumber.getCountryCode(), + nationalNumber: '' + parsedNumber.getNationalNumber(), + e164: libphonenumber.format(parsedNumber, libphonenumber.PhoneNumberFormat.E164) + }; + } catch (ex) { + return { error: ex, isValidNumber: false }; } }, diff --git a/js/models/conversations.js b/js/models/conversations.js index 775ecb10..aad418f9 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -42,7 +42,6 @@ conversation: this }); - this.on('change:id change:name', this.updateTokens); this.on('change:avatar', this.updateAvatarUrl); this.on('destroy', this.revokeAvatarUrl); }, @@ -56,20 +55,20 @@ return "Invalid conversation type: " + attributes.type; } - if (!attributes.tokens) { - return this.updateTokens(); - } + var error = this.validateNumber(); + if (error) { return error; } + + this.updateTokens(); }, validateNumber: function() { - try { - this.id = libphonenumber.util.verifyNumber(this.id); - } catch (ex) { - if (ex === "Invalid country calling code") { - var regionCode = storage.get('regionCode'); - this.id = libphonenumber.util.verifyNumber(this.id, regionCode); + if (this.isPrivate()) { + var regionCode = storage.get('regionCode'); + var number = libphonenumber.util.parseNumber(this.id, regionCode); + if (number.isValidNumber) { + this.set({ id: number.e164 }); } else { - throw ex; + return number.error || "Invalid phone number"; } } }, @@ -78,26 +77,17 @@ var tokens = []; var name = this.get('name'); if (typeof name === 'string') { - tokens = name.trim().toLowerCase().split(/[\s\-_\(\)\+]+/); + tokens.push(name.toLowerCase()); + tokens = tokens.concat(name.trim().toLowerCase().split(/[\s\-_\(\)\+]+/)); } - if (this.isPrivate()) { - try { - this.validateNumber(); - var number = libphonenumber.util.splitCountryCode(this.id); - var international_number = '' + number.country_code + number.national_number; - var national_number = '' + number.national_number; - - this.set({ - id: this.id, - e164_number: this.id, - national_number: national_number, - international_number: international_number - }); - tokens = tokens.concat(this.id, national_number, international_number); - } catch(ex) { - return ex; - } + var regionCode = storage.get('regionCode'); + var number = libphonenumber.util.parseNumber(this.id, regionCode); + tokens.push( + this.id, + number.nationalNumber, + number.countryCode + number.nationalNumber + ); } this.set({tokens: tokens}); }, diff --git a/js/views/phone-input-view.js b/js/views/phone-input-view.js index 812d5736..48c981e0 100644 --- a/js/views/phone-input-view.js +++ b/js/views/phone-input-view.js @@ -22,20 +22,18 @@ validateNumber: function() { var input = this.$('input.number'); - try { - var regionCode = this.$('li.active').attr('data-country-code').toUpperCase(); - var number = input.val(); - - var parsedNumber = libphonenumber.util.verifyNumber(number, regionCode); + var regionCode = this.$('li.active').attr('data-country-code').toUpperCase(); + var number = input.val(); + var parsedNumber = libphonenumber.util.parseNumber(number, regionCode); + if (parsedNumber.isValidNumber) { this.$('.number-container').removeClass('invalid'); this.$('.number-container').addClass('valid'); - return parsedNumber; - } catch(e) { + return parsedNumber.e164; + } else { this.$('.number-container').removeClass('valid'); - } finally { - input.trigger('validation'); } + input.trigger('validation'); } }); })(); diff --git a/test/index.html b/test/index.html index 372690b3..2e92128e 100644 --- a/test/index.html +++ b/test/index.html @@ -127,6 +127,7 @@ + diff --git a/test/libphonenumber_util_test.js b/test/libphonenumber_util_test.js new file mode 100644 index 00000000..9e98bead --- /dev/null +++ b/test/libphonenumber_util_test.js @@ -0,0 +1,29 @@ +/* + * vim: ts=4:sw=4:expandtab + */ + +(function () { + 'use strict'; + describe('libphonenumber util', function() { + describe('parseNumber', function() { + it('numbers with + are valid without providing regionCode', function() { + var result = libphonenumber.util.parseNumber('+14155555555'); + assert.isTrue(result.isValidNumber); + assert.strictEqual(result.nationalNumber, '4155555555'); + assert.strictEqual(result.e164, '+14155555555'); + assert.strictEqual(result.regionCode, 'US'); + assert.strictEqual(result.countryCode, '1'); + }); + it('variant numbers with the right regionCode are valid', function() { + [ '4155555555', '14155555555', '+14155555555', ].forEach(function(number) { + var result = libphonenumber.util.parseNumber(number, 'US'); + assert.isTrue(result.isValidNumber); + assert.strictEqual(result.nationalNumber, '4155555555'); + assert.strictEqual(result.e164, '+14155555555'); + assert.strictEqual(result.regionCode, 'US'); + assert.strictEqual(result.countryCode, '1'); + }); + }); + }); + }); +})(); diff --git a/test/models/conversations_test.js b/test/models/conversations_test.js index bd4f7876..4e130f6f 100644 --- a/test/models/conversations_test.js +++ b/test/models/conversations_test.js @@ -153,6 +153,43 @@ convo.revokeAvatarUrl(); assert.notOk(convo.avatarUrl); }); + + describe('phone number parsing', function() { + after(function() { storage.remove('regionCode'); }); + function checkAttributes(number) { + var convo = new Whisper.ConversationCollection().add({type: 'private'}); + convo.set('id', number); + convo.validate(convo.attributes); + assert.strictEqual(convo.get('id'), '+14155555555', number); + } + it('processes the phone number when validating', function() { + [ '+14155555555', ].forEach(checkAttributes); + }); + it('defaults to the local regionCode', function() { + storage.put('regionCode', 'US'); + [ '14155555555', '4155555555' ].forEach(checkAttributes); + }); + it('works with common phone number formats', function() { + storage.put('regionCode', 'US'); + [ + '415 555 5555', + '415-555-5555', + '(415) 555 5555', + '(415) 555-5555', + '1 415 555 5555', + '1 415-555-5555', + '1 (415) 555 5555', + '1 (415) 555-5555', + '+1 415 555 5555', + '+1 415-555-5555', + '+1 (415) 555 5555', + '+1 (415) 555-5555', + + ].forEach(checkAttributes); + }); + }); + }); + }); })();;