From c70679da541db540817082e369d911843cb1394d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 6 Oct 2023 20:35:51 +0530 Subject: [PATCH 1/8] fix: make max attempts setting fallback to default The max attempts setting for a problem xblock should be set to null for course default max attempt setting to take effect. This makes sure that xblock setting is updated if course default is updated. --- .../EditProblemView/SettingsWidget/hooks.js | 16 +++++++------- .../settingsComponents/ScoringCard.jsx | 2 ++ .../components/SelectTypeModal/hooks.js | 2 +- .../data/ReactStateSettingsParser.js | 2 +- .../ProblemEditor/data/SettingsParser.js | 22 ++++++++++++++----- src/editors/data/redux/problem/reducers.js | 7 ++---- .../data/redux/thunkActions/problem.js | 10 ++++----- 7 files changed, 35 insertions(+), 26 deletions(-) diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js index d8b058d9c..23e02b052 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js @@ -115,29 +115,29 @@ export const scoringCardHooks = (scoring, updateSettings, defaultValue) => { 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; + // TODO: impove below condition handling if (_.isNaN(attemptNumber)) { - if (value === '') { - attemptNumber = defaultValue; + if (value === '' && !_.isNil(defaultValue)) { + attemptNumber = null; setAttemptDisplayValue(`${defaultValue} (Default)`); - } else { - attemptNumber = ''; + } else if (_.isNil(defaultValue)) { + attemptNumber = null; unlimitedAttempts = true; } } else if (attemptNumber <= 0) { attemptNumber = 0; } else if (attemptNumber === defaultValue) { - const attemptNumberStr = value.replace(' (Default)'); - attemptNumber = parseInt(attemptNumberStr); + attemptNumber = null; } updateSettings({ scoring: { ...scoring, attempts: { number: attemptNumber, unlimited: unlimitedAttempts } } }); }; 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..17c2b2a4a 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'; @@ -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/SelectTypeModal/hooks.js b/src/editors/containers/ProblemEditor/components/SelectTypeModal/hooks.js index e28e3d306..c07663dfd 100644 --- a/src/editors/containers/ProblemEditor/components/SelectTypeModal/hooks.js +++ b/src/editors/containers/ProblemEditor/components/SelectTypeModal/hooks.js @@ -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); } 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..34590b1c6 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.js @@ -2,21 +2,31 @@ 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)) { + // TODO: impove below condition handling + if ((_.isEmpty(attempts) || _.isNaN(attempts.number)) && _.isNaN(defaultSettings.max_attempts)) { attempts = { unlimited: true, number: '' }; + } else if (!_.isEmpty(attempts) && !_.isNaN(attempts.number)) { + attempts.unlimited = false; + attempts.number = attempts.number; + if (attempts.number < 0) { + attempts.number = 0; + } + } else if (!_.isNaN(defaultSettings.max_attempts)) { + attempts.unlimited = false; + attempts.number = defaultSettings.max_attempts; } else { attempts.unlimited = false; if (attempts.number < 0) { @@ -43,14 +53,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/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 }))); } }; From 5df26bf83b9fc3101c2d519a2bf6d2d29ef76c12 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 9 Oct 2023 20:34:05 +0530 Subject: [PATCH 2/8] test: fix related test cases --- .../EditProblemView/SettingsWidget/hooks.js | 5 ++-- .../SettingsWidget/hooks.test.js | 16 ++++++------- .../__snapshots__/ScoringCard.test.jsx.snap | 3 +++ .../ProblemEditor/data/SettingsParser.js | 2 +- .../ProblemEditor/data/SettingsParser.test.js | 23 ++++++++++--------- 5 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js index 23e02b052..4183d5bf4 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js @@ -126,12 +126,11 @@ export const scoringCardHooks = (scoring, updateSettings, defaultValue) => { let attemptNumber = parseInt(event.target.value); const { value } = event.target; // TODO: impove below condition handling - if (_.isNaN(attemptNumber)) { + if (_.isNaN(attemptNumber) || _.isNil(attemptNumber)) { + attemptNumber = null; if (value === '' && !_.isNil(defaultValue)) { - attemptNumber = null; setAttemptDisplayValue(`${defaultValue} (Default)`); } else if (_.isNil(defaultValue)) { - attemptNumber = null; unlimitedAttempts = true; } } else if (attemptNumber <= 0) { 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..49dc36170 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js @@ -155,13 +155,13 @@ 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 +175,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; 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..131399c82 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 @@ -49,6 +49,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card 1`] = `
{ let attempts = popuplateItem({}, 'max_attempts', 'number', metadata); // TODO: impove below condition handling - if ((_.isEmpty(attempts) || _.isNaN(attempts.number)) && _.isNaN(defaultSettings.max_attempts)) { + if ((_.isEmpty(attempts) || _.isNaN(attempts.number)) && (_.isEmpty(defaultSettings) || _.isNaN(defaultSettings.max_attempts))) { attempts = { unlimited: true, number: '' }; } else if (!_.isEmpty(attempts) && !_.isNaN(attempts.number)) { attempts.unlimited = false; diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js index 2f01f3f38..c5278ae11 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js @@ -8,39 +8,40 @@ 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 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 invalid randomization', () => { - const settings = parseSettings(numericWithHints.metadata); + const settings = parseSettings(numericWithHints.metadata, defaultSettings); expect(settings.randomization).toBeUndefined(); }); @@ -55,12 +56,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({}); }); }); From 398839d76ca471a3e3052cf73bd0bdbdcd1ee21b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 9 Oct 2023 21:29:53 +0530 Subject: [PATCH 3/8] refactor: improve setting parser condition handling --- .../ProblemEditor/data/SettingsParser.js | 33 +++++++++---------- .../ProblemEditor/data/SettingsParser.test.js | 15 +++++++++ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.js b/src/editors/containers/ProblemEditor/data/SettingsParser.js index 586cccad0..36307b200 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.js @@ -15,24 +15,23 @@ export const parseScoringSettings = (metadata, defaultSettings) => { let scoring = {}; let attempts = popuplateItem({}, 'max_attempts', 'number', metadata); - // TODO: impove below condition handling - if ((_.isEmpty(attempts) || _.isNaN(attempts.number)) && (_.isEmpty(defaultSettings) || _.isNaN(defaultSettings.max_attempts))) { - attempts = { unlimited: true, number: '' }; - } else if (!_.isEmpty(attempts) && !_.isNaN(attempts.number)) { - attempts.unlimited = false; - attempts.number = attempts.number; - if (attempts.number < 0) { - attempts.number = 0; - } - } else if (!_.isNaN(defaultSettings.max_attempts)) { - attempts.unlimited = false; - attempts.number = defaultSettings.max_attempts; - } else { - attempts.unlimited = false; - if (attempts.number < 0) { - attempts.number = 0; - } + attempts.unlimited = false; + + // isFinite checks if value is a finite primitive number. + if (!_.isFinite(_.get(attempts, 'number', null))) { + attempts.number = _.get(defaultSettings, 'max_attempts', null); } + + // if above statement was true and no default was found, set unlimited to true + if (_.isNil(attempts.number)) { + attempts.number = ''; + attempts.unlimited = true; + } + + if (attempts.number < 0) { + attempts.number = 0; + } + scoring = { ...scoring, attempts }; scoring = popuplateItem(scoring, 'weight', 'weight', metadata); diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js index c5278ae11..b415a8422 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js @@ -30,6 +30,11 @@ describe('Test Settings to State Parser', () => { 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: 1, unlimited: false}); + }); + test('Test negative attempts in score', () => { const scoreSettings = parseScoringSettings(negativeAttempts.metadata, defaultSettings); expect(scoreSettings.attempts).toStrictEqual(negativeAttempts.state.settings.scoring.attempts); @@ -40,6 +45,16 @@ describe('Test Settings to State Parser', () => { 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: 1, unlimited: false}}); + }); + + test('Test score settings missing with null default', () => { + const settings = parseSettings(singleSelectWithHints.metadata, {max_attempts: null}); + expect(settings.scoring).toStrictEqual({attempts: {number: '', unlimited: true}}); + }); + test('Test invalid randomization', () => { const settings = parseSettings(numericWithHints.metadata, defaultSettings); expect(settings.randomization).toBeUndefined(); From 82b770bdef984bf6fbf312dc94bc0fc0901422da Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 12:13:46 +0530 Subject: [PATCH 4/8] refactor: improve hooks condition handling --- .../EditProblemView/SettingsWidget/hooks.js | 21 +++++++++++-------- .../SettingsWidget/hooks.test.js | 9 +++++++- .../SettingsWidget/messages.js | 2 +- .../settingsComponents/ScoringCard.jsx | 2 +- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js index 4183d5bf4..5cdc2c34a 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js @@ -109,7 +109,11 @@ 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 (scoring.attempts.number === defaultValue) { + loadedAttemptsNumber = `${scoring.attempts.number} (Default)`; + updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } }); + } const [attemptDisplayValue, setAttemptDisplayValue] = module.state.attemptDisplayValue(loadedAttemptsNumber); const handleUnlimitedChange = (event) => { const isUnlimited = event.target.checked; @@ -117,27 +121,26 @@ export const scoringCardHooks = (scoring, updateSettings, defaultValue) => { setAttemptDisplayValue(''); updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: true } } }); } else { - setAttemptDisplayValue(`${defaultValue} (Default)`); updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } }); } }; + const handleMaxAttemptChange = (event) => { let unlimitedAttempts = false; let attemptNumber = parseInt(event.target.value); - const { value } = event.target; - // TODO: impove below condition handling - if (_.isNaN(attemptNumber) || _.isNil(attemptNumber)) { + + if (!_.isFinite(attemptNumber) || attemptNumber === defaultValue) { attemptNumber = null; - if (value === '' && !_.isNil(defaultValue)) { + if (_.isFinite(defaultValue)) { setAttemptDisplayValue(`${defaultValue} (Default)`); - } else if (_.isNil(defaultValue)) { + } else { + setAttemptDisplayValue(''); unlimitedAttempts = true; } } else if (attemptNumber <= 0) { attemptNumber = 0; - } else if (attemptNumber === defaultValue) { - attemptNumber = null; } + 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 49dc36170..1fcc05a1b 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.test.js @@ -159,7 +159,6 @@ describe('Problem settings hooks', () => { }); 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: null, unlimited: false } } }); }); @@ -206,6 +205,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 17c2b2a4a..03a0ffb6b 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx @@ -33,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 ? attempts : defaultValue }); return summary; }; From 2209e5b963e15f8569027706e62fd2120cb93a53 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 12:19:14 +0530 Subject: [PATCH 5/8] fix: lint issues --- package.json | 1 + .../SettingsWidget/settingsComponents/ScoringCard.jsx | 2 +- .../containers/ProblemEditor/data/SettingsParser.js | 4 ++-- .../containers/ProblemEditor/data/SettingsParser.test.js | 8 ++++---- 4 files changed, 8 insertions(+), 7 deletions(-) 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/settingsComponents/ScoringCard.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx index 03a0ffb6b..06ca2cb59 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx @@ -33,7 +33,7 @@ export const ScoringCard = ({ summary += ` ${String.fromCharCode(183)} `; summary += unlimited ? intl.formatMessage(messages.unlimitedAttemptsSummary) - : intl.formatMessage(messages.attemptsSummary, { attempts: attempts ? attempts : defaultValue }); + : intl.formatMessage(messages.attemptsSummary, { attempts: attempts || defaultValue }); return summary; }; diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.js b/src/editors/containers/ProblemEditor/data/SettingsParser.js index 36307b200..d12fe4f0e 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.js @@ -2,7 +2,7 @@ import _ from 'lodash-es'; import { ShowAnswerTypes, RandomizationTypesKeys } from '../../../data/constants/problem'; -export const popuplateItem = (parentObject, itemName, statekey, metadata, allowNull=false) => { +export const popuplateItem = (parentObject, itemName, statekey, metadata, allowNull = false) => { let parent = parentObject; const item = _.get(metadata, itemName, null); if (!_.isNil(item) || allowNull) { @@ -14,7 +14,7 @@ export const popuplateItem = (parentObject, itemName, statekey, metadata, allowN export const parseScoringSettings = (metadata, defaultSettings) => { let scoring = {}; - let attempts = popuplateItem({}, 'max_attempts', 'number', metadata); + const attempts = popuplateItem({}, 'max_attempts', 'number', metadata); attempts.unlimited = false; // isFinite checks if value is a finite primitive number. diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js index b415a8422..bb9515154 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js @@ -32,7 +32,7 @@ describe('Test Settings to State Parser', () => { test('Test score settings attempts missing with default max_attempts', () => { const scoreSettings = parseScoringSettings(singleSelectWithHints.metadata, defaultSettings); - expect(scoreSettings.attempts).toStrictEqual({number: 1, unlimited: false}); + expect(scoreSettings.attempts).toStrictEqual({ number: 1, unlimited: false }); }); test('Test negative attempts in score', () => { @@ -47,12 +47,12 @@ describe('Test Settings to State Parser', () => { test('Test score settings missing with default', () => { const settings = parseSettings(singleSelectWithHints.metadata, defaultSettings); - expect(settings.scoring).toStrictEqual({attempts: {number: 1, unlimited: false}}); + expect(settings.scoring).toStrictEqual({ attempts: { number: 1, unlimited: false } }); }); test('Test score settings missing with null default', () => { - const settings = parseSettings(singleSelectWithHints.metadata, {max_attempts: null}); - expect(settings.scoring).toStrictEqual({attempts: {number: '', unlimited: true}}); + const settings = parseSettings(singleSelectWithHints.metadata, { max_attempts: null }); + expect(settings.scoring).toStrictEqual({ attempts: { number: '', unlimited: true } }); }); test('Test invalid randomization', () => { From e6766163863fa0cd178380461e3e6934f82b386b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 12:53:11 +0530 Subject: [PATCH 6/8] fix: snapshots --- .../__snapshots__/ScoringCard.test.jsx.snap | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 131399c82..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`] = ` /> @@ -116,7 +116,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card max attempts 1`] = /> @@ -191,7 +191,7 @@ exports[`ScoringCard snapshot snapshot: scoring setting card zero zero weight 1` /> From 35a2f3bb7f7c40692f08147d9b3ed41f682be5b9 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 19:48:07 +0530 Subject: [PATCH 7/8] fix: use only null in state for empty value --- .../EditProblemView/SettingsWidget/hooks.js | 8 +++++--- .../containers/ProblemEditor/data/SettingsParser.js | 12 +++++++----- .../ProblemEditor/data/SettingsParser.test.js | 6 +++--- .../ProblemEditor/data/mockData/problemTestData.js | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js index 5cdc2c34a..b115d8cd0 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/SettingsWidget/hooks.js @@ -110,11 +110,13 @@ export const resetCardHooks = (updateSettings) => { export const scoringCardHooks = (scoring, updateSettings, defaultValue) => { let loadedAttemptsNumber = scoring.attempts.number; - if (scoring.attempts.number === defaultValue) { - loadedAttemptsNumber = `${scoring.attempts.number} (Default)`; - updateSettings({ scoring: { ...scoring, attempts: { number: null, unlimited: false } } }); + 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) { diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.js b/src/editors/containers/ProblemEditor/data/SettingsParser.js index d12fe4f0e..918a9eac2 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.js @@ -15,16 +15,18 @@ export const parseScoringSettings = (metadata, defaultSettings) => { let scoring = {}; 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(_.get(attempts, 'number', null))) { - attempts.number = _.get(defaultSettings, 'max_attempts', null); + 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 above statement was true and no default was found, set unlimited to true - if (_.isNil(attempts.number)) { - attempts.number = ''; + // if both block number and default number are null set unlimited to true. + if (_.isNil(initialAttempts) && _.isNil(defaultAttempts)) { attempts.unlimited = true; } diff --git a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js index bb9515154..063484cea 100644 --- a/src/editors/containers/ProblemEditor/data/SettingsParser.test.js +++ b/src/editors/containers/ProblemEditor/data/SettingsParser.test.js @@ -32,7 +32,7 @@ describe('Test Settings to State Parser', () => { test('Test score settings attempts missing with default max_attempts', () => { const scoreSettings = parseScoringSettings(singleSelectWithHints.metadata, defaultSettings); - expect(scoreSettings.attempts).toStrictEqual({ number: 1, unlimited: false }); + expect(scoreSettings.attempts).toStrictEqual({ number: null, unlimited: false }); }); test('Test negative attempts in score', () => { @@ -47,12 +47,12 @@ describe('Test Settings to State Parser', () => { test('Test score settings missing with default', () => { const settings = parseSettings(singleSelectWithHints.metadata, defaultSettings); - expect(settings.scoring).toStrictEqual({ attempts: { number: 1, unlimited: false } }); + 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: '', unlimited: true } }); + expect(settings.scoring).toStrictEqual({ attempts: { number: null, unlimited: true } }); }); test('Test invalid randomization', () => { 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, From 6889cd1e82797e820d1f78056a243f10e08a0ea8 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 20:08:00 +0530 Subject: [PATCH 8/8] test: add test for initial attempts display value --- .../SettingsWidget/hooks.test.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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 1fcc05a1b..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); });