change: [M3-8511] - Refactor and Improve the User Details Page (#10861)

* initial refactor of user details page * fix existing cypress tests * fix faulty cypress test * add some more unit testing * Added changeset: Refactor and Improve the User Details Page --------- Co-authored-by: Banks Nussman <banks@nussman.us>
linode · Sep 3, 2024 · 990fa49 · 990fa49
1 parent e338cc5
commit 990fa49
Show file tree

Hide file tree

Showing 17 changed files with 618 additions and 494 deletions.
diff --git a/packages/manager/.changeset/pr-10861-changed-1725028189510.md b/packages/manager/.changeset/pr-10861-changed-1725028189510.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Changed
+---
+
+Refactor and Improve the User Details Page ([#10861](https://github.com/linode/manager/pull/10861))
diff --git a/packages/manager/cypress/e2e/core/account/user-profile.spec.ts b/packages/manager/cypress/e2e/core/account/user-profile.spec.ts
@@ -36,8 +36,8 @@ describe('User Profile', () => {
       cy.visitWithLogin(`account/users/${activeUsername}`);
       cy.wait('@getUser');
 
-      cy.findByText('Username').should('be.visible');
-      cy.findByText('Email').should('be.visible');
+      cy.findByLabelText('Username').should('be.visible');
+      cy.findByLabelText('Email').should('be.visible');
       cy.findByText('Delete User').should('be.visible');
 
       // Confirm the currently active user cannot be deleted.
@@ -125,8 +125,8 @@ describe('User Profile', () => {
 
       cy.wait('@getUser');
 
-      cy.findByText('Username').should('be.visible');
-      cy.findByText('Email').should('be.visible');
+      cy.findByLabelText('Username').should('be.visible');
+      cy.findByLabelText('Email').should('be.visible');
       cy.findByText('Delete User').should('be.visible');
       ui.button.findByTitle('Delete').should('be.visible').should('be.enabled');
 
@@ -210,8 +210,8 @@ describe('User Profile', () => {
 
       cy.wait('@getUser');
 
-      cy.findByText('Username').should('be.visible');
-      cy.findByText('Email').should('be.visible');
+      cy.findByLabelText('Username').should('be.visible');
+      cy.findByLabelText('Email').should('be.visible');
       cy.findByText('Delete User').should('be.visible');
 
       cy.get('[id="username"]')

diff --git a/packages/manager/cypress/e2e/core/account/users-landing-page.spec.ts b/packages/manager/cypress/e2e/core/account/users-landing-page.spec.ts
@@ -444,7 +444,6 @@ describe('Users landing page', () => {
     });
 
     mockGetUsers([mockUser]).as('getUsers');
-    mockGetUser(mockUser);
     mockGetUserGrantsUnrestrictedAccess(mockUser.username);
     mockAddUser(newUser).as('addUser');
 
@@ -488,6 +487,8 @@ describe('Users landing page', () => {
       .should('be.enabled')
       .click();
 
+    mockGetUser(newUser).as('getUser');
+
     // confirm to add a new user
     ui.drawer
       .findByTitle('Add a User')
@@ -535,10 +536,8 @@ describe('Users landing page', () => {
     cy.wait('@addUser').then((intercept) => {
       expect(intercept.request.body['restricted']).to.equal(newUser.restricted);
     });
-    cy.wait('@getUsers');
 
-    // the new user is displayed in the user list
-    cy.findByText(newUser.username).should('be.visible');
+    cy.wait('@getUser');
 
     // redirects to the new user's "User Permissions" page
     cy.url().should('endWith', `/users/${newUser.username}/permissions`);

diff --git a/packages/manager/src/features/Users/UserDetail.tsx b/packages/manager/src/features/Users/UserDetail.tsx
@@ -1,215 +1,56 @@
-import { User, getUser, updateUser } from '@linode/api-v4/lib/account';
-import { updateProfile } from '@linode/api-v4/lib/profile';
-import { APIError } from '@linode/api-v4/lib/types';
 import { useQueryClient } from '@tanstack/react-query';
-import { clone } from 'ramda';
 import * as React from 'react';
-import {
-  matchPath,
-  useHistory,
-  useLocation,
-  useParams,
-} from 'react-router-dom';
+import { useHistory, useLocation, useParams } from 'react-router-dom';
 
+import { CircleProgress } from 'src/components/CircleProgress';
 import { ErrorState } from 'src/components/ErrorState/ErrorState';
 import { LandingHeader } from 'src/components/LandingHeader';
-import { Notice } from 'src/components/Notice/Notice';
 import { SafeTabPanel } from 'src/components/Tabs/SafeTabPanel';
 import { TabLinkList } from 'src/components/Tabs/TabLinkList';
 import { TabPanels } from 'src/components/Tabs/TabPanels';
 import { Tabs } from 'src/components/Tabs/Tabs';
-import { accountQueries } from 'src/queries/account/queries';
 import { useAccountUser } from 'src/queries/account/users';
 import { useProfile } from 'src/queries/profile/profile';
-import { getAPIErrorOrDefault } from 'src/utilities/errorUtils';
 
 import UserPermissions from './UserPermissions';
-import { UserProfile } from './UserProfile';
+import { UserProfile } from './UserProfile/UserProfile';
 
 export const UserDetail = () => {
-  const { username: currentUsername } = useParams<{ username: string }>();
-  const location = useLocation<{ newUsername: string; success: boolean }>();
+  const { username } = useParams<{ username: string }>();
+  const location = useLocation();
   const history = useHistory();
 
-  const { data: profile, refetch: refreshProfile } = useProfile();
-  const { data: user } = useAccountUser(currentUsername ?? '');
+  const { data: profile } = useProfile();
+  const { data: user, error, isLoading } = useAccountUser(username ?? '');
 
   const queryClient = useQueryClient();
 
-  const [error, setError] = React.useState<string | undefined>();
-  const [username, setUsername] = React.useState<string>('');
-  const [createdUsername, setCreatedUsername] = React.useState<
-    string | undefined
-  >();
-  const [originalUsername, setOriginalUsername] = React.useState<
-    string | undefined
-  >();
-  const [originalEmail, setOriginalEmail] = React.useState<
-    string | undefined
-  >();
-  const [email, setEmail] = React.useState<string | undefined>('');
-  const [restricted, setRestricted] = React.useState<boolean>(false);
-
-  const [accountSaving, setAccountSaving] = React.useState<boolean>(false);
-  const [accountSuccess, setAccountSuccess] = React.useState<boolean>(false);
-  const [accountErrors, setAccountErrors] = React.useState<
-    APIError[] | undefined
-  >();
-  const [profileSaving, setProfileSaving] = React.useState<boolean>(false);
-  const [profileSuccess, setProfileSuccess] = React.useState<boolean>(false);
-  const [profileErrors, setProfileErrors] = React.useState<
-    APIError[] | undefined
-  >();
-
   const tabs = [
-    /* NB: These must correspond to the routes inside the Switch */
     {
-      routeName: `/account/users/${currentUsername}/profile`,
+      routeName: `/account/users/${username}/profile`,
       title: 'User Profile',
     },
     {
-      routeName: `/account/users/${currentUsername}/permissions`,
+      routeName: `/account/users/${username}/permissions`,
       title: 'User Permissions',
     },
   ];
 
-  React.useEffect(() => {
-    getUser(currentUsername)
-      .then((user) => {
-        setOriginalUsername(user.username);
-        setUsername(user.username);
-        setOriginalEmail(user.email);
-        setEmail(user.email);
-        setRestricted(user.restricted);
-      })
-      .catch((errorResponse) => {
-        setError(
-          getAPIErrorOrDefault(errorResponse, 'Error loading user data.')[0]
-            .reason
-        );
-      });
-
-    if (location.state) {
-      setAccountSuccess(clone(location.state.success));
-      setCreatedUsername(clone(location.state.newUsername));
-      history.replace({
-        pathname: history.location.pathname,
-        state: {},
-      });
-    }
-  }, []);
-
-  const clearNewUser = () => {
-    setCreatedUsername(undefined);
-  };
-
-  const handleChangeUsername = (
-    e:
-      | React.ChangeEvent<HTMLInputElement>
-      | React.FocusEvent<HTMLInputElement | HTMLTextAreaElement>
-  ) => {
-    setUsername(e.target.value);
-  };
-
-  const handleChangeEmail = (e: React.ChangeEvent<HTMLInputElement>) => {
-    setEmail(e.target.value);
-  };
-
-  const handleSaveAccount = () => {
-    if (!originalUsername) {
-      return;
-    }
-
-    setAccountSuccess(false);
-    setAccountSaving(true);
-    setAccountErrors([]);
-    setProfileSuccess(false);
-    setProfileErrors([]);
-
-    updateUser(originalUsername, { restricted, username })
-      .then((user) => {
-        /**
-         * Update the state of the component with the updated information.
-         */
-        setOriginalUsername(user.username);
-        setUsername(user.username);
-        setAccountSaving(false);
-        setAccountErrors(undefined);
-
-        /**
-         * If the user we updated is the current user, we need to reflect that change at the global level.
-         * Otherwise, refetch the account's users so it has the new username
-         */
-        if (profile?.username === originalUsername) {
-          refreshProfile();
-        } else {
-          // Invalidate the paginated store
-          queryClient.invalidateQueries({
-            queryKey: accountQueries.users._ctx.paginated._def,
-          });
-          // set the user in the store
-          queryClient.setQueryData<User>(
-            accountQueries.users._ctx.user(user.username).queryKey,
-            user
-          );
-        }
-
-        history.replace(`/account/users/${user.username}`, {
-          success: true,
-        });
-      })
-      .catch((errResponse) => {
-        setAccountErrors(
-          getAPIErrorOrDefault(errResponse, 'Error updating username')
-        );
-        setAccountSaving(false);
-        setAccountSuccess(false);
-      });
-  };
-
-  const handleSaveProfile = () => {
-    setProfileSuccess(false);
-    setProfileSaving(true);
-    setProfileErrors([]);
-    setAccountSuccess(false);
-    setAccountErrors([]);
-
-    updateProfile({ email })
-      .then((profile) => {
-        setProfileSaving(false);
-        setProfileSuccess(true);
-        setProfileErrors(undefined);
-        /**
-         * If the user we updated is the current user, we need to reflect that change at the global level.
-         */
-        if (profile.username === originalUsername) {
-          refreshProfile();
-        }
-      })
-      .catch((errResponse) => {
-        setProfileErrors(
-          getAPIErrorOrDefault(errResponse, 'Error updating email')
-        );
-        setProfileSaving(false);
-        setProfileSuccess(false);
-      });
-  };
-
-  const matches = (p: string) => {
-    return Boolean(matchPath(p, { path: location.pathname }));
-  };
-
-  const navToURL = (index: number) => {
-    history.push(tabs[index].routeName);
-  };
+  const currentTabIndex = tabs.findIndex((tab) =>
+    location.pathname.includes(tab.routeName)
+  );
 
   const isProxyUser = user?.user_type === 'proxy';
 
+  if (isLoading) {
+    return <CircleProgress />;
+  }
+
   if (error) {
     return (
       <React.Fragment>
         <LandingHeader title={username || ''} />
-        <ErrorState errorText={error} />
+        <ErrorState errorText={error[0].reason} />
       </React.Fragment>
     );
   }
@@ -227,43 +68,17 @@ export const UserDetail = () => {
         title={username}
       />
       <Tabs
-        index={Math.max(
-          tabs.findIndex((tab) => matches(tab.routeName)),
-          0
-        )}
-        onChange={navToURL}
+        index={currentTabIndex === -1 ? 0 : currentTabIndex}
+        onChange={(index) => history.push(tabs[index].routeName)}
       >
         {!isProxyUser && <TabLinkList tabs={tabs} />}
-
-        {createdUsername && (
-          <Notice
-            text={`User ${createdUsername} created successfully`}
-            variant="success"
-          />
-        )}
         <TabPanels>
           <SafeTabPanel index={0}>
-            <UserProfile
-              accountErrors={accountErrors}
-              accountSaving={accountSaving}
-              accountSuccess={accountSuccess || false}
-              changeEmail={handleChangeEmail}
-              changeUsername={handleChangeUsername}
-              email={email}
-              originalEmail={originalEmail}
-              originalUsername={originalUsername}
-              profileErrors={profileErrors}
-              profileSaving={profileSaving}
-              profileSuccess={profileSuccess || false}
-              saveAccount={handleSaveAccount}
-              saveProfile={handleSaveProfile}
-              username={username}
-            />
+            <UserProfile />
           </SafeTabPanel>
           <SafeTabPanel index={1}>
             <UserPermissions
               accountUsername={profile?.username}
-              clearNewUser={clearNewUser}
               currentUsername={username}
               queryClient={queryClient}
             />

diff --git a/packages/manager/src/features/Users/UserPermissions.tsx b/packages/manager/src/features/Users/UserPermissions.tsx
@@ -61,7 +61,6 @@ import {
 import { UserPermissionsEntitySection } from './UserPermissionsEntitySection';
 interface Props {
   accountUsername?: string;
-  clearNewUser: () => void;
   currentUsername?: string;
   queryClient: QueryClient;
 }
@@ -429,7 +428,7 @@ class UserPermissions extends React.Component<CombinedProps, State> {
     const isProxyUser = this.state.userType === 'proxy';
 
     return (
-      <Box sx={{ marginTop: (theme) => theme.spacing(4) }}>
+      <Box>
         {generalError && (
           <Notice spacingTop={8} text={generalError} variant="error" />
         )}
@@ -690,7 +689,7 @@ class UserPermissions extends React.Component<CombinedProps, State> {
 
   savePermsType = (type: keyof Grants) => () => {
     this.setState({ errors: undefined });
-    const { clearNewUser, currentUsername } = this.props;
+    const { currentUsername } = this.props;
     const { grants } = this.state;
     if (!currentUsername || !(grants && grants[type])) {
       return this.setState({
@@ -702,8 +701,6 @@ class UserPermissions extends React.Component<CombinedProps, State> {
       });
     }
 
-    clearNewUser();
-
     if (type === 'global') {
       this.setState({ isSavingGlobal: true });
       updateGrants(currentUsername, { global: grants.global })