Port CharInv goal to use redux-toolkit

[imnasnainaec]

Part of #1953

---

This change is 

[codecov-commenter]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
src/components/App/DefaultState.ts	100.00% <ø> (ø)
...CharacterInventory/CharInv/CharacterList/index.tsx	65.51% <100.00%> (ø)
src/goals/CharacterInventory/CharInv/index.tsx	92.85% <100.00%> (ø)
src/rootReducer.ts	100.00% <ø> (ø)
...racterInventory/Redux/CharacterInventoryActions.ts	91.30% <85.71%> (+48.56%)	⬆️
...racterInventory/Redux/CharacterInventoryReducer.ts	75.55% <75.60%> (+53.93%)	⬆️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!

[jmgrady]

Reviewed 4 of 11 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec)

---

src/goals/CharacterInventory/Redux/CharacterInventoryActions.ts line 161 at r2 (raw file):

    console.error(`countOccurrences expects length 1 char, but got: ${char}`);
  }
  let count = 0;

This adds the test to verify that the "character" is a single character but does nothing to alert the user that there is a problem. Presumably this was added because there have been problems with incorrect char inputs. Can that be prevented?

---

src/goals/CharacterInventory/Redux/CharacterInventoryActions.ts line 239 at r2 (raw file):

  project.rejectedCharacters = state.characterInventoryState.rejectedCharacters;
  return project;
}

This would be cleaner as a dispatch to the reducer for the currentProjectState with the validCharacters and rejectedCharacters as the payload. For one, there would be no need to make a copy of the project when using the redux-toolkit.

Code quote:

function updateCurrentProject(state: StoreState): Project {
  const project = { ...state.currentProjectState.project };
  project.validCharacters = state.characterInventoryState.validCharacters;
  project.rejectedCharacters = state.characterInventoryState.rejectedCharacters;
  return project;
}

---

src/goals/CharacterInventory/Redux/CharacterInventoryReducer.ts line 57 at r2 (raw file):

      }
    },
    resetAction: () => defaultState,

This is too easy to confuse with the global reset action. The name should be more specific, such as clearCharInventory or resetCharInventory. Same goes for the reset() function in CharacterInventoryActions.ts

Code quote:

resetAction: () => defaultState,

---

src/goals/CharacterInventory/Redux/CharacterInventoryReducer.ts line 67 at r2 (raw file):

    },
    setRejectedCharactersAction: (state, action) => {
      state.rejectedCharacters = [...new Set(action.payload as string)];

I do not believe as string is necessary. I added the following to setValidCharactersAction which uses the same construct:

const validCharArray = [...new Set(action.payload)];
console.log(`payload: ${JSON.stringify(validCharArray, null, 2)}`);
const validCharSpread = [...new Set(action.payload as string)];
console.log(
   `payload as string: ${JSON.stringify(validCharSpread, null, 2)}`
);

and the output is the same. (The structures are the same in the debugger, too).

Code quote:

      state.rejectedCharacters = [...new Set(action.payload as string)];

[jmgrady]

Reviewed 3 of 11 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @imnasnainaec)

---

src/goals/CharacterInventory/Redux/tests/CharacterInventoryActions.test.tsx line 149 at r2 (raw file):

      await store.dispatch(fetchWords());
      const { allWords } = store.getState().characterInventoryState;
      expect(allWords).toHaveLength(words.length);

Why not verify that the right 2 words were fetched?

Code quote:

      expect(allWords).toHaveLength(words.length);

---

src/goals/CharacterInventory/Redux/tests/CharacterInventoryActions.test.tsx line 164 at r2 (raw file):

      await store.dispatch(getAllCharacters());
      const { characterSet } = store.getState().characterInventoryState;
      expect(characterSet).toHaveLength(9);

As in fetchWords why not check that the correct character set was created?

Code quote:

      expect(characterSet).toHaveLength(9);

---

src/goals/CharacterInventory/Redux/tests/CharacterInventoryActions.test.tsx line 187 at r2 (raw file):

      expect(state.characterSet).toHaveLength(mockWord.vernacular.length);
      expect(state.rejectedCharacters).toHaveLength(rejectedCharacters.length);
      expect(state.validCharacters).toHaveLength(validCharacters.length);

... and here - check values rather than just length.

Code quote:

      expect(state.allWords).toHaveLength(1);
      expect(state.characterSet).toHaveLength(mockWord.vernacular.length);
      expect(state.rejectedCharacters).toHaveLength(rejectedCharacters.length);
      expect(state.validCharacters).toHaveLength(validCharacters.length);

[jmgrady]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @imnasnainaec)

---

src/goals/CharacterInventory/Redux/CharacterInventoryReducer.ts line 57 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

This is too easy to confuse with the global reset action. The name should be more specific, such as clearCharInventory or resetCharInventory. Same goes for the reset() function in CharacterInventoryActions.ts

In addition, this is only called when the component unmounts. This suggests to me that redux is not the correct technology for this problem. The useState (or useContext if the info needs to be propagated to child elements) should be used here instead.

This does not need to be resolved in this PR. I will create an issue for it where we can track our decision whether or not to change it.

[imnasnainaec]

Reviewable status: 8 of 13 files reviewed, 3 unresolved discussions (waiting on @jmgrady)

---

src/goals/CharacterInventory/Redux/CharacterInventoryActions.ts line 161 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

This adds the test to verify that the "character" is a single character but does nothing to alert the user that there is a problem. Presumably this was added because there have been problems with incorrect char inputs. Can that be prevented?

It's more a safe-guard against future changes. This function shouldn't be directly accessible from user-input so I wasn't worried about user feedback.

---

src/goals/CharacterInventory/Redux/CharacterInventoryActions.ts line 239 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

This would be cleaner as a dispatch to the reducer for the currentProjectState with the validCharacters and rejectedCharacters as the payload. For one, there would be no need to make a copy of the project when using the redux-toolkit.

The result of this is passed to a generic update-project action and uses the project in state.currentProjectState that is required anyway for getChanges. Adding a project action specific to this scenario requires introducing char-inv types to the project redux.

---

src/goals/CharacterInventory/Redux/CharacterInventoryReducer.ts line 57 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

In addition, this is only called when the component unmounts. This suggests to me that redux is not the correct technology for this problem. The useState (or useContext if the info needs to be propagated to child elements) should be used here instead.

This does not need to be resolved in this PR. I will create an issue for it where we can track our decision whether or not to change it.

Changed reset() to resetCharInv().

---

src/goals/CharacterInventory/Redux/CharacterInventoryReducer.ts line 67 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I do not believe as string is necessary. I added the following to setValidCharactersAction which uses the same construct:

const validCharArray = [...new Set(action.payload)];
console.log(`payload: ${JSON.stringify(validCharArray, null, 2)}`);
const validCharSpread = [...new Set(action.payload as string)];
console.log(
   `payload as string: ${JSON.stringify(validCharSpread, null, 2)}`
);

and the output is the same. (The structures are the same in the debugger, too).

Having just state.rejectedCharacters = [...new Set(action.payload)]; gives TS error Type 'unknown[]' is not assignable to type 'string[]'. But I did change it from as string to as string[]. The former satisfied TS linting but wasn't correct.

[jmgrady]

Reviewed 5 of 5 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)