From 14c441e6db182b71ac07e850304c4d011739295e Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Wed, 4 Aug 2021 22:07:53 +0800 Subject: [PATCH 1/4] fix: ACNA-1232 - app run & deploy messages should differentiate between web action and non-web action URLs --- src/commands/app/deploy.js | 21 +++++++++++++++++---- src/lib/app-helper.js | 15 +++++++++++++++ src/lib/deploy-actions.js | 19 ++++++++++++++++--- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/commands/app/deploy.js b/src/commands/app/deploy.js index d3d4d3b3..5d181bc6 100644 --- a/src/commands/app/deploy.js +++ b/src/commands/app/deploy.js @@ -19,7 +19,7 @@ const BaseCommand = require('../../BaseCommand') const BuildCommand = require('./build') const webLib = require('@adobe/aio-lib-web') const { flags } = require('@oclif/command') -const { runScript, buildExtensionPointPayloadWoMetadata, buildExcShellViewExtensionMetadata } = require('../../lib/app-helper') +const { createWebExportFilter, runScript, buildExtensionPointPayloadWoMetadata, buildExcShellViewExtensionMetadata } = require('../../lib/app-helper') const rtLib = require('@adobe/aio-lib-runtime') class Deploy extends BuildCommand { @@ -154,9 +154,22 @@ class Deploy extends BuildCommand { // log deployed resources if (deployedRuntimeEntities.actions) { this.log(chalk.blue(chalk.bold('Your deployed actions:'))) - deployedRuntimeEntities.actions.forEach(a => { - this.log(chalk.blue(chalk.bold(` -> ${a.url || a.name} `))) - }) + const web = deployedRuntimeEntities.actions.filter(createWebExportFilter(true)) + const nonWeb = deployedRuntimeEntities.actions.filter(createWebExportFilter(false)) + + if (web.length > 0) { + this.log('web actions:') + web.forEach(a => { + this.log(chalk.blue(chalk.bold(` -> ${a.url || a.name} `))) + }) + } + + if (nonWeb.length > 0) { + this.log('non-web actions:') + nonWeb.forEach(a => { + this.log(chalk.blue(chalk.bold(` -> ${a.url || a.name} `))) + }) + } } // TODO urls should depend on extension point, exc shell only for exc shell extension point - use a post-app-deploy hook ? if (deployedFrontendUrl) { diff --git a/src/lib/app-helper.js b/src/lib/app-helper.js index dae70f4e..364d0593 100644 --- a/src/lib/app-helper.js +++ b/src/lib/app-helper.js @@ -483,7 +483,22 @@ function deleteUserConfig (configData) { fs.writeFileSync(configData.file, yaml.safeDump(phyConfig)) } +/** @private */ +const createWebExportFilter = (filterValue) => { + return (action) => { + if (!action && !action.body && !action.body.annotations) { + return false + } + + const weAnnotation = action.body.annotations.filter(a => a.key === 'web-export') + const weValue = (weAnnotation.length > 0) ? !!(weAnnotation[0].value) : false + + return (filterValue === weValue) + } +} + module.exports = { + createWebExportFilter, isNpmInstalled, isGitInstalled, installPackages, diff --git a/src/lib/deploy-actions.js b/src/lib/deploy-actions.js index 233360ab..5fb444d6 100644 --- a/src/lib/deploy-actions.js +++ b/src/lib/deploy-actions.js @@ -27,9 +27,22 @@ module.exports = async (config, isLocal = false, log = () => {}) => { if (!script) { const entities = await deployActions(config, { isLocalDev: isLocal }, log) if (entities.actions) { - entities.actions.forEach(a => { - log(` -> ${a.url || a.name}`) - }) + const web = entities.actions.filter(utils.createWebExportFilter(true)) + const nonWeb = entities.actions.filter(utils.createWebExportFilter(false)) + + if (web.length > 0) { + log('web actions:') + web.forEach(a => { + log(` -> ${a.url || a.name}`) + }) + } + + if (nonWeb.length > 0) { + log('non-web actions:') + nonWeb.forEach(a => { + log(` -> ${a.url || a.name}`) + }) + } } } utils.runScript(config.hooks['post-app-deploy']) From 038a24d67c0ca2250c3a268c6086e5a620ee5c8f Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Wed, 4 Aug 2021 22:08:10 +0800 Subject: [PATCH 2/4] partial fix: unit tests --- test/commands/app/deploy.test.js | 2 ++ test/commands/lib/deploy-actions.test.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/test/commands/app/deploy.test.js b/test/commands/app/deploy.test.js index 62a80f9d..df89d295 100644 --- a/test/commands/app/deploy.test.js +++ b/test/commands/app/deploy.test.js @@ -100,9 +100,11 @@ beforeEach(() => { helpers.runScript.mockReset() helpers.buildExtensionPointPayloadWoMetadata.mockReset() helpers.buildExcShellViewExtensionMetadata.mockReset() + helpers.createWebExportFilter.mockReset() jest.restoreAllMocks() helpers.wrapError.mockImplementation(msg => msg) + helpers.createWebExportFilter.mockImplementation(value => (() => value)) }) test('exports', async () => { diff --git a/test/commands/lib/deploy-actions.test.js b/test/commands/lib/deploy-actions.test.js index 40ebef0e..4078bd96 100644 --- a/test/commands/lib/deploy-actions.test.js +++ b/test/commands/lib/deploy-actions.test.js @@ -18,8 +18,12 @@ jest.mock('../../../src/lib/app-helper') beforeEach(() => { utils.runScript.mockReset() + utils.createWebExportFilter.mockReset() + rtDeployActions.mockReset() rtDeployActions.mockImplementation(() => ({})) + + utils.createWebExportFilter.mockImplementation(value => (() => true)) }) test('exports', () => { From 9376e451cfbe99698683ded1ad4958c9bbcecdf8 Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Wed, 4 Aug 2021 23:47:51 +0800 Subject: [PATCH 3/4] fix: unit tests and linter issues --- src/commands/app/deploy.js | 4 +- src/lib/app-helper.js | 2 +- test/commands/app/deploy.test.js | 40 ++++++++++-- test/commands/lib/app-helper.test.js | 82 ++++++++++++++++++++++++ test/commands/lib/deploy-actions.test.js | 44 +++++++++++-- 5 files changed, 160 insertions(+), 12 deletions(-) diff --git a/src/commands/app/deploy.js b/src/commands/app/deploy.js index 5d181bc6..126b06f5 100644 --- a/src/commands/app/deploy.js +++ b/src/commands/app/deploy.js @@ -156,14 +156,14 @@ class Deploy extends BuildCommand { this.log(chalk.blue(chalk.bold('Your deployed actions:'))) const web = deployedRuntimeEntities.actions.filter(createWebExportFilter(true)) const nonWeb = deployedRuntimeEntities.actions.filter(createWebExportFilter(false)) - + if (web.length > 0) { this.log('web actions:') web.forEach(a => { this.log(chalk.blue(chalk.bold(` -> ${a.url || a.name} `))) }) } - + if (nonWeb.length > 0) { this.log('non-web actions:') nonWeb.forEach(a => { diff --git a/src/lib/app-helper.js b/src/lib/app-helper.js index 364d0593..1ecf634f 100644 --- a/src/lib/app-helper.js +++ b/src/lib/app-helper.js @@ -486,7 +486,7 @@ function deleteUserConfig (configData) { /** @private */ const createWebExportFilter = (filterValue) => { return (action) => { - if (!action && !action.body && !action.body.annotations) { + if (!action || !action.body || !action.body.annotations) { return false } diff --git a/test/commands/app/deploy.test.js b/test/commands/app/deploy.test.js index df89d295..3cb3e3dc 100644 --- a/test/commands/app/deploy.test.js +++ b/test/commands/app/deploy.test.js @@ -14,6 +14,7 @@ const TheCommand = require('../../../src/commands/app/deploy') const BaseCommand = require('../../../src/BaseCommand') const cloneDeep = require('lodash.clonedeep') const dataMocks = require('../../data-mocks/config-loader') +const helpersActual = jest.requireActual('../../../src/lib/app-helper.js') const mockBundleFunc = jest.fn() @@ -43,6 +44,17 @@ jest.mock('../../../src/lib/config-loader', () => { jest.mock('cli-ux') const { cli } = require('cli-ux') +const createWebExportAnnotation = (value) => ({ + body: { + annotations: [ + { + key: 'web-export', + value + } + ] + } +}) + const createAppConfig = (aioConfig = {}, appFixtureName = 'legacy-app') => { const appConfig = dataMocks(appFixtureName, aioConfig).all // change this, dataMocks allows to inject custom configuration @@ -101,10 +113,12 @@ beforeEach(() => { helpers.buildExtensionPointPayloadWoMetadata.mockReset() helpers.buildExcShellViewExtensionMetadata.mockReset() helpers.createWebExportFilter.mockReset() + helpers.createWebExportFilter.mockReset() + jest.restoreAllMocks() helpers.wrapError.mockImplementation(msg => msg) - helpers.createWebExportFilter.mockImplementation(value => (() => value)) + helpers.createWebExportFilter.mockImplementation(filterValue => helpersActual.createWebExportFilter(filterValue)) }) test('exports', async () => { @@ -488,18 +502,36 @@ describe('run', () => { expect(cli.open).toHaveBeenCalledWith('http://prefix?fake=https://example.com') }) - test('deploy should show action urls', async () => { + test('deploy should show action urls (web-export: true)', async () => { + command.getAppExtConfigs.mockReturnValueOnce(createAppConfig(command.appConfig)) + mockRuntimeLib.deployActions.mockResolvedValue({ + actions: [ + { name: 'pkg/action', url: 'https://fake.com/action', ...createWebExportAnnotation(true) }, + { name: 'pkg/actionNoUrl', ...createWebExportAnnotation(true) } + ] + }) + + command.argv = [] + await command.run() + expect(command.error).toHaveBeenCalledTimes(0) + expect(command.log).toHaveBeenCalledWith(expect.stringContaining('web actions:')) + expect(command.log).toHaveBeenCalledWith(expect.stringContaining('https://fake.com/action')) + expect(command.log).toHaveBeenCalledWith(expect.stringContaining('pkg/actionNoUrl')) + }) + + test('deploy should show action urls (web-export: false)', async () => { command.getAppExtConfigs.mockReturnValueOnce(createAppConfig(command.appConfig)) mockRuntimeLib.deployActions.mockResolvedValue({ actions: [ - { name: 'pkg/action', url: 'https://fake.com/action' }, - { name: 'pkg/actionNoUrl' } + { name: 'pkg/action', url: 'https://fake.com/action', ...createWebExportAnnotation(false) }, + { name: 'pkg/actionNoUrl', ...createWebExportAnnotation(false) } ] }) command.argv = [] await command.run() expect(command.error).toHaveBeenCalledTimes(0) + expect(command.log).toHaveBeenCalledWith(expect.stringContaining('non-web actions:')) expect(command.log).toHaveBeenCalledWith(expect.stringContaining('https://fake.com/action')) expect(command.log).toHaveBeenCalledWith(expect.stringContaining('pkg/actionNoUrl')) }) diff --git a/test/commands/lib/app-helper.test.js b/test/commands/lib/app-helper.test.js index e753fd9c..99c9ca71 100644 --- a/test/commands/lib/app-helper.test.js +++ b/test/commands/lib/app-helper.test.js @@ -778,3 +778,85 @@ describe('getCliInfo', () => { ) }) }) + +describe('createWebExportFilter', () => { + const webFilter = appHelper.createWebExportFilter(true) + const nonWebFilter = appHelper.createWebExportFilter(false) + + test('no web-export annotation', () => { + const action = { + name: 'abcde', url: 'https://fake.site', body: { annotations: [] } + } + + expect(webFilter(action)).toEqual(false) + expect(nonWebFilter(action)).toEqual(true) + }) + + test('web-export:(true or truthy) annotation', () => { + const action1 = { + name: 'abcde', + url: 'https://fake.site', + body: { + annotations: [ + { + key: 'web-export', + value: true + } + ] + } + } + + expect(webFilter(action1)).toEqual(true) + expect(nonWebFilter(action1)).toEqual(false) + + const action2 = { + name: 'abcde', + url: 'https://fake.site', + body: { + annotations: [ + { + key: 'web-export', + value: 1 + } + ] + } + } + + expect(webFilter(action2)).toEqual(true) + expect(nonWebFilter(action2)).toEqual(false) + }) + + test('web-export:(false or falsy) annotation', () => { + const action1 = { + name: 'abcde', + url: 'https://fake.site', + body: { + annotations: [ + { + key: 'web-export', + value: false + } + ] + } + } + + expect(webFilter(action1)).toEqual(false) + expect(nonWebFilter(action1)).toEqual(true) + + const action2 = { + name: 'abcde', + url: 'https://fake.site', + body: { + annotations: [ + { + key: 'web-export', + value: null + } + ] + } + } + + expect(webFilter(action2)).toEqual(false) + expect(nonWebFilter(action2)).toEqual(true) + }) +}) diff --git a/test/commands/lib/deploy-actions.test.js b/test/commands/lib/deploy-actions.test.js index 4078bd96..a6dd7b45 100644 --- a/test/commands/lib/deploy-actions.test.js +++ b/test/commands/lib/deploy-actions.test.js @@ -13,17 +13,29 @@ governing permissions and limitations under the License. const deployActions = require('../../../src/lib/deploy-actions') const utils = require('../../../src/lib/app-helper') const { deployActions: rtDeployActions } = require('@adobe/aio-lib-runtime') +const appHelperActual = jest.requireActual('../../../src/lib/app-helper') jest.mock('../../../src/lib/app-helper') +const createWebExportAnnotation = (value) => ({ + body: { + annotations: [ + { + key: 'web-export', + value + } + ] + } +}) + beforeEach(() => { utils.runScript.mockReset() utils.createWebExportFilter.mockReset() rtDeployActions.mockReset() rtDeployActions.mockImplementation(() => ({})) - - utils.createWebExportFilter.mockImplementation(value => (() => true)) + + utils.createWebExportFilter.mockImplementation(filterValue => appHelperActual.createWebExportFilter(filterValue)) }) test('exports', () => { @@ -74,16 +86,38 @@ test('use default parameters (coverage)', async () => { expect(utils.runScript).toHaveBeenNthCalledWith(3, undefined) // post-app-deploy }) -test('should log actions url or name when actions are deployed', async () => { +test('should log actions url or name when actions are deployed (web-export: true)', async () => { rtDeployActions.mockResolvedValue({ actions: [ - { name: 'pkg/action', url: 'https://fake.com/action' }, - { name: 'pkg/actionNoUrl' } + { name: 'pkg/action', url: 'https://fake.com/action', ...createWebExportAnnotation(true) }, + { name: 'pkg/actionNoUrl', ...createWebExportAnnotation(true) } + ] + }) + const log = jest.fn() + await deployActions({ hooks: {} }, false, log) + + expect(log).toHaveBeenCalledWith(expect.stringContaining('web actions:')) + expect(log).toHaveBeenCalledWith(expect.stringContaining('https://fake.com/action')) + expect(log).toHaveBeenCalledWith(expect.stringContaining('pkg/actionNoUrl')) +}) + +test('should log actions url or name when actions are deployed (web-export: false)', async () => { + rtDeployActions.mockResolvedValue({ + actions: [ + { name: 'pkg/action', url: 'https://fake.com/action', ...createWebExportAnnotation(false) }, + { name: 'pkg/actionNoUrl', ...createWebExportAnnotation(false) } ] }) const log = jest.fn() await deployActions({ hooks: {} }, false, log) + expect(log).toHaveBeenCalledWith(expect.stringContaining('non-web actions:')) expect(log).toHaveBeenCalledWith(expect.stringContaining('https://fake.com/action')) expect(log).toHaveBeenCalledWith(expect.stringContaining('pkg/actionNoUrl')) + + log.mockReset() + await deployActions({ hooks: {} }, false) // empty logger + expect(log).not.toHaveBeenCalledWith(expect.stringContaining('non-web actions:')) + expect(log).not.toHaveBeenCalledWith(expect.stringContaining('https://fake.com/action')) + expect(log).not.toHaveBeenCalledWith(expect.stringContaining('pkg/actionNoUrl')) }) From 2b0c585075d9229e576f58f8c511f133e635276e Mon Sep 17 00:00:00 2001 From: Shazron Abdullah Date: Wed, 4 Aug 2021 23:57:43 +0800 Subject: [PATCH 4/4] fix: remove duplicate line --- test/commands/app/deploy.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/commands/app/deploy.test.js b/test/commands/app/deploy.test.js index 3cb3e3dc..dd8d6ee2 100644 --- a/test/commands/app/deploy.test.js +++ b/test/commands/app/deploy.test.js @@ -113,7 +113,6 @@ beforeEach(() => { helpers.buildExtensionPointPayloadWoMetadata.mockReset() helpers.buildExcShellViewExtensionMetadata.mockReset() helpers.createWebExportFilter.mockReset() - helpers.createWebExportFilter.mockReset() jest.restoreAllMocks()