From 80f0e6fd0d6f8e435ecf9aa8938f69b10befe88b Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Wed, 24 Oct 2018 17:14:43 -0700 Subject: [PATCH 01/12] Refactors askVotingPlanStatus templates as transitions --- config/lib/helpers/contentfulEntry.js | 32 ++++++++++++++++++++++----- lib/helpers/contentfulEntry.js | 23 ++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/config/lib/helpers/contentfulEntry.js b/config/lib/helpers/contentfulEntry.js index 1f3c4e47..eda1dcd7 100644 --- a/config/lib/helpers/contentfulEntry.js +++ b/config/lib/helpers/contentfulEntry.js @@ -1,5 +1,7 @@ 'use strict'; +const transitionFieldType = 'transition'; + /** * This maps the fields in our Contentful types into broadcast, topic, and defaultTopicTriggers. * @@ -26,15 +28,30 @@ module.exports = { askVotingPlanStatus: { type: 'askVotingPlanStatus', broadcastable: true, - templates: [ - 'votingPlanStatusCantVote', - 'votingPlanStatusNotVoting', - 'votingPlanStatusVoted', - ], + templates: { + votingPlanStatusCantVote: { + fieldName: 'cantVoteTransition', + fieldType: transitionFieldType, + name: 'votingPlanStatusCantVote', + }, + votingPlanStatusNotVoting: { + fieldName: 'notVotingTransition', + fieldType: transitionFieldType, + name: 'votingPlanStatusNotVoting', + }, + votingPlanStatusVoted: { + fieldName: 'votedTransition', + fieldType: transitionFieldType, + name: 'votingPlanStatusVoted', + }, + }, }, askYesNo: { type: 'askYesNo', broadcastable: true, + // TODO: Refactor templates as an object. We'll want new transition fields, but also need to + // backfill legacy saidYes, saidYesTopic, saidNo, and saidNoTopic fields. + // That or we define a new content type, as we can't easily rename our content type name. templates: [ 'saidYes', 'saidNo', @@ -43,6 +60,7 @@ module.exports = { }, autoReply: { type: 'autoReply', + // TODO: Refactor templates as an object. templates: [ 'autoReply', ], @@ -64,14 +82,16 @@ module.exports = { photoPostConfig: { type: 'photoPostConfig', postType: 'photo', + // TODO: Refactor topic config to define templates here and DRY. }, textPostBroadcast: { type: 'textPostBroadcast', - broadcastable: true, }, textPostConfig: { type: 'textPostConfig', postType: 'text', + broadcastable: true, + // TODO: Refactor topic config to define templates here and DRY. }, // Legacy types: // Ideally we'd backfill all legacy entries as their new types, but we likely can't change the diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index ddd9ebc7..fb879030 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -123,15 +123,32 @@ async function getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEn } /** + * TODO: We need pass a parameter to force clearing cache for all topics. * @param {Object} contentfulEntry * @return {Promise} */ async function getTopicTemplates(contentfulEntry) { const contentType = contentful.getContentTypeFromContentfulEntry(contentfulEntry); - const templateFieldNames = config.contentTypes[contentType].templates; + const templatesConfig = config.contentTypes[contentType].templates; const templates = {}; - if (!contentfulEntry || !templateFieldNames) { + if (!contentfulEntry || !templatesConfig) { + return templates; + } + + if (contentType === config.contentTypes.askVotingPlanStatus.type) { + const templateNames = Object.keys(templatesConfig); + const promises = []; + templateNames.forEach((templateName) => { + const fieldName = templatesConfig[templateName].fieldName; + // TODO: Pass cache clear parameter. + promises.push(module.exports + .getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEntry.fields[fieldName])); + }); + const messageTemplates = await Promise.all(promises); + templateNames.forEach((templateName, index) => { + templates[templateName] = messageTemplates[index]; + }); return templates; } @@ -139,7 +156,7 @@ async function getTopicTemplates(contentfulEntry) { /* eslint-disable */ // TODO: Disabling our linter probably isn't best way to handle this, find topics in parallel // instead of serially per https://github.com/DoSomething/gambit-campaigns/pull/1088/files#r220969795 - for (const fieldName of templateFieldNames) { + for (const fieldName of templatesConfig) { const templateTopicEntry = fields[`${fieldName}Topic`]; templates[fieldName] = { text: fields[fieldName], From ade53b0ec05b40d79931ccb220ef8ceb4cf918e5 Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Wed, 24 Oct 2018 21:03:41 -0700 Subject: [PATCH 02/12] Refactors autoReply templates as object --- config/lib/helpers/contentfulEntry.js | 18 +++++++++------- lib/helpers/contentfulEntry.js | 30 ++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/config/lib/helpers/contentfulEntry.js b/config/lib/helpers/contentfulEntry.js index eda1dcd7..f5cdd3b3 100644 --- a/config/lib/helpers/contentfulEntry.js +++ b/config/lib/helpers/contentfulEntry.js @@ -1,5 +1,6 @@ 'use strict'; +const textFieldType = 'text'; const transitionFieldType = 'transition'; /** @@ -60,10 +61,13 @@ module.exports = { }, autoReply: { type: 'autoReply', - // TODO: Refactor templates as an object. - templates: [ - 'autoReply', - ], + templates: { + autoReply: { + fieldName: 'autoReply', + fieldType: textFieldType, + name: 'autoReply', + }, + }, }, autoReplyBroadcast: { type: 'autoReplyBroadcast', @@ -81,17 +85,17 @@ module.exports = { }, photoPostConfig: { type: 'photoPostConfig', + // TODO: Refactor photoPostConfig in config/lib/helpers/topic here to DRY. postType: 'photo', - // TODO: Refactor topic config to define templates here and DRY. }, textPostBroadcast: { type: 'textPostBroadcast', + broadcastable: true, }, textPostConfig: { type: 'textPostConfig', + // TODO: Move textPostConfig in config/lib/helpers/topic here to DRY. postType: 'text', - broadcastable: true, - // TODO: Refactor topic config to define templates here and DRY. }, // Legacy types: // Ideally we'd backfill all legacy entries as their new types, but we likely can't change the diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index fb879030..2b7ad53a 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -136,18 +136,28 @@ async function getTopicTemplates(contentfulEntry) { return templates; } - if (contentType === config.contentTypes.askVotingPlanStatus.type) { + if (contentType !== config.contentTypes.askYesNo.type) { const templateNames = Object.keys(templatesConfig); const promises = []; templateNames.forEach((templateName) => { const fieldName = templatesConfig[templateName].fieldName; - // TODO: Pass cache clear parameter. - promises.push(module.exports - .getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEntry.fields[fieldName])); + const fieldValue = contentfulEntry.fields[fieldName]; + if (module.exports.isTransitionTemplate(contentType, templateName)) { + // TODO: Pass cache clear parameter. + promises.push(module.exports + .getMessageTemplateFromContentfulEntryAndTemplateName(fieldValue)); + } else { + templates[templateName] = { + text: fieldValue, + topic: {}, + }; + } }); const messageTemplates = await Promise.all(promises); templateNames.forEach((templateName, index) => { - templates[templateName] = messageTemplates[index]; + if (module.exports.isTransitionTemplate(contentType, templateName)) { + templates[templateName] = messageTemplates[index]; + } }); return templates; } @@ -210,6 +220,15 @@ function isMessage(contentfulEntry) { return contentful.isContentType(contentfulEntry, config.contentTypes.message.type); } +/** + * @param {String} contentType + * @param {String} templateName + * @return {Boolean} + */ +function isTransitionTemplate(contentType, templateName) { + return config.contentTypes[contentType].templates[templateName].fieldType === 'transition'; +} + module.exports = { fetch, fetchById, @@ -224,5 +243,6 @@ module.exports = { isDefaultTopicTrigger, isLegacyBroadcast, isMessage, + isTransitionTemplate, parseContentfulEntry, }; From 4ca7e4c2bf4ff513be5b4634d7ff9d2e56499964 Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Wed, 24 Oct 2018 21:43:23 -0700 Subject: [PATCH 03/12] Adds test for isTransitionTemplate --- config/lib/helpers/contentfulEntry.js | 18 ++++++++++++------ lib/helpers/contentfulEntry.js | 5 +++-- test/lib/lib-helpers/contentfulEntry.test.js | 15 +++++++++++++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/config/lib/helpers/contentfulEntry.js b/config/lib/helpers/contentfulEntry.js index f5cdd3b3..7775c696 100644 --- a/config/lib/helpers/contentfulEntry.js +++ b/config/lib/helpers/contentfulEntry.js @@ -1,7 +1,9 @@ 'use strict'; -const textFieldType = 'text'; -const transitionFieldType = 'transition'; +const templateFieldTypes = { + text: 'text', + transition: 'transition', +}; /** * This maps the fields in our Contentful types into broadcast, topic, and defaultTopicTriggers. @@ -30,19 +32,22 @@ module.exports = { type: 'askVotingPlanStatus', broadcastable: true, templates: { + // These template names correspond to the macros that get executed if user matches a trigger + // within the ask_voting_plan_status topic in Gambit Conversations. + // @see https://github.com/DoSomething/gambit-conversations/blob/master/brain/topics/askVotingPlanStatus.rive votingPlanStatusCantVote: { fieldName: 'cantVoteTransition', - fieldType: transitionFieldType, + fieldType: templateFieldTypes.transition, name: 'votingPlanStatusCantVote', }, votingPlanStatusNotVoting: { fieldName: 'notVotingTransition', - fieldType: transitionFieldType, + fieldType: templateFieldTypes.transition, name: 'votingPlanStatusNotVoting', }, votingPlanStatusVoted: { fieldName: 'votedTransition', - fieldType: transitionFieldType, + fieldType: templateFieldTypes.transition, name: 'votingPlanStatusVoted', }, }, @@ -64,7 +69,7 @@ module.exports = { templates: { autoReply: { fieldName: 'autoReply', - fieldType: textFieldType, + fieldType: templateFieldTypes.text, name: 'autoReply', }, }, @@ -111,4 +116,5 @@ module.exports = { broadcastable: true, }, }, + templateFieldTypes, }; diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index 2b7ad53a..5b7914a6 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -123,7 +123,7 @@ async function getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEn } /** - * TODO: We need pass a parameter to force clearing cache for all topics. + * TODO: We need pass a parameter to force clearing cache for all topics within the templates. * @param {Object} contentfulEntry * @return {Promise} */ @@ -226,7 +226,8 @@ function isMessage(contentfulEntry) { * @return {Boolean} */ function isTransitionTemplate(contentType, templateName) { - return config.contentTypes[contentType].templates[templateName].fieldType === 'transition'; + const templatesConfig = config.contentTypes[contentType].templates; + return templatesConfig[templateName].fieldType === config.templateFieldTypes.transition; } module.exports = { diff --git a/test/lib/lib-helpers/contentfulEntry.test.js b/test/lib/lib-helpers/contentfulEntry.test.js index 51c11142..7d7bd656 100644 --- a/test/lib/lib-helpers/contentfulEntry.test.js +++ b/test/lib/lib-helpers/contentfulEntry.test.js @@ -10,6 +10,9 @@ const sinon = require('sinon'); const contentful = require('../../../lib/contentful'); const helpers = require('../../../lib/helpers'); const stubs = require('../../utils/stubs'); + +const config = require('../../../config/lib/helpers/contentfulEntry'); + // Contentful factories const askYesNoEntryFactory = require('../../utils/factories/contentful/askYesNo'); const autoReplyFactory = require('../../utils/factories/contentful/autoReply'); @@ -194,3 +197,15 @@ test('isMessage returns whether content type is message', (t) => { t.truthy(contentfulEntryHelper.isMessage(messageEntry)); t.falsy(contentfulEntryHelper.isMessage(autoReplyEntry)); }); + +// isTransitionTemplate +test('isTransitionTemplate returns whether contentType config for templateName is a transition field', (t) => { + const askVotingPlanStatus = config.contentTypes.askVotingPlanStatus; + const autoReply = config.contentTypes.autoReply; + let templateName = askVotingPlanStatus.templates.votingPlanStatusVoted.name; + t.truthy(contentfulEntryHelper + .isTransitionTemplate(askVotingPlanStatus.type, templateName)); + templateName = autoReply.templates.autoReply.name; + t.falsy(contentfulEntryHelper + .isTransitionTemplate(autoReply.type, templateName)); +}); From e15e8da057b0657e28c3e120e5dbc21202462a1c Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 12:51:12 -0700 Subject: [PATCH 04/12] Renames as getMessageTemplate --- lib/helpers/broadcast.js | 2 +- lib/helpers/campaign.js | 2 +- lib/helpers/contentfulEntry.js | 11 +++++------ test/lib/lib-helpers/broadcast.test.js | 6 +++--- test/lib/lib-helpers/campaign.test.js | 2 +- test/lib/lib-helpers/contentfulEntry.test.js | 14 +++++++------- 6 files changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/helpers/broadcast.js b/lib/helpers/broadcast.js index 76e819ea..6778324e 100644 --- a/lib/helpers/broadcast.js +++ b/lib/helpers/broadcast.js @@ -75,7 +75,7 @@ async function parseBroadcastFromContentfulEntry(contentfulEntry) { const [message, templates] = await Promise.all([ // Parse the outbound broadcast message to send. helpers.contentfulEntry - .getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEntry, data.type), + .getMessageTemplate(contentfulEntry, data.type), // Parse topic templates if they exist. helpers.contentfulEntry.getTopicTemplates(contentfulEntry), ]); diff --git a/lib/helpers/campaign.js b/lib/helpers/campaign.js index 99411f6f..1759e859 100644 --- a/lib/helpers/campaign.js +++ b/lib/helpers/campaign.js @@ -96,7 +96,7 @@ async function parseCampaignConfig(contentfulEntry) { } data.templates.webSignup = await helpers.contentfulEntry - .getMessageTemplateFromContentfulEntryAndTemplateName(webSignupEntry, 'webSignup'); + .getMessageTemplate(webSignupEntry, 'webSignup'); return data; } diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index 5b7914a6..2e2443fe 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -104,10 +104,10 @@ function getSummaryFromContentfulEntry(contentfulEntry) { /** * @param {Object} contentfulEntry - * @param {String} templateName + * @param {String} templateName - optional * @return {Promise} */ -async function getMessageTemplateFromContentfulEntryAndTemplateName(contentfulEntry, templateName) { +async function getMessageTemplate(contentfulEntry, templateName) { if (!contentfulEntry || !contentfulEntry.fields) { return {}; } @@ -143,9 +143,8 @@ async function getTopicTemplates(contentfulEntry) { const fieldName = templatesConfig[templateName].fieldName; const fieldValue = contentfulEntry.fields[fieldName]; if (module.exports.isTransitionTemplate(contentType, templateName)) { - // TODO: Pass cache clear parameter. - promises.push(module.exports - .getMessageTemplateFromContentfulEntryAndTemplateName(fieldValue)); + // TODO: Pass cache clear parameter to refresh the transition topic. + promises.push(module.exports.getMessageTemplate(fieldValue)); } else { templates[templateName] = { text: fieldValue, @@ -236,7 +235,7 @@ module.exports = { formatForApi, formatSys, getContentTypeConfigs, - getMessageTemplateFromContentfulEntryAndTemplateName, + getMessageTemplate, getSummaryFromContentfulEntry, getTopicTemplates, isAutoReply, diff --git a/test/lib/lib-helpers/broadcast.test.js b/test/lib/lib-helpers/broadcast.test.js index 852c7d2f..e1404be4 100644 --- a/test/lib/lib-helpers/broadcast.test.js +++ b/test/lib/lib-helpers/broadcast.test.js @@ -123,13 +123,13 @@ test('getById returns fetchById if resetCache arg is true', async () => { }); // parseBroadcastFromContentfulEntry -test('parseBroadcastFromContentfulEntry returns object with message from getMessageTemplateFromContentfulEntryAndTemplateName', async () => { +test('parseBroadcastFromContentfulEntry returns object with message from getMessageTemplate', async () => { const contentfulEntry = autoReplyBroadcastFactory.getValidAutoReplyBroadcast(); const stubContentType = stubs.getRandomWord(); sandbox.stub(helpers.contentfulEntry, 'getSummaryFromContentfulEntry') .returns({ type: stubContentType }); const stubTemplate = { text: stubs.getRandomMessageText(), template: stubContentType }; - sandbox.stub(helpers.contentfulEntry, 'getMessageTemplateFromContentfulEntryAndTemplateName') + sandbox.stub(helpers.contentfulEntry, 'getMessageTemplate') .returns(stubTemplate); const result = await broadcastHelper.parseBroadcastFromContentfulEntry(contentfulEntry); @@ -137,7 +137,7 @@ test('parseBroadcastFromContentfulEntry returns object with message from getMess .should.have.been.calledWith(contentfulEntry); result.message.text.should.equal(stubTemplate.text); result.message.template.should.equal(stubContentType); - helpers.contentfulEntry.getMessageTemplateFromContentfulEntryAndTemplateName + helpers.contentfulEntry.getMessageTemplate .should.have.been.calledWith(contentfulEntry, stubContentType); }); diff --git a/test/lib/lib-helpers/campaign.test.js b/test/lib/lib-helpers/campaign.test.js index 7963d0cf..7bf084c2 100644 --- a/test/lib/lib-helpers/campaign.test.js +++ b/test/lib/lib-helpers/campaign.test.js @@ -185,7 +185,7 @@ test('parseCampaignConfig should return empty object if contentfulEntry undefine test('parseCampaignConfig should return object with id and templates properties', async () => { const stubTemplate = { text: stubs.getRandomMessageText() }; - sandbox.stub(helpers.contentfulEntry, 'getMessageTemplateFromContentfulEntryAndTemplateName') + sandbox.stub(helpers.contentfulEntry, 'getMessageTemplate') .returns(Promise.resolve(stubTemplate)); const result = await campaignHelper.parseCampaignConfig(campaignConfigEntry); result.id.should.equal(campaignConfigEntry.sys.id); diff --git a/test/lib/lib-helpers/contentfulEntry.test.js b/test/lib/lib-helpers/contentfulEntry.test.js index 7d7bd656..4c7b887a 100644 --- a/test/lib/lib-helpers/contentfulEntry.test.js +++ b/test/lib/lib-helpers/contentfulEntry.test.js @@ -98,8 +98,8 @@ test('formatSys returns object with properties from contentfulEntry.sys', () => }); }); -// getMessageTemplateFromContentfulEntryAndTemplateName -test('getMessageTemplateFromContentfulEntryAndTemplateName should call topic.getById to set topic if topic reference field saved', async () => { +// getMessageTemplate +test('getMessageTemplate should call topic.getById to set topic if topic reference field saved', async () => { const messageTemplate = stubs.getRandomWord(); const messageText = stubs.getRandomMessageText(); const messageAttachments = [{ name: 'Tyrion' }, { name: 'Cersei' }]; @@ -111,7 +111,7 @@ test('getMessageTemplateFromContentfulEntryAndTemplateName should call topic.get .returns(fetchTopicResult); const result = await contentfulEntryHelper - .getMessageTemplateFromContentfulEntryAndTemplateName(autoReplyBroadcastEntry, messageTemplate); + .getMessageTemplate(autoReplyBroadcastEntry, messageTemplate); helpers.topic.getById.should.have.been.calledWith(autoReplyBroadcastEntry.fields.topic.sys.id); result.text.should.equal(messageText); result.attachments.should.deep.equal(messageAttachments); @@ -119,18 +119,18 @@ test('getMessageTemplateFromContentfulEntryAndTemplateName should call topic.get result.template.should.equal(messageTemplate); }); -test('getMessageTemplateFromContentfulEntryAndTemplateName result should be empty object if contentfulEntry undefined', async () => { +test('getMessageTemplate result should be empty object if contentfulEntry undefined', async () => { const result = await contentfulEntryHelper - .getMessageTemplateFromContentfulEntryAndTemplateName(null, stubs.getRandomWord()); + .getMessageTemplate(null, stubs.getRandomWord()); result.should.deep.equal({}); }); -test('getMessageTemplateFromContentfulEntryAndTemplateName should not call topic.getById if topic reference field undefined', async () => { +test('getMessageTemplate should not call topic.getById if topic reference field undefined', async () => { sandbox.stub(helpers.topic, 'getById') .returns(fetchTopicResult); const result = await contentfulEntryHelper - .getMessageTemplateFromContentfulEntryAndTemplateName(askYesNoEntry, stubs.getRandomWord()); + .getMessageTemplate(askYesNoEntry, stubs.getRandomWord()); helpers.topic.getById.should.not.have.been.called; result.topic.should.deep.equal({}); }); From 2a4ad2ad21fcf7d3a1e61e8d6aacb76795949f0a Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 11:33:23 -0700 Subject: [PATCH 05/12] Refactors askYesNo with transition fields --- config/lib/helpers/contentfulEntry.js | 25 +++++--- lib/helpers/contentfulEntry.js | 86 ++++++++++++++++----------- 2 files changed, 67 insertions(+), 44 deletions(-) diff --git a/config/lib/helpers/contentfulEntry.js b/config/lib/helpers/contentfulEntry.js index 7775c696..b7c14ed7 100644 --- a/config/lib/helpers/contentfulEntry.js +++ b/config/lib/helpers/contentfulEntry.js @@ -55,14 +55,23 @@ module.exports = { askYesNo: { type: 'askYesNo', broadcastable: true, - // TODO: Refactor templates as an object. We'll want new transition fields, but also need to - // backfill legacy saidYes, saidYesTopic, saidNo, and saidNoTopic fields. - // That or we define a new content type, as we can't easily rename our content type name. - templates: [ - 'saidYes', - 'saidNo', - 'invalidAskYesNoResponse', - ], + templates: { + invalidAskYesNoResponse: { + fieldName: 'invalidAskYesNoResponse', + fieldType: templateFieldTypes.text, + name: 'invalidAskYesNoResponse', + }, + saidNo: { + fieldName: 'noTransition', + fieldType: templateFieldTypes.transition, + name: 'saidNo', + }, + saidYes: { + fieldName: 'yesTransition', + fieldType: templateFieldTypes.transition, + name: 'saidYes', + }, + }, }, autoReply: { type: 'autoReply', diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index 2e2443fe..b8d1565e 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -136,48 +136,61 @@ async function getTopicTemplates(contentfulEntry) { return templates; } - if (contentType !== config.contentTypes.askYesNo.type) { - const templateNames = Object.keys(templatesConfig); - const promises = []; - templateNames.forEach((templateName) => { - const fieldName = templatesConfig[templateName].fieldName; - const fieldValue = contentfulEntry.fields[fieldName]; - if (module.exports.isTransitionTemplate(contentType, templateName)) { - // TODO: Pass cache clear parameter to refresh the transition topic. - promises.push(module.exports.getMessageTemplate(fieldValue)); - } else { - templates[templateName] = { - text: fieldValue, - topic: {}, - }; - } - }); - const messageTemplates = await Promise.all(promises); - templateNames.forEach((templateName, index) => { - if (module.exports.isTransitionTemplate(contentType, templateName)) { - templates[templateName] = messageTemplates[index]; - } - }); - return templates; - } + const templateNames = Object.keys(templatesConfig); + const promises = []; + templateNames.forEach((templateName) => { + const fieldName = templatesConfig[templateName].fieldName; + const fieldValue = contentfulEntry.fields[fieldName]; + if (module.exports.isTransitionTemplate(contentType, templateName)) { + // TODO: Pass cache clear parameter. + promises.push(module.exports.getMessageTemplate(fieldValue)); + } else { + templates[templateName] = { + text: fieldValue, + topic: {}, + }; + } + }); + const messageTemplates = await Promise.all(promises); + templateNames.forEach((templateName, index) => { + if (module.exports.isTransitionTemplate(contentType, templateName)) { + templates[templateName] = messageTemplates[index]; + } + }); - const fields = contentfulEntry.fields; - /* eslint-disable */ - // TODO: Disabling our linter probably isn't best way to handle this, find topics in parallel - // instead of serially per https://github.com/DoSomething/gambit-campaigns/pull/1088/files#r220969795 - for (const fieldName of templatesConfig) { - const templateTopicEntry = fields[`${fieldName}Topic`]; - templates[fieldName] = { - text: fields[fieldName], - topic: templateTopicEntry ? await helpers.topic - .getById(contentful.getContentfulIdFromContentfulEntry(templateTopicEntry)) : {}, - }; + // The first iteration of askYesNo did not use transition fields for saidYes, saidNo templates. + // it had separate text and topic fields per template. This backfills entries created during that + // first iteration -- ideally we could run a migration to backfill with transition references and + // remove this code once complete. + if (module.exports.isAskYesNo(contentfulEntry)) { + const fields = contentfulEntry.fields; + if (!templates.saidNo || !templates.saidNo.text) { + const saidNoTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidNoTopic); + templates.saidNo = { + text: fields.saidNo, + topic: saidNoTopicId ? await helpers.topic.getById(saidNoTopicId) : {}, + }; + } + if (!templates.saidYes || !templates.saidNo.text) { + const saidYesTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidYesTopic); + templates.saidYes = { + text: fields.saidYes, + topic: saidYesTopicId ? await helpers.topic.getById(saidYesTopicId) : {}, + }; + } } - /* eslint-enable */ return templates; } +/** + * @param {Object} contentfulEntry + * @return {Boolean} + */ +function isAskYesNo(contentfulEntry) { + return contentful.isContentType(contentfulEntry, config.contentTypes.askYesNo.type); +} + /** * @param {Object} contentfulEntry * @return {Boolean} @@ -238,6 +251,7 @@ module.exports = { getMessageTemplate, getSummaryFromContentfulEntry, getTopicTemplates, + isAskYesNo, isAutoReply, isBroadcastable, isDefaultTopicTrigger, From a6ef0e7c9c157e3eee6152b10a7da8aefff9cb65 Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 11:37:21 -0700 Subject: [PATCH 06/12] Adds test for isAskYesNo --- test/lib/lib-helpers/contentfulEntry.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/lib/lib-helpers/contentfulEntry.test.js b/test/lib/lib-helpers/contentfulEntry.test.js index 4c7b887a..134a117b 100644 --- a/test/lib/lib-helpers/contentfulEntry.test.js +++ b/test/lib/lib-helpers/contentfulEntry.test.js @@ -174,6 +174,12 @@ test('getTopicTemplates should call topic.getById to set saidYes and saidNo topi result.saidNo.topic.should.deep.equal(fetchTopicResult); }); +// isAskYesNo +test('isAskYesNo returns whether content type is askYesNo', (t) => { + t.truthy(contentfulEntryHelper.isAskYesNo(askYesNoEntry)); + t.falsy(contentfulEntryHelper.isAskYesNo(autoReplyEntry)); +}); + // isAutoReply test('isAutoReply returns whether content type is autoReply', (t) => { t.falsy(contentfulEntryHelper.isAutoReply(askYesNoEntry)); From 57da47862081a48c9eea05679b9d07690768df30 Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 12:30:09 -0700 Subject: [PATCH 07/12] Cleanup spacing --- lib/helpers/contentfulEntry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index b8d1565e..e1b391d0 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -158,7 +158,7 @@ async function getTopicTemplates(contentfulEntry) { } }); - // The first iteration of askYesNo did not use transition fields for saidYes, saidNo templates. + // The first iteration of askYesNo did not use transition fields for saidYes and saidNo templates; // it had separate text and topic fields per template. This backfills entries created during that // first iteration -- ideally we could run a migration to backfill with transition references and // remove this code once complete. From cc57021ff991f2a38d367d1b83527cb140d89da7 Mon Sep 17 00:00:00 2001 From: Rafael Date: Thu, 25 Oct 2018 14:07:01 -0700 Subject: [PATCH 08/12] :eagle: :eyes: Co-Authored-By: aaronschachter --- lib/helpers/contentfulEntry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index e1b391d0..6ece406e 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -171,7 +171,7 @@ async function getTopicTemplates(contentfulEntry) { topic: saidNoTopicId ? await helpers.topic.getById(saidNoTopicId) : {}, }; } - if (!templates.saidYes || !templates.saidNo.text) { + if (!templates.saidYes || !templates.saidYes.text) { const saidYesTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidYesTopic); templates.saidYes = { text: fields.saidYes, From b71764b921e7b80398c87823c29755d381000944 Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 19:14:51 -0700 Subject: [PATCH 09/12] Fixes https://github.com/DoSomething/gambit-campaigns/pull/1092#issuecomment-433218411 --- lib/helpers/contentfulEntry.js | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index 6ece406e..dd29d6b4 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -141,21 +141,16 @@ async function getTopicTemplates(contentfulEntry) { templateNames.forEach((templateName) => { const fieldName = templatesConfig[templateName].fieldName; const fieldValue = contentfulEntry.fields[fieldName]; - if (module.exports.isTransitionTemplate(contentType, templateName)) { - // TODO: Pass cache clear parameter. - promises.push(module.exports.getMessageTemplate(fieldValue)); - } else { - templates[templateName] = { - text: fieldValue, - topic: {}, - }; - } + const isTransition = module.exports.isTransitionTemplate(contentType, templateName); + const promise = isTransition ? module.exports.getMessageTemplate(fieldValue) : Promise.resolve({ + text: fieldValue, + topic: {}, + }); + promises.push(promise); }); const messageTemplates = await Promise.all(promises); templateNames.forEach((templateName, index) => { - if (module.exports.isTransitionTemplate(contentType, templateName)) { - templates[templateName] = messageTemplates[index]; - } + templates[templateName] = messageTemplates[index]; }); // The first iteration of askYesNo did not use transition fields for saidYes and saidNo templates; From 1a0943f3c7ed83bd965c59ff2ddbde0eb0ab47eb Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 19:20:11 -0700 Subject: [PATCH 10/12] Cleanup --- lib/helpers/contentfulEntry.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index dd29d6b4..26fc215e 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -137,16 +137,13 @@ async function getTopicTemplates(contentfulEntry) { } const templateNames = Object.keys(templatesConfig); - const promises = []; - templateNames.forEach((templateName) => { - const fieldName = templatesConfig[templateName].fieldName; - const fieldValue = contentfulEntry.fields[fieldName]; + const promises = templateNames.map((templateName) => { + const fieldValue = contentfulEntry.fields[templatesConfig[templateName].fieldName]; const isTransition = module.exports.isTransitionTemplate(contentType, templateName); - const promise = isTransition ? module.exports.getMessageTemplate(fieldValue) : Promise.resolve({ + return isTransition ? module.exports.getMessageTemplate(fieldValue) : Promise.resolve({ text: fieldValue, topic: {}, }); - promises.push(promise); }); const messageTemplates = await Promise.all(promises); templateNames.forEach((templateName, index) => { From 04b14a43ab85d66a78503f5fc13df283fc5244ef Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Thu, 25 Oct 2018 19:25:35 -0700 Subject: [PATCH 11/12] Adds saidYes before saidNo --- config/lib/helpers/contentfulEntry.js | 10 +++++----- lib/helpers/contentfulEntry.js | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/config/lib/helpers/contentfulEntry.js b/config/lib/helpers/contentfulEntry.js index b7c14ed7..14544920 100644 --- a/config/lib/helpers/contentfulEntry.js +++ b/config/lib/helpers/contentfulEntry.js @@ -61,16 +61,16 @@ module.exports = { fieldType: templateFieldTypes.text, name: 'invalidAskYesNoResponse', }, - saidNo: { - fieldName: 'noTransition', - fieldType: templateFieldTypes.transition, - name: 'saidNo', - }, saidYes: { fieldName: 'yesTransition', fieldType: templateFieldTypes.transition, name: 'saidYes', }, + saidNo: { + fieldName: 'noTransition', + fieldType: templateFieldTypes.transition, + name: 'saidNo', + }, }, }, autoReply: { diff --git a/lib/helpers/contentfulEntry.js b/lib/helpers/contentfulEntry.js index 26fc215e..7522b653 100644 --- a/lib/helpers/contentfulEntry.js +++ b/lib/helpers/contentfulEntry.js @@ -151,18 +151,11 @@ async function getTopicTemplates(contentfulEntry) { }); // The first iteration of askYesNo did not use transition fields for saidYes and saidNo templates; - // it had separate text and topic fields per template. This backfills entries created during that - // first iteration -- ideally we could run a migration to backfill with transition references and - // remove this code once complete. + // it had separate text and topic fields per template. This code block supports entries created + // during that first iteration -- ideally we could run a migration to backfill these entries with + // transition references created from the respective text and topic fields. if (module.exports.isAskYesNo(contentfulEntry)) { const fields = contentfulEntry.fields; - if (!templates.saidNo || !templates.saidNo.text) { - const saidNoTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidNoTopic); - templates.saidNo = { - text: fields.saidNo, - topic: saidNoTopicId ? await helpers.topic.getById(saidNoTopicId) : {}, - }; - } if (!templates.saidYes || !templates.saidYes.text) { const saidYesTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidYesTopic); templates.saidYes = { @@ -170,6 +163,13 @@ async function getTopicTemplates(contentfulEntry) { topic: saidYesTopicId ? await helpers.topic.getById(saidYesTopicId) : {}, }; } + if (!templates.saidNo || !templates.saidNo.text) { + const saidNoTopicId = contentful.getContentfulIdFromContentfulEntry(fields.saidNoTopic); + templates.saidNo = { + text: fields.saidNo, + topic: saidNoTopicId ? await helpers.topic.getById(saidNoTopicId) : {}, + }; + } } return templates; From 2f7192a44f3b2a86b3578e86ec0af64d08b457ab Mon Sep 17 00:00:00 2001 From: Aaron Schachter Date: Fri, 26 Oct 2018 09:24:34 -0700 Subject: [PATCH 12/12] Bumped version number to 5.15.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c9636692..2e188526 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gambit-campaigns", - "version": "5.14.3", + "version": "5.15.0", "description": "The DoSomething.org chatbot service for campaigns and their activity.", "license": "MIT", "repository": {