-
Notifications
You must be signed in to change notification settings - Fork 3
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
Ops 441 update user details #2671
Conversation
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
… INACTIVE or LOCKED Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
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.
🥇 looks good @johndeange, made some non-blocking suggestions. Would be nice to add some UX like confirmation Alert (if changes were applied)and Cancel/Save buttons to allow user to back out of changes.
* @returns {JSX.Element} - The rendered component. | ||
*/ | ||
function UserEmailComboBox({ selectedUsers, setSelectedUsers }) { | ||
// const [selectedUsers, setSelectedUsers] = React.useState([]); |
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.
pls remove commented code
@@ -16,6 +23,7 @@ const UserInfo = ({ user, isEditable }) => { | |||
|
|||
const { data: divisions, error: errorDivisions, isLoading: isLoadingDivisions } = useGetDivisionsQuery(); | |||
const { data: roles, error: errorRoles, isLoading: isLoadingRoles } = useGetRolesQuery(); | |||
const [updateUser, updateUserResult] = useUpdateUserMutation(); |
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.
⭐ thx for sharing this pattern for handling result from the api call
|
||
console.log("isEditable: ", isEditable); | ||
if (updateUserResult.isError) { | ||
return <div>Oops, an error occurred</div>; |
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.
wonder if we can render this on the page somewhere?
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.
It is - the div is displayed in the case of an API error
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.
ok I guess I was envisioning an error Alert with a message to try again or the div to be displayed above the User card.
// eslint-disable-next-line testing-library/no-node-access | ||
const divisionInput = divisionComboBox.querySelector("input"); | ||
|
||
fireEvent.keyDown(divisionInput, { key: "ArrowDown", code: 40 }); |
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.
learned that the [userEvent](https://testing-library.com/docs/user-event/intro/)
is now the preferred method of handling user interactions
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 have been adding the extension of jsx
to unit-test files since they render a React component.
@@ -1,26 +1,37 @@ | |||
import App from "../../../App"; | |||
import UserEmailComboBox from "../../../components/Users/UserEmailComboBox/index.js"; | |||
import { useGetDivisionsQuery } from "../../../api/opsAPI.js"; | |||
import React from "react"; | |||
import UserInfo from "../../../components/Users/UserInfo/UserInfo.jsx"; |
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.
don't need the double UserInfo on the import
../../../components/Users/UserInfo"
Signed-off-by: John DeAngelis <[email protected]>
…o OPS-441-update-user-details
url: `/users/${id}`, | ||
method: "PUT", | ||
method: "PATCH", |
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.
🤔 does the openAPI spec need to be updated to match?
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 followed the 'How to Test' steps, and the changes worked as expected.
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
Signed-off-by: John DeAngelis <[email protected]>
What changed
USER_ADMIN
role changingUser
details.UserSession
when changingUserStatus
toINACTIVE/LOCKED
Issue
#441
How to test
User Admin
page.LOCKED
ACTIVE
Screenshots
Definition of Done Checklist