Skip to content

Commit

Permalink
QueryLabels refactoring
Browse files Browse the repository at this point in the history
In the response to the request to add or remove a label to/from a query,
we need to return the updated query object.

In order to achieve this in a way that didn't lead to a tonne of
duplicate code, I have refactored the 'addEmbeds' function into a new
helper.

Also, the controllers for adding a label to a query, and removing a
label from a query were, unsurprisingly, almost identical. I have
therefore replaced them with a single function.

Includes:

- addLabelToQuery & removeLabelFromQuery replaced with addRemove
- The relevant router now passes an argument to addRemove to indicate
what operation it should carry out
- New helpers/queries.js helper
- New units tests where appropriate
- Modified unit tests where appropriate
  • Loading branch information
Andrew Isherwood committed Jul 7, 2020
1 parent 76b213e commit c3f930a
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 111 deletions.
8 changes: 6 additions & 2 deletions api/v1.0/routes/queries.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ router.get('/:id', queries.getQuery);
router.post('/', queries.upsertQuery);
router.put('/:id', queries.upsertQuery);
router.delete('/:id', queries.deleteQuery);
router.post('/:query_id/label/:label_id', querylabel.addLabelToQuery);
router.delete('/:query_id/label/:label_id', querylabel.removeLabelFromQuery);
// POST and PUT use the same controller, albeit with a different 'action'
// parameter, so we pass that here
router.post('/:query_id/label/:label_id', (req, res, next) =>
querylabel.addRemove(req, res, next, 'addLabelToQuery'));
router.delete('/:query_id/label/:label_id', (req, res, next) =>
querylabel.addRemove(req, res, next, 'removeLabelFromQuery'));
router.post('/:query_id/user/:user_id', queryuser.addUserToQuery);
router.put('/:query_id/user/:user_id', queryuser.updateMostRecentSeen);

Expand Down
14 changes: 8 additions & 6 deletions api_spec/ems.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -392,12 +392,14 @@ paths:
schema:
type: integer
responses:
'201':
description: The created querylabel object
'200':
description: The updated query object
content:
application/json:
schema:
$ref: 'schemas/querylabel.yaml'
$ref: 'schemas/queryresponse.yaml'
'404':
description: Query / Label combination not found
'500':
description: Server error
delete:
Expand All @@ -417,12 +419,12 @@ paths:
schema:
type: integer
responses:
'204':
description: Successful deletion
'200':
description: The updated query object
content:
application/json:
schema:
type: object
$ref: 'schemas/queryresponse.yaml'
'404':
description: Query / Label combination not found
'500':
Expand Down
57 changes: 4 additions & 53 deletions controllers/queries.js
Original file line number Diff line number Diff line change
@@ -1,55 +1,6 @@
const db = require('../../ems-db');

const addEmbeds = async (queries) => {
// We need the IDs of all queries we received
const query_ids = queries.rows.map(query => query.id);
// Don't proceed if we don't need to
if (query_ids.length === 0) {
return [];
}
// Now get the initiators for all the queries we've
// received, we also receive their associated query ID
const initiators = await db.resolvers.queries.initiators(
query_ids
);
// Now get the participants of all retrieved queries
const participants = await db.resolvers.queries.participants(
query_ids
);
// Now get the labels for all retrieved queries
const labels = await db.resolvers.queries.labels(
query_ids
);
// Finally get the most recent message for each retrieved query
const latest = await db.resolvers.queries.latestMessages(
query_ids
);
// Now we have everything, we can bundle it all up together
return queries.rows.map(query => {
// The initiator for this query
const queryInitiator = initiators.rows.find(
init => init.id === query.id
).initiator;
// The participants of this query
const queryParticipants = participants.rows.filter(
participant => participant.query_id === query.id
).map(final => final.creator_id);
const queryLabels = labels.rows.filter(
label => label.query_id === query.id
).map(final => final.label_id);
// The most recent message for this query
const queryLatest = latest.rows.find(
latestMessage => latestMessage.query_id === query.id
);
return {
...query,
initiator: queryInitiator,
participants: queryParticipants,
latestMessage: queryLatest,
labels: queryLabels
};
});
};
const helpers = require('../helpers/queries');

const queries = {
getQueries: async (req, res, next) => {
Expand All @@ -59,7 +10,7 @@ const queries = {
try {
// First get the queries we're dealing with
const queries = await db.resolvers.queries.allQueries(req);
const toSend = await addEmbeds(queries);
const toSend = await helpers.addEmbeds(queries);
res.json(toSend);
next();
} catch (err) {
Expand All @@ -74,7 +25,7 @@ const queries = {
res.send();
next();
} else if (query.rowCount === 1) {
const toSend = await addEmbeds(query);
const toSend = await helpers.addEmbeds(query);
res.json(toSend[0]);
next();
} else {
Expand All @@ -94,7 +45,7 @@ const queries = {
res.send();
next();
} else {
const toSend = await addEmbeds(result);
const toSend = await helpers.addEmbeds(result);
res.status(req.method === 'POST' ? 201 : 200);
res.json(toSend[0]);
next();
Expand Down
61 changes: 27 additions & 34 deletions controllers/querylabel.js
Original file line number Diff line number Diff line change
@@ -1,40 +1,33 @@
const db = require('../../ems-db');

const helpers = require('../helpers/queries');

const querylabel = {
addLabelToQuery: (req, res, next) =>
db.resolvers.querylabel
.addLabelToQuery(req)
.then((result) => {
if (result.rowCount === 1) {
res.status(201);
res.json(result.rows[0]);
next();
} else {
res.status(500);
res.send();
next();
}
})
.catch((err) => next(err)),
removeLabelFromQuery: (req, res, next) =>
db.resolvers.querylabel
.removeLabelFromQuery(req)
.then((result) => {
if (result.rowCount === 0) {
res.status(404);
res.send();
next();
} else if (result.rowCount === 1) {
res.status(204);
res.json({});
next();
} else {
res.status(500);
res.send();
next();
}
})
.catch((err) => next(err))
addRemove: (req, res, next, action) => {
const func = db.resolvers.querylabel[action];
return func(req).then(async (response) => {
if (response.rowCount === 0) {
res.status(404);
res.send();
next();
} else if (response.rowCount === 1) {
// Fetch and return the updated query object
const queries = await db.resolvers.queries.getQuery({
params: { id: req.params.query_id }
});
const toSend = await helpers.addEmbeds(queries);
res.status(200);
res.json(toSend[0]);
next();
} else {
res.status(500);
res.send();
next();
}
})
.catch((err) => next(err));

}
};

module.exports = querylabel;
56 changes: 56 additions & 0 deletions helpers/queries.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
const db = require('../../ems-db');

const queries = {
addEmbeds: async (queries) => {
// We need the IDs of all queries we received
const query_ids = queries.rows.map(query => query.id);
// Don't proceed if we don't need to
if (query_ids.length === 0) {
return [];
}
// Now get the initiators for all the queries we've
// received, we also receive their associated query ID
const initiators = await db.resolvers.queries.initiators(
query_ids
);
// Now get the participants of all retrieved queries
const participants = await db.resolvers.queries.participants(
query_ids
);
// Now get the labels for all retrieved queries
const labels = await db.resolvers.queries.labels(
query_ids
);
// Finally get the most recent message for each retrieved query
const latest = await db.resolvers.queries.latestMessages(
query_ids
);
// Now we have everything, we can bundle it all up together
return queries.rows.map(query => {
// The initiator for this query
const queryInitiator = initiators.rows.find(
init => init.id === query.id
).initiator;
// The participants of this query
const queryParticipants = participants.rows.filter(
participant => participant.query_id === query.id
).map(final => final.creator_id);
const queryLabels = labels.rows.filter(
label => label.query_id === query.id
).map(final => final.label_id);
// The most recent message for this query
const queryLatest = latest.rows.find(
latestMessage => latestMessage.query_id === query.id
);
return {
...query,
initiator: queryInitiator,
participants: queryParticipants,
latestMessage: queryLatest,
labels: queryLabels
};
});
}
}

module.exports = queries;
57 changes: 41 additions & 16 deletions test/controllers/querylabel.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,31 @@ const db = require('../../../ems-db');

const mockResult = { one: 'one' };

// The querylabel controller calls our addEmbeds helper, so we need
// to mock it
jest.mock('../../helpers/queries', () => ({
addEmbeds: jest.fn(() => Promise.resolve([{id: 1}]))
}));

// Mock ems-db
jest.mock('../../../ems-db', () => ({
resolvers: {
queries: {
getQuery: jest.fn((passed) => {
// A mock DB resolver that returns a promise that resolves
// to whatever it was passed
if (passed) {
return new Promise((resolve) => {
return resolve(passed);
});
} else {
return new Promise((resolve, reject) => {
return reject(new Error('Rejected'));
});
}

})
},
querylabel: {
// A mock DB resolver that returns a promise that resolves
// to whatever it was passed
Expand Down Expand Up @@ -41,7 +63,7 @@ jest.mock('../../../ems-db', () => ({
}));

describe('QueryLabels', () => {
describe('addLabelToQuery', () => {
describe('addRemove (addLabelToQuery)', () => {
// res.json is used, so we should mock that
const res = { json: jest.fn(), send: jest.fn(), status: jest.fn() };
// Mock next so we can check it has been called
Expand All @@ -50,7 +72,7 @@ describe('QueryLabels', () => {
// Make the === 0 call
// Here we're telling our mocked addLabelToQuery DB resolver above to
// pretend it's not inserted/updated a label
querylabel.addLabelToQuery({ rowCount: 0 }, res, next);
querylabel.addRemove({ rowCount: 0 }, res, next, 'addLabelToQuery');

it('should call the DB resolver', (done) => {
expect(db.resolvers.querylabel.addLabelToQuery).toHaveBeenCalled();
Expand All @@ -73,14 +95,15 @@ describe('QueryLabels', () => {
// Here we're telling our mocked addLabelToQuery DB resolver above to
// pretend it's successfully inserted label query relationship
// POST:
querylabel.addLabelToQuery(
{ rowCount: 1, rows: [mockResult] },
querylabel.addRemove(
{ rowCount: 1, params: { query_id: 1 }, rows: [mockResult] },
res,
next
next,
'addLabelToQuery'
);

it('rowCount == 1 should call status(), passing 201', (done) => {
expect(res.status).toBeCalledWith(201);
it('rowCount == 1 should call status(), passing 200', (done) => {
expect(res.status).toBeCalledWith(200);
done();
});
it('rowCount == 1 should call json()', (done) => {
Expand All @@ -96,7 +119,7 @@ describe('QueryLabels', () => {
// Here we're telling our mocked addLabelToQuery DB resolver above to
// pretend it's updated > 1 rows which shouldn't ever happen
// POST:
querylabel.addLabelToQuery({ rowCount: 2 }, res, next);
querylabel.addRemove({ rowCount: 2 }, res, next, 'addLabelToQuery');

it('rowCount > 1 should call status(), passing 500', (done) => {
expect(res.status).toBeCalledWith(500);
Expand All @@ -112,14 +135,14 @@ describe('QueryLabels', () => {
});

// Make the failed call
querylabel.addLabelToQuery(false, res, next);
querylabel.addRemove(false, res, next, 'addLabelToQuery');
it('should call next() from the catch passing the error', (done) => {
expect(next).toHaveBeenCalledWith(new Error('Rejected'));
done();
});
});

describe('removeLabelFromQuery', () => {
describe('addRemove (removeLabelFromQuery)', () => {
// res.json is used, so we should mock that
const res = { json: jest.fn(), send: jest.fn(), status: jest.fn() };
// Mock next so we can check it has been called
Expand All @@ -128,7 +151,7 @@ describe('QueryLabels', () => {
// Make the === 0 call
// Here we're telling our mocked removeLabelFromQuery DB resolver above to
// pretend it's not inserted/updated a label query relationship
querylabel.removeLabelFromQuery({ rowCount: 0 }, res, next);
querylabel.addRemove({ rowCount: 0 }, res, next, 'removeLabelFromQuery');

it('should call the DB resolver', (done) => {
expect(
Expand All @@ -153,10 +176,12 @@ describe('QueryLabels', () => {
// Here we're telling our mocked removeLabelFromQuery DB resolver above to
// pretend it's successfully inserted 1 label query relationship
// POST:
querylabel.removeLabelFromQuery({ rowCount: 1 }, res, next);
querylabel.addRemove({
rowCount: 1, params: { query_id: 1 }
}, res, next, 'removeLabelFromQuery');

it('rowCount == 1 should call status(), passing 204', (done) => {
expect(res.status).toBeCalledWith(204);
it('rowCount == 1 should call status(), passing 200', (done) => {
expect(res.status).toBeCalledWith(200);
done();
});
it('rowCount == 1 should call json()', (done) => {
Expand All @@ -172,7 +197,7 @@ describe('QueryLabels', () => {
// Here we're telling our mocked removeLabelFromQuery DB resolver above to
// pretend it's deleted > 1 rows which shouldn't ever happen
// POST:
querylabel.removeLabelFromQuery({ rowCount: 2 }, res, next);
querylabel.addRemove({ rowCount: 2 }, res, next, 'removeLabelFromQuery');

it('rowCount > 1 should call status(), passing 500', (done) => {
expect(res.status).toBeCalledWith(500);
Expand All @@ -188,7 +213,7 @@ describe('QueryLabels', () => {
});

// Make the failed call
querylabel.removeLabelFromQuery(false, res, next);
querylabel.addRemove(false, res, next, 'removeLabelFromQuery');
it('should call next() from the catch passing the error', (done) => {
expect(next).toHaveBeenCalledWith(new Error('Rejected'));
done();
Expand Down

0 comments on commit c3f930a

Please sign in to comment.