[Issue #2695] opportunity and search toggle with feature flag env vars #3115

doug-s-nava · 2024-12-05T19:17:24Z

Summary

Time to review: 30 mins

Changes proposed

The goal of the PR is the addition of two "kill switch" parameters that can be used to direct the search and opportunity pages, respectively, to a maintenance page. These parameters can be supplied either on the frontend via query params or feature flags admin panel, or via environment variables.

Setting flags via environment variables is a new functionality, so changes were made to the system to get this to work. Some deficiencies of the existing system were also patched up as part of this process.

Context for reviewers

Test Steps

FEATURE_OPPORTUNITY_OFF=true FEATURE_SEARCH_OFF=true npm run dev on this branch
visit http://localhost:3000/search
VERIFY: maintenance page appears
VERIFY: maintenance page matches designs and includes correct text from spec
visit http://localhost:3000/opportunity/1
VERIFY: maintenance page appears
VERIFY: maintenance page matches designs and includes correct text from spec
visit http://localhost:3000/search?_ff=searchOff%3Afalse
VERIFY: search page appears
1. visit http://localhost:3000/opportunity/1?_ff=opportunityOff%3Afalse
VERIFY: opportunity page appears
Repeat 2 - x but in faux prod mode with npm run build && FEATURE_OPPORTUNITY_OFF=true FEATURE_SEARCH_OFF=true npm start

Dockerized

I would like to be able to test the changes to the Dockerfile locally, but can't without creating a docker compose that runs the production Next build and start rather than next dev. I feel like that's out of scope for this change, so we should just deploy out to DEV and test there.

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

doug-s-nava · 2024-12-09T15:52:41Z

frontend/src/services/FeatureFlagManager.ts

  get featureFlags(): FeatureFlags {
    return {
      ...this.defaultFeatureFlags,
+      ...this.featureFlagsFromEnvironment,


note that this means that if a user in any way sets their cookie to override the flag, they'll be able to access search or opportunities

doug-s-nava · 2024-12-09T16:04:07Z

frontend/src/services/FeatureFlagManager.ts

    if (!this.isValidFeatureFlag(featureName)) {
      throw new Error(`\`${featureName}\` is not a valid feature flag`);
    }
    const featureFlags = {
-      ...this.featureFlags,
+      ...this.featureFlagsCookie,


this is a change to the existing functionality - previously whenever we set the feature flags cookie it would enshrine the default feature flag values in the cookie, but that functionality conflicts with idea of honoring flags set in the environment and using cookies as an override. Setting a flag on the cookie should, in this case, only set the single flag value specified

doug-s-nava · 2024-12-09T20:40:13Z

frontend/package.json

@@ -8,7 +8,7 @@
  "scripts": {
    "all-checks": "npm run test && npm run lint && npm run ts:check && npm run build",
    "build": "next build",
-    "dev": "next dev",
+    "dev": "NEW_RELIC_ENABLED=false next dev",


unrelated but makes local development logs less busy

doug-s-nava · 2024-12-09T20:41:05Z

frontend/src/app/[locale]/opportunity/[id]/page.tsx

 export const revalidate = 600; // invalidate ten minutes
+export const dynamic = "force-dynamic";


I honestly don't understand why this needs to be here since we are already including the generateStaticParams, but without the the production build fails to render the page due to the dependence on searchParams in the withFeatureFlag code

doug-s-nava · 2024-12-09T20:41:53Z

frontend/src/constants/featureFlags.ts

  // This should be removed when the search page goes live, before May 2024
-  showSearchV0: true,
+  hideSearchV0: false,


do we still need this? I changed it to a hide vs show variable to match the pattern here and allow us to more easily reason about feature flag effects

I think we could drop this at this point. Or we could try to generalize it as like searchBeta that we can keep rolling things out using and then remove the flag checks as we consider those things Released.

I would remove it. When I turned it on locally it created a 500 error or the search page.

mdragon

Looks good.

mdragon · 2024-12-10T21:44:29Z

frontend/src/constants/featureFlags.ts

  // This should be removed when the search page goes live, before May 2024
-  showSearchV0: true,
+  hideSearchV0: false,


I think we could drop this at this point. Or we could try to generalize it as like searchBeta that we can keep rolling things out using and then remove the flag checks as we consider those things Released.

acouch

Tested locally, looks great.

doug-s-nava force-pushed the dschrashun/2695-opportunity-kill-switch branch from be196a6 to 9f4febc Compare December 9, 2024 14:30

doug-s-nava commented Dec 9, 2024

View reviewed changes

doug-s-nava changed the title ~~Dschrashun/2695 opportunity kill switch~~ [Issue #2695] opportunity and search toggle with feature flag env vars Dec 9, 2024

doug-s-nava marked this pull request as ready for review December 9, 2024 20:43

doug-s-nava requested review from coilysiren, acouch, mdragon and andycochran as code owners December 9, 2024 20:43

doug-s-nava force-pushed the dschrashun/2695-opportunity-kill-switch branch from db3d154 to ddddffa Compare December 10, 2024 14:10

mdragon previously approved these changes Dec 11, 2024

View reviewed changes

acouch previously approved these changes Dec 11, 2024

View reviewed changes

doug-s-nava added 15 commits December 11, 2024 16:34

set groundwork

aca1a71

redirect to placeholder maintanence page when flag is set

422778b

this works for opportunities but needs to be cleaned up / refactored

3efd4ff

cleanup

b7d7161

finish maintenance page

b50c042

type fix

ba8072d

add maintenance meta

a074046

not working! search params not coming through for opportunity page

d9d0680

set proper precedence of env var values

1627df6

explain flag precedence

343d3f2

dockerfile changes and cleanup

34798d9

more cleanup

cb9912f

lint and type fixes

6d90482

add and fix tests

54d8b15

have to add an explict to get this to work

56867aa

doug-s-nava dismissed stale reviews from acouch and mdragon via 14c5623 December 11, 2024 21:37

doug-s-nava force-pushed the dschrashun/2695-opportunity-kill-switch branch from ddddffa to 14c5623 Compare December 11, 2024 21:37

remove search v0 flag

ce959ae

doug-s-nava force-pushed the dschrashun/2695-opportunity-kill-switch branch from 14c5623 to ce959ae Compare December 11, 2024 21:41

mdragon approved these changes Dec 11, 2024

View reviewed changes

doug-s-nava merged commit 7a7b8a8 into main Dec 11, 2024
19 checks passed

doug-s-nava deleted the dschrashun/2695-opportunity-kill-switch branch December 11, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #2695] opportunity and search toggle with feature flag env vars #3115

[Issue #2695] opportunity and search toggle with feature flag env vars #3115

doug-s-nava commented Dec 5, 2024 •

edited

Loading

doug-s-nava Dec 9, 2024

doug-s-nava Dec 9, 2024

doug-s-nava Dec 9, 2024

doug-s-nava Dec 9, 2024

doug-s-nava Dec 9, 2024

mdragon Dec 10, 2024

acouch Dec 11, 2024

mdragon left a comment

mdragon Dec 10, 2024

acouch left a comment

		export const revalidate = 600; // invalidate ten minutes
		export const dynamic = "force-dynamic";

[Issue #2695] opportunity and search toggle with feature flag env vars #3115

[Issue #2695] opportunity and search toggle with feature flag env vars #3115

Conversation

doug-s-nava commented Dec 5, 2024 • edited Loading

Summary

Time to review: 30 mins

Changes proposed

Context for reviewers

Test Steps

Dockerized

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acouch left a comment

Choose a reason for hiding this comment

doug-s-nava commented Dec 5, 2024 •

edited

Loading