From e5abefb4af76f4a40e2d38a421ccf64eeee073bc Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Mon, 29 Apr 2019 01:22:48 +0700 Subject: [PATCH 1/2] Improvements for product/project template refactoring --- src/routes/productTemplates/create.js | 4 +- src/routes/productTemplates/create.spec.js | 31 ++++++++++- src/routes/productTemplates/update.js | 4 +- src/routes/productTemplates/update.spec.js | 30 ++++++++++- src/routes/projectTemplates/create.js | 56 ++++--------------- src/routes/projectTemplates/create.spec.js | 33 ++++++++++++ src/routes/projectTemplates/update.js | 62 ++++------------------ src/routes/projectTemplates/update.spec.js | 32 +++++++++++ src/routes/projectTemplates/upgrade.js | 42 ++------------- 9 files changed, 153 insertions(+), 141 deletions(-) diff --git a/src/routes/productTemplates/create.js b/src/routes/productTemplates/create.js index 87a70b91..9d208e72 100644 --- a/src/routes/productTemplates/create.js +++ b/src/routes/productTemplates/create.js @@ -34,7 +34,9 @@ const schema = { createdBy: Joi.any().strip(), updatedBy: Joi.any().strip(), deletedBy: Joi.any().strip(), - }).required(), + }) + .xor('form', 'template') + .required(), }, }; diff --git a/src/routes/productTemplates/create.spec.js b/src/routes/productTemplates/create.spec.js index 2da8feba..14f3807d 100644 --- a/src/routes/productTemplates/create.spec.js +++ b/src/routes/productTemplates/create.spec.js @@ -89,13 +89,18 @@ describe('CREATE product template', () => { }, }; - const bodyWithForm = _.cloneDeep(body); - bodyWithForm.param.form = { + const bodyDefinedFormTemplate = _.cloneDeep(body); + bodyDefinedFormTemplate.param.form = { version: 1, key: 'dev', }; + + const bodyWithForm = _.cloneDeep(bodyDefinedFormTemplate); delete bodyWithForm.param.template; + const bodyMissingFormTemplate = _.cloneDeep(bodyWithForm); + delete bodyMissingFormTemplate.param.form; + const bodyInvalidForm = _.cloneDeep(body); bodyInvalidForm.param.form = { version: 1, @@ -277,5 +282,27 @@ describe('CREATE product template', () => { .send(bodyInvalidForm) .expect(422, done); }); + + it('should return 422 if both form or template field are defined', (done) => { + request(server) + .post('/v4/projects/metadata/productTemplates') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyDefinedFormTemplate) + .expect('Content-Type', /json/) + .expect(422, done); + }); + + it('should return 422 if both form or template field are missing', (done) => { + request(server) + .post('/v4/projects/metadata/productTemplates') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyMissingFormTemplate) + .expect('Content-Type', /json/) + .expect(422, done); + }); }); }); diff --git a/src/routes/productTemplates/update.js b/src/routes/productTemplates/update.js index 5a313424..1095d9bf 100644 --- a/src/routes/productTemplates/update.js +++ b/src/routes/productTemplates/update.js @@ -37,7 +37,9 @@ const schema = { createdBy: Joi.any().strip(), updatedBy: Joi.any().strip(), deletedBy: Joi.any().strip(), - }).required(), + }) + .xor('form', 'template') + .required(), }, }; diff --git a/src/routes/productTemplates/update.spec.js b/src/routes/productTemplates/update.spec.js index c17d9427..633ca3e0 100644 --- a/src/routes/productTemplates/update.spec.js +++ b/src/routes/productTemplates/update.spec.js @@ -133,12 +133,18 @@ describe('UPDATE product template', () => { }, }; - const bodyWithForm = _.cloneDeep(body); - bodyWithForm.param.form = { + const bodyDefinedFormTemplate = _.cloneDeep(body); + bodyDefinedFormTemplate.param.form = { version: 1, key: 'dev', }; + const bodyWithForm = _.cloneDeep(bodyDefinedFormTemplate); + delete bodyWithForm.param.template; + + const bodyMissingFormTemplate = _.cloneDeep(bodyWithForm); + delete bodyMissingFormTemplate.param.form; + const bodyInvalidForm = _.cloneDeep(body); bodyInvalidForm.param.form = { version: 1, @@ -312,5 +318,25 @@ describe('UPDATE product template', () => { .send(bodyInvalidForm) .expect(422, done); }); + + it('should return 422 if both form or template field are defined', (done) => { + request(server) + .patch(`/v4/projects/metadata/productTemplates/${templateId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyDefinedFormTemplate) + .expect(422, done); + }); + + it('should return 422 if both form or template field are missing', (done) => { + request(server) + .patch(`/v4/projects/metadata/productTemplates/${templateId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyMissingFormTemplate) + .expect(422, done); + }); }); }); diff --git a/src/routes/projectTemplates/create.js b/src/routes/projectTemplates/create.js index 12a0dfd2..cbd28e2e 100644 --- a/src/routes/projectTemplates/create.js +++ b/src/routes/projectTemplates/create.js @@ -35,7 +35,11 @@ const schema = { createdBy: Joi.any().strip(), updatedBy: Joi.any().strip(), deletedBy: Joi.any().strip(), - }).required(), + }) + .xor('form', 'scope') + .xor('phases', 'planConfig') + .nand('priceConfig', 'scope') + .required(), }, }; @@ -47,54 +51,12 @@ module.exports = [ const param = req.body.param; const { form, priceConfig, planConfig } = param; - const checkModel = (keyInfo, modelName, model) => { - let errorMessage = ''; - if (keyInfo == null) { - return Promise.resolve(null); - } - if ((keyInfo.version != null) && (keyInfo.key != null)) { - errorMessage = `${modelName} with key ${keyInfo.key} and version ${keyInfo.version}` - + ' referred in the project template is not found'; - return (model.findOne({ - where: { - key: keyInfo.key, - version: keyInfo.version, - }, - })).then((record) => { - if (record == null) { - return Promise.resolve(errorMessage); - } - return Promise.resolve(null); - }); - } else if ((keyInfo.version == null) && (keyInfo.key != null)) { - errorMessage = `${modelName} with key ${keyInfo.key}` - + ' referred in the project template is not found'; - return model.findOne({ - where: { - key: keyInfo.key, - }, - }).then((record) => { - if (record == null) { - return Promise.resolve(errorMessage); - } - return Promise.resolve(null); - }); - } - return Promise.resolve(null); - }; - return Promise.all([ - checkModel(form, 'Form', models.Form, next), - checkModel(priceConfig, 'PriceConfig', models.PriceConfig, next), - checkModel(planConfig, 'PlanConfig', models.PlanConfig, next), + util.checkModel(form, 'Form', models.Form, 'project template'), + util.checkModel(priceConfig, 'PriceConfig', models.PriceConfig, 'project template'), + util.checkModel(planConfig, 'PlanConfig', models.PlanConfig, 'project template'), ]) - .then((errorMessages) => { - const errorMessage = errorMessages.find(e => e && e.length > 0); - if (errorMessage) { - const apiErr = new Error(errorMessage); - apiErr.status = 422; - throw apiErr; - } + .then(() => { const entity = _.assign(req.body.param, { createdBy: req.authUser.userId, updatedBy: req.authUser.userId, diff --git a/src/routes/projectTemplates/create.spec.js b/src/routes/projectTemplates/create.spec.js index 9d9fd594..98980998 100644 --- a/src/routes/projectTemplates/create.spec.js +++ b/src/routes/projectTemplates/create.spec.js @@ -108,6 +108,17 @@ describe('CREATE project template', () => { }, }; + const bodyDefinedFormScope = _.cloneDeep(body); + bodyDefinedFormScope.param.form = { + scope1: { + subScope1A: 1, + subScope1B: 2, + }, + scope2: [1, 2, 3], + }; + const bodyMissingFormScope = _.cloneDeep(body); + delete bodyMissingFormScope.param.scope; + it('should return 403 if user is not authenticated', (done) => { request(server) .post('/v4/projects/metadata/projectTemplates') @@ -270,5 +281,27 @@ describe('CREATE project template', () => { done(); }); }); + + it('should return 422 if both scope and form are defined', (done) => { + request(server) + .post('/v4/projects/metadata/projectTemplates') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyDefinedFormScope) + .expect('Content-Type', /json/) + .expect(422, done); + }); + + it('should return 422 if both scope and form are missing', (done) => { + request(server) + .post('/v4/projects/metadata/projectTemplates') + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyMissingFormScope) + .expect('Content-Type', /json/) + .expect(422, done); + }); }); }); diff --git a/src/routes/projectTemplates/update.js b/src/routes/projectTemplates/update.js index 4991a310..74445796 100644 --- a/src/routes/projectTemplates/update.js +++ b/src/routes/projectTemplates/update.js @@ -38,7 +38,11 @@ const schema = { createdBy: Joi.any().strip(), updatedBy: Joi.any().strip(), deletedBy: Joi.any().strip(), - }).required(), + }) + .xor('form', 'scope') + .xor('phases', 'planConfig') + .nand('priceConfig', 'scope') + .required(), }, }; @@ -50,54 +54,12 @@ module.exports = [ const param = req.body.param; const { form, priceConfig, planConfig } = param; - const checkModel = (keyInfo, modelName, model) => { - let errorMessage = ''; - if (keyInfo == null) { - return Promise.resolve(null); - } - if ((keyInfo.version != null) && (keyInfo.key != null)) { - errorMessage = `${modelName} with key ${keyInfo.key} and version ${keyInfo.version}` - + ' referred in the project template is not found'; - return (model.findOne({ - where: { - key: keyInfo.key, - version: keyInfo.version, - }, - })).then((record) => { - if (record == null) { - return Promise.resolve(errorMessage); - } - return Promise.resolve(null); - }); - } else if ((keyInfo.version == null) && (keyInfo.key != null)) { - errorMessage = `${modelName} with key ${keyInfo.key}` - + ' referred in the project template is not found'; - return model.findOne({ - where: { - key: keyInfo.key, - }, - }).then((record) => { - if (record == null) { - return Promise.resolve(errorMessage); - } - return Promise.resolve(null); - }); - } - return Promise.resolve(null); - }; - return Promise.all([ - checkModel(form, 'Form', models.Form, next), - checkModel(priceConfig, 'PriceConfig', models.PriceConfig, next), - checkModel(planConfig, 'PlanConfig', models.PlanConfig, next), + util.checkModel(form, 'Form', models.Form, 'project template'), + util.checkModel(priceConfig, 'PriceConfig', models.PriceConfig, 'project template'), + util.checkModel(planConfig, 'PlanConfig', models.PlanConfig, 'project template'), ]) - .then((errorMessages) => { - const errorMessage = errorMessages.find(e => e && e.length > 0); - if (errorMessage) { - const apiErr = new Error(errorMessage); - apiErr.status = 422; - throw apiErr; - } + .then(() => { const entityToUpdate = _.assign(req.body.param, { updatedBy: req.authUser.userId, }); @@ -115,7 +77,7 @@ module.exports = [ if (!projectTemplate) { const apiErr = new Error(`Project template not found for template id ${req.params.templateId}`); apiErr.status = 404; - return Promise.reject(apiErr); + throw apiErr; } // Merge JSON fields @@ -132,9 +94,7 @@ module.exports = [ }) .then((projectTemplate) => { res.json(util.wrapResponse(req.id, projectTemplate)); - return Promise.resolve(); - }) - .catch(next); + }); }).catch(next); }, ]; diff --git a/src/routes/projectTemplates/update.spec.js b/src/routes/projectTemplates/update.spec.js index f62a8252..2f65ff6f 100644 --- a/src/routes/projectTemplates/update.spec.js +++ b/src/routes/projectTemplates/update.spec.js @@ -2,6 +2,7 @@ * Tests for get.js */ import chai from 'chai'; +import _ from 'lodash'; import request from 'supertest'; import models from '../../models'; @@ -158,6 +159,17 @@ describe('UPDATE project template', () => { }, }; + const bodyDefinedFormScope = _.cloneDeep(body); + bodyDefinedFormScope.param.form = { + scope1: { + subScope1A: 1, + subScope1B: 2, + }, + scope2: [1, 2, 3], + }; + const bodyMissingFormScope = _.cloneDeep(body); + delete bodyMissingFormScope.param.scope; + it('should return 403 if user is not authenticated', (done) => { request(server) .patch(`/v4/projects/metadata/projectTemplates/${templateId}`) @@ -336,5 +348,25 @@ describe('UPDATE project template', () => { .expect(200) .end(done); }); + + it('should return 422 if both scope and form are defined', (done) => { + request(server) + .patch(`/v4/projects/metadata/projectTemplates/${templateId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyDefinedFormScope) + .expect(422, done); + }); + + it('should return 422 if both scope and form are missing', (done) => { + request(server) + .patch(`/v4/projects/metadata/projectTemplates/${templateId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send(bodyMissingFormScope) + .expect(422, done); + }); }); }); diff --git a/src/routes/projectTemplates/upgrade.js b/src/routes/projectTemplates/upgrade.js index f1eb4625..c6706b80 100644 --- a/src/routes/projectTemplates/upgrade.js +++ b/src/routes/projectTemplates/upgrade.js @@ -44,37 +44,14 @@ module.exports = [ if (pt == null) { const apiErr = new Error(`project template not found for id ${req.body.param.templateId}`); apiErr.status = 404; - return Promise.reject(apiErr); + throw apiErr; } if ((pt.scope == null) || (pt.phases == null)) { const apiErr = new Error('Current project template\'s scope or phases is null'); apiErr.status = 422; - return Promise.reject(apiErr); + throw apiErr; } - const checkModel = (keyInfo, modelName, model) => { - let errorMessage = ''; - errorMessage = `${modelName} with key ${keyInfo.key} and version ${keyInfo.version}` - + ' referred in param is not found'; - return (model.findOne({ - where: { - key: keyInfo.key, - version: keyInfo.version, - }, - })).then((record) => { - if (record == null) { - return Promise.resolve(errorMessage); - } - return Promise.resolve(null); - }); - }; - - const reportError = (errorMessage) => { - const apiErr = new Error(errorMessage); - apiErr.status = 422; - return Promise.reject(apiErr).catch(next); - }; - // get form field let newForm = {}; if (req.body.param.form == null) { @@ -90,10 +67,7 @@ module.exports = [ }; } else { newForm = req.body.param.form; - const err = await checkModel(newForm, 'Form', models.Form); - if (err != null) { - reportError(err); - } + await util.checkModel(newForm, 'Form', models.Form, 'project template'); } // get price config field let newPriceConfig = {}; @@ -111,10 +85,7 @@ module.exports = [ }; } else { newPriceConfig = req.body.param.priceConfig; - const err = await checkModel(newPriceConfig, 'PriceConfig', models.PriceConfig); - if (err != null) { - reportError(err); - } + await util.checkModel(newPriceConfig, 'PriceConfig', models.PriceConfig, 'project template'); } // get plan config field let newPlanConfig = {}; @@ -126,10 +97,7 @@ module.exports = [ }; } else { newPlanConfig = req.body.param.planConfig; - const err = await checkModel(newPlanConfig, 'PlanConfig', models.PlanConfig); - if (err != null) { - reportError(err); - } + await util.checkModel(newPlanConfig, 'PlanConfig', models.PlanConfig, 'project template'); } const updateInfo = { From bae50009d4ed196cf5601ca475966bbcfe80e000 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Mon, 29 Apr 2019 20:00:48 +0700 Subject: [PATCH 2/2] update joi validation --- src/routes/productTemplates/create.js | 4 ++-- src/routes/productTemplates/update.js | 4 ++-- src/routes/projectTemplates/create.js | 10 +++++----- src/routes/projectTemplates/update.js | 10 +++++----- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/routes/productTemplates/create.js b/src/routes/productTemplates/create.js index 9d208e72..489d37ed 100644 --- a/src/routes/productTemplates/create.js +++ b/src/routes/productTemplates/create.js @@ -23,8 +23,8 @@ const schema = { brief: Joi.string().max(45).required(), details: Joi.string().max(255).required(), aliases: Joi.array().required(), - template: Joi.object(), - form: Joi.object(), + template: Joi.object().empty(null), + form: Joi.object().empty(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), isAddOn: Joi.boolean().optional(), diff --git a/src/routes/productTemplates/update.js b/src/routes/productTemplates/update.js index 1095d9bf..73ab5856 100644 --- a/src/routes/productTemplates/update.js +++ b/src/routes/productTemplates/update.js @@ -26,8 +26,8 @@ const schema = { brief: Joi.string().max(45), details: Joi.string().max(255), aliases: Joi.array(), - template: Joi.object(), - form: Joi.object(), + template: Joi.object().empty(null), + form: Joi.object().empty(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), isAddOn: Joi.boolean().optional(), diff --git a/src/routes/projectTemplates/create.js b/src/routes/projectTemplates/create.js index cbd28e2e..0869a694 100644 --- a/src/routes/projectTemplates/create.js +++ b/src/routes/projectTemplates/create.js @@ -22,11 +22,11 @@ const schema = { question: Joi.string().max(255).required(), info: Joi.string().max(255).required(), aliases: Joi.array().required(), - scope: Joi.object().optional().allow(null), - phases: Joi.object().optional().allow(null), - form: Joi.object().optional().allow(null), - planConfig: Joi.object().optional().allow(null), - priceConfig: Joi.object().optional().allow(null), + scope: Joi.object().empty(null), + phases: Joi.object().empty(null), + form: Joi.object().empty(null), + planConfig: Joi.object().empty(null), + priceConfig: Joi.object().empty(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), createdAt: Joi.any().strip(), diff --git a/src/routes/projectTemplates/update.js b/src/routes/projectTemplates/update.js index 74445796..c356508e 100644 --- a/src/routes/projectTemplates/update.js +++ b/src/routes/projectTemplates/update.js @@ -25,11 +25,11 @@ const schema = { question: Joi.string().max(255), info: Joi.string().max(255), aliases: Joi.array(), - scope: Joi.object().optional().allow(null), - phases: Joi.object().optional().allow(null), - form: Joi.object().optional().allow(null), - planConfig: Joi.object().optional().allow(null), - priceConfig: Joi.object().optional().allow(null), + scope: Joi.object().empty(null), + phases: Joi.object().empty(null), + form: Joi.object().empty(null), + planConfig: Joi.object().empty(null), + priceConfig: Joi.object().empty(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), createdAt: Joi.any().strip(),