From 6287001a844e8b82564cf55d418dc59693bfec4b Mon Sep 17 00:00:00 2001 From: ianguerin Date: Wed, 28 Feb 2024 13:26:48 -0800 Subject: [PATCH 1/4] Move lat, long parsing and validations logic into GeoPoint This logic will be removed from Viewfinder in future commit. --- src/js/models/maps/GeoPoint.js | 49 ++++++- .../specs/unit/models/maps/GeoPoint.spec.js | 137 +++++++++++++++++- 2 files changed, 178 insertions(+), 8 deletions(-) diff --git a/src/js/models/maps/GeoPoint.js b/src/js/models/maps/GeoPoint.js index f48abb800..5a29e960d 100644 --- a/src/js/models/maps/GeoPoint.js +++ b/src/js/models/maps/GeoPoint.js @@ -4,6 +4,12 @@ define(["backbone", "models/maps/GeoUtilities"], function ( Backbone, GeoUtilities ) { + // Regular expression matching a string that contains two numbers optionally separated by a comma. + const FLOATS_REGEX = /[+-]?[0-9]*[.]?[0-9]+/g; + + // Regular expression matching everything except numbers, periods, and commas. + const NON_LAT_LONG_CHARS_REGEX = /[^0-9,.+-\s]/g; + /** * @class GeoPoint * @classdesc The GeoPoint model stores geographical coordinates including @@ -118,19 +124,56 @@ define(["backbone", "models/maps/GeoUtilities"], function ( */ validate: function (attrs) { if (attrs.latitude < -90 || attrs.latitude > 90) { - return "Invalid latitude. Must be between -90 and 90."; + return { latitude: "Invalid latitude. Must be between -90 and 90." }; } if (attrs.longitude < -180 || attrs.longitude > 180) { - return "Invalid longitude. Must be between -180 and 180."; + return { + longitude: "Invalid longitude. Must be between -180 and 180." + }; } // Assuming height is in meters and can theoretically be below sea // level. Adjust the height constraints as needed for your specific // application. if (typeof attrs.height !== "number") { - return "Invalid height. Must be a number."; + return { height: "Invalid height. Must be a number." }; + } + }, + }, + { + /** + * Parse a string according to a regular expression. + * @param {string} value A user-entered value for parsing into a latiude + * and longitude pair. + * @throws An error indicating that more than two numbers have been + * entered. + * @returns + */ + fromString(value) { + const matches = value.match(FLOATS_REGEX); + if (matches?.length !== 2 || isNaN(matches[0]) || isNaN(matches[1]) + || !GeoPoint.couldBeLatLong(value)) { + throw new Error( + 'Try entering a search query with two numerical values representing a latitude and longitude (e.g. 64.84, -147.72).' + ); } + + const latitude = Number(matches[0]); + const longitude = Number(matches[1]); + + return new GeoPoint({ latitude, longitude }); + }, + + /** + * Determine whether the user is could be typing a lat, long pair. + * @param {string} value is the currently entered query string. + * @return {boolean} Whether the current value could be a lat,long pair + * due to the string NOT containing characters (e.g. a-z) that could not + * be in a lat,long pair. + */ + couldBeLatLong(value) { + return value.match(NON_LAT_LONG_CHARS_REGEX) == null; }, } ); diff --git a/test/js/specs/unit/models/maps/GeoPoint.spec.js b/test/js/specs/unit/models/maps/GeoPoint.spec.js index 96319bf73..c1023e1c0 100644 --- a/test/js/specs/unit/models/maps/GeoPoint.spec.js +++ b/test/js/specs/unit/models/maps/GeoPoint.spec.js @@ -1,16 +1,16 @@ -define([ - "../../../../../../../../src/js/models/maps/GeoPoint", -], function (GeoPoint) { +"use strict"; + +define(["models/maps/GeoPoint",], function (GeoPoint) { // Configure the Chai assertion library var should = chai.should(); var expect = chai.expect; describe("GeoPoint Test Suite", function () { /* Set up */ - beforeEach(function () {}); + beforeEach(function () { }); /* Tear down */ - afterEach(function () {}); + afterEach(function () { }); describe("Initialization", function () { it("should create a GeoPoint instance", function () { @@ -37,6 +37,20 @@ define([ point.isValid().should.be.false; }); + it("includes a latitude-specific validation error", function () { + var point = new GeoPoint({ + latitude: 100, + longitude: 0, + height: 0 + }); + + point.isValid(); + + expect(point.validationError).to.deep.equal({ + latitude: 'Invalid latitude. Must be between -90 and 90.' + }); + }); + it("should invalidate a GeoPoint with an invalid longitude", function () { var point = new GeoPoint({ latitude: 0, @@ -46,6 +60,20 @@ define([ point.isValid().should.be.false; }); + it("includes a longitude-specific validation error", function () { + var point = new GeoPoint({ + latitude: 0, + longitude: 200, + height: 0 + }); + + point.isValid(); + + expect(point.validationError).to.deep.equal({ + longitude: 'Invalid longitude. Must be between -180 and 180.' + }); + }); + it("should invalidate a GeoPoint with an invalid height", function () { var point = new GeoPoint({ latitude: 0, @@ -54,8 +82,107 @@ define([ }); point.isValid().should.be.false; }); + + it("includes a height-specific validation error", function () { + var point = new GeoPoint({ + latitude: 0, + longitude: 0, + height: "foo" + }); + + point.isValid(); + + expect(point.validationError).to.deep.equal({ + height: 'Invalid height. Must be a number.' + }); + }); + }); + + describe('Instantiation', () => { + describe('from a good string', () => { + it('uses the user\'s search query when zooming', () => { + const geoPoint = GeoPoint.fromString('13,37'); + + expect(geoPoint.attributes.latitude).to.equal(13); + expect(geoPoint.attributes.longitude).to.equal(37); + }); + + it('accepts two space-separated numbers', () => { + const geoPoint = GeoPoint.fromString('13 37'); + + expect(geoPoint.attributes.latitude).to.equal(13); + expect(geoPoint.attributes.longitude).to.equal(37); + }); + + it('accepts input with \'-\' signs', () => { + const geoPoint = GeoPoint.fromString('13,-37'); + + expect(geoPoint.attributes.latitude).to.equal(13); + expect(geoPoint.attributes.longitude).to.equal(-37); + }); + + it('accepts input of with \'+\' signs', () => { + const geoPoint = GeoPoint.fromString('13,+37'); + + expect(geoPoint.attributes.latitude).to.equal(13); + expect(geoPoint.attributes.longitude).to.equal(37); + }); + + it('accepts input with a trailing comma', () => { + const geoPoint = GeoPoint.fromString('13,37,'); + + expect(geoPoint.attributes.latitude).to.equal(13); + expect(geoPoint.attributes.longitude).to.equal(37); + }); + }); + + describe('from a bad string', () => { + it('shows an error when only a single number is entered', () => { + expect(() => { + GeoPoint.fromString('13'); + }).to.throw(Error, /Try entering a search query with two numerical values/); + }); + + it('shows an error when non-numeric characters are entered', () => { + expect(() => { + GeoPoint.fromString('13,37a'); + }).to.throw(Error, /Try entering a search query with two numerical values/); + }); + }); }); + describe("Detecting latitude, longitude in string", () => { + it('accepts empty string', () => { + expect(GeoPoint.couldBeLatLong('')).to.be.true; + }); + + it('accepts white space in a string', () => { + expect(GeoPoint.couldBeLatLong(' 1 ')).to.be.true; + }); + + it('accepts input with a single number', () => { + expect(GeoPoint.couldBeLatLong('13')).to.be.true; + }); + + it('accepts input with floating point numbers', () => { + expect(GeoPoint.couldBeLatLong('13.0001, .0002')).to.be.true; + }); + it('accepts input with a trailing comma', () => { + expect(GeoPoint.couldBeLatLong('13,')).to.be.true; + }); + + it('accepts input with a \'-\' or \'+\'', () => { + expect(GeoPoint.couldBeLatLong('-13 +37')).to.be.true; + }); + + it('does not accept input with alpha characters', () => { + expect(GeoPoint.couldBeLatLong('13,37a')).to.be.false; + }); + + it('does not accept input with symbols', () => { + expect(GeoPoint.couldBeLatLong('13,37/')).to.be.false; + }); + }); }); }); \ No newline at end of file From 1e88cb9c7d1f7dac6ab54ffa8f746411546b73db Mon Sep 17 00:00:00 2001 From: ianguerin Date: Wed, 6 Mar 2024 08:45:57 -0800 Subject: [PATCH 2/4] Fixup: Undefined checks for GeoPoint static functions --- src/js/models/maps/GeoPoint.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/models/maps/GeoPoint.js b/src/js/models/maps/GeoPoint.js index 5a29e960d..2056f47da 100644 --- a/src/js/models/maps/GeoPoint.js +++ b/src/js/models/maps/GeoPoint.js @@ -151,7 +151,7 @@ define(["backbone", "models/maps/GeoUtilities"], function ( * @returns */ fromString(value) { - const matches = value.match(FLOATS_REGEX); + const matches = value?.match(FLOATS_REGEX); if (matches?.length !== 2 || isNaN(matches[0]) || isNaN(matches[1]) || !GeoPoint.couldBeLatLong(value)) { throw new Error( @@ -166,14 +166,14 @@ define(["backbone", "models/maps/GeoUtilities"], function ( }, /** - * Determine whether the user is could be typing a lat, long pair. + * Determine whether the user could be typing a lat, long pair. * @param {string} value is the currently entered query string. * @return {boolean} Whether the current value could be a lat,long pair * due to the string NOT containing characters (e.g. a-z) that could not * be in a lat,long pair. */ couldBeLatLong(value) { - return value.match(NON_LAT_LONG_CHARS_REGEX) == null; + return value?.match(NON_LAT_LONG_CHARS_REGEX) == null; }, } ); From 6e7892f5346feb1ac6c0f9c70087ab91bcf8cf46 Mon Sep 17 00:00:00 2001 From: ianguerin Date: Wed, 6 Mar 2024 11:23:29 -0800 Subject: [PATCH 3/4] Fixup: Migrate fromString static method to Backbone-style parse function Add type safety checks. --- src/js/models/maps/GeoPoint.js | 51 ++++++++++--------- .../specs/unit/models/maps/GeoPoint.spec.js | 30 +++++------ 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/src/js/models/maps/GeoPoint.js b/src/js/models/maps/GeoPoint.js index 2056f47da..76ae74171 100644 --- a/src/js/models/maps/GeoPoint.js +++ b/src/js/models/maps/GeoPoint.js @@ -48,6 +48,30 @@ define(["backbone", "models/maps/GeoUtilities"], function ( }; }, + /** + * Parse a string according to a regular expression. + * @param {string} value A user-entered value for parsing into a latiude + * and longitude pair. + * @throws An error indicating that more than two numbers have been + * entered. + * @returns + */ + parse(value) { + if (typeof value !== 'string') { + return new GeoPoint(); + } + + const matches = value?.match(FLOATS_REGEX); + if (matches?.length !== 2 || isNaN(matches[0]) || isNaN(matches[1]) + || !GeoPoint.couldBeLatLong(value)) { + throw new Error( + 'Try entering a search query with two numerical values representing a latitude and longitude (e.g. 64.84, -147.72).' + ); + } + + return { latitude: Number(matches[0]), longitude: Number(matches[1]) }; + }, + /** * Get the long and lat of the point as an array * @returns {Array} An array in the form [longitude, latitude] @@ -142,29 +166,6 @@ define(["backbone", "models/maps/GeoUtilities"], function ( }, }, { - /** - * Parse a string according to a regular expression. - * @param {string} value A user-entered value for parsing into a latiude - * and longitude pair. - * @throws An error indicating that more than two numbers have been - * entered. - * @returns - */ - fromString(value) { - const matches = value?.match(FLOATS_REGEX); - if (matches?.length !== 2 || isNaN(matches[0]) || isNaN(matches[1]) - || !GeoPoint.couldBeLatLong(value)) { - throw new Error( - 'Try entering a search query with two numerical values representing a latitude and longitude (e.g. 64.84, -147.72).' - ); - } - - const latitude = Number(matches[0]); - const longitude = Number(matches[1]); - - return new GeoPoint({ latitude, longitude }); - }, - /** * Determine whether the user could be typing a lat, long pair. * @param {string} value is the currently entered query string. @@ -173,6 +174,10 @@ define(["backbone", "models/maps/GeoUtilities"], function ( * be in a lat,long pair. */ couldBeLatLong(value) { + if (typeof value !== 'string') { + return false; + } + return value?.match(NON_LAT_LONG_CHARS_REGEX) == null; }, } diff --git a/test/js/specs/unit/models/maps/GeoPoint.spec.js b/test/js/specs/unit/models/maps/GeoPoint.spec.js index c1023e1c0..29b359ffb 100644 --- a/test/js/specs/unit/models/maps/GeoPoint.spec.js +++ b/test/js/specs/unit/models/maps/GeoPoint.spec.js @@ -46,9 +46,7 @@ define(["models/maps/GeoPoint",], function (GeoPoint) { point.isValid(); - expect(point.validationError).to.deep.equal({ - latitude: 'Invalid latitude. Must be between -90 and 90.' - }); + expect(point.validationError.latitude).not.to.be.empty; }); it("should invalidate a GeoPoint with an invalid longitude", function () { @@ -69,9 +67,7 @@ define(["models/maps/GeoPoint",], function (GeoPoint) { point.isValid(); - expect(point.validationError).to.deep.equal({ - longitude: 'Invalid longitude. Must be between -180 and 180.' - }); + expect(point.validationError.longitude).not.to.be.empty; }); it("should invalidate a GeoPoint with an invalid height", function () { @@ -92,44 +88,42 @@ define(["models/maps/GeoPoint",], function (GeoPoint) { point.isValid(); - expect(point.validationError).to.deep.equal({ - height: 'Invalid height. Must be a number.' - }); + expect(point.validationError.height).not.to.be.empty; }); }); describe('Instantiation', () => { describe('from a good string', () => { it('uses the user\'s search query when zooming', () => { - const geoPoint = GeoPoint.fromString('13,37'); + const geoPoint = new GeoPoint('13,37', { parse: true }); expect(geoPoint.attributes.latitude).to.equal(13); expect(geoPoint.attributes.longitude).to.equal(37); }); it('accepts two space-separated numbers', () => { - const geoPoint = GeoPoint.fromString('13 37'); + const geoPoint = new GeoPoint('13 37', { parse: true }); expect(geoPoint.attributes.latitude).to.equal(13); expect(geoPoint.attributes.longitude).to.equal(37); }); it('accepts input with \'-\' signs', () => { - const geoPoint = GeoPoint.fromString('13,-37'); + const geoPoint = new GeoPoint('13,-37', { parse: true }); expect(geoPoint.attributes.latitude).to.equal(13); expect(geoPoint.attributes.longitude).to.equal(-37); }); it('accepts input of with \'+\' signs', () => { - const geoPoint = GeoPoint.fromString('13,+37'); + const geoPoint = new GeoPoint('13,+37', { parse: true }); expect(geoPoint.attributes.latitude).to.equal(13); expect(geoPoint.attributes.longitude).to.equal(37); }); it('accepts input with a trailing comma', () => { - const geoPoint = GeoPoint.fromString('13,37,'); + const geoPoint = new GeoPoint('13,37,', { parse: true }); expect(geoPoint.attributes.latitude).to.equal(13); expect(geoPoint.attributes.longitude).to.equal(37); @@ -139,14 +133,14 @@ define(["models/maps/GeoPoint",], function (GeoPoint) { describe('from a bad string', () => { it('shows an error when only a single number is entered', () => { expect(() => { - GeoPoint.fromString('13'); - }).to.throw(Error, /Try entering a search query with two numerical values/); + new GeoPoint('13', { parse: true }); + }).to.throw(Error); }); it('shows an error when non-numeric characters are entered', () => { expect(() => { - GeoPoint.fromString('13,37a'); - }).to.throw(Error, /Try entering a search query with two numerical values/); + new GeoPoint('13,37a', { parse: true }); + }).to.throw(Error); }); }); }); From ce4a1c20d2e0b8a1bf6209772ebb192047e4e7d6 Mon Sep 17 00:00:00 2001 From: Ian Guerin Date: Mon, 11 Mar 2024 10:55:04 -0700 Subject: [PATCH 4/4] Fixup: Parse always returns Backbone-expected value Co-authored-by: Robyn <26600641+robyngit@users.noreply.github.com> --- src/js/models/maps/GeoPoint.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/models/maps/GeoPoint.js b/src/js/models/maps/GeoPoint.js index 76ae74171..a61b57704 100644 --- a/src/js/models/maps/GeoPoint.js +++ b/src/js/models/maps/GeoPoint.js @@ -58,7 +58,7 @@ define(["backbone", "models/maps/GeoUtilities"], function ( */ parse(value) { if (typeof value !== 'string') { - return new GeoPoint(); + return {}; } const matches = value?.match(FLOATS_REGEX);