feat: migrates analytics category to support in app messaging channel notifications #11158

lazpavel · 2022-10-11T14:22:59Z

Description of changes

migrates analytics category to support the new in-app messaging channel
ensures the migration gets executed when the customer executes any of the [add, remove, update] commands on the analytics or notifications category

Issue #11087

Description of how you validated changes

unit and e2e tests pass
new e2e tests added to test migration scenarios
manual testing

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)
New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* feat: in app messaging initial commit

… in-app-messaging

… notifications

lgtm-com · 2022-10-14T02:24:26Z

This pull request introduces 3 alerts when merging a867eaf into 015187a - view on LGTM.com

new alerts:

3 for Syntax error

lazpavel · 2022-10-14T04:14:49Z

packages/amplify-category-analytics/src/analytics-resource-api.ts

-  }
-  return resourceList;
-};
+export const analyticsPluginAPIGetResources = (


body moved as it was needed to be reused for migration logic
packages/amplify-category-analytics/src/utils/pinpoint-helper.ts

lazpavel · 2022-10-14T04:16:09Z

packages/amplify-category-analytics/src/migrations/in-app-messaging-migration.ts

+  }
+};
+
+const migratePinpointCFN = (cfn: $TSAny): $TSAny => {


adds pinpoint policy and properties required for in app messaging channel

lazpavel · 2022-10-14T04:17:14Z

packages/amplify-category-auth/src/commands/auth.ts

- * @param context amplify cli context
- * @returns push command response
- */
-export const authPush = async (context: $TSContext): Promise<$TSAny> => {


export not needed as we could import directly in packages/amplify-category-auth/src/index.js

lazpavel · 2022-10-14T04:18:58Z

packages/amplify-category-notifications/src/channel-in-app-msg.ts

-/**
- * checks if pinpoint template contains the in app messaging policy
- */
-export const pinpointTemplateHasInAppMessagingPolicy = async (context: $TSContext): Promise<boolean> => {


moved to analytics category since it is checking a template in analytics category and it is reused in a couple of other places there for the migration purposes

lazpavel · 2022-10-14T04:19:36Z

packages/amplify-cli-npm/index.ts

@@ -17,4 +17,4 @@ export const install = async (): Promise<void> => {
 };

 // force minor version bump
-// -


gets the cli and cli-internal version in sync, this change is tied to a feat commit

lazpavel · 2022-10-14T18:16:02Z

packages/amplify-category-analytics/src/analytics-resource-api.ts

-export const analyticsPluginAPIGetResources = (resourceProviderServiceName?: string, context?: $TSContext): Array<IAnalyticsResource> => {
-  const resourceList: Array<IAnalyticsResource> = [];
-  const amplifyMeta = (context) ? context.exeInfo.amplifyMeta : stateManager.getMeta();
-  if (amplifyMeta?.[AmplifyCategories.ANALYTICS]) {


method extracted in order to be reused in this category as well: packages/amplify-category-analytics/src/utils/analytics-helper.ts

edwardfoyle · 2022-10-14T18:53:08Z

packages/amplify-category-analytics/src/commands/analytics.ts


 /**
 * Analytics category command router. Invokes functionality for all CLI calls
 * @param context amplify cli context
 */
-const analyticsRun = async (context:$TSContext): Promise<$TSAny> => {
+export const run = async (context: $TSContext): Promise<$TSAny> => {
  if (/^win/.test(process.platform)) {


I know this is existing code, but why is this only executing on windows?

not sure but when I debug I never hit this, always going through https://github.com/lazpavel/amplify-cli/blob/in-app-messaging-analytics-migrations/packages/amplify-category-analytics/src/index.ts#L105

edwardfoyle · 2022-10-14T19:15:56Z

packages/amplify-category-analytics/src/migrations/in-app-messaging-migration.ts

+      const cfn = JSONUtilities.readJson(templateFilePath);
+      const updatedCfn = migratePinpointCFN(cfn);
+      fs.ensureDirSync(resourcePath);
+      fs.writeFileSync(templateFilePath, JSON.stringify(updatedCfn, null, 4), 'utf8');


use JSONUtilities.writeJson

edwardfoyle · 2022-10-14T19:22:42Z

packages/amplify-category-analytics/src/utils/pinpoint-helper.ts

+      `pinpoint-cloudformation-template.json`,
+    );
+    const { cfnTemplate } = readCFNTemplate(pinpointCloudFormationTemplatePath, { throwIfNotExist: false }) || {};
+    return !!cfnTemplate?.Parameters?.pinpointInAppMessagingPolicyName;


I'd recommend putting pinpointInAppMessagingPolicyName in a const that is shared between where this value is written and read to ensure it stays in sync

edwardfoyle · 2022-10-14T19:22:55Z

packages/amplify-category-analytics/src/utils/pinpoint-helper.ts

+ */
+export const pinpointHasInAppMessagingPolicy = (context: $TSContext): boolean => {
+  const resources = getAnalyticsResources(context, AmplifySupportedService.PINPOINT);
+  if (resources?.length > 0) {


flip and early return

edwardfoyle · 2022-10-14T19:24:38Z

packages/amplify-category-auth/src/index.js

+    context.exeInfo = (context.exeInfo) || {};
+    context.exeInfo.inputParams = (context.exeInfo.inputParams) || {};
+    context.exeInfo.inputParams.yes = true; // force yes to avoid prompts
+    await authRunPush(context);


wouldn't this need to assign context.parameters.first = 'push' to force push to run?

it is working as is

edwardfoyle · 2022-10-14T19:25:19Z

packages/amplify-category-auth/src/index.js

-  context.exeInfo.inputParams.yes = true; // force yes to avoid prompts
-  await commandModule.run(context);
+const authPushYes = async context => {
+  const exeInfoClone = Object.assign({}, (context.exeInfo) || {});


nit: I think {...context?.exeInfo} does the same thing and is a little more concise

codecov-commenter · 2022-10-14T20:38:31Z

Codecov Report

Merging #11158 (9fb0c27) into dev (015187a) will increase coverage by 0.00%.
The diff coverage is 30.00%.

@@           Coverage Diff           @@
##              dev   #11158   +/-   ##
=======================================
  Coverage   49.36%   49.36%           
=======================================
  Files         677      678    +1     
  Lines       32684    32691    +7     
  Branches     6677     6674    -3     
=======================================
+ Hits        16133    16139    +6     
- Misses      15071    15072    +1     
  Partials     1480     1480

Impacted Files	Coverage Δ
...otifications/src/notifications-amplify-meta-api.ts	`16.66% <0.00%> (ø)`
...lify-category-notifications/src/pinpoint-helper.ts	`18.51% <0.00%> (ø)`
...s/amplify-cli-core/src/errors/amplify-exception.ts	`65.21% <ø> (ø)`
...amplify-cli-core/src/state-manager/stateManager.ts	`37.75% <0.00%> (+0.19%)`	⬆️
...y-provider-awscloudformation/src/push-resources.ts	`19.51% <0.00%> (ø)`
packages/amplify-category-auth/src/index.js	`16.50% <22.22%> (+0.22%)`	⬆️
...y-notifications/src/plugin-client-api-analytics.ts	`53.33% <50.00%> (-1.22%)`	⬇️
...amplify-category-notifications/src/channel-apns.ts	`65.04% <100.00%> (ø)`
...y-category-notifications/src/channel-in-app-msg.ts	`92.64% <100.00%> (+7.12%)`	⬆️
...es/amplify-category-auth/src/commands/auth/push.ts	`40.00% <0.00%> (ø)`
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

akshbhu · 2022-10-14T20:56:20Z

yarn.lock

@@ -10224,9 +10224,9 @@ amdefine@>=0.0.4:
  resolved "https://registry.npmjs.org/amdefine/-/amdefine-1.0.1.tgz#4a5282ac164729e93619bcfd3ad151f817ce91f5"
  integrity sha1-SlKCrBZHKek2Gbz9OtFR+BfOkfU=

-amplify-codegen@^3.2.0:
+amplify-codegen@^3.1.0:


Is this related ?

not sure why is 2 in dev here? we only reference 3.1.0

I'd check with data team. downgrading code gen doesn't feel right.
Do we reference exact 3.1.0 or ^ ?

… notifications (#11158) * feat: migrates analytics category to support in app messaging channel notifications

lazpavel and others added 28 commits September 26, 2022 15:44

feat: in app messaging initial commit

b09ce19

feat: add in app messaging e2e test

4142ebf

Delete channel-SMS.ts

074cc98

Delete channel-FCM.ts

35bdd26

Delete channel-APNS.ts

adcf2c8

Delete channel-Email.ts

480d21e

feat: convert errors to amplify errors

acb9876

Merge branch 'dev' into in-app-messaging

60f83ae

feat: address PR feedback

f291641

Merge branch 'dev' into in-app-messaging

633f4b1

Merge branch 'dev' into in-app-messaging

675826b

feat: address PR feedback

44d6cd1

feat: removing unused code

8d59f51

feat: narrowing down the scope for lambda execution role policy

6219ab9

feat: remove arm build from tagged release

645ca43

In-app-messaging (#11103)

758bead

* feat: in app messaging initial commit

Merge branch 'dev' into in-app-messaging

5dc4646

Merge branch 'dev' into in-app-messaging

b09659c

Merge branch 'tagged-release-without-e2e-tests/in-app-messaging' into…

1626e01

… in-app-messaging

fix: prevent adding in app messaging to projects with legacy analytics

f07e5e1

fix: update error handling for notification channels commands

6cf7029

Merge branch 'dev' into in-app-messaging

00185b4

fix: test that pulled project has correct aws-exports content

95c47e3

fix: incorrect status for notifications after pull

a6b7800

fix: fixed notifications unit test

a22d9ed

chore: remove arm target from tagged releases

5d8807c

Merge branch 'remove-arm-64-tagged-release' into in-app-messaging

3f61866

chore: self-review PR updates

99a2b16

lazpavel changed the base branch from dev to in-app-messaging October 11, 2022 15:28

Merge branch 'dev' into in-app-messaging

75e5ee8

feat: migrates analytics category to support in app messaging channel…

922cfdf

… notifications

lazpavel marked this pull request as ready for review October 11, 2022 21:05

lazpavel requested a review from a team as a code owner October 11, 2022 21:05

lazpavel changed the title ~~In-app-messaging-analytics-migrations~~ feat: migrates analytics category to support in app messaging channel notifications Oct 11, 2022

lazpavel changed the base branch from in-app-messaging to dev October 12, 2022 00:23

test: migrates analytics category to support in app messaging channel…

d6256e6

… notifications

lazpavel mentioned this pull request Oct 13, 2022

feat: in app messaging notification #11067

Merged

5 tasks

lazpavel changed the base branch from dev to 2493-clear-require-cache-between-lambda-invocations October 13, 2022 22:41

lazpavel changed the base branch from 2493-clear-require-cache-between-lambda-invocations to dev October 13, 2022 22:42

Pavel Lazar added 2 commits October 13, 2022 19:18

chore: merged dev

223d223

chore: PR feedback

8475eaf

chore: PR feedback

8803dda

lazpavel commented Oct 14, 2022

View reviewed changes

edwardfoyle reviewed Oct 14, 2022

View reviewed changes

feat: minor bump no-op

9fb0c27

edwardfoyle approved these changes Oct 14, 2022

View reviewed changes

akshbhu approved these changes Oct 14, 2022

View reviewed changes

lazpavel merged commit 9dfbf6c into aws-amplify:dev Oct 14, 2022

awsluja pushed a commit that referenced this pull request Oct 15, 2022

feat: migrates analytics category to support in app messaging channel…

84cc868

… notifications (#11158) * feat: migrates analytics category to support in app messaging channel notifications

awsluja pushed a commit that referenced this pull request Oct 17, 2022

feat: migrates analytics category to support in app messaging channel…

cf56333

… notifications (#11158) * feat: migrates analytics category to support in app messaging channel notifications

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrates analytics category to support in app messaging channel notifications #11158

feat: migrates analytics category to support in app messaging channel notifications #11158

lazpavel commented Oct 11, 2022 •

edited

lgtm-com bot commented Oct 14, 2022

lazpavel Oct 14, 2022

lazpavel Oct 14, 2022

lazpavel Oct 14, 2022

lazpavel Oct 14, 2022

lazpavel Oct 14, 2022 •

edited

lazpavel Oct 14, 2022

edwardfoyle Oct 14, 2022

lazpavel Oct 14, 2022

edwardfoyle Oct 14, 2022

edwardfoyle Oct 14, 2022

lazpavel Oct 14, 2022

edwardfoyle Oct 14, 2022

lazpavel Oct 14, 2022

edwardfoyle Oct 14, 2022

lazpavel Oct 14, 2022

edwardfoyle Oct 14, 2022

lazpavel Oct 14, 2022

codecov-commenter commented Oct 14, 2022 •

edited

akshbhu Oct 14, 2022

lazpavel Oct 14, 2022

sobolk Oct 14, 2022

feat: migrates analytics category to support in app messaging channel notifications #11158

feat: migrates analytics category to support in app messaging channel notifications #11158

Conversation

lazpavel commented Oct 11, 2022 • edited

Description of changes

Issue #11087

Description of how you validated changes

Checklist

lgtm-com bot commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazpavel Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 14, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazpavel commented Oct 11, 2022 •

edited

lazpavel Oct 14, 2022 •

edited

codecov-commenter commented Oct 14, 2022 •

edited