From 35767f097c2c42d605853043c4ba2ed38c64f4af Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 14:15:36 +0800 Subject: [PATCH 01/28] feat(AuthService): add verifyLoginOtp fn --- src/public/services/AuthService.ts | 52 ++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index c08e1c5ccf..0dc7292ffd 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -4,6 +4,38 @@ import { Opaque } from 'type-fest' // Exported for testing. export const AUTH_ENDPOINT = '/api/v3/auth' +const STORAGE_ADMIN_KEY = 'user' + +type AdminId = Opaque +type AgencyId = Opaque + +export type Agency = { + emailDomain: string[] + fullName: string + shortName: string + logo: string + _id: AgencyId +} + +export type Admin = { + _id: AdminId + email: string + agency: Agency + created: string + lastAccessed: string + updatedAt: string +} + +/** + * Save logged in admin to localStorage. + * May not be needed in React depending on implementation. + * + * @param admin the admin to save to local storage + */ +const saveAdminToLocalStorage = (admin: Admin) => { + return localStorage.setItem(STORAGE_ADMIN_KEY, JSON.stringify(admin)) +} + type Email = Opaque /** @@ -31,3 +63,23 @@ export const sendLoginOtp = async (email: Email): Promise => { }) .then(({ data }) => data) } + +/** + * Verifies the login OTP and saves the returned user to localStorage if OTP is + * valid. + * @param params.email the email to verify + * @param params.otp the OTP sent to the given email to verify + * @returns logged in user when successful + * @throws Error on non 2xx response + */ +export const verifyLoginOtp = async (params: { + otp: string + email: string +}): Promise => { + return axios + .post(`${AUTH_ENDPOINT}/otp/verify`, params) + .then(({ data }) => { + saveAdminToLocalStorage(data) + return data + }) +} From 5e9897c3e3e247cbfede6584d7fd165f906f2802 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 14:16:25 +0800 Subject: [PATCH 02/28] ref: use AdminAuthService.verifyLoginOtp --- .../authentication.client.controller.js | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/public/modules/users/controllers/authentication.client.controller.js b/src/public/modules/users/controllers/authentication.client.controller.js index 4ac92def2b..fa9d44abca 100755 --- a/src/public/modules/users/controllers/authentication.client.controller.js +++ b/src/public/modules/users/controllers/authentication.client.controller.js @@ -1,6 +1,6 @@ 'use strict' -const HttpStatus = require('http-status-codes') +const { StatusCodes } = require('http-status-codes') const get = require('lodash/get') const validator = require('validator').default const AuthService = require('../../../services/AuthService') @@ -216,8 +216,13 @@ function AuthenticationController( */ vm.verifyOtp = function () { vm.buttonClicked = true - Auth.verifyOtp(vm.credentials).then( - function () { + $q.when( + AuthService.verifyLoginOtp({ + otp: vm.credentials.otp, + email: vm.credentials.email, + }), + ) + .then(() => { vm.buttonClicked = false // Configure message to be show vm.signInMsg = { @@ -239,25 +244,32 @@ function AuthenticationController( }, 500) GTag.loginSuccess('otp') - }, - function (error) { + }) + .catch((error) => { + let errorMsg = get( + error, + 'response.data', + 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', + ) + const errorStatus = get(error, 'response.status') + if (errorStatus >= StatusCodes.INTERNAL_SERVER_ERROR) { + errorMsg = 'An unknown error occurred' + } + vm.buttonClicked = false // Configure message to be show vm.signInMsg = { isMsg: true, isError: true, - msg: - error.status >= HttpStatus.INTERNAL_SERVER_ERROR - ? 'An unknown error occurred' - : error.data, + msg: errorMsg, } $timeout(function () { angular.element('#otp-input').focus() angular.element('#otp-input').select() }, 100) - GTag.loginFailure('otp', String(error.status || error.data || '')) - }, - ) + + GTag.loginFailure('otp', String(errorStatus || errorMsg)) + }) } const cancelNotifDelayTimeout = () => { From 2cfa8c4d5583940a49de1bb8dce091fac9b1153e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 15:14:19 +0800 Subject: [PATCH 03/28] ref: rename Admin to User better fit API prefix --- src/public/services/AuthService.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index 0dc7292ffd..e66525c35a 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -4,9 +4,9 @@ import { Opaque } from 'type-fest' // Exported for testing. export const AUTH_ENDPOINT = '/api/v3/auth' -const STORAGE_ADMIN_KEY = 'user' +const STORAGE_USER_KEY = 'user' -type AdminId = Opaque +type UserId = Opaque type AgencyId = Opaque export type Agency = { @@ -17,8 +17,8 @@ export type Agency = { _id: AgencyId } -export type Admin = { - _id: AdminId +export type User = { + _id: UserId email: string agency: Agency created: string @@ -27,13 +27,13 @@ export type Admin = { } /** - * Save logged in admin to localStorage. + * Save logged in user to localStorage. * May not be needed in React depending on implementation. * - * @param admin the admin to save to local storage + * @param user the user to save to local storage */ -const saveAdminToLocalStorage = (admin: Admin) => { - return localStorage.setItem(STORAGE_ADMIN_KEY, JSON.stringify(admin)) +const saveUserToLocalStorage = (user: User) => { + return localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) } type Email = Opaque @@ -75,11 +75,11 @@ export const sendLoginOtp = async (email: Email): Promise => { export const verifyLoginOtp = async (params: { otp: string email: string -}): Promise => { +}): Promise => { return axios - .post(`${AUTH_ENDPOINT}/otp/verify`, params) + .post(`${AUTH_ENDPOINT}/otp/verify`, params) .then(({ data }) => { - saveAdminToLocalStorage(data) + saveUserToLocalStorage(data) return data }) } From 64b350cb5564bd3647863ac3be48c2a528e886fd Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 15:25:17 +0800 Subject: [PATCH 04/28] ref(AuthService): move User related fns to UserService --- src/public/services/AuthService.ts | 34 ++---------------------------- src/public/services/UserService.ts | 33 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 32 deletions(-) create mode 100644 src/public/services/UserService.ts diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index e66525c35a..dd0bcd6ab9 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -1,41 +1,11 @@ import axios from 'axios' import { Opaque } from 'type-fest' +import { saveUserToLocalStorage, User } from './UserService' + // Exported for testing. export const AUTH_ENDPOINT = '/api/v3/auth' -const STORAGE_USER_KEY = 'user' - -type UserId = Opaque -type AgencyId = Opaque - -export type Agency = { - emailDomain: string[] - fullName: string - shortName: string - logo: string - _id: AgencyId -} - -export type User = { - _id: UserId - email: string - agency: Agency - created: string - lastAccessed: string - updatedAt: string -} - -/** - * Save logged in user to localStorage. - * May not be needed in React depending on implementation. - * - * @param user the user to save to local storage - */ -const saveUserToLocalStorage = (user: User) => { - return localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) -} - type Email = Opaque /** diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts new file mode 100644 index 0000000000..f871f3742b --- /dev/null +++ b/src/public/services/UserService.ts @@ -0,0 +1,33 @@ +import { Opaque } from 'type-fest' + +const STORAGE_USER_KEY = 'user' + +type UserId = Opaque +type AgencyId = Opaque + +export type Agency = { + emailDomain: string[] + fullName: string + shortName: string + logo: string + _id: AgencyId +} + +export type User = { + _id: UserId + email: string + agency: Agency + created: string + lastAccessed: string + updatedAt: string +} + +/** + * Save logged in user to localStorage. + * May not be needed in React depending on implementation. + * + * @param user the user to save to local storage + */ +export const saveUserToLocalStorage = (user: User | null): void => { + localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) +} From 427b6ac4c0d52ec967ee93795e978bda3bf19a9a Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 15:57:26 +0800 Subject: [PATCH 05/28] test(AuthService): add unit tests for verifyLoginOtp --- .../services/__tests__/AuthService.test.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/public/services/__tests__/AuthService.test.ts b/src/public/services/__tests__/AuthService.test.ts index 7ce4b2053e..d30005de5e 100644 --- a/src/public/services/__tests__/AuthService.test.ts +++ b/src/public/services/__tests__/AuthService.test.ts @@ -6,10 +6,14 @@ import { AUTH_ENDPOINT, checkIsEmailAllowed, sendLoginOtp, + verifyLoginOtp, } from '../AuthService' +import * as UserService from '../UserService' jest.mock('axios') +jest.mock('../UserService') +const MockUserService = mocked(UserService) const MockAxios = mocked(axios, true) // Duplicated here instead of exporting from AuthService to prevent production @@ -57,4 +61,33 @@ describe('AuthService', () => { }) }) }) + + describe('verifyLoginOtp', () => { + const EXPECTED_POST_ENDPOINT = `${AUTH_ENDPOINT}/otp/verify` + const MOCK_OTP = '123456' + const MOCK_EMAIL = 'mockEmail@example.com' + + it('should save returned user to localStorage and return user on success', async () => { + // Arrange + const mockUser = { + _id: 'some id', + email: MOCK_EMAIL, + } + MockAxios.post.mockResolvedValueOnce({ data: mockUser }) + const expectedParams = { otp: MOCK_OTP, email: MOCK_EMAIL } + + // Act + const actual = await verifyLoginOtp(expectedParams) + + // Assert + expect(actual).toEqual(mockUser) + expect(MockUserService.saveUserToLocalStorage).toHaveBeenCalledWith( + mockUser, + ) + expect(MockAxios.post).toHaveBeenCalledWith( + EXPECTED_POST_ENDPOINT, + expectedParams, + ) + }) + }) }) From 6ab3771963bde0ed9e0f2b6a10d9a88c7247c12f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:06:02 +0800 Subject: [PATCH 06/28] chore: add jest-localstorage-mock package for mocking localstorage in the frontend jest tests --- package-lock.json | 6 ++++++ package.json | 1 + tests/unit/frontend/jest.config.js | 2 ++ 3 files changed, 9 insertions(+) diff --git a/package-lock.json b/package-lock.json index 600f48ea48..911ee8cd81 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15689,6 +15689,12 @@ "pretty-format": "^26.6.2" } }, + "jest-localstorage-mock": { + "version": "2.4.14", + "resolved": "https://registry.npmjs.org/jest-localstorage-mock/-/jest-localstorage-mock-2.4.14.tgz", + "integrity": "sha512-B+Y0y3J4wBOHdmcFSicWmVYMFAZFbJvjs1EfRIzUJRg2UAK+YVVUgTn7/MdjENey5xbBKmraBmKY5EX+x1NJXA==", + "dev": true + }, "jest-matcher-utils": { "version": "26.6.2", "resolved": "https://registry.npmjs.org/jest-matcher-utils/-/jest-matcher-utils-26.6.2.tgz", diff --git a/package.json b/package.json index f6a4a6e396..2ae4b2f97a 100644 --- a/package.json +++ b/package.json @@ -222,6 +222,7 @@ "husky": "^6.0.0", "jest": "^26.6.3", "jest-extended": "^0.11.5", + "jest-localstorage-mock": "^2.4.14", "jest-mock-axios": "^4.4.0", "lint-staged": "^11.0.0", "maildev": "^1.1.0", diff --git a/tests/unit/frontend/jest.config.js b/tests/unit/frontend/jest.config.js index 17526446ae..6294e2c5aa 100644 --- a/tests/unit/frontend/jest.config.js +++ b/tests/unit/frontend/jest.config.js @@ -2,6 +2,7 @@ module.exports = { preset: 'ts-jest/presets/js-with-ts', rootDir: '../../../', modulePaths: [''], + testEnvironment: 'jsdom', roots: ['/src/public/', '/tests/unit/frontend/'], globals: { // Revert when memory leak in ts-jest is fixed. @@ -12,4 +13,5 @@ module.exports = { }, clearMocks: true, setupFilesAfterEnv: ['jest-extended'], + setupFiles: ['jest-localstorage-mock'], } From c7fadc8aa462c6736456217c92040d801a03c1fc Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:06:59 +0800 Subject: [PATCH 07/28] test(UserService): add unit tests for saveUserToLocalStorage --- src/public/services/UserService.ts | 3 +- .../services/__tests__/UserService.test.ts | 33 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/public/services/__tests__/UserService.test.ts diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index f871f3742b..a0914b6990 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -1,6 +1,7 @@ import { Opaque } from 'type-fest' -const STORAGE_USER_KEY = 'user' +/** Exported for testing */ +export const STORAGE_USER_KEY = 'user' type UserId = Opaque type AgencyId = Opaque diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts new file mode 100644 index 0000000000..f2f5a2ca6c --- /dev/null +++ b/src/public/services/__tests__/UserService.test.ts @@ -0,0 +1,33 @@ +import { saveUserToLocalStorage, STORAGE_USER_KEY, User } from '../UserService' + +describe('UserService', () => { + describe('saveUserToLocalStorage', () => { + it('should successfully save given user to localStorage', () => { + // Arrange + const mockUser: User = { + _id: 'some id' as User['_id'], + email: 'mock@example.com', + agency: { + _id: 'some agency id' as User['agency']['_id'], + emailDomain: ['example.com'], + fullName: 'Example Agency', + logo: 'path/to/agency/logo', + shortName: 'e', + }, + created: 'some created date', + lastAccessed: 'some last accessed date', + updatedAt: 'some last updated at date', + } + + // Act + saveUserToLocalStorage(mockUser) + + // Assert + expect(localStorage.setItem).toHaveBeenLastCalledWith( + STORAGE_USER_KEY, + // Should be stringified. + JSON.stringify(mockUser), + ) + }) + }) +}) From fe4f5aa33e697ec08296744fddb4dcc44d38c612 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:17:52 +0800 Subject: [PATCH 08/28] chore: move date-fns from devDeps to deps --- package-lock.json | 3 +-- package.json | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 911ee8cd81..233c3a9bcb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10035,8 +10035,7 @@ "date-fns": { "version": "2.22.1", "resolved": "https://registry.npmjs.org/date-fns/-/date-fns-2.22.1.tgz", - "integrity": "sha512-yUFPQjrxEmIsMqlHhAhmxkuH769baF21Kk+nZwZGyrMoyLA+LugaQtC0+Tqf9CBUUULWwUJt6Q5ySI3LJDDCGg==", - "dev": true + "integrity": "sha512-yUFPQjrxEmIsMqlHhAhmxkuH769baF21Kk+nZwZGyrMoyLA+LugaQtC0+Tqf9CBUUULWwUJt6Q5ySI3LJDDCGg==" }, "dateformat": { "version": "1.0.12", diff --git a/package.json b/package.json index 2ae4b2f97a..dab9b1d2ff 100644 --- a/package.json +++ b/package.json @@ -101,6 +101,7 @@ "cookie-parser": "~1.4.0", "css-toggle-switch": "^4.1.0", "csv-string": "^4.0.1", + "date-fns": "^2.22.1", "dedent-js": "~1.0.1", "ejs": "^3.1.6", "express": "^4.16.4", @@ -205,7 +206,6 @@ "coveralls": "^3.1.0", "css-loader": "^2.1.1", "csv-parse": "^4.15.4", - "date-fns": "^2.22.1", "env-cmd": "^10.1.0", "eslint": "^7.28.0", "eslint-config-prettier": "^8.3.0", From 442be1332ee45319052504cb3f8cb87a63151e74 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:19:59 +0800 Subject: [PATCH 09/28] feat: add base typings for Agency and User in preparation for extending from to close #2066 --- src/public/services/AuthService.ts | 4 ++- src/public/services/UserService.ts | 22 +--------------- .../services/__tests__/UserService.test.ts | 3 ++- src/types/api/agency.ts | 14 +++++++++++ src/types/api/user.ts | 25 +++++++++++++++++++ 5 files changed, 45 insertions(+), 23 deletions(-) create mode 100644 src/types/api/agency.ts create mode 100644 src/types/api/user.ts diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index dd0bcd6ab9..a6a9ceed49 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -1,7 +1,9 @@ import axios from 'axios' import { Opaque } from 'type-fest' -import { saveUserToLocalStorage, User } from './UserService' +import { User } from '../../types/api/user' + +import { saveUserToLocalStorage } from './UserService' // Exported for testing. export const AUTH_ENDPOINT = '/api/v3/auth' diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index a0914b6990..ac83342465 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -1,28 +1,8 @@ -import { Opaque } from 'type-fest' +import { User } from '../../types/api/user' /** Exported for testing */ export const STORAGE_USER_KEY = 'user' -type UserId = Opaque -type AgencyId = Opaque - -export type Agency = { - emailDomain: string[] - fullName: string - shortName: string - logo: string - _id: AgencyId -} - -export type User = { - _id: UserId - email: string - agency: Agency - created: string - lastAccessed: string - updatedAt: string -} - /** * Save logged in user to localStorage. * May not be needed in React depending on implementation. diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts index f2f5a2ca6c..27817f9c8d 100644 --- a/src/public/services/__tests__/UserService.test.ts +++ b/src/public/services/__tests__/UserService.test.ts @@ -1,4 +1,5 @@ -import { saveUserToLocalStorage, STORAGE_USER_KEY, User } from '../UserService' +import { User } from '../../../types/api/user' +import { saveUserToLocalStorage, STORAGE_USER_KEY } from '../UserService' describe('UserService', () => { describe('saveUserToLocalStorage', () => { diff --git a/src/types/api/agency.ts b/src/types/api/agency.ts new file mode 100644 index 0000000000..d65a809515 --- /dev/null +++ b/src/types/api/agency.ts @@ -0,0 +1,14 @@ +import { Opaque } from 'type-fest' +import { z } from 'zod' + +type AgencyId = Opaque + +export const Agency = z.object({ + emailDomain: z.array(z.string()), + fullName: z.string(), + shortName: z.string(), + logo: z.string(), + _id: z.string() as unknown as z.Schema, +}) + +export type Agency = z.infer diff --git a/src/types/api/user.ts b/src/types/api/user.ts new file mode 100644 index 0000000000..25b589f30d --- /dev/null +++ b/src/types/api/user.ts @@ -0,0 +1,25 @@ +import { isDate, parseISO } from 'date-fns' +import { Opaque } from 'type-fest' +import { z } from 'zod' + +import { Agency } from './agency' + +type UserId = Opaque +type UserContact = Opaque + +const DateString = z.string().refine( + (val) => isDate(parseISO(val)), + (val) => ({ message: `${val} is not a valid date` }), +) + +export const User = z.object({ + _id: z.string() as unknown as z.Schema, + email: z.string().email(), + agency: Agency, + created: DateString, + lastAccessed: DateString, + updatedAt: DateString, + contact: (z.string() as unknown as z.Schema).optional(), +}) + +export type User = z.infer From 82723dc957620c56c083be2daa60d48a141a4ba9 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:30:48 +0800 Subject: [PATCH 10/28] feat: remove unused verifyOtp function --- .../modules/users/services/auth.client.service.js | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/public/modules/users/services/auth.client.service.js b/src/public/modules/users/services/auth.client.service.js index a5bf7f719e..a0b8f715aa 100644 --- a/src/public/modules/users/services/auth.client.service.js +++ b/src/public/modules/users/services/auth.client.service.js @@ -21,7 +21,6 @@ function Auth($q, $http, $state, $window) { */ let authService = { - verifyOtp, getUser, setUser, refreshUser, @@ -64,20 +63,6 @@ function Auth($q, $http, $state, $window) { return null }) } - function verifyOtp(credentials) { - let deferred = $q.defer() - $http.post('/api/v3/auth/otp/verify', credentials).then( - function (response) { - setUser(response.data) - deferred.resolve() - }, - function (error) { - deferred.reject(error) - }, - ) - return deferred.promise - } - function signOut() { $http.get('/api/v3/auth/logout').then( function () { From 4fc45708401d67486ad76cba128cd1c5682a2361 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:08:04 +0800 Subject: [PATCH 11/28] chore: set VSCode default TS version to follow node_modules --- .vscode/settings.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 7702f5b168..8ad8525a2e 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -16,5 +16,6 @@ "source.fixAll.eslint": false }, "editor.defaultFormatter": "esbenp.prettier-vscode" - } + }, + "typescript.tsdk": "node_modules/typescript/lib" } From aa596c988ce6ef3c6039a98542e61c3bf12033e3 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 8 Jun 2021 10:38:16 +0800 Subject: [PATCH 12/28] ref: move saveUserToLocalStorage responsibility to calling controller --- .../controllers/authentication.client.controller.js | 4 +++- src/public/services/AuthService.ts | 10 ++-------- src/public/services/UserService.ts | 2 +- src/public/services/__tests__/AuthService.test.ts | 9 +-------- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/src/public/modules/users/controllers/authentication.client.controller.js b/src/public/modules/users/controllers/authentication.client.controller.js index fa9d44abca..0941dfb764 100755 --- a/src/public/modules/users/controllers/authentication.client.controller.js +++ b/src/public/modules/users/controllers/authentication.client.controller.js @@ -4,6 +4,7 @@ const { StatusCodes } = require('http-status-codes') const get = require('lodash/get') const validator = require('validator').default const AuthService = require('../../../services/AuthService') +const UserService = require('../../../services/UserService') angular .module('users') @@ -222,7 +223,8 @@ function AuthenticationController( email: vm.credentials.email, }), ) - .then(() => { + .then((user) => { + UserService.saveUserToLocalStorage(user) vm.buttonClicked = false // Configure message to be show vm.signInMsg = { diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index a6a9ceed49..539a350caf 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -3,8 +3,6 @@ import { Opaque } from 'type-fest' import { User } from '../../types/api/user' -import { saveUserToLocalStorage } from './UserService' - // Exported for testing. export const AUTH_ENDPOINT = '/api/v3/auth' @@ -37,8 +35,7 @@ export const sendLoginOtp = async (email: Email): Promise => { } /** - * Verifies the login OTP and saves the returned user to localStorage if OTP is - * valid. + * Verifies the login OTP and returns the user if OTP is valid. * @param params.email the email to verify * @param params.otp the OTP sent to the given email to verify * @returns logged in user when successful @@ -50,8 +47,5 @@ export const verifyLoginOtp = async (params: { }): Promise => { return axios .post(`${AUTH_ENDPOINT}/otp/verify`, params) - .then(({ data }) => { - saveUserToLocalStorage(data) - return data - }) + .then(({ data }) => data) } diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index ac83342465..62b52d11e8 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -9,6 +9,6 @@ export const STORAGE_USER_KEY = 'user' * * @param user the user to save to local storage */ -export const saveUserToLocalStorage = (user: User | null): void => { +export const saveUserToLocalStorage = (user: User): void => { localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) } diff --git a/src/public/services/__tests__/AuthService.test.ts b/src/public/services/__tests__/AuthService.test.ts index d30005de5e..95167d10db 100644 --- a/src/public/services/__tests__/AuthService.test.ts +++ b/src/public/services/__tests__/AuthService.test.ts @@ -8,12 +8,8 @@ import { sendLoginOtp, verifyLoginOtp, } from '../AuthService' -import * as UserService from '../UserService' jest.mock('axios') -jest.mock('../UserService') - -const MockUserService = mocked(UserService) const MockAxios = mocked(axios, true) // Duplicated here instead of exporting from AuthService to prevent production @@ -67,7 +63,7 @@ describe('AuthService', () => { const MOCK_OTP = '123456' const MOCK_EMAIL = 'mockEmail@example.com' - it('should save returned user to localStorage and return user on success', async () => { + it('should return user on success', async () => { // Arrange const mockUser = { _id: 'some id', @@ -81,9 +77,6 @@ describe('AuthService', () => { // Assert expect(actual).toEqual(mockUser) - expect(MockUserService.saveUserToLocalStorage).toHaveBeenCalledWith( - mockUser, - ) expect(MockAxios.post).toHaveBeenCalledWith( EXPECTED_POST_ENDPOINT, expectedParams, From 48dbe663aa058932d4b70c07179ad8aedc703658 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 15:31:13 +0800 Subject: [PATCH 13/28] feat(UserService): add fetchUser function --- src/public/services/UserService.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index 62b52d11e8..6885d6f2d6 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -1,8 +1,12 @@ +import axios from 'axios' + import { User } from '../../types/api/user' /** Exported for testing */ export const STORAGE_USER_KEY = 'user' +const USER_ENDPOINT = '/api/v3/user' + /** * Save logged in user to localStorage. * May not be needed in React depending on implementation. @@ -12,3 +16,22 @@ export const STORAGE_USER_KEY = 'user' export const saveUserToLocalStorage = (user: User): void => { localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) } + +/** + * Fetches the user from the server using the current session cookie. + * + * Side effect: Saves the retrieved user to localStorage + * @returns the logged in user if session is valid, `null` otherwise + */ +export const fetchUser = async (): Promise => { + return axios + .get(USER_ENDPOINT) + .then(({ data: user }) => { + saveUserToLocalStorage(user) + return user + }) + .catch(() => { + saveUserToLocalStorage(null) + return null + }) +} From e1d367710c0555b1766d8f962a96d088cd4c7434 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 15:33:02 +0800 Subject: [PATCH 14/28] ref(avatar-dropdown.client): use UserService.fetchUser() --- .../core/components/avatar-dropdown.client.component.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index f04e80fd55..635f3317a2 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -1,5 +1,7 @@ const get = require('lodash/get') +const UserService = require('../../../services/UserService') + angular.module('core').component('avatarDropdownComponent', { templateUrl: 'modules/core/componentViews/avatar-dropdown.html', bindings: {}, @@ -39,7 +41,7 @@ function avatarDropdownController( async function retrieveUser() { try { - const trueUser = await Auth.refreshUser() + const trueUser = await UserService.fetchUser() if (!trueUser) { $state.go('signin') return From 04216820a83ff40dd66d5c66a8340b541ad8331c Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 16:42:49 +0800 Subject: [PATCH 15/28] test(UserService): add unit tests for fetchUser --- src/public/services/UserService.ts | 4 +- .../services/__tests__/UserService.test.ts | 81 +++++++++++++++---- 2 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index 6885d6f2d6..e07ffac0c0 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -4,8 +4,8 @@ import { User } from '../../types/api/user' /** Exported for testing */ export const STORAGE_USER_KEY = 'user' - -const USER_ENDPOINT = '/api/v3/user' +/** Exported for testing */ +export const USER_ENDPOINT = '/api/v3/user' /** * Save logged in user to localStorage. diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts index 27817f9c8d..610248ac28 100644 --- a/src/public/services/__tests__/UserService.test.ts +++ b/src/public/services/__tests__/UserService.test.ts @@ -1,33 +1,80 @@ +import axios from 'axios' +import { mocked } from 'ts-jest/utils' + import { User } from '../../../types/api/user' -import { saveUserToLocalStorage, STORAGE_USER_KEY } from '../UserService' +import * as UserService from '../UserService' + +const { STORAGE_USER_KEY, USER_ENDPOINT } = UserService + +jest.mock('axios') + +const MockAxios = mocked(axios, true) describe('UserService', () => { + const MOCK_USER: User = { + _id: 'some id' as User['_id'], + email: 'mock@example.com', + agency: { + _id: 'some agency id' as User['agency']['_id'], + emailDomain: ['example.com'], + fullName: 'Example Agency', + logo: 'path/to/agency/logo', + shortName: 'e', + }, + created: 'some created date', + lastAccessed: 'some last accessed date', + updatedAt: 'some last updated at date', + } + + beforeEach(() => { + jest.clearAllMocks() + }) + describe('saveUserToLocalStorage', () => { it('should successfully save given user to localStorage', () => { + // Act + UserService.saveUserToLocalStorage(MOCK_USER) + + // Assert + expect(localStorage.setItem).toHaveBeenLastCalledWith( + STORAGE_USER_KEY, + // Should be stringified. + JSON.stringify(MOCK_USER), + ) + }) + }) + + describe('fetchUser', () => { + it('should save returned user to localStorage when successfully fetched', async () => { // Arrange - const mockUser: User = { - _id: 'some id' as User['_id'], - email: 'mock@example.com', - agency: { - _id: 'some agency id' as User['agency']['_id'], - emailDomain: ['example.com'], - fullName: 'Example Agency', - logo: 'path/to/agency/logo', - shortName: 'e', - }, - created: 'some created date', - lastAccessed: 'some last accessed date', - updatedAt: 'some last updated at date', - } + MockAxios.get.mockResolvedValueOnce({ data: MOCK_USER }) // Act - saveUserToLocalStorage(mockUser) + const actual = await UserService.fetchUser() // Assert + expect(actual).toEqual(MOCK_USER) + expect(MockAxios.get).toHaveBeenLastCalledWith(USER_ENDPOINT) expect(localStorage.setItem).toHaveBeenLastCalledWith( STORAGE_USER_KEY, // Should be stringified. - JSON.stringify(mockUser), + JSON.stringify(MOCK_USER), + ) + }) + + it('should return null and save null user to localStorage on API rejection', async () => { + // Arrange + MockAxios.get.mockRejectedValueOnce(new Error('something error')) + + // Act + const actual = await UserService.fetchUser() + + // Assert + expect(actual).toEqual(null) + expect(MockAxios.get).toHaveBeenLastCalledWith(USER_ENDPOINT) + expect(localStorage.setItem).toHaveBeenLastCalledWith( + STORAGE_USER_KEY, + 'null', ) }) }) From 150b6ed888bca2425eb67692f6db06fe14071c2a Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:15:15 +0800 Subject: [PATCH 16/28] feat: remove unused refreshUser function --- .../modules/users/services/auth.client.service.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/public/modules/users/services/auth.client.service.js b/src/public/modules/users/services/auth.client.service.js index a0b8f715aa..74052c0b9a 100644 --- a/src/public/modules/users/services/auth.client.service.js +++ b/src/public/modules/users/services/auth.client.service.js @@ -23,7 +23,6 @@ function Auth($q, $http, $state, $window) { let authService = { getUser, setUser, - refreshUser, signOut, } return authService @@ -50,19 +49,6 @@ function Auth($q, $http, $state, $window) { return null } } - - function refreshUser() { - return $http - .get('/api/v3/user') - .then(({ data }) => { - setUser(data) - return data - }) - .catch(() => { - setUser(null) - return null - }) - } function signOut() { $http.get('/api/v3/auth/logout').then( function () { From ee8fce58a3351c8fdcb4ec691160c7ded7789157 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 8 Jun 2021 11:16:45 +0800 Subject: [PATCH 17/28] feat(UserService): add clearUserFromLocalStorage function (and tests) --- src/public/services/UserService.ts | 8 ++++++++ src/public/services/__tests__/UserService.test.ts | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index e07ffac0c0..5224c2b5e6 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -17,6 +17,14 @@ export const saveUserToLocalStorage = (user: User): void => { localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(user)) } +/** + * Clear logged in user from localStorage. + * May not even be needed in React depending on implementation. + */ +export const clearUserFromLocalStorage = (): void => { + localStorage.removeItem(STORAGE_USER_KEY) +} + /** * Fetches the user from the server using the current session cookie. * diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts index 610248ac28..6a8b3a26af 100644 --- a/src/public/services/__tests__/UserService.test.ts +++ b/src/public/services/__tests__/UserService.test.ts @@ -44,6 +44,16 @@ describe('UserService', () => { }) }) + describe('clearUserFromLocalStorage', () => { + it('should successfully clear user from localStorage', () => { + // Act + UserService.clearUserFromLocalStorage() + + // Assert + expect(localStorage.removeItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + }) + }) + describe('fetchUser', () => { it('should save returned user to localStorage when successfully fetched', async () => { // Arrange From 86ba290bf7aa0981d7721c0f1a571c9e80e87a33 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 8 Jun 2021 11:17:54 +0800 Subject: [PATCH 18/28] feat: move responsibility of clearing user from localStorage to ctl --- .../avatar-dropdown.client.component.js | 5 +++- src/public/services/UserService.ts | 11 +-------- .../services/__tests__/UserService.test.ts | 23 +------------------ 3 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index 635f3317a2..c14b2df866 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -41,8 +41,11 @@ function avatarDropdownController( async function retrieveUser() { try { - const trueUser = await UserService.fetchUser() + const trueUser = await UserService.fetchUser().then((user) => { + UserService.saveUserToLocalStorage(user) + }) if (!trueUser) { + UserService.clearUserFromLocalStorage() $state.go('signin') return } diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index 5224c2b5e6..f034bf08c5 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -32,14 +32,5 @@ export const clearUserFromLocalStorage = (): void => { * @returns the logged in user if session is valid, `null` otherwise */ export const fetchUser = async (): Promise => { - return axios - .get(USER_ENDPOINT) - .then(({ data: user }) => { - saveUserToLocalStorage(user) - return user - }) - .catch(() => { - saveUserToLocalStorage(null) - return null - }) + return axios.get(USER_ENDPOINT).then(({ data }) => data) } diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts index 6a8b3a26af..bd76e37455 100644 --- a/src/public/services/__tests__/UserService.test.ts +++ b/src/public/services/__tests__/UserService.test.ts @@ -55,7 +55,7 @@ describe('UserService', () => { }) describe('fetchUser', () => { - it('should save returned user to localStorage when successfully fetched', async () => { + it('should return user successfully', async () => { // Arrange MockAxios.get.mockResolvedValueOnce({ data: MOCK_USER }) @@ -65,27 +65,6 @@ describe('UserService', () => { // Assert expect(actual).toEqual(MOCK_USER) expect(MockAxios.get).toHaveBeenLastCalledWith(USER_ENDPOINT) - expect(localStorage.setItem).toHaveBeenLastCalledWith( - STORAGE_USER_KEY, - // Should be stringified. - JSON.stringify(MOCK_USER), - ) - }) - - it('should return null and save null user to localStorage on API rejection', async () => { - // Arrange - MockAxios.get.mockRejectedValueOnce(new Error('something error')) - - // Act - const actual = await UserService.fetchUser() - - // Assert - expect(actual).toEqual(null) - expect(MockAxios.get).toHaveBeenLastCalledWith(USER_ENDPOINT) - expect(localStorage.setItem).toHaveBeenLastCalledWith( - STORAGE_USER_KEY, - 'null', - ) }) }) }) From 6a908374bb042221153663f1b11cde8b5803487b Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Tue, 8 Jun 2021 11:42:54 +0800 Subject: [PATCH 19/28] fix: correctly set truser when fetching user from server --- .../components/avatar-dropdown.client.component.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index c14b2df866..218082c7b0 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -41,11 +41,17 @@ function avatarDropdownController( async function retrieveUser() { try { - const trueUser = await UserService.fetchUser().then((user) => { - UserService.saveUserToLocalStorage(user) - }) + const trueUser = await UserService.fetchUser() + .then((user) => { + UserService.saveUserToLocalStorage(user) + return user + }) + .catch(() => { + UserService.clearUserFromLocalStorage() + return null + }) + if (!trueUser) { - UserService.clearUserFromLocalStorage() $state.go('signin') return } From e2e94251d726d298544dbf94495d036fb2447db4 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 17:38:10 +0800 Subject: [PATCH 20/28] ref: use UserService.saveUserToLocalStorage --- .../modules/core/components/avatar-dropdown.client.component.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index 218082c7b0..bf9b8eb3e0 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -109,7 +109,7 @@ function avatarDropdownController( // Update success, update user. if (returnVal) { vm.user = returnVal - Auth.setUser(returnVal) + UserService.saveUserToLocalStorage(returnVal) vm.showExclamation = !returnVal.contact } }) From 501c9b5d24d0210bb8766337c9b790725ac9c4cc Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Thu, 3 Jun 2021 18:01:53 +0800 Subject: [PATCH 21/28] feat(AuthService): add and use logout function --- .../core/componentViews/avatar-dropdown.html | 2 +- .../avatar-dropdown.client.component.js | 18 +++++++++++++++++- src/public/services/AuthService.ts | 4 ++++ src/public/services/UserService.ts | 3 ++- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/public/modules/core/componentViews/avatar-dropdown.html b/src/public/modules/core/componentViews/avatar-dropdown.html index 37e3c975fd..57aa85af90 100644 --- a/src/public/modules/core/componentViews/avatar-dropdown.html +++ b/src/public/modules/core/componentViews/avatar-dropdown.html @@ -68,7 +68,7 @@
  • Logout diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index bf9b8eb3e0..2d7cf9b7e3 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -1,11 +1,13 @@ const get = require('lodash/get') +const AuthService = require('../../../services/AuthService') const UserService = require('../../../services/UserService') angular.module('core').component('avatarDropdownComponent', { templateUrl: 'modules/core/componentViews/avatar-dropdown.html', bindings: {}, controller: [ + '$q', '$scope', '$state', '$uibModal', @@ -19,6 +21,7 @@ angular.module('core').component('avatarDropdownComponent', { }) function avatarDropdownController( + $q, $scope, $state, $uibModal, @@ -92,7 +95,20 @@ function avatarDropdownController( }, ) - vm.signOut = () => Auth.signOut() + vm.logout = () => { + return $q + .when(AuthService.logout()) + .then(() => { + // Clear user and contact banner on logout + UserService.clearUserFromLocalStorage() + $window.localStorage.removeItem('contactBannerDismissed') + // Redirect to landing page + $state.go('landing') + }) + .catch((error) => { + console.error('sign out failed:', error) + }) + } vm.openContactNumberModal = () => { $uibModal diff --git a/src/public/services/AuthService.ts b/src/public/services/AuthService.ts index 539a350caf..0dbdd7477a 100644 --- a/src/public/services/AuthService.ts +++ b/src/public/services/AuthService.ts @@ -49,3 +49,7 @@ export const verifyLoginOtp = async (params: { .post(`${AUTH_ENDPOINT}/otp/verify`, params) .then(({ data }) => data) } + +export const logout = async (): Promise => { + return axios.get(`${AUTH_ENDPOINT}/logout`) +} diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index f034bf08c5..94494674a8 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -28,7 +28,8 @@ export const clearUserFromLocalStorage = (): void => { /** * Fetches the user from the server using the current session cookie. * - * Side effect: Saves the retrieved user to localStorage + * Side effect: On success, save the retrieved user to localStorage. + * Side effect: On error, clear the user (if any) from localStorage. * @returns the logged in user if session is valid, `null` otherwise */ export const fetchUser = async (): Promise => { From ead3fccc5a7517041f4e1d5f166e44bde2c45d5f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:23:41 +0800 Subject: [PATCH 22/28] feat: remove unused Auth.signOut and setUser functions --- .../users/services/auth.client.service.js | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/src/public/modules/users/services/auth.client.service.js b/src/public/modules/users/services/auth.client.service.js index 74052c0b9a..1219d46366 100644 --- a/src/public/modules/users/services/auth.client.service.js +++ b/src/public/modules/users/services/auth.client.service.js @@ -22,19 +22,9 @@ function Auth($q, $http, $state, $window) { let authService = { getUser, - setUser, - signOut, } return authService - /** - * User setter function - * @param {User} user - */ - function setUser(user) { - $window.localStorage.setItem('user', JSON.stringify(user)) - } - /** * User getter function * @returns {User} user @@ -49,17 +39,4 @@ function Auth($q, $http, $state, $window) { return null } } - function signOut() { - $http.get('/api/v3/auth/logout').then( - function () { - $window.localStorage.removeItem('user') - // Clear contact banner on logout - $window.localStorage.removeItem('contactBannerDismissed') - $state.go('landing') - }, - function (error) { - console.error('sign out failed:', error) - }, - ) - } } From 4b50c6978ebcb7ca29a668a6e46d2df508012f07 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:27:11 +0800 Subject: [PATCH 23/28] test(AuthService): add unit test for logout test(UserService): add tests for clearUserFromLocalStorage --- .../services/__tests__/AuthService.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/public/services/__tests__/AuthService.test.ts b/src/public/services/__tests__/AuthService.test.ts index 95167d10db..2969cb045a 100644 --- a/src/public/services/__tests__/AuthService.test.ts +++ b/src/public/services/__tests__/AuthService.test.ts @@ -5,6 +5,7 @@ import { Opaque } from 'type-fest' import { AUTH_ENDPOINT, checkIsEmailAllowed, + logout, sendLoginOtp, verifyLoginOtp, } from '../AuthService' @@ -83,4 +84,20 @@ describe('AuthService', () => { ) }) }) + + describe('logout', () => { + const EXPECTED_ENDPOINT = `${AUTH_ENDPOINT}/logout` + it('should call endpoint successfully', async () => { + // Arrange + const mockReturn = { status: 200 } + MockAxios.get.mockResolvedValueOnce(mockReturn) + + // Act + const actual = await logout() + + // Assert + expect(actual).toEqual(mockReturn) + expect(MockAxios.get).toHaveBeenLastCalledWith(EXPECTED_ENDPOINT) + }) + }) }) From 352368f7c467873a9acecd60bed795d2b6ccc64c Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 4 Jun 2021 11:49:25 +0800 Subject: [PATCH 24/28] feat(UserService): add getUserFromLocalStorage function --- src/public/services/UserService.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/public/services/UserService.ts b/src/public/services/UserService.ts index 94494674a8..2cda68d0d0 100644 --- a/src/public/services/UserService.ts +++ b/src/public/services/UserService.ts @@ -7,6 +7,27 @@ export const STORAGE_USER_KEY = 'user' /** Exported for testing */ export const USER_ENDPOINT = '/api/v3/user' +/** + * Get logged in user from localStorage. + * May not be needed in React depending on implementation. + * + * @returns user if available, null otherwise + */ +export const getUserFromLocalStorage = (): User | null => { + const userStringified = localStorage.getItem(STORAGE_USER_KEY) + + if (userStringified) { + try { + return User.parse(JSON.parse(userStringified)) + } catch (error) { + // Invalid shape, clear from storage. + clearUserFromLocalStorage() + return null + } + } + return null +} + /** * Save logged in user to localStorage. * May not be needed in React depending on implementation. From aea00487eeacb7440c197cc18ae81705c2ac4c06 Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 4 Jun 2021 11:58:07 +0800 Subject: [PATCH 25/28] ref: replace all instances of Auth.getUser with UserService equivalent --- .../avatar-dropdown.client.component.js | 4 +--- ...t-contact-number-modal.client.controller.js | 6 +++--- .../core/services/gtag.client.service.js | 8 +++++--- .../components/share-form.client.component.js | 12 +++++++----- .../admin-form.client.controller.js | 5 ++--- .../edit-fields-modal.client.controller.js | 18 +++++++++++------- .../list-forms.client.controller.js | 6 +++--- .../settings-form.client.directive.js | 7 +++---- .../forms/config/forms.client.routes.js | 7 +++---- .../authentication.client.controller.js | 13 ++----------- .../controllers/billing.client.controller.js | 6 +++--- .../examples-card.client.directive.js | 6 +++--- .../examples-list.client.controller.js | 14 +++----------- 13 files changed, 49 insertions(+), 63 deletions(-) diff --git a/src/public/modules/core/components/avatar-dropdown.client.component.js b/src/public/modules/core/components/avatar-dropdown.client.component.js index 2d7cf9b7e3..4384ed05d0 100644 --- a/src/public/modules/core/components/avatar-dropdown.client.component.js +++ b/src/public/modules/core/components/avatar-dropdown.client.component.js @@ -12,7 +12,6 @@ angular.module('core').component('avatarDropdownComponent', { '$state', '$uibModal', '$window', - 'Auth', 'Features', 'Toastr', avatarDropdownController, @@ -26,14 +25,13 @@ function avatarDropdownController( $state, $uibModal, $window, - Auth, Features, Toastr, ) { const vm = this // Preload user with current details, redirect to signin if unable to get user - vm.user = Auth.getUser() || $state.go('signin') + vm.user = UserService.getUserFromLocalStorage() || $state.go('signin') vm.avatarText = generateAvatarText() vm.isDropdownHover = false diff --git a/src/public/modules/core/controllers/edit-contact-number-modal.client.controller.js b/src/public/modules/core/controllers/edit-contact-number-modal.client.controller.js index 3ad98f2a8e..f2628145bc 100644 --- a/src/public/modules/core/controllers/edit-contact-number-modal.client.controller.js +++ b/src/public/modules/core/controllers/edit-contact-number-modal.client.controller.js @@ -1,3 +1,5 @@ +const UserService = require('../../../services/UserService') + angular .module('core') .controller('EditContactNumberModalController', [ @@ -8,7 +10,6 @@ angular '$scope', '$uibModalInstance', '$window', - 'Auth', 'Toastr', EditContactNumberModalController, ]) @@ -21,7 +22,6 @@ function EditContactNumberModalController( $scope, $uibModalInstance, $window, - Auth, Toastr, ) { const vm = this @@ -47,7 +47,7 @@ function EditContactNumberModalController( vm.VERIFY_STATE = VERIFY_STATE // Redirect to signin if unable to get user - vm.user = Auth.getUser() || $state.go('signin') + vm.user = UserService.getUserFromLocalStorage() || $state.go('signin') vm.vfnState = vm.user.contact ? VERIFY_STATE.SUCCESS : VERIFY_STATE.IDLE diff --git a/src/public/modules/core/services/gtag.client.service.js b/src/public/modules/core/services/gtag.client.service.js index 632ff5b1bb..d468f221ee 100644 --- a/src/public/modules/core/services/gtag.client.service.js +++ b/src/public/modules/core/services/gtag.client.service.js @@ -1,12 +1,14 @@ -angular.module('core').factory('GTag', ['Auth', '$rootScope', '$window', GTag]) +const UserService = require('../../../services/UserService') -function GTag(Auth, $rootScope, $window) { +angular.module('core').factory('GTag', ['$rootScope', '$window', GTag]) + +function GTag($rootScope, $window) { // Google Analytics tracking ID provided on signup. const GATrackingID = $window.GATrackingID let gtagService = {} const getUserEmail = () => { - const user = Auth.getUser() + const user = UserService.getUserFromLocalStorage() return user && user.email } diff --git a/src/public/modules/forms/admin/components/share-form.client.component.js b/src/public/modules/forms/admin/components/share-form.client.component.js index 8f06d5d00b..cbd8abd34e 100644 --- a/src/public/modules/forms/admin/components/share-form.client.component.js +++ b/src/public/modules/forms/admin/components/share-form.client.component.js @@ -1,5 +1,7 @@ 'use strict' -let dedent = require('dedent-js') +const dedent = require('dedent-js') + +const UserService = require('../../../../services/UserService') angular.module('forms').component('shareFormComponent', { templateUrl: 'modules/forms/admin/componentViews/share-form.client.view.html', @@ -9,11 +11,11 @@ angular.module('forms').component('shareFormComponent', { status: '<', userCanEdit: '<', }, - controller: ['$state', '$translate', 'Auth', shareFormController], + controller: ['$state', '$translate', shareFormController], controllerAs: 'vm', }) -function shareFormController($state, $translate, Auth) { +function shareFormController($state, $translate) { const vm = this vm.$onInit = () => { @@ -54,6 +56,6 @@ function shareFormController($state, $translate, Auth) { } // Show different pro tip for user depending on their email - const { email } = Auth.getUser() - vm.isGovOfficer = String(email).endsWith('.gov.sg') + const user = UserService.getUserFromLocalStorage() + vm.isGovOfficer = user && String(user.email).endsWith('.gov.sg') } diff --git a/src/public/modules/forms/admin/controllers/admin-form.client.controller.js b/src/public/modules/forms/admin/controllers/admin-form.client.controller.js index 51f148f7b9..9efd6f5912 100644 --- a/src/public/modules/forms/admin/controllers/admin-form.client.controller.js +++ b/src/public/modules/forms/admin/controllers/admin-form.client.controller.js @@ -4,6 +4,7 @@ const { StatusCodes } = require('http-status-codes') const get = require('lodash/get') const { LogicType } = require('../../../../../types') const UpdateFormService = require('../../../../services/UpdateFormService') +const UserService = require('../../../../services/UserService') const FieldFactory = require('../../helpers/field-factory') const { UPDATE_FORM_TYPES } = require('../constants/update-form-types') @@ -42,7 +43,6 @@ angular '$uibModal', 'FormData', 'FormFields', - 'Auth', 'moment', 'Toastr', '$state', @@ -58,7 +58,6 @@ function AdminFormController( $uibModal, FormData, FormFields, - Auth, moment, Toastr, $state, @@ -71,7 +70,7 @@ function AdminFormController( // Redirect to signin if unable to get user $scope.user = - Auth.getUser() || + UserService.getUserFromLocalStorage() || $state.go( 'signin', { diff --git a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js index 997c3ed742..5edbc987bc 100644 --- a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js @@ -4,6 +4,8 @@ const axios = require('axios').default const values = require('lodash/values') const cloneDeep = require('lodash/cloneDeep') +const UserService = require('../../../../services/UserService') + const { VALID_UPLOAD_FILE_TYPES, MAX_UPLOAD_FILE_SIZE, @@ -28,7 +30,6 @@ angular 'Attachment', 'FormFields', '$q', - 'Auth', '$state', 'Toastr', EditFieldsModalController, @@ -42,7 +43,6 @@ function EditFieldsModalController( Attachment, FormFields, $q, - Auth, $state, Toastr, ) { @@ -66,7 +66,7 @@ function EditFieldsModalController( } // Serialize allowed email domains - vm.user = Auth.getUser() || $state.go('signin') + vm.user = UserService.getUserFromLocalStorage() || $state.go('signin') if (vm.field.fieldType === 'email') { const userEmailDomain = '@' + vm.user.email.split('@').pop() @@ -81,8 +81,9 @@ function EditFieldsModalController( vm.field.allowedEmailDomainsPlaceholder = `${userEmailDomain}\n@agency.gov.sg` if (vm.field.hasAllowedEmailDomains) { - vm.field.allowedEmailDomainsFromText = - vm.field.allowedEmailDomains.join('\n') + vm.field.allowedEmailDomainsFromText = vm.field.allowedEmailDomains.join( + '\n', + ) } $scope.$watch('vm.field.isVerifiable', (newValue) => { if (newValue) { @@ -195,8 +196,11 @@ function EditFieldsModalController( return false } - const { selectedDateValidation, customMinDate, customMaxDate } = - dateValidation + const { + selectedDateValidation, + customMinDate, + customMaxDate, + } = dateValidation if (selectedDateValidation !== DateValidationOptions.Custom) { return false diff --git a/src/public/modules/forms/admin/controllers/list-forms.client.controller.js b/src/public/modules/forms/admin/controllers/list-forms.client.controller.js index f9ed175d8f..ece7565ded 100644 --- a/src/public/modules/forms/admin/controllers/list-forms.client.controller.js +++ b/src/public/modules/forms/admin/controllers/list-forms.client.controller.js @@ -3,6 +3,8 @@ const get = require('lodash/get') const BetaService = require('../../../../services/BetaService') +const UserService = require('../../../../services/UserService') + // Forms controller angular .module('forms') @@ -10,7 +12,6 @@ angular '$scope', 'FormApi', '$uibModal', - 'Auth', 'moment', '$state', '$timeout', @@ -24,7 +25,6 @@ function ListFormsController( $scope, FormApi, $uibModal, - Auth, moment, $state, $timeout, @@ -41,7 +41,7 @@ function ListFormsController( // Duplicated form outline on newly dup forms vm.duplicatedForms = [] // Redirect to signin if unable to get user - vm.user = Auth.getUser() || $state.go('signin') + vm.user = UserService.getUserFromLocalStorage() || $state.go('signin') // Brings user to edit form page vm.editForm = function (form) { diff --git a/src/public/modules/forms/admin/directives/settings-form.client.directive.js b/src/public/modules/forms/admin/directives/settings-form.client.directive.js index da44a48570..8e2826fbc9 100644 --- a/src/public/modules/forms/admin/directives/settings-form.client.directive.js +++ b/src/public/modules/forms/admin/directives/settings-form.client.directive.js @@ -3,6 +3,8 @@ const dedent = require('dedent-js') const { get, set, isEqual } = require('lodash') const AdminSubmissionsService = require('../../../../services/AdminSubmissionsService') +const UserService = require('../../../../services/UserService') + const SETTINGS_PATH = [ 'title', 'emails', @@ -35,8 +37,6 @@ angular '$timeout', 'responseModeEnum', '$uibModal', - 'Auth', - 'Submissions', settingsFormDirective, ]) @@ -46,7 +46,6 @@ function settingsFormDirective( $timeout, responseModeEnum, $uibModal, - Auth, ) { return { templateUrl: @@ -59,7 +58,7 @@ function settingsFormDirective( controller: [ '$scope', function ($scope) { - $scope.user = Auth.getUser() + $scope.user = UserService.getUserFromLocalStorage() $scope.responseModeEnum = responseModeEnum $scope.tempForm = createTempSettings($scope.myform) diff --git a/src/public/modules/forms/config/forms.client.routes.js b/src/public/modules/forms/config/forms.client.routes.js index 220e2ece7f..07bb491756 100644 --- a/src/public/modules/forms/config/forms.client.routes.js +++ b/src/public/modules/forms/config/forms.client.routes.js @@ -1,6 +1,7 @@ 'use strict' const ExamplesService = require('../../../services/ExamplesService') +const UserService = require('../../../services/UserService') // Setting up route angular.module('forms').config([ @@ -74,16 +75,14 @@ angular.module('forms').config([ url: '/{formId:[0-9a-fA-F]{24}}/use-template', templateUrl: 'modules/users/views/examples.client.view.html', resolve: { - Auth: 'Auth', FormErrorService: 'FormErrorService', // If the user is logged in, this field will contain the form data of the provided formId, // otherwise it will only contain the formId itself. FormData: [ - 'Auth', 'FormErrorService', '$stateParams', - function (Auth, FormErrorService, $stateParams) { - if (!Auth.getUser()) { + function (FormErrorService, $stateParams) { + if (!UserService.getUserFromLocalStorage()) { return $stateParams.formId } diff --git a/src/public/modules/users/controllers/authentication.client.controller.js b/src/public/modules/users/controllers/authentication.client.controller.js index 0941dfb764..9fb498d2a1 100755 --- a/src/public/modules/users/controllers/authentication.client.controller.js +++ b/src/public/modules/users/controllers/authentication.client.controller.js @@ -14,20 +14,11 @@ angular '$state', '$timeout', '$window', - 'Auth', 'GTag', AuthenticationController, ]) -function AuthenticationController( - $q, - $scope, - $state, - $timeout, - $window, - Auth, - GTag, -) { +function AuthenticationController($q, $scope, $state, $timeout, $window, GTag) { const vm = this let notifDelayTimeout @@ -73,7 +64,7 @@ function AuthenticationController( * Redirects user with active session to target page */ vm.redirectIfActiveSession = function () { - if (Auth.getUser()) { + if (UserService.getUserFromLocalStorage()) { $state.go($state.params.targetState) } } diff --git a/src/public/modules/users/controllers/billing.client.controller.js b/src/public/modules/users/controllers/billing.client.controller.js index 5a580bcfbf..21a6972568 100644 --- a/src/public/modules/users/controllers/billing.client.controller.js +++ b/src/public/modules/users/controllers/billing.client.controller.js @@ -2,6 +2,7 @@ const { CsvGenerator } = require('../../forms/helpers/CsvGenerator') const BillingService = require('../../../services/BillingService') +const UserService = require('../../../services/UserService') angular .module('users') @@ -9,15 +10,14 @@ angular '$q', '$state', '$timeout', - 'Auth', 'NgTableParams', BillingController, ]) -function BillingController($q, $state, $timeout, Auth, NgTableParams) { +function BillingController($q, $state, $timeout, NgTableParams) { const vm = this - vm.user = Auth.getUser() + vm.user = UserService.getUserFromLocalStorage() // Send non-logged in personnel to sign in page if (!vm.user) { diff --git a/src/public/modules/users/controllers/examples-card.client.directive.js b/src/public/modules/users/controllers/examples-card.client.directive.js index d0f9faf37a..e2897490bb 100644 --- a/src/public/modules/users/controllers/examples-card.client.directive.js +++ b/src/public/modules/users/controllers/examples-card.client.directive.js @@ -1,6 +1,8 @@ 'use strict' const BetaService = require('../../../services/BetaService') +const UserService = require('../../../services/UserService') + angular.module('users').directive('examplesCard', [examplesCard]) function examplesCard() { @@ -20,7 +22,6 @@ function examplesCard() { '$uibModal', '$state', 'GTag', - 'Auth', '$location', 'Toastr', '$q', @@ -36,12 +37,11 @@ function examplesCardController( $uibModal, $state, GTag, - Auth, $location, Toastr, $q, ) { - $scope.user = Auth.getUser() + $scope.user = UserService.getUserFromLocalStorage() $scope.openTemplateModal = () => { $scope.templateUrl = $state.href( diff --git a/src/public/modules/users/controllers/examples-list.client.controller.js b/src/public/modules/users/controllers/examples-list.client.controller.js index c1a6c6047c..5569241b41 100644 --- a/src/public/modules/users/controllers/examples-list.client.controller.js +++ b/src/public/modules/users/controllers/examples-list.client.controller.js @@ -1,13 +1,13 @@ 'use strict' const ExamplesService = require('../../../services/ExamplesService') +const UserService = require('../../../services/UserService') const PAGE_SIZE = 16 angular.module('users').controller('ExamplesController', [ '$q', '$scope', - 'Auth', 'GTag', '$state', '$timeout', @@ -18,18 +18,10 @@ angular.module('users').controller('ExamplesController', [ ExamplesController, ]) -function ExamplesController( - $q, - $scope, - Auth, - GTag, - $state, - $timeout, - FormData, -) { +function ExamplesController($q, $scope, GTag, $state, $timeout, FormData) { const vm = this - vm.user = Auth.getUser() + vm.user = UserService.getUserFromLocalStorage() // Send non-logged in personnel to sign in page if (!vm.user) { From f881aa9c963a260fbf7296481caf2fb8c7042b2f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Fri, 4 Jun 2021 13:59:09 +0800 Subject: [PATCH 26/28] feat: delete unused auth.client.service.js --- src/public/main.js | 3 -- .../users/services/auth.client.service.js | 42 ------------------- 2 files changed, 45 deletions(-) delete mode 100644 src/public/modules/users/services/auth.client.service.js diff --git a/src/public/main.js b/src/public/main.js index e29766efe4..62c40691f4 100644 --- a/src/public/main.js +++ b/src/public/main.js @@ -275,9 +275,6 @@ require('./modules/forms/services/mailto.client.factory.js') require('./modules/users/config/users.client.config.js') require('./modules/users/config/users.client.routes.js') -// User services -require('./modules/users/services/auth.client.service.js') - // User controllers require('./modules/users/controllers/authentication.client.controller.js') require('./modules/users/controllers/billing.client.controller.js') diff --git a/src/public/modules/users/services/auth.client.service.js b/src/public/modules/users/services/auth.client.service.js deleted file mode 100644 index 1219d46366..0000000000 --- a/src/public/modules/users/services/auth.client.service.js +++ /dev/null @@ -1,42 +0,0 @@ -'use strict' - -angular - .module('users') - .factory('Auth', ['$q', '$http', '$state', '$window', Auth]) - -function Auth($q, $http, $state, $window) { - /** - * Object representing a logged-in user and agency - * @typedef {Object} User - * @property {String} _id - The database ID of the user - * @property {String} email - The email of the user - * @property {Object} betaFlags - Whether the user has beta privileges for different features - * @property {Object} agency - The agency that the user belongs to - * @property {String} agency.created - Date created - * @property {Array} agency.emailDomain - Email domains belonging to agency - * @property {String} agency.fullName - The full name of the agency - * @property {String} agency.shortName - The short name of the agency - * @property {String} agency.logo - URI specifying location of agency logo - * @property {String} agency._id - The database ID of the agency - */ - - let authService = { - getUser, - } - return authService - - /** - * User getter function - * @returns {User} user - */ - function getUser() { - // TODO: For backwards compatibility in case user is still logged in - // and details are still saved on window - let user = $window.localStorage.getItem('user') - try { - return user && JSON.parse(user) - } catch (error) { - return null - } - } -} From 42c9e3663b3a3b083cab1f7d7c4a41648b1b496e Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:37:35 +0800 Subject: [PATCH 27/28] test(UserService): add unit tests for getUserFromLocalStorage --- .../services/__tests__/UserService.test.ts | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/public/services/__tests__/UserService.test.ts b/src/public/services/__tests__/UserService.test.ts index bd76e37455..e4097cf7aa 100644 --- a/src/public/services/__tests__/UserService.test.ts +++ b/src/public/services/__tests__/UserService.test.ts @@ -30,6 +30,49 @@ describe('UserService', () => { jest.clearAllMocks() }) + describe('getUserFromLocalStorage', () => { + it('should return user from localStorage when valid user exists', async () => { + // Arrange + localStorage.setItem(STORAGE_USER_KEY, JSON.stringify(MOCK_USER)) + + // Act + const actual = UserService.getUserFromLocalStorage() + + // Assert + expect(actual).toEqual(MOCK_USER) + expect(localStorage.getItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + }) + + it('should return null and clear user from localStorage when user is an invalid shape', async () => { + // Arrange + localStorage.setItem( + STORAGE_USER_KEY, + 'this string is obviously not a well formed user', + ) + + // Act + const actual = UserService.getUserFromLocalStorage() + + // Assert + expect(localStorage.getItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + expect(actual).toEqual(null) + expect(localStorage.removeItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + }) + + it('should return null and clear user from localStorage when JSON.parse throws', async () => { + // Arrange + localStorage.setItem(STORAGE_USER_KEY, '{ invalid JSON string') + + // Act + const actual = UserService.getUserFromLocalStorage() + + // Assert + expect(localStorage.getItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + expect(actual).toEqual(null) + expect(localStorage.removeItem).toHaveBeenLastCalledWith(STORAGE_USER_KEY) + }) + }) + describe('saveUserToLocalStorage', () => { it('should successfully save given user to localStorage', () => { // Act From 62b4e23199895c688b167205b007e781544d380f Mon Sep 17 00:00:00 2001 From: Kar Rui Lau Date: Mon, 7 Jun 2021 17:55:45 +0800 Subject: [PATCH 28/28] chore: run npm run lint i didn't even touch this part of the code... --- .../edit-fields-modal.client.controller.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js index 5edbc987bc..2ffef32d71 100644 --- a/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js +++ b/src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js @@ -81,9 +81,8 @@ function EditFieldsModalController( vm.field.allowedEmailDomainsPlaceholder = `${userEmailDomain}\n@agency.gov.sg` if (vm.field.hasAllowedEmailDomains) { - vm.field.allowedEmailDomainsFromText = vm.field.allowedEmailDomains.join( - '\n', - ) + vm.field.allowedEmailDomainsFromText = + vm.field.allowedEmailDomains.join('\n') } $scope.$watch('vm.field.isVerifiable', (newValue) => { if (newValue) { @@ -196,11 +195,8 @@ function EditFieldsModalController( return false } - const { - selectedDateValidation, - customMinDate, - customMaxDate, - } = dateValidation + const { selectedDateValidation, customMinDate, customMaxDate } = + dateValidation if (selectedDateValidation !== DateValidationOptions.Custom) { return false