From 651d42dbea07abce127f075b66d3e1bcba1f7afc Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 19 May 2020 16:10:53 +0200 Subject: [PATCH] fix PR comments, move pagination logic to the database layer, handle searches Signed-off-by: Convly --- packages/strapi-admin/controllers/user.js | 9 +- .../services/__tests__/user.test.js | 61 +++++--- packages/strapi-admin/services/user.js | 17 ++- .../strapi-connector-bookshelf/lib/queries.js | 4 - .../strapi-connector-mongoose/lib/queries.js | 5 +- .../queries/__tests__/create-query.test.js | 2 +- .../__tests__/paginated-queries.test.js | 132 ++++++++++++++++++ .../lib/queries/create-query.js | 33 ++--- .../strapi-database/lib/queries/helpers.js | 35 +++++ .../lib/queries/paginated-queries.js | 55 ++++++++ .../__tests__/findPageQueryFactory.test.js | 47 ------- .../strapi-utils/lib/findPageQueryFactory.js | 47 ------- packages/strapi-utils/lib/index.js | 2 - 13 files changed, 291 insertions(+), 158 deletions(-) create mode 100644 packages/strapi-database/lib/queries/__tests__/paginated-queries.test.js create mode 100644 packages/strapi-database/lib/queries/helpers.js create mode 100644 packages/strapi-database/lib/queries/paginated-queries.js delete mode 100644 packages/strapi-utils/lib/__tests__/findPageQueryFactory.test.js delete mode 100644 packages/strapi-utils/lib/findPageQueryFactory.js diff --git a/packages/strapi-admin/controllers/user.js b/packages/strapi-admin/controllers/user.js index b4622d82e7..9c03a25a2f 100644 --- a/packages/strapi-admin/controllers/user.js +++ b/packages/strapi-admin/controllers/user.js @@ -32,10 +32,15 @@ module.exports = { }, async find(ctx) { - const users = await strapi.query('user', 'admin').findPage(ctx.query); + const method = _.has(ctx.query, '_q') ? 'searchPage' : 'findPage'; + + const { results, pagination } = await strapi.admin.services.user[method](ctx.query); return { - data: users, + data: { + results: results.map(strapi.admin.services.user.sanitizeUser), + pagination, + }, }; }, }; diff --git a/packages/strapi-admin/services/__tests__/user.test.js b/packages/strapi-admin/services/__tests__/user.test.js index 50aee44420..b9d7470f0a 100644 --- a/packages/strapi-admin/services/__tests__/user.test.js +++ b/packages/strapi-admin/services/__tests__/user.test.js @@ -129,48 +129,63 @@ describe('User', () => { }); }); - describe('Find users (paginated)', () => { - const defaults = {page: 1, pageSize: 100}; + describe('Fetch users (paginated)', () => { + const defaults = { page: 1, pageSize: 100 }; beforeEach(() => { - const findPage = jest.fn(({page = defaults.page, pageSize = defaults.pageSize} = {}) => { + const fetchPage = jest.fn(({ page = defaults.page, pageSize = defaults.pageSize } = {}) => { return { - results: Array.from({length: pageSize}).map((_, i) => i + (page - 1) * pageSize), - pagination: {page, pageSize, total: page * pageSize, pageCount: page}, + results: Array.from({ length: pageSize }).map((_, i) => i + (page - 1) * pageSize), + pagination: { page, pageSize, total: page * pageSize, pageCount: page }, }; }); global.strapi = { query() { - return {findPage}; + return { findPage: fetchPage, searchPage: fetchPage }; }, }; }); - test('Find users with custom pagination', async () => { - const pagination = {page: 2, pageSize: 15}; - const page = await userService.findPage(pagination); + test('Fetch users with custom pagination', async () => { + const pagination = { page: 2, pageSize: 15 }; + const foundPage = await userService.findPage(pagination); + const searchedPage = await userService.searchPage(pagination); - expect(page.results.length).toBe(15); - expect(page.results[0]).toBe(15); - expect((page.pagination.total = 30)); + expect(foundPage.results.length).toBe(15); + expect(foundPage.results[0]).toBe(15); + expect((foundPage.pagination.total = 30)); + + expect(searchedPage.results.length).toBe(15); + expect(searchedPage.results[0]).toBe(15); + expect((searchedPage.pagination.total = 30)); }); - test('Find users with default pagination', async () => { - const page = await userService.findPage(); + test('Fetch users with default pagination', async () => { + const foundPage = await userService.findPage(); + const searchedPage = await userService.searchPage(); - expect(page.results.length).toBe(100); - expect(page.results[0]).toBe(0); - expect((page.pagination.total = 100)); + expect(foundPage.results.length).toBe(100); + expect(foundPage.results[0]).toBe(0); + expect((foundPage.pagination.total = 100)); + + expect(searchedPage.results.length).toBe(100); + expect(searchedPage.results[0]).toBe(0); + expect((searchedPage.pagination.total = 100)); }); - test('Find users with partial pagination', async () => { - const pagination = {page: 2}; - const page = await userService.findPage(pagination); + test('Fetch users with partial pagination', async () => { + const pagination = { page: 2 }; + const foundPage = await userService.findPage(pagination); + const searchedPage = await userService.searchPage(pagination); - expect(page.results.length).toBe(100); - expect(page.results[0]).toBe(100); - expect((page.pagination.total = 200)); + expect(foundPage.results.length).toBe(100); + expect(foundPage.results[0]).toBe(100); + expect((foundPage.pagination.total = 200)); + + expect(searchedPage.results.length).toBe(100); + expect(searchedPage.results[0]).toBe(100); + expect((searchedPage.pagination.total = 200)); }); }); diff --git a/packages/strapi-admin/services/user.js b/packages/strapi-admin/services/user.js index b6eeaddd92..d3c9d0352c 100644 --- a/packages/strapi-admin/services/user.js +++ b/packages/strapi-admin/services/user.js @@ -62,11 +62,11 @@ const findRegistrationInfo = async registrationToken => { /** * Registers a user based on a registrationToken and some informations to update * @param {Object} params - * @param {Object} params.registrationInfo registration token + * @param {Object} params.registrationToken registration token * @param {Object} params.userInfo user info */ const register = async ({ registrationToken, userInfo }) => { - const matchingUser = await strapi.query('user', 'admin').findOne({registrationToken}); + const matchingUser = await strapi.query('user', 'admin').findOne({ registrationToken }); if (!matchingUser) { throw strapi.errors.badRequest('Invalid registration info'); @@ -75,7 +75,7 @@ const register = async ({ registrationToken, userInfo }) => { const hashedPassword = await strapi.admin.services.auth.hashPassword(userInfo.password); return strapi.admin.services.user.update( - {id: matchingUser.id}, + { id: matchingUser.id }, { password: hashedPassword, firstname: userInfo.firstname, @@ -84,7 +84,7 @@ const register = async ({ registrationToken, userInfo }) => { isActive: true, } ); -} +}; /** Find many users (paginated) * @param query @@ -94,6 +94,14 @@ const findPage = async query => { return strapi.query('user', 'admin').findPage(query); }; +/** Search for many users (paginated) + * @param query + * @returns {Promise} + */ +const searchPage = async query => { + return strapi.query('user', 'admin').searchPage(query); +}; + module.exports = { create, update, @@ -102,4 +110,5 @@ module.exports = { register, sanitizeUser, findPage, + searchPage, }; diff --git a/packages/strapi-connector-bookshelf/lib/queries.js b/packages/strapi-connector-bookshelf/lib/queries.js index d42b0f5bbb..42ec368d2c 100644 --- a/packages/strapi-connector-bookshelf/lib/queries.js +++ b/packages/strapi-connector-bookshelf/lib/queries.js @@ -8,7 +8,6 @@ const { convertRestQueryParams, buildQuery, escapeQuery, - findPageQueryFactory, } = require('strapi-utils'); module.exports = function createQueryBuilder({ model, strapi }) { @@ -70,8 +69,6 @@ module.exports = function createQueryBuilder({ model, strapi }) { .then(results => results.toJSON()); } - const findPage = findPageQueryFactory(find, count); - /** * Count entries based on filters */ @@ -623,7 +620,6 @@ module.exports = function createQueryBuilder({ model, strapi }) { return { findOne, find, - findPage, create, update, delete: deleteMany, diff --git a/packages/strapi-connector-mongoose/lib/queries.js b/packages/strapi-connector-mongoose/lib/queries.js index 6933e41be9..fe27b00aae 100644 --- a/packages/strapi-connector-mongoose/lib/queries.js +++ b/packages/strapi-connector-mongoose/lib/queries.js @@ -4,7 +4,7 @@ */ const _ = require('lodash'); -const { convertRestQueryParams, buildQuery, findPageQueryFactory } = require('strapi-utils'); +const { convertRestQueryParams, buildQuery } = require('strapi-utils'); const { findComponentByGlobalId } = require('./utils/helpers'); @@ -395,8 +395,6 @@ module.exports = ({ model, strapi }) => { }).then(results => results.map(result => (result ? result.toObject() : null))); } - const findPage = findPageQueryFactory(find, count); - async function findOne(params, populate) { const entries = await find({ ...params, _limit: 1 }, populate); return entries[0] || null; @@ -507,7 +505,6 @@ module.exports = ({ model, strapi }) => { return { findOne, find, - findPage, create, update, delete: deleteMany, diff --git a/packages/strapi-database/lib/queries/__tests__/create-query.test.js b/packages/strapi-database/lib/queries/__tests__/create-query.test.js index 48b7b20972..033189021b 100644 --- a/packages/strapi-database/lib/queries/__tests__/create-query.test.js +++ b/packages/strapi-database/lib/queries/__tests__/create-query.test.js @@ -4,7 +4,7 @@ const _ = require('lodash'); const createQuery = require('../create-query'); describe('Database queries', () => { - describe('Subsitute id with primaryKey in paramters', () => { + describe('Substitute id with primaryKey in parameters', () => { test.each(['create', 'update', 'delete', 'find', 'findOne', 'search', 'count', 'countSearch'])( 'Calling "%s" replaces id by the primaryKey in the params of the model before calling the underlying connector', async method => { diff --git a/packages/strapi-database/lib/queries/__tests__/paginated-queries.test.js b/packages/strapi-database/lib/queries/__tests__/paginated-queries.test.js new file mode 100644 index 0000000000..5b7d5d28ec --- /dev/null +++ b/packages/strapi-database/lib/queries/__tests__/paginated-queries.test.js @@ -0,0 +1,132 @@ +'use strict'; + +const { + createFindPageQuery, + createSearchPageQuery, + createPaginatedQuery, + getPaginationInfos, +} = require('../paginated-queries'); + +describe('Paginated Queries', () => { + describe('createPaginatedQuery', () => { + test('Successfully create a paginated query based on given fetch and count', async () => { + const fetch = jest.fn(() => [1, 2]); + const count = jest.fn(() => 2); + + const paginatedQuery = createPaginatedQuery({ fetch, count }); + + const data = await paginatedQuery({}); + + expect(fetch).toHaveBeenCalled(); + expect(count).toHaveBeenCalled(); + expect(data).toMatchObject({ + results: [1, 2], + pagination: { + page: 1, + pageSize: 100, + total: 2, + pageCount: 1, + }, + }); + }); + + test('Use custom pagination options to find a specific page', async () => { + const fetch = jest.fn(() => [5, 6]); + const count = jest.fn(() => 6); + + const paginatedQuery = createPaginatedQuery({ fetch, count }); + + const data = await paginatedQuery({ page: 2, pageSize: 4 }); + + expect(fetch).toHaveBeenCalled(); + expect(count).toHaveBeenCalled(); + expect(data).toMatchObject({ + results: [5, 6], + pagination: { + page: 2, + pageSize: 4, + total: 6, + pageCount: 2, + }, + }); + }); + }); + + describe('createFindPageQuery', () => { + test('Successfully create a findPage query based on given find and count', async () => { + const find = jest.fn(() => [1, 2]); + const count = jest.fn(() => 2); + + const paginatedQuery = createFindPageQuery({ find, count }); + + const data = await paginatedQuery({}); + + expect(find).toHaveBeenCalled(); + expect(count).toHaveBeenCalled(); + expect(data).toMatchObject({ + results: [1, 2], + pagination: { + page: 1, + pageSize: 100, + total: 2, + pageCount: 1, + }, + }); + }); + }); + + describe('createSearchPageQuery', () => { + test('Successfully create a searchPage query based on given search and countSearch', async () => { + const search = jest.fn(() => [1, 2]); + const countSearch = jest.fn(() => 2); + + const paginatedQuery = createSearchPageQuery({ search, countSearch }); + + const data = await paginatedQuery({}); + + expect(search).toHaveBeenCalled(); + expect(countSearch).toHaveBeenCalled(); + expect(data).toMatchObject({ + results: [1, 2], + pagination: { + page: 1, + pageSize: 100, + total: 2, + pageCount: 1, + }, + }); + }); + }); + + describe('getPaginationInfos', () => { + test('Incomplete last page', async () => { + const queryParams = { page: 2, pageSize: 6 }; + const count = jest.fn(() => 8); + + const pagination = await getPaginationInfos(queryParams, count); + + expect(count).toHaveBeenCalled(); + expect(pagination).toEqual({ + page: 2, + pageSize: 6, + total: 8, + pageCount: 2, + }); + }); + + test('Complete last page', async () => { + const queryParams = { page: 2, pageSize: 6 }; + const count = jest.fn(() => 18); + + const pagination = await getPaginationInfos(queryParams, count); + + expect(count).toHaveBeenCalled(); + expect(pagination).toEqual({ + page: 2, + pageSize: 6, + total: 18, + pageCount: 3, + }); + }); + }); +}); diff --git a/packages/strapi-database/lib/queries/create-query.js b/packages/strapi-database/lib/queries/create-query.js index 6ff6540c8e..5f040c54ab 100644 --- a/packages/strapi-database/lib/queries/create-query.js +++ b/packages/strapi-database/lib/queries/create-query.js @@ -1,7 +1,7 @@ 'use strict'; -const { replaceIdByPrimaryKey } = require('../utils/primary-key'); -const { executeBeforeLifecycle, executeAfterLifecycle } = require('../utils/lifecycles'); +const { createQueryWithLifecycles, withLifecycles } = require('./helpers'); +const { createFindPageQuery, createSearchPageQuery } = require('./paginated-queries'); /** * @param {Object} opts options @@ -52,30 +52,15 @@ module.exports = function createQuery(opts) { delete: createQueryWithLifecycles({ query: 'delete', model, connectorQuery }), find: createQueryWithLifecycles({ query: 'find', model, connectorQuery }), findOne: createQueryWithLifecycles({ query: 'findOne', model, connectorQuery }), - findPage: createQueryWithLifecycles({ query: 'findPage', model, connectorQuery }), count: createQueryWithLifecycles({ query: 'count', model, connectorQuery }), search: createQueryWithLifecycles({ query: 'search', model, connectorQuery }), countSearch: createQueryWithLifecycles({ query: 'countSearch', model, connectorQuery }), + + findPage: withLifecycles({ query: 'findPage', model, fn: createFindPageQuery(connectorQuery) }), + searchPage: withLifecycles({ + query: 'searchPage', + model, + fn: createSearchPageQuery(connectorQuery), + }), }; }; - -// wraps a connectorQuery call with: -// - param substitution -// - lifecycle hooks -const createQueryWithLifecycles = ({ query, model, connectorQuery }) => async (params, ...rest) => { - // substitute id for primaryKey value in params - const newParams = replaceIdByPrimaryKey(params, model); - const queryArguments = [newParams, ...rest]; - - // execute before hook - await executeBeforeLifecycle(query, model, ...queryArguments); - - // execute query - const result = await connectorQuery[query](...queryArguments); - - // execute after hook with result and arguments - await executeAfterLifecycle(query, model, result, ...queryArguments); - - // return result - return result; -}; diff --git a/packages/strapi-database/lib/queries/helpers.js b/packages/strapi-database/lib/queries/helpers.js new file mode 100644 index 0000000000..e9786e56e3 --- /dev/null +++ b/packages/strapi-database/lib/queries/helpers.js @@ -0,0 +1,35 @@ +'use strict'; + +const { replaceIdByPrimaryKey } = require('../utils/primary-key'); +const { executeBeforeLifecycle, executeAfterLifecycle } = require('../utils/lifecycles'); + +const withLifecycles = ({ query, model, fn }) => async (params, ...rest) => { + // substitute id for primaryKey value in params + const newParams = replaceIdByPrimaryKey(params, model); + const queryArguments = [newParams, ...rest]; + + // execute before hook + await executeBeforeLifecycle(query, model, ...queryArguments); + + // execute query + const result = await fn(...queryArguments); + + // execute after hook with result and arguments + await executeAfterLifecycle(query, model, result, ...queryArguments); + + // return result + return result; +}; + +// wraps a connectorQuery call with: +// - param substitution +// - lifecycle hooks +const createQueryWithLifecycles = ({ query, model, connectorQuery }) => { + return withLifecycles({ + query, + model, + fn: (...queryParameters) => connectorQuery[query](...queryParameters), + }); +}; + +module.exports = { withLifecycles, createQueryWithLifecycles }; diff --git a/packages/strapi-database/lib/queries/paginated-queries.js b/packages/strapi-database/lib/queries/paginated-queries.js new file mode 100644 index 0000000000..eff74597e9 --- /dev/null +++ b/packages/strapi-database/lib/queries/paginated-queries.js @@ -0,0 +1,55 @@ +'use strict'; + +const _ = require('lodash'); + +const createPaginatedQuery = ({ fetch, count }) => async (queryParams, ...args) => { + const params = _.omit(queryParams, ['page', 'pageSize']); + const pagination = await getPaginationInfos(queryParams, count); + + Object.assign(params, paginationToQueryParams(pagination)); + + const results = await fetch(params, ...args); + + return { results, pagination }; +}; + +const createSearchPageQuery = ({ search, countSearch }) => + createPaginatedQuery({ fetch: search, count: countSearch }); + +const createFindPageQuery = ({ find, count }) => createPaginatedQuery({ fetch: find, count }); + +const getPaginationInfos = async (queryParams, count) => { + const { page, pageSize, ...params } = withDefaultPagination(queryParams); + + const total = await count(params); + + return { + page, + pageSize, + pageCount: Math.ceil(total / pageSize), + total, + }; +}; + +const withDefaultPagination = params => { + const { page = 1, pageSize = 100, ...rest } = params; + + return { + page: parseInt(page), + pageSize: parseInt(pageSize), + ...rest, + }; +}; + +const paginationToQueryParams = ({ page, pageSize }) => ({ + _start: Math.max(page - 1, 0) * pageSize, + _limit: pageSize, +}); + +module.exports = { + getPaginationInfos, + withDefaultPagination, + createPaginatedQuery, + createFindPageQuery, + createSearchPageQuery, +}; diff --git a/packages/strapi-utils/lib/__tests__/findPageQueryFactory.test.js b/packages/strapi-utils/lib/__tests__/findPageQueryFactory.test.js deleted file mode 100644 index 87ee814079..0000000000 --- a/packages/strapi-utils/lib/__tests__/findPageQueryFactory.test.js +++ /dev/null @@ -1,47 +0,0 @@ -'use strict'; - -const findPageQueryFactory = require('../findPageQueryFactory'); - -describe('findPageQueryFactory', () => { - test('Successfully create a query based on given find and count', async () => { - const find = jest.fn(() => [1, 2]); - const count = jest.fn(() => 2); - - const findPage = findPageQueryFactory(find, count); - - const data = await findPage({}); - - expect(find).toHaveBeenCalled(); - expect(count).toHaveBeenCalled(); - expect(data).toMatchObject({ - results: [1, 2], - pagination: { - page: 1, - pageSize: 100, - total: 2, - pageCount: 1, - }, - }); - }); - - test('Use custom pagination options to find a specific page', async () => { - const find = jest.fn(() => [5, 6]); - const count = jest.fn(() => 6); - - const findPage = findPageQueryFactory(find, count); - - const data = await findPage({ page: 2, pageSize: 4 }); - - expect(find).toHaveBeenCalled(); - expect(count).toHaveBeenCalled(); - expect(data).toMatchObject({ - results: [5, 6], - pagination: { - page: 2, - pageSize: 4, - total: 6, - pageCount: 2, - }, - }); - }); -}); diff --git a/packages/strapi-utils/lib/findPageQueryFactory.js b/packages/strapi-utils/lib/findPageQueryFactory.js deleted file mode 100644 index 9cdddf81a3..0000000000 --- a/packages/strapi-utils/lib/findPageQueryFactory.js +++ /dev/null @@ -1,47 +0,0 @@ -'use strict'; - -const DEFAULT_PAGINATION_OPTS = { page: 1, pageSize: 100 }; - -/** - * Create a findPage query based on the find and count queries provided to the factory - * @param find The find query - * @param count The count query - * @returns {function(*, ...[*]): {pagination: {pageCount: number, total: number, pageSize: number, page: number}, results: [*]}} - */ -const findPageQueryFactory = (find, count) => async (queryParams, ...args) => { - const { page, pageSize, ...params } = withDefaultPagination(queryParams); - - const total = await count(params); - - const pagination = { - page, - pageSize, - pageCount: Math.ceil(total / pageSize), - total, - }; - - Object.assign(params, { - _start: Math.max(pagination.page - 1, 0) * pagination.pageSize, - _limit: pagination.pageSize, - }); - - const results = await find(params, ...args); - - return { results, pagination }; -}; - -function withDefaultPagination(params) { - const { - page = DEFAULT_PAGINATION_OPTS.page, - pageSize = DEFAULT_PAGINATION_OPTS.pageSize, - ...rest - } = params; - - return { - page: parseInt(page), - pageSize: parseInt(pageSize), - ...rest, - }; -} - -module.exports = findPageQueryFactory; diff --git a/packages/strapi-utils/lib/index.js b/packages/strapi-utils/lib/index.js index 7197eaddcc..3c4fb14b77 100644 --- a/packages/strapi-utils/lib/index.js +++ b/packages/strapi-utils/lib/index.js @@ -23,7 +23,6 @@ const { } = require('./stringFormatting'); const { removeUndefined } = require('./objectFormatting'); const { getConfigUrls, getAbsoluteAdminUrl, getAbsoluteServerUrl } = require('./config'); -const findPageQueryFactory = require('./findPageQueryFactory'); module.exports = { yup, @@ -46,5 +45,4 @@ module.exports = { removeUndefined, getAbsoluteAdminUrl, getAbsoluteServerUrl, - findPageQueryFactory, };