Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move lat, long parsing and validations logic into GeoPoint #2290

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/js/models/maps/GeoPoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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." };
}
},
},
{
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
/**
* Parse a string according to a regular expression.
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
* @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);
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
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.
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
* @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;
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
},
}
);
Expand Down
137 changes: 132 additions & 5 deletions test/js/specs/unit/models/maps/GeoPoint.spec.js
Original file line number Diff line number Diff line change
@@ -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 () {
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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.'
});
ianguerin marked this conversation as resolved.
Show resolved Hide resolved
});
});

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;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why could an empty string be a lat, long pair? maybe it's just the name of the function that is confusing me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean for this function to disqualify strings from being a lat,long pair and not necessarily that they are a lat,long pair. For example, "13" could be a lat,long pair, because it doesn't have any disqualifying characters in it, though it is not a valid lat,long pair.


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;
});
});
});
});
Loading