Skip to content
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

☑️ Refacto "Select All/Unselect all" on indexes #5320

Merged
merged 49 commits into from
Jul 15, 2024
Merged

Conversation

gitstart-twenty
Copy link
Contributor

@gitstart-twenty gitstart-twenty commented May 7, 2024

  • Refacto "Select All/Unselect all" on indexes
  • Add sequential mass deletion from front end (limited to 10k records)
  • Fixed coverage with new unit tests on new useFetchAllRecordIds hook and other utils

Closes #4397
Closes #5169

Enregistrement.de.l.ecran.2024-07-09.a.17.58.47.mov

@gitstart-twenty
Copy link
Contributor Author

Hey @charlesBochet
What do you think of this v1?

@charlesBochet
Copy link
Member

@gitstart-twenty could you rebase your PR on main, tests should be green :)

@gitstart-twenty
Copy link
Contributor Author

@gitstart-twenty could you rebase your PR on main, tests should be green :)

Alright

gitstart-twenty and others added 2 commits May 8, 2024 11:33
Co-authored-by: v1b3m <[email protected]>
Co-authored-by: Toledodev <[email protected]>
Co-authored-by: v1b3m <[email protected]>
Co-authored-by: Toledodev <[email protected]>
@gitstart-twenty
Copy link
Contributor Author

gitstart-twenty commented May 8, 2024

Hey @charlesBochet,
We have duplicate code in useFindManyRecords and useLazyFindManyRecords, what do you think of this approach to making it DRY?

// packages/twenty-front/src/modules/object-record/hooks/useFindManyRecords.ts
import { useCallback, useMemo } from 'react';
import {
  ApolloQueryResult,
  FetchMoreQueryOptions,
  OperationVariables,
  useQuery,
  WatchQueryFetchPolicy,
} from '@apollo/client';
import { isNonEmptyArray } from '@apollo/client/utilities';
import { isNonEmptyString } from '@sniptt/guards';
import { useRecoilState, useRecoilValue, useSetRecoilState } from 'recoil';

import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { ObjectMetadataItemIdentifier } from '@/object-metadata/types/ObjectMetadataItemIdentifier';
import { getRecordsFromRecordConnection } from '@/object-record/cache/utils/getRecordsFromRecordConnection';
import { RecordGqlConnection } from '@/object-record/graphql/types/RecordGqlConnection';
import { RecordGqlEdge } from '@/object-record/graphql/types/RecordGqlEdge';
import { RecordGqlOperationFindManyResult } from '@/object-record/graphql/types/RecordGqlOperationFindManyResult';
import { RecordGqlOperationGqlRecordFields } from '@/object-record/graphql/types/RecordGqlOperationGqlRecordFields';
import { RecordGqlOperationVariables } from '@/object-record/graphql/types/RecordGqlOperationVariables';
import { useFindManyRecordsQuery } from '@/object-record/hooks/useFindManyRecordsQuery';
import { ObjectRecord } from '@/object-record/types/ObjectRecord';
import { filterUniqueRecordEdgesByCursor } from '@/object-record/utils/filterUniqueRecordEdgesByCursor';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { isDefined } from '~/utils/isDefined';
import { logError } from '~/utils/logError';
import { capitalize } from '~/utils/string/capitalize';

import { cursorFamilyState } from '../states/cursorFamilyState';
import { hasNextPageFamilyState } from '../states/hasNextPageFamilyState';
import { isFetchingMoreRecordsFamilyState } from '../states/isFetchingMoreRecordsFamilyState';

export type UseFindManyRecordsParams<T> = ObjectMetadataItemIdentifier &
  RecordGqlOperationVariables & {
    onCompleted?: (
      records: T[],
      options?: {
        pageInfo?: RecordGqlConnection['pageInfo'];
        totalCount?: number;
      },
    ) => void;
    skip?: boolean;
    recordGqlFields?: RecordGqlOperationGqlRecordFields;
    fetchPolicy?: WatchQueryFetchPolicy;
  };

type UseFindManyRecordsStateParams<
  T,
  TData = RecordGqlOperationFindManyResult,
> = Omit<
  UseFindManyRecordsParams<T>,
  'skip' | 'recordGqlFields' | 'fetchPolicy'
> & {
  data: RecordGqlOperationFindManyResult | undefined;
  fetchMore<
    TFetchData = TData,
    TFetchVars extends OperationVariables = OperationVariables,
  >(
    fetchMoreOptions: FetchMoreQueryOptions<TFetchVars, TFetchData> & {
      updateQuery?: (
        previousQueryResult: TData,
        options: {
          fetchMoreResult: TFetchData;
          variables: TFetchVars;
        },
      ) => TData;
    },
  ): Promise<ApolloQueryResult<TFetchData>>;
  objectMetadataItem: ObjectMetadataItem;
};

export const useFindManyRecordsState = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  data,
  fetchMore,
  objectMetadataItem,
}: UseFindManyRecordsStateParams<T>) => {
  const findManyQueryStateIdentifier =
    objectNameSingular +
    JSON.stringify(filter) +
    JSON.stringify(orderBy) +
    limit;

  const [lastCursor, setLastCursor] = useRecoilState(
    cursorFamilyState(findManyQueryStateIdentifier),
  );

  const [hasNextPage, setHasNextPage] = useRecoilState(
    hasNextPageFamilyState(findManyQueryStateIdentifier),
  );

  const setIsFetchingMoreObjects = useSetRecoilState(
    isFetchingMoreRecordsFamilyState(findManyQueryStateIdentifier),
  );

  const { enqueueSnackBar } = useSnackBar();

  const fetchMoreRecords = useCallback(async () => {
    if (hasNextPage) {
      setIsFetchingMoreObjects(true);

      try {
        await fetchMore({
          variables: {
            filter,
            orderBy,
            lastCursor: isNonEmptyString(lastCursor) ? lastCursor : undefined,
          },
          updateQuery: (prev, { fetchMoreResult }) => {
            const previousEdges = prev?.[objectMetadataItem.namePlural]?.edges;
            const nextEdges =
              fetchMoreResult?.[objectMetadataItem.namePlural]?.edges;

            let newEdges: RecordGqlEdge[] = [];

            if (isNonEmptyArray(previousEdges) && isNonEmptyArray(nextEdges)) {
              newEdges = filterUniqueRecordEdgesByCursor([
                ...(prev?.[objectMetadataItem.namePlural]?.edges ?? []),
                ...(fetchMoreResult?.[objectMetadataItem.namePlural]?.edges ??
                  []),
              ]);
            }

            const pageInfo =
              fetchMoreResult?.[objectMetadataItem.namePlural]?.pageInfo;

            if (isDefined(data?.[objectMetadataItem.namePlural])) {
              setLastCursor(pageInfo.endCursor ?? '');
              setHasNextPage(pageInfo.hasNextPage ?? false);
            }

            const records = getRecordsFromRecordConnection({
              recordConnection: {
                edges: newEdges,
                pageInfo,
              },
            }) as T[];

            onCompleted?.(records, {
              pageInfo,
              totalCount:
                fetchMoreResult?.[objectMetadataItem.namePlural]?.totalCount,
            });

            return Object.assign({}, prev, {
              [objectMetadataItem.namePlural]: {
                __typename: `${capitalize(
                  objectMetadataItem.nameSingular,
                )}Connection`,
                edges: newEdges,
                pageInfo:
                  fetchMoreResult?.[objectMetadataItem.namePlural].pageInfo,
                totalCount:
                  fetchMoreResult?.[objectMetadataItem.namePlural].totalCount,
              },
            } as RecordGqlOperationFindManyResult);
          },
        });
      } catch (error) {
        logError(
          `fetchMoreObjects for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during fetchMoreObjects for "${objectMetadataItem.namePlural}", ${error}`,
          {
            variant: 'error',
          },
        );
      } finally {
        setIsFetchingMoreObjects(false);
      }
    }
  }, [
    hasNextPage,
    setIsFetchingMoreObjects,
    fetchMore,
    filter,
    orderBy,
    lastCursor,
    objectMetadataItem.namePlural,
    objectMetadataItem.nameSingular,
    onCompleted,
    data,
    setLastCursor,
    setHasNextPage,
    enqueueSnackBar,
  ]);

  const totalCount = data?.[objectMetadataItem.namePlural].totalCount ?? 0;

  const records = useMemo(
    () =>
      data?.[objectMetadataItem.namePlural]
        ? getRecordsFromRecordConnection<T>({
            recordConnection: data?.[objectMetadataItem.namePlural],
          })
        : ([] as T[]),

    [data, objectMetadataItem.namePlural],
  );

  return {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  };
};

export const useFindManyRecords = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  skip,
  recordGqlFields,
  fetchPolicy,
}: UseFindManyRecordsParams<T>) => {
  const { enqueueSnackBar } = useSnackBar();
  const currentWorkspaceMember = useRecoilValue(currentWorkspaceMemberState);
  const { objectMetadataItem } = useObjectMetadataItem({
    objectNameSingular,
  });
  const { findManyRecordsQuery } = useFindManyRecordsQuery({
    objectNameSingular,
    recordGqlFields,
  });

  const { data, loading, error, fetchMore } =
    useQuery<RecordGqlOperationFindManyResult>(findManyRecordsQuery, {
      skip: skip || !objectMetadataItem || !currentWorkspaceMember,
      variables: {
        filter,
        limit,
        orderBy,
      },
      fetchPolicy: fetchPolicy,
      onCompleted: (data) => {
        if (!isDefined(data)) {
          onCompleted?.([]);
        }

        const pageInfo = data?.[objectMetadataItem.namePlural]?.pageInfo;

        const records = getRecordsFromRecordConnection({
          recordConnection: data?.[objectMetadataItem.namePlural],
        }) as T[];

        onCompleted?.(records, {
          pageInfo,
          totalCount: data?.[objectMetadataItem.namePlural]?.totalCount,
        });

        if (isDefined(data?.[objectMetadataItem.namePlural])) {
          setLastCursor(pageInfo.endCursor ?? '');
          setHasNextPage(pageInfo.hasNextPage ?? false);
        }
      },
      onError: (error) => {
        logError(
          `useFindManyRecords for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during useFindManyRecords for "${objectMetadataItem.namePlural}", ${error.message}`,
          {
            variant: 'error',
          },
        );
      },
    });

  const {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  } = useFindManyRecordsState<T>({
    objectNameSingular,
    filter,
    orderBy,
    limit,
    onCompleted,
    fetchMore,
    data,
    objectMetadataItem,
  });

  return {
    objectMetadataItem,
    records,
    totalCount,
    loading,
    error,
    fetchMoreRecords,
    queryStateIdentifier: findManyQueryStateIdentifier,
  };
};
// packages/twenty-front/src/modules/object-record/hooks/useLazyFindManyRecords.ts
import { useLazyQuery } from '@apollo/client';
import { useRecoilValue } from 'recoil';

import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { getRecordsFromRecordConnection } from '@/object-record/cache/utils/getRecordsFromRecordConnection';
import { RecordGqlOperationFindManyResult } from '@/object-record/graphql/types/RecordGqlOperationFindManyResult';
import {
  UseFindManyRecordsParams,
  useFindManyRecordsState,
} from '@/object-record/hooks/useFindManyRecords';
import { useFindManyRecordsQuery } from '@/object-record/hooks/useFindManyRecordsQuery';
import { ObjectRecord } from '@/object-record/types/ObjectRecord';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { isDefined } from '~/utils/isDefined';
import { logError } from '~/utils/logError';

type UseLazyFindManyRecordsParams<T> = Omit<
  UseFindManyRecordsParams<T>,
  'skip'
>;

export const useLazyFindManyRecords = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  recordGqlFields,
  fetchPolicy,
}: UseLazyFindManyRecordsParams<T>) => {
  const { objectMetadataItem } = useObjectMetadataItem({
    objectNameSingular,
  });

  const { findManyRecordsQuery } = useFindManyRecordsQuery({
    objectNameSingular,
    recordGqlFields,
  });

  const { enqueueSnackBar } = useSnackBar();
  const currentWorkspaceMember = useRecoilValue(currentWorkspaceMemberState);

  const [findManyRecords, { data, loading, error, fetchMore }] =
    useLazyQuery<RecordGqlOperationFindManyResult>(findManyRecordsQuery, {
      variables: {
        filter,
        limit,
        orderBy,
      },
      fetchPolicy: fetchPolicy,
      onCompleted: (data) => {
        if (!isDefined(data)) {
          onCompleted?.([]);
        }

        const pageInfo = data?.[objectMetadataItem.namePlural]?.pageInfo;

        const records = getRecordsFromRecordConnection({
          recordConnection: data?.[objectMetadataItem.namePlural],
        }) as T[];

        onCompleted?.(records, {
          pageInfo,
          totalCount: data?.[objectMetadataItem.namePlural]?.totalCount,
        });

        if (isDefined(data?.[objectMetadataItem.namePlural])) {
          setLastCursor(pageInfo.endCursor ?? '');
          setHasNextPage(pageInfo.hasNextPage ?? false);
        }
      },
      onError: (error) => {
        logError(
          `useFindManyRecords for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during useFindManyRecords for "${objectMetadataItem.namePlural}", ${error.message}`,
          {
            variant: 'error',
          },
        );
      },
    });

  const {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  } = useFindManyRecordsState<T>({
    objectNameSingular,
    filter,
    orderBy,
    limit,
    onCompleted,
    fetchMore,
    data,
    objectMetadataItem,
  });

  return {
    objectMetadataItem,
    records,
    totalCount,
    loading,
    error,
    fetchMoreRecords,
    queryStateIdentifier: findManyQueryStateIdentifier,
    findManyRecords: currentWorkspaceMember ? findManyRecords : () => {},
  };
};

@FelixMalfait
Copy link
Member

Hey, video looks great (nice improvement!) but the total count in the view switcher doesn't get refreshed apparently, we should fix that if that hasn't been fixed already.
(I didn't check the code)

@lucasbordeau
Copy link
Contributor

@gitstart-twenty Seems like a good DRY increment for me, this starts to be a big file and difficult to read but it would need a more in depth refactor.

@lucasbordeau
Copy link
Contributor

Though I would name it according to its context which is mainly about pagination. Maybe something like useFindManyRecordsPaginationUtils ?

@gitstart-twenty
Copy link
Contributor Author

gitstart-twenty commented May 14, 2024

Though I would name it according to its context which is mainly about pagination. Maybe something like useFindManyRecordsPaginationUtils ?

Hey @lucasbordeau
Thanks for the comments. Looking into them!

@gitstart-twenty
Copy link
Contributor Author

gitstart-twenty commented May 14, 2024

Hey, video looks great (nice improvement!) but the total count in the view switcher doesn't get refreshed apparently, we should fix that if that hasn't been fixed already. (I didn't check the code)

Hey @FelixMalfait
Thanks for the comment. Looking into this!

@gitstart-twenty
Copy link
Contributor Author

@FelixMalfait
We're attempting to refetch the records after the import(to get the updated total count) but we keep running into the error below

 + refetchQueries: [getOperationName(findManyRecordsQuery) ?? ''],

The error

 [
    InternalServerErrorException: GraphQL errors on person: {"message":"Unknown field \"personCollection\" on type Query"}
        at computePgGraphQLError (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/compute-pg-graphql-error.util.ts:52:10)
        at WorkspaceQueryRunnerService.parseResult (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:612:42)
        at WorkspaceQueryRunnerService.findMany (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:116:17)
        at processTicksAndRejections (node:internal/process/task_queues:95:5)
        at field.resolve (/.../node_modules/@envelop/on-resolve/cjs/index.js:36:42)
        at /.../node_modules/graphql-yoga/node_modules/@graphql-tools/executor/cjs/execution/promiseForObject.js:18:35
        at async Promise.all (index 0) {
      path: [ 'people' ],
      locations: [ [Object] ],
      extensions: [Object: null prototype] {}
    }
  ]

@FelixMalfait
Copy link
Member

Not sure sorry!

@gitstart-twenty
Copy link
Contributor Author

Not sure sorry!

Alright, debugging!

@gitstart-twenty
Copy link
Contributor Author

Not sure sorry!

Alright!

gitstart-twenty and others added 3 commits May 23, 2024 10:05
Co-authored-by: v1b3m <[email protected]>
Co-authored-by: Toledodev <[email protected]>
Co-authored-by: v1b3m <[email protected]>
Co-authored-by: Toledodev <[email protected]>
Co-authored-by: v1b3m <[email protected]>
Co-authored-by: Toledodev <[email protected]>
@gitstart-twenty
Copy link
Contributor Author

@FelixMalfait We're attempting to refetch the records after the import(to get the updated total count) but we keep running into the error below

 + refetchQueries: [getOperationName(findManyRecordsQuery) ?? ''],

The error

 [
    InternalServerErrorException: GraphQL errors on person: {"message":"Unknown field \"personCollection\" on type Query"}
        at computePgGraphQLError (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/compute-pg-graphql-error.util.ts:52:10)
        at WorkspaceQueryRunnerService.parseResult (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:612:42)
        at WorkspaceQueryRunnerService.findMany (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:116:17)
        at processTicksAndRejections (node:internal/process/task_queues:95:5)
        at field.resolve (/.../node_modules/@envelop/on-resolve/cjs/index.js:36:42)
        at /.../node_modules/graphql-yoga/node_modules/@graphql-tools/executor/cjs/execution/promiseForObject.js:18:35
        at async Promise.all (index 0) {
      path: [ 'people' ],
      locations: [ [Object] ],
      extensions: [Object: null prototype] {}
    }
  ]

Seems to work now!

@gitstart-twenty
Copy link
Contributor Author

Something(new changes) broke the deletion

@lucasbordeau
Copy link
Contributor

@gitstart-twenty Do you need something from our side to move forward ?

@lucasbordeau
Copy link
Contributor

@gitstart-twenty I'll take on from here as this PR is connected to multiple other refactors and another issue.

@charlesBochet charlesBochet mentioned this pull request Jul 11, 2024
@lucasbordeau lucasbordeau merged commit d560d25 into main Jul 15, 2024
13 checks passed
@lucasbordeau lucasbordeau deleted the TWNTY-4397 branch July 15, 2024 10:26
const firstQueryResult =
findManyRecordsDataResult?.data?.[objectMetadataItem.namePlural];

const totalCount = firstQueryResult?.totalCount ?? 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 1 as default and not 0?


let lastCursor = firstQueryResult?.pageInfo.endCursor ?? '';

for (let i = 0; i < remainingPages; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pageIndex rather than i


resetTableRowSelection();

for (const recordIdToDelete of recordIdsToDelete) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a big fan this is the frontend that has to think about deleting favorites

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the Backend for Mass Record Deletion ☑️ Enable "Select All" for delete action
6 participants