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

Adds Posthog support to the frontend. #693

Merged
merged 32 commits into from
Apr 5, 2024
Merged

Conversation

jkachel
Copy link
Contributor

@jkachel jkachel commented Mar 28, 2024

What are the relevant tickets?

Closes #589

Description (What does it do?)

Adds PostHog integration to the frontend with these settings:

  • The timeout is configurable in the .env file
  • The PostHog provider is pre-loaded with the feature flags the Django app knows about
  • Enabling the provider can also be set in the .env file.

The PostHog provider is situated near the top of the provider stack for the app - right after StrictMode - which should allow for feature flag checking and event collection for all parts of the app.

How can this be tested?

Automated tests should pass.

Test setup of the PostHog provider: If the POSTHOG_ENABLED flag is set to True, you should see the PostHogProvider in the Components tree in the React dev tools. You should not see it if the flag is set to False.

Test pre-loading of flags: With the provider enabled, you should be able to see the flags that the Django app knows about pre-loaded into the provider by navigating to the provider component in React dev tools and looking through the options. The preloading will use the currently logged-in user's username as the unique user ID for retrieving flags. Doing it this way should also load the flags from cache (see PR #682) if there's a cached value.

Copy link

gitguardian bot commented Mar 28, 2024

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9430286 Triggered Generic Password 0218f76 docker-compose.yml View secret
9430286 Triggered Generic Password 5ac97af docker-compose.yml View secret
9430286 Triggered Generic Password 392893b docker-compose.yml View secret
9430286 Triggered Generic Password b838810 docker-compose.yml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@jkachel jkachel force-pushed the jkachel/posthog-frontend-support branch from 0ff97b1 to 3380595 Compare April 1, 2024 16:09
@jkachel jkachel force-pushed the jkachel/posthog-frontend-support branch from a2b3247 to 6e2903d Compare April 1, 2024 16:58
@jkachel jkachel marked this pull request as ready for review April 1, 2024 17:15
@jkachel jkachel added Needs Review An open Pull Request that is ready for review product:mit-open Issues related to the MIT Open product labels Apr 1, 2024
Comment on lines 24 to 25
const resources = factory.resources({ count: 4 })
setMockResponse.get(urls.learningResources.list(), resources)
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Apr 2, 2024

Choose a reason for hiding this comment

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

renderTestApp renders the whole app, which makes some API calls, you you had to mock, which is a pain.

I would recommend using renderWithProviders for this:

renderWithProviders(<div data-testid="some-children" />)

then you won't need to mock the API calls. (renderTestApp uses renderWithProviders internally)

@jkachel jkachel force-pushed the jkachel/posthog-frontend-support branch from c2011e0 to 4c987a7 Compare April 3, 2024 14:28
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

main/views.py Outdated
Comment on lines 36 to 41
"posthog": {
"api_key": settings.POSTHOG_PROJECT_API_KEY,
"enabled": settings.POSTHOG_ENABLED,
"timeout": settings.POSTHOG_TIMEOUT_MS,
"bootstrap_flags": all_flags,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently @shanbady did some work to remove environment settings from the django view. Instead, any env variables that the frontend needs can be injected via the webpack build: https://github.com/mitodl/mit-open/blob/87fa1ad3e93905a391ad7cbb18c09fb1ed771904/frontends/mit-open/webpack.config.js#L87-L99

This is part of some work to make the HTML static so that it can be served by a CDN.

Copy link
Contributor

Choose a reason for hiding this comment

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

(user is still injected via django, Jon is doing some work to stop that and get user info via an api)

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 changed this to not depend on Django injection at all - however this does change how the flag bootstrapping works; instead of being able to pull from the durable cache present there, it'll use the settings in the .env file instead. The default is for it to look at items that begin with FEATURE_ and this is configurable by setting MITOPEN_FEATURES_PREFIX (which will also change it in the Django app). It doesn't do any processing of the feature flag name other than stripping the prefix so it's up to us to make sure these agree between the app and the PostHog settings. (The code does normalize a Python "True" into a JavaScript true - other values are left as-is.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove the posthog HTML injection all together now?

With this, the frontend won't pull feature flag data from the Django cache as it won't have access to that anymore. You can, however, set the same feature flags in the .env file as "FEATURE_<flag name>". Boolean flags set using Python convention (i.e. True) will be set to JavaScript true; anything else will be stringified.
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍

One request, but IMO no need to re-review

main/views.py Outdated
Comment on lines 36 to 41
"posthog": {
"api_key": settings.POSTHOG_PROJECT_API_KEY,
"enabled": settings.POSTHOG_ENABLED,
"timeout": settings.POSTHOG_TIMEOUT_MS,
"bootstrap_flags": all_flags,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove the posthog HTML injection all together now?

Comment on lines +53 to +55
<PostHogProvider apiKey={phSettings.api_key} options={phOptions}>
{interiorElements}
</PostHogProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if PostHog's provider had an enabled prop that made it a no-op of enabled=false.

@jkachel jkachel merged commit 170909c into main Apr 5, 2024
12 checks passed
@jkachel jkachel deleted the jkachel/posthog-frontend-support branch April 5, 2024 18:27
This was referenced Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review product:mit-open Issues related to the MIT Open product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature flag support (Posthog)
3 participants