Andal.LND/frontend-app-authoring
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #404 from open-craft/navin/fix-max-attempts-setting

Browse Source 
fix: make max attempts setting fallback to default
This commit is contained in:
kenclary
2023-10-11 10:04:16 -04:00
committed by GitHub
parent a76c93c789 6889cd1e82
commit b55b86cf56
13 changed files with 124 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
package.json
View File

@@ -10,6 +10,7 @@
"build": "make build",
"i18n_extract": "BABEL_ENV=i18n fedx-scripts babel src --quiet > /dev/null",
"lint": "fedx-scripts eslint --ext .js --ext .jsx .",
"lint:fix": "fedx-scripts eslint --fix --ext .js --ext .jsx .",
"snapshot": "fedx-scripts jest --updateSnapshot",
"start": "fedx-scripts webpack-dev-server --progress",
"test": "fedx-scripts jest --coverage",

28
src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js
View File

@@ -109,36 +109,40 @@ export const resetCardHooks = (updateSettings) => {
};
export const scoringCardHooks = (scoring, updateSettings, defaultValue) => {
const loadedAttemptsNumber = scoring.attempts.number === defaultValue ? `${scoring.attempts.number} (Default)` : scoring.attempts.number;
let loadedAttemptsNumber = scoring.attempts.number;
if ((loadedAttemptsNumber === defaultValue || !_.isFinite(loadedAttemptsNumber)) && _.isFinite(defaultValue)) {
loadedAttemptsNumber = `${defaultValue} (Default)`;
} else if (loadedAttemptsNumber === defaultValue && _.isNil(defaultValue)) {
loadedAttemptsNumber = '';
}
const [attemptDisplayValue, setAttemptDisplayValue] = module.state.attemptDisplayValue(loadedAttemptsNumber);
const handleUnlimitedChange = (event) => {
const isUnlimited = event.target.checked;
if (isUnlimited) {
setAttemptDisplayValue('');
updateSettings({ scoring: { ...scoring, attempts: { number: '', unlimited: true } } });
updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: true } } });
} else {
setAttemptDisplayValue(`${defaultValue} (Default)`);
updateSettings({ scoring: { ...scoring, attempts: { number: defaultValue, unlimited: false } } });
updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
}
};
const handleMaxAttemptChange = (event) => {
let unlimitedAttempts = false;
let attemptNumber = parseInt(event.target.value);
const { value } = event.target;
if (_.isNaN(attemptNumber)) {
if (value === '') {
attemptNumber = defaultValue;
if (!_.isFinite(attemptNumber) || attemptNumber === defaultValue) {
attemptNumber = null;
if (_.isFinite(defaultValue)) {
setAttemptDisplayValue(`${defaultValue} (Default)`);
} else {
attemptNumber = '';
setAttemptDisplayValue('');
unlimitedAttempts = true;
}
} else if (attemptNumber <= 0) {
attemptNumber = 0;
} else if (attemptNumber === defaultValue) {
const attemptNumberStr = value.replace(' (Default)');
attemptNumber = parseInt(attemptNumberStr);
}
updateSettings({ scoring: { ...scoring, attempts: { number: attemptNumber, unlimited: unlimitedAttempts } } });
};

45
src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js
View File

@@ -148,6 +148,26 @@ describe('Problem settings hooks', () => {
},
};
const defaultValue = 1;
test('test scoringCardHooks initializes display value when attempts.number is null', () => {
const nilScoring = { ...scoring, attempts: { unlimited: false, number: null } };
output = hooks.scoringCardHooks(nilScoring, updateSettings, defaultValue);
expect(state.stateVals[state.keys.attemptDisplayValue]).toEqual(`${defaultValue} (Default)`);
});
test('test scoringCardHooks initializes display value when attempts.number is blank', () => {
const nilScoring = { ...scoring, attempts: { unlimited: false, number: '' } };
output = hooks.scoringCardHooks(nilScoring, updateSettings, defaultValue);
expect(state.stateVals[state.keys.attemptDisplayValue]).toEqual(`${defaultValue} (Default)`);
});
test('test scoringCardHooks initializes display value when attempts.number is not null', () => {
const nonNilScoring = { ...scoring, attempts: { unlimited: false, number: 2 } };
output = hooks.scoringCardHooks(nonNilScoring, updateSettings, defaultValue);
expect(state.stateVals[state.keys.attemptDisplayValue]).toEqual(2);
});
test('test scoringCardHooks initializes display value when attempts.number and defaultValue is null', () => {
const nonNilScoring = { ...scoring, attempts: { unlimited: false, number: null } };
output = hooks.scoringCardHooks(nonNilScoring, updateSettings, null);
expect(state.stateVals[state.keys.attemptDisplayValue]).toEqual('');
});
beforeEach(() => {
output = hooks.scoringCardHooks(scoring, updateSettings, defaultValue);
});
@@ -155,13 +175,12 @@ describe('Problem settings hooks', () => {
output.handleUnlimitedChange({ target: { checked: true } });
expect(state.setState[state.keys.attemptDisplayValue]).toHaveBeenCalledWith('');
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: '', unlimited: true } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: true } } });
});
test('test handleUnlimitedChange sets attempts.unlimited to false when unchecked', () => {
output.handleUnlimitedChange({ target: { checked: false } });
expect(state.setState[state.keys.attemptDisplayValue]).toHaveBeenCalledWith(`${defaultValue} (Default)`);
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: defaultValue, unlimited: false } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
});
test('test handleMaxAttemptChange', () => {
const value = 6;
@@ -175,30 +194,30 @@ describe('Problem settings hooks', () => {
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: value, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to null value', () => {
test('test handleMaxAttemptChange set attempts to null value when default max_attempts is present', () => {
const value = null;
output.handleMaxAttemptChange({ target: { value } });
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: '', unlimited: true } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to default value', () => {
test('test handleMaxAttemptChange set attempts to null when default value is inputted', () => {
const value = '1 (Default)';
output.handleMaxAttemptChange({ target: { value } });
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: 1, unlimited: false } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to non-numeric value', () => {
const value = 'abc';
output.handleMaxAttemptChange({ target: { value } });
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: '', unlimited: true } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to empty value', () => {
const value = '';
output.handleMaxAttemptChange({ target: { value } });
expect(state.setState[state.keys.attemptDisplayValue]).toHaveBeenCalledWith(`${defaultValue} (Default)`);
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: 1, unlimited: false } } });
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to negative value', () => {
const value = -1;
@@ -206,6 +225,14 @@ describe('Problem settings hooks', () => {
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: 0, unlimited: false } } });
});
test('test handleMaxAttemptChange set attempts to empty value with no default', () => {
const value = '';
output = hooks.scoringCardHooks(scoring, updateSettings, null);
output.handleMaxAttemptChange({ target: { value } });
expect(state.setState[state.keys.attemptDisplayValue]).toHaveBeenCalledWith('');
expect(updateSettings)
.toHaveBeenCalledWith({ scoring: { ...scoring, attempts: { number: null, unlimited: true } } });
});
test('test handleOnChange', () => {
const value = 6;
output.handleOnChange({ target: { value } });

2
src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/messages.js
View File

@@ -109,7 +109,7 @@ const messages = defineMessages({
},
attemptsHint: {
id: 'authoring.problemeditor.settings.scoring.attempts.hint',
defaultMessage: 'If a value is not set, unlimited attempts are allowed',
defaultMessage: 'If a default value is not set in advanced settings, unlimited attempts are allowed',
description: 'Summary text for scoring weight',
},
weightHint: {

4
src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx
View File

@@ -1,4 +1,5 @@
import React from 'react';
import _ from 'lodash-es';
import PropTypes from 'prop-types';
import { connect } from 'react-redux';
import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
@@ -32,7 +33,7 @@ export const ScoringCard = ({
summary += ` ${String.fromCharCode(183)} `;
summary += unlimited
? intl.formatMessage(messages.unlimitedAttemptsSummary)
: intl.formatMessage(messages.attemptsSummary, { attempts });
: intl.formatMessage(messages.attemptsSummary, { attempts: attempts || defaultValue });
return summary;
};
@@ -71,6 +72,7 @@ export const ScoringCard = ({
className="mt-3 decoration-control-label"
checked={scoring.attempts.unlimited}
onChange={handleUnlimitedChange}
disabled={!_.isNil(defaultValue)}
>
<div className="x-small">
<FormattedMessage {...messages.unlimitedAttemptsCheckboxLabel} />

9
src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap
View File

@@ -41,7 +41,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card 1`] = `
/>
<Form.Control.Feedback>
<FormattedMessage
defaultMessage="If a value is not set, unlimited attempts are allowed"
defaultMessage="If a default value is not set in advanced settings, unlimited attempts are allowed"
description="Summary text for scoring weight"
id="authoring.problemeditor.settings.scoring.attempts.hint"
/>
@@ -49,6 +49,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card 1`] = `
<Form.Checkbox
checked={false}
className="mt-3 decoration-control-label"
disabled={true}
>
<div
className="x-small"
@@ -115,7 +116,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card max attempts 1`] =
/>
<Form.Control.Feedback>
<FormattedMessage
defaultMessage="If a value is not set, unlimited attempts are allowed"
defaultMessage="If a default value is not set in advanced settings, unlimited attempts are allowed"
description="Summary text for scoring weight"
id="authoring.problemeditor.settings.scoring.attempts.hint"
/>
@@ -123,6 +124,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card max attempts 1`] =
<Form.Checkbox
checked={true}
className="mt-3 decoration-control-label"
disabled={true}
>
<div
className="x-small"
@@ -189,7 +191,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card zero zero weight 1`
/>
<Form.Control.Feedback>
<FormattedMessage
defaultMessage="If a value is not set, unlimited attempts are allowed"
defaultMessage="If a default value is not set in advanced settings, unlimited attempts are allowed"
description="Summary text for scoring weight"
id="authoring.problemeditor.settings.scoring.attempts.hint"
/>
@@ -197,6 +199,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card zero zero weight 1`
<Form.Checkbox
checked={false}
className="mt-3 decoration-control-label"
disabled={true}
>
<div
className="x-small"

2
src/editors/containers/ProblemEditor/components/SelectTypeModal/hooks.js
View File

@@ -25,7 +25,7 @@ export const onSelect = ({ selected, updateField, setBlockTitle }) => () => {
setBlockTitle(AdvanceProblems[selected].title);
} else {
const newOLX = ProblemTypes[selected].template;
const { settings, ...newState } = getDataFromOlx({ rawOLX: newOLX, rawSettings: {} });
const { settings, ...newState } = getDataFromOlx({ rawOLX: newOLX, rawSettings: {}, defaultSettings: {} });
updateField({ ...newState });
setBlockTitle(ProblemTypes[selected].title);
}

2
src/editors/containers/ProblemEditor/data/ReactStateSettingsParser.js
View File

@@ -19,7 +19,7 @@ class ReactStateSettingsParser {
let settings = {};
const stateSettings = this.problem.settings;
settings = popuplateItem(settings, 'number', 'max_attempts', stateSettings.scoring.attempts);
settings = popuplateItem(settings, 'number', 'max_attempts', stateSettings.scoring.attempts, true);
settings = popuplateItem(settings, 'weight', 'weight', stateSettings.scoring);
settings = popuplateItem(settings, 'on', 'showanswer', stateSettings.showAnswer);
settings = popuplateItem(settings, 'afterAttempts', 'attempts_before_showanswer_button', stateSettings.showAnswer);

37
src/editors/containers/ProblemEditor/data/SettingsParser.js
View File

@@ -2,27 +2,38 @@ import _ from 'lodash-es';
import { ShowAnswerTypes, RandomizationTypesKeys } from '../../../data/constants/problem';
export const popuplateItem = (parentObject, itemName, statekey, metadata) => {
export const popuplateItem = (parentObject, itemName, statekey, metadata, allowNull = false) => {
let parent = parentObject;
const item = _.get(metadata, itemName, null);
if (!_.isNil(item)) {
if (!_.isNil(item) || allowNull) {
parent = { ...parentObject, [statekey]: item };
}
return parent;
};
export const parseScoringSettings = (metadata) => {
export const parseScoringSettings = (metadata, defaultSettings) => {
let scoring = {};
let attempts = popuplateItem({}, 'max_attempts', 'number', metadata);
if (_.isEmpty(attempts) || _.isNaN(attempts.number)) {
attempts = { unlimited: true, number: '' };
} else {
attempts.unlimited = false;
if (attempts.number < 0) {
attempts.number = 0;
}
const attempts = popuplateItem({}, 'max_attempts', 'number', metadata);
const initialAttempts = _.get(attempts, 'number', null);
const defaultAttempts = _.get(defaultSettings, 'max_attempts', null);
attempts.unlimited = false;
// isFinite checks if value is a finite primitive number.
if (!_.isFinite(initialAttempts) || initialAttempts === defaultAttempts) {
// set number to null in any case as lms will pick default value if it exists.
attempts.number = null;
}
// if both block number and default number are null set unlimited to true.
if (_.isNil(initialAttempts) && _.isNil(defaultAttempts)) {
attempts.unlimited = true;
}
if (attempts.number < 0) {
attempts.number = 0;
}
scoring = { ...scoring, attempts };
scoring = popuplateItem(scoring, 'weight', 'weight', metadata);
@@ -43,14 +54,14 @@ export const parseShowAnswer = (metadata) => {
return showAnswer;
};
export const parseSettings = (metadata) => {
export const parseSettings = (metadata, defaultSettings) => {
let settings = {};
if (_.isNil(metadata) || _.isEmpty(metadata)) {
return settings;
}
const scoring = parseScoringSettings(metadata);
const scoring = parseScoringSettings(metadata, defaultSettings);
if (!_.isEmpty(scoring)) {
settings = { ...settings, scoring };
}

38
src/editors/containers/ProblemEditor/data/SettingsParser.test.js
View File

@@ -8,39 +8,55 @@ import {
} from './mockData/problemTestData';
describe('Test Settings to State Parser', () => {
const defaultSettings = { max_attempts: 1 };
test('Test all fields populated', () => {
const settings = parseSettings(checklistWithFeebackHints.metadata);
const settings = parseSettings(checklistWithFeebackHints.metadata, defaultSettings);
const { hints, ...settingsPayload } = checklistWithFeebackHints.state.settings;
expect(settings).toStrictEqual(settingsPayload);
});
test('Test score settings', () => {
const scoreSettings = parseScoringSettings(checklistWithFeebackHints.metadata);
const scoreSettings = parseScoringSettings(checklistWithFeebackHints.metadata, defaultSettings);
expect(scoreSettings).toStrictEqual(checklistWithFeebackHints.state.settings.scoring);
});
test('Test score settings zero attempts', () => {
const scoreSettings = parseScoringSettings(numericWithHints.metadata);
const scoreSettings = parseScoringSettings(numericWithHints.metadata, defaultSettings);
expect(scoreSettings).toStrictEqual(numericWithHints.state.settings.scoring);
});
test('Test score settings attempts missing', () => {
const scoreSettings = parseScoringSettings(singleSelectWithHints.metadata);
test('Test score settings attempts missing with no default max_attempts', () => {
const scoreSettings = parseScoringSettings(singleSelectWithHints.metadata, {});
expect(scoreSettings.attempts).toStrictEqual(singleSelectWithHints.state.settings.scoring.attempts);
});
test('Test score settings attempts missing with default max_attempts', () => {
const scoreSettings = parseScoringSettings(singleSelectWithHints.metadata, defaultSettings);
expect(scoreSettings.attempts).toStrictEqual({ number: null, unlimited: false });
});
test('Test negative attempts in score', () => {
const scoreSettings = parseScoringSettings(negativeAttempts.metadata);
const scoreSettings = parseScoringSettings(negativeAttempts.metadata, defaultSettings);
expect(scoreSettings.attempts).toStrictEqual(negativeAttempts.state.settings.scoring.attempts);
});
test('Test score settings missing', () => {
const settings = parseSettings(singleSelectWithHints.metadata);
test('Test score settings missing with no default', () => {
const settings = parseSettings(singleSelectWithHints.metadata, {});
expect(settings.scoring).toStrictEqual(singleSelectWithHints.state.settings.scoring);
});
test('Test score settings missing with default', () => {
const settings = parseSettings(singleSelectWithHints.metadata, defaultSettings);
expect(settings.scoring).toStrictEqual({ attempts: { number: null, unlimited: false } });
});
test('Test score settings missing with null default', () => {
const settings = parseSettings(singleSelectWithHints.metadata, { max_attempts: null });
expect(settings.scoring).toStrictEqual({ attempts: { number: null, unlimited: true } });
});
test('Test invalid randomization', () => {
const settings = parseSettings(numericWithHints.metadata);
const settings = parseSettings(numericWithHints.metadata, defaultSettings);
expect(settings.randomization).toBeUndefined();
});
@@ -55,12 +71,12 @@ describe('Test Settings to State Parser', () => {
});
test('Test empty metadata', () => {
const scoreSettings = parseSettings({});
const scoreSettings = parseSettings({}, defaultSettings);
expect(scoreSettings).toStrictEqual({});
});
test('Test null metadata', () => {
const scoreSettings = parseSettings(null);
const scoreSettings = parseSettings(null, defaultSettings);
expect(scoreSettings).toStrictEqual({});
});
});

2
src/editors/containers/ProblemEditor/data/mockData/problemTestData.js
View File

@@ -356,7 +356,7 @@ export const singleSelectWithHints = {
scoring: {
attempts: {
unlimited: true,
number: '',
number: null,
},
},
timeBetween: 0,

7
src/editors/data/redux/problem/reducers.js
View File

@@ -21,8 +21,8 @@ const initialState = {
scoring: {
weight: 1,
attempts: {
unlimited: true,
number: '',
unlimited: false,
number: null,
},
},
hints: [],
@@ -210,9 +210,6 @@ const problem = createSlice({
setEnableTypeSelection: (state, { payload }) => {
const { maxAttempts, showanswer, showResetButton } = payload;
const attempts = { number: maxAttempts, unlimited: false };
if (!maxAttempts) {
attempts.unlimited = true;
}
return {
...state,
settings: {

10
src/editors/data/redux/thunkActions/problem.js
View File

@@ -25,7 +25,7 @@ export const isBlankProblem = ({ rawOLX }) => {
return false;
};
export const getDataFromOlx = ({ rawOLX, rawSettings }) => {
export const getDataFromOlx = ({ rawOLX, rawSettings, defaultSettings }) => {
let olxParser;
let parsedProblem;
try {
@@ -33,13 +33,13 @@ export const getDataFromOlx = ({ rawOLX, rawSettings }) => {
parsedProblem = olxParser.getParsedOLXData();
} catch (error) {
console.error('The Problem Could Not Be Parsed from OLX. redirecting to Advanced editor.', error);
return { problemType: ProblemTypeKeys.ADVANCED, rawOLX, settings: parseSettings(rawSettings) };
return { problemType: ProblemTypeKeys.ADVANCED, rawOLX, settings: parseSettings(rawSettings, defaultSettings) };
}
if (parsedProblem?.problemType === ProblemTypeKeys.ADVANCED) {
return { problemType: ProblemTypeKeys.ADVANCED, rawOLX, settings: parseSettings(rawSettings) };
return { problemType: ProblemTypeKeys.ADVANCED, rawOLX, settings: parseSettings(rawSettings, defaultSettings) };
}
const { settings, ...data } = parsedProblem;
const parsedSettings = { ...settings, ...parseSettings(rawSettings) };
const parsedSettings = { ...settings, ...parseSettings(rawSettings, defaultSettings) };
if (!_.isEmpty(rawOLX) && !_.isEmpty(data)) {
return { ...data, rawOLX, settings: parsedSettings };
}
@@ -50,7 +50,7 @@ export const loadProblem = ({ rawOLX, rawSettings, defaultSettings }) => (dispat
if (isBlankProblem({ rawOLX })) {
dispatch(actions.problem.setEnableTypeSelection(camelizeKeys(defaultSettings)));
} else {
dispatch(actions.problem.load(getDataFromOlx({ rawOLX, rawSettings })));
dispatch(actions.problem.load(getDataFromOlx({ rawOLX, rawSettings, defaultSettings })));
}
};