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

show a message if ENABLE_STUDY_FULL=(1/True/true) in process environment #6

Merged
merged 9 commits into from
Sep 10, 2021

Conversation

rrix
Copy link

@rrix rrix commented Aug 30, 2021

Deploy heroku with this variable set in the application configuration to disable new signups but not
revocation+verification endpoints

Some of the text still needs to be edited/audited, nonpublic Google
Docs URL in it which I can also include easily enough when Maggie can
provide it.

not a big fan of the massive changes in the diff , but that'll happen when you have to add new branches to code... none of the logic is really changed except for the escape hatch.

Deploy heroku with this variable set to disable new signups but not
revocation+verification endpoints

Some of the text still needs to be edited/audited, nonpublic Google
Docs URL in it which I can also include easily enough when Maggie can
provide it.
@rrix
Copy link
Author

rrix commented Aug 30, 2021

image

I used the same .column which is used to divide the regular pages in to two sections, but i'm not sure it looks good offset like this. defer to folks with an aesthetic sense.

@rrix
Copy link
Author

rrix commented Aug 30, 2021

@moates @ginnyfahs i expect this to be pretty straightforward to review; to test it you can add ENABLE_STUDY_FULL to docker-compose.yml

i can pretty quickly add a FAQ page given a doc before we land this and update the URL

src/webapp/study-full.js Outdated Show resolved Hide resolved
src/webapp/views/full.mustache Outdated Show resolved Hide resolved
@ginnyfahs
Copy link
Contributor

Thank you!

I have time with Legal this week for final approval of the FAQ, at which point we can add it as a new page in the app to avoid the google docs link. Will let you know when that's ready. So appreciate the help here!

@rrix
Copy link
Author

rrix commented Aug 31, 2021

Cleared the nit; i'll let this marinate until copy is approved and we can merge it all in one go.

@moates
Copy link
Contributor

moates commented Sep 8, 2021

When I tried to Docker build this, I ran into a syntax error. Not sure if I somehow have a stricter syntax checker enabled?

/opt/webapp/member.js:22
webapp_1 | const member = await Member.create({

webapp_1 | SyntaxError: await is only valid in async function
webapp_1 | at wrapSafe (internal/modules/cjs/loader.js:984:16)
webapp_1 | at Module._compile (internal/modules/cjs/loader.js:1032:27)
...

It's been a hot minute since I wrote any javascript, so idk if this is the kosher approach, but I was able to build by adding the async keyword to studyFullCheck's callback function in member.js

router.post('/sign-up', handleAsync(async (req, res) => {
  studyFullCheck(req, res, async () => {
    const member = await Member.create({ ...

@rrix
Copy link
Author

rrix commented Sep 8, 2021

no, i don't think it's your or your environment's fault, it looks like it's mine :) i apparently don't understand all this async/await stuff, not sure why I thought I could push it like this!

I validated this in-browser that it works, but i gotta say I don't
really understand the async/await stuff in JS still :|
Ryan Rix and others added 2 commits September 8, 2021 15:47
checks for any form of TruE TrUe etc, and is probably more legible
this way even if it's a bit more repetetetitive
@moates
Copy link
Contributor

moates commented Sep 9, 2021

@rrix I added views/faq.mustache with the approved copy and markup.

I started to add the routing logic myself but got a little confused about where to best handle that so it wouldn't get caught in study-full, so I'll leave that to you, or ask you for a pointer.

@rrix
Copy link
Author

rrix commented Sep 10, 2021

thanks for pushing the copy, i added GET /faq in 3e37a2f

@moates moates self-requested a review September 10, 2021 21:12
Copy link
Contributor

@moates moates left a comment

Choose a reason for hiding this comment

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

Built this locally, checked out the FAQ, tested external links! Thank you!

@moates moates merged commit b91f56d into consumerreports:main Sep 10, 2021
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.

3 participants