From 69d5a87861f199afe1a52b00606d0d9ac2917f1d Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Tue, 14 May 2019 16:49:57 +0200 Subject: [PATCH] Fixes API controller tests The tests mock the mogoose User model and the express Response model which isn't good. We should find a solution that makes use of the actual model object. --- .../user.controller/__tests__/apiKey.test.js | 92 +++++++++++++------ server/controllers/user.controller/apiKey.js | 27 +++++- 2 files changed, 89 insertions(+), 30 deletions(-) diff --git a/server/controllers/user.controller/__tests__/apiKey.test.js b/server/controllers/user.controller/__tests__/apiKey.test.js index 01fe67e8..8d1396c2 100644 --- a/server/controllers/user.controller/__tests__/apiKey.test.js +++ b/server/controllers/user.controller/__tests__/apiKey.test.js @@ -1,13 +1,18 @@ /* @jest-environment node */ +import last from 'lodash/last'; import { createApiKey, removeApiKey } from '../../user.controller/apiKey'; jest.mock('../../../models/user'); -const createResponseMock = function (done) { +/* + Create a mock object representing an express Response +*/ +const createResponseMock = function createResponseMock(done) { const json = jest.fn(() => { if (done) { done(); } }); + const status = jest.fn(() => ({ json })); return { @@ -16,6 +21,41 @@ const createResponseMock = function (done) { }; }; +/* + Create a mock of the mongoose User model +*/ +const createUserMock = function createUserMock() { + const apiKeys = []; + let nextId = 0; + + apiKeys.push = ({ label, hashedKey }) => { + const id = nextId; + nextId += 1; + const publicFields = { id, label }; + const allFields = { ...publicFields, hashedKey }; + + Object.defineProperty(allFields, 'publicFields', { + value: publicFields, + enumerable: false + }); + + return Array.prototype.push.call(apiKeys, allFields); + }; + + apiKeys.pull = ({ _id }) => { + const index = apiKeys.findIndex(({ id }) => id === _id); + return apiKeys.splice(index, 1); + }; + + return { + apiKeys, + get publicApiKeys() { + return apiKeys.map(k => k.publicFields) + }, + save: jest.fn(callback => callback()) + }; +}; + const User = require('../../../models/user').default; describe('user.controller', () => { @@ -38,9 +78,8 @@ describe('user.controller', () => { }); it('returns an error if label not provided', () => { - User.__setFindById(undefined, { - apiKeys: [] - }); + User.__setFindById(undefined, createUserMock()); + const request = { user: { id: '1234' }, body: {} }; const response = createResponseMock(); @@ -60,17 +99,18 @@ describe('user.controller', () => { body: { label: 'my key' } }; - const foundUser = { - apiKeys: [], - save: jest.fn(callback => callback()) - }; + const user = createUserMock(); const checkExpecations = () => { - expect(foundUser.apiKeys[0].label).toBe('my key'); - expect(typeof foundUser.apiKeys[0].hashedKey).toBe('string'); + const lastKey = last(user.apiKeys); + + expect(lastKey.label).toBe('my key'); + expect(typeof lastKey.hashedKey).toBe('string'); expect(response.json).toHaveBeenCalledWith({ - token: foundUser.apiKeys[0].hashedKey + apiKeys: [ + { id: 0, label: 'my key', token: lastKey.hashedKey } + ] }); done(); @@ -78,7 +118,7 @@ describe('user.controller', () => { response = createResponseMock(checkExpecations); - User.__setFindById(undefined, foundUser); + User.__setFindById(undefined, user); createApiKey(request, response); }); @@ -105,11 +145,8 @@ describe('user.controller', () => { }; const response = createResponseMock(); - const foundUser = { - apiKeys: [], - save: jest.fn(callback => callback()) - }; - User.__setFindById(undefined, foundUser); + const user = createUserMock(); + User.__setFindById(undefined, user); removeApiKey(request, response); @@ -119,24 +156,27 @@ describe('user.controller', () => { }); }); - it.skip('removes key if it exists', () => { + it('removes key if it exists', () => { const request = { user: { id: '1234' }, - params: { keyId: 'the-key' } + params: { keyId: 0 } }; const response = createResponseMock(); - const foundUser = { - apiKeys: [{ label: 'the-label', id: 'the-key' }], - save: jest.fn(callback => callback()) - }; - User.__setFindById(undefined, foundUser); + const user = createUserMock(); + + user.apiKeys.push({ label: 'first key' }); // id 0 + user.apiKeys.push({ label: 'second key' }); // id 1 + + User.__setFindById(undefined, user); removeApiKey(request, response); - expect(response.status).toHaveBeenCalledWith(404); + expect(response.status).toHaveBeenCalledWith(200); expect(response.json).toHaveBeenCalledWith({ - error: 'Key does not exist for user' + apiKeys: [ + { id: 1, label: 'second key' } + ] }); }); }); diff --git a/server/controllers/user.controller/apiKey.js b/server/controllers/user.controller/apiKey.js index 35a0ba36..80f508f0 100644 --- a/server/controllers/user.controller/apiKey.js +++ b/server/controllers/user.controller/apiKey.js @@ -30,9 +30,9 @@ export function createApiKey(req, res) { return; } - const hashedKey = await generateApiKey(); + const keyToBeHashed = await generateApiKey(); - user.apiKeys.push({ label: req.body.label, hashedKey }); + const addedApiKeyIndex = user.apiKeys.push({ label: req.body.label, hashedKey: keyToBeHashed }); user.save((saveErr) => { if (saveErr) { @@ -40,7 +40,17 @@ export function createApiKey(req, res) { return; } - res.json({ token: hashedKey }); + const apiKeys = user.apiKeys + .map((apiKey, index) => { + const fields = apiKey.publicFields; + const shouldIncludeToken = index === addedApiKeyIndex - 1; + + return shouldIncludeToken ? + { ...fields, token: keyToBeHashed } : + fields; + }); + + res.json({ apiKeys }); }); }); } @@ -60,7 +70,16 @@ export function removeApiKey(req, res) { res.status(404).json({ error: 'Key does not exist for user' }); return; } + user.apiKeys.pull({ _id: req.params.keyId }); - saveUser(res, user); + + user.save((saveErr) => { + if (saveErr) { + res.status(500).json({ error: saveErr }); + return; + } + + res.status(200).json({ apiKeys: user.publicApiKeys }); + }); }); }