diff --git a/package.json b/package.json index 74c4779ab..d8bdcf60e 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js index d8b058d9c..b115d8cd0 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js @@ -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 } } }); }; diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js index a93ddc294..8b636c0a8 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js @@ -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 } }); diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/messages.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/messages.js index 77b771f6c..26c5414a1 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/messages.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/messages.js @@ -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: { diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx index 74fc79edb..06ca2cb59 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx @@ -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)} >
diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap index b4d64a781..59a32a66e 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap @@ -41,7 +41,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card 1`] = ` /> @@ -49,6 +49,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card 1`] = `
@@ -123,6 +124,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card max attempts 1`] =
@@ -197,6 +199,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card zero zero weight 1`
() => { 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); } diff --git a/src/editors/containers/ProblemEditor/data/ReactStateSettingsParser.js b/src/editors/containers/ProblemEditor/data/ReactStateSettingsParser.js index 2c22dcb77..2d7ddf764 100644 --- a/src/editors/containers/ProblemEditor/data/ReactStateSettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/ReactStateSettingsParser.js @@ -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); diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.js b/src/editors/containers/ProblemEditor/data/SettingsParser.js index 68af5d47d..918a9eac2 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.js @@ -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 }; } diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js index 2f01f3f38..063484cea 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js @@ -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({}); }); }); diff --git a/src/editors/containers/ProblemEditor/data/mockData/problemTestData.js b/src/editors/containers/ProblemEditor/data/mockData/problemTestData.js index 3dba52653..65e6d3a89 100644 --- a/src/editors/containers/ProblemEditor/data/mockData/problemTestData.js +++ b/src/editors/containers/ProblemEditor/data/mockData/problemTestData.js @@ -356,7 +356,7 @@ export const singleSelectWithHints = { scoring: { attempts: { unlimited: true, - number: '', + number: null, }, }, timeBetween: 0, diff --git a/src/editors/data/redux/problem/reducers.js b/src/editors/data/redux/problem/reducers.js index 8c0683560..4173cbc9b 100644 --- a/src/editors/data/redux/problem/reducers.js +++ b/src/editors/data/redux/problem/reducers.js @@ -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: { diff --git a/src/editors/data/redux/thunkActions/problem.js b/src/editors/data/redux/thunkActions/problem.js index 0d29101de..4ed432660 100644 --- a/src/editors/data/redux/thunkActions/problem.js +++ b/src/editors/data/redux/thunkActions/problem.js @@ -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 }))); } };