Port Login to use redux-toolkit

[imnasnainaec]

Part of #1953

Uses https://jestjs.io/docs/timer-mocks

---

This change is 

[codecov-commenter]

Codecov Report

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

Files	Coverage Δ
src/components/App/DefaultState.ts	100.00% <ø> (ø)
src/components/Login/LoginPage/LoginComponent.tsx	58.62% <ø> (ø)
src/components/Login/Redux/LoginReducer.ts	100.00% <100.00%> (ø)
...rc/components/Login/SignUpPage/SignUpComponent.tsx	51.11% <ø> (ø)
src/components/Login/SignUpPage/index.ts	0.00% <ø> (ø)
src/rootReducer.ts	100.00% <ø> (ø)
src/components/Login/LoginPage/index.ts	0.00% <0.00%> (ø)
src/components/Login/Redux/LoginActions.ts	92.00% <83.33%> (ø)
src/components/ProjectInvite/ProjectInvite.tsx	0.00% <0.00%> (ø)

... and 194 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[jmgrady]

Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @imnasnainaec)

---

src/components/Login/LoginPage/index.ts line 19 at r2 (raw file):

  return {
    loginAttempt: state.loginState.loginStatus === LoginStatus.Attempt,
    loginFailure: state.loginState.loginStatus === LoginStatus.Failure,

Since these Booleans are mutually exclusive, why not just provide the loginStatus as a property? Is there no case where one needs to distinguish between Default and Success when both of these are false?

Code quote:

    loginAttempt: state.loginState.loginStatus === LoginStatus.Attempt,
    loginFailure: state.loginState.loginStatus === LoginStatus.Failure,

---

src/components/Login/LoginPage/index.ts line 26 at r2 (raw file):

  return {
    login: (username: string, password: string) => {
      dispatch(asyncLogIn(username, password));

Login is more consistent with most if not all the other uses in this component. It's also more consistent with the change of SignUp to Signup.

Code quote:

LogIn

---

src/components/Login/Redux/LoginReducer.ts line 22 at r2 (raw file):

      state.error = action.payload;
      state.loginStatus = LoginStatus.Failure;
    },

Consider clearing the username on failure. Leaving old data lying around can cause problems but it can also be useful to have the "last login attempt" available - especially when debugging. I do not see anything in the current implementation where it would be a problem to not clear the username so it is fine to leave things as they are if you prefer.

The same applies for setSignupFailureAction.

Code quote:

      state.error = action.payload;
      state.loginStatus = LoginStatus.Failure;
    },

---

src/components/Login/Redux/LoginReduxTypes.ts line 6 at r2 (raw file):

  Failure = "Failure",
  Success = "Success",
}

The status names are really actions. As the code in src/components/Login/SignUpPage/index.ts suggests, better status names would be

export enum LoginStatus {
  Complete = "Complete",  // normally I would prefer LoggedIn but this enum is also used for SignupState
  Default = "Default",    // similarly, I would prefer LoggedOut
  Failed = "Failed",
  InProgress = "InProgress",
}

I don't have strong objections but am blocking so that we can discuss.

Code quote:

export enum LoginStatus {
  Attempt = "Attempt",
  Default = "Default",
  Failure = "Failure",
  Success = "Success",
}

---

src/components/Login/Redux/LoginReduxTypes.ts line 12 at r2 (raw file):

  loginStatus: LoginStatus;
  signupStatus: LoginStatus;
  username: string;

Along the lines of my comment on src/components/Login/Redux/LoginReducer.ts, if the username is not cleared on failure, it could be helpful to add a comment here that states the value is only valid in the Attempt, and Success states.

Code quote:

  username: string;

---

src/components/Login/Redux/tests/LoginActions.test.tsx line 55 at r2 (raw file):

      expect(loginState.loginStatus).toEqual(LoginStatus.Failure);
      expect(loginState.signupStatus).toEqual(LoginStatus.Default);
      expect(loginState.username).toEqual(mockUsername);

Reminder: This would need to change if username is cleared on failure.

Code quote:

      expect(loginState.username).toEqual(mockUsername);

---

src/components/Login/Redux/tests/LoginActions.test.tsx line 81 at r2 (raw file):

      expect(loginState.loginStatus).toEqual(LoginStatus.Default);
      expect(loginState.signupStatus).toEqual(LoginStatus.Failure);
      expect(loginState.username).toEqual(mockUsername);

Reminder: This would need to change if username is cleared on failure.

Code quote:

     expect(loginState.username).toEqual(mockUsername);

[imnasnainaec]

Reviewable status: 10 of 16 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

---

src/components/Login/LoginPage/index.ts line 26 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

Login is more consistent with most if not all the other uses in this component. It's also more consistent with the change of SignUp to Signup.

"login"/"signup" with a lowercase "i"/"u" is a noun or adjective. I was going with asyncLogIn/asyncSignUp with capital "I"/"U" thinking of them as the verb phrases "log in" and "sign up". Though I guess I'm not consistent with that thinking everywhere.

[jmgrady]

Reviewed 6 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

[imnasnainaec]

Reviewable status: 11 of 16 files reviewed, 1 unresolved discussion (waiting on @jmgrady)

---

src/components/Login/Redux/LoginReduxTypes.ts line 6 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

The status names are really actions. As the code in src/components/Login/SignUpPage/index.ts suggests, better status names would be

export enum LoginStatus {
  Complete = "Complete",  // normally I would prefer LoggedIn but this enum is also used for SignupState
  Default = "Default",    // similarly, I would prefer LoggedOut
  Failed = "Failed",
  InProgress = "InProgress",
}

I don't have strong objections but am blocking so that we can discuss.

Partially done.

[jmgrady]

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