fix: cannot select criterion (openedx#100)

* fix: cannot select criterion

* fix: refactor fill grade data

* fix: update tests

Co-authored-by: Ben Warzeski <[email protected]>

Author: leangseu-edx

src/data/redux/app/selectors.js

@@ -124,44 +124,46 @@ rubric.criteriaIndices = createSelector(
  * Returns true iff the passed feedback value is required or optional
  * @return {bool} - should include feedback?
  */
-const shouldIncludeFeedback = (feedback) => ([
+export const shouldIncludeFeedback = (feedback) => [
   feedbackRequirement.required,
   feedbackRequirement.optional,
-]).includes(feedback);
+].includes(feedback);
 
 /**
- * Returns an empty grade data object based on the rubric config loaded in the app model.
- * @return {obj} - empty grade data object
+ * take current grade and fill the empty fill with default value
+ * @param {obj} gradeData
+ * @returns
  */
-export const emptyGrade = createSelector(
-  [module.rubric.hasConfig, module.rubric.criteria, module.rubric.feedbackConfig],
-  (hasConfig, criteria, feedbackConfig) => {
-    if (!hasConfig) {
-      return null;
-    }
-    const gradeData = {};
-    if (shouldIncludeFeedback(feedbackConfig)) {
-      gradeData.overallFeedback = '';
-    }
-    gradeData.criteria = criteria.map(criterion => {
-      const entry = {
-        orderNum: criterion.orderNum,
-        name: criterion.name,
-        selectedOption: '',
-      };
-      if (shouldIncludeFeedback(criterion.feedback)) {
-        entry.feedback = '';
-      }
-      return entry;
-    });
-    return gradeData;
-  },
-);
+export const fillGradeData = (state, data) => {
+  const hasConfig = module.rubric.hasConfig(state);
+  if (!hasConfig || Array.isArray(data?.criteria)) {
+    return data;
+  }
+
+  const feedbackConfig = module.rubric.feedbackConfig(state);
+  const criteria = module.rubric.criteria(state);
+
+  const overallFeedback = (
+    module.shouldIncludeFeedback(feedbackConfig) && { overallFeedback: '' }
+  );
+  const criteriaFeedback = (feedback) => (
+    module.shouldIncludeFeedback(feedback) && { feedback: '' }
+  );
+
+  const gradeData = { ...overallFeedback };
+  gradeData.criteria = criteria.map(({ feedback, name, orderNum }) => ({
+    ...criteriaFeedback(feedback),
+    name,
+    orderNum,
+    selectedOption: '',
+  }));
+  return gradeData;
+};
 
 export default StrictDict({
   ...simpleSelectors,
   courseId,
   ora,
   rubric: StrictDict(rubric),
-  emptyGrade,
+  fillGradeData,
 });

src/data/redux/app/selectors.test.js

@@ -1,5 +1,6 @@
 import { feedbackRequirement } from 'data/services/lms/constants';
 
+import { keyStore } from '../../../utils';
 // import * in order to mock in-file references
 import * as selectors from './selectors';
 
@@ -44,6 +45,8 @@ const testState = {
   },
 };
 
+const selectorKeys = keyStore(selectors);
+
 describe('app selectors unit tests', () => {
   const { appSelector, simpleSelectors, rubric } = selectors;
   describe('appSelector', () => {
@@ -180,56 +183,86 @@ describe('app selectors unit tests', () => {
       });
     });
   });
-  describe('emptyGrade selector', () => {
-    const { rubricConfig } = testState.app.oraMetadata;
-    let preSelectors;
-    let cb;
+  describe('shouldIncludeFeedback', () => {
+    it('returns true iff the passed feedback is optional or required', () => {
+      expect(selectors.shouldIncludeFeedback(feedbackRequirement.optional)).toEqual(true);
+      expect(selectors.shouldIncludeFeedback(feedbackRequirement.required)).toEqual(true);
+      expect(selectors.shouldIncludeFeedback(feedbackRequirement.disabled)).toEqual(false);
+      expect(selectors.shouldIncludeFeedback('aribitrary')).toEqual(false);
+    });
+  });
+  describe('fillGradeData selector', () => {
+    const cb = selectors.fillGradeData;
+    const spies = {};
+    let oldRubric;
+    const criteria = [
+      { name: 'criteria1', orderNum: 0, feedback: true },
+      { name: 'criteria2', orderNum: 1, feedback: false },
+      { name: 'criteria3', orderNum: 2, feedback: true },
+    ];
+
+    const data = { arbitrary: 'data', criteria };
+    beforeAll(() => {
+      oldRubric = { ...rubric };
+    });
     beforeEach(() => {
-      ({ preSelectors, cb } = selectors.emptyGrade);
-    });
-    it('is a memoized selector based on rubric.[hasConfig, criteria, feedbackConfig]', () => {
-      expect(preSelectors).toEqual([
-        rubric.hasConfig,
-        rubric.criteria,
-        rubric.feedbackConfig,
-      ]);
-    });
-    describe('If the config is not loaded (hasConfig = undefined)', () => {
-      it('returns null', () => {
-        expect(cb(false, {}, '')).toEqual(null);
+      rubric.hasConfig = jest.fn(() => true);
+      rubric.feedbackConfig = jest.fn(() => true);
+      rubric.criteria = jest.fn(() => criteria);
+      spies.shouldIncludeFeedback = jest.spyOn(
+        selectors,
+        selectorKeys.shouldIncludeFeedback,
+      ).mockImplementation(val => val);
+    });
+    afterEach(() => {
+      spies[selectorKeys.shouldIncludeFeedback].mockRestore();
+    });
+    afterAll(() => {
+      selectors.rubric = { ...oldRubric };
+    });
+
+    describe('if rubric config is not loaded', () => {
+      it('returns passed gradeData', () => {
+        rubric.hasConfig.mockReturnValueOnce(false);
+        expect(cb(testState, data)).toEqual(data);
       });
     });
-    describe('The generated object', () => {
-      it('loads an overallFeedback field iff feedbackConfig is optional or required', () => {
-        let gradeData = cb(true, rubricConfig.criteria, feedbackRequirement.optional);
-        expect(gradeData.overallFeedback).toEqual('');
-        gradeData = cb(true, rubricConfig.criteria, feedbackRequirement.required);
-        expect(gradeData.overallFeedback).toEqual('');
-        gradeData = cb(true, rubricConfig.criteria, feedbackRequirement.disabled);
-        expect(gradeData.overallFeedback).toEqual(undefined);
+
+    describe('if rubric config is loaded', () => {
+      describe('gradeData is passed, contains criteria', () => {
+        it('returns the passed gradeData', () => {
+          expect(cb(testState, data)).toEqual(data);
+        });
       });
-      it('loads criteria with feedback field based on requirement config', () => {
-        const gradeData = cb(true, rubricConfig.criteria, rubricConfig.feedback);
-        const { criteria } = rubricConfig;
-        expect(gradeData.criteria).toEqual([
-          {
-            orderNum: criteria[0].orderNum,
-            name: criteria[0].name,
-            selectedOption: '',
-            feedback: '',
-          },
-          {
-            orderNum: criteria[1].orderNum,
-            name: criteria[1].name,
-            selectedOption: '',
-          },
-          {
-            orderNum: criteria[2].orderNum,
-            name: criteria[2].name,
-            selectedOption: '',
-            feedback: '',
-          },
-        ]);
+      describe('gradeData is not passed', () => {
+        it('adds overall feedback iff is configured for inclusion', () => {
+          expect(cb(testState, null).overallFeedback).toEqual('');
+          rubric.feedbackConfig.mockReturnValueOnce(false);
+          expect(cb(testState, null).overallFeedback).toEqual(undefined);
+        });
+        describe('criteria', () => {
+          it('displays name, orderNum, and feedback per config and empty selection', () => {
+            expect(cb(testState, null).criteria).toEqual([
+              {
+                name: criteria[0].name,
+                orderNum: criteria[0].orderNum,
+                feedback: '',
+                selectedOption: '',
+              },
+              {
+                name: criteria[1].name,
+                orderNum: criteria[1].orderNum,
+                selectedOption: '',
+              },
+              {
+                name: criteria[2].name,
+                orderNum: criteria[2].orderNum,
+                feedback: '',
+                selectedOption: '',
+              },
+            ]);
+          });
+        });
       });
     });
   });

src/data/redux/thunkActions/grading.js

@@ -49,11 +49,8 @@ export const loadSubmission = () => (dispatch, getState) => {
       dispatch(actions.grading.loadSubmission({ ...response, submissionUUID }));
       if (selectors.grading.selected.isGrading(getState())) {
         dispatch(actions.app.setShowRubric(true));
-        // safety constraints
         let { gradeData } = response;
-        if (gradeData === null || gradeData === undefined || Object.keys(gradeData).length) {
-          gradeData = selectors.app.emptyGrade(getState());
-        }
+        gradeData = selectors.app.fillGradeData(getState(), gradeData);
         const lockStatus = selectors.grading.selected.lockStatus(getState());
         dispatch(actions.grading.startGrading({ lockStatus, gradeData }));
       }
@@ -76,10 +73,7 @@ export const startGrading = () => (dispatch, getState) => {
     onSuccess: (response) => {
       dispatch(actions.app.setShowRubric(true));
       let gradeData = selectors.grading.selected.gradeData(getState());
-      // safety constraints
-      if (gradeData === null || gradeData === undefined || Object.keys(gradeData).length) {
-        gradeData = selectors.app.emptyGrade(getState());
-      }
+      gradeData = selectors.app.fillGradeData(getState(), gradeData);
       dispatch(actions.grading.startGrading({ ...response, gradeData }));
     },
     onFailure: (error) => {

src/data/redux/thunkActions/grading.test.js

@@ -10,7 +10,7 @@ jest.mock('./requests', () => ({
 }));
 
 jest.mock('data/redux/app/selectors', () => ({
-  emptyGrade: (state) => ({ emptyGrade: state }),
+  fillGradeData: (state, data) => ({ fillGradeData: state, data }),
 }));
 
 jest.mock('data/redux/grading/selectors', () => ({
@@ -140,32 +140,22 @@ describe('grading thunkActions', () => {
       beforeEach(() => {
         dispatch.mockClear();
       });
-      test('dispatches startGrading with selected gradeData if truthy', () => {
+      const fillString = 'selectors.app.fillGradeData based on selected gradeData';
+      test(`dispatches startGrading w/ ${fillString}`, () => {
         actionArgs.onSuccess(startResponse);
         expect(dispatch.mock.calls).toContainEqual([
           actions.app.setShowRubric(true),
         ], [
           actions.grading.startGrading({
             ...startResponse,
-            gradeData: selectors.grading.selected.gradeData(testState),
+            gradeData: selectors.app.fillGradeData(
+              testState,
+              selectors.grading.selected.gradeData(testState),
+            ),
           }),
         ]);
         expect(dispatch.mock.calls).toContainEqual([actions.app.setShowRubric(true)]);
       });
-      test('dispatches startGrading with empty grade if selected gradeData is null', () => {
-        const emptyGrade = selectors.app.emptyGrade(testState);
-        const expected = [
-          actions.grading.startGrading({ ...startResponse, gradeData: emptyGrade }),
-        ];
-        selectors.grading.selected.gradeData.mockReturnValue(null);
-        actionArgs.onSuccess({ ...startResponse, gradeData: null });
-        expect(dispatch.mock.calls).toContainEqual(expected);
-        expect(dispatch.mock.calls).toContainEqual([actions.app.setShowRubric(true)]);
-        dispatch.mockClear();
-        actionArgs.onSuccess({ ...startResponse, gradeData: null });
-        expect(dispatch.mock.calls).toContainEqual(expected);
-        expect(dispatch.mock.calls).toContainEqual([actions.app.setShowRubric(true)]);
-      });
     });
   });