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
Fix the feature flags in production on hydration #4084
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/4084 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
It turns out I didn't understand these instructions correctly:
I was just running the app using build and start, but did not pass The error shows up for me now, so I'll get back to testing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly LGTM. I was nervous about this change when I saw the line count, but in reviewing the code, I see that it is mostly moving things around, rather than actually making changes to the logic in any way.
As I mentioned private, I still think it is best not to deploy new features to production in the second half of the week, and I did think this was our agreed approach (which is why we target Tuesdays for releases).
However, I am requesting changes, because one of the tests added in prefereces.spec.ts
is incorrect. The requested change is either to (a) also fix the checkbox bug in this PR, (b) fix the checkbox bug in another PR that blocks this one, or (c) remove that test from this PR, fix the checkbox bug in a critical a11y PR, and in that PR add the test back.
The test as it stands should not be committed to main, though, no more than any other factually and logically incorrect test should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to request changes, no approve.
1b672a0
to 9ce309b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the changes work as expected locally. I am able to toggle all the preferences and see their effect in the app as well as in the cookies.
9ce309b
to b2be0b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good 👍 Looking forward to a future approach where we do not need to specify a list of feature flag values in frontend/test/playwright/utils/navigation.ts
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
b2be0b9
to 8ab1495
Compare
Fixes
Fixes #4082 by @obulat
Description
The feature flag store was using the env variable to get the deployment environment and the flag state for this environment. However, the env variable is not available on the client. I moved the setting to
publicRuntimeConfig
. To use the config in the feature flag store functions (getFlagStatus
,getFeatureState
), I had to move them inside the store itself. Becausethis
was not available in some getters that used these functions, I converted them to actions.I also updated the e2e setup to only set the feature cookies when we explicitly set some feature state in
preparePageForTests
, otherwise we don't set it to not change the default state.Testing Instructions
On main, run the app in production mode using
just frontend/run build
and thenDEPLOYMENT_ENV=production just frontend/run start
.Go to http://localhost:8443/preferences and open console. You should see the errors mentioned in the linked issue in the console. When you attempt to click on the switchable input, the inner circle disappears.
Do the same in this branch, and you should be able to change the state of the switchable flags without errors. You can check the cookie state to confirm the flag was set. You can also see that additional search views are visible when you turn them on.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin