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

fix: make max attempts setting fallback to default
openedx · Oct 11, 2023 · b55b86c · b55b86c
2 parents a76c93c + 6889cd1
commit b55b86c
Show file tree

Hide file tree

Showing 13 changed files with 124 additions and 63 deletions.
diff --git 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
@@ -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
@@ -148,20 +148,39 @@ 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);
     });
     test('test handleUnlimitedChange sets attempts.unlimited to true when checked', () => {
       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,37 +194,45 @@ 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;
       output.handleMaxAttemptChange({ target: { value } });
       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
@@ -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/...roblemEditor/components/EditProblemView/SettingsWidget/settingsComponents/ScoringCard.jsx b/...roblemEditor/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)}
         >
           <div className="x-small">
             <FormattedMessage {...messages.unlimitedAttemptsCheckboxLabel} />

diff --git a/...EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap b/...EditProblemView/SettingsWidget/settingsComponents/__snapshots__/ScoringCard.test.jsx.snap
@@ -41,14 +41,15 @@ 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"
       />
     </Form.Control.Feedback>
     <Form.Checkbox
       checked={false}
       className="mt-3 decoration-control-label"
+      disabled={true}
     >
       <div
         className="x-small"
@@ -115,14 +116,15 @@ 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"
       />
     </Form.Control.Feedback>
     <Form.Checkbox
       checked={true}
       className="mt-3 decoration-control-label"
+      disabled={true}
     >
       <div
         className="x-small"
@@ -189,14 +191,15 @@ 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"
       />
     </Form.Control.Feedback>
     <Form.Checkbox
       checked={false}
       className="mt-3 decoration-control-label"
+      disabled={true}
     >
       <div
         className="x-small"

diff --git 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
@@ -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
@@ -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 };
   }