Users search fix (#2847)

# Changes - Prevent duplicated call to /users on search - Fix the issue described at #2842 where there was a concurrency issue between the page load request and the search request ## Which issue(s) this PR fixes Fix for https://github.com/grafana/oncall/issues/2842 ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
2023-08-22 16:30:42 +03:00 · 2023-08-22 16:30:42 +03:00 · b8a04b69aa
commit b8a04b69aa
parent b9a9cd2659
6 changed files with 137 additions and 119 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Changed HTTP Endpoint to Email for inbound email integrations
  ([#2816](https://github.com/grafana/oncall/issues/2816))
 - Enable inbound email feature flag by default by @vadimkerr ([#2846](https://github.com/grafana/oncall/pull/2846))
+- Fixed initial search on Users page ([#2842](https://github.com/grafana/oncall/issues/2842))

 ## v1.3.25 (2023-08-18)

--- a/grafana-plugin/e2e-tests/users/usersActions.test.ts
+++ b/grafana-plugin/e2e-tests/users/usersActions.test.ts
@ -57,6 +57,20 @@ test.describe('Users screen actions', () => {
    await _testButtons(editorRolePage.page, 'button.edit-other-profile-button:not([disabled])');
  });

+  test('Search updates the table view', async ({ adminRolePage }) => {
+    const { page } = adminRolePage;
+    await goToOnCallPage(page, 'users');
+
+    const searchInput = page.locator(`[data-testid="search-users"]`);
+
+    await searchInput.fill('oncall');
+    await page.waitForTimeout(5000);
+
+    const result = page.locator(`[data-testid="users-username"]`);
+
+    expect(await result.count()).toBe(1);
+  });
+
  /*
   * Helper methods
   */
--- a/grafana-plugin/src/components/UsersFilters/UsersFilters.tsx
+++ b/grafana-plugin/src/components/UsersFilters/UsersFilters.tsx
@ -11,10 +11,11 @@ interface UsersFiltersProps {
  value: any;
  onChange: (filters: any) => void;
  className?: string;
+  isLoading?: boolean;
 }

 const UsersFilters = (props: UsersFiltersProps) => {
-  const { value = { searchTerm: '' }, onChange, className } = props;
+  const { value = { searchTerm: '' }, onChange, className, isLoading } = props;

  const onSearchTermChangeCallback = useCallback(
    (e: ChangeEvent<HTMLInputElement>) => {
@ -31,11 +32,13 @@ const UsersFilters = (props: UsersFiltersProps) => {
  return (
    <div className={cx('root', className)}>
      <Input
+        loading={isLoading}
        prefix={<Icon name="search" />}
        className={cx('search', 'control')}
        placeholder="Search users..."
        value={value.searchTerm}
        onChange={onSearchTermChangeCallback}
+        data-testid="search-users"
      />
    </div>
  );
--- a/grafana-plugin/src/pages/schedule/Schedule.tsx
+++ b/grafana-plugin/src/pages/schedule/Schedule.tsx
@ -1,15 +1,6 @@
 import React from 'react';

-import {
-  Button,
-  HorizontalGroup,
-  VerticalGroup,
-  IconButton,
-  ToolbarButton,
-  Icon,
-  Modal,
-  LoadingPlaceholder,
-} from '@grafana/ui';
+import { Button, HorizontalGroup, VerticalGroup, IconButton, ToolbarButton, Icon, Modal } from '@grafana/ui';
 import cn from 'classnames/bind';
 import dayjs from 'dayjs';
 import { observer } from 'mobx-react';
@ -48,9 +39,7 @@ import styles from './Schedule.module.css';

 const cx = cn.bind(styles);

-interface SchedulePageProps extends PageProps, WithStoreProps, RouteComponentProps<{ id: string }> {
-  basicDataLoaded: boolean;
-}
+interface SchedulePageProps extends PageProps, WithStoreProps, RouteComponentProps<{ id: string }> {}

 interface SchedulePageState extends PageBaseState {
  startMoment: dayjs.Dayjs;
@ -123,7 +112,6 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
      match: {
        params: { id: scheduleId },
      },
-      basicDataLoaded,
    } = this.props;

    const {
@ -160,9 +148,7 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
      !!shiftIdToShowOverridesForm ||
      shiftIdToShowRotationForm;

-    return !basicDataLoaded ? (
-      <LoadingPlaceholder text="Loading..." />
-    ) : (
+    return (
      <PageErrorHandlingWrapper errorData={errorData} objectName="schedule" pageName="schedules">
        {() => (
          <>
--- a/grafana-plugin/src/pages/users/Users.tsx
+++ b/grafana-plugin/src/pages/users/Users.tsx
@ -47,28 +47,34 @@ interface UsersState extends PageBaseState {
    searchTerm: string;
  };
  initialUsersLoaded: boolean;
+  queuedUpdateUsers: boolean;
 }

@observer
 class Users extends React.Component<UsersProps, UsersState> {
-  state: UsersState = {
-    page: 1,
-    isWrongTeam: false,
-    userPkToEdit: undefined,
-    usersFilters: {
-      searchTerm: '',
-    },
+  constructor(props: UsersProps) {
+    super(props);

-    errorData: initErrorDataState(),
-    initialUsersLoaded: false,
-  };
-
-  async componentDidMount() {
    const {
      query: { p },
-    } = this.props;
-    this.setState({ page: p ? Number(p) : 1 }, this.updateUsers);
+    } = props;

+    this.state = {
+      page: p ? Number(p) : 1,
+      isWrongTeam: false,
+      userPkToEdit: undefined,
+      usersFilters: {
+        searchTerm: '',
+      },
+
+      errorData: initErrorDataState(),
+      initialUsersLoaded: false,
+      queuedUpdateUsers: false,
+    };
+  }
+
+  async componentDidMount() {
+    this.updateUsers();
    this.parseParams();
  }

@ -84,14 +90,15 @@ class Users extends React.Component<UsersProps, UsersState> {
    LocationHelper.update({ p: page }, 'partial');
    await userStore.updateItems(usersFilters, page);

-    this.setState({ initialUsersLoaded: true });
+    const { queuedUpdateUsers } = this.state;
+    this.setState({ initialUsersLoaded: true, queuedUpdateUsers: false }, () => {
+      if (queuedUpdateUsers) {
+        this.updateUsers();
+      }
+    });
  };

  componentDidUpdate(prevProps: UsersProps) {
-    if (!this.state.initialUsersLoaded) {
-      this.updateUsers();
-    }
-
    if (prevProps.match.params.id !== this.props.match.params.id) {
      this.parseParams();
    }
@ -176,14 +183,15 @@ class Users extends React.Component<UsersProps, UsersState> {
    const {
      store: { userStore },
    } = this.props;
-    const { usersFilters, page, initialUsersLoaded, userPkToEdit } = this.state;
+
+    const { usersFilters, page, initialUsersLoaded, userPkToEdit, queuedUpdateUsers } = this.state;

    const { count, results } = userStore.getSearchResult();
    const columns = this.getTableColumns();

    const handleClear = () =>
      this.setState({ usersFilters: { searchTerm: '' } }, () => {
-        this.debouncedUpdateUsers();
+        this.updateUsers();
      });

    return (
@ -194,6 +202,7 @@ class Users extends React.Component<UsersProps, UsersState> {
              <UsersFilters
                className={cx('users-filters')}
                value={usersFilters}
+                isLoading={queuedUpdateUsers}
                onChange={this.handleUsersFiltersChange}
              />
              <Button variant="secondary" icon="times" onClick={handleClear} className={cx('searchIntegrationClear')}>
@ -425,6 +434,11 @@ class Users extends React.Component<UsersProps, UsersState> {

  handleUsersFiltersChange = (usersFilters: any) => {
    this.setState({ usersFilters, page: 1 }, () => {
+      if (!this.state.initialUsersLoaded) {
+        // queue delayed users update
+        return this.setState({ queuedUpdateUsers: true });
+      }
+
      this.debouncedUpdateUsers();
    });
  };
@ -435,10 +449,6 @@ class Users extends React.Component<UsersProps, UsersState> {

    history.push(`${PLUGIN_ROOT}/users`);
  };
-
-  handleUserUpdate = () => {
-    this.updateUsers();
-  };
 }

 export default withRouter(withMobXProviderContext(Users));
--- a/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx
+++ b/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx
@ -1,5 +1,6 @@
 import React, { useEffect, useState } from 'react';

+import { LoadingPlaceholder } from '@grafana/ui';
 import classnames from 'classnames';
 import dayjs from 'dayjs';
 import customParseFormat from 'dayjs/plugin/customParseFormat';
@ -76,6 +77,8 @@ export const Root = observer((props: AppRootProps) => {
    updateBasicData();
  }, []);

+  const location = useLocation();
+
  useEffect(() => {
    let link = document.createElement('link');
    link.type = 'text/css';
@ -101,13 +104,9 @@ export const Root = observer((props: AppRootProps) => {
    setBasicDataLoaded(true);
  };

-  const location = useLocation();
-
  const page = getMatchedPage(location.pathname);
-
  const pagePermissionAction = pages[page]?.action;
  const userHasAccess = pagePermissionAction ? isUserActionAllowed(pagePermissionAction) : true;
-
  const query = getQueryParams();

  return (
@ -126,80 +125,85 @@ export const Root = observer((props: AppRootProps) => {
        })}
      >
        {userHasAccess ? (
-          <Switch>
-            <Route path={getRoutesForPage('alert-groups')} exact>
-              <Incidents query={query} />
-            </Route>
-            <Route path={getRoutesForPage('alert-group')} exact>
-              <Incident query={query} />
-            </Route>
-            <Route path={getRoutesForPage('users')} exact>
-              <Users query={query} />
-            </Route>
-            <Route path={getRoutesForPage('integrations')} exact>
-              <Integrations query={query} />
-            </Route>
-            <Route path={getRoutesForPage('integration')} exact>
-              <Integration query={query} />
-            </Route>
-            <Route path={getRoutesForPage('escalations')} exact>
-              <EscalationChains query={query} />
-            </Route>
-            <Route path={getRoutesForPage('schedules')} exact>
-              <Schedules query={query} />
-            </Route>
-            <Route path={getRoutesForPage('schedule')} exact>
-              <Schedule query={query} basicDataLoaded={basicDataLoaded} />
-            </Route>
-            <Route path={getRoutesForPage('outgoing_webhooks')} exact>
-              <OutgoingWebhooks query={query} />
-            </Route>
-            <Route path={getRoutesForPage('maintenance')} exact>
-              <Maintenance />
-            </Route>
-            <Route path={getRoutesForPage('settings')} exact>
-              <SettingsPage />
-            </Route>
-            <Route path={getRoutesForPage('chat-ops')} exact>
-              <ChatOps query={query} />
-            </Route>
-            <Route path={getRoutesForPage('live-settings')} exact>
-              <LiveSettings />
-            </Route>
-            <Route path={getRoutesForPage('cloud')} exact>
-              <CloudPage />
-            </Route>
+          // Otherwise we'll run into concurrency issues
+          !basicDataLoaded ? (
+            <LoadingPlaceholder text="Loading..." />
+          ) : (
+            <Switch>
+              <Route path={getRoutesForPage('alert-groups')} exact>
+                <Incidents query={query} />
+              </Route>
+              <Route path={getRoutesForPage('alert-group')} exact>
+                <Incident query={query} />
+              </Route>
+              <Route path={getRoutesForPage('users')} exact>
+                <Users query={query} />
+              </Route>
+              <Route path={getRoutesForPage('integrations')} exact>
+                <Integrations query={query} />
+              </Route>
+              <Route path={getRoutesForPage('integration')} exact>
+                <Integration query={query} />
+              </Route>
+              <Route path={getRoutesForPage('escalations')} exact>
+                <EscalationChains query={query} />
+              </Route>
+              <Route path={getRoutesForPage('schedules')} exact>
+                <Schedules query={query} />
+              </Route>
+              <Route path={getRoutesForPage('schedule')} exact>
+                <Schedule query={query} />
+              </Route>
+              <Route path={getRoutesForPage('outgoing_webhooks')} exact>
+                <OutgoingWebhooks query={query} />
+              </Route>
+              <Route path={getRoutesForPage('maintenance')} exact>
+                <Maintenance />
+              </Route>
+              <Route path={getRoutesForPage('settings')} exact>
+                <SettingsPage />
+              </Route>
+              <Route path={getRoutesForPage('chat-ops')} exact>
+                <ChatOps query={query} />
+              </Route>
+              <Route path={getRoutesForPage('live-settings')} exact>
+                <LiveSettings />
+              </Route>
+              <Route path={getRoutesForPage('cloud')} exact>
+                <CloudPage />
+              </Route>

-            {/* Backwards compatibility redirect routes */}
-            <Route
-              path={getRoutesForPage('incident')}
-              exact
-              render={({ location }) => (
-                <Redirect
-                  to={{
-                    ...location,
-                    pathname: location.pathname.replace(/incident/, 'alert-group'),
-                  }}
-                ></Redirect>
-              )}
-            ></Route>
-            <Route
-              path={getRoutesForPage('incidents')}
-              exact
-              render={({ location }) => (
-                <Redirect
-                  to={{
-                    ...location,
-                    pathname: location.pathname.replace(/incidents/, 'alert-groups'),
-                  }}
-                ></Redirect>
-              )}
-            ></Route>
+              {/* Backwards compatibility redirect routes */}
+              <Route
+                path={getRoutesForPage('incident')}
+                exact
+                render={({ location }) => (
+                  <Redirect
+                    to={{
+                      ...location,
+                      pathname: location.pathname.replace(/incident/, 'alert-group'),
+                    }}
+                  ></Redirect>
+                )}
+              ></Route>
+              <Route
+                path={getRoutesForPage('incidents')}
+                exact
+                render={({ location }) => (
+                  <Redirect
+                    to={{
+                      ...location,
+                      pathname: location.pathname.replace(/incidents/, 'alert-groups'),
+                    }}
+                  ></Redirect>
+                )}
+              ></Route>

-            <Route path="*">
-              <NoMatch />
-            </Route>
-          </Switch>
+              <Route path="*">
+                <NoMatch />
+              </Route>
+            </Switch>
+          )
        ) : (
          <Unauthorized requiredUserAction={pagePermissionAction} />
        )}