From 041f2f6eb3ef140101a82b96eba182bb4be2fd4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20No=C3=ABl?= Date: Tue, 12 Apr 2022 16:32:05 +0200 Subject: [PATCH] apply feedback --- .../server/tests/fields/date.test.e2e.js | 7 +++-- .../server/content-types/file/schema.js | 2 +- .../server/content-types/folder/schema.js | 2 +- .../upload/server/controllers/admin-file.js | 7 +++-- .../upload/server/controllers/admin-folder.js | 4 +-- .../upload/server/controllers/admin-upload.js | 8 +++--- .../upload/server/controllers/content-api.js | 8 +++--- .../find-entity-and-check-permissions.js | 4 +-- .../controllers/validation/admin/upload.js | 2 +- .../server/services/__tests__/file.test.js | 12 ++++---- .../server/services/__tests__/folder.test.js | 22 +++++++-------- .../server/services/__tests__/upload.test.js | 2 +- packages/core/upload/server/services/file.js | 6 ++-- .../core/upload/server/services/folder.js | 28 ++++--------------- .../core/upload/server/services/upload.js | 4 +-- .../tests/admin/file-folder.test.e2e.js | 24 ++++++++-------- .../upload/tests/admin/folder.test.e2e.js | 21 ++++++-------- 17 files changed, 74 insertions(+), 89 deletions(-) diff --git a/packages/core/content-manager/server/tests/fields/date.test.e2e.js b/packages/core/content-manager/server/tests/fields/date.test.e2e.js index 281d7951d3..c40b3a1397 100644 --- a/packages/core/content-manager/server/tests/fields/date.test.e2e.js +++ b/packages/core/content-manager/server/tests/fields/date.test.e2e.js @@ -92,17 +92,20 @@ describe('Test type date', () => { }); test('Updating entry sets the right value and format JSON', async () => { + const now = new Date(2018, 7, 5); + const res = await rq.post('/content-manager/collection-types/api::withdate.withdate', { body: { - field: '2018-06-05', + field: now, }, }); + const newDate = new Date(2017, 10, 23); const updateRes = await rq.put( `/content-manager/collection-types/api::withdate.withdate/${res.body.id}`, { body: { - field: '2017-11-23', + field: newDate, }, } ); diff --git a/packages/core/upload/server/content-types/file/schema.js b/packages/core/upload/server/content-types/file/schema.js index 1fe660cf79..f54a6258e3 100644 --- a/packages/core/upload/server/content-types/file/schema.js +++ b/packages/core/upload/server/content-types/file/schema.js @@ -95,7 +95,7 @@ module.exports = { target: 'plugin::upload.folder', inversedBy: 'files', }, - location: { + folderPath: { type: 'string', min: 1, required: true, diff --git a/packages/core/upload/server/content-types/folder/schema.js b/packages/core/upload/server/content-types/folder/schema.js index dfb6806268..4d2feff37a 100644 --- a/packages/core/upload/server/content-types/folder/schema.js +++ b/packages/core/upload/server/content-types/folder/schema.js @@ -45,7 +45,7 @@ module.exports = { target: 'plugin::upload.file', mappedBy: 'folder', }, - location: { + path: { type: 'string', min: 1, required: true, diff --git a/packages/core/upload/server/controllers/admin-file.js b/packages/core/upload/server/controllers/admin-file.js index 46f0530e4b..c4e208e5b6 100644 --- a/packages/core/upload/server/controllers/admin-file.js +++ b/packages/core/upload/server/controllers/admin-file.js @@ -58,8 +58,11 @@ module.exports = { id ); - await getService('upload').remove(file); + const [body] = await Promise.all([ + pm.sanitizeOutput(file, { action: ACTIONS.read }), + getService('upload').remove(file), + ]); - ctx.body = await pm.sanitizeOutput(file, { action: ACTIONS.read }); + ctx.body = body; }, }; diff --git a/packages/core/upload/server/controllers/admin-folder.js b/packages/core/upload/server/controllers/admin-folder.js index 3a8df09005..38d439bcbe 100644 --- a/packages/core/upload/server/controllers/admin-folder.js +++ b/packages/core/upload/server/controllers/admin-folder.js @@ -39,10 +39,10 @@ module.exports = { await validateCreateFolder(body); - const { setLocationAndUID } = getService('folder'); + const { setPathAndUID } = getService('folder'); // TODO: wrap with a transaction - const enrichFolder = pipeAsync(setLocationAndUID, setCreatorFields({ user })); + const enrichFolder = pipeAsync(setPathAndUID, setCreatorFields({ user })); const enrichedFolder = await enrichFolder(body); const folder = await strapi.entityService.create(folderModel, { diff --git a/packages/core/upload/server/controllers/admin-upload.js b/packages/core/upload/server/controllers/admin-upload.js index 145679245e..b964fa6b53 100644 --- a/packages/core/upload/server/controllers/admin-upload.js +++ b/packages/core/upload/server/controllers/admin-upload.js @@ -75,11 +75,11 @@ module.exports = { request: { files: { files } = {} }, } = ctx; - if (id && (_.isEmpty(files) || files.size === 0)) { - return this.updateFileInfo(ctx); - } - if (_.isEmpty(files) || files.size === 0) { + if (id) { + return this.updateFileInfo(ctx); + } + throw new ApplicationError('Files are empty'); } diff --git a/packages/core/upload/server/controllers/content-api.js b/packages/core/upload/server/controllers/content-api.js index 7977302d86..a4d68df1b0 100644 --- a/packages/core/upload/server/controllers/content-api.js +++ b/packages/core/upload/server/controllers/content-api.js @@ -9,9 +9,9 @@ const validateUploadBody = require('./validation/content-api/upload'); const { sanitize } = utils; const { ValidationError } = utils.errors; -const removeLocation = data => { - if (isArray(data)) return data.map(omit('location')); - if (isPlainObject(data)) return omit('location', data); +const removeFolderPath = data => { + if (isArray(data)) return data.map(omit('folderPath')); + if (isPlainObject(data)) return omit('folderPath', data); return data; }; @@ -19,7 +19,7 @@ const sanitizeOutput = (data, ctx) => { const schema = strapi.getModel('plugin::upload.file'); const { auth } = ctx.state; - return sanitize.contentAPI.output(removeLocation(data), schema, { auth }); + return sanitize.contentAPI.output(removeFolderPath(data), schema, { auth }); }; module.exports = { diff --git a/packages/core/upload/server/controllers/utils/find-entity-and-check-permissions.js b/packages/core/upload/server/controllers/utils/find-entity-and-check-permissions.js index d986473f29..d653d0d3c6 100644 --- a/packages/core/upload/server/controllers/utils/find-entity-and-check-permissions.js +++ b/packages/core/upload/server/controllers/utils/find-entity-and-check-permissions.js @@ -1,12 +1,10 @@ 'use strict'; const _ = require('lodash'); -const { contentTypes: contentTypesUtils } = require('@strapi/utils'); +const { CREATED_BY_ATTRIBUTE } = require('@strapi/utils').contentTypes.constants; const { NotFoundError, ForbiddenError } = require('@strapi/utils').errors; const { getService } = require('../../utils'); -const { CREATED_BY_ATTRIBUTE } = contentTypesUtils.constants; - const findEntityAndCheckPermissions = async (ability, action, model, id) => { const file = await getService('upload').findOne(id, [CREATED_BY_ATTRIBUTE, 'folder']); diff --git a/packages/core/upload/server/controllers/validation/admin/upload.js b/packages/core/upload/server/controllers/validation/admin/upload.js index cfa333f0d5..1ac793df64 100644 --- a/packages/core/upload/server/controllers/validation/admin/upload.js +++ b/packages/core/upload/server/controllers/validation/admin/upload.js @@ -11,7 +11,7 @@ const fileInfoSchema = yup.object({ folder: yup .strapiID() .nullable() - .test('folder-exists', "the folder doesn't exist", async folderId => { + .test('folder-exists', 'the folder does not exist', async folderId => { if (isNil(folderId)) { return true; } diff --git a/packages/core/upload/server/services/__tests__/file.test.js b/packages/core/upload/server/services/__tests__/file.test.js index 9a46b578c6..1f81a4030d 100644 --- a/packages/core/upload/server/services/__tests__/file.test.js +++ b/packages/core/upload/server/services/__tests__/file.test.js @@ -1,25 +1,25 @@ 'use strict'; -const { getLocation } = require('../file'); +const { getFolderPath } = require('../file'); -const folderLocation = '/9bc2352b-e29b-4ba3-810f-7b91033222de'; +const folderPath = '/9bc2352b-e29b-4ba3-810f-7b91033222de'; describe('file', () => { - describe('getLocation', () => { + describe('getFolderPath', () => { beforeAll(() => { global.strapi = { entityService: { - findOne: jest.fn(() => ({ location: folderLocation })), + findOne: jest.fn(() => ({ path: folderPath })), }, }; }); test.each([ - [[1, 'myFile.txt'], folderLocation], + [[1, 'myFile.txt'], folderPath], [[undefined, 'myFile.txt'], '/'], [[null, 'myFile.txt'], '/'], ])('inputs %s should give %s', async (args, expectedResult) => { - const result = await getLocation(...args); + const result = await getFolderPath(...args); expect(result).toBe(expectedResult); }); diff --git a/packages/core/upload/server/services/__tests__/folder.test.js b/packages/core/upload/server/services/__tests__/folder.test.js index 8a8bf8f7ae..9aee278d33 100644 --- a/packages/core/upload/server/services/__tests__/folder.test.js +++ b/packages/core/upload/server/services/__tests__/folder.test.js @@ -1,38 +1,38 @@ 'use strict'; -const { setLocationAndUID } = require('../folder'); +const { setPathAndUID } = require('../folder'); const folderUID = '9bc2352b-e29b-4ba3-810f-7b91033222de'; const uuidRegex = /^[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; -const rootLocationRegex = /^\/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; -const folderLocationRegex = new RegExp( +const rootPathRegex = /^\/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; +const folderPathRegex = new RegExp( '^/' + folderUID + '/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$', 'i' ); describe('folder', () => { - describe('setLocationAndUID', () => { + describe('setPathAndUID', () => { beforeAll(() => { global.strapi = { entityService: { - findOne: jest.fn(() => ({ location: `/${folderUID}` })), + findOne: jest.fn(() => ({ path: `/${folderUID}` })), }, }; }); test.each([ - [{ parent: 1 }, folderLocationRegex], - [{}, rootLocationRegex], - [{ parent: null }, rootLocationRegex], - ])('inputs %s', async (folder, expectedLocation) => { + [{ parent: 1 }, folderPathRegex], + [{}, rootPathRegex], + [{ parent: null }, rootPathRegex], + ])('inputs %s', async (folder, expectedPath) => { const clonedFolder = { ...folder }; - const result = await setLocationAndUID(clonedFolder); + const result = await setPathAndUID(clonedFolder); expect(result).toBe(clonedFolder); expect(result).toMatchObject({ ...folder, uid: expect.stringMatching(uuidRegex), - location: expect.stringMatching(expectedLocation), + path: expect.stringMatching(expectedPath), }); }); }); diff --git a/packages/core/upload/server/services/__tests__/upload.test.js b/packages/core/upload/server/services/__tests__/upload.test.js index fdcbcb33ad..0ebab147b5 100644 --- a/packages/core/upload/server/services/__tests__/upload.test.js +++ b/packages/core/upload/server/services/__tests__/upload.test.js @@ -9,7 +9,7 @@ describe('Upload service', () => { upload: { services: { file: { - getLocation: () => '/a-location', + getFolderPath: () => '/a-folder-path', }, }, }, diff --git a/packages/core/upload/server/services/file.js b/packages/core/upload/server/services/file.js index e1bb5bdbbf..7335dc13fe 100644 --- a/packages/core/upload/server/services/file.js +++ b/packages/core/upload/server/services/file.js @@ -2,14 +2,14 @@ const folderModel = 'plugin::upload.folder'; -const getLocation = async folderId => { +const getFolderPath = async folderId => { if (!folderId) return '/'; const parentFolder = await strapi.entityService.findOne(folderModel, folderId); - return parentFolder.location; + return parentFolder.path; }; module.exports = { - getLocation, + getFolderPath, }; diff --git a/packages/core/upload/server/services/folder.js b/packages/core/upload/server/services/folder.js index 3b7536a666..9ddfda77e3 100644 --- a/packages/core/upload/server/services/folder.js +++ b/packages/core/upload/server/services/folder.js @@ -1,39 +1,23 @@ 'use strict'; const uuid = require('uuid/v4'); -const { trimChars, trimCharsEnd, trimCharsStart } = require('lodash/fp'); - -// TODO: to use once https://github.com/strapi/strapi/pull/12534 is merged -// const { joinBy } = require('@strapi/utils'); +const { joinBy } = require('@strapi/utils'); const folderModel = 'plugin::upload.folder'; -const joinBy = (joint, ...args) => { - const trim = trimChars(joint); - const trimEnd = trimCharsEnd(joint); - const trimStart = trimCharsStart(joint); - - return args.reduce((url, path, index) => { - if (args.length === 1) return path; - if (index === 0) return trimEnd(path); - if (index === args.length - 1) return url + joint + trimStart(path); - return url + joint + trim(path); - }, ''); -}; - const generateUID = () => uuid(); -const setLocationAndUID = async folder => { +const setPathAndUID = async folder => { const uid = generateUID(); - let parentLocation = '/'; + let parentPath = '/'; if (folder.parent) { const parentFolder = await strapi.entityService.findOne(folderModel, folder.parent); - parentLocation = parentFolder.location; + parentPath = parentFolder.path; } return Object.assign(folder, { uid, - location: joinBy('/', parentLocation, uid), + path: joinBy('/', parentPath, uid), }); }; @@ -61,5 +45,5 @@ const exists = async (params = {}) => { module.exports = { exists, deleteByIds, - setLocationAndUID, + setPathAndUID, }; diff --git a/packages/core/upload/server/services/upload.js b/packages/core/upload/server/services/upload.js index 3cf6588163..f14b14da90 100644 --- a/packages/core/upload/server/services/upload.js +++ b/packages/core/upload/server/services/upload.js @@ -75,7 +75,7 @@ module.exports = ({ strapi }) => ({ alternativeText: fileInfo.alternativeText, caption: fileInfo.caption, folder: fileInfo.folder, - location: await fileService.getLocation(fileInfo.folder), + folderPath: await fileService.getFolderPath(fileInfo.folder), hash: generateFileName(basename), ext, mime: type, @@ -216,7 +216,7 @@ module.exports = ({ strapi }) => ({ alternativeText: _.isNil(alternativeText) ? dbFile.alternativeText : alternativeText, caption: _.isNil(caption) ? dbFile.caption : caption, folder: _.isUndefined(folder) ? dbFile.folder : folder, - location: _.isUndefined(folder) ? dbFile.path : await fileService.getLocation(folder), + folderPath: _.isUndefined(folder) ? dbFile.path : await fileService.getFolderPath(folder), }; return this.update(id, newInfos, { user }); diff --git a/packages/core/upload/tests/admin/file-folder.test.e2e.js b/packages/core/upload/tests/admin/file-folder.test.e2e.js index c05f63fad5..84f37ea535 100644 --- a/packages/core/upload/tests/admin/file-folder.test.e2e.js +++ b/packages/core/upload/tests/admin/file-folder.test.e2e.js @@ -76,7 +76,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: null, - location: '/', + folderPath: '/', }); data.files.push(file); @@ -115,7 +115,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: { id: data.folders[0].id }, - location: data.folders[0].location, + folderPath: data.folders[0].path, }); data.files.push(file); @@ -134,7 +134,7 @@ describe('File', () => { }); expect(res.status).toBe(400); - expect(res.body.error.message).toBe("the folder doesn't exist"); + expect(res.body.error.message).toBe('the folder does not exist'); }); }); @@ -170,7 +170,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: { id: data.folders[1].id }, - location: data.folders[1].location, + folderPath: data.folders[1].path, }); data.files[1] = file; }); @@ -204,7 +204,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: { id: data.folders[0].id }, - location: data.folders[0].location, + folderPath: data.folders[0].path, }); data.files[1] = file; }); @@ -240,7 +240,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: { id: data.folders[0].id }, - location: data.folders[0].location, + folderPath: data.folders[0].path, }); data.files[0] = file; }); @@ -274,7 +274,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: { id: data.folders[1].id }, - location: data.folders[1].location, + folderPath: data.folders[1].path, }); data.files[1] = file; }); @@ -311,7 +311,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: null, - location: '/', + folderPath: '/', }); data.files[0] = file; }); @@ -345,7 +345,7 @@ describe('File', () => { url: expect.any(String), provider: 'local', folder: null, - location: '/', + folderPath: '/', }); data.files[1] = file; }); @@ -365,7 +365,7 @@ describe('File', () => { }); expect(res.status).toBe(400); - expect(res.body.error.message).toBe("the folder doesn't exist"); + expect(res.body.error.message).toBe('the folder does not exist'); }); test('whithout replacing the file', async () => { @@ -374,13 +374,13 @@ describe('File', () => { url: `/upload?id=${data.files[1].id}`, formData: { fileInfo: JSON.stringify({ - folder: '1234', // id that doesn't exist + folder: '1234', // id that does not exist }), }, }); expect(res.status).toBe(400); - expect(res.body.error.message).toBe("the folder doesn't exist"); + expect(res.body.error.message).toBe('the folder does not exist'); }); }); }); diff --git a/packages/core/upload/tests/admin/folder.test.e2e.js b/packages/core/upload/tests/admin/folder.test.e2e.js index f15497891c..ad6601b5d8 100644 --- a/packages/core/upload/tests/admin/folder.test.e2e.js +++ b/packages/core/upload/tests/admin/folder.test.e2e.js @@ -15,8 +15,8 @@ let data = { }; const uuidRegex = /^[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; -const rootLocationRegex = /^\/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; -const getFolderLocationRegex = uid => +const rootPathRegex = /^\/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$/i; +const getFolderPathRegex = uid => new RegExp( '^/' + uid + '/[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$', 'i' @@ -51,12 +51,12 @@ describe('Folder', () => { id: expect.anything(), name: 'folder 1', uid: expect.stringMatching(uuidRegex), - location: expect.stringMatching(rootLocationRegex), + path: expect.stringMatching(rootPathRegex), createdAt: expect.anything(), updatedAt: expect.anything(), parent: null, }); - expect(res.body.data.uid).toBe(res.body.data.location.split('/').pop()); + expect(res.body.data.uid).toBe(res.body.data.path.split('/').pop()); data.folders.push(omit('parent', res.body.data)); }); @@ -75,12 +75,12 @@ describe('Folder', () => { id: expect.anything(), name: 'folder-2', uid: expect.stringMatching(uuidRegex), - location: expect.stringMatching(getFolderLocationRegex(data.folders[0].uid)), + path: expect.stringMatching(getFolderPathRegex(data.folders[0].uid)), createdAt: expect.anything(), updatedAt: expect.anything(), parent: data.folders[0], }); - expect(res.body.data.uid).toBe(res.body.data.location.split('/').pop()); + expect(res.body.data.uid).toBe(res.body.data.path.split('/').pop()); data.folders.push(omit('parent', res.body.data)); }); @@ -188,10 +188,7 @@ describe('Folder', () => { username: null, }, files: { count: 0 }, - parent: pick( - ['createdAt', 'id', 'name', 'location', 'uid', 'updatedAt'], - data.folders[0] - ), + parent: pick(['createdAt', 'id', 'name', 'path', 'uid', 'updatedAt'], data.folders[0]), updatedBy: { firstname: expect.anything(), id: expect.anything(), @@ -216,8 +213,8 @@ describe('Folder', () => { expect(res.body.data).toEqual( expect.arrayContaining([ - pick(['id', 'name', 'location', 'uid', 'updatedAt', 'createdAt'])(data.folders[0]), - pick(['id', 'name', 'location', 'uid', 'updatedAt', 'createdAt'])(data.folders[1]), + pick(['id', 'name', 'path', 'uid', 'updatedAt', 'createdAt'])(data.folders[0]), + pick(['id', 'name', 'path', 'uid', 'updatedAt', 'createdAt'])(data.folders[1]), ]) ); });