Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor user reducers and actions using redux toolkit #3127

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

PiyushChandra17
Copy link
Contributor

Associated issue: #2042

Changes:

  • Deleted all the corresponding constants
  • Used createSlice to rewrite user reducers
  • Exported the automatically generated action creator functions
  • Deleted the existing action creator functions

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of thoughts here:

You need to be very cognizant of what the payloads are. The existing code has authenticateUser(response.data) and export function authenticateUser(user) so we can infer that response.data is the user object. That means your authUser reducer isn't right because you are expecting user to be a property of the action.payload when in fact the entire action.payload is the user.

You don't need to do ...state stuff when using createSlice. Just update the property that you want to update and don't return anything. But I think you get the point by now if you see this after my other reviews.

This is a place where createAsyncThunk would be very helpful because we currently have actions for the various stages of the process (verify/verified/invalid). We would call createAsyncThunk once and that would create a verifyEmail action. In the reducer we would respond independently (using extraReducers) to verifyEmail.pending, verifyEmail.fulfilled, and verifyEmail.rejected.

There are things in the existing code that are silly and/or unnecessary. The reducer always sets the status to 'checking' when encountering the EMAIL_VERIFICATION_VERIFY action type. So we don't need to include state: 'checking' in the action. Likewise, EMAIL_VERIFICATION_VERIFIED and EMAIL_VERIFICATION_INVALID both include a message that is not used, though I can see how we might want to use that for a toast or something in the future.

Every property other than authenticated is optional in the current code. The initial state right now is just { authenticated: false }. I do like that you include all of the properties in your version of the initialState though because it makes the structure much clearer. But in that case I would change your unauthUser and authError actions to return => initialState instead of => ({ authenticated: false }) so that we keep those properties present. I did check the codebase to make sure that changing emailVerificationTokenState from not present (undefined) to null won't break anything and that seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants