[Issue 877] Refactor Form Submission #919
Conversation
* feat: Adds new targets to Makefile - sprint-data-export: Exports data from the sprint project - issue-data-export: Exports issue data from the repo - gh-data-export: Exports both the sprint and issue data - sprint-burndown: Runs sprint burndown metric - percent-complete: Runs deliverable percent complete metric - sprint-reports: Runs both sprint-burndown and percent-complete - sprint-reports-with-export: Runs gh-data-export before report * refactor: Moves test e2e run of reports to ci-analytics.yml Instead of running the reports and posting them to slack on every PR, this commit moves the dry-run of the reports to the ci-analytics.yml This shift helps reduce potential noise in the slack channel * feat: Adds scheduled run for analytics Scheduled to run Monday-Friday at 12:30pm ET
[Issue 698] Add rest of opportunity table columns --------- Co-authored-by: nava-platform-bot <[email protected]>
[Issue 822] Add Alembic check command
Updated the ADR based on information from Sammy and Andy when implementing the newsletter on simpler.grants.gov
* make links bold (to match Figma) * nix stray period after the link
* Change site title from "Simpler Grants.gov" to "Simpler.Grants.gov" * Do the same in tab titles for each page
* build: Adds pytest-cov to dependencies * feat: Adds test coverage to Makefile * tests: Expands test coverage for analytics package * refactor: Updates BaseMetric to accept output_dir for exports This allows us to have a flexible but consistent output directory for all exports * fix: How @current is interpreted on sprint end dates Previously using @current would give you the burndown for the sprint that just started when it was run on the first day of a sprint. This commit fixes that so it gives the burndown of the previous sprint. * fix: Issue with current_sprint By default pd.Timestamp.today() returns the current timestamp In order to make date comparisons we needed to use .floor("d") * fix: Wraps SPRINT var in quotes in Makefile Prevents the Makefile from throwing an error with multi-word sprint names
* Add border below roadmap section * Add chevron icons to roadmap * Update ContentLayout component to accepts gridGap sizes
* [Issue 710] Add query logging and update logging docs
…917) [Issue 893] Make opportunity API model have more fields from the DB --------- Co-authored-by: nava-platform-bot <[email protected]>
* Add prefixes to milestone headers, styled as smaller pre-heading
[Issue 779] Writeup Alembic pros & cons ADR
| @@ -35,6 +35,8 @@ The application can be ran natively or in a Docker container. | |||
|
|
|||
| #### Native | |||
|
|
|||
| There are several secret environment variables necessary to submit the form related to the newsletter. Duplicate the `/frontend/env.development` file and name the copy `/frontend/.env.local`, which will not be checked into github. Fill in the three variables related to Sendy. Ask another engineer on the team for those values if you don't have them. | |||
There was a problem hiding this comment.
(praise) Thanks for updating the docs
(question) Where are we storing the env vars, OnePass / AWS?
There was a problem hiding this comment.
for deployed environments, these values are stored in AWS SSM, which is then added to our ECS containers via terraform so they are available at runtime
| @@ -0,0 +1,73 @@ | |||
| import type { NextApiRequest, NextApiResponse } from "next"; | |||
There was a problem hiding this comment.
(thought) The app router just landed in the template. The api would be a logical place to introduce the app router as it NextJS recommends incrementally updating to the app router since the app and page routers can be used simultaneously. This could be an opportunity to introduce the app router, depending on your energy to invest in that.
There was a problem hiding this comment.
Yes, I'd suggest breaking that out into a tech debt ticket as this PR has gotten pretty big so far.
| const sendyApiUrl = process.env.SENDY_API_URL || ""; | ||
| const sendyApiKey = process.env.SENDY_API_KEY || ""; | ||
| const list = process.env.SENDY_LIST_ID || ""; |
There was a problem hiding this comment.
If these aren't set, should it error/return instead? Otherwise I guess it would error somewhere in the request process (ie. if the URL is null we'd get some 404 error presumably?).
There was a problem hiding this comment.
Good call out, this is what the user sees if they try to submit the form without those values set:
| sendy_api_key = var.sendy_api_key != null ? [{ name = "SENDY_API_KEY", value = var.sendy_api_key }] : [] | ||
| sendy_api_url = var.sendy_api_url != null ? [{ name = "SENDY_API_URL", value = var.sendy_api_url }] : [] | ||
| sendy_list_id = var.sendy_list_id != null ? [{ name = "SENDY_LIST_ID", value = var.sendy_list_id }] : [] |
There was a problem hiding this comment.
⭐ (very much optional, just nice to have) the != null check seems non-ideal since, to the best of my understanding, we want the sendy credentials to always be defined. I would rather the container blow up if they're undefined, rather than them being empty.
Unless of course I'm totally missing something here ^^
There was a problem hiding this comment.
The intention of the != null is that the backend api ecs containers do not need these variables, but the frontend and the api both use the same service module. This would be the equivalent of saying if this is on the frontend, then set these values, if it's on the api, set an empty list.
Is there a cleaner way to do that here?
Summary
Fixes #877
Time to review: 30 mins
Changes proposed
Context for reviewers
Additional information
Already signed up state:
Other sendy errors:
Missing Inputs:
Invalid email:
