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

CSV importing and exporting fixes #8824

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eliasylonen
Copy link
Contributor

Fixes issue #5793 (and duplicate #8822)

  • Fix importing multi-select and array fields.
  • Fix exporting and importing RAW_JSON fields.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances CSV import/export functionality for multi-select and RAW_JSON fields in Twenty's object record system.

  • Added JSON string parsing for multi-select/array fields in buildRecordFromImportedStructuredRow.ts with fallback to empty array
  • Added RAW_JSON field handling in useExportProcessRecordsForCSV.ts to properly stringify JSON content for export
  • Added Zod schema validation for array/multi-select fields during import to ensure data integrity
  • Added proper error handling for malformed JSON input during import operations

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +37 to +39
default:
return processedRecord;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The default case drops the field value entirely by returning unmodified record. This breaks the export of multi-select fields, which was the main issue to fix.

Comment on lines +17 to +19
if (!isDefined(record[field.name])) {
return processedRecord;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Early return for undefined values means null values will be excluded from CSV export. Consider if this is the desired behavior.

Comment on lines +209 to +220
const stringArrayJSONSchema = z
.preprocess((value) => {
try {
if (typeof value !== 'string') {
return [];
}
return JSON.parse(value);
} catch {
return [];
}
}, z.array(z.string()))
.catch([]);
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 handling comma-separated values as fallback when JSON parsing fails, to support both JSON array format and simple comma-separated values

Comment on lines +227 to +233
if (typeof importedFieldValue === 'string') {
try {
recordToBuild[field.name] = JSON.parse(importedFieldValue);
} catch {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Should validate the parsed JSON structure matches expected schema before assigning to recordToBuild

@FelixMalfait FelixMalfait self-assigned this Dec 2, 2024
@FelixMalfait
Copy link
Member

Please ignore previous comment, I wasn't on the right branch 🤦‍♂️

@FelixMalfait
Copy link
Member

FelixMalfait commented Dec 2, 2024

Great! How are would it be to add a matching space, as we do for the Select field already? I think it would be best to have a consistent experience between the 2 field types

image

(Note array fields are different because they can take any target value so no mapping is needed)

@FelixMalfait
Copy link
Member

1 test to be fixed :)
Screenshot 2024-12-02 at 10 58 43

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.

3 participants