From a894cc77660210a0254acba4cb4027310a769690 Mon Sep 17 00:00:00 2001 From: Phil Moorhouse Date: Wed, 16 Oct 2024 14:26:29 +0100 Subject: [PATCH 1/2] use url builder for generating all URLs --- src/interfaces/service-container.ts | 2 +- src/middleware/ensure-authenticated.ts | 9 +-- src/middleware/language-switcher.ts | 4 +- src/middleware/translations/en.json | 23 +++--- src/routes/auth.ts | 4 +- src/routes/error-handler.ts | 2 +- src/routes/publish.ts | 96 +++++++++----------------- src/views/index.ejs | 6 +- src/views/publish/preview.ejs | 10 +-- src/views/publish/sources.ejs | 2 +- src/views/publish/start.ejs | 4 +- src/views/publish/tasklist.ejs | 2 +- src/views/publish/title.ejs | 2 +- src/views/publish/upload.ejs | 2 +- test/publish.test.ts | 4 +- 15 files changed, 64 insertions(+), 108 deletions(-) diff --git a/src/interfaces/service-container.ts b/src/interfaces/service-container.ts index 6cbe773..9db4c09 100644 --- a/src/interfaces/service-container.ts +++ b/src/interfaces/service-container.ts @@ -3,5 +3,5 @@ import { StatsWalesApi } from '../services/stats-wales-api'; export interface ServiceContainer { swapi: StatsWalesApi; - buildUrl: (path: string, locale: Locale, query?: Record) => string; + buildUrl: (path: string, locale: Locale | string, query?: Record) => string; } diff --git a/src/middleware/ensure-authenticated.ts b/src/middleware/ensure-authenticated.ts index 5e6c3ab..6db1322 100644 --- a/src/middleware/ensure-authenticated.ts +++ b/src/middleware/ensure-authenticated.ts @@ -4,17 +4,12 @@ import JWT from 'jsonwebtoken'; import { JWTPayloadWithUser } from '../interfaces/jwt-payload-with-user'; import { logger } from '../utils/logger'; import { appConfig } from '../config'; -import { Locale } from '../enums/locale'; - -import { localeUrl } from './language-switcher'; const config = appConfig(); export const ensureAuthenticated = (req: Request, res: Response, next: NextFunction) => { logger.debug(`checking if user is authenticated for route ${req.originalUrl}...`); - const locale = req.language as Locale; - try { if (!req.cookies.jwt) { throw new Error('JWT cookie not found'); @@ -30,7 +25,7 @@ export const ensureAuthenticated = (req: Request, res: Response, next: NextFunct if (decoded.exp && decoded.exp <= Date.now() / 1000) { logger.error('JWT token has expired'); res.status(401); - return res.redirect(localeUrl('/auth/login', locale, { error: 'expired' })); + return res.redirect(req.buildUrl(`/auth/login`, req.language, { error: 'expired' })); } // store the token string in the request as we need it for Authorization header in API requests @@ -43,7 +38,7 @@ export const ensureAuthenticated = (req: Request, res: Response, next: NextFunct } catch (err) { logger.error(`authentication failed: ${err}`); res.status(401); - return res.redirect(localeUrl('/auth/login', locale)); + return res.redirect(req.buildUrl(`/auth/login`, req.language)); } return next(); diff --git a/src/middleware/language-switcher.ts b/src/middleware/language-switcher.ts index cf89f61..b2350aa 100644 --- a/src/middleware/language-switcher.ts +++ b/src/middleware/language-switcher.ts @@ -6,7 +6,7 @@ import { Locale } from '../enums/locale'; import { ignoreRoutes, SUPPORTED_LOCALES, i18next } from './translation'; -export const localeUrl = (path: string, locale: Locale, query?: Record): string => { +export const localeUrl = (path: string, locale: Locale | string, query?: Record): string => { const locales = SUPPORTED_LOCALES as string[]; let pathElements = path @@ -14,7 +14,7 @@ export const localeUrl = (path: string, locale: Locale, query?: Record !locales.includes(element)); // strip language from the path if present - if (![Locale.English, Locale.EnglishGb].includes(locale)) { + if (![Locale.English, Locale.EnglishGb].includes(locale as Locale)) { // translate the url path to the new locale pathElements = pathElements.map((element) => i18next.t(`routes.${element}`, { lng: locale })); } diff --git a/src/middleware/translations/en.json b/src/middleware/translations/en.json index c7d67a9..4d7c209 100644 --- a/src/middleware/translations/en.json +++ b/src/middleware/translations/en.json @@ -214,18 +214,15 @@ "routes": { "healthcheck": "healthcheck", "feedback": "feedback", - "view": { - "start": "dataset" - }, - "publish": { - "start": "publish", - "title": "title", - "upload": "upload", - "preview": "preview", - "confirm": "confirm", - "sources": "sources", - "source_confirmation": "source-confirmation", - "tasklist": "tasklist" - } + "dataset": "dataset", + "publish": "publish", + "start": "start", + "title": "title", + "preview": "preview", + "upload": "upload", + "confirm": "confirm", + "sources": "sources", + "source_confirmation": "source-confirmation", + "tasklist": "tasklist" } } diff --git a/src/routes/auth.ts b/src/routes/auth.ts index cd96a97..2e132cb 100644 --- a/src/routes/auth.ts +++ b/src/routes/auth.ts @@ -55,11 +55,11 @@ auth.get('/callback', (req: Request, res: Response) => { } logger.debug('User successfully logged in'); - res.redirect(`/${req.language}`); + res.redirect(req.buildUrl('/', req.language)); }); auth.get('/logout', (req: Request, res: Response) => { logger.debug('logging out user'); res.clearCookie('jwt', { domain: cookieDomain }); - res.redirect(`/${req.language}/auth/login`); + res.redirect(req.buildUrl(`/auth/login`, req.language)); }); diff --git a/src/routes/error-handler.ts b/src/routes/error-handler.ts index 6872052..d78cb6f 100644 --- a/src/routes/error-handler.ts +++ b/src/routes/error-handler.ts @@ -11,7 +11,7 @@ export const errorHandler: ErrorRequestHandler = (err: any, req: Request, res: R case 401: logger.error('401 error detected, logging user out'); res.clearCookie('jwt', { domain: cookieDomain }); - res.redirect(`/${req.language}/auth/login`); + res.redirect(req.buildUrl(`/auth/login`, req.language)); break; case 404: diff --git a/src/routes/publish.ts b/src/routes/publish.ts index 7efea24..3efb5b2 100644 --- a/src/routes/publish.ts +++ b/src/routes/publish.ts @@ -74,28 +74,26 @@ function generateError(field: string, tag: string, params: object): ViewError { } function checkCurrentDataset(req: Request, res: Response): DatasetDTO | undefined { - const lang = req.i18n.language; const currentDataset = req.session.currentDataset; if (!currentDataset) { logger.error('No current dataset found in the session... user may have navigated here by mistake'); req.session.errors = generateViewErrors(undefined, 500, [ generateError('session', 'errors.session.current_dataset_missing', {}) ]); - res.redirect(`/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}`); + res.redirect(req.buildUrl('/publish', req.language)); return undefined; } return currentDataset; } function checkCurrentRevision(req: Request, res: Response): RevisionDTO | undefined { - const lang = req.i18n.language; const currentRevision = req.session.currentRevision; if (!currentRevision) { logger.error('No current revision found in the session... user may have navigated here by mistake'); req.session.errors = generateViewErrors(undefined, 500, [ generateError('session', 'errors.session.current_revision_missing', {}) ]); - res.redirect(`/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}`); + res.redirect(req.buildUrl('/publish', req.language)); return undefined; } return currentRevision; @@ -109,21 +107,20 @@ function checkCurrentFileImport(req: Request, res: Response): FileImportDTO | un req.session.errors = generateViewErrors(undefined, 500, [ generateError('session', 'errors.session.current_import_missing', {}) ]); - res.redirect(`/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}`); + res.redirect(req.buildUrl('/publish', req.language)); return undefined; } return currentFileImport; } async function createNewDataset(req: Request, res: Response, next: NextFunction): Promise { - const lng = req.language as Locale; const title = req.session.currentTitle; const file = req.file; if (!title) { logger.debug('Current title missing from the session'); req.session.errors = generateViewErrors(undefined, 400, [generateError('title', 'errors.title_missing', {})]); - res.redirect(`/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${t('routes.publish.title', { lng })}`); + res.redirect(req.buildUrl('/publish/title', req.language)); return; } @@ -138,16 +135,14 @@ async function createNewDataset(req: Request, res: Response, next: NextFunction) try { const dataset = await req.swapi.uploadCSVtoCreateDataset(fileData, fileName, title); setCurrentToSession(dataset, req); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.preview', { lng })}` - ); + res.redirect(req.buildUrl('/publish/preview', req.language)); } catch (err: any) { if (err?.status === 401) { next(err); return; } req.session.errors = generateViewErrors(undefined, 500, err?.errors); - res.redirect(`/${lng}/${req.i18n.t('routes.publish.start', { lng })}`); + res.redirect(req.buildUrl('/publish', req.language)); } } @@ -176,16 +171,14 @@ async function uploadNewFileToExistingDataset(req: Request, res: Response, next: fileName ); setCurrentToSession(dataset, req); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.preview', { lng })}` - ); + res.redirect(req.buildUrl('/publish/preview', req.language)); } catch (err: any) { if (err?.status === 401) { next(err); return; } req.session.errors = generateViewErrors(undefined, 500, err?.errors); - res.redirect(`/${lng}/${req.i18n.t('routes.publish.start', { lng })}`); + res.redirect(req.buildUrl('/publish', req.language)); } } @@ -212,8 +205,8 @@ publish.get('/title', (req: Request, res: Response) => { errors: req.session.errors, isMetadata: false, currrentTitle: req.session.currentTitle, - postAction: `/${req.language}/${t('routes.publish.start')}/${t('routes.publish.title')}`, - backButtonLink: `/${req.language}/${t('routes.publish.start')}/` + postAction: req.buildUrl('/publish/title', req.language), + backButtonLink: req.buildUrl('/publish', req.language) }); }); @@ -225,17 +218,15 @@ publish.post('/title', upload.none(), (req: Request, res: Response) => { errors: generateViewErrors(undefined, 500, [generateError('title', 'errors.title.missing', {})]), isMetadata: false, currrentTitle: req.session.currentTitle, - postAction: `/${req.language}/${t('routes.publish.start')}/${t('routes.publish.title')}`, - backButtonLink: `/${req.language}/${t('routes.publish.start')}/` + postAction: req.buildUrl('/publish/title', req.language), + backButtonLink: req.buildUrl('/publish', req.language) }); return; } req.session.currentTitle = req.body.title; req.session.save(); const lang = req.i18n.language; - res.redirect( - `/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}/${req.i18n.t('routes.publish.upload', { lng: lang })}` - ); + res.redirect(req.buildUrl('/publish/upload', req.language)); }); publish.get('/upload', (req: Request, res: Response) => { @@ -245,10 +236,7 @@ publish.get('/upload', (req: Request, res: Response) => { logger.error('There is no title or currentDataset in the session. Abandoning this create journey'); req.session.errors = generateViewErrors(undefined, 500, [generateError('title', 'errors.title.missing', {})]); req.session.save(); - const lang = req.i18n.language; - res.redirect( - `/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}/${t('routes.publish.title', { lng: lang })}` - ); + res.redirect(req.buildUrl('/publish/title', req.language)); return; } const title = currentDataset?.datasetInfo?.find((info) => info.language === req.i18n.language) || currentTitle; @@ -266,8 +254,6 @@ publish.post('/upload', upload.single('csv'), async (req: Request, res: Response }); publish.get('/preview', async (req: Request, res: Response, next: NextFunction) => { - const lng = req.language as Locale; - const currentDataset = checkCurrentDataset(req, res); if (!currentDataset) { return; @@ -306,7 +292,7 @@ publish.get('/preview', async (req: Request, res: Response, next: NextFunction) generateError('preview', 'errors.preview.failed_to_get_preview', {}) ]); req.session.save(); - res.redirect(`/${lng}/${req.i18n.t('routes.publish.start', { lng })}`); + res.redirect(req.buildUrl('/publish', req.language)); } }); @@ -318,7 +304,6 @@ async function confirmFileUpload( res: Response, next: NextFunction ) { - const lng = req.language; try { const fileImport: FileImportDTO = await req.swapi.confirmFileImport( currentDataset.id, @@ -328,9 +313,7 @@ async function confirmFileUpload( // eslint-disable-next-line require-atomic-updates req.session.currentImport = fileImport; req.session.save(); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.sources', { lng })}` - ); + res.redirect(req.buildUrl('/publish/sources', req.language)); } catch (err: any) { if (err?.status === 401) { next(err); @@ -344,9 +327,7 @@ async function confirmFileUpload( generateError('confirm', 'errors.preview.confirm_error', {}) ]); req.session.save(); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.preview', { lng })}` - ); + res.redirect(req.buildUrl('/publish/preview', req.language)); } } @@ -363,9 +344,7 @@ async function rejectFileReturnToUpload( req.session.currentImport = undefined; await req.swapi.removeFileImport(currentDataset.id, currentRevision.id, currentFileImport.id); req.session.save(); - res.redirect( - `/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}/${req.i18n.t('routes.publish.upload')}` - ); + res.redirect(req.buildUrl('/publish/upload', req.language)); } catch (err: any) { if (err?.status === 401) { next(err); @@ -378,9 +357,7 @@ async function rejectFileReturnToUpload( generateError('confirm', 'errors.preview.remove_error', {}) ]); req.session.save(); - res.redirect( - `/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}/${req.i18n.t('routes.publish.preview', { lng: lang })}` - ); + res.redirect(req.buildUrl('/publish/preview', req.language)); } } @@ -400,7 +377,6 @@ publish.post('/preview', upload.none(), async (req: Request, res: Response, next return; } - const lng = req.language as Locale; const confirmData = req.body?.confirm; if (!confirmData) { logger.error('The confirm variable is missing on the form submission'); @@ -408,9 +384,7 @@ publish.post('/preview', upload.none(), async (req: Request, res: Response, next generateError('confirmBtn', 'errors.confirm.missing', {}) ]); req.session.save(); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.preview', { lng })}` - ); + res.redirect(req.buildUrl('/publish/preview', req.language)); return; } if (confirmData === 'true') { @@ -433,7 +407,6 @@ function updateCurrentImport(currentImport: FileImportDTO, dimensionCreationRequ } publish.get('/sources', upload.none(), (req: Request, res: Response) => { - const lang = req.i18n.language; let currentFileImport = checkCurrentFileImport(req, res); const dimensionCreationRequest = req.session.dimensionCreationRequest; if (!currentFileImport) { @@ -445,7 +418,7 @@ publish.get('/sources', upload.none(), (req: Request, res: Response) => { generateError('session', 'errors.session.no_sources_on_import', {}) ]); req.session.save(); - res.redirect(`/${lang}/${req.i18n.t('routes.publish.start', { lng: lang })}`); + res.redirect(req.buildUrl('/publish', req.language)); return; } const errs = req.session.errors; @@ -472,7 +445,6 @@ publish.get('/sources', upload.none(), (req: Request, res: Response) => { }); publish.post('/sources', upload.none(), async (req: Request, res: Response, next: NextFunction) => { - const lng = req.language as Locale; const currentDataset = checkCurrentDataset(req, res); if (!currentDataset) { return; @@ -494,7 +466,7 @@ publish.post('/sources', upload.none(), async (req: Request, res: Response, next generateError('session', 'errors.session.no_sources_on_import', {}) ]); req.session.save(); - res.redirect(`/${lng}/${req.i18n.t('routes.publish.start', { lng })}`); + res.redirect(req.buildUrl('/publish', req.language)); return; } @@ -580,9 +552,7 @@ publish.post('/sources', upload.none(), async (req: Request, res: Response, next dimensionCreationRequest ); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${dataset.id}/${req.i18n.t('routes.publish.tasklist', { lng })}` - ); + res.redirect(req.buildUrl(`/publish/${dataset.id}/tasklist`, req.language)); } catch (err: any) { if (err?.status === 401) { next(err); @@ -651,7 +621,6 @@ function buildStateFromDataset(lang: string, dataset: DatasetDTO): TaskListState } publish.get('/:datasetId/tasklist', async (req: Request, res: Response, next: NextFunction) => { - const lng = req.language as Locale; const datasetId = req.params.datasetId as string; try { @@ -659,8 +628,8 @@ publish.get('/:datasetId/tasklist', async (req: Request, res: Response, next: Ne const dataset = await req.swapi.getDataset(datasetId); setCurrentToSession(dataset, req); res.render('publish/tasklist', { - taskList: buildStateFromDataset(lng, dataset), - sourcesUrl: `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${req.i18n.t('routes.publish.sources', { lng })}` + taskList: buildStateFromDataset(req.language, dataset), + sourcesUrl: req.buildUrl('/publish/sources', req.language) }); } catch (err: any) { if (err?.status === 401) { @@ -674,22 +643,21 @@ publish.get('/:datasetId/tasklist', async (req: Request, res: Response, next: Ne }); publish.get('/:datasetId/title', async (req: Request, res: Response) => { - const lang = req.language as Locale; const datasetId = req.params.datasetId as string; try { if (!validateUUID(datasetId)) throw new Error('Invalid dataset ID'); const dataset = await req.swapi.getDataset(datasetId); setCurrentToSession(dataset, req); - const singleLanguageDataset = singleLangDataset(lang, dataset); + const singleLanguageDataset = singleLangDataset(req.language, dataset); res.render('publish/title', { errors: req.session.errors, currentTitle: singleLanguageDataset.datasetInfo?.title, isMetadata: true, datasetId, - postAction: `/${req.language}/${t('routes.publish.start')}/${datasetId}/${t('routes.publish.title')}`, - backButtonLink: `/${req.language}/${t('routes.publish.start')}/${datasetId}/${t('routes.publish.tasklist')}` + postAction: req.buildUrl(`/publish/${dataset.id}/title`, req.language), + backButtonLink: req.buildUrl(`/publish/${dataset.id}/tasklist`, req.language) }); } catch (err) { logger.error(`Something went wrong trying to load the title: ${err}`); @@ -714,17 +682,15 @@ publish.post('/:datasetId/title', upload.none(), async (req: Request, res: Respo currentTitle: singleLanguageDataset.datasetInfo?.title, isMetadata: true, datasetId, - postAction: `/${req.language}/${t('routes.publish.start')}/${datasetId}/${t('routes.publish.title')}`, - backButtonLink: `/${req.language}/${t('routes.publish.start')}/${datasetId}/${t('routes.publish.tasklist')}` + postAction: req.buildUrl(`/publish/${dataset.id}/title`, req.language), + backButtonLink: req.buildUrl(`/publish/${dataset.id}/tasklist`, req.language) }); return; } const infoDto: DatasetInfoDTO = singleLanguageDataset.datasetInfo || ({ language: lng } as DatasetInfoDTO); infoDto.title = req.body.title; await req.swapi.sendDatasetInfo(datasetId, infoDto); - res.redirect( - `/${lng}/${req.i18n.t('routes.publish.start', { lng })}/${dataset.id}/${req.i18n.t('routes.publish.tasklist', { lng })}` - ); + res.redirect(req.buildUrl(`/publish/${dataset.id}/tasklist`, req.language)); } catch (err) { logger.error(`Something went wrong trying to load the title: ${err}`); res.status(404); diff --git a/src/views/index.ejs b/src/views/index.ejs index 47ff3fe..371fd66 100644 --- a/src/views/index.ejs +++ b/src/views/index.ejs @@ -6,12 +6,12 @@

<%= t('homepage.title') %>

<%= t('homepage.welcome') %>

-<%- include("partials/footer"); %> \ No newline at end of file +<%- include("partials/footer"); %> diff --git a/src/views/publish/preview.ejs b/src/views/publish/preview.ejs index c6af6ab..5fe0198 100644 --- a/src/views/publish/preview.ejs +++ b/src/views/publish/preview.ejs @@ -60,7 +60,7 @@ <% if (page==='previous' ) { %>
  • <%= page %> @@ -113,7 +113,7 @@
    • -
    + @@ -136,7 +136,7 @@
    - + diff --git a/src/views/publish/sources.ejs b/src/views/publish/sources.ejs index aa480b5..b7c8589 100644 --- a/src/views/publish/sources.ejs +++ b/src/views/publish/sources.ejs @@ -5,7 +5,7 @@

    <%= t('publish.sources.heading') %>

    <%- include("../partials/error-handler"); %> - +
    <% locals.currentImport.sources.forEach((source) => { %>
    diff --git a/src/views/publish/start.ejs b/src/views/publish/start.ejs index 32731e4..5879a15 100644 --- a/src/views/publish/start.ejs +++ b/src/views/publish/start.ejs @@ -12,8 +12,8 @@
  • <%= t('publish.start.metadata') %>
    • - <%= t('buttons.continue') %> - <%= t('buttons.cancel') %> + <%= t('buttons.continue') %> + <%= t('buttons.cancel') %>
    diff --git a/src/views/publish/tasklist.ejs b/src/views/publish/tasklist.ejs index b0c0709..799aa8f 100644 --- a/src/views/publish/tasklist.ejs +++ b/src/views/publish/tasklist.ejs @@ -42,7 +42,7 @@
    • - + <%= t('publish.tasklist.metadata.title') %>
      diff --git a/src/views/publish/title.ejs b/src/views/publish/title.ejs index 5dad080..7c4e355 100644 --- a/src/views/publish/title.ejs +++ b/src/views/publish/title.ejs @@ -7,7 +7,7 @@
      <%= t('buttons.back') %> - <%= t('publish.header.overview') %> + <%= t('publish.header.overview') %>
      <% } %> diff --git a/src/views/publish/upload.ejs b/src/views/publish/upload.ejs index ca70e41..77d5fe4 100644 --- a/src/views/publish/upload.ejs +++ b/src/views/publish/upload.ejs @@ -5,7 +5,7 @@

      <%= t('publish.upload.title') %>

      <%- include("../partials/error-handler"); %> - +