feat: in app messaging notification #11067

lazpavel · 2022-09-26T16:58:55Z

Description of changes

in app messaging feature
-- adds a new channel for in app messaging
-- changes the place for pinpoint resource to always stay in the analytics category
-- ensures auth and analytics are added as a pre-requirement for the notifications channels
-- notification channels re-use the pinpoint resource from the analytics category

Description of how you validated changes

manual tests
cloud-e2e pass
e2e tests and unit test added for in-app-messaging channel

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

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

lgtm-com · 2022-09-26T17:14:16Z

This pull request introduces 7 alerts when merging eb3c10e into 6f287b1 - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class
3 for Useless conditional

lgtm-com · 2022-09-26T17:27:50Z

This pull request introduces 7 alerts when merging de2a2d7 into 6f287b1 - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class
3 for Useless conditional

lgtm-com · 2022-09-26T18:10:41Z

This pull request introduces 4 alerts when merging de5d49b into f06c821 - view on LGTM.com

new alerts:

3 for Useless conditional
1 for Unused variable, import, function or class

lgtm-com · 2022-09-27T14:38:51Z

This pull request introduces 1 alert when merging a77f14c into f06c821 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-09-27T18:23:19Z

This pull request introduces 1 alert when merging addef6b into f06c821 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-09-27T19:42:47Z

This pull request introduces 1 alert when merging 46a1675 into f06c821 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

edwardfoyle

I didn't go through this in depth enough to really get the bigger picture so most of my comments are relatively localized

edwardfoyle · 2022-09-29T00:35:43Z

...vider-utils/awscloudformation/cloudformation-templates/pinpoint-cloudformation-template.json

+                    "logs:CreateLogStream",
+                    "logs:PutLogEvents"
+                  ],
+                  "Resource": "arn:aws:logs:*:*:*"


we should scope this policy better

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

packages/amplify-category-analytics/src/plugin-client-api-auth.ts

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

packages/amplify-category-notifications/src/notifications-amplify-meta-api.ts

packages/amplify-category-notifications/src/notifications-backend-cfg-api.ts

packages/amplify-category-notifications/src/notifications-backend-cfg-channel-api.ts

packages/amplify-category-notifications/src/pinpoint-helper.ts

packages/amplify-cli/src/extensions/amplify-helpers/get-resource-outputs.ts

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

packages/amplify-category-notifications/src/auth-helper.ts

packages/amplify-category-notifications/src/legacy-pinpoint-helper.ts

akshbhu · 2022-09-29T17:13:56Z

packages/amplify-category-notifications/package.json

@@ -29,7 +29,8 @@
    "inquirer": "^7.3.3",
    "lodash": "^4.17.21",
    "ora": "^4.0.3",
-    "promise-sequential": "^1.1.1"
+    "promise-sequential": "^1.1.1",
+    "chalk": "^4.1.1"
  },
  "devDependencies": {
    "mockirer": "^2.0.1"


nit: see if we can replace this : https://www.npmjs.com/package/mockirer . package seems not maintained

we probably should have a separate task to get rid of all inquirer references, this will allow us to remove the mockirer in the same step

packages/amplify-category-notifications/src/legacy-pinpoint-helper.ts

awsluja · 2022-09-29T20:39:40Z

packages/amplify-e2e-tests/src/__tests__/notifications.test.ts

 import {
-  addSMSNotification,


Is this the only e2e test we have for notifications? if so, what concepts do you think should be verified (what's a good e2e test for this PR)

this is the only one, it used to test just SMS channel and now was changed to do a push/pull for SMS and In-App Messaging channels.
not sure what can we do with a e2e test, but we should test that creating resources with older version and then upgrading to the new implementation and updating/adding new channels work as expected. One of the main changes in this PR is that notifications and analytics no longer create 2 distinct pinpoint resources but whoever will share the same resource, also pinpont resource is always managed by the analytics category so adding a notification category will also add a analytics category with the pinpoint resource.

For the above we should test various combinations for when adding/updating analytics/notifications channgels are mixed and matched with old and new implementation

Other things that could be tested are update and delete for the notifications channels (this tests only tests add)

lgtm-com · 2022-09-29T22:54:51Z

This pull request introduces 2 alerts when merging 915acd1 into e4593c4 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

packages/amplify-category-notifications/src/multi-env-manager.ts

lgtm-com · 2022-09-30T15:44:25Z

This pull request introduces 2 alerts when merging 50fa878 into e4593c4 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

codecov-commenter · 2022-09-30T16:07:50Z

Codecov Report

Merging #11067 (84444b7) into dev (4a4fee8) will decrease coverage by 0.31%.
The diff coverage is 47.90%.

@@            Coverage Diff             @@
##              dev   #11067      +/-   ##
==========================================
- Coverage   49.64%   49.32%   -0.32%     
==========================================
  Files         666      676      +10     
  Lines       31923    32628     +705     
  Branches     6500     6664     +164     
==========================================
+ Hits        15848    16094     +246     
- Misses      14595    15051     +456     
- Partials     1480     1483       +3

Impacted Files	Coverage Δ
...s/amplify-cli-core/src/errors/amplify-exception.ts	`65.21% <ø> (ø)`
packages/amplify-category-auth/src/index.js	`16.27% <14.28%> (-0.05%)`	⬇️
...otifications/src/notifications-amplify-meta-api.ts	`16.66% <16.66%> (ø)`
.../amplify-category-notifications/src/auth-helper.ts	`21.83% <20.00%> (ø)`
...y-provider-awscloudformation/src/push-resources.ts	`19.58% <20.00%> (+<0.01%)`	⬆️
...notifications/src/notifications-backend-cfg-api.ts	`20.58% <20.58%> (ø)`
...lify-category-notifications/src/pinpoint-helper.ts	`18.51% <21.51%> (ø)`
...category-notifications/src/apns-cert-p12decoder.ts	`16.66% <25.00%> (+1.74%)`	⬆️
...tions/src/notifications-backend-cfg-channel-api.ts	`29.34% <29.34%> (ø)`
...amplify-cli-core/src/state-manager/stateManager.ts	`37.56% <37.50%> (-0.01%)`	⬇️
... and 22 more

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

lgtm-com · 2022-09-30T17:24:31Z

This pull request introduces 1 alert when merging 104a409 into e4593c4 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

* feat: in app messaging initial commit

… in-app-messaging

lgtm-com · 2022-10-05T20:33:22Z

This pull request introduces 3 alerts when merging 6c65379 into 85696bd - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2022-10-06T00:44:32Z

This pull request introduces 3 alerts when merging c6a1ac6 into c454ae7 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2022-10-06T03:14:32Z

This pull request introduces 1 alert when merging 6cf7029 into c09e541 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-10-06T23:44:11Z

This pull request introduces 1 alert when merging 95c47e3 into 9f3bba7 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

edwardfoyle

added some comments but I don't think any of them are merge blockers

edwardfoyle · 2022-10-13T17:39:53Z

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

+ * @returns push command response
+ */
+export const authPush = async (context: $TSContext): Promise<$TSAny> => {
+  const { run } = await import(path.join('.', name, 'push'));


could we turn this into a normal import?

addressed in follow-up PR: #11158

edwardfoyle · 2022-10-13T17:41:30Z

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

+ * Execute auth Push command with force yes
+ * @param {Object} context - The amplify context.
+ */
+const authPush = async context => {


rename to authPushYes

addressed in follow-up PR: #11158

edwardfoyle · 2022-10-13T17:42:16Z

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

+  const commandPath = path.normalize(path.join(__dirname, 'commands', category, 'push'));
+  const commandModule = await import(commandPath);


static import?

addressed in follow-up PR: #11158

edwardfoyle · 2022-10-13T17:43:40Z

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

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


will this have any downstream impact on other prompts in the original flow? It could also lead to some inconsistencies because amplify-prompter looks for the --yes flag in process.argv not in context

not in this particular case, but it is injecting side-effects that could affect future implementation that look at this parameters after this call

addressed restoring the original value after push

edwardfoyle · 2022-10-13T17:48:58Z

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

+export const run = async (context: $TSContext): Promise<$TSAny> => {
  try {
-    const { run } = await import(path.join('.', name, context.parameters.first));
-    return run(context);
+    const { run: authRun } = await import(path.join('.', name, context.parameters.first));
+    return authRun(context);


What about leaving this as the only function exported by this file and add an optional second parameter to override the action:

export const run = async (context: $TSContext, actionOverride?: 'push'): Promise<$TSAny> => { try { const { run: authRun } = await import(path.join('.', name, actionOverride ?? context.parameters.first)); return authRun(context);

edwardfoyle · 2022-10-13T17:51:55Z

packages/amplify-category-notifications/src/__tests__/channel-apns.test.ts

+    updateApnsChannel: jest.fn(() => ({
+      promise: () => new Promise((resolve, __) => {
+        resolve(mockPinpointResponseData);
+      }),


nit all of these could simplify to:

Suggested change

updateApnsChannel: jest.fn(() => ({

promise: () => new Promise((resolve, __) => {

resolve(mockPinpointResponseData);

}),

updateApnsChannel: jest.fn().mockReturnValue({

promise: jest.fn().mockResolvedValue(mockPinpointResponseData),

}),

edwardfoyle · 2022-10-13T17:56:14Z

packages/amplify-category-notifications/src/__tests__/channel-fcm.test.ts

+const mockInquirer = (answers : $TSAny): $TSAny => {
+  (inquirer as any).prompt = async (prompts:$TSAny):Promise<$TSAny> => {
+    [].concat(prompts).forEach(prompt => {
+      if (!((prompt as unknown as any).name in answers) && typeof (prompt as unknown as any).default !== 'undefined') {
+        // eslint-disable-next-line no-param-reassign
+        answers[(prompt as unknown as any).name] = (prompt as unknown as any).default;
+      }
+    });
+    return answers;
+  };
+};


can we use jest mocks instead of all this reassigning and type asserting?

edwardfoyle · 2022-10-13T18:15:13Z

packages/amplify-category-notifications/src/display-utils.ts

+ * Display status that Auth and Pinpoint resources are being deployed to the cloud
+ */
+export const viewShowInlineModeInstructionsStart = async (channelName: string): Promise<void> => {
+  spinner.start(`Channel ${channelName} requires a Pinpoint resource in the cloud. Proceeding to deploy Auth and Pinpoint resources...`);


It's probably not worth a bunch of rework at this point but maybe a TODO or backlog item is appropriate

edwardfoyle · 2022-10-13T19:55:04Z

packages/amplify-category-notifications/src/channel-sms.ts

+    return buildPinpointChannelResponseSuccess(ChannelAction.ENABLE, deploymentType, channelName, data.SMSChannelResponse);
+  } catch (e) {
+    spinner.stop();
+    throw amplifyFaultWithTroubleshootingLink('NotificationsChannelEmailFault', {


we should pass the underlying stack into the amplfiy fault as well (applies to lots of places). Not sure how feasible it is to update all of them

akshbhu

Some nits otherwise LGTM!!

akshbhu · 2022-09-29T20:06:34Z

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

+      Region: pinpointMeta.Region,
+    }
+    : undefined;
+  stateManager.setTeamProviderInfo(projectPath, teamProviderInfo);


Can use envManager here to populate tpi data

akshbhu · 2022-10-13T19:36:20Z

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

+      break;
+    }
+    default: {
+      throw Error(`Channel ${notificationChannel} is not supported on Analytics resource`);


Suggested change

throw Error(`Channel ${notificationChannel} is not supported on Analytics resource`);

throw new AmplifyError('ConfigurationError', {

message: `Channel ${notificationChannel} is not supported on Analytics resource`,

resolution: 'Use one of the supported channels',

akshbhu · 2022-10-13T19:40:06Z

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

@@ -79,7 +88,7 @@ export const getPermissionPolicies = async (context: $TSContext, resourceOpsMapp
      printer.warn(`Could not get policies for ${category}: ${resourceName}`);
      throw e;


nit: We can throw Amplify Error here

akshbhu · 2022-10-13T19:42:50Z

packages/amplify-category-analytics/src/plugin-client-api-auth.ts

+ * @returns Resource in Notifications category (IAmplifyResource type)
+ */
+export const invokeAuthPush = async (context: $TSContext): Promise<void> => {
+  await context.amplify.invokePluginMethod(context, AmplifyCategories.AUTH, undefined, 'authPluginAPIPush', [context]);


nit: I think we can directly call context.amplify directly

akshbhu · 2022-10-13T20:21:41Z

packages/amplify-category-notifications/src/notifications-api.ts

+        context.exeInfo.amplifyMeta = await toggleNotificationsChannelAppMeta(channelAPIResponse.channel, false,
+          context.exeInfo.amplifyMeta);
+        break;
+      case ChannelAction.CONFIGURE:


nit: we can remove the case if the operation is noop

it should break out instead of printing an error

akshbhu · 2022-10-13T20:23:04Z

packages/amplify-category-notifications/src/notifications-api.ts

+  try {
+    deployedBackendConfig = (stateManager.getCurrentBackendConfig()) || undefined;
+  } catch (e) {
+    appInitialized = false;


Do we have to throw error here if file operation fails ?

akshbhu · 2022-10-13T21:23:03Z

packages/amplify-provider-awscloudformation/src/push-resources.ts

@@ -445,6 +447,13 @@ export const run = async (context: $TSContext, resourceDefinition: $TSObject, re
      await context.amplify.updateamplifyMetaAfterPush(resources);
    }

+    // Generate frontend resources for any notifications channels enabled on analytics resources.
+    const analyticsResources = resourcesToBeCreated.filter((r: { category: string; }) => r.category === AmplifyCategories.ANALYTICS);


Suggested change

const analyticsResources = resourcesToBeCreated.filter((r: { category: string; }) => r.category === AmplifyCategories.ANALYTICS);

const analyticsResources = resourcesToBeCreated.filter((resource: { category: string; }) => r.category === AmplifyCategories.ANALYTICS);

lazpavel · 2022-10-13T22:34:36Z

going to address the remaining nits as part of the analytics migration PR: #11158

lazpavel changed the title ~~feat: in app messaging initial commit~~ feat: in app messaging notification Sep 26, 2022

lazpavel added 5 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

lazpavel marked this pull request as ready for review September 26, 2022 20:44

lazpavel requested a review from a team as a code owner September 26, 2022 20:44

lazpavel added 2 commits September 26, 2022 17:37

Delete channel-Email.ts

480d21e

feat: convert errors to amplify errors

acb9876

Merge branch 'dev' into in-app-messaging

60f83ae

edwardfoyle reviewed Sep 29, 2022

View reviewed changes

lazpavel added 2 commits September 29, 2022 11:31

feat: address PR feedback

f291641

Merge branch 'dev' into in-app-messaging

633f4b1

akshbhu reviewed Sep 29, 2022

View reviewed changes

Merge branch 'dev' into in-app-messaging

675826b

awsluja reviewed Sep 29, 2022

View reviewed changes

sobolk reviewed Sep 30, 2022

View reviewed changes

packages/amplify-category-notifications/src/multi-env-manager.ts Outdated Show resolved Hide resolved

sobolk reviewed Sep 30, 2022

View reviewed changes

packages/amplify-category-notifications/src/multi-env-manager.ts Outdated Show resolved Hide resolved

feat: address PR feedback

44d6cd1

lazpavel and others added 8 commits October 3, 2022 14:25

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

Pavel Lazar added 2 commits October 6, 2022 17:11

Merge branch 'dev' into in-app-messaging

00185b4

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

95c47e3

Pavel Lazar and others added 6 commits October 6, 2022 20:55

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

Merge branch 'dev' into in-app-messaging

75e5ee8

edwardfoyle reviewed Oct 13, 2022

View reviewed changes

akshbhu approved these changes Oct 13, 2022

View reviewed changes

lazpavel merged commit 0c70a05 into aws-amplify:dev Oct 13, 2022

		const commandPath = path.normalize(path.join(__dirname, 'commands', category, 'push'));
		const commandModule = await import(commandPath);

-      throw Error(`Channel ${notificationChannel} is not supported on Analytics resource`);
+      throw new AmplifyError('ConfigurationError', {
+        message: `Channel ${notificationChannel} is not supported on Analytics resource`,
+        resolution: 'Use one of the supported channels',

		@@ -79,7 +88,7 @@ export const getPermissionPolicies = async (context: $TSContext, resourceOpsMapp
		printer.warn(`Could not get policies for ${category}: ${resourceName}`);
		throw e;

	const analyticsResources = resourcesToBeCreated.filter((r: { category: string; }) => r.category === AmplifyCategories.ANALYTICS);
	const analyticsResources = resourcesToBeCreated.filter((resource: { category: string; }) => r.category === AmplifyCategories.ANALYTICS);

feat: in app messaging notification #11067

feat: in app messaging notification #11067

Conversation

lazpavel commented Sep 26, 2022 • edited

Description of changes

Description of how you validated changes

Checklist

lgtm-com bot commented Sep 26, 2022

lgtm-com bot commented Sep 26, 2022

lgtm-com bot commented Sep 26, 2022

lgtm-com bot commented Sep 27, 2022

lgtm-com bot commented Sep 27, 2022

lgtm-com bot commented Sep 27, 2022

edwardfoyle left a 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

lgtm-com bot commented Sep 29, 2022

lgtm-com bot commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 • edited

Codecov Report

lgtm-com bot commented Sep 30, 2022

lgtm-com bot commented Oct 5, 2022

lgtm-com bot commented Oct 6, 2022

lgtm-com bot commented Oct 6, 2022

lgtm-com bot commented Oct 6, 2022

edwardfoyle left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshbhu left a 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

lazpavel commented Oct 13, 2022

lazpavel commented Sep 26, 2022 •

edited

codecov-commenter commented Sep 30, 2022 •

edited