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

feat: refactor non instruction components #132

Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 0 additions & 4 deletions src/context.jsx

This file was deleted.

35 changes: 0 additions & 35 deletions src/core/ExamStateProvider.jsx

This file was deleted.

13 changes: 5 additions & 8 deletions src/core/OuterExamTimer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@ import React, { useEffect, useContext } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import { AppContext } from '@edx/frontend-platform/react';
import ExamStateContext from '../context';
import { getLatestAttemptData } from '../data';
import { ExamTimerBlock } from '../timer';
import ExamAPIError from '../exam/ExamAPIError';
import ExamStateProvider from './ExamStateProvider';
import { getLatestAttemptData } from '../data';
import { IS_STARTED_STATUS } from '../constants';

const ExamTimer = ({ courseId }) => {
const examState = useContext(ExamStateContext);
const { activeAttempt } = useSelector(state => state.specialExams);
const { authenticatedUser } = useContext(AppContext);
const { showTimer } = examState;
const showTimer = !!(activeAttempt && IS_STARTED_STATUS(activeAttempt.attempt_status));

const { apiErrorMsg } = useSelector(state => state.specialExams);

Expand Down Expand Up @@ -48,9 +47,7 @@ ExamTimer.propTypes = {
* will be shown.
*/
const OuterExamTimer = ({ courseId }) => (
<ExamStateProvider>
<ExamTimer courseId={courseId} />
</ExamStateProvider>
<ExamTimer courseId={courseId} />
);

OuterExamTimer.propTypes = {
Expand Down
13 changes: 8 additions & 5 deletions src/core/OuterExamTimer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ import '@testing-library/jest-dom';
import { Factory } from 'rosie';
import React from 'react';
import OuterExamTimer from './OuterExamTimer';
import { store, getLatestAttemptData } from '../data';
import { render } from '../setupTest';
import { getLatestAttemptData } from '../data';
import { initializeTestStore, render } from '../setupTest';
import { ExamStatus } from '../constants';

jest.mock('../data', () => ({
store: {},
getLatestAttemptData: jest.fn(),
Emitter: {
on: () => jest.fn(),
Expand All @@ -17,12 +16,16 @@ jest.mock('../data', () => ({
},
}));
getLatestAttemptData.mockReturnValue(jest.fn());
store.subscribe = jest.fn();
store.dispatch = jest.fn();

describe('OuterExamTimer', () => {
const courseId = 'course-v1:test+test+test';

let store;

beforeEach(async () => {
store = await initializeTestStore();
});

it('is successfully rendered and shows timer if there is an exam in progress', () => {
const attempt = Factory.build('attempt', {
attempt_status: ExamStatus.STARTED,
Expand Down
5 changes: 1 addition & 4 deletions src/core/SequenceExamWrapper.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import ExamWrapper from '../exam/ExamWrapper';
import ExamStateProvider from './ExamStateProvider';

/**
* SequenceExamWrapper is the component responsible for handling special exams.
Expand All @@ -14,9 +13,7 @@ import ExamStateProvider from './ExamStateProvider';
* </SequenceExamWrapper>
*/
const SequenceExamWrapper = (props) => (
<ExamStateProvider>
<ExamWrapper {...props} />
</ExamStateProvider>
<ExamWrapper {...props} />
);

export default SequenceExamWrapper;
2 changes: 1 addition & 1 deletion src/data/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ export {
resetExam,
getAllowProctoringOptOut,
examRequiresAccessToken,
checkExamEntry,
} from './thunks';

export {
expireExamAttempt,
} from './slice';

export { default as store } from './store';
export { default as Emitter } from './emitter';
8 changes: 0 additions & 8 deletions src/data/store.js

This file was deleted.

21 changes: 12 additions & 9 deletions src/exam/Exam.jsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/* eslint-disable react-hooks/exhaustive-deps */
import React, { useContext, useEffect, useState } from 'react';
import React, { useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import { injectIntl, intlShape, FormattedMessage } from '@edx/frontend-platform/i18n';
import { Alert, Spinner } from '@edx/paragon';
import { Info } from '@edx/paragon/icons';
import { ExamTimerBlock } from '../timer';
import Instructions from '../instructions';
import ExamStateContext from '../context';
import ExamAPIError from './ExamAPIError';
import { ExamStatus, ExamType } from '../constants';
import { ExamStatus, ExamType, IS_STARTED_STATUS } from '../constants';
import messages from './messages';
import { getProctoringSettings } from '../data';

/**
* Exam component is intended to render exam instructions before and after exam.
Expand All @@ -23,10 +23,12 @@ import messages from './messages';
const Exam = ({
isGated, isTimeLimited, originalUserIsStaff, canAccessProctoredExams, children, intl,
}) => {
const state = useContext(ExamStateContext);
const {
isLoading, showTimer, exam, apiErrorMsg, getProctoringSettings,
} = state;
isLoading, activeAttempt, exam, apiErrorMsg,
} = useSelector(state => state.specialExams);
const dispatch = useDispatch();

const showTimer = !!(activeAttempt && IS_STARTED_STATUS(activeAttempt.attempt_status));

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. If you wanted to reduce some of the duplication in the definition of showTimer, you could put it in a custom hook.

const {
attempt,
Expand Down Expand Up @@ -59,7 +61,7 @@ const Exam = ({
if (proctoredExamTypes.includes(examType)) {
// only fetch proctoring settings for a proctored exam
if (examId) {
getProctoringSettings();
dispatch(getProctoringSettings());
}

// Only exclude Timed Exam when restricting access to exams
Expand All @@ -68,7 +70,8 @@ const Exam = ({
// this makes sure useEffect gets called only one time after the exam has been fetched
// we can't leave this empty since initially exam is just an empty object, so
// API calls above would not get triggered
}, [examId]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [examId, dispatch]);
Copy link
Member

Choose a reason for hiding this comment

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

[question] I noticed this in a couple of places. Could you clarify the need for this / if this will eventually be unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lint rule is meant to try and prevent developers from forgetting dependencies in the use effect. Because I didn't change anything within the use effect, I'm planning to leave this as is for now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be removed?

I'm wondering if it's complaining because you're using dispatch in the useEffect hook, so dispatch has to be added to the dependency array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, fair point! I will try adding that in and see what happens.


if (isLoading) {
return (
Expand Down
7 changes: 3 additions & 4 deletions src/exam/ExamAPIError.jsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import React, { useContext } from 'react';
import React from 'react';
import { useSelector } from 'react-redux';
import { getConfig } from '@edx/frontend-platform';
import { Alert, Hyperlink, Icon } from '@edx/paragon';
import { Info } from '@edx/paragon/icons';
import { injectIntl, intlShape, FormattedMessage } from '@edx/frontend-platform/i18n';
import ExamStateContext from '../context';
import messages from './messages';

const ExamAPIError = ({ intl }) => {
const state = useContext(ExamStateContext);
const { SITE_NAME, SUPPORT_URL } = getConfig();
const { apiErrorMsg } = state;
const { apiErrorMsg } = useSelector(state => state.specialExams);
const shouldShowApiErrorMsg = !!apiErrorMsg && !apiErrorMsg.includes('<');

return (
Expand Down
36 changes: 12 additions & 24 deletions src/exam/ExamAPIError.test.jsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import '@testing-library/jest-dom';
import React from 'react';
import { getConfig } from '@edx/frontend-platform';
import { store } from '../data';
import { render } from '../setupTest';
import ExamStateProvider from '../core/ExamStateProvider';
import { initializeTestStore, render } from '../setupTest';
import ExamAPIError from './ExamAPIError';

const originalConfig = jest.requireActual('@edx/frontend-platform').getConfig();
Expand All @@ -13,22 +11,20 @@ jest.mock('@edx/frontend-platform', () => ({
}));
getConfig.mockImplementation(() => originalConfig);

jest.mock('../data', () => ({
store: {},
}));
store.subscribe = jest.fn();
store.dispatch = jest.fn();

describe('ExamAPIError', () => {
const defaultMessage = 'A system error has occurred with your exam.';

let store;

beforeEach(async () => {
store = await initializeTestStore();
});

it('renders with the default information', () => {
store.getState = () => ({ specialExams: {} });

const tree = render(
<ExamStateProvider>
<ExamAPIError />
</ExamStateProvider>,
<ExamAPIError />,
{ store },
);

Expand All @@ -45,9 +41,7 @@ describe('ExamAPIError', () => {
store.getState = () => ({ specialExams: {} });

const { getByTestId } = render(
<ExamStateProvider>
<ExamAPIError />
</ExamStateProvider>,
<ExamAPIError />,
{ store },
);

Expand All @@ -62,9 +56,7 @@ describe('ExamAPIError', () => {
});

const { queryByTestId } = render(
<ExamStateProvider>
<ExamAPIError />
</ExamStateProvider>,
<ExamAPIError />,
{ store },
);

Expand All @@ -77,9 +69,7 @@ describe('ExamAPIError', () => {
});

const { queryByTestId } = render(
<ExamStateProvider>
<ExamAPIError />
</ExamStateProvider>,
<ExamAPIError />,
{ store },
);

Expand All @@ -92,9 +82,7 @@ describe('ExamAPIError', () => {
});

const { queryByTestId } = render(
<ExamStateProvider>
<ExamAPIError />
</ExamStateProvider>,
<ExamAPIError />,
{ store },
);

Expand Down
18 changes: 13 additions & 5 deletions src/exam/ExamWrapper.jsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import { useDispatch, useSelector } from 'react-redux';
import React, { useContext, useEffect } from 'react';
import { AppContext } from '@edx/frontend-platform/react';
import PropTypes from 'prop-types';
import Exam from './Exam';
import ExamStateContext from '../context';
import {
getExamAttemptsData,
getAllowProctoringOptOut,
checkExamEntry,
} from '../data';

/**
* Exam wrapper is responsible for triggering initial exam data fetching and rendering Exam.
*/
const ExamWrapper = ({ children, ...props }) => {
const state = useContext(ExamStateContext);
const { authenticatedUser } = useContext(AppContext);
const {
sequence,
Expand All @@ -17,9 +21,13 @@ const ExamWrapper = ({ children, ...props }) => {
originalUserIsStaff,
canAccessProctoredExams,
} = props;
const { getExamAttemptsData, getAllowProctoringOptOut, checkExamEntry } = state;

const { isLoading } = useSelector(state => state.specialExams);

const dispatch = useDispatch();

const loadInitialData = async () => {
await getExamAttemptsData(courseId, sequence.id);
await dispatch(getExamAttemptsData(courseId, sequence.id));
await getAllowProctoringOptOut(sequence.allowProctoringOptOut);
await checkExamEntry();
};
Expand All @@ -28,7 +36,7 @@ const ExamWrapper = ({ children, ...props }) => {

useEffect(() => {
// fetch exam data on exam sequences or if no exam data has been fetched yet
if (sequence.isTimeLimited || state.isLoading) {
if (sequence.isTimeLimited || isLoading) {
loadInitialData();
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
Loading
Loading