Fix CSV import upsert #12048

ijreilly · 2025-05-14T15:58:48Z

Fixes #11864 and twentyhq/core-team-issues#908

We should not send createManyXXX mutations with FE-forged ids in the payload if we want to do an upsert, because that 1) prevents records from being merged 2) triggers optimistic rendering while we can't know before-hand which records will actually be created and which records will only be updated

Also noticed createdBy was being overriden even for records we are updating and not creating, which did not seem right, so fixed that too

greptile-apps

PR Summary

This PR addresses CSV import functionality issues with a focus on preventing duplicate records and proper handling of the 'createdBy' field. Here's a summary of the key changes:

Modified useCreateManyRecords to skip frontend-generated IDs during upsert operations to enable proper record merging
Added conditional optimistic updates in useCreateManyRecords to only trigger when not performing upserts
Introduced shouldNotEraseCreatedByField parameter in formatData.util.ts to preserve createdBy field during updates
Enhanced graphql-query-create-many-resolver.service.ts to properly handle unique field constraints during upserts
Improved handling of email-based record matching when importing without explicit record IDs

The changes look well-structured and directly address the reported issues with CSV imports creating duplicates instead of updating existing records.

_{3 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings | Greptile}

ijreilly · 2025-05-14T16:01:23Z

packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts

@@ -47,6 +48,15 @@ export function formatData<T>(
      objectMetadataItemWithFieldMaps.fieldsByName[key] ||
      fieldMetadataByJoinColumnName.get(key);

+    if (


may seem very custom but this still object-agnostic as we have it on all objects subject to user changes

ijreilly · 2025-05-14T16:08:18Z

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts

@@ -270,9 +270,12 @@ export class GraphqlQueryCreateManyResolverService extends GraphqlQueryBaseResol
      // For insert, formating is done in the server
      // While for update, formatting is done at the resolver level

+      const shouldNotEraseCreatedByField = true;


I did not add it to create-one as we don't have the "complex" merging logic that we have here for createMany, probably because this is useful for csv imports only which only uses createMany

if you use destructured parameters you won't need that 'hard coded' constant which seems rough at first sight

github-actions · 2025-05-14T16:10:25Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:9408

This environment will automatically shut down when the PR is closed or after 5 hours.

ijreilly · 2025-05-15T07:50:50Z

TODO: add a FE test if possible

martmull

Hey, I have some comments about the code organization, but the logic looks good to me.

martmull · 2025-05-15T07:52:30Z

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts

@@ -270,9 +270,12 @@ export class GraphqlQueryCreateManyResolverService extends GraphqlQueryBaseResol
      // For insert, formating is done in the server
      // While for update, formatting is done at the resolver level

+      const shouldNotEraseCreatedByField = true;
+
      const formattedRecord = formatData(


I think we need destructured parameters for more than 3 params

martmull · 2025-05-15T08:04:26Z

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts

@@ -270,9 +270,12 @@ export class GraphqlQueryCreateManyResolverService extends GraphqlQueryBaseResol
      // For insert, formating is done in the server
      // While for update, formatting is done at the resolver level

+      const shouldNotEraseCreatedByField = true;


if you use destructured parameters you won't need that 'hard coded' constant which seems rough at first sight

martmull · 2025-05-15T08:10:05Z

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts

      const formattedRecord = formatData(
        record,
        objectMetadataItemWithFieldMaps,
+        shouldNotEraseCreatedByField,


i don't feel that this parameter is very relevant for a formatData method. Maybe we should create a small utils removeNotErasableFields or something like that to do that before calling formatData (which in my opinion should remains agnostic of any specific field)

martmull · 2025-05-15T08:11:06Z

packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts

+    if (
+      fieldMetadata?.name === 'createdBy' &&
+      fieldMetadata.isCustom === false
+    ) {
+      if (shouldNotEraseCreatedByField) {
+        continue;
+      }
+    }


see comment above, i would keep formatData agnostic and remove the createdBy field from data in a dedicated utils before calling formatData

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts

martmull

seen together, lgtm

* Messaging issues (twentyhq#12041) messageImportException for messaging and calendar add more context to the Sentry Exceptions Fixes twentyhq#11994 * [Rest Api] Fix find duplicates endpoint (twentyhq#12044) - fix endpoint - migrate to new rest api v2 service - add integration test * Correct default fallback logo path in Logo component (twentyhq#12053) closes twentyhq#11849 The Logo component’s fallback URL was pointing to `/icons/android/android-launchericon-192-192.png`, but the asset lives under `/images/icons/....`. This updates defaultPrimaryLogoUrl to use the correct `/images/icons/android/android-launchericon-192-192.png` path, restoring the default logo display when no primaryLogo prop is provided. * Fix kanban loading bug (twentyhq#12042) Fixes twentyhq/core-team-issues#956 This PR fixes a bug that appeared when switching between two kanban views multiple times. Steps to reproduce : - Go to kanban view A - Go to kanban view B - Go back to kanban view A Video before : https://github.com/user-attachments/assets/4fa789ae-7187-498e-82b4-ee7896cd95d1 Video after : https://github.com/user-attachments/assets/2b323a2d-2f76-405d-9abd-38fe72ee2214 The problem was that we allowed a hook to take a nullable parameter that can be nullable between page switch, and threw when it was undefined. In order to be more cautious in the future, let's be sure that we don't throw for undefined props of a hook, when it is expected that those props can be in an undefined state for several React render loops, while fetching new data. Here I identified the parent effect : `<RecordIndexBoardDataLoader />` that loads all states for a board when we switch between views, for each column, by calling `<RecordIndexBoardColumnLoaderEffect />` in a `.map()`. Each `RecordIndexBoardColumnLoaderEffect` was calling `useLoadRecordIndexBoardColumn` with a potentially undefined `boardFieldMetadataId`. So to fix this, I cut the render flow higher than the throw in `useLoadRecordIndexBoardColumn`, and by doing that I was able to ensure that `recordIndexKanbanFieldMetadataItem` in `RecordIndexBoardDataLoader` was already defined when using it inside `useLoadRecordIndexBoardColumn`. `recordIndexKanbanFieldMetadataItem` was unnecessarily fetched two times, one time in `RecordIndexBoardDataLoader` and another time in its child `useLoadRecordIndexBoardColumn` hook. By implementing this flow-cut higher up, I could then remove the `| null` in TypeScript in children components, and expect a defined value in all the children of `RecordIndexBoardDataLoader`, thus removing the need to ask ourselves if we should throw or not in `useLoadRecordIndexBoardColumn`. * Validate existing fields on creation (twentyhq#12057) Fixes twentyhq#12040 When fields are deleted but still used in workflows we do not update create record action settings. It breaks all following workflow execution and the user cannot update the settings anymore. This PR fixes the bug by filtering on existing fields. Next step will be to clean settings on field deletion. Adding it to fast follows. Also lowering throttle limit because some infinite loops are not catched. * Exclude workflows from relation field object dropdown when inactive (twentyhq#12033) closes twentyhq#11996 - Switched the “Object destination” select in SettingsDataModelFieldRelationForm to use activeObjectMetadataItems.filter(...) so workflows, system, and remote objects are excluded - Updated useRelationSettingsFormInitialValues to fall back on the same filtered activeObjectMetadataItems list for its initial value This ensures workflows (and any unwanted system/remote objects) no longer show up in the dropdown or as the default. * Fixed error with previous filters on ACTOR with new sub-field filtering (twentyhq#12050) Fixes twentyhq/core-team-issues#969 that appeared since the new ACTOR sub-field filtering that changed the default sub-field filtering from name to source. Now we apply any existing filter value on an ACTOR field, whether for a sub-field or not, to the name sub-field by default. If the user wants to create a sub-field filter on source, he has to create a new advanced filter. Fixes twentyhq/core-team-issues#967 --------- Co-authored-by: Félix Malfait <[email protected]> * [reconnect account] Reseting calendar status and stage on reconnect (twentyhq#12061) ## Description When a calendar channel fails, its status is not reset during the reconnect step. In some cases, resetting is necessary—especially when we’ve deployed a fix and users try to reconnect their accounts after the patch. ## Why reseting to initial state Our processes are idempotent, so we can safely set the status to null to restart the flow. This approach covers all cases and avoids edge conditions caused by inconsistent statuses. Fixes : twentyhq#12026 * Make workflow custom fields editable (twentyhq#12063) Fixes twentyhq#11989 <img width="1267" alt="Capture d’écran 2025-05-15 à 14 05 15" src="https://github.com/user-attachments/assets/fbb22f52-2c76-424f-8b8c-fb030fef9fa8" /> `isCustom` was not properly set everywhere because it was not mandatory in `isFieldValueReadOnly`. Removing the default value and adding it to missing places * Fix CSV import upsert (twentyhq#12048) Fixes twentyhq#11864 and twentyhq/core-team-issues#908 We should not send `createManyXXX` mutations with FE-forged ids in the payload if we want to do an upsert, because that 1) prevents records from being merged 2) triggers optimistic rendering while we can't know before-hand which records will actually be created and which records will only be updated Also noticed createdBy was being overriden even for records we are updating and not creating, which did not seem right, so fixed that too * Gmail temporary error (twentyhq#12058) # Handling Google error This bug is hard to reproduce so the resolution is based only on the Sentry logs. There is not definite error code for this error message. in the documentation. https://developers.google.com/workspace/gmail/api/guides This is why i added more 5xx code types in order to be sure we catch this. In order to refine this later, i added the error code to the message. Fixes twentyhq#12025 * [QRQC_2] No explicit any in `twenty-server` (twentyhq#12068) # Introduction Added a no-explicit-any rule to the twenty-server, not applicable to tests and integration tests folder Related to twentyhq/core-team-issues#975 Discussed with Charles ## In case of conflicts Until this is approved I won't rebased and handle conflict, just need to drop two latest commits and re run the scripts etc ## Legacy We decided not to handle the existing lint error occurrences and programmatically ignored them through a disable next line rule comment ## Open question We might wanna activate the [no-explicit-any](https://typescript-eslint.io/rules/no-explicit-any/) `ignoreRestArgs` for our use case ? ``` ignoreRestArgs?: boolean; ``` --------- Co-authored-by: etiennejouan <[email protected]> * Fix missing exception catch (twentyhq#12069) add a check about uuid in rest api findOne handler --------- Co-authored-by: Guillim <[email protected]> Co-authored-by: martmull <[email protected]> Co-authored-by: nitin <[email protected]> Co-authored-by: Lucas Bordeau <[email protected]> Co-authored-by: Thomas Trompette <[email protected]> Co-authored-by: Félix Malfait <[email protected]> Co-authored-by: Marie <[email protected]> Co-authored-by: Paul Rastoin <[email protected]> Co-authored-by: etiennejouan <[email protected]>

Fixes twentyhq#11864 and twentyhq/core-team-issues#908 We should not send `createManyXXX` mutations with FE-forged ids in the payload if we want to do an upsert, because that 1) prevents records from being merged 2) triggers optimistic rendering while we can't know before-hand which records will actually be created and which records will only be updated Also noticed createdBy was being overriden even for records we are updating and not creating, which did not seem right, so fixed that too

Fix CSV import upsert

f92cf32

greptile-apps bot reviewed May 14, 2025

View reviewed changes

move code around

76b40b1

ijreilly commented May 14, 2025

View reviewed changes

FelixMalfait self-assigned this May 14, 2025

ijreilly commented May 14, 2025

View reviewed changes

FelixMalfait removed their assignment May 15, 2025

martmull requested changes May 15, 2025

View reviewed changes

ijreilly added 2 commits May 15, 2025 11:46

reorganize code to keep format-data agnostic

f0c9549

add test

191faff

martmull reviewed May 15, 2025

View reviewed changes

...ine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts Show resolved Hide resolved

martmull approved these changes May 15, 2025

View reviewed changes

ijreilly merged commit f782f4d into main May 15, 2025
52 checks passed

ijreilly deleted the fix-csv-import-duplicate branch May 15, 2025 12:44

ijreilly mentioned this pull request May 15, 2025

Don't perform optimistic rendering on CSV import since it's an upsert twentyhq/core-team-issues#908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix CSV import upsert #12048

Fix CSV import upsert #12048

Uh oh!

ijreilly commented May 14, 2025 •

edited

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

ijreilly May 14, 2025

Uh oh!

ijreilly May 14, 2025

Uh oh!

martmull May 15, 2025

Uh oh!

github-actions bot commented May 14, 2025 •

edited

Loading

Uh oh!

ijreilly commented May 15, 2025

Uh oh!

martmull left a comment

Uh oh!

martmull May 15, 2025

Uh oh!

martmull May 15, 2025

Uh oh!

martmull May 15, 2025

Uh oh!

martmull May 15, 2025

Uh oh!

Uh oh!

martmull left a comment

Uh oh!

Uh oh!

Uh oh!

Fix CSV import upsert #12048

Fix CSV import upsert #12048

Uh oh!

Conversation

ijreilly commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

ijreilly May 14, 2025

Choose a reason for hiding this comment

Uh oh!

ijreilly May 14, 2025

Choose a reason for hiding this comment

Uh oh!

martmull May 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijreilly commented May 15, 2025

Uh oh!

martmull left a comment

Choose a reason for hiding this comment

Uh oh!

martmull May 15, 2025

Choose a reason for hiding this comment

Uh oh!

martmull May 15, 2025

Choose a reason for hiding this comment

Uh oh!

martmull May 15, 2025

Choose a reason for hiding this comment

Uh oh!

martmull May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martmull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ijreilly commented May 14, 2025 •

edited

Loading

github-actions bot commented May 14, 2025 •

edited

Loading