From 08feb63c9413574bdba0ed64879d1229c6a2eddc Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Thu, 31 Oct 2019 12:41:01 +0800 Subject: [PATCH 1/4] fix: unit tests always pass Fixed method expectObj() which is used to assert objects. Before it always return true for any passed values. The reason for that was that "_.pick(obj1, _.identity)" returns an empty object {} for any obj1. --- test/common/testHelper.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/common/testHelper.js b/test/common/testHelper.js index cdb03cb..3b0d98f 100644 --- a/test/common/testHelper.js +++ b/test/common/testHelper.js @@ -51,21 +51,21 @@ async function getMetadataESData (id) { } /** - * Expect given objects are equal, ignoring some fields if provided. - * @param {Object} obj1 obj1 - * @param {Object} obj2 obj2 + * Expect given objects are equal, or only some fields should be tested if provided. + * + * @param {Object} target target + * @param {Object} expected expected * @param {Object} compareFields compare fields - * @returns {ExpectStatic} the elastic search data of id of configured index type in configured index */ -function expectObj (obj1, obj2, compareFields) { - let o1 = _.pick(obj1, _.identity) - let o2 = _.pick(obj2, _.identity) - if (compareFields) { - o1 = _.pick(o1, compareFields) - o2 = _.pick(o2, compareFields) - } +function expectObj (target, expected, compareFields) { + expect(target, 'expectObj(): "target" object should not be undefined').to.not.be.an('undefined') + expect(expected, 'expectObj(): "expected" object should not be undefined').to.not.be.an('undefined') - expect(_.isEqual(o1, o2)).to.equal(true) + const targetClean = compareFields ? _.pick(target, compareFields) : target + const expectedClean = compareFields ? _.pick(expected, compareFields) : expected + + // check if objects are deeply equal by values + expect(targetClean).to.eql(expectedClean) } module.exports = { From f173a7110a1ebd307188edc179071aacc2fbdc3a Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Thu, 31 Oct 2019 12:43:50 +0800 Subject: [PATCH 2/4] fix: unit tests for productCategories This test failed after the fixing of expectObj() method. --- src/services/ProcessorServiceProductCategory.js | 6 +++--- test/common/testData.js | 8 ++++---- test/e2e/processor.metadata.index.test.js | 16 ++++++++-------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/services/ProcessorServiceProductCategory.js b/src/services/ProcessorServiceProductCategory.js index ca3fd94..ee712f3 100644 --- a/src/services/ProcessorServiceProductCategory.js +++ b/src/services/ProcessorServiceProductCategory.js @@ -76,7 +76,7 @@ async function create (message) { } await helper.updateMetadadaESPromise(updateDocPromise) - logger.debug(`Product category created successfully in elasticsearch index, (productCategoryId: ${message.key})`) + logger.debug(`Product category created successfully in elasticsearch index, (productCategoryKey: ${message.key})`) } create.schema = { @@ -102,7 +102,7 @@ async function update (message) { } await helper.updateMetadadaESPromise(updateDocPromise) - logger.debug(`Product category updated successfully in elasticsearch index, (productCategoryId: ${message.key})`) + logger.debug(`Product category updated successfully in elasticsearch index, (productCategoryKey: ${message.key})`) } update.schema = { @@ -122,7 +122,7 @@ async function deleteMessage (message) { } await helper.updateMetadadaESPromise(updateDocPromise) - logger.debug(`Product category deleted successfully in elasticsearch index, (productCategoryId: ${message.key})`) + logger.debug(`Product category deleted successfully in elasticsearch index, (productCategoryKey: ${message.key})`) } deleteMessage.schema = { diff --git a/test/common/testData.js b/test/common/testData.js index 1313b3e..d489570 100644 --- a/test/common/testData.js +++ b/test/common/testData.js @@ -136,8 +136,8 @@ const orgConfigId = orgConfigCreatedMessage.payload.id const projectTemplateId = projectTemplateCreatedMessage.payload.id const projectTypeId = projectTypeCreatedMessage.payload.key const projectTypeNotFoundId = `${projectTypeCreatedMessage.payload.key}_notFound` -const productCategoryId = productCategoryCreatedMessage.payload.id -const productCategoryNotFoundId = `${productCategoryCreatedMessage.payload.key}_notFound` +const productCategoryKey = productCategoryCreatedMessage.payload.key +const productCategoryNotFoundKey = `${productCategoryCreatedMessage.payload.key}_notFound` const metadataId = config.get('esConfig.ES_METADATA_DEFAULT_ID') let notFoundId = getRandomInt(10000000) // we use projectId+1, timelineId+1, attachmentId+1,... to create another object if need @@ -186,8 +186,8 @@ module.exports = { projectTemplateId, projectTypeId, projectTypeNotFoundId, - productCategoryId, - productCategoryNotFoundId, + productCategoryKey, + productCategoryNotFoundKey, productCategoryUpdatedMessage, productCategoryCreatedMessage, productCategoryDeletedMessage, diff --git a/test/e2e/processor.metadata.index.test.js b/test/e2e/processor.metadata.index.test.js index bacd09b..f8e4b13 100644 --- a/test/e2e/processor.metadata.index.test.js +++ b/test/e2e/processor.metadata.index.test.js @@ -25,8 +25,8 @@ const { projectTemplateId, projectTypeId, projectTypeNotFoundId, - productCategoryId, - productCategoryNotFoundId, + productCategoryKey, + productCategoryNotFoundKey, productCategoryUpdatedMessage, productCategoryCreatedMessage, productCategoryDeletedMessage, @@ -106,7 +106,7 @@ describe('TC Product Category Topic Tests', () => { it('create product category message', async () => { await ProcessorService.create(productCategoryCreatedMessage) const data = await testHelper.getMetadataESData(metadataId) - testHelper.expectObj(_.find(data.productCategories, { key: productCategoryId }), + testHelper.expectObj(_.find(data.productCategories, { key: productCategoryKey }), productCategoryCreatedMessage.payload, _.keys(_.omit(productCategoryCreatedMessage.payload, ['resource']))) }) @@ -114,7 +114,7 @@ describe('TC Product Category Topic Tests', () => { it('create product category message - already exists', async () => { await ProcessorService.create(productCategoryCreatedMessage) const data = await testHelper.getMetadataESData(metadataId) - testHelper.expectObj(_.find(data.productCategories, { key: productCategoryId }), + testHelper.expectObj(_.find(data.productCategories, { key: productCategoryKey }), productCategoryCreatedMessage.payload, _.keys(_.omit(productCategoryCreatedMessage.payload, ['resource']))) }) @@ -122,23 +122,23 @@ describe('TC Product Category Topic Tests', () => { it('update product category message', async () => { await ProcessorService.update(productCategoryUpdatedMessage) const data = await testHelper.getMetadataESData(metadataId) - testHelper.expectObj(_.find(data.productCategories, { key: productCategoryId }), + testHelper.expectObj(_.find(data.productCategories, { key: productCategoryKey }), productCategoryUpdatedMessage.payload, _.keys(_.omit(productCategoryUpdatedMessage.payload, ['resource']))) }) it('update product category message - not found', async () => { const message = _.cloneDeep(productCategoryUpdatedMessage) - message.payload.key = productCategoryNotFoundId + message.payload.key = productCategoryNotFoundKey await ProcessorService.update(message) const data = await testHelper.getMetadataESData(metadataId) - expect(_.find(data.productCategories, { key: productCategoryNotFoundId })).to.be.an('undefined') + expect(_.find(data.productCategories, { key: productCategoryNotFoundKey })).to.be.an('undefined') }) it('delete product category message', async () => { await ProcessorService.deleteMessage(productCategoryDeletedMessage) const data = await testHelper.getMetadataESData(metadataId) - expect(_.find(data.productCategories, { key: productCategoryId })).to.be.an('undefined') + expect(_.find(data.productCategories, { key: productCategoryKey })).to.be.an('undefined') }) }) From c507299ebb7dd690af8bafa7c6579a4227c9c6ba Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Thu, 31 Oct 2019 13:49:47 +0800 Subject: [PATCH 3/4] fix: unit tests for ProjectTemplates and ProductTemplates This issue become visible after fixing the expectObj() method. The thing here is that when we use .empty(null) in Joi then if property is passed as "null" it's stripped from the object. As a result, we pass an object with "null" value, but we are trying to save in ES the object without value. This leads to the unit test fails, as saved value doesn't match the object which has been send in the message. --- .../ProcessorServiceProductTemplate.js | 11 +++++++--- .../ProcessorServiceProjectTemplate.js | 22 ++++++++++++------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/services/ProcessorServiceProductTemplate.js b/src/services/ProcessorServiceProductTemplate.js index ce56aff..611eea2 100644 --- a/src/services/ProcessorServiceProductTemplate.js +++ b/src/services/ProcessorServiceProductTemplate.js @@ -32,11 +32,11 @@ function createSchema () { brief: Joi.string().max(45), details: Joi.string().max(255), aliases: Joi.array(), - template: Joi.object().empty(null), + template: Joi.object().allow(null), form: Joi.object().keys({ key: Joi.string().required(), version: Joi.number() - }).empty(null), + }).allow(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), isAddOn: Joi.boolean().optional(), @@ -47,7 +47,12 @@ function createSchema () { updatedBy: Joi.any(), deletedBy: Joi.any() }) - .xor('form', 'template') + // TODO rewrite this condition so only one of these must be "present" and "not-null" + // - if we just uncomment it, then if one these is passed as null, it would be treated as passed + // - if we uncomment it and change above from '.allow(null)' to '.empty(null)' + // then this check would pass successfully, BUT the null value would be removed from the object + // so the object would end up WITHOUT null value which is expected for consistency. + // .xor('form', 'template') .unknown(true) .required() } diff --git a/src/services/ProcessorServiceProjectTemplate.js b/src/services/ProcessorServiceProjectTemplate.js index fd68073..dc3330e 100644 --- a/src/services/ProcessorServiceProjectTemplate.js +++ b/src/services/ProcessorServiceProjectTemplate.js @@ -24,20 +24,20 @@ function createIdSchema () { */ function updateSchema () { return createIdSchema().keys({ - scope: Joi.object().empty(null), - phases: Joi.object().empty(null), + scope: Joi.object().allow(null), + phases: Joi.object().allow(null), form: Joi.object().keys({ key: Joi.string().required(), version: Joi.number() - }).empty(null), + }).allow(null), planConfig: Joi.object().keys({ key: Joi.string().required(), version: Joi.number() - }).empty(null), + }).allow(null), priceConfig: Joi.object().keys({ key: Joi.string().required(), version: Joi.number() - }).empty(null), + }).allow(null), disabled: Joi.boolean().optional(), hidden: Joi.boolean().optional(), createdAt: Joi.any(), @@ -63,9 +63,15 @@ function createSchema () { info: Joi.string().max(255).required(), aliases: Joi.array().required() }) - .xor('form', 'scope') - .xor('phases', 'planConfig') - .nand('priceConfig', 'scope') + // TODO rewrite these condition so only one of these must be "present" and "not-null" + // - if we just uncomment it, then if one these is passed as null, it would be treated as passed + // - if we uncomment it and change above from '.allow(null)' to '.empty(null)' + // then this check would pass successfully, BUT the null value would be removed from the object + // so the object would end up WITHOUT null value which is expected for consistency. + // .xor('form', 'scope') + // .xor('phases', 'planConfig') + // TODO rewrite this rule too accordingly + // .nand('priceConfig', 'scope') } /** From 9200263dbf1bc9c29787e45108e969e203db61e6 Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Thu, 31 Oct 2019 14:23:17 +0800 Subject: [PATCH 4/4] feat: disable indexing objects on milestone and phase create and update Temporary disable indexing these objects as for now we keep the logic for this inside Project Service. It's because creating or updating a phase or milestone may cause cascading updates of other phases or milestones. In such cases in Project Service we are doing one ES index call instead of multiple calls. Otherwise ES may fail with error `version conflict`. This would be turned on back, as soon as we get rid of such cascading updates inside Project Service. --- src/services/ProcessorServiceMilestone.js | 24 +++++++++++++++++------ src/services/ProcessorServicePhase.js | 24 +++++++++++++++++------ test/e2e/processor.project.index.test.js | 4 ++-- test/e2e/processor.timeline.index.test.js | 2 +- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/services/ProcessorServiceMilestone.js b/src/services/ProcessorServiceMilestone.js index a837029..9fa1d6c 100644 --- a/src/services/ProcessorServiceMilestone.js +++ b/src/services/ProcessorServiceMilestone.js @@ -66,7 +66,7 @@ function createSchema () { */ async function create (message) { // handle ES Update - async function updateDocPromise (doc) { + async function updateDocPromise (doc) { // eslint-disable-line no-unused-vars const milestones = _.isArray(doc._source.milestones) ? doc._source.milestones : [] const existingMilestoneIndex = _.findIndex(milestones, p => p.id === message.id)// if milestone does not exists already @@ -88,8 +88,14 @@ async function create (message) { return _.assign(doc._source, { milestones }) } - await helper.updateTimelineESPromise(message.timelineId, updateDocPromise) - logger.debug(`Milestone created successfully in elasticsearch index, (milestoneId: ${message.id})`) + // NOTE Disable indexing milestones when create at the moment, as it's now being indexed inside Project Service. + // It's because adding a milestones may cause cascading updates of other milestones and in such cases we are doing + // one ES index call instead of multiple calls. Otherwise ES may fail with error `version conflict`. + // This would be turned on back, as soon as we get rid of such cascading updates inside Project Service. + // + // await helper.updateTimelineESPromise(message.timelineId, updateDocPromise) + // logger.debug(`Milestone created successfully in elasticsearch index, (milestoneId: ${message.id})`) + logger.debug(`TEMPORARY SKIPPED: Milestone created successfully in elasticsearch index, (milestoneId: ${message.id})`) } create.schema = { @@ -103,7 +109,7 @@ create.schema = { */ async function update (message) { // handle ES Update - async function updateDocPromise (doc) { + async function updateDocPromise (doc) { // eslint-disable-line no-unused-vars const milestones = _.map(doc._source.milestones, (single) => { if (single.id === message.id) { return _.assign(single, message) @@ -113,8 +119,14 @@ async function update (message) { return _.assign(doc._source, { milestones }) } - await helper.updateTimelineESPromise(message.timelineId, updateDocPromise) - logger.debug(`Milestone updated successfully in elasticsearch index, (milestoneId: ${message.id})`) + // NOTE Disable indexing milestones when update at the moment, as it's now being indexed inside Project Service. + // It's because updating a milestones may cause cascading updates of other milestones and in such cases we are doing + // one ES index call instead of multiple calls. Otherwise ES may fail with error `version conflict`. + // This would be turned on back, as soon as we get rid of such cascading updates inside Project Service. + // + // await helper.updateTimelineESPromise(message.timelineId, updateDocPromise) + // logger.debug(`Milestone updated successfully in elasticsearch index, (milestoneId: ${message.id})`) + logger.debug(`TEMPORARY SKIPPED: Milestone updated successfully in elasticsearch index, (milestoneId: ${message.id})`) } update.schema = { diff --git a/src/services/ProcessorServicePhase.js b/src/services/ProcessorServicePhase.js index b0bc75e..80fdaa8 100644 --- a/src/services/ProcessorServicePhase.js +++ b/src/services/ProcessorServicePhase.js @@ -45,7 +45,7 @@ function createSchema () { */ async function create (message) { // handle ES Update - async function updateDocPromise (doc) { + async function updateDocPromise (doc) { // eslint-disable-line no-unused-vars const phases = _.isArray(doc._source.phases) ? doc._source.phases : [] const existingPhaseIndex = _.findIndex(phases, p => p.id === message.id)// if phase does not exists already if (existingPhaseIndex === -1) { @@ -66,8 +66,14 @@ async function create (message) { return _.assign(doc._source, { phases }) } - await helper.updateProjectESPromise(message.projectId, updateDocPromise) - logger.debug(`Project phase created successfully in elasticsearch index, (projectPhaseId: ${message.id})`) + // NOTE Disable indexing phases when create at the moment, as it's now being indexed inside Project Service. + // It's because adding a phase may cause cascading updates of other phases and in such cases we are doing + // one ES index call instead of multiple calls. Otherwise ES may fail with error `version conflict`. + // This would be turned on back, as soon as we get rid of such cascading updates inside Project Service. + // + // await helper.updateProjectESPromise(message.projectId, updateDocPromise) + // logger.debug(`Project phase created successfully in elasticsearch index, (projectPhaseId: ${message.id})`) + logger.debug(`TEMPORARY SKIPPED: Project phase created successfully in elasticsearch index, (projectPhaseId: ${message.id})`) } create.schema = { @@ -81,7 +87,7 @@ create.schema = { */ async function update (message) { // handle ES Update - async function updateDocPromise (doc) { + async function updateDocPromise (doc) { // eslint-disable-line no-unused-vars const phases = _.map(doc._source.phases, (single) => { if (single.id === message.id) { return _.assign(single, message) @@ -91,8 +97,14 @@ async function update (message) { return _.assign(doc._source, { phases }) } - await helper.updateProjectESPromise(message.projectId, updateDocPromise) - logger.debug(`Project phase updated successfully in elasticsearch index, (projectPhaseId: ${message.id})`) + // NOTE Disable indexing phases when update at the moment, as it's now being indexed inside Project Service. + // It's because updating a phase may cause cascading updates of other phases and in such cases we are doing + // one ES index call instead of multiple calls. Otherwise ES may fail with error `version conflict`. + // This would be turned on back, as soon as we get rid of such cascading updates inside Project Service. + // + // await helper.updateProjectESPromise(message.projectId, updateDocPromise) + // logger.debug(`Project phase updated successfully in elasticsearch index, (projectPhaseId: ${message.id})`) + logger.debug(`TEMPORARY SKIPPED: Project phase updated successfully in elasticsearch index, (projectPhaseId: ${message.id})`) } update.schema = { diff --git a/test/e2e/processor.project.index.test.js b/test/e2e/processor.project.index.test.js index 0e89b6c..bff092e 100644 --- a/test/e2e/processor.project.index.test.js +++ b/test/e2e/processor.project.index.test.js @@ -683,7 +683,7 @@ describe('TC Attachment Topic Tests', () => { }) }) -describe('TC Phase Topic Tests', () => { +xdescribe('TC Phase Topic Tests', () => { before(async () => { // runs before all tests in this block await ProcessorService.create(projectCreatedMessage) @@ -738,7 +738,7 @@ describe('TC Phase Topic Tests', () => { }) }) -describe('TC Phase Product Topic Tests', () => { +xdescribe('TC Phase Product Topic Tests', () => { before(async () => { // runs before all tests in this block await ProcessorService.create(projectCreatedMessage) diff --git a/test/e2e/processor.timeline.index.test.js b/test/e2e/processor.timeline.index.test.js index f162aca..33473b5 100644 --- a/test/e2e/processor.timeline.index.test.js +++ b/test/e2e/processor.timeline.index.test.js @@ -620,7 +620,7 @@ describe('TC Timeline And Nested Timeline Topic Tests', () => { }) }) -describe('TC Milestone Topic Tests', () => { +xdescribe('TC Milestone Topic Tests', () => { before(async () => { // runs before all tests in this block await ProcessorService.create(timelineCreatedMessage)