feat: Add suspend user action #1275
Conversation
| @@ -19,20 +25,42 @@ export const UsersPage: React.FC = () => { | |||
| usersSend("GET_USERS") | |||
| }, [usersSend]) | |||
|
|
|||
| if (usersState.matches("error")) { | |||
| return <ErrorSummary error={getUsersError} /> | |||
There was a problem hiding this comment.
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.
My idea for that was to not go to the error state for non-blocking errors. This may work fine too.
There was a problem hiding this comment.
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.
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 the error finite state. I see that state as meaning "the page is unusable."
There was a problem hiding this comment.
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.
Idk why this is happening, but I guess jest lost the reference when it is passed directly. 🤔
There was a problem hiding this comment.
What happened? I've seen some weird behavior in tests so this might be a clue :)
There was a problem hiding this comment.
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.
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.
This adds some helpful test matchers!
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.
|
| @@ -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.
Can we also clear this user afterwards?
There was a problem hiding this comment.
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.
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.
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.
I was thinking on entering idle. 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.
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.
| error={getUsersError} | ||
| /> | ||
|
|
||
| <ConfirmDialog |
Closes #738