Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCM-7465: implement backend api and update automated tests PT.2 #240

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bhansell1
Copy link
Contributor

Description

  • replaces Amplify data with Backend-api
  • updates automated tests to be authenticated

Context

Type of changes

  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I am familiar with the contributing guidelines
  • I have followed the code style of the project
  • I have added tests to cover my changes
  • I have updated the documentation accordingly
  • This PR is a result of pair or mob programming

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

@@ -7,6 +7,8 @@ const domain = process.env.NOTIFY_DOMAIN_NAME ?? 'localhost:3000';

const nextConfig = (phase) => {
const isDevServer = phase === PHASE_DEVELOPMENT_SERVER;
const includeAuthPages =
Copy link
Contributor Author

@bhansell1 bhansell1 Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this in because the automated tests running in CI do a production build of the we application.

This build does not include the the auth pages. So we're unable to login to the app.

I'm not entirely happy with it. Any alternative suggestions would be great.

--

In the automated tests we take what is effectively a snapshot of the cookies and store them in a file called user.json. This information is then passed into the browser context on each test run.

See this comment -> #240 (comment)

@bhansell1 bhansell1 changed the title CCM-7465: implement backend api and update automated tests CCM-7465: implement backend api and update automated tests PT.2 Dec 20, 2024

# Includes auth pages when building web frontend. This is needed in CI for the automated tests
# which run the app in production mode.
INCLUDE_AUTH_PAGES=''
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
return aCreatedAt < bCreatedAt ? 1 : -1;
});
const sortedData = data.sort((a, b) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Re-add isValidTemplate and update the function to accept TemplateDTO

@@ -34,5 +34,7 @@ resource "aws_amplify_app" "main" {
NEXT_PUBLIC_DISABLE_CONTENT = var.disable_content
AMPLIFY_MONOREPO_APP_ROOT = "frontend"
BACKEND_API_URL = module.backend_api.api_base_url
COGNITO_USER_POOL_ID = jsondecode(data.aws_ssm_parameter.cognito_config.value)["USER_POOL_ID"]
Copy link
Contributor Author

@bhansell1 bhansell1 Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct getting the values out like this.

Any suggestions on a better way to do it?


Amplify.configure(amplifyConfig, { ssr: true });
Amplify.configure(AMPLIFY_OUTPUTS(), { ssr: true });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure why but calling AMPLIFY_OUTPUTS() causes the cognito login page to say "Cognito not configured".

You can do a Amplify.getConfig right after this call and see that it's been set correctly.

The only way I've seen to work around this is to NOT called process.env.COGNITO_USER_POOL_ID and instead use a string literal.

timeout-minutes: 10
steps:
- name: "Checkout code"
uses: actions/checkout@v4
- uses: actions/download-artifact@v4
with:
name: amplify_outputs.json
path: frontend/
name: sandbox_tf_outputs.json
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to read in this output files in the automated tests. But I get a pipeline failure saying values are undefined.

So I assume this file get put in the wrong place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to set the COGNITO_USER_POOL_ID and COGNITO_USER_POOL_CLIENT_ID somewhere for the web-app.

@@ -0,0 +1,8 @@
export const AMPLIFY_OUTPUTS = () => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention here was to have a single place for Amplify outputs driven by environment variables.

But that doesn't seem to work with Amplify. Only string literals appear to work.

So this idea might need to be scrapped and we generate our own amplify_ouputs.json file probably via generate-dependencies?

@@ -0,0 +1,18 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a weird one

When running the web-app in dev mode npm run dev then running ALL the automated tests. The component tests become flakey.

Running the web-app in production mode npm run buld && npm run start the tests pass quickly and are not flakey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants