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

fix: Optimizely refactor #57

Merged
merged 7 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions .env.test
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ SITE_NAME=localhost
USER_INFO_COOKIE_NAME='edx-user-info'
APP_ID=''
MFE_CONFIG_API_URL=''
OPTIMIZELY_FULL_STACK_SDK_KEY='test-optimizely-sdk-full-stack-key'
1 change: 1 addition & 0 deletions module.config.js.example
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ module.exports = {
// { moduleName: '@openedx/paragon/icons', dir: '../paragon', dist: 'icons' },
// { moduleName: '@openedx/paragon', dir: '../paragon', dist: 'dist' },
// { moduleName: '@edx/frontend-platform', dir: '../frontend-platform', dist: 'dist' },
// { moduleName: '@optimizely/react-sdk', dir: '../src/frontend-lib-learning-assistant/src/mocks/optimizely', dist: '.' },
],
};
17 changes: 13 additions & 4 deletions src/components/MessageForm/index.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import PropTypes from 'prop-types';
import React, { useEffect, useRef } from 'react';
import { useDispatch, useSelector } from 'react-redux';

import { useDecision } from '@optimizely/react-sdk';
import { Button, Form, Icon } from '@openedx/paragon';
import { Send } from '@openedx/paragon/icons';
import { getAuthenticatedUser } from '@edx/frontend-platform/auth';

import { OPTIMIZELY_PROMPT_EXPERIMENT_KEY } from '../../data/optimizely';
import {
acknowledgeDisclosure,
addChatMessage,
Expand All @@ -17,6 +19,11 @@ const MessageForm = ({ courseId, shouldAutofocus, unitId }) => {
const dispatch = useDispatch();
const inputRef = useRef();

const { userId } = getAuthenticatedUser();
const [decision] = useDecision(OPTIMIZELY_PROMPT_EXPERIMENT_KEY, { autoUpdate: true }, { id: userId.toString() });
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why autoUpdate is true here? I believe the default is false. Reading the documentation, I don't exactly understand why we'd want to set this to true.

Copy link
Member

Choose a reason for hiding this comment

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

Should the id override have the name overrideUserId?

Copy link
Member

Choose a reason for hiding this comment

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

If you send the userId via the OptimizelyProvider, which you do via the ExperimentsProvider component, you shouldn't need to send a user ID override, because that user ID will already be available to Optimizely throughout the component tree. Is there a reason we need to override the ID here? I have the same question for src/components/Sidebar/index.jsx.

Copy link
Member Author

@rijuma rijuma Jul 18, 2024

Choose a reason for hiding this comment

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

Could you explain why autoUpdate is true here? I believe the default is false. Reading the documentation, I don't exactly understand why we'd want to set this to true.

I understood that having the autoUpdate: true would react to changes on the experiment immediately while the user is in the experiment. I think it's a good thing to have in cases where we detect a bug and want to remove the experiment. I thought it was an useful thing to use to be able to react in real time.

Should the id override have the name overrideUserId?

Indeed. Good catch. I've made a small refactor on the useDecision() hook into its own usePromptExperimentDecision() hook to avoid repetition.

If you send the userId via the OptimizelyProvider, which you do via the ExperimentsProvider component, you shouldn't need to send a user ID override, because that user ID will already be available to Optimizely throughout the component tree. Is there a reason we need to override the ID here? I have the same question for src/components/Sidebar/index.jsx.

I was worried if for some reason the user changed after the initialization. Since the getAuthenticatedUser() is a getter, the provider might not update if this happens. The override would have the updated userId when the call is made, so I thought it was more accurate.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. I don't think those cases are likely to occur in the experiment, but I don't see a good reason not have these options set this way because, as you said, it's safer. I think my main concern is it makes it a little more difficult to understand. The user override, for example, feels like it's serving to perform an override, but the user ID is exactly the same. But that's why I asked the question, and I appreciate you explaining their function! I don't have any concerns with leaving them in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same thing. I saw the usage in some examples in Optimizely's documentation and saw no reason to exclude it. I've added a short inline comment on each prop to explain the usage.

const { enabled, variationKey } = decision || {};
const promptExperimentVariationKey = enabled ? variationKey : undefined;

useEffect(() => {
if (inputRef.current && !apiError && !apiIsLoading && shouldAutofocus) {
inputRef.current.focus();
Expand All @@ -25,10 +32,11 @@ const MessageForm = ({ courseId, shouldAutofocus, unitId }) => {

const handleSubmitMessage = (event) => {
event.preventDefault();

if (currentMessage) {
dispatch(acknowledgeDisclosure(true));
dispatch(addChatMessage('user', currentMessage, courseId));
dispatch(getChatResponse(courseId, unitId));
dispatch(addChatMessage('user', currentMessage, courseId, promptExperimentVariationKey));
dispatch(getChatResponse(courseId, unitId, promptExperimentVariationKey));
}
};

Expand All @@ -43,13 +51,14 @@ const MessageForm = ({ courseId, shouldAutofocus, unitId }) => {
onClick={handleSubmitMessage}
size="inline"
variant="tertiary"
data-testid="message-form-submit"
>
<Icon src={Send} />
</Button>
);

return (
<Form className="w-100 pl-2" onSubmit={handleSubmitMessage}>
<Form className="w-100 pl-2" onSubmit={handleSubmitMessage} data-testid="message-form">
<Form.Group>
<Form.Control
data-hj-suppress
Expand Down
195 changes: 195 additions & 0 deletions src/components/MessageForm/index.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
import React from 'react';
import {
screen, act, fireEvent, waitFor,
} from '@testing-library/react';
import { useDecision } from '@optimizely/react-sdk';
import { render as renderComponent } from '../../utils/utils.test';
import { initialState } from '../../data/slice';
import { OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS } from '../../data/optimizely';
import {
acknowledgeDisclosure,
addChatMessage,
getChatResponse,
updateCurrentMessage,
} from '../../data/thunks';

import MessageForm from '.';

jest.mock('../../utils/surveyMonkey', () => ({
showControlSurvey: jest.fn(),
showVariationSurvey: jest.fn(),
}));

jest.mock('@edx/frontend-platform/analytics', () => ({
sendTrackEvent: jest.fn(),
}));

const mockedAuthenticatedUser = { userId: 123 };
jest.mock('@edx/frontend-platform/auth', () => ({
getAuthenticatedUser: () => mockedAuthenticatedUser,
}));

jest.mock('@edx/frontend-platform/analytics', () => ({
sendTrackEvent: jest.fn(),
}));

const mockDispatch = jest.fn();
jest.mock('react-redux', () => ({
...jest.requireActual('react-redux'),
useDispatch: () => mockDispatch,
}));

jest.mock('@optimizely/react-sdk', () => ({
useDecision: jest.fn(),
}));

jest.mock('../../data/thunks', () => ({
acknowledgeDisclosure: jest.fn(),
addChatMessage: jest.fn(),
getChatResponse: jest.fn(),
updateCurrentMessage: jest.fn(),
}));

const defaultProps = {
courseId: 'some-course-id',
isOpen: true,
setIsOpen: jest.fn(),
unitId: 'some-unit-id',
};

const render = async (props = {}, sliceState = {}) => {
const componentProps = {
...defaultProps,
...props,
};

const initState = {
preloadedState: {
learningAssistant: {
...initialState,
...sliceState,
},
},
};
return act(async () => renderComponent(
<MessageForm {...componentProps} />,
initState,
));
};

describe('<MessageForm />', () => {
beforeEach(() => {
jest.resetAllMocks();
useDecision.mockReturnValue([]);
});

describe('when rendered', () => {
it('should focus if shouldAutofocus is enabled', () => {
const currentMessage = 'How much wood';
const sliceState = {
apiIsLoading: false,
currentMessage,
apiError: false,
};

render({ shouldAutofocus: true }, sliceState);

waitFor(() => {
expect(screen.getByDisplayValue(currentMessage)).toHaveFocus();
});

expect(screen.queryByTestId('message-form')).toBeInTheDocument();
});

it('should dispatch updateCurrentMessage() when updating the form control', () => {
const currentMessage = 'How much wood';
const updatedMessage = 'How much wood coud a woodchuck chuck';
const sliceState = {
apiIsLoading: false,
currentMessage,
apiError: false,
};

render(undefined, sliceState);

act(() => {
const input = screen.getByDisplayValue(currentMessage);
fireEvent.change(input, { target: { value: updatedMessage } });
});

expect(updateCurrentMessage).toHaveBeenCalledWith(updatedMessage);
expect(mockDispatch).toHaveBeenCalledTimes(1);
});

it('should dispatch message on submit as expected', () => {
const currentMessage = 'How much wood could a woodchuck chuck if a woodchuck could chuck wood?';
const sliceState = {
apiIsLoading: false,
currentMessage,
apiError: false,
};

render(undefined, sliceState);

act(() => {
screen.queryByTestId('message-form-submit').click();
});

expect(acknowledgeDisclosure).toHaveBeenCalledWith(true);
expect(addChatMessage).toHaveBeenCalledWith('user', currentMessage, defaultProps.courseId, undefined);
expect(getChatResponse).toHaveBeenCalledWith(defaultProps.courseId, defaultProps.unitId, undefined);
expect(mockDispatch).toHaveBeenCalledTimes(3);
});

it('should not dispatch on submit if there\'s no message', () => {
render();

act(() => {
screen.queryByTestId('message-form-submit').click();
});

expect(acknowledgeDisclosure).not.toHaveBeenCalled();
expect(addChatMessage).not.toHaveBeenCalled();
expect(getChatResponse).not.toHaveBeenCalled();
expect(mockDispatch).not.toHaveBeenCalled();
});
});

describe('prmpt experiment', () => {
beforeEach(() => {
useDecision.mockReturnValue([{
enabled: true,
variationKey: OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS.UPDATED_PROMPT,
}]);
});

it('should include experiment data on submit', () => {
const currentMessage = 'How much wood could a woodchuck chuck if a woodchuck could chuck wood?';
const sliceState = {
apiIsLoading: false,
currentMessage,
apiError: false,
};

render(undefined, sliceState);

act(() => {
screen.queryByTestId('message-form-submit').click();
});

expect(acknowledgeDisclosure).toHaveBeenCalledWith(true);
expect(addChatMessage).toHaveBeenCalledWith(
'user',
currentMessage,
defaultProps.courseId,
OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS.UPDATED_PROMPT,
);
expect(getChatResponse).toHaveBeenCalledWith(
defaultProps.courseId,
defaultProps.unitId,
OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS.UPDATED_PROMPT,
);
expect(mockDispatch).toHaveBeenCalledTimes(3);
});
});
});
18 changes: 13 additions & 5 deletions src/components/Sidebar/index.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import React, { useRef, useEffect } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import { useDecision } from '@optimizely/react-sdk';
import { sendTrackEvent } from '@edx/frontend-platform/analytics';
import { getAuthenticatedUser } from '@edx/frontend-platform/auth';
import {
Button,
Icon,
Expand All @@ -10,7 +12,7 @@ import {
import { Close } from '@openedx/paragon/icons';

import { clearMessages } from '../../data/thunks';
import { PROMPT_EXPERIMENT_FLAG, PROMPT_EXPERIMENT_KEY } from '../../constants/experiments';
import { OPTIMIZELY_PROMPT_EXPERIMENT_KEY, OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS } from '../../data/optimizely';
import { showControlSurvey, showVariationSurvey } from '../../utils/surveyMonkey';

import APIError from '../APIError';
Expand All @@ -29,12 +31,18 @@ const Sidebar = ({
apiError,
disclosureAcknowledged,
messageList,
experiments,
} = useSelector(state => state.learningAssistant);
const { variationKey } = experiments?.[PROMPT_EXPERIMENT_FLAG] || {};
const chatboxContainerRef = useRef(null);
const dispatch = useDispatch();

const { userId } = getAuthenticatedUser();
const [decision] = useDecision(OPTIMIZELY_PROMPT_EXPERIMENT_KEY, { autoUpdate: true }, { id: userId.toString() });
const { enabled: enabledExperiment, variationKey } = decision || {};
const experimentPayload = enabledExperiment ? {
experiment_name: OPTIMIZELY_PROMPT_EXPERIMENT_KEY,
variation_key: variationKey,
} : {};

// this use effect is intended to scroll to the bottom of the chat window, in the case
// that a message is larger than the chat window height.
useEffect(() => {
Expand Down Expand Up @@ -73,7 +81,7 @@ const Sidebar = ({
setIsOpen(false);

if (messageList.length >= 2) {
if (variationKey === PROMPT_EXPERIMENT_KEY) {
if (enabledExperiment && variationKey === OPTIMIZELY_PROMPT_EXPERIMENT_VARIATION_KEYS.UPDATED_PROMPT) {
showVariationSurvey();
} else {
showControlSurvey();
Expand All @@ -85,7 +93,7 @@ const Sidebar = ({
dispatch(clearMessages());
sendTrackEvent('edx.ui.lms.learning_assistant.clear', {
course_id: courseId,
...(variationKey ? { experiment_name: PROMPT_EXPERIMENT_FLAG, variation_key: variationKey } : {}),
...experimentPayload,
});
};

Expand Down
Loading
Loading