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

Add: Reducer Test for feedback.js #5683

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhjiang1103
Copy link

What this PR does

This PR revises the feedback.js file. It creates a new custom array for newState[action.assignmentId], including the existing items (if any) and the new feedback item. This ensures that the state is not mutated directly, avoiding the "Cannot add property 0, object is not extensible" error. This PR also adds a test file for reducer feedback.js

Screenshots

Before:
Screen Shot 2024-02-18 at 11 41 11 PM

After:
Screen Shot 2024-02-27 at 11 13 34 PM

#2082

@@ -15,12 +15,22 @@ export default function feedback(state = initialState, action) {
}
case POST_USER_FEEDBACK: {
const newState = { ...state };
newState[action.assignmentId].custom.push({ message: action.feedback, messageId: action.messageId, userId: action.userId });
const assignmentId = action.assignmentId;
Copy link
Member

Choose a reason for hiding this comment

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

The code for these actions is pretty complex and it's hard to reason about how it's supposed to work. Could you add a comment illustrating the typical structure of the feedback state? That would make the code easier to think about.

Copy link
Author

Choose a reason for hiding this comment

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

@ragesoss I add comments to explain the structure of the feedback state.

  1. The original method uses push() to add an object to an array within an object in a state variable. It directly mutates the custom array inside the newState[action.assignmentId] object. I use the spread operator and object destructuring to create a new object with the updated custom array. It does not mutate the existing state but instead creates a new object with the updated custom array.
  2. Previous code uses splice(), which mutates the original array by removing elements and potentially adding new ones. I use filter() method to create a new array containing only the elements that pass the test implemented by the provided function. It does not modify the original array.

Hope this explains the revision clearly. Thank you.

@ragesoss
Copy link
Member

ragesoss commented Mar 4, 2024

Great! Have you tested the UI flow of adding a feedback comment and viewing an existing one? Since this also changes the reducer in addition to adding tests, I want to double-check that we're not introducing a regression.

@zhjiang1103
Copy link
Author

@ragesoss The UI appears to be working fine on my end. I've noticed that the CI build has failed. Could you please inform me about the error? I'm not sure why it failed.

@ragesoss
Copy link
Member

ragesoss commented Mar 5, 2024

That CI failure is unrelated (and is already fixed on the main branch).

Can you post a video of before and after of using the feedback UI feature?

@zhjiang1103
Copy link
Author

@ragesoss I've just realized that I was searching for the wrong UI element. I've created a new course but can't seem to find the assignment and feedback options. Could you please guide me on where to locate the feedback UI feature?

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

Successfully merging this pull request may close these issues.

None yet

2 participants