- Notifications
You must be signed in to change notification settings - Fork927
feat: Add suspend user action#1275
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -19,20 +25,42 @@ export const UsersPage: React.FC = () => { | |||
| usersSend("GET_USERS") | |||
| }, [usersSend]) | |||
| if (usersState.matches("error")) { | |||
| return <ErrorSummary error={getUsersError} /> | |||
BrunoQuaresmaMay 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I moved this to be inside of UsersPage, so the getUsersError does not block the UI(not showing the table) for the other non-blocking errors like the suspendUserError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My idea for that was to not go to theerror state for non-blocking errors. This may work fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I didn't understand. Without removing this statement, any error will remove the user table from the screen and show the error summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm suggesting that every error gets assigned to context so that we can display error messages, but that only blocking errors put the page in theerror finite state. I see that state as meaning "the page is unusable."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ahh I see it, good to know!
| // Passing API.getUsers directly does not invoke the function properly | ||
| // when it is mocked. This happen in the UsersPage tests inside of the | ||
| // "shows a success message and refresh the page" test case. | ||
| getUsers: () => API.getUsers(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Idk why this is happening, but I guess jest lost the reference when it is passed directly. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What happened? I've seen some weird behavior in tests so this might be a clue :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No idea, but I would guess when you pass it directly, Jest lost the reference. Maybe I should open an issue in the jest repo, but I'm not sure if it is a problem on their side on something in the XState engine... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ahh, I saw some of the errors that you were having last week, but they were related to some components trying to access some theme variables without being rendered inside of a ThemeProvider component.
| @@ -58,6 +58,7 @@ | |||
| "@storybook/addon-essentials": "6.4.22", | |||
| "@storybook/addon-links": "6.4.22", | |||
| "@storybook/react": "6.4.22", | |||
| "@testing-library/jest-dom": "5.16.4", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This adds some helpful test matchers!
codecovbot commentedMay 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1275 +/- ##==========================================+ Coverage 66.10% 66.11% +0.01%========================================== Files 281 277 -4 Lines 18400 18224 -176 Branches 216 220 +4 ==========================================- Hits 12163 12049 -114+ Misses 4974 4931 -43+ Partials 1263 1244 -19
Continue to review full report at Codecov.
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -108,6 +156,9 @@ export const usersMachine = createMachine( | |||
| assignGetUsersError: assign({ | |||
| getUsersError: (_, event) => event.data, | |||
| }), | |||
| assignUserIdToSuspend: assign({ | |||
| userIdToSuspend: (_, event) => event.userId, | |||
| }), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we also clear this user afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An issue with clearing this right after is the text will blink/disappear before the modal gets completely closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, the next thing I would try is clearing the user on exiting or entering a state to see if that avoids the flash, but if it doesn't work we can just leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What potential problems do you see with keeping it? On exiting, the flash happens. On entering there is no need because a new user will be assigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was thinking on enteringidle. It's not creating a problem now, but I thought there was potential to create a bug down the line because when you're not mid-suspend, you wouldn't expect to have a user to suspend in state. Sounds like it's more trouble than it's worth right now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, the issue with making that on entering the idle state is because it "breaks" the modal during the close transition. The text disappears before it gets fully closed.
Uh oh!
There was an error while loading.Please reload this page.
| error={getUsersError} | ||
| /> | ||
| <ConfirmDialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is so clean, love it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think there's a need to move the error check from the page to the view now that the suspend error goes toidle, but I'm fine with it either way. Looks great!
Closes#738