Add mappable Unique ID field to company card CSV import - #99201
Conversation
Use a mapped Unique ID column as the transaction externalID instead of generating a fresh rand64() per row on every import, so the existing backend dedup in Transaction::isExternalDupe can match and re-uploads stop creating duplicates. Falls back to a generated ID when the column is unmapped or a row's cell is blank, leaving existing behavior unchanged. Also treats externalID as an advanced column when deciding whether a re-import from feed settings re-enables the advanced fields, so the mapping survives the re-upload it exists for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/es.ts b/src/languages/es.ts
index d986a169..5a3db2fe 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -5827,7 +5827,7 @@ ${amount} para ${merchant} - ${date}`,
comment: 'Comentario',
category: 'Categoría',
tag: 'Etiqueta',
- uniqueID: 'ID único',
+ uniqueID: 'ID única',
},
csvErrors: {
requiredColumns: (missingColumns: string) => `Por favor, asigna una columna a cada uno de los atributos: ${missingColumns}.`,
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 5eae494b..0d3d15d3 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -5861,7 +5861,7 @@ _詳しい手順については、[ヘルプサイトをご覧ください](${CO
comment: 'コメント',
category: 'カテゴリ',
tag: 'タグ',
- uniqueID: '一意のID',
+ uniqueID: '一意の ID',
},
csvErrors: {
requiredColumns: (missingColumns: string) => `各属性に列を割り当ててください：${missingColumns}`,
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index b01defd7..316e1235 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -5917,7 +5917,7 @@ _Para instruções mais detalhadas, [visite nossa central de ajuda](${CONST.NETS
comment: 'Comentário',
category: 'Categoria',
tag: 'Etiqueta',
- uniqueID: 'ID único',
+ uniqueID: 'ID exclusivo',
},
csvErrors: {
requiredColumns: (missingColumns: string) => `Atribua uma coluna a cada um dos atributos: ${missingColumns}.`,
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@marufsharifi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3d90b3ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| CONST.CSV_IMPORT_COLUMNS.COMMENT, | ||
| CONST.CSV_IMPORT_COLUMNS.CATEGORY, | ||
| CONST.CSV_IMPORT_COLUMNS.TAG, | ||
| CONST.CSV_IMPORT_COLUMNS.EXTERNAL_ID, |
There was a problem hiding this comment.
The import action appends externalID to the submitted mappings whenever the user does not select a Unique ID, so persisted layouts can contain this key even when advanced fields were disabled. Treating every stored externalID as advanced makes re-imports of those basic feeds enable advanced fields; worse, if a later spreadsheet has a real column at the formerly synthetic index, applyCompanyCardSavedColumnMappings automatically maps that unrelated column as the Unique ID, and repeated values can cause valid transactions to be deduplicated. Determine whether the stored mapping corresponds to an actual source column before using it to infer advanced mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and confirmed against real data rather than reasoning. The persisted layout for a feed imported on a dev VM is:
"uploadLayoutSettings":{"columnMappings":{"cardNumber":"0","postedDate":"1","merchant":"2","amount":"3","currency":"4","externalID":"5"},"instanceID":"1787332070659"}
That externalID: "5" came from a column the user really mapped, but a synthetic one is indistinguishable: the append puts it at index = column count, so a five-column basic feed persists externalID: "5" too. No client-side signal separates the two cases, so inferring anything from a stored externalID index is unsafe — including the auto-map, which could have pointed Unique ID at an unrelated column and let repeated values dedupe valid transactions.
Fixed in e771bca:
- Reverted
EXTERNAL_IDfromADVANCED_CSV_COLUMNS, so a basic feed no longer re-imports in advanced mode. applyCompanyCardSavedColumnMappingsnow skips savedexternalIDmappings outright, with a test that fails when the guard is removed.
The tradeoff is that the re-import-from-feed-settings path cannot restore a Unique ID mapping, so the user re-maps it. That is strictly better than silently mis-mapping the dedup key, and properly distinguishing a user-mapped externalID from the synthetic one needs a backend signal — part of the "add advanced fields to an existing feed" work the linked issue puts out of scope.
🤖 Posted by Claude (an AI agent) on behalf of the user.
| // Fills the synthetic column, and any row whose mapped Unique ID cell is blank. | ||
| if (!row.at(externalIDColumnIndex)?.trim()) { | ||
| row[externalIDColumnIndex] = transactionID; | ||
| } |
There was a problem hiding this comment.
When mapped IDs already exist during an exact or overlapping re-import, preserving them here lets the backend skip those rows, but every valid input row is still added to transactions and therefore included in transactionsCount. The success modal consequently says all input transactions “have been added” even when none, or only a subset, were inserted; use the backend's inserted count or avoid presenting the input-row count as the number added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that this is now inaccurate: before this PR dedup could never match, so every input row really was inserted and the count was right. With stable IDs a fully-deduped re-import will still report all rows as added.
Not fixing it here, for two reasons:
- The true inserted count only exists backend-side. The file is parsed asynchronously by the scraper well after
ImportCSVCompanyCardsreturns, and this modal is built client-side fromsuccessData, so there is nothing accurate to substitute yet — it needs a backend count plumbed through. - Rewording the modal so it does not claim a count (e.g. "Import started") is user-facing copy, which needs approval rather than a drive-by change in this PR.
Raised with the PR author to decide between softening the copy here or opening a follow-up for the backend count.
🤖 Posted by Claude (an AI agent) on behalf of the user.
A saved externalID index cannot be trusted to point at a real source column: the import appends a synthetic externalID column when the user maps no Unique ID, and that index is indistinguishable from a Unique ID mapped to the last column. Treating it as an advanced field made basic feeds re-import in advanced mode, and restoring the mapping could point Unique ID at an unrelated column of the next file, whose repeated values would make the backend dedupe valid transactions. Also applies the translation corrections for es, ja and pt-BR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@tgolen, could you please check this. it still seems to duplicate rows. RF.mp4 |
3 participants
Explanation of Change
Expensify Classic's domain company-card CSV upload lets you map a
Unique IDcolumn that acts as a dedup key, so re-uploading the same (or an overlapping) file skips rows you've already imported. New Expensify's company card CSV import had no such field, so every re-import created duplicate transactions.A saved
externalIDmapping is deliberately never restored on a later import. The import appends a syntheticexternalIDcolumn whenever the user maps noUnique ID, and the persisted index is then indistinguishable from aUnique IDmapped to the last column (both look like"externalID":"5"on a six-column layout), so restoring it could pointUnique IDat an unrelated column whose repeated values would make the backend dedupe valid transactions. Re-mapping it by hand is the safe behavior until the backend can distinguish the two.Fixed Issues
$ #99195
Tests
Note
For testing on a local environment, you might need to refer to this SO to get your uploads working
company-cards-unique-id-last-column.csv
Offline tests
This change does not alter offline behavior. The mapping screen renders from the locally parsed spreadsheet, and the import itself uses the existing
ImportCSVCompanyCardswrite, which queues like any other write while offline.QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari