Closed humphd closed 2 years ago
Apologies while I get this CI run to pass, it will take a few tries.
Amazing work! Since I'm still quite a bit out of my depth (and practice!) it's probably best if @devarsh19 @HaiderZaidiDev @Sync271 do this review as I'm not entirely sure what I'm looking at/for.
Great descriptions! I'm learning so much as we go through this! :)
I am not sure if I am qualified to review these changes 😅 but I'll do my best!
I am not sure if I am qualified to review these changes 😅 but I'll do my best!
Even asking me to explain anything that you don't understand would be useful, as it might help us catch something I missed, or wasn't clear on. Thanks for helping me.
Even asking me to explain anything that you don't understand would be useful, as it might help us catch something I missed, or wasn't clear on. Thanks for helping me.
Yup, I was looking into the below code and wondering why use OR Operator over Null Coalescing operator, and later realized the node version we're using doesn't support ECMA2020. https://github.com/mekkim/donatemask/blob/5e5c54cf99f5bebc2506f6979a197c2395e5d144/server/util.js#L4
I can't seem to get the API key to pass through to the container. @mekkim, can you confirm for me that you have the STRIPE_API_KEY
and STRIPE_PRICE_ID
GitHub Actions Secrets defined like in this pic (NOTE: I've not used the full API key here, but you get the idea):
If you did it correctly, I need to try and figure out why it's failing in my code. Thanks.
I looked at the repo secrets config with @mekkim, and I think he's done everything correctly. I'm suspicious that GitHub Actions isn't allowing the secrets to be accessed by a pull request, and that if this gets merged to main
, it will work.
I'm going to try merging this, and then see how it behaves. Once I know the answer to that, I can do follow-ups to correct any problems. It also means we shouldn't rebase prod
until this is sorted.
@Sync271 the reason I'm duplicating that env in server/config-dev.env
and server/config-sample.env
, is that the latter defines what is needed to run locally, and gets loaded automatically with npm run dev
; while the latter is documentation of everything that can be set, and serves as a guide for creating a production config.
Nope. I'll follow up in https://github.com/mekkim/donatemask/issues/112.
This is really interesting. I'm learning a pile as we go with all of this. I had been out of touch from this world for a decade or so. :)
Fixes #103 Fixes #94
Apologies for the size of this PR, but adding the integration test for Stripe required me to build a lot of scaffolding around the dev/test/CI code. I'll try to explain it all so it's clear what this is and does. Let me know what isn't clear.
The main goal of this change is to run an end-to-end, integration test for the Stripe payments. I'm using a combination of Docker/docker-compose (i.e., to build and run the back-end + front-end like in production) and Playwright to run automated tests in headless browsers. I've created a test Stripe API key and Price ID to use, and @mekkim has added them as secrets to the repo. With these in place, I can automate testing payments with Stripe in an identical way to how it works in production. There is a risk of some part of this failing in CI, since it depends on real network requests to Stripe. However, prod has the same dependency, so this is a useful test.
The Docker setup is necessary for simulating how the site gets run in production. I have a
Dockerfile
to build the front-end and then run the back-end using the same version of node as prod (i.e., v.13.14.0). Doing this work, I noticed some dependencies missing from the serverpackage.json
, which I've added. Thedocker-compose.yml
file defines how to run everything with a local, containerized MongoDB instance. I don't assume people will use this in dev; it's more for running in CI. NOTE: it requires the Stripe secrets in order to be useful.I've updated the
npm
scripts to add some better ways to run things locally. Usingnpm run dev
will start the back-end and front-end together, and you can now run everything locally without the prod server being hard-coded. The front-end will proxy the back-end vialocalhost:4443
. One thing I've removed, and I think is OK (please let me know) is the catch-all routing for theindex.html
file. In production, this seems to get served by Apache vs. Express, so I don't think this is needed, expect for testing? I use it in the Docker setup now. I've updated theREADME.md
to give instructions on how these work. Let me know if it isn't clear. In local development, I'm adding a way to use an in-memory MongoDB server by passingmemory
as yourATLAS_URI
. Theserver/config-dev.env
does this for you, and gets loaded automatically when you donpm run dev
.While doing this testing, I noticed that there were cases in which the Stripe checkout code could fail, but there's no user error handling (i.e., it just dies). I've created another case for
/donate?success=false
, similar to what we added for?success=true
, and it shows an error to the user, so they know something went wrong. These two components share a lot of code, and could get refactored to reduce the shared bits in a follow up.I've tried to be careful about how these changes affect the running code, but I might have missed some things, or misunderstood some existing code, so it would be good to get some review on this before we merge. Hopefully the extra tests in CI will give us more confidence that the site can be built and runs how we expect.
I'm marking this as a Draft until I feel more confident that it's safe to land.