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

Basic import for select in CSV #6047

Merged
merged 2 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { isNonEmptyString } from '@sniptt/guards';
import { IconComponent, useIcons } from 'twenty-ui';
import { useIcons } from 'twenty-ui';

import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { useCreateManyRecords } from '@/object-record/hooks/useCreateManyRecords';
import { getSpreadSheetValidation } from '@/object-record/spreadsheet-import/util/getSpreadSheetValidation';
import { useSpreadsheetImport } from '@/spreadsheet-import/hooks/useSpreadsheetImport';
import { SpreadsheetOptions, Validation } from '@/spreadsheet-import/types';
import { Field, SpreadsheetOptions } from '@/spreadsheet-import/types';
import { SnackBarVariant } from '@/ui/feedback/snack-bar-manager/components/SnackBar';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { FieldMetadataType } from '~/generated-metadata/graphql';
Expand All @@ -32,15 +32,7 @@ export const useSpreadsheetRecordImport = (objectNameSingular: string) => {
)
.sort((a, b) => a.name.localeCompare(b.name));

const templateFields: {
icon: IconComponent;
label: string;
key: string;
fieldType: {
type: 'input' | 'checkbox';
};
validations?: Validation[];
}[] = [];
const templateFields: Field<string>[] = [];
for (const field of fields) {
if (field.type === FieldMetadataType.FullName) {
templateFields.push({
Expand Down Expand Up @@ -80,6 +72,24 @@ export const useSpreadsheetRecordImport = (objectNameSingular: string) => {
field.label + ' (ID)',
),
});
} else if (field.type === FieldMetadataType.Select) {
templateFields.push({
icon: getIcon(field.icon),
label: field.label,
key: field.name,
fieldType: {
type: 'select',
options:
field.options?.map((option) => ({
label: option.label,
value: option.value,
})) || [],
},
validations: getSpreadSheetValidation(
field.type,
field.label + ' (ID)',
),
});
} else {
templateFields.push({
icon: getIcon(field.icon),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export const MatchColumnSelect = ({
<DropdownMenu
data-select-disable
ref={dropdownContainerRef}
width={refs.domReference.current?.clientWidth}
// width={refs.domReference.current?.clientWidth}
>
<DropdownMenuSearchInput
value={searchFilter}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ export const ValidationStep = <T extends string>({
(importColumn) =>
(importColumn.type === ColumnType.matched &&
importColumn.value === column.key) ||
(importColumn.type === ColumnType.matchedSelect &&
Copy link
Member

Choose a reason for hiding this comment

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

For more clarity? Wdyt? Not sure haha

      generateColumns(fields)
        .map((column) => {
          const isColumnMatched = (
            importColumn: Column<string>,
            columnKey: string,
          ): boolean => {
            const matchTypes = [
              ColumnType.matched,
              ColumnType.matchedSelect,
              ColumnType.matchedSelectOptions,
            ];

            return (
              matchTypes.includes(importColumn.type) &&
              importColumn.value === columnKey
            );
          };

          const hasBeenImported = importedColumns.some(
            (importColumn) =>
              isColumnMatched(importColumn, column.key) ||
              column.key === 'select-row',
          );

          if (!hasBeenImported && !showUnmatchedColumns) return null;
          return column;
        })

Copy link
Member Author

Choose a reason for hiding this comment

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

Much nicer 😍 thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-06-26 at 22 15 01

Not sure if I'm missing something but I feel like often when I try to do these kind of optimizations then Typescript gets lost

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-06-26 at 22 17 42 Initially I had tried this but it was the same

importColumn.value === column.key) ||
(importColumn.type === ColumnType.matchedSelectOptions &&
importColumn.value === column.key) ||
column.key === 'select-row',
).length > 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@ describe('setColumn', () => {
value: 'oldValue',
};

it('should return a matchedSelect column if field type is "select"', () => {
it('should return a matchedSelectOptions column if field type is "select"', () => {
const field = {
...defaultField,
fieldType: { type: 'select' },
fieldType: {
type: 'select',
options: [{ value: 'John' }, { value: 'Alice' }],
},
} as Field<'Name'>;

const data = [['John'], ['Alice']];
Expand All @@ -32,14 +35,16 @@ describe('setColumn', () => {
expect(result).toEqual({
index: 0,
header: 'Name',
type: ColumnType.matchedSelect,
type: ColumnType.matchedSelectOptions,
value: 'Name',
matchedOptions: [
{
entry: 'John',
value: 'John',
},
{
entry: 'Alice',
value: 'Alice',
},
],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
Column,
ColumnType,
MatchColumnsStepProps,
MatchedOptions,
} from '@/spreadsheet-import/steps/components/MatchColumnsStep/MatchColumnsStep';
import { Field } from '@/spreadsheet-import/types';

Expand All @@ -12,33 +13,56 @@ export const setColumn = <T extends string>(
field?: Field<T>,
data?: MatchColumnsStepProps<T>['data'],
): Column<T> => {
switch (field?.fieldType.type) {
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the switch case pattern here? If it's too big maybe we can split the code inside functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh I'm not sure why but the switch/case caused a wired typescript issue which I didn't figure out

case 'select':
return {
...oldColumn,
type: ColumnType.matchedSelect,
value: field.key,
matchedOptions: uniqueEntries(data || [], oldColumn.index),
};
case 'checkbox':
return {
index: oldColumn.index,
type: ColumnType.matchedCheckbox,
value: field.key,
header: oldColumn.header,
};
case 'input':
return {
index: oldColumn.index,
type: ColumnType.matched,
value: field.key,
header: oldColumn.header,
};
default:
return {
index: oldColumn.index,
header: oldColumn.header,
type: ColumnType.empty,
};
if (field?.fieldType.type === 'select') {
const fieldOptions = field.fieldType.options;
const uniqueData = uniqueEntries(
data || [],
oldColumn.index,
) as MatchedOptions<T>[];
const matchedOptions = uniqueData.map((record) => {
const value = fieldOptions.find(
(fieldOption) =>
fieldOption.value === record.entry ||
fieldOption.label === record.entry,
)?.value;
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

🧠 logic: Potential issue if fieldOptions is undefined. Ensure fieldOptions is always defined or handle the case when it's not.

return value
? ({ ...record, value } as MatchedOptions<T>)
: (record as MatchedOptions<T>);
});
const allMatched =
matchedOptions.filter((o) => o.value).length === uniqueData?.length;
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

🪶 style: Consider optimizing the filter and length comparison for better performance.


return {
...oldColumn,
type: allMatched
? ColumnType.matchedSelectOptions
: ColumnType.matchedSelect,
value: field.key,
matchedOptions,
};
}

if (field?.fieldType.type === 'checkbox') {
return {
index: oldColumn.index,
type: ColumnType.matchedCheckbox,
value: field.key,
header: oldColumn.header,
};
}

if (field?.fieldType.type === 'input') {
return {
index: oldColumn.index,
type: ColumnType.matched,
value: field.key,
header: oldColumn.header,
};
}

return {
index: oldColumn.index,
header: oldColumn.header,
type: ColumnType.empty,
};
};
Loading