cremich / kiko-backend Goto Github PK

Is your feature request related to a problem? Please describe.
Right now a tenant manager gets no direct feedback loop whether the resulting pinpoint campaign was successful or how many endpoints received the SMS.

Describe the solution you'd like
We want to provide more transparency for a tenant manager about success rates and KPIs.

Is your feature request related to a problem? Please describe.
Onboarding new tenants requires to create some AWS resources or datasets in the KIKO backend. This should be done in an automated way to reduce the risk of errors due to misconfigurations.

What do we have to create for every new tenant:

• a project in Amazon Pinpoint
• a user group in Amazon Cognito
• a user for the tenant manager that is associated with the user group
• a set of test pools as new items in the test-pool DynamoDb Table

Describe the solution you'd like
Preferably a solution that provides a reliable and consistent mechanisms to create all resources. Idealwise something that is also maintained by cloudformation. To ensure that once we delete the whole KIKO app, we are sure that all related resources are also deleted. In the end I think of a solution that is built upon or extends how the demo tenant is created by using Cloudformation custom resources.

Describe alternatives you've considered
An alternative might be to create all things using for example a Lambda function. But this might result in some retained resources once we delete the whole stack and corona packs his bag!

I finally got around to trying to send me an SMS. I didn't receive one and saw no errors in the logs or CloudTrail. After further investigation Pinpoint campaign details show that one SMS was sent but none were delivered, resulting in a delivery rate of 0%.

Only when I tried to send myself another message via pinpoint's test feature, it showed me this:

Message not sent: We weren't able to send your SMS message to +49172xxxxxxx.
Details
Request Id 415 : Your account requires additional review prior to being able to send SMS messages. Please create a support ticket with an explanation of your use case. https://console.aws.amazon.com/support/cases#/create?issueType=service-limit-increase&limitType=service-code-sns-text-messaging

I reckon you went through that process @cremich ? Should this maybe be noted in the Readme?

More importantly, alerting on the delivery rate of campaigns would be good to have. Failure to deliver the message, moreover silently, undermines the whole idea.

To Reproduce
Use an AWS account that was never used/unlocked for SMS sending before.

The snapshot tests fail due to resource hash values apparently changing a lot depending on actual stack changes but also just due to where they're run. For instance a newly updated snapshot that's working on one machine fails in the Github workflow.

An approach could be to ignore them as described here.

Is your feature request related to a problem? Please describe.
Sending SMS immediately produces costs in the AWS account. Sending the SMS might not be needed in all cases during development or testing.

Describe the solution you'd like
Extend the test result processing workflow to something like a "dry" run to only simulate the step to send the SMS or add the test result to the activity log.

Describe the bug
The mutation description contains a wrong attribute named groupName. We have to change this to poolName in the graphql schema as well as in the request mapping.

To Reproduce
Try to call the graphQL endpoint via:

curl 'https://my-endpoint/graphql' -H 'Content-Type: application/json' --data-raw '{"query":"mutation ($input: ProcessTestResultInput!) {\n  processTestResult(input: $input) {\n    status\n  }\n}\n","variables":{"input":{"groupName":"Gamma","testResult":"P"}}}'

Expected behavior
The step function execution of the test result processing is in state "succeded"

Seems not to be the case, that dependabot is able to create consolidated PRs. Only updating one CDK lib might be not recommended and increases the risk of test errors. But I found this here:

https://www.hrvey.com/blog/combine-dependabot-prs

Would be cool to have something similar.

Originally posted by @cremich in #8 (comment)

Is your feature request related to a problem? Please describe.
Code coverage is currently not measured. So we are a bit blind on this eye.

Describe the solution you'd like
We want to constantly measure code coverage with every pull request and show the current coverage in the README.

new Date().toISOString().split('T')[0] might get us a more uniformly formatted date.
The current approach would today create 2021-4-4.

Originally posted by @Bakkenrak in #48 (comment)

Following the instructions in the README i tried to deploy the kiko-backend to AWS using cdk.
The deployment for dev-kiko-pool-management went fine, but when cdk tried deploying dev-kiko-tenant-management the CloudFormation stack creation was aborted with the following error:

Domain already associated with another user pool. (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: 911ac4c0-42cf-4bf3-a4b4-e980e22cbf99; Proxy: null)

Not sure what domain can already associated with another user pool and why, since this was the very first time i tried deploying the app 🤷‍♀️

Since i am not that familiar with CloudFormation and AWS in general i started looking into bin/kiko.js and saw that the userpool domain prefix is generated using kiko-${deployStage}.

In the end i was able to deploy kiko-backend using something different for deployStage (i simply used test-dev 🤓).

Not sure if this a bug or expected behaviour, though.
If this is expected behaviour i would suggest adding this to the README.

Desktop (please complete the following information):

• OS: Mac OS
• CDK version (using npx): 1.100.0 (build d996c6d)

A very important feature to support onboarding new tenants will be, to give tenant managers the ability to upload and maintain recipient lists for each test pool. Those recipient lists are basically phone numbers we have to add as endpoints into Amazon Pinpoint. We have to keep in mind, that a phone number can be associated to multiple test pools but will be modeled as one endpoint in Pinpoint. A common use case for example in Kindergartens, where a mom or a dad has two kids in different groups.

Test Pool associations are modeled as endpoint attributes in pinpoint. So in a nutshell, a tenant manager must be able to

• add phone numbers
• associate phone numbers to test pools
• delete phone numbers (in case of a request for deletion)

Describe the bug
After rebasing to the current main i can't npm install in my fork anymore.
It gives me:

npm ERR! code E401
npm ERR! Incorrect or missing password.
npm ERR! If you were trying to login, change your password, create an
npm ERR! authentication token or enable two-factor authentication then
npm ERR! that means you likely typed your password in incorrectly.
npm ERR! Please try again, or recover your password at:
npm ERR!     https://www.npmjs.com/forgot
npm ERR! 
npm ERR! If you were doing some other operation then your saved credentials are
npm ERR! probably out of date. To correct this please try logging in again with:
npm ERR!     npm login

npm ERR! A complete log of this run can be found in:
npm ERR!     ...
npm i  8.42s user 10.30s system 215% cpu 8.696 total

Additional context

I would assume it has something to do with the latest fixes to package-lock.json as it introduced some private npm registry it looks like: https://dflds.jfrog.io/dflds/api/npm/npm-dfl/fsevents/-/fsevents-2.3.2.tgz

If I try to open the given URL i am propted with HTTP Basic Auth:

Is your feature request related to a problem? Please describe.
We face problems with our github build when someone pushes a package-lock that is created from npm version > 6.
See the following reference: aws/aws-cdk#13743

Describe the solution you'd like
We should add some instructions to the README that it is recommended to use node 14 with npm 6.x

Wow... this is more complexe than i thought and i cant really find a good way of testing it.
All @aws-cdk have to be updated simultaneously otherwise npm run build will fail, due to peer dependency mismatches.

Therefore this custom github action will not work or at least we would have to ignore the build status. And than it will still fail due to merge conflicts regarding the @aws-cdk dependency update PRs.

My take would currently be:

• ignore this combine-prs action
• exclude @aws-cdk dependencies from dependabot (and update them manually)
• see how noisy this actually will be (maybe it is ok for now)
• if it is too noisy take a look into renovatebot which is also free and easy to integrate

Renovatebot has out-of-the-box support for monorepos and will create consolidated PRs for them (in this case for @aws-cdk).

Would that ok with you @cremich?

Originally posted by @timvancleef in #14 (comment)