From 3367e601103b6e375e5c2d0398b37e31415a9a36 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 8 Dec 2023 15:47:12 -0500 Subject: [PATCH 01/11] make labelVars as optional in AppConfig type def While looking at dynamicConfig.ts I realized `labelVars` should be optional - if the `labelTemplate` doesn't reference any vars, `labelVars` can be omitted. --- www/js/types/appConfigTypes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/www/js/types/appConfigTypes.ts b/www/js/types/appConfigTypes.ts index 1a2e50722..2f258b575 100644 --- a/www/js/types/appConfigTypes.ts +++ b/www/js/types/appConfigTypes.ts @@ -19,7 +19,7 @@ export type EnketoSurveyConfig = { [surveyName: string]: { formPath: string; labelTemplate: { [lang: string]: string }; - labelVars: { [activity: string]: { [key: string]: string; type: string } }; + labelVars?: { [activity: string]: { [key: string]: string; type: string } }; version: number; compatibleWith: number; dataKey?: string; From 8c31b655b6e867b61c5bd621a6249a95295aa560 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 8 Dec 2023 15:57:16 -0500 Subject: [PATCH 02/11] fix backwards compat on retrieved configs The fix is in getConfig(), lines 257 and 265 - _backwardsCompatSurveyFill needed to be applied there. But while I was here, something else I noticed is that these functions mutate the config object parameter, which means they are not pure functions. For best practice, I changed them so they return a new object with the change rather than mutating their inputs. While I was doing that, I applied the AppConfig type definition we have declared in types/appConfigTypes.ts. And I cleaned up the log statements in readConfigFromServer() --- www/js/config/dynamicConfig.ts | 61 +++++++++++++++++----------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index 801e24b07..9c099a6d1 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -2,6 +2,7 @@ import i18next from 'i18next'; import { displayError, logDebug, logWarn } from '../plugin/logger'; import { fetchUrlCached } from '../services/commHelper'; import { storageClear, storageGet, storageSet } from '../plugin/storage'; +import { AppConfig } from '../types/appConfigTypes'; export const CONFIG_PHONE_UI = 'config/app_ui_config'; export const CONFIG_PHONE_UI_KVSTORE = 'CONFIG_PHONE_UI'; @@ -29,19 +30,20 @@ const _getStudyName = function (connectUrl) { return study_name; }; -const _fillStudyName = function (config) { - if (!config.name) { - if (config.server) { - config.name = _getStudyName(config.server.connectUrl); - } else { - config.name = 'dev'; - } +const _fillStudyName = (config: Partial): AppConfig => { + if (config.name) return config as AppConfig; + if (config.server) { + return { ...config, name: _getStudyName(config.server.connectUrl) } as AppConfig; + } else { + return { ...config, name: 'dev' } as AppConfig; } }; -const _backwardsCompatSurveyFill = function (config) { - if (!config.survey_info) { - config.survey_info = { +const _backwardsCompatSurveyFill = (config: Partial): AppConfig => { + if (config.survey_info) return config as AppConfig; + return { + ...config, + survey_info: { surveys: { UserProfileSurvey: { formPath: 'json/demo-survey-v2.json', @@ -55,8 +57,8 @@ const _backwardsCompatSurveyFill = function (config) { }, }, 'trip-labels': 'MULTILABEL', - }; - } + }, + } as AppConfig; }; /* Fetch and cache any surveys resources that are referenced by URL in the config, @@ -76,25 +78,22 @@ const cacheResourcesFromConfig = (config) => { }; const readConfigFromServer = async (label) => { - const config = await fetchConfig(label); - logDebug('Successfully found config, result is ' + JSON.stringify(config).substring(0, 10)); + const fetchedConfig = await fetchConfig(label); + logDebug(`Successfully found config, + fetchedConfig = ${JSON.stringify(fetchedConfig).substring(0, 10)}`); + const filledConfig = _backwardsCompatSurveyFill(_fillStudyName(fetchedConfig)); + logDebug(`Applied backwards compat fills, + filledConfig = ${JSON.stringify(filledConfig).substring(0, 10)}`); // fetch + cache any resources referenced in the config, but don't 'await' them so we don't block // the config loading process - cacheResourcesFromConfig(config); + cacheResourcesFromConfig(filledConfig); - const connectionURL = config.server ? config.server.connectUrl : 'dev defaults'; - _fillStudyName(config); - _backwardsCompatSurveyFill(config); - logDebug( - 'Successfully downloaded config with version ' + - config.version + - ' for ' + - config.intro.translated_text.en.deployment_name + - ' and data collection URL ' + - connectionURL, - ); - return config; + logDebug(`Successfully read config, returning config with + version = ${filledConfig.version}; + deployment_name = ${filledConfig.intro.translated_text.en.deployment_name}; + connectionURL = ${fetchedConfig.server ? fetchedConfig.server.connectUrl : 'dev defaults'}`); + return filledConfig; }; const fetchConfig = async (label, alreadyTriedLocal = false) => { @@ -255,16 +254,16 @@ export function getConfig() { return storageGet(CONFIG_PHONE_UI_KVSTORE).then((config) => { if (config && Object.keys(config).length) { logDebug('Got config from KVStore: ' + JSON.stringify(config)); - storedConfig = config; - return config; + storedConfig = _backwardsCompatSurveyFill(config); + return storedConfig; } logDebug('No config found in KVStore, fetching from native storage'); return window['cordova'].plugins.BEMUserCache.getDocument(CONFIG_PHONE_UI, false).then( (config) => { if (config && Object.keys(config).length) { logDebug('Got config from native storage: ' + JSON.stringify(config)); - storedConfig = config; - return config; + storedConfig = _backwardsCompatSurveyFill(config); + return storedConfig; } logWarn('No config found in native storage either. Returning null'); return null; From 05275693c5fbb2f2efa7cba060969dc0ce9a4760 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 8 Dec 2023 23:39:32 -0500 Subject: [PATCH 03/11] dynamicConfig: use 'function' syntax for big fns To be more consistent with the rest of the codebase, as a rule of thumb we're having top-level functions use 'function' synax while one-liners or nested functions are arrow functions. Also, resetStoredConfig was renamed to _test_resetStoredConfig because that change is coming anyway in https://github.com/e-mission/e-mission-phone/pull/1113 and this will make it easier to resolve merge conflicts --- www/__tests__/enketoHelper.test.ts | 4 ++-- www/js/config/dynamicConfig.ts | 28 ++++++++++++++-------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/www/__tests__/enketoHelper.test.ts b/www/__tests__/enketoHelper.test.ts index c4fda7dc4..ee5529199 100644 --- a/www/__tests__/enketoHelper.test.ts +++ b/www/__tests__/enketoHelper.test.ts @@ -8,7 +8,7 @@ import { } from '../js/survey/enketo/enketoHelper'; import { mockBEMUserCache } from '../__mocks__/cordovaMocks'; import { mockLogger } from '../__mocks__/globalMocks'; -import { getConfig, resetStoredConfig } from '../../www/js/config/dynamicConfig'; +import { getConfig, _test_resetStoredConfig } from '../../www/js/config/dynamicConfig'; import fakeConfig from '../__mocks__/fakeConfig.json'; import initializedI18next from '../js/i18nextInit'; @@ -21,7 +21,7 @@ global.URL = require('url').URL; global.Blob = require('node:buffer').Blob; beforeEach(() => { - resetStoredConfig(); + _test_resetStoredConfig(); }); it('gets the survey config', async () => { diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index 9c099a6d1..9312e6def 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -11,12 +11,12 @@ export let storedConfig = null; export let configChanged = false; export const setConfigChanged = (b) => (configChanged = b); -//used test multiple configs, not used outside of test -export const resetStoredConfig = function () { +// used to test multiple configs, not used outside of test +export const _test_resetStoredConfig = () => { storedConfig = null; }; -const _getStudyName = function (connectUrl) { +function _getStudyName(connectUrl) { const orig_host = new URL(connectUrl).hostname; const first_domain = orig_host.split('.')[0]; if (first_domain == 'openpath-stage') { @@ -28,18 +28,18 @@ const _getStudyName = function (connectUrl) { } const study_name = first_domain.substr(0, openpath_index); return study_name; -}; +} -const _fillStudyName = (config: Partial): AppConfig => { +function _fillStudyName(config: Partial): AppConfig { if (config.name) return config as AppConfig; if (config.server) { return { ...config, name: _getStudyName(config.server.connectUrl) } as AppConfig; } else { return { ...config, name: 'dev' } as AppConfig; } -}; +} -const _backwardsCompatSurveyFill = (config: Partial): AppConfig => { +function _backwardsCompatSurveyFill(config: Partial): AppConfig { if (config.survey_info) return config as AppConfig; return { ...config, @@ -59,13 +59,13 @@ const _backwardsCompatSurveyFill = (config: Partial): AppConfig => { 'trip-labels': 'MULTILABEL', }, } as AppConfig; -}; +} /* Fetch and cache any surveys resources that are referenced by URL in the config, as well as the label_options config if it is present. This way they will be available when the user needs them, and we won't have to fetch them again unless local storage is cleared. */ -const cacheResourcesFromConfig = (config) => { +function cacheResourcesFromConfig(config) { if (config.survey_info?.surveys) { Object.values(config.survey_info.surveys).forEach((survey) => { if (!survey?.['formPath']) throw new Error(i18next.t('config.survey-missing-formpath')); @@ -75,9 +75,9 @@ const cacheResourcesFromConfig = (config) => { if (config.label_options) { fetchUrlCached(config.label_options); } -}; +} -const readConfigFromServer = async (label) => { +async function readConfigFromServer(label) { const fetchedConfig = await fetchConfig(label); logDebug(`Successfully found config, fetchedConfig = ${JSON.stringify(fetchedConfig).substring(0, 10)}`); @@ -94,9 +94,9 @@ const readConfigFromServer = async (label) => { deployment_name = ${filledConfig.intro.translated_text.en.deployment_name}; connectionURL = ${fetchedConfig.server ? fetchedConfig.server.connectUrl : 'dev defaults'}`); return filledConfig; -}; +} -const fetchConfig = async (label, alreadyTriedLocal = false) => { +async function fetchConfig(label, alreadyTriedLocal = false) { logDebug('Received request to join ' + label); const downloadURL = `https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/${label}.nrel-op.json`; if (!__DEV__ || alreadyTriedLocal) { @@ -115,7 +115,7 @@ const fetchConfig = async (label, alreadyTriedLocal = false) => { return fetchConfig(label, true); } } -}; +} /* * We want to support both old style and new style tokens. From b011bcae895bb5966fb0e52ceb4379ee46c82672 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Sat, 9 Dec 2023 00:29:28 -0500 Subject: [PATCH 04/11] add JSdoc and types to fns in dynamicConfigs.ts --- www/js/config/dynamicConfig.ts | 94 +++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 30 deletions(-) diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index 9312e6def..d0e1cb2c7 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -16,7 +16,12 @@ export const _test_resetStoredConfig = () => { storedConfig = null; }; -function _getStudyName(connectUrl) { +/** + * @param connectUrl The URL endpoint specified in the config + * @returns The study name (like 'stage' or whatever precedes 'openpath' in the URL), + * or undefined if it can't be determined + */ +function _getStudyName(connectUrl: `https://${string}`) { const orig_host = new URL(connectUrl).hostname; const first_domain = orig_host.split('.')[0]; if (first_domain == 'openpath-stage') { @@ -30,6 +35,10 @@ function _getStudyName(connectUrl) { return study_name; } +/** + * @param config The app config which might be missing 'name' + * @returns Shallow copy of the app config with 'name' filled in if it was missing + */ function _fillStudyName(config: Partial): AppConfig { if (config.name) return config as AppConfig; if (config.server) { @@ -39,6 +48,10 @@ function _fillStudyName(config: Partial): AppConfig { } } +/** + * @param config The app config which might be missing 'survey_info' + * @returns Shallow copy of the app config with the default 'survey_info' filled in if it was missing + */ function _backwardsCompatSurveyFill(config: Partial): AppConfig { if (config.survey_info) return config as AppConfig; return { @@ -61,11 +74,14 @@ function _backwardsCompatSurveyFill(config: Partial): AppConfig { } as AppConfig; } -/* Fetch and cache any surveys resources that are referenced by URL in the config, - as well as the label_options config if it is present. - This way they will be available when the user needs them, and we won't have to - fetch them again unless local storage is cleared. */ -function cacheResourcesFromConfig(config) { +/** + * @description Fetch and cache any surveys resources that are referenced by URL in the config, + * as well as the label_options config if it is present. + * This way they will be available when the user needs them, and we won't have to + * fetch them again unless local storage is cleared. + * @param config The app config + */ +function cacheResourcesFromConfig(config: AppConfig) { if (config.survey_info?.surveys) { Object.values(config.survey_info.surveys).forEach((survey) => { if (!survey?.['formPath']) throw new Error(i18next.t('config.survey-missing-formpath')); @@ -77,8 +93,14 @@ function cacheResourcesFromConfig(config) { } } -async function readConfigFromServer(label) { - const fetchedConfig = await fetchConfig(label); +/** + * @description Fetch the config from the server, fill in any missing fields, and cache any + * resources referenced in the config + * @param studyLabel The 'label' of the study, like 'open-access' or 'dev-emulator-study' + * @returns The filled in app config + */ +async function readConfigFromServer(studyLabel: string) { + const fetchedConfig = await fetchConfig(studyLabel); logDebug(`Successfully found config, fetchedConfig = ${JSON.stringify(fetchedConfig).substring(0, 10)}`); const filledConfig = _backwardsCompatSurveyFill(_fillStudyName(fetchedConfig)); @@ -96,23 +118,30 @@ async function readConfigFromServer(label) { return filledConfig; } -async function fetchConfig(label, alreadyTriedLocal = false) { - logDebug('Received request to join ' + label); - const downloadURL = `https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/${label}.nrel-op.json`; +/** + * @description Fetch the config for a particular study, either from github, or if in dev mode, from + * localhost:9090 and github if that fails + * @param studyLabel The 'label' of the study, like 'open-access' or 'dev-emulator-study' + * @param alreadyTriedLocal Flag for dev environment, if true, will try to fetch from github + * @returns The fetched app config + */ +async function fetchConfig(studyLabel: string, alreadyTriedLocal?: boolean) { + logDebug('Received request to join ' + studyLabel); + const downloadURL = `https://raw.githubusercontent.com/e-mission/nrel-openpath-deploy-configs/main/configs/${studyLabel}.nrel-op.json`; if (!__DEV__ || alreadyTriedLocal) { logDebug('Fetching config from github'); const r = await fetch(downloadURL); if (!r.ok) throw new Error('Unable to fetch config from github'); - return r.json(); + return r.json(); // TODO: validate, make sure it has required fields } else { logDebug('Running in dev environment, checking for locally hosted config'); try { - const r = await fetch('http://localhost:9090/configs/' + label + '.nrel-op.json'); + const r = await fetch('http://localhost:9090/configs/' + studyLabel + '.nrel-op.json'); if (!r.ok) throw new Error('Local config not found'); return r.json(); } catch (err) { logDebug('Local config not found'); - return fetchConfig(label, true); + return fetchConfig(studyLabel, true); } } } @@ -129,7 +158,7 @@ async function fetchConfig(label, alreadyTriedLocal = false) { * * So let's support two separate functions here - extractStudyName and extractSubgroup */ -function extractStudyName(token) { +function extractStudyName(token: string): string { const tokenParts = token.split('_'); if (tokenParts.length < 3) { // all tokens must have at least nrelop_[study name]_... @@ -141,7 +170,7 @@ function extractStudyName(token) { return tokenParts[1]; } -function extractSubgroup(token, config) { +function extractSubgroup(token: string, config: AppConfig): string { if (config.opcode) { // new style study, expects token with sub-group const tokenParts = token.split('_'); @@ -185,13 +214,12 @@ function extractSubgroup(token, config) { } /** - * loadNewConfig download and load a new config from the server if it is a differ - * @param {[]} newToken the new token, which includes parts for the study label, subgroup, and user - * @param {} thenGoToIntro whether to go to the intro screen after loading the config - * @param {} [existingVersion=null] if the new config's version is the same, we won't update - * @returns {boolean} boolean representing whether the config was updated or not + * @description Download and load a new config from the server if it is a different version + * @param newToken The new token, which includes parts for the study label, subgroup, and user + * @param existingVersion If the new config's version is the same, we won't update + * @returns boolean representing whether the config was updated or not */ -function loadNewConfig(newToken, existingVersion = null) { +function loadNewConfig(newToken: string, existingVersion?: number): Promise { const newStudyLabel = extractStudyName(newToken); return readConfigFromServer(newStudyLabel) .then((downloadedConfig) => { @@ -223,16 +251,19 @@ function loadNewConfig(newToken, existingVersion = null) { configChanged = true; return true; }) - .catch((storeError) => - displayError(storeError, i18next.t('config.unable-to-store-config')), - ); + .catch((storeError) => { + displayError(storeError, i18next.t('config.unable-to-store-config')); + return false; + }); }) .catch((fetchErr) => { displayError(fetchErr, i18next.t('config.unable-download-config')); + return false; }); } -export function initByUser(urlComponents) { +// exported wrapper around loadNewConfig that includes error handling +export function initByUser(urlComponents: { token: string }) { const { token } = urlComponents; try { return loadNewConfig(token).catch((fetchErr) => { @@ -244,12 +275,15 @@ export function initByUser(urlComponents) { } } -export function resetDataAndRefresh() { - // const resetNativePromise = window['cordova'].plugins.BEMUserCache.putRWDocument(CONFIG_PHONE_UI, {}); +/** @description Clears all local and native storage, then triggers a refresh */ +export const resetDataAndRefresh = () => storageClear({ local: true, native: true }).then(() => window.location.reload()); -} -export function getConfig() { +/** + * @returns The app config, either from a cached copy, retrieved from local storage, or retrieved + * from user cache with getDocument() + */ +export function getConfig(): Promise { if (storedConfig) return Promise.resolve(storedConfig); return storageGet(CONFIG_PHONE_UI_KVSTORE).then((config) => { if (config && Object.keys(config).length) { From 113ed82e67ad09ff12e4464a41fc0502d4ed02e1 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 14 Dec 2023 18:05:18 -0500 Subject: [PATCH 05/11] handle errors better while fetching/loading config --- www/js/config/dynamicConfig.ts | 7 ++++--- www/js/services/commHelper.ts | 18 +++++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index d0e1cb2c7..6fecfaf80 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -113,7 +113,7 @@ async function readConfigFromServer(studyLabel: string) { logDebug(`Successfully read config, returning config with version = ${filledConfig.version}; - deployment_name = ${filledConfig.intro.translated_text.en.deployment_name}; + deployment_name = ${filledConfig.intro?.translated_text?.en?.deployment_name}; connectionURL = ${fetchedConfig.server ? fetchedConfig.server.connectUrl : 'dev defaults'}`); return filledConfig; } @@ -253,12 +253,12 @@ function loadNewConfig(newToken: string, existingVersion?: number): Promise { displayError(storeError, i18next.t('config.unable-to-store-config')); - return false; + return Promise.reject(storeError); }); }) .catch((fetchErr) => { displayError(fetchErr, i18next.t('config.unable-download-config')); - return false; + return Promise.reject(fetchErr); }); } @@ -268,6 +268,7 @@ export function initByUser(urlComponents: { token: string }) { try { return loadNewConfig(token).catch((fetchErr) => { displayError(fetchErr, i18next.t('config.unable-download-config')); + return Promise.reject(fetchErr); }); } catch (error) { displayError(error, i18next.t('config.invalid-opcode-format')); diff --git a/www/js/services/commHelper.ts b/www/js/services/commHelper.ts index 206fc77c1..215f3fed8 100644 --- a/www/js/services/commHelper.ts +++ b/www/js/services/commHelper.ts @@ -1,5 +1,5 @@ import { DateTime } from 'luxon'; -import { logDebug } from '../plugin/logger'; +import { displayError, logDebug } from '../plugin/logger'; import { ServerConnConfig } from '../types/appConfigTypes'; /** @@ -12,12 +12,16 @@ export async function fetchUrlCached(url) { logDebug(`fetchUrlCached: found cached data for url ${url}, returning`); return Promise.resolve(stored); } - logDebug(`fetchUrlCached: found no cached data for url ${url}, fetching`); - const response = await fetch(url); - const text = await response.text(); - localStorage.setItem(url, text); - logDebug(`fetchUrlCached: fetched data for url ${url}, returning`); - return text; + try { + logDebug(`fetchUrlCached: found no cached data for url ${url}, fetching`); + const response = await fetch(url); + const text = await response.text(); + localStorage.setItem(url, text); + logDebug(`fetchUrlCached: fetched data for url ${url}, returning`); + return text; + } catch (e) { + displayError(e, `While fetching ${url}`); + } } export function getRawEntries( From a99af4d12b73335a4913b545898ec58f998968d2 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 14 Dec 2023 18:07:10 -0500 Subject: [PATCH 06/11] mockBEMUserCache: putRWDocument for storing config This will allow us to test how configs get stored (and then retrieved) --- www/__mocks__/cordovaMocks.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/www/__mocks__/cordovaMocks.ts b/www/__mocks__/cordovaMocks.ts index 1f563c7e7..0b00f8662 100644 --- a/www/__mocks__/cordovaMocks.ts +++ b/www/__mocks__/cordovaMocks.ts @@ -101,6 +101,16 @@ export const mockBEMUserCache = (config?) => { }, 100), ); }, + putRWDocument: (key: string, value: any) => { + if (key == 'config/app_ui_config') { + return new Promise((rs, rj) => + setTimeout(() => { + config = value; + rs(); + }, 100), + ); + } + }, getDocument: (key: string, withMetadata?: boolean) => { //returns the config provided as a paramenter to this mock! if (key == 'config/app_ui_config') { From b6a01f14892e6b4277a00e7271bfaff5f1425adc Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 14 Dec 2023 18:09:04 -0500 Subject: [PATCH 07/11] config backwards compats as one function Instead of applying the backwards compat to fill 'survey_info' and 'name' by 2 function calls every time it needs to be done, let's have one function that does both --- www/js/config/dynamicConfig.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index 6fecfaf80..16e6864c3 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -52,7 +52,7 @@ function _fillStudyName(config: Partial): AppConfig { * @param config The app config which might be missing 'survey_info' * @returns Shallow copy of the app config with the default 'survey_info' filled in if it was missing */ -function _backwardsCompatSurveyFill(config: Partial): AppConfig { +function _fillSurveyInfo(config: Partial): AppConfig { if (config.survey_info) return config as AppConfig; return { ...config, @@ -74,6 +74,13 @@ function _backwardsCompatSurveyFill(config: Partial): AppConfig { } as AppConfig; } +/** + * @description Fill in any fields that might be missing from the config ('name', 'survey_info') for + * backwards compatibility with old configs + */ +const _backwardsCompatFill = (config: Partial): AppConfig => + _fillSurveyInfo(_fillStudyName(config)); + /** * @description Fetch and cache any surveys resources that are referenced by URL in the config, * as well as the label_options config if it is present. @@ -103,7 +110,7 @@ async function readConfigFromServer(studyLabel: string) { const fetchedConfig = await fetchConfig(studyLabel); logDebug(`Successfully found config, fetchedConfig = ${JSON.stringify(fetchedConfig).substring(0, 10)}`); - const filledConfig = _backwardsCompatSurveyFill(_fillStudyName(fetchedConfig)); + const filledConfig = _backwardsCompatFill(fetchedConfig); logDebug(`Applied backwards compat fills, filledConfig = ${JSON.stringify(filledConfig).substring(0, 10)}`); @@ -289,7 +296,7 @@ export function getConfig(): Promise { return storageGet(CONFIG_PHONE_UI_KVSTORE).then((config) => { if (config && Object.keys(config).length) { logDebug('Got config from KVStore: ' + JSON.stringify(config)); - storedConfig = _backwardsCompatSurveyFill(config); + storedConfig = _backwardsCompatFill(config); return storedConfig; } logDebug('No config found in KVStore, fetching from native storage'); @@ -297,7 +304,7 @@ export function getConfig(): Promise { (config) => { if (config && Object.keys(config).length) { logDebug('Got config from native storage: ' + JSON.stringify(config)); - storedConfig = _backwardsCompatSurveyFill(config); + storedConfig = _backwardsCompatFill(config); return storedConfig; } logWarn('No config found in native storage either. Returning null'); From 6676dc562d99ce1ee68322891d2203d74f388932 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 15 Dec 2023 01:33:12 -0500 Subject: [PATCH 08/11] only use fakeConfig if passed to mockBEMUserCache The dynamic config tests will want to actually test how the config is stored, so we don't want this to always fallback to some default 'fakeConfig'. Before the config gets stored we'll want to be able to tell if it's missing or not. We'll remove that fallback, and any test that *does* want to use that 'fakeConfig' can pass it as an argument to the mockBEMUserCache() function --- www/__mocks__/cordovaMocks.ts | 4 ++-- www/__tests__/customMetricsHelper.test.ts | 3 ++- www/__tests__/footprintHelper.test.ts | 3 ++- www/__tests__/metHelper.test.ts | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/www/__mocks__/cordovaMocks.ts b/www/__mocks__/cordovaMocks.ts index 0b00f8662..33c3f2a1f 100644 --- a/www/__mocks__/cordovaMocks.ts +++ b/www/__mocks__/cordovaMocks.ts @@ -1,5 +1,4 @@ import packageJsonBuild from '../../package.cordovabuild.json'; -import fakeConfig from './fakeConfig.json'; export const mockCordova = () => { window['cordova'] ||= {}; @@ -116,7 +115,8 @@ export const mockBEMUserCache = (config?) => { if (key == 'config/app_ui_config') { return new Promise((rs, rj) => setTimeout(() => { - rs(config || fakeConfig); + if (config) rs(config); + else rj('no config'); }, 100), ); } else { diff --git a/www/__tests__/customMetricsHelper.test.ts b/www/__tests__/customMetricsHelper.test.ts index 0ae025bff..8660fcb82 100644 --- a/www/__tests__/customMetricsHelper.test.ts +++ b/www/__tests__/customMetricsHelper.test.ts @@ -7,8 +7,9 @@ import { import { mockBEMUserCache } from '../__mocks__/cordovaMocks'; import { mockLogger } from '../__mocks__/globalMocks'; import fakeLabels from '../__mocks__/fakeLabels.json'; +import fakeConfig from '../__mocks__/fakeConfig.json'; -mockBEMUserCache(); +mockBEMUserCache(fakeConfig); mockLogger(); global.fetch = (url: string) => diff --git a/www/__tests__/footprintHelper.test.ts b/www/__tests__/footprintHelper.test.ts index 842442153..6b1841371 100644 --- a/www/__tests__/footprintHelper.test.ts +++ b/www/__tests__/footprintHelper.test.ts @@ -9,8 +9,9 @@ import { getConfig } from '../js/config/dynamicConfig'; import { mockBEMUserCache } from '../__mocks__/cordovaMocks'; import { mockLogger } from '../__mocks__/globalMocks'; import fakeLabels from '../__mocks__/fakeLabels.json'; +import fakeConfig from '../__mocks__/fakeConfig.json'; -mockBEMUserCache(); +mockBEMUserCache(fakeConfig); mockLogger(); global.fetch = (url: string) => diff --git a/www/__tests__/metHelper.test.ts b/www/__tests__/metHelper.test.ts index bc477daa0..2419100ae 100644 --- a/www/__tests__/metHelper.test.ts +++ b/www/__tests__/metHelper.test.ts @@ -4,8 +4,9 @@ import { mockLogger } from '../__mocks__/globalMocks'; import fakeLabels from '../__mocks__/fakeLabels.json'; import { getConfig } from '../js/config/dynamicConfig'; import { initCustomDatasetHelper } from '../js/metrics/customMetricsHelper'; +import fakeConfig from '../__mocks__/fakeConfig.json'; -mockBEMUserCache(); +mockBEMUserCache(fakeConfig); mockLogger(); global.fetch = (url: string) => From 870bee25d13f51f9800e8386cadbbd9e9509b1bb Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 15 Dec 2023 01:34:14 -0500 Subject: [PATCH 09/11] initial tests for dynamicConfig.test.ts Tests for the biggest functions: getConfig and initByUser Going to check coverage and see whether it's beneficial to test the smaller (helper) functions too --- www/__tests__/dynamicConfig.test.ts | 92 +++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 www/__tests__/dynamicConfig.test.ts diff --git a/www/__tests__/dynamicConfig.test.ts b/www/__tests__/dynamicConfig.test.ts new file mode 100644 index 000000000..30dd5780f --- /dev/null +++ b/www/__tests__/dynamicConfig.test.ts @@ -0,0 +1,92 @@ +import { mockBEMUserCache } from '../__mocks__/cordovaMocks'; +import { mockAlert, mockLogger } from '../__mocks__/globalMocks'; +import { getConfig, initByUser } from '../js/config/dynamicConfig'; + +import initializedI18next from '../js/i18nextInit'; +window['i18next'] = initializedI18next; + +mockLogger(); +mockAlert(); +mockBEMUserCache(); + +global.fetch = (url: string) => { + return new Promise((rs, rj) => { + if (url.includes('nrel-commute.nrel-op.json')) { + rs({ + ok: true, + json: () => + new Promise((rs, rj) => + rs({ + version: 1, + ts: 1655143472, + server: { + connectUrl: 'https://nrel-commute-openpath.nrel.gov/api/', + aggregate_call_auth: 'user_only', + }, + // ... more config would follow but it's good enough for this mock + }), + ), + }); + } + }) as any; +}; + +const fakeBaseConfig = { + version: 1, + server: { + connectUrl: 'https://www.example.com', + aggregate_call_auth: 'no_auth', + }, +}; +const fakeConfigWithSurveys = { + ...fakeBaseConfig, + survey_info: { + surveys: { + CommuteSurvey: { + formPath: 'https://www.example.com', + labelTemplate: { en: 'Trip to {{destination}}' }, + labelVars: { walking: { destination: 'work' } }, + version: 1, + compatibleWith: 1, + dataKey: 'commute', + }, + }, + 'trip-labels': 'MULTILABEL', + }, +}; + +describe('dynamicConfig', () => { + const fakeStudyName = 'gotham-city-transit'; + const validStudyName = 'nrel-commute'; + + describe('getConfig', () => { + it('should reject since no config is set yet', () => { + expect(getConfig()).rejects.toThrow(); + }); + it('should resolve with a config once initByUser is called', async () => { + const validToken = `nrelop_${validStudyName}_user1`; + await initByUser({ token: validToken }); + const config = await getConfig(); + expect(config.server.connectUrl).toBe('https://nrel-commute-openpath.nrel.gov/api/'); + }); + }); + + describe('initByUser', () => { + it('should error if the study is nonexistent', () => { + const fakeBatmanToken = `nrelop_${fakeStudyName}_batman`; + expect(initByUser({ token: fakeBatmanToken })).rejects.toThrow(); + }); + it('should error if the study exists but the token is invalid format', () => { + const badToken1 = validStudyName; + expect(initByUser({ token: badToken1 })).rejects.toThrow(); + const badToken2 = `nrelop_${validStudyName}`; + expect(initByUser({ token: badToken2 })).rejects.toThrow(); + const badToken3 = `nrelop_${validStudyName}_`; + expect(initByUser({ token: badToken3 })).rejects.toThrow(); + }); + it('should return the config for valid token', () => { + const validToken = `nrelop_${validStudyName}_user2`; + expect(initByUser({ token: validToken })).resolves.toBeTruthy(); + }); + }); +}); From 9bc5cd6389c21f8408b769f2abfdbc9c0d377d28 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 15 Dec 2023 13:43:16 -0500 Subject: [PATCH 10/11] update dynamicConfig.test.ts - test initByUser and getConfig using stub configs for 'nrel-commute' (which is old-style / does not have subgroups) and 'denver-casr' (which is new style with subgroups) - check that the 'joined' field gets filled in - isolate tests from one another by using await properly on async tests and by clearing storage in between test runs --- www/__mocks__/cordovaMocks.ts | 2 +- www/__tests__/dynamicConfig.test.ts | 148 ++++++++++++++++++---------- 2 files changed, 96 insertions(+), 54 deletions(-) diff --git a/www/__mocks__/cordovaMocks.ts b/www/__mocks__/cordovaMocks.ts index 33c3f2a1f..209e38b23 100644 --- a/www/__mocks__/cordovaMocks.ts +++ b/www/__mocks__/cordovaMocks.ts @@ -116,7 +116,7 @@ export const mockBEMUserCache = (config?) => { return new Promise((rs, rj) => setTimeout(() => { if (config) rs(config); - else rj('no config'); + else rs({}); // return empty object if config is not set }, 100), ); } else { diff --git a/www/__tests__/dynamicConfig.test.ts b/www/__tests__/dynamicConfig.test.ts index 30dd5780f..12fec433b 100644 --- a/www/__tests__/dynamicConfig.test.ts +++ b/www/__tests__/dynamicConfig.test.ts @@ -3,90 +3,132 @@ import { mockAlert, mockLogger } from '../__mocks__/globalMocks'; import { getConfig, initByUser } from '../js/config/dynamicConfig'; import initializedI18next from '../js/i18nextInit'; +import { storageClear } from '../js/plugin/storage'; window['i18next'] = initializedI18next; mockLogger(); mockAlert(); mockBEMUserCache(); +beforeEach(() => { + // clear all storage and the config document + storageClear({ local: true, native: true }); + window['cordova'].plugins.BEMUserCache.putRWDocument('config/app_ui_config', {}); +}); + +const nrelCommuteConfig = { + version: 1, + server: { + connectUrl: 'https://nrel-commute-openpath.nrel.gov/api/', + aggregate_call_auth: 'user_only', + }, + // ... +}; + +const denverCasrConfig = { + version: 1, + server: { + connectUrl: 'https://denver-casr-openpath.nrel.gov/api/', + aggregate_call_auth: 'user_only', + }, + opcode: { + autogen: true, + subgroups: [ + 'test', + 'qualified-cargo', + 'qualified-regular', + 'standard-cargo', + 'standard-regular', + ], + }, + // ... +}; + global.fetch = (url: string) => { return new Promise((rs, rj) => { if (url.includes('nrel-commute.nrel-op.json')) { rs({ ok: true, - json: () => - new Promise((rs, rj) => - rs({ - version: 1, - ts: 1655143472, - server: { - connectUrl: 'https://nrel-commute-openpath.nrel.gov/api/', - aggregate_call_auth: 'user_only', - }, - // ... more config would follow but it's good enough for this mock - }), - ), + json: () => new Promise((rs, rj) => rs(nrelCommuteConfig)), }); + } else if (url.includes('denver-casr.nrel-op.json')) { + rs({ + ok: true, + json: () => new Promise((rs, rj) => rs(denverCasrConfig)), + }); + } else { + rj(new Error('404 while fetching ' + url)); } }) as any; }; -const fakeBaseConfig = { - version: 1, - server: { - connectUrl: 'https://www.example.com', - aggregate_call_auth: 'no_auth', - }, -}; -const fakeConfigWithSurveys = { - ...fakeBaseConfig, - survey_info: { - surveys: { - CommuteSurvey: { - formPath: 'https://www.example.com', - labelTemplate: { en: 'Trip to {{destination}}' }, - labelVars: { walking: { destination: 'work' } }, - version: 1, - compatibleWith: 1, - dataKey: 'commute', - }, - }, - 'trip-labels': 'MULTILABEL', - }, -}; - describe('dynamicConfig', () => { const fakeStudyName = 'gotham-city-transit'; - const validStudyName = 'nrel-commute'; + const validStudyNrelCommute = 'nrel-commute'; + const validStudyDenverCasr = 'denver-casr'; describe('getConfig', () => { - it('should reject since no config is set yet', () => { - expect(getConfig()).rejects.toThrow(); + it('should resolve with null since no config is set yet', async () => { + await expect(getConfig()).resolves.toBeNull(); }); - it('should resolve with a config once initByUser is called', async () => { - const validToken = `nrelop_${validStudyName}_user1`; + it('should resolve with a valid config once initByUser is called for an nrel-commute token', async () => { + const validToken = `nrelop_${validStudyNrelCommute}_user1`; await initByUser({ token: validToken }); const config = await getConfig(); expect(config.server.connectUrl).toBe('https://nrel-commute-openpath.nrel.gov/api/'); + expect(config.joined).toEqual({ + opcode: validToken, + study_name: validStudyNrelCommute, + subgroup: undefined, + }); + }); + + it('should resolve with a valid config once initByUser is called for a denver-casr token', async () => { + const validToken = `nrelop_${validStudyDenverCasr}_test_user1`; + await initByUser({ token: validToken }); + const config = await getConfig(); + expect(config.server.connectUrl).toBe('https://denver-casr-openpath.nrel.gov/api/'); + expect(config.joined).toEqual({ + opcode: validToken, + study_name: validStudyDenverCasr, + subgroup: 'test', + }); }); }); describe('initByUser', () => { - it('should error if the study is nonexistent', () => { + // fake study (gotham-city-transit) + it('should error if the study is nonexistent', async () => { const fakeBatmanToken = `nrelop_${fakeStudyName}_batman`; - expect(initByUser({ token: fakeBatmanToken })).rejects.toThrow(); + await expect(initByUser({ token: fakeBatmanToken })).rejects.toThrow(); + }); + + // real study without subgroups (nrel-commute) + it('should error if the study exists but the token is invalid format', async () => { + const badToken1 = validStudyNrelCommute; // doesn't start with nrelop_ + await expect(initByUser({ token: badToken1 })).rejects.toThrow(); + const badToken2 = `nrelop_${validStudyNrelCommute}`; // doesn't have enough _ + await expect(initByUser({ token: badToken2 })).rejects.toThrow(); + const badToken3 = `nrelop_${validStudyNrelCommute}_`; // doesn't have user code after last _ + await expect(initByUser({ token: badToken3 })).rejects.toThrow(); + }); + it('should return true after successfully storing the config for a valid token', async () => { + const validToken = `nrelop_${validStudyNrelCommute}_user2`; + await expect(initByUser({ token: validToken })).resolves.toBe(true); + }); + + // real study with subgroups (denver-casr) + it('should error if the study uses subgroups but the token has no subgroup', async () => { + const tokenWithoutSubgroup = `nrelop_${validStudyDenverCasr}_user2`; + await expect(initByUser({ token: tokenWithoutSubgroup })).rejects.toThrow(); }); - it('should error if the study exists but the token is invalid format', () => { - const badToken1 = validStudyName; - expect(initByUser({ token: badToken1 })).rejects.toThrow(); - const badToken2 = `nrelop_${validStudyName}`; - expect(initByUser({ token: badToken2 })).rejects.toThrow(); - const badToken3 = `nrelop_${validStudyName}_`; - expect(initByUser({ token: badToken3 })).rejects.toThrow(); + it('should error if the study uses subgroups and the token is invalid format', async () => { + const badToken1 = `nrelop_${validStudyDenverCasr}_test_`; // doesn't have user code after last _ + await expect(initByUser({ token: badToken1 })).rejects.toThrow(); }); - it('should return the config for valid token', () => { - const validToken = `nrelop_${validStudyName}_user2`; - expect(initByUser({ token: validToken })).resolves.toBeTruthy(); + it('should return true after successfully storing the config for a valid token with subgroup', async () => { + const validToken = `nrelop_${validStudyDenverCasr}_test_user2`; + await expect(initByUser({ token: validToken })).resolves.toBe(true); }); }); }); From 63732f086ee82d8a6ad26f8a32438f4f3df0516f Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Fri, 15 Dec 2023 13:52:41 -0500 Subject: [PATCH 11/11] fix handling of invalid opcode When we split the string by '_', the resulting parts could be empty strings if the opcode is something like `nrelop_nrel-commute_`, or `nrelop__user1`. We should throw an 'invalid opcode' error in this case --- www/js/config/dynamicConfig.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index 16e6864c3..f684f577d 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -167,8 +167,9 @@ async function fetchConfig(studyLabel: string, alreadyTriedLocal?: boolean) { */ function extractStudyName(token: string): string { const tokenParts = token.split('_'); - if (tokenParts.length < 3) { - // all tokens must have at least nrelop_[study name]_... + if (tokenParts.length < 3 || tokenParts.some((part) => part == '')) { + // all tokens must have at least nrelop_[studyname]_[usercode] + // and neither [studyname] nor [usercode] can be blank throw new Error(i18next.t('config.not-enough-parts-old-style', { token: token })); } if (tokenParts[0] != 'nrelop') {