-
-
Notifications
You must be signed in to change notification settings - Fork 255
feat: add override functionality to remote feature flags #7271
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
base: main
Are you sure you want to change the base?
Changes from all commits
bec04d7
e78270a
fe7db8b
4413b2d
e07e67c
ec17188
fbdbd14
c9cd5aa
4d02e47
e3a4f8f
35b8470
4201e40
7ae47a3
71e4b7a
ca479c8
0063bce
9bbf745
414f0f0
f17c947
4d349ed
6a6b98f
7e3500d
434c892
6c75509
e562809
a09ceaa
ffd58e9
c1af969
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
||||
| ## [Unreleased] | ||||
|
|
||||
| ### Added | ||||
|
|
||||
| - Add override functionality to remote feature flags ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
| - `setFlagOverride(flagName, value)` - Set a local override for a specific feature flag | ||||
| - `clearFlagOverride(flagName)` - Clear the local override for a specific feature flag | ||||
| - `clearAllOverrides()` - Clear all local feature flag overrides | ||||
| - Add new controller state properties ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
| - `localOverrides` - Local overrides for feature flags that take precedence over remote flags | ||||
| - `rawRemoteFeatureFlags` - Raw flag value for arrays that were processed from arrays to single value | ||||
| - Export additional controller action types ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
| - `RemoteFeatureFlagControllerSetFlagOverrideAction` | ||||
| - `RemoteFeatureFlagControllerClearFlagOverrideAction` | ||||
| - `RemoteFeatureFlagControllerClearAllOverridesAction` | ||||
|
|
||||
| ### Changed | ||||
|
|
||||
| - Enhanced feature flag processing to preserve raw A/B test arrays for visibility and override capabilities ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... This entry — and the category that this entry is in — make it sound like there are changes to the feature flag processing logic. But I don't see that happening — all I see are a few new methods. Unless this refers to the fact that
Suggested change
|
||||
| - Local overrides now take precedence over remote feature flags when both exist ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this covered by "Add override functionality to remote feature flags"? Perhaps this should be moved to a bullet point under that entry:
Suggested change
|
||||
| - Controller state now includes metadata for new properties with appropriate persistence and logging settings ([#7271](https://github.com/MetaMask/core/pull/7271)) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this covered by "Add new controller state properties"? Perhaps we don't need it.
Suggested change
|
||||
|
|
||||
| ## [3.0.0] | ||||
|
|
||||
| ### Added | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,3 +51,25 @@ export type ServiceResponse = { | |
| remoteFeatureFlags: FeatureFlags; | ||
| cacheTimestamp: number | null; | ||
| }; | ||
|
|
||
| /** | ||
| * Describes the shape of the state object for the {@link RemoteFeatureFlagController}. | ||
| */ | ||
| export type RemoteFeatureFlagControllerState = { | ||
| /** | ||
| * The collection of feature flags and their respective values, which can be objects. | ||
| */ | ||
| remoteFeatureFlags: FeatureFlags; | ||
| /** | ||
| * Local overrides for feature flags that take precedence over remote flags. | ||
| */ | ||
| localOverrides: FeatureFlags; | ||
| /** | ||
| * Raw A/B test flag arrays for flags that were processed from arrays to single values. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this always true? Will feature flags always contain A/B test arrays from now on? There don't seem to be any changes to the |
||
| */ | ||
| rawRemoteFeatureFlags: FeatureFlags; | ||
| /** | ||
| * The timestamp of the last successful feature flag cache. | ||
| */ | ||
| cacheTimestamp: number; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import type { | |||||||||||||||||||||||||
| import type { AbstractClientConfigApiService } from './client-config-api-service/abstract-client-config-api-service'; | ||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
| RemoteFeatureFlagController, | ||||||||||||||||||||||||||
| controllerName, | ||||||||||||||||||||||||||
| DEFAULT_CACHE_DURATION, | ||||||||||||||||||||||||||
| getDefaultRemoteFeatureFlagControllerState, | ||||||||||||||||||||||||||
| } from './remote-feature-flag-controller'; | ||||||||||||||||||||||||||
|
|
@@ -18,8 +19,6 @@ import type { | |||||||||||||||||||||||||
| } from './remote-feature-flag-controller'; | ||||||||||||||||||||||||||
| import type { FeatureFlags } from './remote-feature-flag-controller-types'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const controllerName = 'RemoteFeatureFlagController'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const MOCK_FLAGS: FeatureFlags = { | ||||||||||||||||||||||||||
| feature1: true, | ||||||||||||||||||||||||||
| feature2: { chrome: '<109' }, | ||||||||||||||||||||||||||
|
|
@@ -88,6 +87,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state).toStrictEqual({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| localOverrides: {}, | ||||||||||||||||||||||||||
| rawRemoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| cacheTimestamp: 0, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
@@ -97,6 +98,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state).toStrictEqual({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| localOverrides: {}, | ||||||||||||||||||||||||||
| rawRemoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| cacheTimestamp: 0, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
@@ -105,6 +108,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| const customState = { | ||||||||||||||||||||||||||
| remoteFeatureFlags: MOCK_FLAGS_TWO, | ||||||||||||||||||||||||||
| cacheTimestamp: 123456789, | ||||||||||||||||||||||||||
| rawRemoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| localOverrides: {}, | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const controller = createController({ state: customState }); | ||||||||||||||||||||||||||
|
|
@@ -640,11 +645,154 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| it('should return default state', () => { | ||||||||||||||||||||||||||
| expect(getDefaultRemoteFeatureFlagControllerState()).toStrictEqual({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| localOverrides: {}, | ||||||||||||||||||||||||||
| rawRemoteFeatureFlags: {}, | ||||||||||||||||||||||||||
| cacheTimestamp: 0, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe('override functionality', () => { | ||||||||||||||||||||||||||
| describe('setFlagOverride', () => { | ||||||||||||||||||||||||||
| it('sets a local override for a feature flag', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('testFlag', true); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| testFlag: true, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it('overwrites existing override for the same flag', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('testFlag', true); | ||||||||||||||||||||||||||
|
Comment on lines
+668
to
+670
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of calling the method to prepare the state, and then calling it again to test the intended behavior, what are your thoughts on passing initial state to
Suggested change
|
||||||||||||||||||||||||||
| controller.setFlagOverride('testFlag', false); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| testFlag: false, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it('preserves other overrides when setting a new one', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('flag1', 'value1'); | ||||||||||||||||||||||||||
|
Comment on lines
+679
to
+681
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar suggestion as above:
Suggested change
|
||||||||||||||||||||||||||
| controller.setFlagOverride('flag2', 'value2'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| flag1: 'value1', | ||||||||||||||||||||||||||
| flag2: 'value2', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe('clearFlagOverride', () => { | ||||||||||||||||||||||||||
| it('removes a specific override', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('flag1', 'value1'); | ||||||||||||||||||||||||||
| controller.setFlagOverride('flag2', 'value2'); | ||||||||||||||||||||||||||
|
Comment on lines
+693
to
+696
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar suggestion as above. We are testing
Suggested change
|
||||||||||||||||||||||||||
| controller.clearFlagOverride('flag1'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| flag2: 'value2', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it('does not affect state when clearing non-existent override', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('flag1', 'value1'); | ||||||||||||||||||||||||||
|
Comment on lines
+705
to
+707
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar:
Suggested change
|
||||||||||||||||||||||||||
| controller.clearFlagOverride('nonExistentFlag'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| flag1: 'value1', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe('clearAllOverrides', () => { | ||||||||||||||||||||||||||
| it('removes all overrides', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.setFlagOverride('flag1', 'value1'); | ||||||||||||||||||||||||||
| controller.setFlagOverride('flag2', 'value2'); | ||||||||||||||||||||||||||
|
Comment on lines
+718
to
+721
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar:
Suggested change
|
||||||||||||||||||||||||||
| controller.clearAllOverrides(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({}); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it('does not affect state when no overrides exist', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| controller.clearAllOverrides(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({}); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe('integration with remote flags', () => { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that with these changes we are splitting up the tests for |
||||||||||||||||||||||||||
| it('preserves overrides when remote flags are updated', async () => { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a large number of steps represented here for what I think ought to be a simple test (given my reading of the logic). Similar to other suggestions I've made, is it enough to set up the initial state instead of having to call a few methods? |
||||||||||||||||||||||||||
| const clientConfigApiService = buildClientConfigApiService({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: { remoteFlag: 'initialRemoteValue' }, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| const controller = createController({ clientConfigApiService }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Set override before fetching remote flags | ||||||||||||||||||||||||||
| controller.setFlagOverride('overrideFlag', 'overrideValue'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| await controller.updateRemoteFeatureFlags(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Mock different remote response for second fetch | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mentions that we are mocking the request for the second time, but where are we mocking the request for the first time? |
||||||||||||||||||||||||||
| jest | ||||||||||||||||||||||||||
| .spyOn(clientConfigApiService, 'fetchRemoteFeatureFlags') | ||||||||||||||||||||||||||
| .mockResolvedValueOnce({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: { remoteFlag: 'updatedRemoteValue' }, | ||||||||||||||||||||||||||
| cacheTimestamp: Date.now(), | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Force cache expiration and update | ||||||||||||||||||||||||||
| jest | ||||||||||||||||||||||||||
| .spyOn(Date, 'now') | ||||||||||||||||||||||||||
| .mockReturnValue(Date.now() + DEFAULT_CACHE_DURATION + 1000); | ||||||||||||||||||||||||||
| await controller.updateRemoteFeatureFlags(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Overrides should be preserved | ||||||||||||||||||||||||||
| expect(controller.state.localOverrides).toStrictEqual({ | ||||||||||||||||||||||||||
| overrideFlag: 'overrideValue', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Remote flags should be updated | ||||||||||||||||||||||||||
| expect(controller.state.remoteFeatureFlags).toStrictEqual({ | ||||||||||||||||||||||||||
| remoteFlag: 'updatedRemoteValue', | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it('overrides work with threshold-based feature flags', async () => { | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this test testing? If it's merely testing that overrides do not change when |
||||||||||||||||||||||||||
| const clientConfigApiService = buildClientConfigApiService({ | ||||||||||||||||||||||||||
| remoteFeatureFlags: MOCK_FLAGS_WITH_THRESHOLD, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| const controller = createController({ | ||||||||||||||||||||||||||
| clientConfigApiService, | ||||||||||||||||||||||||||
| getMetaMetricsId: () => MOCK_METRICS_ID, | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| await controller.updateRemoteFeatureFlags(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Override the threshold flag | ||||||||||||||||||||||||||
| controller.setFlagOverride('testFlagForThreshold', 'overriddenValue'); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Access flag value with override taking precedence | ||||||||||||||||||||||||||
| const flagValue = | ||||||||||||||||||||||||||
| controller.state.localOverrides.testFlagForThreshold ?? | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we using |
||||||||||||||||||||||||||
| controller.state.remoteFeatureFlags.testFlagForThreshold; | ||||||||||||||||||||||||||
| expect(flagValue).toBe('overriddenValue'); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe('metadata', () => { | ||||||||||||||||||||||||||
| it('includes expected state in debug snapshots', () => { | ||||||||||||||||||||||||||
| const controller = createController(); | ||||||||||||||||||||||||||
|
|
@@ -658,6 +806,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| ).toMatchInlineSnapshot(` | ||||||||||||||||||||||||||
| Object { | ||||||||||||||||||||||||||
| "cacheTimestamp": 0, | ||||||||||||||||||||||||||
| "localOverrides": Object {}, | ||||||||||||||||||||||||||
| "rawRemoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| "remoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| `); | ||||||||||||||||||||||||||
|
|
@@ -675,6 +825,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| ).toMatchInlineSnapshot(` | ||||||||||||||||||||||||||
| Object { | ||||||||||||||||||||||||||
| "cacheTimestamp": 0, | ||||||||||||||||||||||||||
| "localOverrides": Object {}, | ||||||||||||||||||||||||||
| "rawRemoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| "remoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| `); | ||||||||||||||||||||||||||
|
|
@@ -692,6 +844,8 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| ).toMatchInlineSnapshot(` | ||||||||||||||||||||||||||
| Object { | ||||||||||||||||||||||||||
| "cacheTimestamp": 0, | ||||||||||||||||||||||||||
| "localOverrides": Object {}, | ||||||||||||||||||||||||||
| "rawRemoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| "remoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| `); | ||||||||||||||||||||||||||
|
|
@@ -708,6 +862,7 @@ describe('RemoteFeatureFlagController', () => { | |||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ).toMatchInlineSnapshot(` | ||||||||||||||||||||||||||
| Object { | ||||||||||||||||||||||||||
| "localOverrides": Object {}, | ||||||||||||||||||||||||||
| "remoteFeatureFlags": Object {}, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| `); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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 relates to the comment I made in
RemoteFeatureFlagController.ts— perhaps this is not quite accurate?