From 3752e2c37b75682051355bb3eb26422c3fc6bce5 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 17 Oct 2024 12:15:36 -0400 Subject: [PATCH] combine opcode.joinWithTokenOrUrl with dynamicConfig.initByUser Looking at these 2 functions I realized they can be combined and error handling unified, so I combined these to make a new function using async/await syntax. I chose to put the new function in dynamicConfig because this avoids a circular dependency at the module level (which is allowed but not tidy) Now, `dynamicConfig.ts` imports from `opcode.ts` but `opcode.ts` no longer imports anything from `dynamicConfig.ts`. Updated the tests for dynamicConfig to validate that the correct error messages are shown. --- www/__tests__/dynamicConfig.test.ts | 80 ++++++++++++++++++++--------- www/js/config/dynamicConfig.ts | 31 ++++++----- www/js/config/opcode.ts | 26 +--------- 3 files changed, 76 insertions(+), 61 deletions(-) diff --git a/www/__tests__/dynamicConfig.test.ts b/www/__tests__/dynamicConfig.test.ts index 5693ab3fb..795706731 100644 --- a/www/__tests__/dynamicConfig.test.ts +++ b/www/__tests__/dynamicConfig.test.ts @@ -1,7 +1,8 @@ -import { getConfig, initByUser } from '../js/config/dynamicConfig'; - +import { getConfig, joinWithTokenOrUrl } from '../js/config/dynamicConfig'; import initializedI18next from '../js/i18nextInit'; import { storageClear } from '../js/plugin/storage'; +import i18next from '../js/i18nextInit'; + window['i18next'] = initializedI18next; beforeEach(() => { @@ -56,6 +57,8 @@ global.fetch = (url: string) => { }) as any; }; +const windowAlert = jest.spyOn(window, 'alert').mockImplementation(() => {}); + describe('dynamicConfig', () => { const fakeStudyName = 'gotham-city-transit'; const validStudyNrelCommute = 'nrel-commute'; @@ -65,9 +68,9 @@ describe('dynamicConfig', () => { it('should resolve with null since no config is set yet', async () => { await expect(getConfig()).resolves.toBeNull(); }); - it('should resolve with a valid config once initByUser is called for an nrel-commute token', async () => { + it('should resolve with a valid config once joinWithTokenOrUrl is called for an nrel-commute token', async () => { const validToken = `nrelop_${validStudyNrelCommute}_user1`; - await initByUser({ token: validToken }); + await joinWithTokenOrUrl(validToken); const config = await getConfig(); expect(config!.server.connectUrl).toBe('https://nrel-commute-openpath.nrel.gov/api/'); expect(config!.joined).toEqual({ @@ -77,9 +80,9 @@ describe('dynamicConfig', () => { }); }); - it('should resolve with a valid config once initByUser is called for a denver-casr token', async () => { + it('should resolve with a valid config once joinWithTokenOrUrl is called for a denver-casr token', async () => { const validToken = `nrelop_${validStudyDenverCasr}_test_user1`; - await initByUser({ token: validToken }); + await joinWithTokenOrUrl(validToken); const config = await getConfig(); expect(config!.server.connectUrl).toBe('https://denver-casr-openpath.nrel.gov/api/'); expect(config!.joined).toEqual({ @@ -90,39 +93,68 @@ describe('dynamicConfig', () => { }); }); - describe('initByUser', () => { + describe('joinWithTokenOrUrl', () => { // fake study (gotham-city-transit) - it('should error if the study is nonexistent', async () => { + it('returns false if the study is nonexistent', async () => { const fakeBatmanToken = `nrelop_${fakeStudyName}_batman`; - await expect(initByUser({ token: fakeBatmanToken })).rejects.toThrow(); + await expect(joinWithTokenOrUrl(fakeBatmanToken)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining(i18next.t('config.unable-download-config')), + ); }); // 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('returns false if the study exists but the token is invalid format', async () => { + const badToken1 = `nrelop_${validStudyNrelCommute}`; // doesn't have enough _ + await expect(joinWithTokenOrUrl(badToken1)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining( + i18next.t('config.not-enough-parts-old-style', { token: badToken1 }), + ), + ); + + const badToken2 = `nrelop_${validStudyNrelCommute}_`; // doesn't have user code after last _ + await expect(joinWithTokenOrUrl(badToken2)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining( + i18next.t('config.not-enough-parts-old-style', { token: badToken2 }), + ), + ); + + const badToken3 = `invalid_${validStudyNrelCommute}_user3`; // doesn't start with nrelop_ + await expect(joinWithTokenOrUrl(badToken3)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining(i18next.t('config.no-nrelop-start', { token: badToken3 })), + ); }); - it('should return true after successfully storing the config for a valid token', async () => { + + it('returns true after successfully storing the config for a valid token', async () => { const validToken = `nrelop_${validStudyNrelCommute}_user2`; - await expect(initByUser({ token: validToken })).resolves.toBe(true); + await expect(joinWithTokenOrUrl(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 () => { + it('returns false if the study uses subgroups but the token has no subgroup', async () => { const tokenWithoutSubgroup = `nrelop_${validStudyDenverCasr}_user2`; - await expect(initByUser({ token: tokenWithoutSubgroup })).rejects.toThrow(); + await expect(joinWithTokenOrUrl(tokenWithoutSubgroup)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining( + i18next.t('config.not-enough-parts', { token: tokenWithoutSubgroup }), + ), + ); }); - it('should error if the study uses subgroups and the token is invalid format', async () => { + it('returns false 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(); + await expect(joinWithTokenOrUrl(badToken1)).resolves.toBe(false); + expect(windowAlert).toHaveBeenLastCalledWith( + expect.stringContaining( + i18next.t('config.not-enough-parts-old-style', { token: badToken1 }), + ), + ); }); - it('should return true after successfully storing the config for a valid token with subgroup', async () => { + it('returns 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); + await expect(joinWithTokenOrUrl(validToken)).resolves.toBe(true); }); }); }); diff --git a/www/js/config/dynamicConfig.ts b/www/js/config/dynamicConfig.ts index c1e21db54..bf978bc1c 100644 --- a/www/js/config/dynamicConfig.ts +++ b/www/js/config/dynamicConfig.ts @@ -3,7 +3,12 @@ import { displayError, logDebug, logWarn } from '../plugin/logger'; import { fetchUrlCached } from '../services/commHelper'; import { storageClear, storageGet, storageSet } from '../plugin/storage'; import { AppConfig } from '../types/appConfigTypes'; -import { getStudyNameFromToken, getStudyNameFromUrl, getSubgroupFromToken } from './opcode'; +import { + getStudyNameFromToken, + getStudyNameFromUrl, + getSubgroupFromToken, + getTokenFromUrl, +} from './opcode'; export const CONFIG_PHONE_UI = 'config/app_ui_config'; export const CONFIG_PHONE_UI_KVSTORE = 'CONFIG_PHONE_UI'; @@ -179,26 +184,28 @@ export function loadNewConfig(newToken: string, existingVersion?: number): Promi }) .catch((storeError) => { displayError(storeError, i18next.t('config.unable-to-store-config')); - return Promise.reject(storeError); + return Promise.resolve(false); }); }) .catch((fetchErr) => { displayError(fetchErr, i18next.t('config.unable-download-config')); - return Promise.reject(fetchErr); + return Promise.resolve(false); }); } // exported wrapper around loadNewConfig that includes error handling -export function initByUser(urlComponents: { token: string }) { - const { token } = urlComponents; +export async function joinWithTokenOrUrl(tokenOrUrl: 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')); - return Promise.reject(error); + const token = tokenOrUrl.includes('://') ? getTokenFromUrl(tokenOrUrl) : tokenOrUrl; + try { + return await loadNewConfig(token); + } catch (err) { + displayError(err, i18next.t('config.invalid-opcode-format')); + return false; + } + } catch (err) { + displayError(err, 'Error parsing token or URL: ' + tokenOrUrl); + return false; } } diff --git a/www/js/config/opcode.ts b/www/js/config/opcode.ts index a5483d5c2..cb937f5af 100644 --- a/www/js/config/opcode.ts +++ b/www/js/config/opcode.ts @@ -1,8 +1,6 @@ import i18next from 'i18next'; -import { displayError, logDebug } from '../plugin/logger'; +import { logDebug } from '../plugin/logger'; import AppConfig from '../types/appConfigTypes'; -import { initByUser } from './dynamicConfig'; -import { AlertManager } from '../components/AlertBar'; /** * Adapted from https://stackoverflow.com/a/63363662/4040267 @@ -141,25 +139,3 @@ function getTokenFromUrl(url: string) { throw new Error(`URL ${url} had path ${path}, expected 'join' or 'login_token'`); } } - -export async function joinWithTokenOrUrl(tokenOrUrl: string) { - try { - const token = tokenOrUrl.includes('://') ? getTokenFromUrl(tokenOrUrl) : tokenOrUrl; - try { - const result = await initByUser({ token }); - if (result) { - AlertManager.addMessage({ - text: i18next.t('join.proceeding-with-token', { token }), - style: { wordBreak: 'break-all' }, - }); - } - return result; - } catch (err) { - displayError(err, 'Error logging in with token: ' + token); - return false; - } - } catch (err) { - displayError(err, 'Error parsing token or URL: ' + tokenOrUrl); - return false; - } -}