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

[FSSDK-9654] fix: Default to VUID without user(id) as Provider prop #229

Merged
merged 29 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
3f7f916
feat: Use vuid for user.id if user? provided
mikechu-optimizely Dec 27, 2023
9ed3464
test: Add missing Optimizely.getVuid jest.fn mock
mikechu-optimizely Dec 27, 2023
e8e75cd
refactor: Move set user to componentDidMount
mikechu-optimizely Dec 28, 2023
96ccc41
test: Update for rejected optimizely.onReady
mikechu-optimizely Dec 28, 2023
a649e6f
refactor: Change use sync componentDidMount() lifecycle method
mikechu-optimizely Dec 28, 2023
bbd1d5f
fix: remove localStorage & getVuid use
mikechu-optimizely Dec 28, 2023
ba38dc1
fix: Separate responsiblities of getUserContextInstance
mikechu-optimizely Dec 28, 2023
aa573bd
refactor: getUserContextWithOverrides
mikechu-optimizely Dec 28, 2023
2ca6d23
fix: Update ReactSDKClient interface
mikechu-optimizely Dec 28, 2023
170b02a
refactor: Update warns + clean up
mikechu-optimizely Dec 28, 2023
8706614
refactor: DRY + moves + warn updates
mikechu-optimizely Dec 28, 2023
1922259
refactor: No wait fetchQualifiedSegments on setUser
mikechu-optimizely Dec 28, 2023
8bbda64
fix(user): Allow latent setting of user
mikechu-optimizely Jan 4, 2024
d10f7b6
test: WIP Fixing tests
mikechu-optimizely Jan 4, 2024
7c760b5
nit: Format with Prettier
mikechu-optimizely Jan 4, 2024
df4de19
nit: More formatting with Prettier
mikechu-optimizely Jan 4, 2024
6736c30
test: Fix now that instantiation is now async
mikechu-optimizely Jan 5, 2024
0ac4de4
fix: Remove use sync lifecycle method
mikechu-optimizely Jan 5, 2024
d77f64e
test: WIP graceful promise reject test failing
mikechu-optimizely Jan 5, 2024
aae3fff
test: WIP replace timers in tests
mikechu-optimizely Jan 5, 2024
04aeb61
test: Better timeout handling in jest
mikechu-optimizely Jan 5, 2024
7ab3b8b
refactor: Move client onReady check slightly
mikechu-optimizely Jan 8, 2024
b4beae4
fix: Use FeatureVariableValue; Remove excess checks; Better user assi…
mikechu-optimizely Jan 8, 2024
0a8b997
refactor: Remove FeatureVariableValue for now
mikechu-optimizely Jan 8, 2024
ed35b6d
test: Fix code supported by tests
mikechu-optimizely Jan 8, 2024
33663c5
refactor: Move getUserWithOverrides() back to position
mikechu-optimizely Jan 8, 2024
b3f73f4
refactor: Move isReady() for easier PR
mikechu-optimizely Jan 8, 2024
d8d681c
refactor: Change back to getUserContext()
mikechu-optimizely Jan 12, 2024
58d47c8
refactor: Make set and make user context private
mikechu-optimizely Jan 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Experiment.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/// <reference types="jest" />

import * as React from 'react';
import { act } from 'react-dom/test-utils';
import { render, screen, waitFor } from '@testing-library/react';
Expand Down Expand Up @@ -43,10 +45,11 @@ describe('<OptimizelyExperiment>', () => {
optimizelyMock = ({
onReady: jest.fn().mockImplementation(() => onReadyPromise),
activate: jest.fn().mockImplementation(() => variationKey),
onUserUpdate: jest.fn().mockImplementation(() => () => { }),
onUserUpdate: jest.fn().mockImplementation(() => () => {}),
getVuid: jest.fn().mockImplementation(() => 'vuid_95bf72cebc774dfd8e8e580a5a1'),
notificationCenter: {
addNotificationListener: jest.fn().mockImplementation(() => { }),
removeNotificationListener: jest.fn().mockImplementation(() => { }),
addNotificationListener: jest.fn().mockImplementation(() => {}),
removeNotificationListener: jest.fn().mockImplementation(() => {}),
},
user: {
id: 'testuser',
Expand All @@ -55,7 +58,7 @@ describe('<OptimizelyExperiment>', () => {
isReady: jest.fn().mockImplementation(() => isReady),
getIsReadyPromiseFulfilled: () => true,
getIsUsingSdkKey: () => true,
onForcedVariationsUpdate: jest.fn().mockReturnValue(() => { }),
onForcedVariationsUpdate: jest.fn().mockReturnValue(() => {}),
} as unknown) as ReactSDKClient;
});

Expand Down Expand Up @@ -405,7 +408,9 @@ describe('<OptimizelyExperiment>', () => {
await optimizelyMock.onReady();

expect(optimizelyMock.activate).toHaveBeenCalledWith('experiment1', undefined, undefined);
await waitFor(() => expect(screen.getByTestId('variation-key')).toHaveTextContent('matchingVariation|true|false'));
await waitFor(() =>
expect(screen.getByTestId('variation-key')).toHaveTextContent('matchingVariation|true|false')
);
});

describe('when the onReady() promise return { success: false }', () => {
Expand Down
8 changes: 6 additions & 2 deletions src/Feature.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
/**
* Copyright 2018-2019, Optimizely
* Copyright 2018-2019, 2023 Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/// <reference types="jest" />

import * as React from 'react';
import { act } from 'react-dom/test-utils';
import { render, screen, waitFor } from '@testing-library/react';
Expand Down Expand Up @@ -45,6 +48,7 @@ describe('<OptimizelyFeature>', () => {
getFeatureVariables: jest.fn().mockImplementation(() => featureVariables),
isFeatureEnabled: jest.fn().mockImplementation(() => isEnabledMock),
onUserUpdate: jest.fn().mockImplementation(handler => () => {}),
getVuid: jest.fn().mockImplementation(() => 'vuid_95bf72cebc774dfd8e8e580a5a1'),
notificationCenter: {
addNotificationListener: jest.fn().mockImplementation((type, handler) => {}),
removeNotificationListener: jest.fn().mockImplementation(id => {}),
Expand Down
35 changes: 23 additions & 12 deletions src/Provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as React from 'react';
import { UserAttributes } from '@optimizely/optimizely-sdk';
import { getLogger } from '@optimizely/optimizely-sdk';

// eslint-disable-next-line @typescript-eslint/no-unused-vars
import { OptimizelyContextProvider } from './Context';
import { ReactSDKClient } from './client';
import { ReactSDKClient, DefaultUser } from './client';
import { areUsersEqual, UserInfo } from './utils';

const logger = getLogger('<OptimizelyProvider>');
Expand All @@ -42,9 +43,20 @@ interface OptimizelyProviderState {
export class OptimizelyProvider extends React.Component<OptimizelyProviderProps, OptimizelyProviderState> {
constructor(props: OptimizelyProviderProps) {
super(props);
const { optimizely, userId, userAttributes, user } = props;
}

componentDidMount(): void {
Copy link

Choose a reason for hiding this comment

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

this introduces a bug and breaks SSR:
#234

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internal ticket opened and put into this sprint.

this.setUserInOptimizely();
}

async setUserInOptimizely(): Promise<void> {
const { optimizely, userId, userAttributes, user } = this.props;

if (!optimizely) {
logger.error('OptimizelyProvider must be passed an instance of the Optimizely SDK client');
return;
}

// check if user id/attributes are provided as props and set them ReactSDKClient
let finalUser: UserInfo | null = null;

if (user) {
Expand All @@ -65,17 +77,16 @@ export class OptimizelyProvider extends React.Component<OptimizelyProviderProps,
};
// deprecation warning
logger.warn('Passing userId and userAttributes as props is deprecated, please switch to using `user` prop');
} else {
finalUser = DefaultUser;
}

if (finalUser) {
if (!optimizely) {
logger.error(`Unable to set user ${finalUser} because optimizely object does not exist.`)
} else {
try {
optimizely.setUser(finalUser);
} catch (err) {
logger.error(`Unable to set user ${finalUser} because passed in optimizely object does not contain the setUser function.`)
}
try {
await optimizely.onReady();
await optimizely.setUser(finalUser);
} catch {
logger.error('Error while trying to set user.');
}
}
}
Expand Down Expand Up @@ -109,7 +120,7 @@ export class OptimizelyProvider extends React.Component<OptimizelyProviderProps,
}
}

render() {
render(): JSX.Element {
const { optimizely, children, timeout } = this.props;
const isServerSide = !!this.props.isServerSide;
const value = {
Expand Down
18 changes: 7 additions & 11 deletions src/client.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

jest.mock('@optimizely/optimizely-sdk');
jest.mock('./logger', () => {
return {
logger: {
warn: jest.fn(() => () => { }),
info: jest.fn(() => () => { }),
error: jest.fn(() => () => { }),
debug: jest.fn(() => () => { }),
warn: jest.fn(() => () => {}),
info: jest.fn(() => () => {}),
error: jest.fn(() => () => {}),
debug: jest.fn(() => () => {}),
},
};
});
Expand Down Expand Up @@ -1236,10 +1237,6 @@ describe('ReactSDKClient', () => {
const result = await instance.fetchQualifiedSegments();

expect(result).toEqual(false);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn).toBeCalledWith(
'Unable to fetch qualified segments for user because Optimizely client failed to initialize.'
);
});

it('should return false if fetch fails', async () => {
Expand Down Expand Up @@ -1668,15 +1665,14 @@ describe('ReactSDKClient', () => {
instance.getUserContext();

expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn).toBeCalledWith("Unable to get user context because Optimizely client failed to initialize.");
expect(logger.warn).toBeCalledWith('Unable to get user context. Optimizely client not initialized.');
});


it('should log a warning and return null if setUser is not called first', () => {
instance.getUserContext();

expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn).toBeCalledWith("Unable to get user context because user was not set.");
expect(logger.warn).toBeCalledWith('Unable to get user context. User context not set.');
});

it('should return a userContext if setUser is called', () => {
Expand Down
Loading
Loading