Merge pull request #387 from stockpile-co/177/fix-use-of-knex-modify

177/Fix use of knex.modify in filterQuery

controllers/item.js

@@ -40,7 +40,7 @@ item.withFieldsAndFilters = (req, queryBuilder) => {
     .select('itemStatus.available as available')
 
   // Add filters to query
-    .modify(filterQuery(req, filterParams))
+    .modify(filterQuery, req, filterParams)
 
   // Add pagination
     .modify(paginate.paginateQuery, req, 'item')

services/filter-query.js

@@ -1,21 +1,20 @@
 /**
  * Filter a database query based on request query parameters
  * @module services/filter-query
+ * @param {object} queryBuilder Knex query builder
  * @param {object} req HTTP request
  * @param {map} paramNames Values `req.params` to filter with
  * @return {object} Query builder with `where` clauses appended to it
  */
-module.exports = (req, paramNames = new Map()) => {
-  return (queryBuilder) => {
-    for (const [name, key] of paramNames) {
-      // Get value from request query parameters
-      const value = req.params[name]
+module.exports = (queryBuilder, req, paramNames = new Map()) => {
+  for (const [name, key] of paramNames) {
+    // Get value from request query parameters
+    const value = req.params[name]
 
-      // Add to query if value is defined
-      if (value) {
-        queryBuilder.where(key, value)
-      }
+    // Add to query if value is defined
+    if (value) {
+      queryBuilder.where(key, value)
     }
-    return queryBuilder
   }
+  return queryBuilder
 }

test/filter-query.js

@@ -5,23 +5,34 @@ const fixt = require('./fixtures/filter-query')
 const filterQuery = require('../services/filter-query')
 
 test('Filter query', t => {
-  t.true(typeof filterQuery(null) === 'function', 'returns a function')
-  const queryBuilderNoEffect = {
+  const queryBuilder = {
+    where: sinon.spy()
+  }
+  const paramNames = new Map()
+  paramNames.set(fixt.names[0], fixt.names[0])
+  paramNames.set(fixt.names[1], fixt.names[1])
+  const result = filterQuery(queryBuilder, fixt.req, paramNames)
+  t.true(queryBuilder.where.calledTwice, 'each param is added to query')
+  t.is(result, queryBuilder, 'returns query builder')
+})
+
+test('Filter query when no params match', t => {
+  const queryBuilder = {
     where: sinon.spy()
   }
   const wrongParamNames = new Map()
   wrongParamNames.set(fixt.wrongNames[0], fixt.wrongNames[0])
   wrongParamNames.set(fixt.wrongNames[1], fixt.wrongNames[1])
-  filterQuery(fixt.req, wrongParamNames)(queryBuilderNoEffect)
-  t.false(queryBuilderNoEffect.where.called,
-    'has no effect when no params match')
+  const result = filterQuery(queryBuilder, fixt.req, wrongParamNames)
+  t.false(queryBuilder.where.called, 'has no effect when no params match')
+  t.is(result, queryBuilder, 'returns query builder')
+})
+
+test('Filter query when no params provided', t => {
   const queryBuilder = {
     where: sinon.spy()
   }
-  const paramNames = new Map()
-  paramNames.set(fixt.names[0], fixt.names[0])
-  paramNames.set(fixt.names[1], fixt.names[1])
-  filterQuery(fixt.req, paramNames)(queryBuilder)
-  t.true(queryBuilder.where.calledTwice,
-    'each param is added to query')
+  const result = filterQuery(queryBuilder, fixt.req)
+  t.false(queryBuilder.where.called, 'has no effect when no params provided')
+  t.is(result, queryBuilder, 'returns query builder')
 })