Skip to content

Commit

Permalink
DX: Split out feature flags from the Me user data endpoint (#7734)
Browse files Browse the repository at this point in the history
* split out feature flags from Me endpoint

* add new file to null checks

* fix tests

* Update src/auth/featureFlags.ts

Co-authored-by: Graham Langford <30706330+grahamlangford@users.noreply.github.com>

* add comment back

* clean up

* add tests

* add test file to null checks

* some pr feedback

* reset feature flags on auth change

* make sure flag is cleared

* convert to use webext-storage-cache

* fix jest config, fix storage override, update snapshots

* package lock changes?

* move feature flag fetch to background, and add useFlags tests

* fix manifest snapshot

* fix package lock

* Add CI check

* Add unique identifiers

* restore lockfile

* /2

* Automatically hide comment once resolved

* Test strictNullChecks CI check for messenger

* Drop CI workflows from this PR, move to their own PRs

* `fetchFeatureFlags` - Use readonly array

* `fetchFeatureFlags` - Drop redundant HTTP status check

Axios already throws on HTTP errors

* remove maxAge/expiry timeouts

* package lock

* fix strict null checks with messenger call

* remove test function

* fix import and add file to null checks

---------

Co-authored-by: Ben Loe <ben@pixiebrix.com>
Co-authored-by: Graham Langford <30706330+grahamlangford@users.noreply.github.com>
Co-authored-by: Todd Schiller <todd.schiller@gmail.com>
Co-authored-by: Federico Brigante <me@fregante.com>
  • Loading branch information
5 people committed Feb 28, 2024
1 parent cbd7760 commit c50ddca
Show file tree
Hide file tree
Showing 64 changed files with 547 additions and 188 deletions.
4 changes: 2 additions & 2 deletions eslint-local-rules/persistBackgroundData.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
./src/analysis/analysisVisitors/regexAnalysis.ts
./src/analysis/analysisVisitors/varAnalysis/varMap.ts
./src/auth/authConstants.ts
./src/auth/authStorage.ts
./src/auth/authTypes.ts
./src/auth/authUtils.ts
./src/auth/token.ts
./src/auth/featureFlags.ts
./src/components/annotationAlert/FieldAnnotationAlert.tsx
./src/components/AsyncButton.tsx
./src/components/DelayedRender.tsx
Expand Down Expand Up @@ -255,7 +256,6 @@
./src/store/settings/settingsStorage.ts
./src/store/settings/settingsTypes.ts
./src/store/StorageInterface.ts
./src/store/syncFlags.ts
./src/telemetry/BackgroundLogger.ts
./src/telemetry/deployments.ts
./src/telemetry/dnt.ts
Expand Down
2 changes: 1 addition & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const config = {
"^.+\\.txt$": "<rootDir>/src/testUtils/rawJestTransformer.mjs",
},
transformIgnorePatterns: [
"node_modules/(?!@cfworker|escape-string-regex|filename-reserved-regex|filenamify|idb|webext-|p-timeout|p-retry|p-defer|p-memoize|serialize-error|strip-outer|trim-repeated|mimic-fn|urlpattern-polyfill|url-join|uuid|nanoid|use-debounce|copy-text-to-clipboard|linkify-urls|create-html-element|stringify-attributes|escape-goat|stemmer|uint8array-extras|one-event|abort-utils|batched-function|is-network-error|text-field-edit)",
"node_modules/(?!@cfworker|escape-string-regex|filename-reserved-regex|filenamify|idb|webext-|p-timeout|p-retry|p-defer|p-memoize|serialize-error|strip-outer|trim-repeated|mimic-fn|urlpattern-polyfill|url-join|uuid|nanoid|use-debounce|copy-text-to-clipboard|linkify-urls|create-html-element|stringify-attributes|escape-goat|stemmer|uint8array-extras|one-event|abort-utils|batched-function|is-network-error|text-field-edit|webext-storage-cache|@sindresorhus/to-milliseconds)",
],
setupFiles: [
"dotenv/config",
Expand Down
45 changes: 45 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
"webext-permissions": "^3.1.2",
"webext-polyfill-kinda": "^1.0.2",
"webext-storage": "^1.2.2",
"webext-storage-cache": "^6.0.0",
"webext-tools": "^1.2.3",
"webextension-polyfill": "^0.10.0",
"whatwg-mimetype": "^4.0.0",
Expand Down
2 changes: 2 additions & 0 deletions scripts/__snapshots__/manifest.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/auth/RequireAuth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import React, { useEffect } from "react";
import { useDispatch, useSelector } from "react-redux";
import Loader from "@/components/Loader";
import { useGetMeQuery } from "@/data/service/api";
import { clearCachedAuthSecrets, updateUserData } from "@/auth/token";
import { clearCachedAuthSecrets, updateUserData } from "@/auth/authStorage";
import {
selectExtensionAuthState,
selectUserDataUpdate,
Expand Down
1 change: 0 additions & 1 deletion src/auth/authConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export const anonAuth: AuthState = Object.freeze({
isTestAccount: false,
extension: true,
scope: null,
flags: [],
milestones: [],
organizations: [],
groups: [],
Expand Down
1 change: 0 additions & 1 deletion src/auth/authSelectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export const selectAuth = (state: AuthRootState) => state.auth;
export const selectIsLoggedIn = (state: AuthRootState) =>
selectAuth(state).isLoggedIn;
export const selectScope = (state: AuthRootState) => selectAuth(state).scope;
export const selectFlags = (state: AuthRootState) => selectAuth(state).flags;
export const selectMilestones = (state: AuthRootState) =>
selectAuth(state).milestones;
export const selectOrganizations = (state: AuthRootState) =>
Expand Down
1 change: 0 additions & 1 deletion src/auth/authSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export const authSlice = createSlice({
return {
...payload,
scope: isEmpty(payload.scope) ? null : payload.scope,
flags: Array.isArray(payload.flags) ? payload.flags : [],
organizations: Array.isArray(payload.organizations)
? payload.organizations
: [],
Expand Down
File renamed without changes.
10 changes: 0 additions & 10 deletions src/auth/authTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ export type UserData = Partial<{
* The user's organization for engagement and error attribution
*/
telemetryOrganizationId: UUID | null;
/**
* Feature flags
*/
flags: string[];
/**
* Organizations the user is a member of
*/
Expand Down Expand Up @@ -108,7 +104,6 @@ export const USER_DATA_UPDATE_KEYS: Array<keyof UserDataUpdate> = [
"telemetryOrganizationId",
"organizations",
"groups",
"flags",
"enforceUpdateMillis",
"partner",
"partnerPrincipals",
Expand Down Expand Up @@ -235,11 +230,6 @@ export type AuthState = {
name: string;
}>;

/**
* List of feature flags for the user.
*/
readonly flags: string[];

/**
* List of milestones for the user. A Milestone represents progress through the PixieBrix product.
*/
Expand Down
14 changes: 0 additions & 14 deletions src/auth/authUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import { type Me } from "@/types/contract";
import { type UserDataUpdate, type AuthState } from "@/auth/authTypes";
import { readAuthData } from "@/auth/token";

// Used by the app
function selectOrganizations(
Expand Down Expand Up @@ -52,7 +51,6 @@ export function selectUserDataUpdate({
telemetry_organization: telemetryOrganization,
organization_memberships: organizationMemberships = [],
group_memberships = [],
flags = [],
partner,
enforce_update_millis: enforceUpdateMillis,
partner_principals: partnerPrincipals = [],
Expand All @@ -67,7 +65,6 @@ export function selectUserDataUpdate({
email: email!,
organizationId: organization?.id ?? null,
telemetryOrganizationId: telemetryOrganization?.id ?? null,
flags,
organizations,
groups,
partner: partner ?? null,
Expand All @@ -84,7 +81,6 @@ export function selectExtensionAuthState({
telemetry_organization,
is_onboarded: isOnboarded,
test_account: isTestAccount = false,
flags = [],
milestones = [],
organization_memberships: organizationMemberships = [],
group_memberships = [],
Expand All @@ -106,18 +102,8 @@ export function selectExtensionAuthState({
telemetryOrganizationId: telemetry_organization?.id,
organizations,
groups,
flags,
milestones,
partner,
enforceUpdateMillis,
};
}

/**
* Returns true if the specified flag is on for the current user.
* @param flag the feature flag to check
*/
export async function flagOn(flag: string): Promise<boolean> {
const authData = await readAuthData();
return authData.flags?.includes(flag) ?? false;
}
112 changes: 112 additions & 0 deletions src/auth/featureFlagStorage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Copyright (C) 2024 PixieBrix, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import {
fetchFeatureFlags,
flagOn,
resetFeatureFlags,
TEST_overrideFeatureFlags,
} from "@/auth/featureFlagStorage";
import { appApiMock } from "@/testUtils/appApiMock";
import { TEST_setAuthData, TEST_triggerListeners } from "@/auth/authStorage";
import { tokenAuthDataFactory } from "@/testUtils/factories/authFactories";
import { fetchFeatureFlagsInBackground } from "@/background/messenger/strict/api";

describe("featureFlags", () => {
beforeEach(async () => {
// Wire up the real fetch function so we can mock the api responses
jest
.mocked(fetchFeatureFlagsInBackground)
.mockImplementation(fetchFeatureFlags);
appApiMock.reset();
appApiMock.onGet("/api/me/").reply(200, {
flags: [],
});
});

afterEach(async () => {
await resetFeatureFlags();
});

it("returns true if flag is present", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags([
"test-flag",
"test-other-flag",
"test-other-flag-2",
]);
await expect(flagOn("test-flag")).resolves.toBe(true);
});

it("returns false if flag is not present", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags(["test-other-flag", "test-other-flag-2"]);
await expect(flagOn("test-flag")).resolves.toBe(false);
});

it("fetches flags on initial storage state", async () => {
appApiMock.onGet("/api/me/").reply(200, {
flags: ["test-flag"],
});

await expect(flagOn("test-flag")).resolves.toBe(true);
expect(appApiMock.history.get).toHaveLength(1);
});

it("does not fetch if flags have been updated recently", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags(["test-flag"]);
await expect(flagOn("test-flag")).resolves.toBe(true);
expect(appApiMock.history.get).toHaveLength(0);
});

it("only fetches once if multiple calls are made", async () => {
appApiMock.onGet("/api/me/").reply(200, {
flags: ["test-flag"],
});

await expect(flagOn("test-flag")).resolves.toBe(true);
await expect(flagOn("test-flag")).resolves.toBe(true);
await expect(flagOn("test-flag")).resolves.toBe(true);
expect(appApiMock.history.get).toHaveLength(1);
});

it("fetches flags again if auth is reset in between calls", async () => {
appApiMock.onGet("/api/me/").reply(200, {
flags: ["test-flag", "secret-flag"],
});

await expect(flagOn("secret-flag")).resolves.toBe(true);
await expect(flagOn("secret-flag")).resolves.toBe(true);
expect(appApiMock.history.get).toHaveLength(1);

const authData = tokenAuthDataFactory();
// eslint-disable-next-line new-cap
await TEST_setAuthData(authData);
// eslint-disable-next-line new-cap
TEST_triggerListeners(authData);

// New user doesn't have secret flag
appApiMock.onGet("/api/me/").reply(200, {
flags: ["test-flag"],
});

await expect(flagOn("secret-flag")).resolves.toBe(false);
await expect(flagOn("secret-flag")).resolves.toBe(false);
expect(appApiMock.history.get).toHaveLength(2);
});
});

0 comments on commit c50ddca

Please sign in to comment.