Commit e2373c72 authored by Robert Knight's avatar Robert Knight

Fix default values for settings not being used in the browser extension

The logic for skipping host page configuration of settings that are
ignored by the browser extension did not take into account any default
values configured via the `defaultValue` property.

Also fix and add several related tests:

 - Correct the test that checks behavior when `defaultValue` is not set
   to use `undefined` rather than `null` to indicate a missing
   `defaultValue`.

 - Add test case for when a default value is overridden.

 - Add test case for default values in browser extensions.
parent 2365de71
...@@ -139,9 +139,10 @@ function settingsFrom(window_) { ...@@ -139,9 +139,10 @@ function settingsFrom(window_) {
function hostPageSetting(name, options = {}) { function hostPageSetting(name, options = {}) {
var allowInBrowserExt = options.allowInBrowserExt || false; var allowInBrowserExt = options.allowInBrowserExt || false;
var hasDefaultValue = typeof options.defaultValue !== 'undefined';
if (!allowInBrowserExt && isBrowserExtension(sidebarAppUrl())) { if (!allowInBrowserExt && isBrowserExtension(sidebarAppUrl())) {
return null; return hasDefaultValue ? options.defaultValue : null;
} }
if (configFuncSettings.hasOwnProperty(name)) { if (configFuncSettings.hasOwnProperty(name)) {
...@@ -152,7 +153,7 @@ function settingsFrom(window_) { ...@@ -152,7 +153,7 @@ function settingsFrom(window_) {
return jsonConfigs[name]; return jsonConfigs[name];
} }
if (typeof options.defaultValue !== 'undefined') { if (hasDefaultValue) {
return options.defaultValue; return options.defaultValue;
} }
......
...@@ -514,17 +514,17 @@ describe('annotator.config.settingsFrom', function() { ...@@ -514,17 +514,17 @@ describe('annotator.config.settingsFrom', function() {
expected: 'jsonValue', expected: 'jsonValue',
}, },
{ {
when: 'the defaultValue is null', when: 'no default value is provided',
specify: 'it returns null', specify: 'it returns null',
isBrowserExtension: false, isBrowserExtension: false,
allowInBrowserExt: false, allowInBrowserExt: false,
configFuncSettings: {}, configFuncSettings: {},
jsonSettings: {}, jsonSettings: {},
defaultValue: null, defaultValue: undefined,
expected: null, expected: null,
}, },
{ {
when: 'the defaultValue is specified', when: 'a default value is provided',
specify: 'it returns that default value', specify: 'it returns that default value',
isBrowserExtension: false, isBrowserExtension: false,
allowInBrowserExt: false, allowInBrowserExt: false,
...@@ -533,6 +533,26 @@ describe('annotator.config.settingsFrom', function() { ...@@ -533,6 +533,26 @@ describe('annotator.config.settingsFrom', function() {
defaultValue: 'test value', defaultValue: 'test value',
expected: 'test value', expected: 'test value',
}, },
{
when: 'a default value is provided but it is overridden',
specify: 'it returns the overridden value',
isBrowserExtension: false,
allowInBrowserExt: false,
configFuncSettings: { foo: 'not the default value' },
jsonSettings: {},
defaultValue: 'the default value',
expected: 'not the default value',
},
{
when: 'the client is in a browser extension and a default value is provided',
specify: 'it returns the default value',
isBrowserExtension: true,
allowInBrowserExt: false,
configFuncSettings: { foo: 'ignore me' },
jsonSettings: { foo: 'also ignore me' },
defaultValue: 'the default value',
expected: 'the default value',
},
].forEach(function(test) { ].forEach(function(test) {
context(test.when, function() { context(test.when, function() {
specify(test.specify, function() { specify(test.specify, function() {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment